Test + Fix for GTK window resizing when using toolkit views.

Add in a browser test for dialog resizing, which was catching an issue with the way we were using GTK in toolkit views.  Possibly related to 27365 or 38785.

http://code.google.com/p/chromium-os/issues/detail?id=4126

BUG=chromium-os:4126
TEST=HtmlDialogBrowserTest.SizeWindow

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

[Mattias Nissler (ping if slow), 2010-07-01 04:34:07]

Can you please double-check the bug number?

[Scott Byer, 2010-07-01 16:17:32]

On 2010/07/01 04:34:07, mnissler wrote:
> Can you please double-check the bug number?

Whoops, it's a Chromium OS bug number.  Fixed.  THe unit test captures what is
needed to fix that bug.

[Paweł Hajdan Jr., 2010-07-01 16:21:32]

Drive-by with some test comments. Please let me take another look before
committing.

http://codereview.chromium.org/2768006/diff/12001/13001
File chrome/browser/views/html_dialog_view_uitest.cc (right):

http://codereview.chromium.org/2768006/diff/12001/13001#newcode58
chrome/browser/views/html_dialog_view_uitest.cc:58: class HtmlDialogUITest :
public InProcessBrowserTest {
nit: Please make the file name consistent with the contents. Browser tests
should be named _browsertest, not _uitest.

http://codereview.chromium.org/2768006/diff/12001/13001#newcode71
chrome/browser/views/html_dialog_view_uitest.cc:71: // Mac builds yet.
Is there some TODO or bug for this issue?

http://codereview.chromium.org/2768006/diff/12001/13001#newcode88
chrome/browser/views/html_dialog_view_uitest.cc:88: const int kLoopTimeMS = 250;
Why hardcode a timeout? Are you waiting for some event to happen? Would it be
sufficient to just use ui_test_utils::RunAllPendingInMessageLoop?

[Mattias Nissler (ping if slow), 2010-07-02 07:29:34]

http://codereview.chromium.org/2768006/diff/12001/13003
File views/widget/widget_gtk.cc (right):

http://codereview.chromium.org/2768006/diff/12001/13003#newcode651
views/widget/widget_gtk.cc:651: gtk_widget_set_size_request(widget_,
bounds.width(), bounds.height());
I disucssed this exact call with sky in
http://codereview.chromium.org/2812026/show

The bottom line of that discussion was basically to not call
gtk_widget_set_size_request() here, but rather fix up the code that sets up the
preferred size for the window in the first place. It's important to note that
the preferred size in the context of GtkWindow is not really a preferred size,
but a minimum size instead.

[Scott Byer, 2010-08-06 21:51:33]

(With travel in the middle, took me a bit to get back and dig deep into this).

Here's my current variant of the fix.  

I found one other possibility and that's to revert the change to
native_view_host_gtk.cc made as part of http://codereview.chromium.org/2131021
where gtk_widget_size_allocate was replaced with gtk_widget_set_size_request.  I
believe that the flashing from that original message could be because the layout
done in NativeViewHost::Layout() doesn't propagate the parent's new size (I
could imaging setting up a preferred size on the NativeViewHost, and Layout()
would use that size if set instead of it's current size) - and because the size
doesn't propagate, the resizing loop bounces around between a few sizes a bunch
of times and that could have caused the flickering.  I suspect that correcting
the layout in that way, getting the size right the first time, the bouncing and
flickering wouldn't happen.  But that's a much more intricate change with much
larger implications and goes into some core views system files, and I'm not
experienced enough with the views code to say if that would really be a more
correct fix.

This fix definitely targets the containing page requesting a new size code path
- by sizing the HtmlDialogView (which is a NativeViewHost) first, it's prepping
NativeViewHost::Layout() to get the right answer when it gets the RootView size
changed notification.

And Paweł, thank you for the drive-by.  I dove into why the timed event loop
worked when ui_test_utils::RunAllPendingInMessageLoop didn't, and figured out
that it's the message pump getting into the delayed messages that was the key to
getting the window server information synchronized and valid for testing.  Since
you've been over the testing code more, you can tell me if I really should
refactor this into another ui_test_utils routine - I haven't really seen the
need, but haven't read through a lot of browser test code yet.


I can dive into seeing if the larger fix makes sense if anyone really wants, but
I'd like to do that as a separate issue, since getting a fix in for cloud print
launch is important (and I really don't know for sure I can get a fix going down
that path without some regression elsewhere).  I'd also like to tackle getting
the test working on Windows as a separate issue, since I suspect that there may
be different window sizing synchronization issues there.

-Scott


On 2010/07/02 07:29:34, mnissler wrote:
> http://codereview.chromium.org/2768006/diff/12001/13003
> File views/widget/widget_gtk.cc (right):
> 
> http://codereview.chromium.org/2768006/diff/12001/13003#newcode651
> views/widget/widget_gtk.cc:651: gtk_widget_set_size_request(widget_,
> bounds.width(), bounds.height());
> I disucssed this exact call with sky in
> http://codereview.chromium.org/2812026/show
> 
> The bottom line of that discussion was basically to not call
> gtk_widget_set_size_request() here, but rather fix up the code that sets up
the
> preferred size for the window in the first place. It's important to note that
> the preferred size in the context of GtkWindow is not really a preferred size,
> but a minimum size instead.

[Paweł Hajdan Jr., 2010-08-06 21:56:28]

Drive-by with a test comment. Do not commit without my "LGTM".

http://codereview.chromium.org/2768006/diff/22001/23002
File chrome/browser/views/html_dialog_view_browsertest.cc (right):

http://codereview.chromium.org/2768006/diff/22001/23002#newcode69
chrome/browser/views/html_dialog_view_browsertest.cc:69: void
RunAllPendingAndDelayed() {
This is just wrong. Do you need to wait for an event instead?

[Scott Byer, 2010-08-06 22:13:15]

http://codereview.chromium.org/2768006/diff/22001/23002
File chrome/browser/views/html_dialog_view_browsertest.cc (right):

http://codereview.chromium.org/2768006/diff/22001/23002#newcode69
chrome/browser/views/html_dialog_view_browsertest.cc:69: void
RunAllPendingAndDelayed() {
On 2010/08/06 21:56:28, Paweł Hajdan Jr. wrote:
> This is just wrong. Do you need to wait for an event instead?

I'll dig a little bit more, but no relevant notification is currently happening.
 Hints about where to look would be appreciated.

[sky, 2010-08-06 23:02:15]

http://codereview.chromium.org/2768006/diff/22001/23001
File chrome/browser/views/html_dialog_view.cc (right):

http://codereview.chromium.org/2768006/diff/22001/23001#newcode158
chrome/browser/views/html_dialog_view.cc:158: GetWidget()->SetBounds(pos);
Changing the bounds of the widget should force a layout and the bounds of the
htmldialogview to change. Adding a SetBounds here shouldn't be necessary. The
bug is likely else where.

When the bounds of the widget changes WidgetGtk::OSizeAllocate should get
invoked. The should result in the HTMLDialogView getting resized and
NativeViewHost::Layout getting invoked, which should resize the TabContents
contained in the DOMView.

[sky, 2010-08-06 23:07:34]

Is TabContentsViewGtk::SizeContents getting invoked, which resets the
requested size?

  -Scott

On Fri, Aug 6, 2010 at 2:51 PM,  <scottbyer@chromium.org> wrote:
> (With travel in the middle, took me a bit to get back and dig deep into
> this).
>
> Here's my current variant of the fix.
>
> I found one other possibility and that's to revert the change to
> native_view_host_gtk.cc made as part of
> http://codereview.chromium.org/2131021
> where gtk_widget_size_allocate was replaced with
> gtk_widget_set_size_request.  I
> believe that the flashing from that original message could be because the
> layout
> done in NativeViewHost::Layout() doesn't propagate the parent's new size (I
> could imaging setting up a preferred size on the NativeViewHost, and
> Layout()
> would use that size if set instead of it's current size) - and because the
> size
> doesn't propagate, the resizing loop bounces around between a few sizes a
> bunch
> of times and that could have caused the flickering.  I suspect that
> correcting
> the layout in that way, getting the size right the first time, the bouncing
> and
> flickering wouldn't happen.  But that's a much more intricate change with
> much
> larger implications and goes into some core views system files, and I'm not
> experienced enough with the views code to say if that would really be a more
> correct fix.
>
> This fix definitely targets the containing page requesting a new size code
> path
> - by sizing the HtmlDialogView (which is a NativeViewHost) first, it's
> prepping
> NativeViewHost::Layout() to get the right answer when it gets the RootView
> size
> changed notification.
>
> And Paweł, thank you for the drive-by.  I dove into why the timed event loop
> worked when ui_test_utils::RunAllPendingInMessageLoop didn't, and figured
> out
> that it's the message pump getting into the delayed messages that was the
> key to
> getting the window server information synchronized and valid for testing.
>  Since
> you've been over the testing code more, you can tell me if I really should
> refactor this into another ui_test_utils routine - I haven't really seen the
> need, but haven't read through a lot of browser test code yet.
>
>
> I can dive into seeing if the larger fix makes sense if anyone really wants,
> but
> I'd like to do that as a separate issue, since getting a fix in for cloud
> print
> launch is important (and I really don't know for sure I can get a fix going
> down
> that path without some regression elsewhere).  I'd also like to tackle
> getting
> the test working on Windows as a separate issue, since I suspect that there
> may
> be different window sizing synchronization issues there.
>
> -Scott
>
>
> On 2010/07/02 07:29:34, mnissler wrote:
>>
>> http://codereview.chromium.org/2768006/diff/12001/13003
>> File views/widget/widget_gtk.cc (right):
>
>> http://codereview.chromium.org/2768006/diff/12001/13003#newcode651
>> views/widget/widget_gtk.cc:651: gtk_widget_set_size_request(widget_,
>> bounds.width(), bounds.height());
>> I disucssed this exact call with sky in
>> http://codereview.chromium.org/2812026/show
>
>> The bottom line of that discussion was basically to not call
>> gtk_widget_set_size_request() here, but rather fix up the code that sets
>> up
>
> the
>>
>> preferred size for the window in the first place. It's important to note
>> that
>> the preferred size in the context of GtkWindow is not really a preferred
>> size,
>> but a minimum size instead.
>
>
>
> http://codereview.chromium.org/2768006/show
>

[sky, 2010-08-06 23:08:31]

In fact you might try putting a breakpoint in
gtk_widget_set_size_request to see if it's getting invoked some where.

  -Scott

On Fri, Aug 6, 2010 at 4:07 PM, Scott Violet <sky@chromium.org> wrote:
> Is TabContentsViewGtk::SizeContents getting invoked, which resets the
> requested size?
>
>  -Scott
>
> On Fri, Aug 6, 2010 at 2:51 PM,  <scottbyer@chromium.org> wrote:
>> (With travel in the middle, took me a bit to get back and dig deep into
>> this).
>>
>> Here's my current variant of the fix.
>>
>> I found one other possibility and that's to revert the change to
>> native_view_host_gtk.cc made as part of
>> http://codereview.chromium.org/2131021
>> where gtk_widget_size_allocate was replaced with
>> gtk_widget_set_size_request.  I
>> believe that the flashing from that original message could be because the
>> layout
>> done in NativeViewHost::Layout() doesn't propagate the parent's new size (I
>> could imaging setting up a preferred size on the NativeViewHost, and
>> Layout()
>> would use that size if set instead of it's current size) - and because the
>> size
>> doesn't propagate, the resizing loop bounces around between a few sizes a
>> bunch
>> of times and that could have caused the flickering.  I suspect that
>> correcting
>> the layout in that way, getting the size right the first time, the bouncing
>> and
>> flickering wouldn't happen.  But that's a much more intricate change with
>> much
>> larger implications and goes into some core views system files, and I'm not
>> experienced enough with the views code to say if that would really be a more
>> correct fix.
>>
>> This fix definitely targets the containing page requesting a new size code
>> path
>> - by sizing the HtmlDialogView (which is a NativeViewHost) first, it's
>> prepping
>> NativeViewHost::Layout() to get the right answer when it gets the RootView
>> size
>> changed notification.
>>
>> And Paweł, thank you for the drive-by.  I dove into why the timed event loop
>> worked when ui_test_utils::RunAllPendingInMessageLoop didn't, and figured
>> out
>> that it's the message pump getting into the delayed messages that was the
>> key to
>> getting the window server information synchronized and valid for testing.
>>  Since
>> you've been over the testing code more, you can tell me if I really should
>> refactor this into another ui_test_utils routine - I haven't really seen the
>> need, but haven't read through a lot of browser test code yet.
>>
>>
>> I can dive into seeing if the larger fix makes sense if anyone really wants,
>> but
>> I'd like to do that as a separate issue, since getting a fix in for cloud
>> print
>> launch is important (and I really don't know for sure I can get a fix going
>> down
>> that path without some regression elsewhere).  I'd also like to tackle
>> getting
>> the test working on Windows as a separate issue, since I suspect that there
>> may
>> be different window sizing synchronization issues there.
>>
>> -Scott
>>
>>
>> On 2010/07/02 07:29:34, mnissler wrote:
>>>
>>> http://codereview.chromium.org/2768006/diff/12001/13003
>>> File views/widget/widget_gtk.cc (right):
>>
>>> http://codereview.chromium.org/2768006/diff/12001/13003#newcode651
>>> views/widget/widget_gtk.cc:651: gtk_widget_set_size_request(widget_,
>>> bounds.width(), bounds.height());
>>> I disucssed this exact call with sky in
>>> http://codereview.chromium.org/2812026/show
>>
>>> The bottom line of that discussion was basically to not call
>>> gtk_widget_set_size_request() here, but rather fix up the code that sets
>>> up
>>
>> the
>>>
>>> preferred size for the window in the first place. It's important to note
>>> that
>>> the preferred size in the context of GtkWindow is not really a preferred
>>> size,
>>> but a minimum size instead.
>>
>>
>>
>> http://codereview.chromium.org/2768006/show
>>
>

[Scott Byer, 2010-08-06 23:25:01]

I know gtk_widget_set_size_request is getting invoked in
NativeViewHostGtk::ShowWidget - see http://codereview.chromium.org/2131021

I'll check on TabContentsViewGtk::SizeContents on Monday.

So I take it I really should dive into the larger fix and unwind whatever really
caused someone to feel like calling gtk_widget_set_size_request was needed?  I
suspect what's needed is for NativeViewHost::Layout to pay attention to it's
preferred size, and make sure NativeViewHostGtk::ShowWidget tells it what that
should be - I'm new enough to the Chrome code and GTK to be unsure of all the
implications.  You've given me a couple of additional things to poke at on
Monday...

-Scott

On 2010/08/06 23:08:31, sky wrote:
> In fact you might try putting a breakpoint in
> gtk_widget_set_size_request to see if it's getting invoked some where.
> 
>   -Scott
> 
> On Fri, Aug 6, 2010 at 4:07 PM, Scott Violet <mailto:sky@chromium.org> wrote:
> > Is TabContentsViewGtk::SizeContents getting invoked, which resets the
> > requested size?
> >
> > &nbsp;-Scott
> >
> > On Fri, Aug 6, 2010 at 2:51 PM, mailto: <scottbyer@chromium.org> wrote:
> >> (With travel in the middle, took me a bit to get back and dig deep into
> >> this).
> >>
> >> Here's my current variant of the fix.
> >>
> >> I found one other possibility and that's to revert the change to
> >> native_view_host_gtk.cc made as part of
> >> http://codereview.chromium.org/2131021
> >> where gtk_widget_size_allocate was replaced with
> >> gtk_widget_set_size_request. &nbsp;I
> >> believe that the flashing from that original message could be because the
> >> layout
> >> done in NativeViewHost::Layout() doesn't propagate the parent's new size (I
> >> could imaging setting up a preferred size on the NativeViewHost, and
> >> Layout()
> >> would use that size if set instead of it's current size) - and because the
> >> size
> >> doesn't propagate, the resizing loop bounces around between a few sizes a
> >> bunch
> >> of times and that could have caused the flickering. &nbsp;I suspect that
> >> correcting
> >> the layout in that way, getting the size right the first time, the bouncing
> >> and
> >> flickering wouldn't happen. &nbsp;But that's a much more intricate change
with
> >> much
> >> larger implications and goes into some core views system files, and I'm not
> >> experienced enough with the views code to say if that would really be a
more
> >> correct fix.
> >>
> >> This fix definitely targets the containing page requesting a new size code
> >> path
> >> - by sizing the HtmlDialogView (which is a NativeViewHost) first, it's
> >> prepping
> >> NativeViewHost::Layout() to get the right answer when it gets the RootView
> >> size
> >> changed notification.
> >>
> >> And Paweł, thank you for the drive-by. &nbsp;I dove into why the timed
event loop
> >> worked when ui_test_utils::RunAllPendingInMessageLoop didn't, and figured
> >> out
> >> that it's the message pump getting into the delayed messages that was the
> >> key to
> >> getting the window server information synchronized and valid for testing.
> >> &nbsp;Since
> >> you've been over the testing code more, you can tell me if I really should
> >> refactor this into another ui_test_utils routine - I haven't really seen
the
> >> need, but haven't read through a lot of browser test code yet.
> >>
> >>
> >> I can dive into seeing if the larger fix makes sense if anyone really
wants,
> >> but
> >> I'd like to do that as a separate issue, since getting a fix in for cloud
> >> print
> >> launch is important (and I really don't know for sure I can get a fix going
> >> down
> >> that path without some regression elsewhere). &nbsp;I'd also like to tackle
> >> getting
> >> the test working on Windows as a separate issue, since I suspect that there
> >> may
> >> be different window sizing synchronization issues there.
> >>
> >> -Scott
> >>
> >>
> >> On 2010/07/02 07:29:34, mnissler wrote:
> >>>
> >>> http://codereview.chromium.org/2768006/diff/12001/13003
> >>> File views/widget/widget_gtk.cc (right):
> >>
> >>> http://codereview.chromium.org/2768006/diff/12001/13003#newcode651
> >>> views/widget/widget_gtk.cc:651: gtk_widget_set_size_request(widget_,
> >>> bounds.width(), bounds.height());
> >>> I disucssed this exact call with sky in
> >>> http://codereview.chromium.org/2812026/show
> >>
> >>> The bottom line of that discussion was basically to not call
> >>> gtk_widget_set_size_request() here, but rather fix up the code that sets
> >>> up
> >>
> >> the
> >>>
> >>> preferred size for the window in the first place. It's important to note
> >>> that
> >>> the preferred size in the context of GtkWindow is not really a preferred
> >>> size,
> >>> but a minimum size instead.
> >>
> >>
> >>
> >> http://codereview.chromium.org/2768006/show
> >>
> >
>

[sky, 2010-08-06 23:39:42]

On Fri, Aug 6, 2010 at 4:25 PM,  <scottbyer@chromium.org> wrote:
>
> I know gtk_widget_set_size_request is getting invoked in
> NativeViewHostGtk::ShowWidget - see http://codereview.chromium.org/2131021

I'm more interested if someone else is invoking it.

> I'll check on TabContentsViewGtk::SizeContents on Monday.
>
> So I take it I really should dive into the larger fix and unwind whatever
> really
> caused someone to feel like calling gtk_widget_set_size_request was needed?

Yes please.

Thanks,

  -Scott

>  I
> suspect what's needed is for NativeViewHost::Layout to pay attention to it's
> preferred size, and make sure NativeViewHostGtk::ShowWidget tells it what
> that
> should be - I'm new enough to the Chrome code and GTK to be unsure of all
> the
> implications.  You've given me a couple of additional things to poke at on
> Monday...
>
> -Scott
>
> On 2010/08/06 23:08:31, sky wrote:
>>
>> In fact you might try putting a breakpoint in
>> gtk_widget_set_size_request to see if it's getting invoked some where.
>
>>   -Scott
>
>> On Fri, Aug 6, 2010 at 4:07 PM, Scott Violet <mailto:sky@chromium.org>
>> wrote:
>> > Is TabContentsViewGtk::SizeContents getting invoked, which resets the
>> > requested size?
>> >
>> > &nbsp;-Scott
>> >
>> > On Fri, Aug 6, 2010 at 2:51 PM, mailto: <scottbyer@chromium.org> wrote:
>> >> (With travel in the middle, took me a bit to get back and dig deep into
>> >> this).
>> >>
>> >> Here's my current variant of the fix.
>> >>
>> >> I found one other possibility and that's to revert the change to
>> >> native_view_host_gtk.cc made as part of
>> >> http://codereview.chromium.org/2131021
>> >> where gtk_widget_size_allocate was replaced with
>> >> gtk_widget_set_size_request. &nbsp;I
>> >> believe that the flashing from that original message could be because
>> >> the
>> >> layout
>> >> done in NativeViewHost::Layout() doesn't propagate the parent's new
>> >> size (I
>> >> could imaging setting up a preferred size on the NativeViewHost, and
>> >> Layout()
>> >> would use that size if set instead of it's current size) - and because
>> >> the
>> >> size
>> >> doesn't propagate, the resizing loop bounces around between a few sizes
>> >> a
>> >> bunch
>> >> of times and that could have caused the flickering. &nbsp;I suspect
>> >> that
>> >> correcting
>> >> the layout in that way, getting the size right the first time, the
>> >> bouncing
>> >> and
>> >> flickering wouldn't happen. &nbsp;But that's a much more intricate
>> >> change
>
> with
>>
>> >> much
>> >> larger implications and goes into some core views system files, and I'm
>> >> not
>> >> experienced enough with the views code to say if that would really be a
>
> more
>>
>> >> correct fix.
>> >>
>> >> This fix definitely targets the containing page requesting a new size
>> >> code
>> >> path
>> >> - by sizing the HtmlDialogView (which is a NativeViewHost) first, it's
>> >> prepping
>> >> NativeViewHost::Layout() to get the right answer when it gets the
>> >> RootView
>> >> size
>> >> changed notification.
>> >>
>> >> And Paweł, thank you for the drive-by. &nbsp;I dove into why the timed
>
> event loop
>>
>> >> worked when ui_test_utils::RunAllPendingInMessageLoop didn't, and
>> >> figured
>> >> out
>> >> that it's the message pump getting into the delayed messages that was
>> >> the
>> >> key to
>> >> getting the window server information synchronized and valid for
>> >> testing.
>> >> &nbsp;Since
>> >> you've been over the testing code more, you can tell me if I really
>> >> should
>> >> refactor this into another ui_test_utils routine - I haven't really
>> >> seen
>
> the
>>
>> >> need, but haven't read through a lot of browser test code yet.
>> >>
>> >>
>> >> I can dive into seeing if the larger fix makes sense if anyone really
>
> wants,
>>
>> >> but
>> >> I'd like to do that as a separate issue, since getting a fix in for
>> >> cloud
>> >> print
>> >> launch is important (and I really don't know for sure I can get a fix
>> >> going
>> >> down
>> >> that path without some regression elsewhere). &nbsp;I'd also like to
>> >> tackle
>> >> getting
>> >> the test working on Windows as a separate issue, since I suspect that
>> >> there
>> >> may
>> >> be different window sizing synchronization issues there.
>> >>
>> >> -Scott
>> >>
>> >>
>> >> On 2010/07/02 07:29:34, mnissler wrote:
>> >>>
>> >>> http://codereview.chromium.org/2768006/diff/12001/13003
>> >>> File views/widget/widget_gtk.cc (right):
>> >>
>> >>> http://codereview.chromium.org/2768006/diff/12001/13003#newcode651
>> >>> views/widget/widget_gtk.cc:651: gtk_widget_set_size_request(widget_,
>> >>> bounds.width(), bounds.height());
>> >>> I disucssed this exact call with sky in
>> >>> http://codereview.chromium.org/2812026/show
>> >>
>> >>> The bottom line of that discussion was basically to not call
>> >>> gtk_widget_set_size_request() here, but rather fix up the code that
>> >>> sets
>> >>> up
>> >>
>> >> the
>> >>>
>> >>> preferred size for the window in the first place. It's important to
>> >>> note
>> >>> that
>> >>> the preferred size in the context of GtkWindow is not really a
>> >>> preferred
>> >>> size,
>> >>> but a minimum size instead.
>> >>
>> >>
>> >>
>> >> http://codereview.chromium.org/2768006/show
>> >>
>> >
>
>
>
>
> http://codereview.chromium.org/2768006/show
>

[Scott Byer, 2010-08-10 20:24:14]

Diving back into this.  TabContentsViewGtk::SizeContents is not being called. 
gtk_widget_set_size_request is being called for the
chrome-render-widget-host-view from RenderWidgetHostViewGtk::SetSize for
TOOLKIT_VIEWS builds only, but NativeViewHostGtk::ShowWidget is the only place
it's being called on the views_gtkwidget-child-fixed widget.  Removing the call
from RWHVG::SetSize has no affect on the problem.

I've included two traces - one for the first test, which is sizing the dialog
larger, and the second test which is trying to shrink the height (which fails).

My instinct at this point is to revert http://codereview.chromium.org/2131021,
remove the gtk_widget_set_size_request from RWHVG::SetSize, reproduce the
flickering problem (which I'm pretty sure is actually due to the bouncing resize
requests as shown in the trace below), and remove the bouncing resize requests
by fixing how the NativeViewHost::Layout happens, but I'm looking for guidance
on that.

-Scott

=============================
Test 1, sizing the dialog to 400x300:

Breakpoint 6, HtmlDialogBrowserTest_SizeWindow_Test::RunTestOnMainThread
(this=0xcbd22d0) at chrome/browser/views/html_dialog_view_browsertest.cc:137
$53 = "STARTING WINDOW"

Breakpoint 2, views::WidgetGtk::OnSizeAllocate (this=0xce1cc00,
widget=0xcdcfcc0, allocation=0xffffb780) at views/widget/widget_gtk.cc:944
$54 = {
  x = 0, 
  y = 0, 
  width = 400, 
  height = 300
}

Breakpoint 3, views::NativeViewHost::Layout (this=0xcde8e70) at
views/controls/native/native_view_host.cc:81
$55 = {
  origin_ = {
    x_ = 0, 
    y_ = 0
  }, 
  size_ = {
    width_ = 1, 
    height_ = 1
  }
}

Breakpoint 7, IA__gtk_widget_set_size_request (widget=0xcd94388, width=1,
height=1) at /build/buildd/gtk+2.0-2.20.1/gtk/gtkwidget.c:7946
$56 = (gchar *) 0xce536a0 "views-gtkwidget-child-fixed"
#0  IA__gtk_widget_set_size_request (widget=0xcd94388, width=1, height=1) at
/build/buildd/gtk+2.0-2.20.1/gtk/gtkwidget.c:7946
#1  0x09bd07df in views::NativeViewHostGtk::ShowWidget (this=0xce74c60, x=5,
y=23, w=1, h=1) at views/controls/native/native_view_host_gtk.cc:235
#2  0x09bcf629 in views::NativeViewHost::Layout (this=0xcde8e70) at
views/controls/native/native_view_host.cc:109

Breakpoint 3, views::NativeViewHost::Layout (this=0xcde8e70) at
views/controls/native/native_view_host.cc:81
$57 = {
  origin_ = {
    x_ = 0, 
    y_ = 0
  }, 
  size_ = {
    width_ = 390, 
    height_ = 272
  }
}

Breakpoint 7, IA__gtk_widget_set_size_request (widget=0xcd94388, width=390,
height=272) at /build/buildd/gtk+2.0-2.20.1/gtk/gtkwidget.c:7946
$58 = (gchar *) 0xce536a0 "views-gtkwidget-child-fixed"
#0  IA__gtk_widget_set_size_request (widget=0xcd94388, width=390, height=272) at
/build/buildd/gtk+2.0-2.20.1/gtk/gtkwidget.c:7946
#1  0x09bd07df in views::NativeViewHostGtk::ShowWidget (this=0xce74c60, x=5,
y=23, w=390, h=272) at views/controls/native/native_view_host_gtk.cc:235
#2  0x09bcf629 in views::NativeViewHost::Layout (this=0xcde8e70) at
views/controls/native/native_view_host.cc:109

Breakpoint 3, views::NativeViewHost::Layout (this=0xcde8e70) at
views/controls/native/native_view_host.cc:81
$59 = {
  origin_ = {
    x_ = 0, 
    y_ = 0
  }, 
  size_ = {
    width_ = 390, 
    height_ = 272
  }
}

Breakpoint 7, IA__gtk_widget_set_size_request (widget=0xcd94340, width=390,
height=272) at /build/buildd/gtk+2.0-2.20.1/gtk/gtkwidget.c:7946
$60 = (gchar *) 0xce539a0 "chrome-render-widget-host-view"
#0  IA__gtk_widget_set_size_request (widget=0xcd94340, width=390, height=272) at
/build/buildd/gtk+2.0-2.20.1/gtk/gtkwidget.c:7946
#1  0x09238e6b in RenderWidgetHostViewGtk::SetSize (this=0xce731a0, size=...) at
chrome/browser/renderer_host/render_widget_host_view_gtk.cc:577
#2  0x09372c19 in TabContentsViewGtk::WasSized (this=0xcda73c0, size=...) at
chrome/browser/views/tab_contents/tab_contents_view_gtk.cc:429

Breakpoint 7, IA__gtk_widget_set_size_request (widget=0xcd94388, width=390,
height=272) at /build/buildd/gtk+2.0-2.20.1/gtk/gtkwidget.c:7946
$61 = (gchar *) 0xce536a0 "views-gtkwidget-child-fixed"
#0  IA__gtk_widget_set_size_request (widget=0xcd94388, width=390, height=272) at
/build/buildd/gtk+2.0-2.20.1/gtk/gtkwidget.c:7946
#1  0x09bd07df in views::NativeViewHostGtk::ShowWidget (this=0xce74c60, x=5,
y=23, w=390, h=272) at views/controls/native/native_view_host_gtk.cc:235
#2  0x09bcf629 in views::NativeViewHost::Layout (this=0xcde8e70) at
views/controls/native/native_view_host.cc:109

Breakpoint 3, views::NativeViewHost::Layout (this=0xcde8e70) at
views/controls/native/native_view_host.cc:81
$62 = {
  origin_ = {
    x_ = 0, 
    y_ = 0
  }, 
  size_ = {
    width_ = 390, 
    height_ = 272
  }
}

Breakpoint 7, IA__gtk_widget_set_size_request (widget=0xcd94388, width=390,
height=272) at /build/buildd/gtk+2.0-2.20.1/gtk/gtkwidget.c:7946
$63 = (gchar *) 0xce536a0 "views-gtkwidget-child-fixed"
#0  IA__gtk_widget_set_size_request (widget=0xcd94388, width=390, height=272) at
/build/buildd/gtk+2.0-2.20.1/gtk/gtkwidget.c:7946
#1  0x09bd07df in views::NativeViewHostGtk::ShowWidget (this=0xce74c60, x=5,
y=23, w=390, h=272) at views/controls/native/native_view_host_gtk.cc:235
#2  0x09bcf629 in views::NativeViewHost::Layout (this=0xcde8e70) at
views/controls/native/native_view_host.cc:109



============================
Test 2, sizing the dialog to 500x250:

Breakpoint 10, HtmlDialogBrowserTest_SizeWindow_Test::RunTestOnMainThread
(this=0xcbd22d0) at chrome/browser/views/html_dialog_view_browsertest.cc:147
$65 = "STARTING"

Breakpoint 2, views::WidgetGtk::OnSizeAllocate (this=0xce3fc00,
widget=0xcda8cc0, allocation=0xffffb780) at views/widget/widget_gtk.cc:944
$66 = {
  x = 0, 
  y = 0, 
  width = 550, 
  height = 295
}

Breakpoint 3, views::NativeViewHost::Layout (this=0xcde8e70) at
views/controls/native/native_view_host.cc:81
$67 = {
  origin_ = {
    x_ = 0, 
    y_ = 0
  }, 
  size_ = {
    width_ = 390, 
    height_ = 267
  }
}

Breakpoint 7, IA__gtk_widget_set_size_request (widget=0xcd94388, width=390,
height=272) at /build/buildd/gtk+2.0-2.20.1/gtk/gtkwidget.c:7946
$68 = (gchar *) 0xce5a2a0 "views-gtkwidget-child-fixed"
#0  IA__gtk_widget_set_size_request (widget=0xcd94388, width=390, height=272) at
/build/buildd/gtk+2.0-2.20.1/gtk/gtkwidget.c:7946
#1  0x09bd07df in views::NativeViewHostGtk::ShowWidget (this=0xce84180, x=5,
y=23, w=390, h=272) at views/controls/native/native_view_host_gtk.cc:235
#2  0x09bcf629 in views::NativeViewHost::Layout (this=0xcde8e70) at
views/controls/native/native_view_host.cc:109

Breakpoint 3, views::NativeViewHost::Layout (this=0xcde8e70) at
views/controls/native/native_view_host.cc:81
$69 = {
  origin_ = {
    x_ = 0, 
    y_ = 0
  }, 
  size_ = {
    width_ = 540, 
    height_ = 267
  }
}

Breakpoint 7, IA__gtk_widget_set_size_request (widget=0xcd94388, width=540,
height=267) at /build/buildd/gtk+2.0-2.20.1/gtk/gtkwidget.c:7946
$70 = (gchar *) 0xce5a2a0 "views-gtkwidget-child-fixed"
#0  IA__gtk_widget_set_size_request (widget=0xcd94388, width=540, height=267) at
/build/buildd/gtk+2.0-2.20.1/gtk/gtkwidget.c:7946
#1  0x09bd07df in views::NativeViewHostGtk::ShowWidget (this=0xce84180, x=5,
y=23, w=540, h=267) at views/controls/native/native_view_host_gtk.cc:235
#2  0x09bcf629 in views::NativeViewHost::Layout (this=0xcde8e70) at
views/controls/native/native_view_host.cc:109

Breakpoint 3, views::NativeViewHost::Layout (this=0xcde8e70) at
views/controls/native/native_view_host.cc:81
$71 = {
  origin_ = {
    x_ = 0, 
    y_ = 0
  }, 
  size_ = {
    width_ = 540, 
    height_ = 267
  }
}

Breakpoint 7, IA__gtk_widget_set_size_request (widget=0xcd94340, width=540,
height=267) at /build/buildd/gtk+2.0-2.20.1/gtk/gtkwidget.c:7946
$72 = (gchar *) 0xce5a5a0 "chrome-render-widget-host-view"
#0  IA__gtk_widget_set_size_request (widget=0xcd94340, width=540, height=267) at
/build/buildd/gtk+2.0-2.20.1/gtk/gtkwidget.c:7946
#1  0x09238e6b in RenderWidgetHostViewGtk::SetSize (this=0xce47680, size=...) at
chrome/browser/renderer_host/render_widget_host_view_gtk.cc:577
#2  0x09372c19 in TabContentsViewGtk::WasSized (this=0xcda73c0, size=...) at
chrome/browser/views/tab_contents/tab_contents_view_gtk.cc:429

Breakpoint 7, IA__gtk_widget_set_size_request (widget=0xcd94388, width=540,
height=267) at /build/buildd/gtk+2.0-2.20.1/gtk/gtkwidget.c:7946
$73 = (gchar *) 0xce5a2a0 "views-gtkwidget-child-fixed"
#0  IA__gtk_widget_set_size_request (widget=0xcd94388, width=540, height=267) at
/build/buildd/gtk+2.0-2.20.1/gtk/gtkwidget.c:7946
#1  0x09bd07df in views::NativeViewHostGtk::ShowWidget (this=0xce84180, x=5,
y=23, w=540, h=267) at views/controls/native/native_view_host_gtk.cc:235
#2  0x09bcf629 in views::NativeViewHost::Layout (this=0xcde8e70) at
views/controls/native/native_view_host.cc:109

Breakpoint 3, views::NativeViewHost::Layout (this=0xcde8e70) at
views/controls/native/native_view_host.cc:81
$74 = {
  origin_ = {
    x_ = 0, 
    y_ = 0
  }, 
  size_ = {
    width_ = 540, 
    height_ = 267
  }
}

Breakpoint 7, IA__gtk_widget_set_size_request (widget=0xcd94388, width=540,
height=267) at /build/buildd/gtk+2.0-2.20.1/gtk/gtkwidget.c:7946
$75 = (gchar *) 0xce5a2a0 "views-gtkwidget-child-fixed"
#0  IA__gtk_widget_set_size_request (widget=0xcd94388, width=540, height=267) at
/build/buildd/gtk+2.0-2.20.1/gtk/gtkwidget.c:7946
#1  0x09bd07df in views::NativeViewHostGtk::ShowWidget (this=0xce84180, x=5,
y=23, w=540, h=267) at views/controls/native/native_view_host_gtk.cc:235
#2  0x09bcf629 in views::NativeViewHost::Layout (this=0xcde8e70) at
views/controls/native/native_view_host.cc:109
chrome/browser/views/html_dialog_view_browsertest.cc:152: Failure
Value of: actual_bounds
  Actual: 1274,804 550x295
