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 -~----------~----~----~----~------~----~------~--~---
