Minimize resize calls

Minimize resize calls


BUG=None
TEST=None

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

[DaveMoore, 2011-10-27 00:44:04]

These changes reduce the number of calls for each resize in the shell from 11 to
2 (which is necessary because of the way that we constrain the position)

[sky, 2011-10-27 14:40:47]

http://codereview.chromium.org/8340014/diff/7/views/controls/native/native_vi...
File views/controls/native/native_view_host.cc (right):

http://codereview.chromium.org/8340014/diff/7/views/controls/native/native_vi...
views/controls/native/native_view_host.cc:158: return false;
This comment seems to apply to aura. Why do we want to return false here?

[DaveMoore, 2011-10-27 21:25:13]

http://codereview.chromium.org/8340014/diff/7/views/controls/native/native_vi...
File views/controls/native/native_view_host.cc (right):

http://codereview.chromium.org/8340014/diff/7/views/controls/native/native_vi...
views/controls/native/native_view_host.cc:158: return false;
Do we need this when there's only one native window to host everything? What I
saw was that this gets triggered by the bounds change and then it causes us to
do another layout. I saw no downside on the device when I changed this.

On 2011/10/27 14:40:47, sky wrote:
> This comment seems to apply to aura. Why do we want to return false here?

[sky, 2011-10-27 22:12:20]

http://codereview.chromium.org/8340014/diff/7/views/controls/native/native_vi...
File views/controls/native/native_view_host.cc (right):

http://codereview.chromium.org/8340014/diff/7/views/controls/native/native_vi...
views/controls/native/native_view_host.cc:158: return false;
On 2011/10/27 21:25:13, DaveMoore wrote:
> Do we need this when there's only one native window to host everything?

We still have child windows for the TabContents and RWHV. If we had a situation
where the position in the parent changed but the bounds didn't, then we would
want this code.

> What I
> saw was that this gets triggered by the bounds change and then it causes us to
> do another layout. I saw no downside on the device when I changed this.

Shouldn't it result in a no-op because we invoke SetBounds with the same bounds,
which because of your change to Window does nothing?

> 
> On 2011/10/27 14:40:47, sky wrote:
> > This comment seems to apply to aura. Why do we want to return false here?
>

[DaveMoore, 2011-10-27 22:15:00]

It gets short circuited there, but layout() is called which needs to do
unnecessary calculations..what case are you worried about breaking?

On 2011/10/27 22:12:20, sky wrote:
>
http://codereview.chromium.org/8340014/diff/7/views/controls/native/native_vi...
> File views/controls/native/native_view_host.cc (right):
> 
>
http://codereview.chromium.org/8340014/diff/7/views/controls/native/native_vi...
> views/controls/native/native_view_host.cc:158: return false;
> On 2011/10/27 21:25:13, DaveMoore wrote:
> > Do we need this when there's only one native window to host everything?
> 
> We still have child windows for the TabContents and RWHV. If we had a
situation
> where the position in the parent changed but the bounds didn't, then we would
> want this code.
> 
> > What I
> > saw was that this gets triggered by the bounds change and then it causes us
to
> > do another layout. I saw no downside on the device when I changed this.
> 
> Shouldn't it result in a no-op because we invoke SetBounds with the same
bounds,
> which because of your change to Window does nothing?
> 
> > 
> > On 2011/10/27 14:40:47, sky wrote:
> > > This comment seems to apply to aura. Why do we want to return false here?
> >

[sky, 2011-10-27 22:28:00]

On Thu, Oct 27, 2011 at 3:15 PM,  <davemoore@chromium.org> wrote:
> It gets short circuited there, but layout() is called which needs to do
> unnecessary calculations..

But those calculations are quite cheap. It's just walking the tree
figuring the bounds.

> what case are you worried about breaking?

It would be easy for someone to add something that requires this code.
If you disable it we'll then end up with subtle bugs in such a
situation. I could see making the change if the code path were
expensive, but it doesn't seem expensive to me.

  -Scott

>
> On 2011/10/27 22:12:20, sky wrote:
>
>
http://codereview.chromium.org/8340014/diff/7/views/controls/native/native_vi...
>>
>> File views/controls/native/native_view_host.cc (right):
>
>
>
http://codereview.chromium.org/8340014/diff/7/views/controls/native/native_vi...
>>
>> views/controls/native/native_view_host.cc:158: return false;
>> On 2011/10/27 21:25:13, DaveMoore wrote:
>> > Do we need this when there's only one native window to host everything?
>
>> We still have child windows for the TabContents and RWHV. If we had a
>
> situation
>>
>> where the position in the parent changed but the bounds didn't, then we
>> would
>> want this code.
>
>> > What I
>> > saw was that this gets triggered by the bounds change and then it causes
>> > us
>
> to
>>
>> > do another layout. I saw no downside on the device when I changed this.
>
>> Shouldn't it result in a no-op because we invoke SetBounds with the same
>
> bounds,
>>
>> which because of your change to Window does nothing?
>
>> >
>> > On 2011/10/27 14:40:47, sky wrote:
>> > > This comment seems to apply to aura. Why do we want to return false
>> > > here?
>> >
>
>
>
> http://codereview.chromium.org/8340014/
>

[DaveMoore, 2011-10-28 16:46:48]

http://codereview.chromium.org/8340014/diff/7/views/controls/native/native_vi...
File views/controls/native/native_view_host.cc (right):

http://codereview.chromium.org/8340014/diff/7/views/controls/native/native_vi...
views/controls/native/native_view_host.cc:158: return false;
On 2011/10/27 22:12:20, sky wrote:
> On 2011/10/27 21:25:13, DaveMoore wrote:
> > Do we need this when there's only one native window to host everything?
> 
> We still have child windows for the TabContents and RWHV. If we had a
situation
> where the position in the parent changed but the bounds didn't, then we would
> want this code.
> 
> > What I
> > saw was that this gets triggered by the bounds change and then it causes us
to
> > do another layout. I saw no downside on the device when I changed this.
> 
> Shouldn't it result in a no-op because we invoke SetBounds with the same
bounds,
> which because of your change to Window does nothing?
> 
> > 
> > On 2011/10/27 14:40:47, sky wrote:
> > > This comment seems to apply to aura. Why do we want to return false here?
> > 
> 

Done.

[sky, 2011-10-28 17:48:24]

LGTM