Expected: set_bounds
Which is: 1274,804 550x250

[sky, 2010-08-11 17:01:08]

I believe the problem that lead Dmitry to now use gtk_views_fixed was
some flickering that appeared because we were sizing the widget right
away (gtk_widget_size_allocate). Seems like the right fix is to not
size right away but instead stash the size in a different property.
This should fix the flicker and fix the bug you're seeing. You should
be able to stash the size using g_object_set_data. Does that make
sense?

Dmitry, does this sounds good? Do you have a test case to make sure we
don't see any flicker?

  -Scott

On Tue, Aug 10, 2010 at 1:24 PM,  <scottbyer@chromium.org> wrote:
> Diving back into this.  TabContentsViewGtk::SizeContents is not being
> called.
> gtk_widget_set_size_request is being called for the
> chrome-render-widget-host-view from RenderWidgetHostViewGtk::SetSize for
> TOOLKIT_VIEWS builds only, but NativeViewHostGtk::ShowWidget is the only
> place
> it's being called on the views_gtkwidget-child-fixed widget.  Removing the
> call
> from RWHVG::SetSize has no affect on the problem.
>
> I've included two traces - one for the first test, which is sizing the
> dialog
> larger, and the second test which is trying to shrink the height (which
> fails).
>
> My instinct at this point is to revert
> http://codereview.chromium.org/2131021,
> remove the gtk_widget_set_size_request from RWHVG::SetSize, reproduce the
> flickering problem (which I'm pretty sure is actually due to the bouncing
> resize
> requests as shown in the trace below), and remove the bouncing resize
> requests
> by fixing how the NativeViewHost::Layout happens, but I'm looking for
> guidance
> on that.
>
> -Scott
>
> =============================
> Test 1, sizing the dialog to 400x300:
>
> Breakpoint 6, HtmlDialogBrowserTest_SizeWindow_Test::RunTestOnMainThread
> (this=0xcbd22d0) at chrome/browser/views/html_dialog_view_browsertest.cc:137
> $53 = "STARTING WINDOW"
>
> Breakpoint 2, views::WidgetGtk::OnSizeAllocate (this=0xce1cc00,
> widget=0xcdcfcc0, allocation=0xffffb780) at views/widget/widget_gtk.cc:944
> $54 = {
>  x = 0,
>  y = 0,
>  width = 400,
>  height = 300
> }
>
> Breakpoint 3, views::NativeViewHost::Layout (this=0xcde8e70) at
> views/controls/native/native_view_host.cc:81
> $55 = {
>  origin_ = {
>    x_ = 0,
>    y_ = 0
>  },
>  size_ = {
>    width_ = 1,
>    height_ = 1
>  }
> }
>
> Breakpoint 7, IA__gtk_widget_set_size_request (widget=0xcd94388, width=1,
> height=1) at /build/buildd/gtk+2.0-2.20.1/gtk/gtkwidget.c:7946
> $56 = (gchar *) 0xce536a0 "views-gtkwidget-child-fixed"
> #0  IA__gtk_widget_set_size_request (widget=0xcd94388, width=1, height=1) at
> /build/buildd/gtk+2.0-2.20.1/gtk/gtkwidget.c:7946
> #1  0x09bd07df in views::NativeViewHostGtk::ShowWidget (this=0xce74c60, x=5,
> y=23, w=1, h=1) at views/controls/native/native_view_host_gtk.cc:235
> #2  0x09bcf629 in views::NativeViewHost::Layout (this=0xcde8e70) at
> views/controls/native/native_view_host.cc:109
>
> Breakpoint 3, views::NativeViewHost::Layout (this=0xcde8e70) at
> views/controls/native/native_view_host.cc:81
> $57 = {
>  origin_ = {
>    x_ = 0,
>    y_ = 0
>  },
>  size_ = {
>    width_ = 390,
>    height_ = 272
>  }
> }
>
> Breakpoint 7, IA__gtk_widget_set_size_request (widget=0xcd94388, width=390,
> height=272) at /build/buildd/gtk+2.0-2.20.1/gtk/gtkwidget.c:7946
> $58 = (gchar *) 0xce536a0 "views-gtkwidget-child-fixed"
> #0  IA__gtk_widget_set_size_request (widget=0xcd94388, width=390,
> height=272) at
> /build/buildd/gtk+2.0-2.20.1/gtk/gtkwidget.c:7946
> #1  0x09bd07df in views::NativeViewHostGtk::ShowWidget (this=0xce74c60, x=5,
> y=23, w=390, h=272) at views/controls/native/native_view_host_gtk.cc:235
> #2  0x09bcf629 in views::NativeViewHost::Layout (this=0xcde8e70) at
> views/controls/native/native_view_host.cc:109
>
> Breakpoint 3, views::NativeViewHost::Layout (this=0xcde8e70) at
> views/controls/native/native_view_host.cc:81
> $59 = {
>  origin_ = {
>    x_ = 0,
>    y_ = 0
>  },
>  size_ = {
>    width_ = 390,
>    height_ = 272
>  }
> }
>
> Breakpoint 7, IA__gtk_widget_set_size_request (widget=0xcd94340, width=390,
> height=272) at /build/buildd/gtk+2.0-2.20.1/gtk/gtkwidget.c:7946
> $60 = (gchar *) 0xce539a0 "chrome-render-widget-host-view"
> #0  IA__gtk_widget_set_size_request (widget=0xcd94340, width=390,
> height=272) at
> /build/buildd/gtk+2.0-2.20.1/gtk/gtkwidget.c:7946
> #1  0x09238e6b in RenderWidgetHostViewGtk::SetSize (this=0xce731a0,
> size=...) at
> chrome/browser/renderer_host/render_widget_host_view_gtk.cc:577
> #2  0x09372c19 in TabContentsViewGtk::WasSized (this=0xcda73c0, size=...) at
> chrome/browser/views/tab_contents/tab_contents_view_gtk.cc:429
>
> Breakpoint 7, IA__gtk_widget_set_size_request (widget=0xcd94388, width=390,
> height=272) at /build/buildd/gtk+2.0-2.20.1/gtk/gtkwidget.c:7946
> $61 = (gchar *) 0xce536a0 "views-gtkwidget-child-fixed"
> #0  IA__gtk_widget_set_size_request (widget=0xcd94388, width=390,
> height=272) at
> /build/buildd/gtk+2.0-2.20.1/gtk/gtkwidget.c:7946
> #1  0x09bd07df in views::NativeViewHostGtk::ShowWidget (this=0xce74c60, x=5,
> y=23, w=390, h=272) at views/controls/native/native_view_host_gtk.cc:235
> #2  0x09bcf629 in views::NativeViewHost::Layout (this=0xcde8e70) at
> views/controls/native/native_view_host.cc:109
>
> Breakpoint 3, views::NativeViewHost::Layout (this=0xcde8e70) at
> views/controls/native/native_view_host.cc:81
> $62 = {
>  origin_ = {
>    x_ = 0,
>    y_ = 0
>  },
>  size_ = {
>    width_ = 390,
>    height_ = 272
>  }
> }
>
> Breakpoint 7, IA__gtk_widget_set_size_request (widget=0xcd94388, width=390,
> height=272) at /build/buildd/gtk+2.0-2.20.1/gtk/gtkwidget.c:7946
> $63 = (gchar *) 0xce536a0 "views-gtkwidget-child-fixed"
> #0  IA__gtk_widget_set_size_request (widget=0xcd94388, width=390,
> height=272) at
> /build/buildd/gtk+2.0-2.20.1/gtk/gtkwidget.c:7946
> #1  0x09bd07df in views::NativeViewHostGtk::ShowWidget (this=0xce74c60, x=5,
> y=23, w=390, h=272) at views/controls/native/native_view_host_gtk.cc:235
> #2  0x09bcf629 in views::NativeViewHost::Layout (this=0xcde8e70) at
> views/controls/native/native_view_host.cc:109
>
>
>
> ============================
> Test 2, sizing the dialog to 500x250:
>
> Breakpoint 10, HtmlDialogBrowserTest_SizeWindow_Test::RunTestOnMainThread
> (this=0xcbd22d0) at chrome/browser/views/html_dialog_view_browsertest.cc:147
> $65 = "STARTING"
>
> Breakpoint 2, views::WidgetGtk::OnSizeAllocate (this=0xce3fc00,
> widget=0xcda8cc0, allocation=0xffffb780) at views/widget/widget_gtk.cc:944
> $66 = {
>  x = 0,
>  y = 0,
>  width = 550,
>  height = 295
> }
>
> Breakpoint 3, views::NativeViewHost::Layout (this=0xcde8e70) at
> views/controls/native/native_view_host.cc:81
> $67 = {
>  origin_ = {
>    x_ = 0,
>    y_ = 0
>  },
>  size_ = {
>    width_ = 390,
>    height_ = 267
>  }
> }
>
> Breakpoint 7, IA__gtk_widget_set_size_request (widget=0xcd94388, width=390,
> height=272) at /build/buildd/gtk+2.0-2.20.1/gtk/gtkwidget.c:7946
> $68 = (gchar *) 0xce5a2a0 "views-gtkwidget-child-fixed"
> #0  IA__gtk_widget_set_size_request (widget=0xcd94388, width=390,
> height=272) at
> /build/buildd/gtk+2.0-2.20.1/gtk/gtkwidget.c:7946
> #1  0x09bd07df in views::NativeViewHostGtk::ShowWidget (this=0xce84180, x=5,
> y=23, w=390, h=272) at views/controls/native/native_view_host_gtk.cc:235
> #2  0x09bcf629 in views::NativeViewHost::Layout (this=0xcde8e70) at
> views/controls/native/native_view_host.cc:109
>
> Breakpoint 3, views::NativeViewHost::Layout (this=0xcde8e70) at
> views/controls/native/native_view_host.cc:81
> $69 = {
>  origin_ = {
>    x_ = 0,
>    y_ = 0
>  },
>  size_ = {
>    width_ = 540,
>    height_ = 267
>  }
> }
>
> Breakpoint 7, IA__gtk_widget_set_size_request (widget=0xcd94388, width=540,
> height=267) at /build/buildd/gtk+2.0-2.20.1/gtk/gtkwidget.c:7946
> $70 = (gchar *) 0xce5a2a0 "views-gtkwidget-child-fixed"
> #0  IA__gtk_widget_set_size_request (widget=0xcd94388, width=540,
> height=267) at
> /build/buildd/gtk+2.0-2.20.1/gtk/gtkwidget.c:7946
> #1  0x09bd07df in views::NativeViewHostGtk::ShowWidget (this=0xce84180, x=5,
> y=23, w=540, h=267) at views/controls/native/native_view_host_gtk.cc:235
> #2  0x09bcf629 in views::NativeViewHost::Layout (this=0xcde8e70) at
> views/controls/native/native_view_host.cc:109
>
> Breakpoint 3, views::NativeViewHost::Layout (this=0xcde8e70) at
> views/controls/native/native_view_host.cc:81
> $71 = {
>  origin_ = {
>    x_ = 0,
>    y_ = 0
>  },
>  size_ = {
>    width_ = 540,
>    height_ = 267
>  }
> }
>
> Breakpoint 7, IA__gtk_widget_set_size_request (widget=0xcd94340, width=540,
> height=267) at /build/buildd/gtk+2.0-2.20.1/gtk/gtkwidget.c:7946
> $72 = (gchar *) 0xce5a5a0 "chrome-render-widget-host-view"
> #0  IA__gtk_widget_set_size_request (widget=0xcd94340, width=540,
> height=267) at
> /build/buildd/gtk+2.0-2.20.1/gtk/gtkwidget.c:7946
> #1  0x09238e6b in RenderWidgetHostViewGtk::SetSize (this=0xce47680,
> size=...) at
> chrome/browser/renderer_host/render_widget_host_view_gtk.cc:577
> #2  0x09372c19 in TabContentsViewGtk::WasSized (this=0xcda73c0, size=...) at
> chrome/browser/views/tab_contents/tab_contents_view_gtk.cc:429
>
> Breakpoint 7, IA__gtk_widget_set_size_request (widget=0xcd94388, width=540,
> height=267) at /build/buildd/gtk+2.0-2.20.1/gtk/gtkwidget.c:7946
> $73 = (gchar *) 0xce5a2a0 "views-gtkwidget-child-fixed"
> #0  IA__gtk_widget_set_size_request (widget=0xcd94388, width=540,
> height=267) at
> /build/buildd/gtk+2.0-2.20.1/gtk/gtkwidget.c:7946
> #1  0x09bd07df in views::NativeViewHostGtk::ShowWidget (this=0xce84180, x=5,
> y=23, w=540, h=267) at views/controls/native/native_view_host_gtk.cc:235
> #2  0x09bcf629 in views::NativeViewHost::Layout (this=0xcde8e70) at
> views/controls/native/native_view_host.cc:109
>
> Breakpoint 3, views::NativeViewHost::Layout (this=0xcde8e70) at
> views/controls/native/native_view_host.cc:81
> $74 = {
>  origin_ = {
>    x_ = 0,
>    y_ = 0
>  },
>  size_ = {
>    width_ = 540,
>    height_ = 267
>  }
> }
>
> Breakpoint 7, IA__gtk_widget_set_size_request (widget=0xcd94388, width=540,
> height=267) at /build/buildd/gtk+2.0-2.20.1/gtk/gtkwidget.c:7946
> $75 = (gchar *) 0xce5a2a0 "views-gtkwidget-child-fixed"
> #0  IA__gtk_widget_set_size_request (widget=0xcd94388, width=540,
> height=267) at
> /build/buildd/gtk+2.0-2.20.1/gtk/gtkwidget.c:7946
> #1  0x09bd07df in views::NativeViewHostGtk::ShowWidget (this=0xce84180, x=5,
> y=23, w=540, h=267) at views/controls/native/native_view_host_gtk.cc:235
> #2  0x09bcf629 in views::NativeViewHost::Layout (this=0xcde8e70) at
> views/controls/native/native_view_host.cc:109
> chrome/browser/views/html_dialog_view_browsertest.cc:152: Failure
> Value of: actual_bounds
>  Actual: 1274,804 550x295
> Expected: set_bounds
> Which is: 1274,804 550x250
>
>
> http://codereview.chromium.org/2768006/show
>

