Restore resize throttling on Chrome OS

Restore resize throttling on Chrome OS

r270786 removed DelegatedFrameHost::GetViewBoundsWithResizeLock, but
that was based on the wrong assumption, which is that the resize lock is
always taken at the same time as when RWHI sends the Resize message,
which is not true.

So, reintroduce that logic, although in a saner way. The RWHV is
responsible for throttling, it should be explicit about having 2 sizes:
1- the size that is set by the UI layers
2- the size that the renderer should be resized to.

They are different when throttling.

(1) stays RWHV::GetViewBounds().size().
(2) is introduced as RWHVBase::GetRequestedRendererSize().

BUG=374885
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=272011

[danakj, 2014-05-20 19:28:56]

This is great, thanks LGTM.

The "GetFooBar" thing seems to be a pattern in RWHV land, but is generally
frowned upon right? Should we fight the system and stop using "Get" for getters?

https://codereview.chromium.org/299443003/diff/1/content/browser/compositor/d...
File content/browser/compositor/delegated_frame_host.h (right):

https://codereview.chromium.org/299443003/diff/1/content/browser/compositor/d...
content/browser/compositor/delegated_frame_host.h:91: gfx::Size
GetRequestedRendererSize() const;
s/Get//?

https://codereview.chromium.org/299443003/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/render_widget_host_view_aura.h (right):

https://codereview.chromium.org/299443003/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_view_aura.h:177: virtual
gfx::Size GetRequestedRendererSize() const OVERRIDE;
s/Get//?

https://codereview.chromium.org/299443003/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/render_widget_host_view_base.cc (right):

