On Mon, Apr 27, 2009 at 10:00 AM, Erik Kay <[email protected]> wrote:
> On Wed, Apr 22, 2009 at 3:35 PM, Aaron Boodman <[email protected]> wrote:
>> Updating:
>> - We have bookmarks.setTitle(id, title) and windows.updateWindow({id,
>> top, left, width, height, ...})
>> - My vote. I like the single update method for objects that naturally
>> have lots of fields you can update. For bookmarks, for example, you
>> will eventually be able to update both url and title. It seems natural
>> to me to be able to do both at the same time. It also lends itself
>> well to batch operations, should we ever want to do that.
>
> I like how this approach feels, and I think we should do it. That said, I
> have a few concerns that I think we need to keep an eye on.
> What if there are some properties that are heavier weight and potentially
> have more specific notification and error conditions with you try to change
> them. The side effect of grouping changes to multiple fields with different
> update semantics is that you'll get a weird mishmash.
> For example, let's say you're updating three properties in tab, and two of
> them could actually fail according to the underlying browser API for some
> reason. This means that updates might have partially succeeded (I doubt we
> want to try to implement transactions), and you might have multiple
> independent error messages.
Right. My instinct was that we would have to pre-validate all the
changes. I don't think this is too hard for the cases I can imagine,
since we're essentially single threaded by the time we get to the
place we need to make the changes. But maybe there are some
difficulties I'm not considering.
>> I also
>> really like Rafael's idea to separate out the id parameter, so it
>> would look like windows.updateWindow(id, {top, left, width, height,
>> ...}). This elegantly solves the problem of accidentally using an old
>> object and clobbering more recent state. If we do this, probably we
>> should also split out id in the move apis, to be consistent.
>
> Would the API be: {id, window: {top, left, ...} }? Or would they be
> separate params as you have it above? I think I like the separate params in
> this case, but it does lead to some odd work when we support batching (you
> have to construct two separate arrays with matching indices).
I was thinking updateWindow(42, {top:5, left:10, ...}, function(){...});
I think having the nested arrays (tuples if you prefer) is better than
having a nested object for the common case.
>> Move Event:
>> - We have onBookmarkOrderChanged(int folderid, int[] newchildids) and
>> onTabMoved(int tabid, int windowid, int fromindex, int toindex)
>> - My vote: I think we should go with onTabMoved(int tabid, int
>> newindex). In other places we have assumed that the client is keeping
>> state, so this should be the same. We'd still send only one move event
>> and assume the client either understands the implied shuffling, or
>> will refetch the entire list.
>
> I agree with this in principle, but this is one of those places where we'll
> likely have to reflect the capabilities of the underlying API. For example,
> BookmarkObserver has two different notifications:
> BookmarkNodeMoved - this is following what you're proposing (it doesn't call
> for each node that was moved as a side effect, just the original)
> BookmarkNodeChildrenReordered - this is called if there was a bulk resort of
> the children of a node, once for the parent, not for each node
> So in this case, if we expect the client to be able to keep itself up to
> date, then we'll need both events.
In cases like these, I'm all for following the underlying support.We
just need to document it.
>> Question: Do we need to send fromIndex?
>
> 'need' is a strong word. Does it hurt to do so? I could imagine that it's
> useful for maintaining state to minimize the amount of re-shuffling you need
> to do. Incidentally, doesn't this notification also need a fromWindow?
Yeah, I'm just trying to figure out where to draw the line on how much
preexisting state to send. I don't have a strong opinion. As for
fromWindow, no. The underlying support distinguishes between moving
between windows and moving within a tab, and we decided that we had to
expose this.
- a
--~--~---------~--~----~------------~-------~--~----~
Chromium Developers mailing list: [email protected]
View archives, change email options, or unsubscribe:
http://groups.google.com/group/chromium-dev
-~----------~----~----~----~------~----~------~--~---