[Scott Byer, 2010-08-11 17:08:09]

Would it make sense to use the preferred_size_, which is already part of
NativeViewHost, but currently unused anywhere?  I was thinking that would allow
NativeViewHost::Layout to get the answer right the first time, if set.

-Scott

On 2010/08/11 17:01:08, sky wrote:
> I believe the problem that lead Dmitry to now use gtk_views_fixed was
> some flickering that appeared because we were sizing the widget right
> away (gtk_widget_size_allocate). Seems like the right fix is to not
> size right away but instead stash the size in a different property.
> This should fix the flicker and fix the bug you're seeing. You should
> be able to stash the size using g_object_set_data. Does that make
> sense?
> 
> Dmitry, does this sounds good? Do you have a test case to make sure we
> don't see any flicker?
> 
>   -Scott

[sky, 2010-08-11 17:19:39]

Better to use gtk_views_fixed and push the size to it. That way
gtk_views_fixed doesn't need to know about NativeViewXXX.

  -Scott

On Wed, Aug 11, 2010 at 10:08 AM,  <scottbyer@chromium.org> wrote:
> Would it make sense to use the preferred_size_, which is already part of
> NativeViewHost, but currently unused anywhere?  I was thinking that would
> allow
> NativeViewHost::Layout to get the answer right the first time, if set.
>
> -Scott
>
> On 2010/08/11 17:01:08, sky wrote:
>>
>> I believe the problem that lead Dmitry to now use gtk_views_fixed was
>> some flickering that appeared because we were sizing the widget right
>> away (gtk_widget_size_allocate). Seems like the right fix is to not
>> size right away but instead stash the size in a different property.
>> This should fix the flicker and fix the bug you're seeing. You should
>> be able to stash the size using g_object_set_data. Does that make
>> sense?
>
>> Dmitry, does this sounds good? Do you have a test case to make sure we
>> don't see any flicker?
>
>>   -Scott
>
>
> http://codereview.chromium.org/2768006/show
>

