[chromium-reviews] Make BrowserView layout both extension shelf and bookmarks bar if toolstrips ...

Make BrowserView layout both extension shelf and bookmarks bar if toolstrips change in the extension shelf.

BUG=21269
BUG=21270
BUG=21274
TEST=none

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

[sidchat (Google), 2009-09-08 18:45:07]

When toolstrips change/expand, it calls on the ExtensionShelf in
ChildPreferredSizeChanged(). For --show-extensions-on-top, ExtensionShelf, in
turn, is a child of BrowserView. So, BrowserView has to know to layout both
ExtensionShelf and BookmarkBar when toolstrips are changed. A simple
PreferredSizeChanged() solved the problem. So now, the toolstrips can expand
(try the Tree extensions). Also, the initial size seems to be fine.

[Erik does not do reviews, 2009-09-08 21:33:08]

http://codereview.chromium.org/192041/diff/1/2
File chrome/browser/views/extensions/extension_shelf.cc (right):

http://codereview.chromium.org/192041/diff/1/2#newcode708
Line 708: PreferredSizeChanged();
Does this result in ExtensionShelf::Layout() being called, even in the bottom
bar implementation?

http://codereview.chromium.org/192041/diff/1/2#newcode789
Line 789: PreferredSizeChanged();
this is unnecessary - if the toolstrip changed size, it will trigger a
ChildPreferredSizeChanged.  If it didn't, then this isn't necessary either.

[sidchat (Google), 2009-09-09 00:10:10]

Made changes as per you comments - please have another look.

-Sid

http://codereview.chromium.org/192041/diff/1/2
File chrome/browser/views/extensions/extension_shelf.cc (right):

http://codereview.chromium.org/192041/diff/1/2#newcode708
Line 708: PreferredSizeChanged();
Yes, it does that, which indeed is redundant. Changed code accordingly.

On 2009/09/08 21:33:08, Erik Kay wrote:
> Does this result in ExtensionShelf::Layout() being called, even in the bottom
> bar implementation?

http://codereview.chromium.org/192041/diff/1/2#newcode789
Line 789: PreferredSizeChanged();
On 2009/09/08 21:33:08, Erik Kay wrote:
> this is unnecessary - if the toolstrip changed size, it will trigger a
> ChildPreferredSizeChanged.  If it didn't, then this isn't necessary either.

Done.

[Erik does not do reviews, 2009-09-09 00:15:56]

http://codereview.chromium.org/192041/diff/1002/1003
File chrome/browser/views/extensions/extension_shelf.cc (right):

http://codereview.chromium.org/192041/diff/1002/1003#newcode713
Line 713: if (CommandLine::ForCurrentProcess()->HasSwitch(
can't you just make BrowserView always call Layout() on
ChildPreferredSizeChanged, top or bottom?  Then you don't need to do this
command-line switch hack.

[Aaron Boodman, 2009-09-09 01:37:46]

Going to leave this one to erikkay.

[Erik does not do reviews, 2009-09-09 18:16:22]

On Wed, Sep 9, 2009 at 10:31 AM, <sidchat@chromium.org> wrote:
>
> http://codereview.chromium.org/192041/diff/1002/1003
> File chrome/browser/views/extensions/extension_shelf.cc (right):
>
> http://codereview.chromium.org/192041/diff/1002/1003#newcode713
> Line 713: if (CommandLine::ForCurrentProcess()->HasSwitch(
> Yes, that was what I was doing in the first place. I had replaced
> layout() by PreferredSizeChanged() - so each time
> ChildPreferredSizeChanged() is called on the ExtensionShelf, it asks its
> BrowserView, through the PreferredSizeChanged(), to lay it out,
> irrespective of whether it was a top shelf or a bottom shelf.
>
> Is that what you were asking? If so, I will move back to my first code
> change.

If you look back to my first comment, I simply asked if that's how it
behaved.  I was concerned that with your change that Layout() would no
longer be called in the bottom bar version (which would break things).
 I didn't ask you to change anything.  When you changed it, I assumed
that you did so because it didn't work in the bottom shelf case and
you were trying to fix it.

If the original code worked for both top and bottom bar, then yes,
please go back to your original code.

Erik



> On 2009/09/09 00:15:56, Erik Kay wrote:
>>
>> can't you just make BrowserView always call Layout() on
>> ChildPreferredSizeChanged, top or bottom? =A0Then you don't need to do
>
> this
>>
>> command-line switch hack.
>
> http://codereview.chromium.org/192041
>

[sidchat (Google), 2009-09-09 18:21:53]

Ah right - sorry for the confusion. Yes, to your previous question, original
code works for both top and bottom bar.

Made the appropriate change. Please have another look.

-Sid

On 2009/09/09 18:16:22, Erik Kay wrote:
> On Wed, Sep 9, 2009 at 10:31 AM, <mailto:sidchat@chromium.org> wrote:
> >
> > http://codereview.chromium.org/192041/diff/1002/1003
> > File chrome/browser/views/extensions/extension_shelf.cc (right):
> >
> > http://codereview.chromium.org/192041/diff/1002/1003#newcode713
> > Line 713: if (CommandLine::ForCurrentProcess()->HasSwitch(
> > Yes, that was what I was doing in the first place. I had replaced
> > layout() by PreferredSizeChanged() - so each time
> > ChildPreferredSizeChanged() is called on the ExtensionShelf, it asks its
> > BrowserView, through the PreferredSizeChanged(), to lay it out,
> > irrespective of whether it was a top shelf or a bottom shelf.
> >
> > Is that what you were asking? If so, I will move back to my first code
> > change.
> 
> If you look back to my first comment, I simply asked if that's how it
> behaved.  I was concerned that with your change that Layout() would no
> longer be called in the bottom bar version (which would break things).
>  I didn't ask you to change anything.  When you changed it, I assumed
> that you did so because it didn't work in the bottom shelf case and
> you were trying to fix it.
> 
> If the original code worked for both top and bottom bar, then yes,
> please go back to your original code.
> 
> Erik
> 
> 
> 
> > On 2009/09/09 00:15:56, Erik Kay wrote:
> >>
> >> can't you just make BrowserView always call Layout() on
> >> ChildPreferredSizeChanged, top or bottom? =A0Then you don't need to do
> >
> > this
> >>
> >> command-line switch hack.
> >
> > http://codereview.chromium.org/192041
> >

[Erik does not do reviews, 2009-09-09 18:25:25]

LGTM