re-try r37025 with fix for incognito mode...

re-try r37025 with fix for incognito mode

----------------------------------------

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=37061

[Evan Stade, 2010-01-25 20:44:45]

added model_ check in ViewHierarchyChanged

[Finnur, 2010-01-25 20:52:44]

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

http://codereview.chromium.org/549151/diff/1/3#newcode525
chrome/browser/views/browser_actions_container.cc:525: if (is_add && child ==
this && model_) {
Can you do this instead:
if (is_add && child == this) {
  if (model_)  // |model_| can be NULL in incognito.
  ...

If prefer not to conflate (is_add && child) with model_ != NULL. These are not
related.

http://codereview.chromium.org/549151/diff/1/3#newcode526
chrome/browser/views/browser_actions_container.cc:526: // Initial toolbar button
creation + packing.
What does packing mean in this context?

[Evan Stade, 2010-01-25 20:55:50]

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

http://codereview.chromium.org/549151/diff/1/3#newcode525
chrome/browser/views/browser_actions_container.cc:525: if (is_add && child ==
this && model_) {
On 2010/01/25 20:52:44, Finnur wrote:
> Can you do this instead:
> if (is_add && child == this) {
>   if (model_)  // |model_| can be NULL in incognito.
>   ...
> 
> If prefer not to conflate (is_add && child) with model_ != NULL. These are not
> related.

Done.

http://codereview.chromium.org/549151/diff/1/3#newcode526
chrome/browser/views/browser_actions_container.cc:526: // Initial toolbar button
creation + packing.
On 2010/01/25 20:52:44, Finnur wrote:
> What does packing mean in this context?

placement in the parent container

[Finnur, 2010-01-25 20:57:59]

LGTM - one nit.

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

http://codereview.chromium.org/549151/diff/1/3#newcode526
chrome/browser/views/browser_actions_container.cc:526: // Initial toolbar button
creation + packing.
How about then:

// Initial toolbar button creation and placement in the parent container.

On 2010/01/25 20:55:50, Evan Stade wrote:
> On 2010/01/25 20:52:44, Finnur wrote:
> > What does packing mean in this context?
> 
> placement in the parent container