[Dmitry Polukhin, 2010-08-11 19:35:46]

I had to revert my CL with gtk_views_fixed because of flickering of tab
contents. I spent almost a week trying to replace gtk_fixed with
gtk_views_fixed
but I gave up when I realized that it results in ugly flickering. It looks
like GTK lays out child windows in couple passes that results in flickering.
The most ugly flickering I saw when I tried to resize Chrome window with
omnibox. I observed flickering on tab switching also.

On Wed, Aug 11, 2010 at 9:00 PM, Scott Violet <sky@chromium.org> wrote:

> I believe the problem that lead Dmitry to now use gtk_views_fixed was
> some flickering that appeared because we were sizing the widget right
> away (gtk_widget_size_allocate). Seems like the right fix is to not
> size right away but instead stash the size in a different property.
> This should fix the flicker and fix the bug you're seeing. You should
> be able to stash the size using g_object_set_data. Does that make
> sense?
>
> Dmitry, does this sounds good? Do you have a test case to make sure we
> don't see any flicker?
>
>  -Scott
>
> On Tue, Aug 10, 2010 at 1:24 PM,  <scottbyer@chromium.org> wrote:
> > Diving back into this.  TabContentsViewGtk::SizeContents is not being
> > called.
> > gtk_widget_set_size_request is being called for the
> > chrome-render-widget-host-view from RenderWidgetHostViewGtk::SetSize for
> > TOOLKIT_VIEWS builds only, but NativeViewHostGtk::ShowWidget is the only
> > place
> > it's being called on the views_gtkwidget-child-fixed widget.  Removing
> the
> > call
> > from RWHVG::SetSize has no affect on the problem.
> >
> > I've included two traces - one for the first test, which is sizing the
> > dialog
> > larger, and the second test which is trying to shrink the height (which
> > fails).
> >
> > My instinct at this point is to revert
> > http://codereview.chromium.org/2131021,
> > remove the gtk_widget_set_size_request from RWHVG::SetSize, reproduce the
> > flickering problem (which I'm pretty sure is actually due to the bouncing
> > resize
> > requests as shown in the trace below), and remove the bouncing resize
> > requests
> > by fixing how the NativeViewHost::Layout happens, but I'm looking for
> > guidance
> > on that.
> >
> > -Scott
> >
> > =============================
> > Test 1, sizing the dialog to 400x300:
> >
> > Breakpoint 6, HtmlDialogBrowserTest_SizeWindow_Test::RunTestOnMainThread
> > (this=0xcbd22d0) at
> chrome/browser/views/html_dialog_view_browsertest.cc:137
> > $53 = "STARTING WINDOW"
> >
> > Breakpoint 2, views::WidgetGtk::OnSizeAllocate (this=0xce1cc00,
> > widget=0xcdcfcc0, allocation=0xffffb780) at
> views/widget/widget_gtk.cc:944
> > $54 = {
> >  x = 0,
> >  y = 0,
> >  width = 400,
> >  height = 300
> > }
> >
> > Breakpoint 3, views::NativeViewHost::Layout (this=0xcde8e70) at
> > views/controls/native/native_view_host.cc:81
> > $55 = {
> >  origin_ = {
> >    x_ = 0,
> >    y_ = 0
> >  },
> >  size_ = {
> >    width_ = 1,
> >    height_ = 1
> >  }
> > }
> >
> > Breakpoint 7, IA__gtk_widget_set_size_request (widget=0xcd94388, width=1,
> > height=1) at /build/buildd/gtk+2.0-2.20.1/gtk/gtkwidget.c:7946
> > $56 = (gchar *) 0xce536a0 "views-gtkwidget-child-fixed"
> > #0  IA__gtk_widget_set_size_request (widget=0xcd94388, width=1, height=1)
> at
> > /build/buildd/gtk+2.0-2.20.1/gtk/gtkwidget.c:7946
> > #1  0x09bd07df in views::NativeViewHostGtk::ShowWidget (this=0xce74c60,
> x=5,
> > y=23, w=1, h=1) at views/controls/native/native_view_host_gtk.cc:235
> > #2  0x09bcf629 in views::NativeViewHost::Layout (this=0xcde8e70) at
> > views/controls/native/native_view_host.cc:109
> >
> > Breakpoint 3, views::NativeViewHost::Layout (this=0xcde8e70) at
> > views/controls/native/native_view_host.cc:81
> > $57 = {
> >  origin_ = {
> >    x_ = 0,
> >    y_ = 0
> >  },
> >  size_ = {
> >    width_ = 390,
> >    height_ = 272
> >  }
> > }
> >
> > Breakpoint 7, IA__gtk_widget_set_size_request (widget=0xcd94388,
> width=390,
> > height=272) at /build/buildd/gtk+2.0-2.20.1/gtk/gtkwidget.c:7946
> > $58 = (gchar *) 0xce536a0 "views-gtkwidget-child-fixed"
> > #0  IA__gtk_widget_set_size_request (widget=0xcd94388, width=390,
> > height=272) at
> > /build/buildd/gtk+2.0-2.20.1/gtk/gtkwidget.c:7946
> > #1  0x09bd07df in views::NativeViewHostGtk::ShowWidget (this=0xce74c60,
> x=5,
> > y=23, w=390, h=272) at views/controls/native/native_view_host_gtk.cc:235
> > #2  0x09bcf629 in views::NativeViewHost::Layout (this=0xcde8e70) at
> > views/controls/native/native_view_host.cc:109
> >
> > Breakpoint 3, views::NativeViewHost::Layout (this=0xcde8e70) at
> > views/controls/native/native_view_host.cc:81
> > $59 = {
> >  origin_ = {
> >    x_ = 0,
> >    y_ = 0
> >  },
> >  size_ = {
> >    width_ = 390,
> >    height_ = 272
> >  }
> > }
> >
> > Breakpoint 7, IA__gtk_widget_set_size_request (widget=0xcd94340,
> width=390,
> > height=272) at /build/buildd/gtk+2.0-2.20.1/gtk/gtkwidget.c:7946
> > $60 = (gchar *) 0xce539a0 "chrome-render-widget-host-view"
> > #0  IA__gtk_widget_set_size_request (widget=0xcd94340, width=390,
> > height=272) at
> > /build/buildd/gtk+2.0-2.20.1/gtk/gtkwidget.c:7946
> > #1  0x09238e6b in RenderWidgetHostViewGtk::SetSize (this=0xce731a0,
> > size=...) at
> > chrome/browser/renderer_host/render_widget_host_view_gtk.cc:577
> > #2  0x09372c19 in TabContentsViewGtk::WasSized (this=0xcda73c0, size=...)
> at
> > chrome/browser/views/tab_contents/tab_contents_view_gtk.cc:429
> >
> > Breakpoint 7, IA__gtk_widget_set_size_request (widget=0xcd94388,
> width=390,
> > height=272) at /build/buildd/gtk+2.0-2.20.1/gtk/gtkwidget.c:7946
> > $61 = (gchar *) 0xce536a0 "views-gtkwidget-child-fixed"
> > #0  IA__gtk_widget_set_size_request (widget=0xcd94388, width=390,
> > height=272) at
> > /build/buildd/gtk+2.0-2.20.1/gtk/gtkwidget.c:7946
> > #1  0x09bd07df in views::NativeViewHostGtk::ShowWidget (this=0xce74c60,
> x=5,
> > y=23, w=390, h=272) at views/controls/native/native_view_host_gtk.cc:235
> > #2  0x09bcf629 in views::NativeViewHost::Layout (this=0xcde8e70) at
> > views/controls/native/native_view_host.cc:109
> >
> > Breakpoint 3, views::NativeViewHost::Layout (this=0xcde8e70) at
> > views/controls/native/native_view_host.cc:81
> > $62 = {
> >  origin_ = {
> >    x_ = 0,
> >    y_ = 0
> >  },
> >  size_ = {
> >    width_ = 390,
> >    height_ = 272
> >  }
> > }
> >
> > Breakpoint 7, IA__gtk_widget_set_size_request (widget=0xcd94388,
> width=390,
> > height=272) at /build/buildd/gtk+2.0-2.20.1/gtk/gtkwidget.c:7946
> > $63 = (gchar *) 0xce536a0 "views-gtkwidget-child-fixed"
> > #0  IA__gtk_widget_set_size_request (widget=0xcd94388, width=390,
> > height=272) at
> > /build/buildd/gtk+2.0-2.20.1/gtk/gtkwidget.c:7946
> > #1  0x09bd07df in views::NativeViewHostGtk::ShowWidget (this=0xce74c60,
> x=5,
> > y=23, w=390, h=272) at views/controls/native/native_view_host_gtk.cc:235
> > #2  0x09bcf629 in views::NativeViewHost::Layout (this=0xcde8e70) at
> > views/controls/native/native_view_host.cc:109
> >
> >
> >
> > ============================
> > Test 2, sizing the dialog to 500x250:
> >
> > Breakpoint 10, HtmlDialogBrowserTest_SizeWindow_Test::RunTestOnMainThread
> > (this=0xcbd22d0) at
> chrome/browser/views/html_dialog_view_browsertest.cc:147
> > $65 = "STARTING"
> >
> > Breakpoint 2, views::WidgetGtk::OnSizeAllocate (this=0xce3fc00,
> > widget=0xcda8cc0, allocation=0xffffb780) at
> views/widget/widget_gtk.cc:944
> > $66 = {
> >  x = 0,
> >  y = 0,
> >  width = 550,
> >  height = 295
> > }
> >
> > Breakpoint 3, views::NativeViewHost::Layout (this=0xcde8e70) at
> > views/controls/native/native_view_host.cc:81
> > $67 = {
> >  origin_ = {
> >    x_ = 0,
> >    y_ = 0
> >  },
> >  size_ = {
> >    width_ = 390,
> >    height_ = 267
> >  }
> > }
> >
> > Breakpoint 7, IA__gtk_widget_set_size_request (widget=0xcd94388,
> width=390,
> > height=272) at /build/buildd/gtk+2.0-2.20.1/gtk/gtkwidget.c:7946
> > $68 = (gchar *) 0xce5a2a0 "views-gtkwidget-child-fixed"
> > #0  IA__gtk_widget_set_size_request (widget=0xcd94388, width=390,
> > height=272) at
> > /build/buildd/gtk+2.0-2.20.1/gtk/gtkwidget.c:7946
> > #1  0x09bd07df in views::NativeViewHostGtk::ShowWidget (this=0xce84180,
> x=5,
> > y=23, w=390, h=272) at views/controls/native/native_view_host_gtk.cc:235
> > #2  0x09bcf629 in views::NativeViewHost::Layout (this=0xcde8e70) at
> > views/controls/native/native_view_host.cc:109
> >
> > Breakpoint 3, views::NativeViewHost::Layout (this=0xcde8e70) at
> > views/controls/native/native_view_host.cc:81
> > $69 = {
> >  origin_ = {
> >    x_ = 0,
> >    y_ = 0
> >  },
> >  size_ = {
> >    width_ = 540,
> >    height_ = 267
> >  }
> > }
> >
> > Breakpoint 7, IA__gtk_widget_set_size_request (widget=0xcd94388,
> width=540,
> > height=267) at /build/buildd/gtk+2.0-2.20.1/gtk/gtkwidget.c:7946
> > $70 = (gchar *) 0xce5a2a0 "views-gtkwidget-child-fixed"
> > #0  IA__gtk_widget_set_size_request (widget=0xcd94388, width=540,
> > height=267) at
> > /build/buildd/gtk+2.0-2.20.1/gtk/gtkwidget.c:7946
> > #1  0x09bd07df in views::NativeViewHostGtk::ShowWidget (this=0xce84180,
> x=5,
> > y=23, w=540, h=267) at views/controls/native/native_view_host_gtk.cc:235
> > #2  0x09bcf629 in views::NativeViewHost::Layout (this=0xcde8e70) at
> > views/controls/native/native_view_host.cc:109
> >
> > Breakpoint 3, views::NativeViewHost::Layout (this=0xcde8e70) at
> > views/controls/native/native_view_host.cc:81
> > $71 = {
> >  origin_ = {
> >    x_ = 0,
> >    y_ = 0
> >  },
> >  size_ = {
> >    width_ = 540,
> >    height_ = 267
> >  }
> > }
> >
> > Breakpoint 7, IA__gtk_widget_set_size_request (widget=0xcd94340,
> width=540,
> > height=267) at /build/buildd/gtk+2.0-2.20.1/gtk/gtkwidget.c:7946
> > $72 = (gchar *) 0xce5a5a0 "chrome-render-widget-host-view"
> > #0  IA__gtk_widget_set_size_request (widget=0xcd94340, width=540,
> > height=267) at
> > /build/buildd/gtk+2.0-2.20.1/gtk/gtkwidget.c:7946
> > #1  0x09238e6b in RenderWidgetHostViewGtk::SetSize (this=0xce47680,
> > size=...) at
> > chrome/browser/renderer_host/render_widget_host_view_gtk.cc:577
> > #2  0x09372c19 in TabContentsViewGtk::WasSized (this=0xcda73c0, size=...)
> at
> > chrome/browser/views/tab_contents/tab_contents_view_gtk.cc:429
> >
> > Breakpoint 7, IA__gtk_widget_set_size_request (widget=0xcd94388,
> width=540,
> > height=267) at /build/buildd/gtk+2.0-2.20.1/gtk/gtkwidget.c:7946
> > $73 = (gchar *) 0xce5a2a0 "views-gtkwidget-child-fixed"
> > #0  IA__gtk_widget_set_size_request (widget=0xcd94388, width=540,
> > height=267) at
> > /build/buildd/gtk+2.0-2.20.1/gtk/gtkwidget.c:7946
> > #1  0x09bd07df in views::NativeViewHostGtk::ShowWidget (this=0xce84180,
> x=5,
> > y=23, w=540, h=267) at views/controls/native/native_view_host_gtk.cc:235
> > #2  0x09bcf629 in views::NativeViewHost::Layout (this=0xcde8e70) at
> > views/controls/native/native_view_host.cc:109
> >
> > Breakpoint 3, views::NativeViewHost::Layout (this=0xcde8e70) at
> > views/controls/native/native_view_host.cc:81
> > $74 = {
> >  origin_ = {
> >    x_ = 0,
> >    y_ = 0
> >  },
> >  size_ = {
> >    width_ = 540,
> >    height_ = 267
> >  }
> > }
> >
> > Breakpoint 7, IA__gtk_widget_set_size_request (widget=0xcd94388,
> width=540,
> > height=267) at /build/buildd/gtk+2.0-2.20.1/gtk/gtkwidget.c:7946
> > $75 = (gchar *) 0xce5a2a0 "views-gtkwidget-child-fixed"
> > #0  IA__gtk_widget_set_size_request (widget=0xcd94388, width=540,
> > height=267) at
> > /build/buildd/gtk+2.0-2.20.1/gtk/gtkwidget.c:7946
> > #1  0x09bd07df in views::NativeViewHostGtk::ShowWidget (this=0xce84180,
> x=5,
> > y=23, w=540, h=267) at views/controls/native/native_view_host_gtk.cc:235
> > #2  0x09bcf629 in views::NativeViewHost::Layout (this=0xcde8e70) at
> > views/controls/native/native_view_host.cc:109
> > chrome/browser/views/html_dialog_view_browsertest.cc:152: Failure
> > Value of: actual_bounds
> >  Actual: 1274,804 550x295
> > Expected: set_bounds
> > Which is: 1274,804 550x250
> >
> >
> > http://codereview.chromium.org/2768006/show
> >
>