https://codereview.chromium.org/299443003/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_view_base.cc:401: return
gfx::ToCeiledSize(gfx::ScaleSize(GetRequestedRendererSize(),
Hmmm, should we also have a RequestedRendererDeviceScale and store that inside
the resize lock? I wonder if this is an existing DSF race?

[piman, 2014-05-20 19:49:13]

On Tue, May 20, 2014 at 12:28 PM, <danakj@chromium.org> wrote:

> This is great, thanks LGTM.
>
> The "GetFooBar" thing seems to be a pattern in RWHV land, but is generally
> frowned upon right? Should we fight the system and stop using "Get" for
> getters?
>

On the contrary, it is not used enough in cc ;)  I don't think there is a
written rule, but what I've been taught as a noogler is:
If it's a trivial accessor (inline, etc.), it should have the name of the
field - requested_renderer_size().
If it's a method (non-inline, may or may not be non-trivial) the name
should describe what it does - GetRequestedRendererSize().

Having something like "OutputSurface" for an accessor is confusing at the
call site, because you don't know if OutputSurface() represents a function
call or a constructor. Also, Most Vexing Parse.


https://codereview.chromium.org/299443003/diff/1/content/
> browser/compositor/delegated_frame_host.h
> File content/browser/compositor/delegated_frame_host.h (right):
>
> https://codereview.chromium.org/299443003/diff/1/content/
> browser/compositor/delegated_frame_host.h#newcode91
> content/browser/compositor/delegated_frame_host.h:91: gfx::Size
> GetRequestedRendererSize() const;
> s/Get//?
>
> https://codereview.chromium.org/299443003/diff/1/content/
> browser/renderer_host/render_widget_host_view_aura.h
> File content/browser/renderer_host/render_widget_host_view_aura.h
> (right):
>
> https://codereview.chromium.org/299443003/diff/1/content/
> browser/renderer_host/render_widget_host_view_aura.h#newcode177
> content/browser/renderer_host/render_widget_host_view_aura.h:177:
> virtual gfx::Size GetRequestedRendererSize() const OVERRIDE;
> s/Get//?
>
> https://codereview.chromium.org/299443003/diff/1/content/
> browser/renderer_host/render_widget_host_view_base.cc
> File content/browser/renderer_host/render_widget_host_view_base.cc
> (right):
>
> https://codereview.chromium.org/299443003/diff/1/content/
> browser/renderer_host/render_widget_host_view_base.cc#newcode401
> content/browser/renderer_host/render_widget_host_view_base.cc:401:
> return gfx::ToCeiledSize(gfx::ScaleSize(GetRequestedRendererSize(),
> Hmmm, should we also have a RequestedRendererDeviceScale and store that
> inside the resize lock? I wonder if this is an existing DSF race?
>

The resize lock will accept a frame that has the wrong DSF provided it has
the right size in DIP. The idea is that if we have a frame of the right
size, we can show it without gutters, so it kinda doesn't matter if it's
the right DSF or not.
Also the use case for the resize lock is for resize-scrubbing, where you
wouldn't switch DSF on the fly. When moving a window from one screen to the
other with different DSF, it's probably better to have the move be smooth
and avoid jank while waiting for the renderer to relayout / redraw (since
we can use the existing frame in the mean time), rather than taking a
resize lock and blocking moves until we have the right DSF.

That said, if we think we should do it, we can explore.


>
> https://codereview.chromium.org/299443003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2014-05-20 20:04:51]

On Tue, May 20, 2014 at 3:48 PM, Antoine Labour <piman@chromium.org> wrote:

> On Tue, May 20, 2014 at 12:28 PM, <danakj@chromium.org> wrote:
>
>> This is great, thanks LGTM.
>>
>> The "GetFooBar" thing seems to be a pattern in RWHV land, but is generally
>> frowned upon right? Should we fight the system and stop using "Get" for
>> getters?
>>
>
> On the contrary, it is not used enough in cc ;)  I don't think there is a
> written rule, but what I've been taught as a noogler is:
> If it's a trivial accessor (inline, etc.), it should have the name of the
> field - requested_renderer_size().
> If it's a method (non-inline, may or may not be non-trivial) the name
> should describe what it does - GetRequestedRendererSize().
>
> Having something like "OutputSurface" for an accessor is confusing at the
> call site, because you don't know if OutputSurface() represents a function
> call or a constructor. Also, Most Vexing Parse.
>

Oh, I think I got this from WebKit style too, not the first time this has
happened (eg. for i = 0; i < size; ++i).

"Precede setters with the word "set". Use bare words for getters. Setter
and getter names should match the names of the variables being set/gotten."

Well then.. :)

>
>
> https://codereview.chromium.org/299443003/diff/1/content/
>> browser/compositor/delegated_frame_host.h
>> File content/browser/compositor/delegated_frame_host.h (right):
>>
>> https://codereview.chromium.org/299443003/diff/1/content/
>> browser/compositor/delegated_frame_host.h#newcode91
>> content/browser/compositor/delegated_frame_host.h:91: gfx::Size
>> GetRequestedRendererSize() const;
>> s/Get//?
>>
>> https://codereview.chromium.org/299443003/diff/1/content/
>> browser/renderer_host/render_widget_host_view_aura.h
>> File content/browser/renderer_host/render_widget_host_view_aura.h
>> (right):
>>
>> https://codereview.chromium.org/299443003/diff/1/content/
>> browser/renderer_host/render_widget_host_view_aura.h#newcode177
>> content/browser/renderer_host/render_widget_host_view_aura.h:177:
>> virtual gfx::Size GetRequestedRendererSize() const OVERRIDE;
>> s/Get//?
>>
>> https://codereview.chromium.org/299443003/diff/1/content/
>> browser/renderer_host/render_widget_host_view_base.cc
>> File content/browser/renderer_host/render_widget_host_view_base.cc
>> (right):
>>
>> https://codereview.chromium.org/299443003/diff/1/content/
>> browser/renderer_host/render_widget_host_view_base.cc#newcode401
>> content/browser/renderer_host/render_widget_host_view_base.cc:401:
>> return gfx::ToCeiledSize(gfx::ScaleSize(GetRequestedRendererSize(),
>> Hmmm, should we also have a RequestedRendererDeviceScale and store that
>> inside the resize lock? I wonder if this is an existing DSF race?
>>
>
> The resize lock will accept a frame that has the wrong DSF provided it has
> the right size in DIP. The idea is that if we have a frame of the right
> size, we can show it without gutters, so it kinda doesn't matter if it's
> the right DSF or not.
> Also the use case for the resize lock is for resize-scrubbing, where you
> wouldn't switch DSF on the fly. When moving a window from one screen to the
> other with different DSF, it's probably better to have the move be smooth
> and avoid jank while waiting for the renderer to relayout / redraw (since
> we can use the existing frame in the mean time), rather than taking a
> resize lock and blocking moves until we have the right DSF.
>
> That said, if we think we should do it, we can explore.
>

Ah.. I agree with you about all of that, resize lock doesn't seem right. My
concern was that this method is call GetPhysicalBackingSize, and it's using
the requested renderer size with the current DPI which is a mismatch. I
don't know how this method is used but that worried(s) me. Maybe this
method is fine, or maybe it should go away.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[piman, 2014-05-20 20:28:59]

On Tue, May 20, 2014 at 1:04 PM, Dana Jansens <danakj@chromium.org> wrote:

> On Tue, May 20, 2014 at 3:48 PM, Antoine Labour <piman@chromium.org>wrote:
>
>> On Tue, May 20, 2014 at 12:28 PM, <danakj@chromium.org> wrote:
>>
>>> This is great, thanks LGTM.
>>>
>>> The "GetFooBar" thing seems to be a pattern in RWHV land, but is
>>> generally
>>> frowned upon right? Should we fight the system and stop using "Get" for
>>> getters?
>>>
>>
>> On the contrary, it is not used enough in cc ;)  I don't think there is a
>> written rule, but what I've been taught as a noogler is:
>> If it's a trivial accessor (inline, etc.), it should have the name of the
>> field - requested_renderer_size().
>> If it's a method (non-inline, may or may not be non-trivial) the name
>> should describe what it does - GetRequestedRendererSize().
>>
>> Having something like "OutputSurface" for an accessor is confusing at the
>> call site, because you don't know if OutputSurface() represents a function
>> call or a constructor. Also, Most Vexing Parse.
>>
>
> Oh, I think I got this from WebKit style too, not the first time this has
> happened (eg. for i = 0; i < size; ++i).
>
> "Precede setters with the word "set". Use bare words for getters. Setter
> and getter names should match the names of the variables being set/gotten."
>
> Well then.. :)
>
>>
>>
>> https://codereview.chromium.org/299443003/diff/1/content/
>>> browser/compositor/delegated_frame_host.h
>>> File content/browser/compositor/delegated_frame_host.h (right):
>>>
>>> https://codereview.chromium.org/299443003/diff/1/content/
>>> browser/compositor/delegated_frame_host.h#newcode91
>>> content/browser/compositor/delegated_frame_host.h:91: gfx::Size
>>> GetRequestedRendererSize() const;
>>> s/Get//?
>>>
>>> https://codereview.chromium.org/299443003/diff/1/content/
>>> browser/renderer_host/render_widget_host_view_aura.h
>>> File content/browser/renderer_host/render_widget_host_view_aura.h
>>> (right):
>>>
>>> https://codereview.chromium.org/299443003/diff/1/content/
>>> browser/renderer_host/render_widget_host_view_aura.h#newcode177
>>> content/browser/renderer_host/render_widget_host_view_aura.h:177:
>>> virtual gfx::Size GetRequestedRendererSize() const OVERRIDE;
>>> s/Get//?
>>>
>>> https://codereview.chromium.org/299443003/diff/1/content/
>>> browser/renderer_host/render_widget_host_view_base.cc
>>> File content/browser/renderer_host/render_widget_host_view_base.cc
>>> (right):
>>>
>>> https://codereview.chromium.org/299443003/diff/1/content/
>>> browser/renderer_host/render_widget_host_view_base.cc#newcode401
>>> content/browser/renderer_host/render_widget_host_view_base.cc:401:
>>> return gfx::ToCeiledSize(gfx::ScaleSize(GetRequestedRendererSize(),
>>> Hmmm, should we also have a RequestedRendererDeviceScale and store that
>>> inside the resize lock? I wonder if this is an existing DSF race?
>>>
>>
>> The resize lock will accept a frame that has the wrong DSF provided it
>> has the right size in DIP. The idea is that if we have a frame of the right
>> size, we can show it without gutters, so it kinda doesn't matter if it's
>> the right DSF or not.
>> Also the use case for the resize lock is for resize-scrubbing, where you
>> wouldn't switch DSF on the fly. When moving a window from one screen to the
>> other with different DSF, it's probably better to have the move be smooth
>> and avoid jank while waiting for the renderer to relayout / redraw (since
>> we can use the existing frame in the mean time), rather than taking a
>> resize lock and blocking moves until we have the right DSF.
>>
>> That said, if we think we should do it, we can explore.
>>
>
> Ah.. I agree with you about all of that, resize lock doesn't seem right.
> My concern was that this method is call GetPhysicalBackingSize, and it's
> using the requested renderer size with the current DPI which is a mismatch.
> I don't know how this method is used but that worried(s) me. Maybe this
> method is fine, or maybe it should go away.
>

GetPhysicalBackingSize is what decides the pixel size of the output surface
in the renderer. Android changes this, I though to account for the surface
given to WebView (but I may be wrong about that).

You're right that this is at the very least convoluted - we should just ask
the RWHV for the DSF. re: mismatch, I don't think it's any more / any less
when the ResizeLock isn't used (since then GetRequestedRendererSize() ==
GetViewBounds().size(), so it matches), and when it is used, I think the
DSF is likely not changing so it's not the end of the world (also it's been
that way for a long time). Given this, do you mind if I clean up in a
follow up?

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2014-05-20 20:34:05]

