[chromium-reviews] Fix bug: browser action button disappears when loading a new extension. This ...

Fix bug: browser action button disappears when loading a new extension. This was happenening because BrowserActionContainer, on receiving notification about an extension load/unload/change, deletes all BrowserAction views, re-adds all the views again, and asks the toolbar to SetBounds for the BrowserActionContainer. However, during deletion, it does not re-set the bounds. As a result, when SetBounds is called by the toolbar again, it does not see any changed in BrowserActionContainer, and does not layout again, causing all the browser actions to disappear. The solution lies in forcing  Layout() after SetBounds() in ToolbarView.

BUG=23593
TEST=none

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

[Erik does not do reviews, 2009-10-14 23:14:51]

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

http://codereview.chromium.org/276033/diff/1/2#newcode435
Line 435: SetBounds(0, 0, 0, 0);
rather than this, why not call PreferredSizeChanged?

[sidchat (Google), 2009-10-14 23:46:35]

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

http://codereview.chromium.org/276033/diff/1/2#newcode435
Line 435: SetBounds(0, 0, 0, 0);
That is not working, because PreferredSizeChanged causes ToolbarView, its
parent, to re-layout BrowserActionsContainer, by using SetBound. Unfortunately,
SetBound does an early return because it does not find any difference between
the existing bounds and the new bounds - hence, Layout never happens.

An alternate to this is to call on browser_actions_->Layout() explicitly after
browser_actions_->SetBounds() in ToolbarView::Layout() so that this never
happens.

I have now used the latter approach, as it seems it will solve all possible
occurance of this bug.

On 2009/10/14 23:14:51, Erik Kay wrote:
> rather than this, why not call PreferredSizeChanged?

[sidchat (Google), 2009-10-14 23:47:02]

Moved the change - please have another look.

-Sid

On 2009/10/14 23:46:35, sidchat (Google) wrote:
> http://codereview.chromium.org/276033/diff/1/2
> File chrome/browser/views/browser_actions_container.cc (right):
> 
> http://codereview.chromium.org/276033/diff/1/2#newcode435
> Line 435: SetBounds(0, 0, 0, 0);
> That is not working, because PreferredSizeChanged causes ToolbarView, its
> parent, to re-layout BrowserActionsContainer, by using SetBound.
Unfortunately,
> SetBound does an early return because it does not find any difference between
> the existing bounds and the new bounds - hence, Layout never happens.
> 
> An alternate to this is to call on browser_actions_->Layout() explicitly after
> browser_actions_->SetBounds() in ToolbarView::Layout() so that this never
> happens.
> 
> I have now used the latter approach, as it seems it will solve all possible
> occurance of this bug.
> 
> On 2009/10/14 23:14:51, Erik Kay wrote:
> > rather than this, why not call PreferredSizeChanged?

[Erik does not do reviews, 2009-10-15 00:10:52]

LGTM - sigh

This all feels very fragile to me.  I think this all stems from
RefreshBrowserActionViews which removes and re-adds everything whenever any
extension is loaded, regardless of whether it has a page action.  That's
probably the real thing that should be fixed (handling load/unload/disable
individually).  Maybe add a TODO there as well?

http://codereview.chromium.org/276033/diff/3001/5
File chrome/browser/views/toolbar_view.cc (right):

http://codereview.chromium.org/276033/diff/3001/5#newcode555
Line 555: browser_actions_->Layout();
Add a short comment explaining why this is necessary.

[sidchat (Google), 2009-10-15 00:34:55]

Right - that seems to be the stem of this problem. I will add comments and a
TODO.

-Sid

On 2009/10/15 00:10:52, Erik Kay wrote:
> LGTM - sigh
> 
> This all feels very fragile to me.  I think this all stems from
> RefreshBrowserActionViews which removes and re-adds everything whenever any
> extension is loaded, regardless of whether it has a page action.  That's
> probably the real thing that should be fixed (handling load/unload/disable
> individually).  Maybe add a TODO there as well?
> 
> http://codereview.chromium.org/276033/diff/3001/5
> File chrome/browser/views/toolbar_view.cc (right):
> 
> http://codereview.chromium.org/276033/diff/3001/5#newcode555
> Line 555: browser_actions_->Layout();
> Add a short comment explaining why this is necessary.