[sky, 2010-08-11 19:58:27]

What's the best way to see the flickering? Just switch tabs?

  -Scott

On Wed, Aug 11, 2010 at 12:35 PM, Dmitry Polukhin
<dpolukhin@chromium.org> wrote:
> I had to revert my CL with gtk_views_fixed because of flickering of tab
> contents. I spent almost a week trying to replace gtk_fixed
> with gtk_views_fixed but I gave up when I realized that it results in ugly
> flickering. It looks like GTK lays out child windows in couple passes that
> results in flickering. The most ugly flickering I saw when I tried to resize
> Chrome window with omnibox. I observed flickering on tab switching also.
> On Wed, Aug 11, 2010 at 9:00 PM, Scott Violet <sky@chromium.org> wrote:
>>
>> I believe the problem that lead Dmitry to now use gtk_views_fixed was
>> some flickering that appeared because we were sizing the widget right
>> away (gtk_widget_size_allocate). Seems like the right fix is to not
>> size right away but instead stash the size in a different property.
>> This should fix the flicker and fix the bug you're seeing. You should
>> be able to stash the size using g_object_set_data. Does that make
>> sense?
>>
>> Dmitry, does this sounds good? Do you have a test case to make sure we
>> don't see any flicker?
>>
>>  -Scott
>>
>> On Tue, Aug 10, 2010 at 1:24 PM,  <scottbyer@chromium.org> wrote:
>> > Diving back into this.  TabContentsViewGtk::SizeContents is not being
>> > called.
>> > gtk_widget_set_size_request is being called for the
>> > chrome-render-widget-host-view from RenderWidgetHostViewGtk::SetSize for
>> > TOOLKIT_VIEWS builds only, but NativeViewHostGtk::ShowWidget is the only
>> > place
>> > it's being called on the views_gtkwidget-child-fixed widget.  Removing
>> > the
>> > call
>> > from RWHVG::SetSize has no affect on the problem.
>> >
>> > I've included two traces - one for the first test, which is sizing the
>> > dialog
>> > larger, and the second test which is trying to shrink the height (which
>> > fails).
>> >
>> > My instinct at this point is to revert
>> > http://codereview.chromium.org/2131021,
>> > remove the gtk_widget_set_size_request from RWHVG::SetSize, reproduce
>> > the
>> > flickering problem (which I'm pretty sure is actually due to the
>> > bouncing
>> > resize
>> > requests as shown in the trace below), and remove the bouncing resize
>> > requests
>> > by fixing how the NativeViewHost::Layout happens, but I'm looking for
>> > guidance
>> > on that.
>> >
>> > -Scott
>> >
>> > =============================
>> > Test 1, sizing the dialog to 400x300:
>> >
>> > Breakpoint 6, HtmlDialogBrowserTest_SizeWindow_Test::RunTestOnMainThread
>> > (this=0xcbd22d0) at
>> > chrome/browser/views/html_dialog_view_browsertest.cc:137
>> > $53 = "STARTING WINDOW"
>> >
>> > Breakpoint 2, views::WidgetGtk::OnSizeAllocate (this=0xce1cc00,
>> > widget=0xcdcfcc0, allocation=0xffffb780) at
>> > views/widget/widget_gtk.cc:944
>> > $54 = {
>> >  x = 0,
>> >  y = 0,
>> >  width = 400,
>> >  height = 300
>> > }
>> >
>> > Breakpoint 3, views::NativeViewHost::Layout (this=0xcde8e70) at
>> > views/controls/native/native_view_host.cc:81
>> > $55 = {
>> >  origin_ = {
>> >    x_ = 0,
>> >    y_ = 0
>> >  },
>> >  size_ = {
>> >    width_ = 1,
>> >    height_ = 1
>> >  }
>> > }
>> >
>> > Breakpoint 7, IA__gtk_widget_set_size_request (widget=0xcd94388,
>> > width=1,
>> > height=1) at /build/buildd/gtk+2.0-2.20.1/gtk/gtkwidget.c:7946
>> > $56 = (gchar *) 0xce536a0 "views-gtkwidget-child-fixed"
>> > #0  IA__gtk_widget_set_size_request (widget=0xcd94388, width=1,
>> > height=1) at
>> > /build/buildd/gtk+2.0-2.20.1/gtk/gtkwidget.c:7946
>> > #1  0x09bd07df in views::NativeViewHostGtk::ShowWidget (this=0xce74c60,
>> > x=5,
>> > y=23, w=1, h=1) at views/controls/native/native_view_host_gtk.cc:235
>> > #2  0x09bcf629 in views::NativeViewHost::Layout (this=0xcde8e70) at
>> > views/controls/native/native_view_host.cc:109
>> >
>> > Breakpoint 3, views::NativeViewHost::Layout (this=0xcde8e70) at
>> > views/controls/native/native_view_host.cc:81
>> > $57 = {
>> >  origin_ = {
>> >    x_ = 0,
>> >    y_ = 0
>> >  },
>> >  size_ = {
>> >    width_ = 390,
>> >    height_ = 272
>> >  }
>> > }
>> >
>> > Breakpoint 7, IA__gtk_widget_set_size_request (widget=0xcd94388,
>> > width=390,
>> > height=272) at /build/buildd/gtk+2.0-2.20.1/gtk/gtkwidget.c:7946
>> > $58 = (gchar *) 0xce536a0 "views-gtkwidget-child-fixed"
>> > #0  IA__gtk_widget_set_size_request (widget=0xcd94388, width=390,
>> > height=272) at
>> > /build/buildd/gtk+2.0-2.20.1/gtk/gtkwidget.c:7946
>> > #1  0x09bd07df in views::NativeViewHostGtk::ShowWidget (this=0xce74c60,
>> > x=5,
>> > y=23, w=390, h=272) at views/controls/native/native_view_host_gtk.cc:235
>> > #2  0x09bcf629 in views::NativeViewHost::Layout (this=0xcde8e70) at
>> > views/controls/native/native_view_host.cc:109
>> >
>> > Breakpoint 3, views::NativeViewHost::Layout (this=0xcde8e70) at
>> > views/controls/native/native_view_host.cc:81
>> > $59 = {
>> >  origin_ = {
>> >    x_ = 0,
>> >    y_ = 0
>> >  },
>> >  size_ = {
>> >    width_ = 390,
>> >    height_ = 272
>> >  }
>> > }
>> >
>> > Breakpoint 7, IA__gtk_widget_set_size_request (widget=0xcd94340,
>> > width=390,
>> > height=272) at /build/buildd/gtk+2.0-2.20.1/gtk/gtkwidget.c:7946
>> > $60 = (gchar *) 0xce539a0 "chrome-render-widget-host-view"
>> > #0  IA__gtk_widget_set_size_request (widget=0xcd94340, width=390,
>> > height=272) at
>> > /build/buildd/gtk+2.0-2.20.1/gtk/gtkwidget.c:7946
>> > #1  0x09238e6b in RenderWidgetHostViewGtk::SetSize (this=0xce731a0,
>> > size=...) at
>> > chrome/browser/renderer_host/render_widget_host_view_gtk.cc:577
>> > #2  0x09372c19 in TabContentsViewGtk::WasSized (this=0xcda73c0,
>> > size=...) at
>> > chrome/browser/views/tab_contents/tab_contents_view_gtk.cc:429
>> >
>> > Breakpoint 7, IA__gtk_widget_set_size_request (widget=0xcd94388,
>> > width=390,
>> > height=272) at /build/buildd/gtk+2.0-2.20.1/gtk/gtkwidget.c:7946
>> > $61 = (gchar *) 0xce536a0 "views-gtkwidget-child-fixed"
>> > #0  IA__gtk_widget_set_size_request (widget=0xcd94388, width=390,
>> > height=272) at
>> > /build/buildd/gtk+2.0-2.20.1/gtk/gtkwidget.c:7946
>> > #1  0x09bd07df in views::NativeViewHostGtk::ShowWidget (this=0xce74c60,
>> > x=5,
>> > y=23, w=390, h=272) at views/controls/native/native_view_host_gtk.cc:235
>> > #2  0x09bcf629 in views::NativeViewHost::Layout (this=0xcde8e70) at
>> > views/controls/native/native_view_host.cc:109
>> >
>> > Breakpoint 3, views::NativeViewHost::Layout (this=0xcde8e70) at
>> > views/controls/native/native_view_host.cc:81
>> > $62 = {
>> >  origin_ = {
>> >    x_ = 0,
>> >    y_ = 0
>> >  },
>> >  size_ = {
>> >    width_ = 390,
>> >    height_ = 272
>> >  }
>> > }
>> >
>> > Breakpoint 7, IA__gtk_widget_set_size_request (widget=0xcd94388,
>> > width=390,
>> > height=272) at /build/buildd/gtk+2.0-2.20.1/gtk/gtkwidget.c:7946
>> > $63 = (gchar *) 0xce536a0 "views-gtkwidget-child-fixed"
>> > #0  IA__gtk_widget_set_size_request (widget=0xcd94388, width=390,
>> > height=272) at
>> > /build/buildd/gtk+2.0-2.20.1/gtk/gtkwidget.c:7946
>> > #1  0x09bd07df in views::NativeViewHostGtk::ShowWidget (this=0xce74c60,
>> > x=5,
>> > y=23, w=390, h=272) at views/controls/native/native_view_host_gtk.cc:235
>> > #2  0x09bcf629 in views::NativeViewHost::Layout (this=0xcde8e70) at
>> > views/controls/native/native_view_host.cc:109
>> >
>> >
>> >
>> > ============================
>> > Test 2, sizing the dialog to 500x250:
>> >
>> > Breakpoint 10,
>> > HtmlDialogBrowserTest_SizeWindow_Test::RunTestOnMainThread
>> > (this=0xcbd22d0) at
>> > chrome/browser/views/html_dialog_view_browsertest.cc:147
>> > $65 = "STARTING"
>> >
>> > Breakpoint 2, views::WidgetGtk::OnSizeAllocate (this=0xce3fc00,
>> > widget=0xcda8cc0, allocation=0xffffb780) at
>> > views/widget/widget_gtk.cc:944
>> > $66 = {
>> >  x = 0,
>> >  y = 0,
>> >  width = 550,
>> >  height = 295
>> > }
>> >
>> > Breakpoint 3, views::NativeViewHost::Layout (this=0xcde8e70) at
>> > views/controls/native/native_view_host.cc:81
>> > $67 = {
>> >  origin_ = {
>> >    x_ = 0,
>> >    y_ = 0
>> >  },
>> >  size_ = {
>> >    width_ = 390,
>> >    height_ = 267
>> >  }
>> > }
>> >
>> > Breakpoint 7, IA__gtk_widget_set_size_request (widget=0xcd94388,
>> > width=390,
>> > height=272) at /build/buildd/gtk+2.0-2.20.1/gtk/gtkwidget.c:7946
>> > $68 = (gchar *) 0xce5a2a0 "views-gtkwidget-child-fixed"
>> > #0  IA__gtk_widget_set_size_request (widget=0xcd94388, width=390,
>> > height=272) at
>> > /build/buildd/gtk+2.0-2.20.1/gtk/gtkwidget.c:7946
>> > #1  0x09bd07df in views::NativeViewHostGtk::ShowWidget (this=0xce84180,
>> > x=5,
>> > y=23, w=390, h=272) at views/controls/native/native_view_host_gtk.cc:235
>> > #2  0x09bcf629 in views::NativeViewHost::Layout (this=0xcde8e70) at
>> > views/controls/native/native_view_host.cc:109
>> >
>> > Breakpoint 3, views::NativeViewHost::Layout (this=0xcde8e70) at
>> > views/controls/native/native_view_host.cc:81
>> > $69 = {
>> >  origin_ = {
>> >    x_ = 0,
>> >    y_ = 0
>> >  },
>> >  size_ = {
>> >    width_ = 540,
>> >    height_ = 267
>> >  }
>> > }
>> >
>> > Breakpoint 7, IA__gtk_widget_set_size_request (widget=0xcd94388,
>> > width=540,
>> > height=267) at /build/buildd/gtk+2.0-2.20.1/gtk/gtkwidget.c:7946
>> > $70 = (gchar *) 0xce5a2a0 "views-gtkwidget-child-fixed"
>> > #0  IA__gtk_widget_set_size_request (widget=0xcd94388, width=540,
>> > height=267) at
>> > /build/buildd/gtk+2.0-2.20.1/gtk/gtkwidget.c:7946
>> > #1  0x09bd07df in views::NativeViewHostGtk::ShowWidget (this=0xce84180,
>> > x=5,
>> > y=23, w=540, h=267) at views/controls/native/native_view_host_gtk.cc:235
>> > #2  0x09bcf629 in views::NativeViewHost::Layout (this=0xcde8e70) at
>> > views/controls/native/native_view_host.cc:109
>> >
>> > Breakpoint 3, views::NativeViewHost::Layout (this=0xcde8e70) at
>> > views/controls/native/native_view_host.cc:81
>> > $71 = {
>> >  origin_ = {
>> >    x_ = 0,
>> >    y_ = 0
>> >  },
>> >  size_ = {
>> >    width_ = 540,
>> >    height_ = 267
>> >  }
>> > }
>> >
>> > Breakpoint 7, IA__gtk_widget_set_size_request (widget=0xcd94340,
>> > width=540,
>> > height=267) at /build/buildd/gtk+2.0-2.20.1/gtk/gtkwidget.c:7946
>> > $72 = (gchar *) 0xce5a5a0 "chrome-render-widget-host-view"
>> > #0  IA__gtk_widget_set_size_request (widget=0xcd94340, width=540,
>> > height=267) at
>> > /build/buildd/gtk+2.0-2.20.1/gtk/gtkwidget.c:7946
>> > #1  0x09238e6b in RenderWidgetHostViewGtk::SetSize (this=0xce47680,
>> > size=...) at
>> > chrome/browser/renderer_host/render_widget_host_view_gtk.cc:577
>> > #2  0x09372c19 in TabContentsViewGtk::WasSized (this=0xcda73c0,
>> > size=...) at
>> > chrome/browser/views/tab_contents/tab_contents_view_gtk.cc:429
>> >
>> > Breakpoint 7, IA__gtk_widget_set_size_request (widget=0xcd94388,
>> > width=540,
>> > height=267) at /build/buildd/gtk+2.0-2.20.1/gtk/gtkwidget.c:7946
>> > $73 = (gchar *) 0xce5a2a0 "views-gtkwidget-child-fixed"
>> > #0  IA__gtk_widget_set_size_request (widget=0xcd94388, width=540,
>> > height=267) at
>> > /build/buildd/gtk+2.0-2.20.1/gtk/gtkwidget.c:7946
>> > #1  0x09bd07df in views::NativeViewHostGtk::ShowWidget (this=0xce84180,
>> > x=5,
>> > y=23, w=540, h=267) at views/controls/native/native_view_host_gtk.cc:235
>> > #2  0x09bcf629 in views::NativeViewHost::Layout (this=0xcde8e70) at
>> > views/controls/native/native_view_host.cc:109
>> >
>> > Breakpoint 3, views::NativeViewHost::Layout (this=0xcde8e70) at
>> > views/controls/native/native_view_host.cc:81
>> > $74 = {
>> >  origin_ = {
>> >    x_ = 0,
>> >    y_ = 0
>> >  },
>> >  size_ = {
>> >    width_ = 540,
>> >    height_ = 267
>> >  }
>> > }
>> >
>> > Breakpoint 7, IA__gtk_widget_set_size_request (widget=0xcd94388,
>> > width=540,
>> > height=267) at /build/buildd/gtk+2.0-2.20.1/gtk/gtkwidget.c:7946
>> > $75 = (gchar *) 0xce5a2a0 "views-gtkwidget-child-fixed"
>> > #0  IA__gtk_widget_set_size_request (widget=0xcd94388, width=540,
>> > height=267) at
>> > /build/buildd/gtk+2.0-2.20.1/gtk/gtkwidget.c:7946
>> > #1  0x09bd07df in views::NativeViewHostGtk::ShowWidget (this=0xce84180,
>> > x=5,
>> > y=23, w=540, h=267) at views/controls/native/native_view_host_gtk.cc:235
>> > #2  0x09bcf629 in views::NativeViewHost::Layout (this=0xcde8e70) at
>> > views/controls/native/native_view_host.cc:109
>> > chrome/browser/views/html_dialog_view_browsertest.cc:152: Failure
>> > Value of: actual_bounds
>> >  Actual: 1274,804 550x295
>> > Expected: set_bounds
>> > Which is: 1274,804 550x250
>> >
>> >
>> > http://codereview.chromium.org/2768006/show
>> >
>
>

