Windows: use ExtensionToolbarModel for ordering of browser action buttons....

Views: use ExtensionToolbarModel for ordering of browser action buttons.

This doesn't implement drag and drop reording, but the order is stored on shutdown and restored on startup.

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

[Finnur, 2010-01-23 03:45:08]

Good initiative. Bad timing. You need to update these files. This patch is
already out of date after my checkin earlier today.  Sorry. :)

Rafael, I would suggest you hold off reviewing until the next update.

http://codereview.chromium.org/554057/diff/1/3
File chrome/browser/views/browser_actions_container.cc (right):

http://codereview.chromium.org/554057/diff/1/3#newcode254
chrome/browser/views/browser_actions_container.cc:254:
SetID(VIEW_ID_BROWSER_ACTION_TOOLBAR);
nit: Line break after this line.

http://codereview.chromium.org/554057/diff/1/3#newcode272
chrome/browser/views/browser_actions_container.cc:272: }
AddBrowserAction doesn't exist in this function anymore... See
ViewHierarchyChanged in the new file...

http://codereview.chromium.org/554057/diff/1/3#newcode525
chrome/browser/views/browser_actions_container.cc:525: }
These functions are based on an out of date version of this file. You need to
update...

http://codereview.chromium.org/554057/diff/1/2
File chrome/browser/views/browser_actions_container.h (right):

http://codereview.chromium.org/554057/diff/1/2#newcode1
chrome/browser/views/browser_actions_container.h:1: // Copyright (c) 2010 The
Chromium Authors. All rights reserved.
Dude. You have the 2009 version of this file. I like changed that... like... a
few hours ago! :)

But seriously, I change these two files a lot today. You really should update
before continuing...

[Evan Stade, 2010-01-23 04:03:20]

merged

questions:
1. do we need to do onbrowseractionvisibilitychanged() at the end of
browseractionremoved()?
2. why was the browser action area minimum size the first time I ran chrome
after the merge? is that expected (or expected for now) or did I break
something?

[Finnur, 2010-01-23 04:20:33]

1. It's been a long day and a long week, so I might be slightly off, but I don't
see function at the end of BrowserActionRemoved (not in my code and not in
yours). Am I missing something?

2. I suspect a bug in my code. We should be calling ClampToNearestIconCount(-1)
in the constructor if the pref is not found, instead of using the return value
blindly. I can create a patch if you don't get to it first.

On 2010/01/23 04:03:20, Evan Stade wrote:
> merged
> 
> questions:
> 1. do we need to do onbrowseractionvisibilitychanged() at the end of
> browseractionremoved()?
> 2. why was the browser action area minimum size the first time I ran chrome
> after the merge? is that expected (or expected for now) or did I break
> something?

[Finnur, 2010-01-23 04:21:23]

Ooops, that was supposed to read:

I don't see _that_ function at the end of BrowserActionRemoved

[Evan Stade, 2010-01-25 18:08:43]

On 2010/01/23 04:20:33, Finnur wrote:
> 1. It's been a long day and a long week, so I might be slightly off, but I
don't
> see function at the end of BrowserActionRemoved (not in my code and not in
> yours). Am I missing something?

right, it's not there, but seems like it might should be (in particular because
it was formerly called after RemoveBrowserAction in  Observe())

> 2. I suspect a bug in my code. We should be calling
ClampToNearestIconCount(-1)
> in the constructor if the pref is not found, instead of using the return value
> blindly. I can create a patch if you don't get to it first.

I'll let you do it since you seem to know the right fix
 
> On 2010/01/23 04:03:20, Evan Stade wrote:
> > merged
> > 
> > questions:
> > 1. do we need to do onbrowseractionvisibilitychanged() at the end of
> > browseractionremoved()?
> > 2. why was the browser action area minimum size the first time I ran chrome
> > after the merge? is that expected (or expected for now) or did I break
> > something?

[Finnur, 2010-01-25 18:22:38]

First to answer your question:

> 1. do we need to do onbrowseractionvisibilitychanged()
> at the end of browseractionremoved()?

No, I don't think so. The animation should take care of updating the visibility
it correctly.

http://codereview.chromium.org/554057/diff/6003/8002
File chrome/browser/views/browser_actions_container.cc (right):

