On Thu, Apr 9, 2009 at 12:17 PM, Nick Carter <[email protected]> wrote:
> Does the "other bookmarks" node need special handling like the Bookmarks > Bar? Since the title of this permanent node is an internationalized string, > there needs to be another way to identify it. Note that in our existing > BookmarkModel, the other bookmarks node is not a child of the bookmark node, > but a sibling -- though this does not mean we can't aspire to expose it in > some other way. > the isBookmarksBar boolean should handle this. If it's false, then it must be "Other Bookmarks". > > Using a (parentid,index) tuple to communicate position changes in an > asynchronous interface leads to funkiness, because an "move X to index 0" > operation requires sending bookmarkUpdated notifications for every > subsequent item in the sibling ordering whose index gets incremented. If > single-item moves are going to result in O(#siblings) updates anyway, we > might prefer to just communicate positions as a list IDs or items, bound to > the parent. In other words, to allow the |children| array to be the one and > only way to read and write position, and drop the redundant reflection onto > a (parentid, index) tuple. > > - nick > > On Thu, Apr 9, 2009 at 12:16 PM, Nick Carter <[email protected]> wrote: > >> Does the "other bookmarks" node need special handling like the Bookmarks >> Bar? Since the title of this permanent node is an internationalized string, >> there needs to be another way to identify it. Note that in our existing >> BookmarkModel, the other bookmarks node is not a child of the bookmark node, >> but a sibling -- though this does not mean we can't aspire to expose it in >> some other way. >> >> Using a (parentid,index) tuple to communicate position changes in an >> asynchronous interface leads to funkiness, because an "move X to index 0" >> operation requires sending bookmarkUpdated notifications for every >> subsequent item in the sibling ordering whose index gets incremented. If >> single-item moves are going to result in O(#siblings) updates anyway, we >> might prefer to just communicate positions as a list IDs or items, bound to >> the parent. In other words, to allow the |children| array to be the one and >> only way to read and write position, and drop the redundant reflection onto >> a (parentid, index) tuple. >> >> - nick >> >> On Thu, Apr 9, 2009 at 11:55 AM, Nick Baum <[email protected]> wrote: >> >>> >>> >>> On Thu, Apr 9, 2009 at 11:22 AM, Aaron Boodman <[email protected]> wrote: >>> >>>> On Thu, Apr 9, 2009 at 10:28 AM, Erik Kay <[email protected]> wrote: >>>> > On Wed, Apr 8, 2009 at 11:52 PM, Aaron Boodman <[email protected]> >>>> wrote: >>>> >>> Should changes to the contents of a folder trigger >>>> eventbookmarkupdated for >>>> >>> that folder? How about the folders above it? >>>> >> >>>> >> This is a meta question for the entire system. My current thinking is >>>> >> that changes you make should not fire events back at you. That just >>>> >> seems annoying. I think there should also be defined behavior in >>>> cases >>>> >> where something you do will obviously affect other nodes, and that >>>> >> those changes shouldn't be fired at you either. >>>> > >>>> > Agree in principle. The callback within the CRUD method should handle >>>> > this. I'd argue that all of the methods should have callbacks for >>>> > this reason (as well as error handling). >>>> >>>> All the methods do currently have a callback ... and they all should >>>> have an errback too. But I'm arguing that for "expected" >>>> reorganizations of related nodes, we shouldn't send back information >>>> about what happened. I'm not really sure how this will work in real >>>> life, I need to play with it. It's a first stake in the ground. >>> >>> >>> So if a bookmark gets added to a folder: >>> >>> - we should send an update event for the bookmark >>> - but we should NOT send an update event for that folder >>> >>> Is that right? >>> >>>> >>>> >>>> >> For example, moving a bookmark item within the folder obviously >>>> >> affects some of the other nodes. We should have a defined behavior >>>> for >>>> >> what happens when you do this, and we shouldn't fire events back at >>>> >> you for any of those changes. >>>> > >>>> > This is a bit tricky though since we need to distinguish between the >>>> > calling extension and other extensions. Let's say you had two >>>> > extensions that hooked into bookmarks API (one was an alternate UI and >>>> > one was a backend sync tool). >>>> >>>> We have that state in the current implementation, we can make this >>>> distinction. >>>> >>>> >> Other stuff >>>> >> * I camel cased the events (in all the APIs) :-) >>>> > >>>> > It looks like you missed history. I just updated it. >>>> >>>> Thanks. >>>> >>>> >> * I added letters to the left of all the properties in each struct of >>>> >> each API. There is 'r', 'w', and 'c', which refer to read, write, and >>>> >> create. This determines whether it is valid to include this property >>>> >> in each of these scenarios. So for example, it doesn't make sense to >>>> >> create a bookmark with a predefined API. There will be other >>>> >> validation we have to do on top of this. Like it doesn't make sense >>>> to >>>> >> change a bookmark to a folder, but this handles a good percentage. >>>> > >>>> > For createDownload, don't more of the Download fields need create >>>> > rights (path, url, etc.)? >>>> >>>> Oh yeah, url, duh. I fixed that. I'm not sure about path though. To >>>> me, the download system is about getting stuff onto the system. Moving >>>> things around in the file system is a different (more complicated) >>>> API. >>> >>> >>> I agree. >>> >>>> >>>> >>>> >>> What kind of events does clearAllDownloads trigger? do we need a >>>> separate >>>> >>> event for this? Do we even need this in the first place? >>>> >> >>>> >> See previous point about events. I'm <meh> about the need for >>>> >> clearAllDownloads(). Take it or leave it. >>>> > >>>> > I think we need it for privacy folks. We generally need the "clear" >>>> > APIs for all of our data so that someone can implement the extensions >>>> > that periodically delete everything. >>>> >>>> My point is that you can implement clearAll by fetching all ids and >>>> calling remove in a loop. I guess having a clearAll is slightly more >>>> convenient, but ... meh :) >>> >>> >>> The history API also has a clear function, that takes a query as input. >>> This seems very useful (clear a day, a particular url, etc). >>> We could generalize this to bookmarks too, but then we move away from >>> simple CRUD. I say we just leave it for history now and remove it from >>> downloads. >>> >>>> >>>> >>>> >> Other stuff: >>>> >> * I added two booleans: showInShelf and showInDownloadUI. >>>> > >>>> > What's the distinction between the two? Is the second supposed to be >>>> > download history? >>>> >>>> Yes, download history. Idea is that somebody might want to download >>>> something in their extension and open it for the user, but it really >>>> isn't a "download" in the sense of the things that show up in that UI. >>>> >>>> >> I modified the signature of the getHistory callback to make it more >>>> >> consistent with the other APIs. Instead of returning the number of >>>> >> pages, I think it is more useful to return the total number of items. >>>> >> Also I don't think we need to return the page number since the caller >>>> >> already has that. >>>> > >>>> > It looks like this change didn't get committed (or got clobbered by >>>> someone). >>>> >>>> Whoops, I forgot to press save. >>>> >>>> I also removed "timeSpent" per brett's feedback. I didn't do anything >>>> about brett's other feedback because I need to get more clarification >>>> (it appears that chrome's UI currently does page-based results, even >>>> for text queries). >>>> >>>> - a >>>> >>> >>> >>> >>> >>> >> > --~--~---------~--~----~------------~-------~--~----~ Chromium Developers mailing list: [email protected] View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~----------~----~----~----~------~----~------~--~---