[Dmitry Polukhin, 2010-08-12 19:02:03]

Yes, and also try to resize Chrome window. Also one of interactive UI test
failed (unfortunatly I don't remember which one).

Sent from Android phone.

On Aug 11, 2010 11:58 PM, "Scott Violet" <sky@chromium.org> wrote:

What's the best way to see the flickering? Just switch tabs?

 -Scott


On Wed, Aug 11, 2010 at 12:35 PM, Dmitry Polukhin
<dpolukhin@chromium.org> wrote:
> I had to revert...

[Scott Byer, 2010-08-18 00:13:09]

Ok, I had the extra properties stuff all implemented, had it all working, and
was debugging into the GTK source when it hit me; the problem with the original
patch was that it was re-triggering window layout while in the middle of window
layout.  This, I'm pretty sure, caused the flashing.

Instead, by just setting the allocation, indicating that the allocation should
be used for the size, and queuing up the size change, we don't get into the
situation that caused the flashing in the first place, and avoid the
gtk_widget_set_size_request which causes the problems with the window not being
able to shrink.  This also allows for the use of GtkViewsFixed instead of
GtkFixed.

(This patch also has appropriate event triggers in the browser test for quitting
the event loop after the window sizing).

I've tested against the flashing scenarios, and have been running through the
unit tests - paying particular attention to the interactive_ui_tests and
browser_tests - and everything seems OK so far, but notes on any additional
concerns or places I should look into would be appreciated.

(I currently have the extra properties patch tucked off in an intermediate git
commit if anyone is interested, but it's not nearly as elegant a fix, but it
does preserve ).

I'm also concerned that the TabContentsViewGtk::GetContainerBounds views version
is quite wrong, but it's not showing up in a failed test anywhere.  Thoughts? 
I'm thinking it should copy the requested_size_ pattern of the non-views
version.

-Scott


On 2010/08/12 19:02:03, Dmitry Polukhin wrote:
> Yes, and also try to resize Chrome window. Also one of interactive UI test
> failed (unfortunatly I don't remember which one).
> 
> Sent from Android phone.
> 
> On Aug 11, 2010 11:58 PM, "Scott Violet" <mailto:sky@chromium.org> wrote:
> 
> What's the best way to see the flickering? Just switch tabs?
> 
>  -Scott
> 
> 
> On Wed, Aug 11, 2010 at 12:35 PM, Dmitry Polukhin
> <mailto:dpolukhin@chromium.org> wrote:
> > I had to revert...
>

[Paweł Hajdan Jr., 2010-08-18 00:18:30]

The waiting logic looks much better to me. Thanks for the hard work!

http://codereview.chromium.org/2768006/diff/40001/41001
File chrome/browser/views/html_dialog_view_browsertest.cc (right):

http://codereview.chromium.org/2768006/diff/40001/41001#newcode70
chrome/browser/views/html_dialog_view_browsertest.cc:70: // Post a delayed quit
just as a backstop.  We should always get
If this runs inside browser_tests, you automatically get a bulletproof timeout.

Please remove this hardcoded timeout from the code, we should avoid introducing
more kinds of timeouts.

http://codereview.chromium.org/2768006/diff/40001/41001#newcode77
chrome/browser/views/html_dialog_view_browsertest.cc:77: #if  defined(OS_WIN)
nit: Just one space after #if.

http://codereview.chromium.org/2768006/diff/40001/41001#newcode124
chrome/browser/views/html_dialog_view_browsertest.cc:124: #if defined(OS_LINUX)
nit: Remove empty lines inside this #if.

http://codereview.chromium.org/2768006/diff/40001/41001#newcode133
chrome/browser/views/html_dialog_view_browsertest.cc:133: #define
MAYBE_SizeWindow DISABLED_SizeWindow
Every disabled test should have a bug on file with Tests-Disabled label. The
link to the bug should be added as a comment here.

[sky, 2010-08-18 15:45:57]

http://codereview.chromium.org/2768006/diff/40001/41003
File views/controls/native/native_view_host_gtk.cc (right):

http://codereview.chromium.org/2768006/diff/40001/41003#newcode236
views/controls/native/native_view_host_gtk.cc:236:
gtk_widget_set_allocation(host_->native_view(), &allocation);
I'm worried about using this for two reasons:

. It's only in 2.18.
. The docs say it shouldn't be called directly.

I think it safer to stash the size we want in an property that gtk_views_fixed
looks for.

[Scott Byer, 2010-08-18 16:50:01]

No problem, allocation-as-property was what I had earlier, easy to bring back
and clean up here.  gtk_widget_set_allocation really just does
widget->allocation = *allocation, but I can understand having a healthy mistrust
of GTK.  What that code does with requisition sizes and the hidden auxiliary
size fields... I feel better having gotten the naked GtkFixed out of the view
hierarchy.

On 2010/08/18 15:45:57, sky wrote:
> http://codereview.chromium.org/2768006/diff/40001/41003
> File views/controls/native/native_view_host_gtk.cc (right):
> 
> http://codereview.chromium.org/2768006/diff/40001/41003#newcode236
> views/controls/native/native_view_host_gtk.cc:236:
> gtk_widget_set_allocation(host_->native_view(), &allocation);
> I'm worried about using this for two reasons:
> 
> . It's only in 2.18.
> . The docs say it shouldn't be called directly.
> 
> I think it safer to stash the size we want in an property that gtk_views_fixed
> looks for.

http://codereview.chromium.org/2768006/diff/40001/41001
File chrome/browser/views/html_dialog_view_browsertest.cc (right):

http://codereview.chromium.org/2768006/diff/40001/41001#newcode70
chrome/browser/views/html_dialog_view_browsertest.cc:70: // Post a delayed quit
just as a backstop.  We should always get
On 2010/08/18 00:18:30, Paweł Hajdan Jr. wrote:
> If this runs inside browser_tests, you automatically get a bulletproof
timeout.
> 
> Please remove this hardcoded timeout from the code, we should avoid
introducing
> more kinds of timeouts.

Done.

http://codereview.chromium.org/2768006/diff/40001/41001#newcode77
chrome/browser/views/html_dialog_view_browsertest.cc:77: #if  defined(OS_WIN)
On 2010/08/18 00:18:30, Paweł Hajdan Jr. wrote:
> nit: Just one space after #if.

Done.

http://codereview.chromium.org/2768006/diff/40001/41001#newcode124
chrome/browser/views/html_dialog_view_browsertest.cc:124: #if defined(OS_LINUX)
On 2010/08/18 00:18:30, Paweł Hajdan Jr. wrote:
> nit: Remove empty lines inside this #if.

Done.

http://codereview.chromium.org/2768006/diff/40001/41001#newcode133
chrome/browser/views/html_dialog_view_browsertest.cc:133: #define
MAYBE_SizeWindow DISABLED_SizeWindow
On 2010/08/18 00:18:30, Paweł Hajdan Jr. wrote:
> Every disabled test should have a bug on file with Tests-Disabled label. The
> link to the bug should be added as a comment here.

Done.

[sky, 2010-08-18 17:11:43]

http://codereview.chromium.org/2768006/diff/46001/36005
File views/widget/gtk_views_fixed.cc (right):

http://codereview.chromium.org/2768006/diff/46001/36005#newcode30
views/widget/gtk_views_fixed.cc:30: int request_width, request_height;
Shouldn't this be on the inner loop per child?

http://codereview.chromium.org/2768006/diff/46001/36005#newcode104
views/widget/gtk_views_fixed.cc:104: gtk_widget_set_allocation(widget,
&allocation);
Why does this need to set and get the current allocation? Shouldn't the code
work without lines 100-104?

http://codereview.chromium.org/2768006/diff/46001/36006
File views/widget/gtk_views_fixed.h (right):

http://codereview.chromium.org/2768006/diff/46001/36006#newcode46
views/widget/gtk_views_fixed.h:46: void
gtk_views_fixed_set_use_allocated_size(GtkWidget* widget, bool value);
These three methods should be combined so that only have something like:

gtk_views_fixed_set_widget_size(GtkWidget* widget, int w, int h);


bool gtk_views_fixed_get_widget_size(GtkWidget* widget, int* w, int* h);

[Paweł Hajdan Jr., 2010-08-18 18:12:07]

Test code LGTM.

[Scott Byer, 2010-08-18 18:15:49]

Is this more like what you're looking for?  This sidesteps widget->allocation
completely, and avoids the call to gtk_widget_size_allocation on
NativeViewHostGtk.fixed_ at a dangerous time.

-Scott

On 2010/08/18 17:11:43, sky wrote:
> http://codereview.chromium.org/2768006/diff/46001/36005
> File views/widget/gtk_views_fixed.cc (right):
> 
> http://codereview.chromium.org/2768006/diff/46001/36005#newcode30
> views/widget/gtk_views_fixed.cc:30: int request_width, request_height;
> Shouldn't this be on the inner loop per child?
> 
> http://codereview.chromium.org/2768006/diff/46001/36005#newcode104
> views/widget/gtk_views_fixed.cc:104: gtk_widget_set_allocation(widget,
> &allocation);
> Why does this need to set and get the current allocation? Shouldn't the code
> work without lines 100-104?
> 
> http://codereview.chromium.org/2768006/diff/46001/36006
> File views/widget/gtk_views_fixed.h (right):
> 
> http://codereview.chromium.org/2768006/diff/46001/36006#newcode46
> views/widget/gtk_views_fixed.h:46: void
> gtk_views_fixed_set_use_allocated_size(GtkWidget* widget, bool value);
> These three methods should be combined so that only have something like:
> 
> gtk_views_fixed_set_widget_size(GtkWidget* widget, int w, int h);
> 
> 
> bool gtk_views_fixed_get_widget_size(GtkWidget* widget, int* w, int* h);

[sky, 2010-08-18 18:22:28]

Yay, this looks good. My only request is to improve the comments, then LGTM

http://codereview.chromium.org/2768006/diff/53001/41007
File views/controls/native/native_view_host_gtk.cc (right):

http://codereview.chromium.org/2768006/diff/53001/41007#newcode232
views/controls/native/native_view_host_gtk.cc:232: // Set the allocation and
queue up the resize.  We're likely in the
Update the doc as this doesn't set the allocation or queue up the resize.

http://codereview.chromium.org/2768006/diff/53001/41009
File views/widget/gtk_views_fixed.h (right):

http://codereview.chromium.org/2768006/diff/53001/41009#newcode13
views/widget/gtk_views_fixed.h:13: // current size rather than their requested
size. This behavior is controlled
Please update the doc appropriately.

[Scott Byer, 2010-08-18 18:58:30]

http://codereview.chromium.org/2768006/diff/53001/41007
File views/controls/native/native_view_host_gtk.cc (right):

http://codereview.chromium.org/2768006/diff/53001/41007#newcode232
views/controls/native/native_view_host_gtk.cc:232: // Set the allocation and
queue up the resize.  We're likely in the
On 2010/08/18 18:22:28, sky wrote:
> Update the doc as this doesn't set the allocation or queue up the resize.

Right now I do still have the gtk_widget_queue_resize() call in
gtk_views_fixed_set_widget_size().  My inclination is to keep it there - worth
still noting in the comment?

http://codereview.chromium.org/2768006/diff/53001/41009
File views/widget/gtk_views_fixed.h (right):

http://codereview.chromium.org/2768006/diff/53001/41009#newcode13
views/widget/gtk_views_fixed.h:13: // current size rather than their requested
size. This behavior is controlled
On 2010/08/18 18:22:28, sky wrote:
> Please update the doc appropriately.

Done.

[sky, 2010-08-18 20:40:53]

On Wed, Aug 18, 2010 at 11:58 AM,  <scottbyer@chromium.org> wrote:
>
> http://codereview.chromium.org/2768006/diff/53001/41007
> File views/controls/native/native_view_host_gtk.cc (right):
>
> http://codereview.chromium.org/2768006/diff/53001/41007#newcode232
> views/controls/native/native_view_host_gtk.cc:232: // Set the allocation
> and queue up the resize.  We're likely in the
> On 2010/08/18 18:22:28, sky wrote:
>>
>> Update the doc as this doesn't set the allocation or queue up the
>
> resize.
>
> Right now I do still have the gtk_widget_queue_resize() call in
> gtk_views_fixed_set_widget_size().  My inclination is to keep it there -
> worth still noting in the comment?

I agree, gtk_widget_queue_resize() should stay in
gtk_views_fixed_set_widget_size. You should document that
gtk_views_fixed_set_widget_size does the gtk_widget_queue_resize.

> http://codereview.chromium.org/2768006/diff/53001/41009
> File views/widget/gtk_views_fixed.h (right):
>
> http://codereview.chromium.org/2768006/diff/53001/41009#newcode13
> views/widget/gtk_views_fixed.h:13: // current size rather than their
> requested size. This behavior is controlled
> On 2010/08/18 18:22:28, sky wrote:
>>
>> Please update the doc appropriately.
>
> Done.
>
> http://codereview.chromium.org/2768006/show
>

[Scott Byer, 2010-08-18 23:22:51]

Added in the queue resize notes to the comments; found a couple of compile +
test issues and fixed those.

http://codereview.chromium.org/2768006/diff/62009/70002
File chrome/browser/views/tab_contents/tab_contents_view_gtk.cc (right):

http://codereview.chromium.org/2768006/diff/62009/70002#newcode230
chrome/browser/views/tab_contents/tab_contents_view_gtk.cc:230: }
I was right to be worried about this routine; issue caught by
TabDraggingTest.Tab2OutOfTabStrip on a views build (but not by design, by a
compounded bug).

http://codereview.chromium.org/2768006/diff/62009/70003
File chrome/browser/views/tabs/dragged_tab_view.cc (right):

http://codereview.chromium.org/2768006/diff/62009/70003#newcode167
chrome/browser/views/tabs/dragged_tab_view.cc:167: ps.height() -
kDragFrameBorderSize - tab_size_.height();
Take a look at ::GetPreferredSize above.  If the contents_size_.height was zero,
then image_h ended up negative, and chrome crashed.  This was accidentally
exposed by the problem with TabContentsViewGtk::GetContainerSize.

[sky, 2010-08-18 23:51:49]

http://codereview.chromium.org/2768006/diff/62009/70002
File chrome/browser/views/tab_contents/tab_contents_view_gtk.cc (right):

http://codereview.chromium.org/2768006/diff/62009/70002#newcode230
chrome/browser/views/tab_contents/tab_contents_view_gtk.cc:230: }
On 2010/08/18 23:22:51, Scott Byer wrote:
> I was right to be worried about this routine; issue caught by
> TabDraggingTest.Tab2OutOfTabStrip on a views build (but not by design, by a
> compounded bug).