http://codereview.chromium.org/554057/diff/6003/8002#newcode309
chrome/browser/views/browser_actions_container.cc:309:
SetID(VIEW_ID_BROWSER_ACTION_TOOLBAR);
This is no longer needed.

http://codereview.chromium.org/554057/diff/6003/8002#newcode530
chrome/browser/views/browser_actions_container.cc:530: // calls Layout on the
Toolbar, which needs this object to be constructed.
No period at the end here, the sentence continues on the next line.

http://codereview.chromium.org/554057/diff/6003/8002#newcode686
chrome/browser/views/browser_actions_container.cc:686: void
BrowserActionsContainer::BrowserActionRemoved(Extension* extension) {
The code before you changed it checked to make sure that:
a) the extension has a browser action
b) if there was a popup showing, it would close.

Are you sure this is no longer needed?

Part a) of that question also applies to BrowserActionAdded.

[Evan Stade, 2010-01-25 18:36:17]

http://codereview.chromium.org/554057/diff/6003/8002
File chrome/browser/views/browser_actions_container.cc (right):

http://codereview.chromium.org/554057/diff/6003/8002#newcode309
chrome/browser/views/browser_actions_container.cc:309:
SetID(VIEW_ID_BROWSER_ACTION_TOOLBAR);
On 2010/01/25 18:22:38, Finnur wrote:
> This is no longer needed.

Done.

http://codereview.chromium.org/554057/diff/6003/8002#newcode530
chrome/browser/views/browser_actions_container.cc:530: // calls Layout on the
Toolbar, which needs this object to be constructed.
On 2010/01/25 18:22:38, Finnur wrote:
> No period at the end here, the sentence continues on the next line.

Done.

http://codereview.chromium.org/554057/diff/6003/8002#newcode686
chrome/browser/views/browser_actions_container.cc:686: void
BrowserActionsContainer::BrowserActionRemoved(Extension* extension) {
On 2010/01/25 18:22:38, Finnur wrote:
> The code before you changed it checked to make sure that:
> a) the extension has a browser action
> b) if there was a popup showing, it would close.
> 
> Are you sure this is no longer needed?
> 
> Part a) of that question also applies to BrowserActionAdded.

yup, the model makes those checks

[Evan Stade, 2010-01-25 18:37:35]

oh sorry, b) is still necessary

[Finnur, 2010-01-25 18:46:36]

> yup, the model makes those checks

Only for adding, not for removing...

On 2010/01/25 18:36:17, Evan Stade wrote:
> http://codereview.chromium.org/554057/diff/6003/8002
> File chrome/browser/views/browser_actions_container.cc (right):
> 
> http://codereview.chromium.org/554057/diff/6003/8002#newcode309
> chrome/browser/views/browser_actions_container.cc:309:
> SetID(VIEW_ID_BROWSER_ACTION_TOOLBAR);
> On 2010/01/25 18:22:38, Finnur wrote:
> > This is no longer needed.
> 
> Done.
> 
> http://codereview.chromium.org/554057/diff/6003/8002#newcode530
> chrome/browser/views/browser_actions_container.cc:530: // calls Layout on the
> Toolbar, which needs this object to be constructed.
> On 2010/01/25 18:22:38, Finnur wrote:
> > No period at the end here, the sentence continues on the next line.
> 
> Done.
> 
> http://codereview.chromium.org/554057/diff/6003/8002#newcode686
> chrome/browser/views/browser_actions_container.cc:686: void
> BrowserActionsContainer::BrowserActionRemoved(Extension* extension) {
> On 2010/01/25 18:22:38, Finnur wrote:
> > The code before you changed it checked to make sure that:
> > a) the extension has a browser action
> > b) if there was a popup showing, it would close.
> > 
> > Are you sure this is no longer needed?
> > 
> > Part a) of that question also applies to BrowserActionAdded.
> 
> yup, the model makes those checks

[Evan Stade, 2010-01-25 18:56:03]

On 2010/01/25 18:46:36, Finnur wrote:
> > yup, the model makes those checks
> 
> Only for adding, not for removing...

well, indirectly it does check when removing. |toolitems_| only contains the
extensions with browser actions.

[Finnur, 2010-01-25 19:01:30]

LGTM