On Tue, May 20, 2014 at 4:28 PM, Antoine Labour <piman@chromium.org> wrote:

>
>
>
> On Tue, May 20, 2014 at 1:04 PM, Dana Jansens <danakj@chromium.org> wrote:
>
>> On Tue, May 20, 2014 at 3:48 PM, Antoine Labour <piman@chromium.org>wrote:
>>
>>> On Tue, May 20, 2014 at 12:28 PM, <danakj@chromium.org> wrote:
>>>
>>>> This is great, thanks LGTM.
>>>>
>>>> The "GetFooBar" thing seems to be a pattern in RWHV land, but is
>>>> generally
>>>> frowned upon right? Should we fight the system and stop using "Get" for
>>>> getters?
>>>>
>>>
>>> On the contrary, it is not used enough in cc ;)  I don't think there is
>>> a written rule, but what I've been taught as a noogler is:
>>> If it's a trivial accessor (inline, etc.), it should have the name of
>>> the field - requested_renderer_size().
>>> If it's a method (non-inline, may or may not be non-trivial) the name
>>> should describe what it does - GetRequestedRendererSize().
>>>
>>> Having something like "OutputSurface" for an accessor is confusing at
>>> the call site, because you don't know if OutputSurface() represents a
>>> function call or a constructor. Also, Most Vexing Parse.
>>>
>>
>> Oh, I think I got this from WebKit style too, not the first time this has
>> happened (eg. for i = 0; i < size; ++i).
>>
>> "Precede setters with the word "set". Use bare words for getters. Setter
>> and getter names should match the names of the variables being set/gotten."
>>
>> Well then.. :)
>>
>>>
>>>
>>> https://codereview.chromium.org/299443003/diff/1/content/
>>>> browser/compositor/delegated_frame_host.h
>>>> File content/browser/compositor/delegated_frame_host.h (right):
>>>>
>>>> https://codereview.chromium.org/299443003/diff/1/content/
>>>> browser/compositor/delegated_frame_host.h#newcode91
>>>> content/browser/compositor/delegated_frame_host.h:91: gfx::Size
>>>> GetRequestedRendererSize() const;
>>>> s/Get//?
>>>>
>>>> https://codereview.chromium.org/299443003/diff/1/content/
>>>> browser/renderer_host/render_widget_host_view_aura.h
>>>> File content/browser/renderer_host/render_widget_host_view_aura.h
>>>> (right):
>>>>
>>>> https://codereview.chromium.org/299443003/diff/1/content/
>>>> browser/renderer_host/render_widget_host_view_aura.h#newcode177
>>>> content/browser/renderer_host/render_widget_host_view_aura.h:177:
>>>> virtual gfx::Size GetRequestedRendererSize() const OVERRIDE;
>>>> s/Get//?
>>>>
>>>> https://codereview.chromium.org/299443003/diff/1/content/
>>>> browser/renderer_host/render_widget_host_view_base.cc
>>>> File content/browser/renderer_host/render_widget_host_view_base.cc
>>>> (right):
>>>>
>>>> https://codereview.chromium.org/299443003/diff/1/content/
>>>> browser/renderer_host/render_widget_host_view_base.cc#newcode401
>>>> content/browser/renderer_host/render_widget_host_view_base.cc:401:
>>>> return gfx::ToCeiledSize(gfx::ScaleSize(GetRequestedRendererSize(),
>>>> Hmmm, should we also have a RequestedRendererDeviceScale and store that
>>>> inside the resize lock? I wonder if this is an existing DSF race?
>>>>
>>>
>>> The resize lock will accept a frame that has the wrong DSF provided it
>>> has the right size in DIP. The idea is that if we have a frame of the right
>>> size, we can show it without gutters, so it kinda doesn't matter if it's
>>> the right DSF or not.
>>> Also the use case for the resize lock is for resize-scrubbing, where you
>>> wouldn't switch DSF on the fly. When moving a window from one screen to the
>>> other with different DSF, it's probably better to have the move be smooth
>>> and avoid jank while waiting for the renderer to relayout / redraw (since
>>> we can use the existing frame in the mean time), rather than taking a
>>> resize lock and blocking moves until we have the right DSF.
>>>
>>> That said, if we think we should do it, we can explore.
>>>
>>
>> Ah.. I agree with you about all of that, resize lock doesn't seem right.
>> My concern was that this method is call GetPhysicalBackingSize, and it's
>> using the requested renderer size with the current DPI which is a mismatch.
>> I don't know how this method is used but that worried(s) me. Maybe this
>> method is fine, or maybe it should go away.
>>
>
> GetPhysicalBackingSize is what decides the pixel size of the output
> surface in the renderer. Android changes this, I though to account for the
> surface given to WebView (but I may be wrong about that).
>
> You're right that this is at the very least convoluted - we should just
> ask the RWHV for the DSF. re: mismatch, I don't think it's any more / any
> less when the ResizeLock isn't used (since then GetRequestedRendererSize()
> == GetViewBounds().size(), so it matches), and when it is used, I think the
> DSF is likely not changing so it's not the end of the world (also it's been
> that way for a long time). Given this, do you mind if I clean up in a
> follow up?
>
>
Oh absolutely, that's why I LGTM'd earlier. This looks equally awkward to
before. :)

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[piman, 2014-05-21 02:28:12]

The CQ bit was checked by piman@chromium.org

[commit-bot: I haz the power, 2014-05-21 02:30:40]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/piman@chromium.org/299443003/20001

[commit-bot: I haz the power, 2014-05-21 20:02:43]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[commit-bot: I haz the power, 2014-05-21 23:28:50]

Change committed as 272011