This assumes WidgetGtk uses a gtk_views_fixed, which is easy to break. Could you
promote this as a method to WidgetGtk, say GetRequestedSize or something.

http://codereview.chromium.org/2768006/diff/62009/70003
File chrome/browser/views/tabs/dragged_tab_view.cc (right):

http://codereview.chromium.org/2768006/diff/62009/70003#newcode167
chrome/browser/views/tabs/dragged_tab_view.cc:167: ps.height() -
kDragFrameBorderSize - tab_size_.height();
On 2010/08/18 23:22:51, Scott Byer wrote:
> Take a look at ::GetPreferredSize above.  If the contents_size_.height was
zero,
> then image_h ended up negative, and chrome crashed.  This was accidentally
> exposed by the problem with TabContentsViewGtk::GetContainerSize.

Can this be contents_size_.height()?

http://codereview.chromium.org/2768006/diff/62009/70008
File views/widget/widget_gtk.cc (right):

http://codereview.chromium.org/2768006/diff/62009/70008#newcode344
views/widget/widget_gtk.cc:344: gtk_widget_size_allocate(child, &alloc);
Can we remove the size_allocate here now?

[Scott Byer, 2010-08-19 20:47:38]

http://codereview.chromium.org/2768006/diff/62009/70002
File chrome/browser/views/tab_contents/tab_contents_view_gtk.cc (right):

