Extensions: create a simple model for the browser action buttons toolstrip an...

Extensions: create a simple model for the browser action buttons toolstrip and use it on Linux.

TODO: persist the order of buttons
TODO: use the model on windows, mac
TODO: let the view change the order of buttons

My plan for persisting the order is just to write/read a simple text file to/from disk, one extension id per line. Similar to the bookmark model, except that is structured data (because they try to hold more information).

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

[Evan Stade, 2009-12-04 21:08:45]

ping

[Erik does not do reviews, 2009-12-05 15:30:37]

Sorry for the delay.  Expect us to be slow with reviews for the next week or so.

http://codereview.chromium.org/462026/diff/1/8
File chrome/browser/extensions/extension_toolbar_model.cc (right):

http://codereview.chromium.org/462026/diff/1/8#newcode49
chrome/browser/extensions/extension_toolbar_model.cc:49: 
EXTENSION_LOADED can be delivered before EXTENSIONS_READY, so you need to check
service_->is_ready() before continuing here.

http://codereview.chromium.org/462026/diff/1/7
File chrome/browser/extensions/extension_toolbar_model.h (right):

http://codereview.chromium.org/462026/diff/1/7#newcode24
chrome/browser/extensions/extension_toolbar_model.h:24: typedef
std::list<Extension*> ExtensionToolbarList;
There's already a similar typedef in extension.h called ExtensionList (a vector,
not a list).  I think it's always just used for iteration at the moment, so if
you think it's better off as a list to support faster reordering, then I'd just
go ahead and change that typedef.

http://codereview.chromium.org/462026/diff/1/7#newcode61
chrome/browser/extensions/extension_toolbar_model.h:61: class
ExtensionToolbarModelObserver {
In general, I prefer these helper classes to be nested
(ExtensionToolbarModel::Observer), but I don't feel that strongly about it.  All
classes need a comment though.

http://codereview.chromium.org/462026/diff/1/7#newcode65
chrome/browser/extensions/extension_toolbar_model.h:65: virtual void
ExtensionAdded(Extension* extension, int index) {}
I think these need to have a more specific name. (e.g.
ExtensionBrowserActionAdded)

[Evan Stade, 2009-12-07 21:15:40]

http://codereview.chromium.org/462026/diff/1/8
File chrome/browser/extensions/extension_toolbar_model.cc (right):

http://codereview.chromium.org/462026/diff/1/8#newcode49
chrome/browser/extensions/extension_toolbar_model.cc:49: 
On 2009/12/05 15:30:37, Erik Kay wrote:
> EXTENSION_LOADED can be delivered before EXTENSIONS_READY, so you need to
check
> service_->is_ready() before continuing here.

Done.

http://codereview.chromium.org/462026/diff/1/7
File chrome/browser/extensions/extension_toolbar_model.h (right):

http://codereview.chromium.org/462026/diff/1/7#newcode24
chrome/browser/extensions/extension_toolbar_model.h:24: typedef
std::list<Extension*> ExtensionToolbarList;
On 2009/12/05 15:30:37, Erik Kay wrote:
> There's already a similar typedef in extension.h called ExtensionList (a
vector,
> not a list).  I think it's always just used for iteration at the moment, so if
> you think it's better off as a list to support faster reordering, then I'd
just
> go ahead and change that typedef.

Done (left it as a vector; the performance difference is inconsequential here)

http://codereview.chromium.org/462026/diff/1/7#newcode61
chrome/browser/extensions/extension_toolbar_model.h:61: class
ExtensionToolbarModelObserver {
On 2009/12/05 15:30:37, Erik Kay wrote:
> In general, I prefer these helper classes to be nested
> (ExtensionToolbarModel::Observer), but I don't feel that strongly about it. 
All
> classes need a comment though.

me too, done.

http://codereview.chromium.org/462026/diff/1/7#newcode65
chrome/browser/extensions/extension_toolbar_model.h:65: virtual void
ExtensionAdded(Extension* extension, int index) {}
On 2009/12/05 15:30:37, Erik Kay wrote:
> I think these need to have a more specific name. (e.g.
> ExtensionBrowserActionAdded)

Done.

[Erik does not do reviews, 2009-12-07 22:16:06]

LGTM

[Aaron Boodman, 2009-12-07 22:58:51]

Note: I recommend storing the order in the prefs. We already have an extension
node, it would be convenient to just put this state there. That way a lot of
loading/saving machinery is also taken care of for you.

[Evan Stade, 2009-12-07 23:18:19]

ok, thanks for the heads up

-- Evan Stade


On Mon, Dec 7, 2009 at 2:58 PM, <aa@chromium.org> wrote:

> Note: I recommend storing the order in the prefs. We already have an
> extension
> node, it would be convenient to just put this state there. That way a lot
> of
> loading/saving machinery is also taken care of for you.
>
>
> http://codereview.chromium.org/462026
>