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 12:16 PM, Nick Carter <ncar...@google.com> 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 <nickb...@chromium.org> wrote:
>
>>
>>
>> On Thu, Apr 9, 2009 at 11:22 AM, Aaron Boodman <a...@chromium.org> wrote:
>>
>>> On Thu, Apr 9, 2009 at 10:28 AM, Erik Kay <erik...@google.com> wrote:
>>> > On Wed, Apr 8, 2009 at 11:52 PM, Aaron Boodman <a...@chromium.org>
>>> 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: chromium-dev@googlegroups.com
View archives, change email options, or unsubscribe:
http://groups.google.com/group/chromium-dev
-~----------~----~----~----~------~----~------~--~---
