more extensions bookmarks changes:...

more extensions bookmarks changes:* add schema verification* add unit tests for schema* add a few new methods (getTree, getChildren)* add events

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=14877

[Aaron Boodman, 2009-04-29 18:27:44]

http://codereview.chromium.org/102009/diff/1003/1007
File chrome/browser/extensions/extension_bookmarks_module.cc (right):

http://codereview.chromium.org/102009/diff/1003/1007#newcode21
Line 21: const wchar_t* kOldParentIdKey = L"parentId";
It looks like these keys are exactly the same as kIndexKey and kParentIdKey.
Should it be oldIndex and oldParentId?

http://codereview.chromium.org/102009/diff/1003/1007#newcode93
Line 93: // TODO(erikkay) temporary hack until adding an event listener can
notify the
Doesn't this mean that we won't start listening until the first bookmark method
is called? I wonder if there is some other hack we could do that wouldn't have
this problem.

http://codereview.chromium.org/102009/diff/1003/1007#newcode95
Line 95: ExtensionBookmarkEventRouter* eventRouter =
event_router

http://codereview.chromium.org/102009/diff/1003/1007#newcode114
Line 114: if (find(models_.begin(), models_.end(), model) == models_.end()) {
Use models_.find() instead?

http://codereview.chromium.org/102009/diff/1003/1007#newcode174
Line 174: object_args->SetInteger(kIndexKey, index);
I think for removed events, all that is absolutely required is the parent ID.

http://codereview.chromium.org/102009/diff/1003/1007#newcode186
Line 186: object_args->SetString(kTitleKey, node->GetTitle());
Do we know what property changed? The signature we talked about for change
events is:

event onChange(int id, Object props)

Where props are the name/value pairs that changed. If we don't know what
properties changed, we should at least match this signature and have a TODO to
only send the changes.

http://codereview.chromium.org/102009/diff/1003/1007#newcode203
Line 203: DictionaryValue* object_args = new DictionaryValue();
I was thinking we'd have separate parameters here:

onChildrenReordered(int id, int[] new_children_ids)

One general thing is that we seem to not have a rule as to when to use named vs
normal params.

http://codereview.chromium.org/102009/diff/1003/1007#newcode270
Line 270: EXTENSION_FUNCTION_VALIDATE(args_->IsType(Value::TYPE_INTEGER));
This one isn't necessary as the check on GetAsInteger() does the same thing.

http://codereview.chromium.org/102009/diff/1003/1007#newcode280
Line 280: int childCount = node->GetChildCount();
child_count

http://codereview.chromium.org/102009/diff/1003/1005
File chrome/browser/extensions/extension_bookmarks_module.h (right):

http://codereview.chromium.org/102009/diff/1003/1005#newcode12
Line 12: class ExtensionBookmarkEventRouter : public BookmarkModelObserver {
This class needs comment sprucing. The class and all its new methods need
comments, but I don't think the methods inherited from BoookmarkModelObserver
need comments. What I have seen other code do is just:

// BookmarkModelObserver

above all the methods inherited through that class. I think that makes sense.

http://codereview.chromium.org/102009/diff/1003/1005#newcode61
Line 61: private:
Add a blank line before private:

http://codereview.chromium.org/102009/diff/1003/1005#newcode69
Line 69: // Note: we don't own these pointers.
Maybe add a sentence to this comment describing why we hold this state.

http://codereview.chromium.org/102009/diff/1003/1008
File chrome/renderer/extensions/extension_api_client_unittest.cc (right):

http://codereview.chromium.org/102009/diff/1003/1008#newcode185
Line 185: TEST_F(ExtensionAPIClientTest, CreateBookmark) {
Nice tests :)

http://codereview.chromium.org/102009/diff/1003/1009
File chrome/renderer/resources/extension_process_bindings.js (right):

http://codereview.chromium.org/102009/diff/1003/1009#newcode57
Line 57: //message += "  From caller " + validate.caller;
Remove dead code?

http://codereview.chromium.org/102009/diff/1003/1009#newcode255
Line 255: //minItems: 0,
Remove dead code?

http://codereview.chromium.org/102009/diff/1003/1009#newcode271
Line 271: chromium.bookmarks.getTree = function(callback) {
Do you think it would be useful to let getTree take an optional starting point
as an argument?

http://codereview.chromium.org/102009/diff/1003/1009#newcode371
Line 371: // sends ({id, childrenIds)
Missing closing brace.

[Erik does not do reviews, 2009-04-29 19:08:32]

http://codereview.chromium.org/102009/diff/1003/1007
File chrome/browser/extensions/extension_bookmarks_module.cc (right):

http://codereview.chromium.org/102009/diff/1003/1007#newcode21
Line 21: const wchar_t* kOldParentIdKey = L"parentId";
On 2009/04/29 18:27:44, Aaron Boodman wrote:
> It looks like these keys are exactly the same as kIndexKey and kParentIdKey.
> Should it be oldIndex and oldParentId?

oops.  good catch.

http://codereview.chromium.org/102009/diff/1003/1007#newcode93
Line 93: // TODO(erikkay) temporary hack until adding an event listener can
notify the
On 2009/04/29 18:27:44, Aaron Boodman wrote:
> Doesn't this mean that we won't start listening until the first bookmark
method
> is called? I wonder if there is some other hack we could do that wouldn't have
> this problem.

yes, it does.  The other hacks I thought of were uglier.  The reason I'm OK with
this is that it seems unlikely that you can do anything sensible with bookmark
notifications until you've actually asked for the bookmarks.  It'll go away once
we plumb the event listener registration to the browser.

http://codereview.chromium.org/102009/diff/1003/1007#newcode95
Line 95: ExtensionBookmarkEventRouter* eventRouter =
On 2009/04/29 18:27:44, Aaron Boodman wrote:
> event_router

ugh.  sucks switching back and forth between C++ and JS styles.

http://codereview.chromium.org/102009/diff/1003/1007#newcode114
Line 114: if (find(models_.begin(), models_.end(), model) == models_.end()) {
On 2009/04/29 18:27:44, Aaron Boodman wrote:
> Use models_.find() instead?

as far as I can tell vector doesn't have a find method on it

http://codereview.chromium.org/102009/diff/1003/1007#newcode174
Line 174: object_args->SetInteger(kIndexKey, index);
On 2009/04/29 18:27:44, Aaron Boodman wrote:
> I think for removed events, all that is absolutely required is the parent ID.

Not really.  If you don't include the index, then you require the client to make
a new round trip to find out which child got removed.

http://codereview.chromium.org/102009/diff/1003/1007#newcode186
Line 186: object_args->SetString(kTitleKey, node->GetTitle());
On 2009/04/29 18:27:44, Aaron Boodman wrote:
> Do we know what property changed? 

The model does, but doesn't expose it in the API.  It turns out that there are
only two properties that it notifies about: title and favicon (which we don't
expose), so we could assume title for now.  Otherwise, we'll need to add more
info to the BookmarkModelObserver API.  I'll just send title for now.

> The signature we talked about for change
> events is:
> 
> event onChange(int id, Object props)
> 
> Where props are the name/value pairs that changed. 
If we don't know what
> properties changed, we should at least match this signature and have a TODO to
> only send the changes.

I changed the signature and added a TODO for a more general solution.

http://codereview.chromium.org/102009/diff/1003/1007#newcode203
Line 203: DictionaryValue* object_args = new DictionaryValue();
On 2009/04/29 18:27:44, Aaron Boodman wrote:
> I was thinking we'd have separate parameters here:
> 
> onChildrenReordered(int id, int[] new_children_ids)

sure.


> One general thing is that we seem to not have a rule as to when to use named
vs
> normal params.

It would be good to formalize this.  Right now, it feels fairly arbitrary. 
Maybe it's that events never use named params (not counting objects in the
params).

http://codereview.chromium.org/102009/diff/1003/1007#newcode270
Line 270: EXTENSION_FUNCTION_VALIDATE(args_->IsType(Value::TYPE_INTEGER));
On 2009/04/29 18:27:44, Aaron Boodman wrote:
> This one isn't necessary as the check on GetAsInteger() does the same thing.

fixed

http://codereview.chromium.org/102009/diff/1003/1007#newcode280
Line 280: int childCount = node->GetChildCount();
On 2009/04/29 18:27:44, Aaron Boodman wrote:
> child_count

fixed

http://codereview.chromium.org/102009/diff/1003/1005
File chrome/browser/extensions/extension_bookmarks_module.h (right):

http://codereview.chromium.org/102009/diff/1003/1005#newcode61
Line 61: private:
On 2009/04/29 18:27:44, Aaron Boodman wrote:
> Add a blank line before private:

done

http://codereview.chromium.org/102009/diff/1003/1005#newcode69
Line 69: // Note: we don't own these pointers.
On 2009/04/29 18:27:44, Aaron Boodman wrote:
> Maybe add a sentence to this comment describing why we hold this state.

done

http://codereview.chromium.org/102009/diff/1003/1008
File chrome/renderer/extensions/extension_api_client_unittest.cc (right):

http://codereview.chromium.org/102009/diff/1003/1008#newcode185
Line 185: TEST_F(ExtensionAPIClientTest, CreateBookmark) {
On 2009/04/29 18:27:44, Aaron Boodman wrote:
> Nice tests :)

Good to have some, but I added a TODO for a few more.  I basically did the
minimum to prove to myself that verify kind of worked. ;-)

http://codereview.chromium.org/102009/diff/1003/1009
File chrome/renderer/resources/extension_process_bindings.js (right):

http://codereview.chromium.org/102009/diff/1003/1009#newcode57
Line 57: //message += "  From caller " + validate.caller;
On 2009/04/29 18:27:44, Aaron Boodman wrote:
> Remove dead code?

done

http://codereview.chromium.org/102009/diff/1003/1009#newcode255
Line 255: //minItems: 0,
On 2009/04/29 18:27:44, Aaron Boodman wrote:
> Remove dead code?

done

http://codereview.chromium.org/102009/diff/1003/1009#newcode271
Line 271: chromium.bookmarks.getTree = function(callback) {
On 2009/04/29 18:27:44, Aaron Boodman wrote:
> Do you think it would be useful to let getTree take an optional starting point
> as an argument?

it probably doesn't hurt.  I'll add a TODO.

http://codereview.chromium.org/102009/diff/1003/1009#newcode371
Line 371: // sends ({id, childrenIds)
On 2009/04/29 18:27:44, Aaron Boodman wrote:
> Missing closing brace.

fixed

[Aaron Boodman, 2009-04-29 20:24:07]

LGTM!