http://codereview.chromium.org/2768006/diff/62009/70002#newcode230
chrome/browser/views/tab_contents/tab_contents_view_gtk.cc:230: }
On 2010/08/18 23:51:49, sky wrote:
> On 2010/08/18 23:22:51, Scott Byer wrote:
> > I was right to be worried about this routine; issue caught by
> > TabDraggingTest.Tab2OutOfTabStrip on a views build (but not by design, by a
> > compounded bug).
> 
> This assumes WidgetGtk uses a gtk_views_fixed, which is easy to break. Could
you
> promote this as a method to WidgetGtk, say GetRequestedSize or something.

Done.

http://codereview.chromium.org/2768006/diff/62009/70003
File chrome/browser/views/tabs/dragged_tab_view.cc (right):

http://codereview.chromium.org/2768006/diff/62009/70003#newcode167
chrome/browser/views/tabs/dragged_tab_view.cc:167: ps.height() -
kDragFrameBorderSize - tab_size_.height();
On 2010/08/18 23:51:49, sky wrote:
> On 2010/08/18 23:22:51, Scott Byer wrote:
> > Take a look at ::GetPreferredSize above.  If the contents_size_.height was
> zero,
> > then image_h ended up negative, and chrome crashed.  This was accidentally
> > exposed by the problem with TabContentsViewGtk::GetContainerSize.
> 
> Can this be contents_size_.height()?

Done.

http://codereview.chromium.org/2768006/diff/62009/70008
File views/widget/widget_gtk.cc (right):

http://codereview.chromium.org/2768006/diff/62009/70008#newcode344
views/widget/widget_gtk.cc:344: gtk_widget_size_allocate(child, &alloc);
On 2010/08/18 23:51:49, sky wrote:
> Can we remove the size_allocate here now?

Done.  I fixed the unit_test that needed it.

[sky, 2010-08-19 21:10:33]

Do the following change, and LGTM

http://codereview.chromium.org/2768006/diff/68002/41020
File views/widget/widget_gtk.h (right):

http://codereview.chromium.org/2768006/diff/68002/41020#newcode155
views/widget/widget_gtk.h:155: void GetRequestedSize(gfx::Rect* out) const;
Make this return a gfx::Size

[Scott Byer, 2010-08-19 21:58:18]

http://codereview.chromium.org/2768006/diff/68002/41020
File views/widget/widget_gtk.h (right):

http://codereview.chromium.org/2768006/diff/68002/41020#newcode155
views/widget/widget_gtk.h:155: void GetRequestedSize(gfx::Rect* out) const;
On 2010/08/19 21:10:33, sky wrote:
> Make this return a gfx::Size

Done.

[sky, 2010-08-19 22:00:14]

LGTM

[sky, 2010-08-19 22:00:29]

And thanks for seeing the right fix through!

  -Scott

[oshima, 2010-08-24 19:17:32]

scottbyer, looks like this borke WidgetGtk::Init and causing crosbug.com/5987.
can you look into it?