Remove RWHV::SetBounds() from the public API, and make comments clearer.

Remove RWHV::SetBounds() from the public API, and make comments clearer.

RenderWidgetHostView::SetBounds() is only being called from a single
call point within libcontent (within RWHImpl). It's purpose is to
position/size top-level windows for pop-up windows, in response to a
page calling window.resizeTo() or window.moveTo(). Therefore, this
change moves the base declaration of this method to RWHVBase.

Furthermore, SetBounds() can be ambiguous since: 1) it doesn't
necessarily set the size/position of the view, but maybe its top-level
window instead; 2) the coordinate system is that of the screen and not
the native view; 3) it's also acceptable for the platform implementation
to ignore the request in some circumstances. Therefore, this change
accounts for all this by renaming the method to
RequestTopLevelBoundsInScreen().

To be clear, there are no functional or behavior changes being made by
this change.

BUG=73362, 693953
TEST=Explicit unit test coverage from RWHVAura unit tests and SitePerProcess browser tests; and implicit coverage via many others.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

[miu, 2017-02-15 23:16:25]

Description was changed from

==========
Remove RWHV::SetBounds() from the public API, and make comments clearer.

RenderWidgetHostView::SetBounds() is only being called from a single
call point within libcontent (within RWHImpl). It's purpose is to
position/size top-level windows for pop-up windows, in response to a
page calling window.resizeTo() or window.moveTo(). Therefore, this
change moves the base declaration of this method to RWHVBase.

Furthermore, SetBounds() can be ambiguous since: 1) it doesn't
necessarily set the size/position of the view, but maybe its top-level
window instead; 2) the coordinate system is that of the screen and not
the native view; 3) it's also acceptable for the platform implementation
to ignore the request in some circumstances. Therefore, this change
accounts for all this by renaming the method to
RequestTopLevelBoundsInScreen().

BUG=73362
TEST=Explicit unit test coverage from RWHVAura unit tests and SitePerProcess
browser tests; and implicit coverage via many others.
==========

to

==========
Remove RWHV::SetBounds() from the public API, and make comments clearer.

RenderWidgetHostView::SetBounds() is only being called from a single
call point within libcontent (within RWHImpl). It's purpose is to
position/size top-level windows for pop-up windows, in response to a
page calling window.resizeTo() or window.moveTo(). Therefore, this
change moves the base declaration of this method to RWHVBase.

Furthermore, SetBounds() can be ambiguous since: 1) it doesn't
necessarily set the size/position of the view, but maybe its top-level
window instead; 2) the coordinate system is that of the screen and not
the native view; 3) it's also acceptable for the platform implementation
to ignore the request in some circumstances. Therefore, this change
accounts for all this by renaming the method to
RequestTopLevelBoundsInScreen().

BUG=73362
TEST=Explicit unit test coverage from RWHVAura unit tests and SitePerProcess
browser tests; and implicit coverage via many others.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[miu, 2017-02-15 23:18:34]

Description was changed from

==========
Remove RWHV::SetBounds() from the public API, and make comments clearer.

RenderWidgetHostView::SetBounds() is only being called from a single
call point within libcontent (within RWHImpl). It's purpose is to
position/size top-level windows for pop-up windows, in response to a
page calling window.resizeTo() or window.moveTo(). Therefore, this
change moves the base declaration of this method to RWHVBase.

Furthermore, SetBounds() can be ambiguous since: 1) it doesn't
necessarily set the size/position of the view, but maybe its top-level
window instead; 2) the coordinate system is that of the screen and not
the native view; 3) it's also acceptable for the platform implementation
to ignore the request in some circumstances. Therefore, this change
accounts for all this by renaming the method to
RequestTopLevelBoundsInScreen().

BUG=73362
TEST=Explicit unit test coverage from RWHVAura unit tests and SitePerProcess
browser tests; and implicit coverage via many others.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Remove RWHV::SetBounds() from the public API, and make comments clearer.

RenderWidgetHostView::SetBounds() is only being called from a single
call point within libcontent (within RWHImpl). It's purpose is to
position/size top-level windows for pop-up windows, in response to a
page calling window.resizeTo() or window.moveTo(). Therefore, this
change moves the base declaration of this method to RWHVBase.

Furthermore, SetBounds() can be ambiguous since: 1) it doesn't
necessarily set the size/position of the view, but maybe its top-level
window instead; 2) the coordinate system is that of the screen and not
the native view; 3) it's also acceptable for the platform implementation
to ignore the request in some circumstances. Therefore, this change
accounts for all this by renaming the method to
RequestTopLevelBoundsInScreen().

To be clear, there are no functional or behavior changes being made by
this change.

BUG=73362
TEST=Explicit unit test coverage from RWHVAura unit tests and SitePerProcess
browser tests; and implicit coverage via many others.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[miu, 2017-02-15 23:18:34]

miu@chromium.org changed reviewers:
+ kenrb@chromium.org, nick@chromium.org, thakis@chromium.org

[miu, 2017-02-15 23:18:51]

The CQ bit was checked by miu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-15 23:20:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[miu, 2017-02-15 23:24:17]

kenrb: PTAL at content/browser/frame_host/*. Seems you last touched the relevant
code here, for OOPIF that control pop-up windows. I wanted to double-check that
what you think GetViewBounds() and SetBounds() meant meshes with what it means
elsewhere.

nick: PTAL for content OWNERS approval.

thakis: PTAL at changes to method comments in
content/public/render_widget_host_view.h for correctness on assumptions. These
updated comments reflect days of me digging throughout the Chromium tree to
determine what's going on, and are a little more up-to-date than my original
findings in the crbug.

[ncarter (slow), 2017-02-15 23:35:02]

This seems okay to me from a high level.

https://codereview.chromium.org/2695093005/diff/1/content/public/browser/rend...
File content/public/browser/render_widget_host_view.h (right):

https://codereview.chromium.org/2695093005/diff/1/content/public/browser/rend...
content/public/browser/render_widget_host_view.h:122: // discrepancy.
How does this differ from (or relate to) GetVisibleViewportSize()? Should that
comment be clarified?

[miu, 2017-02-16 00:06:52]

The CQ bit was checked by miu@chromium.org to run a CQ dry run

[miu, 2017-02-16 00:07:07]

https://codereview.chromium.org/2695093005/diff/1/content/public/browser/rend...
File content/public/browser/render_widget_host_view.h (right):

https://codereview.chromium.org/2695093005/diff/1/content/public/browser/rend...
content/public/browser/render_widget_host_view.h:122: // discrepancy.
On 2017/02/15 23:35:02, ncarter wrote:
> How does this differ from (or relate to) GetVisibleViewportSize()? Should that
> comment be clarified?

Took a look. It seems that there is Ash UI code assuming it's in "app window"
coordinates, while render_widget_host_view_child_frame.cc assumes it's in screen
coordinates, and then render_widget_host_impl.cc assumes it's in view
coordinates (what it sends to Blink in the resize_params).

Ugh, so this needs to be resolved. I'll note this in the crbug for tracking, so
I can address it in a future CL. For now, I won't change the public API comments
because I don't know yet which of the 3 coordinate systems it's supposed to be.

[miu, 2017-02-16 00:07:54]

miu@chromium.org changed reviewers:
+ sky@chromium.org

[miu, 2017-02-16 00:07:55]

+sky: PTAL at changes to chrome/browser/ui/web_contents_sizer.*

[commit-bot: I haz the power, 2017-02-16 00:08:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sky, 2017-02-16 00:29:03]

https://codereview.chromium.org/2695093005/diff/20001/chrome/browser/ui/web_c...
File chrome/browser/ui/web_contents_sizer.cc (right):

https://codereview.chromium.org/2695093005/diff/20001/chrome/browser/ui/web_c...
chrome/browser/ui/web_contents_sizer.cc:19: // TODO(miu): This is a layering
violation, since only the RWHV should know
I don't understand this comment (or the one in the mac file). Why is it wrong to
resize here? This comment also doesn't apply to the USE_AURA case it's resizing
the WebContents.

[commit-bot: I haz the power, 2017-02-16 00:37:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-16 00:37:16]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[miu, 2017-02-17 00:09:03]

https://codereview.chromium.org/2695093005/diff/20001/chrome/browser/ui/web_c...
File chrome/browser/ui/web_contents_sizer.cc (right):

https://codereview.chromium.org/2695093005/diff/20001/chrome/browser/ui/web_c...
chrome/browser/ui/web_contents_sizer.cc:19: // TODO(miu): This is a layering
violation, since only the RWHV should know
On 2017/02/16 00:29:02, sky wrote:
> I don't understand this comment (or the one in the mac file). Why is it wrong
to
> resize here? This comment also doesn't apply to the USE_AURA case it's
resizing
> the WebContents.

A few reasons:

1. We're in the platform-agnostic part of the UI code (i.e., not in a "views" or
"cocoa" dir), so it's already a little strange to have per-platform conditional
code compiles here.

2. Encapsulation: This directly modifies the size of the RenderWidgetHostView's
native view. However, shouldn't that be done through the RWHV class itself,
where it knows more about whether/how the size should be changed for specific
platforms?

3. It seems that this utility function actually isn't needed at all. Callers
could just call web_contents->GetRenderWidgetHostView()->SetSize() themselves.

[sky, 2017-02-17 16:53:08]

https://codereview.chromium.org/2695093005/diff/20001/chrome/browser/ui/web_c...
File chrome/browser/ui/web_contents_sizer.cc (right):

https://codereview.chromium.org/2695093005/diff/20001/chrome/browser/ui/web_c...
chrome/browser/ui/web_contents_sizer.cc:19: // TODO(miu): This is a layering
violation, since only the RWHV should know
On 2017/02/17 00:09:02, miu wrote:
> On 2017/02/16 00:29:02, sky wrote:
> > I don't understand this comment (or the one in the mac file). Why is it
wrong
> to
> > resize here? This comment also doesn't apply to the USE_AURA case it's
> resizing
> > the WebContents.
> 
> A few reasons:
> 
> 1. We're in the platform-agnostic part of the UI code (i.e., not in a "views"
or
> "cocoa" dir), so it's already a little strange to have per-platform
conditional
> code compiles here.
> 
> 2. Encapsulation: This directly modifies the size of the
RenderWidgetHostView's
> native view. However, shouldn't that be done through the RWHV class itself,
> where it knows more about whether/how the size should be changed for specific
> platforms?
> 
> 3. It seems that this utility function actually isn't needed at all. Callers
> could just call web_contents->GetRenderWidgetHostView()->SetSize() themselves.

Could point on the first 1. This code should really be in the platform specific
directories. But I like you're third comment about removing entirely.

[Nico, 2017-02-17 22:05:11]

https://codereview.chromium.org/2695093005/diff/1/content/public/browser/rend...
File content/public/browser/render_widget_host_view.h (right):

https://codereview.chromium.org/2695093005/diff/1/content/public/browser/rend...
content/public/browser/render_widget_host_view.h:121: // rendering size, and NOT
assume a device scale factor will resolve this
I have a hard time understanding this comment. Maybe an example with actual
numbers might help?

[miu, 2017-02-17 23:50:59]

https://codereview.chromium.org/2695093005/diff/1/content/public/browser/rend...
File content/public/browser/render_widget_host_view.h (right):

https://codereview.chromium.org/2695093005/diff/1/content/public/browser/rend...
content/public/browser/render_widget_host_view.h:121: // rendering size, and NOT
assume a device scale factor will resolve this
On 2017/02/17 22:05:11, Nico wrote:
> I have a hard time understanding this comment. Maybe an example with actual
> numbers might help?

Example on Mac: The RWHV's native view (NSView) will be added as a subview in
the view hierarchy in the browser window. Somewhere, one of its ancestor NSViews
may have a coordinate transform. One way this can happen is automatic: when UI
code sets a NSView's "bounds" to have a different size than that of its "frame."
In such cases, the RWHV's NSView, a descendant of the "scaled view," will have a
coordinate space being some scale factor of the screen coordinate space. Also,
note that this "scaling" is totally unrelated to the DIP<-->DevicePixels
transforms we do all over the place.

Example on Aura: Similarly, an ancestor aura::Window of the RWHV's aura::Window
could have a gfx::Transform applied to it (via aura::Window::SetTransform()). In
this case, the size from RWHV's aura::Window::GetBounds() will be in a
coordinate system that is not 1:1 with the screen coordinate system.
RWHV::GetViewBounds() *does* already account for this, returning position+size
in screen coordinates.

So far, the above "scaling transforms" are only theoretically possible. Meaning,
[AFAIK] we don't do this with RWHV's or any ancestor native view in a Chrome
browser. However, I know of two (separate) teams working on new UI features that
*do* want to leverage this "transform" functionality provided by the the UI
frameworks to improve user experiences in the media space. For example:
http://crbug.com/692790.

Also, I've found, and begun documenting in http://crbug.com/73362, several areas
of the browser UI code that currently mis-interpret the semantics around
GetViewBounds() and SetSize(). And, there are examples of code that does
correctly interpret the sizes, but is fragile because it employs its own
customized coordinate transforms around these APIs. So, I'm trying to clean all
that up too. :)

https://codereview.chromium.org/2695093005/diff/20001/chrome/browser/ui/web_c...
File chrome/browser/ui/web_contents_sizer.cc (right):

https://codereview.chromium.org/2695093005/diff/20001/chrome/browser/ui/web_c...
chrome/browser/ui/web_contents_sizer.cc:19: // TODO(miu): This is a layering
violation, since only the RWHV should know
On 2017/02/17 16:53:07, sky wrote:
> Could point on the first 1. This code should really be in the platform
specific
> directories. But I like you're third comment about removing entirely.

Sounds like we agree with the change. I was about to revise my TODO comment, but
this is such a simple thing and it should not cause any behavior change; and so
I just went ahead and did it. :) LGTY?

I'll get rid of ResizeWebContents() altogether in a follow-up change.

Also, a weirdness: This function takes a gfx::Rect, but only the size is ever
used. I've added yusufo@ to this code review to confirm the position isn't
actually needed on Android.

[sky, 2017-02-17 23:54:03]

LGTM

[miu, 2017-02-17 23:54:51]

miu@chromium.org changed reviewers:
+ yusufo@chromium.org

[miu, 2017-02-17 23:54:52]

yusufo: PTAL at chrome/browser/ui/web_contents_sizer.*. I'm trying to get rid of
this, as part of a wider effort to clean-up the RenderWidgetHostView public API.
I took a look at your two changes ([1] and [2]) that made this change the RWHV's
bounds instead of just the size; but I do not understand why the position of the
RWHV should matter. Won't the position be updated later, when the RWHV's native
view is attached to the UI hierarchy for display anyway?

[1] https://codereview.chromium.org/2259533003
[2] https://codereview.chromium.org/2268323003

[miu, 2017-02-17 23:55:03]

The CQ bit was checked by miu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-17 23:56:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-18 00:22:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-18 00:23:00]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[miu, 2017-02-18 00:36:17]

The CQ bit was checked by miu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-18 00:37:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-18 02:04:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-18 02:04:39]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[miu, 2017-02-18 23:05:39]

The CQ bit was checked by miu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-18 23:05:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[miu, 2017-02-18 23:07:14]

Description was changed from

==========
Remove RWHV::SetBounds() from the public API, and make comments clearer.

RenderWidgetHostView::SetBounds() is only being called from a single
call point within libcontent (within RWHImpl). It's purpose is to
position/size top-level windows for pop-up windows, in response to a
page calling window.resizeTo() or window.moveTo(). Therefore, this
change moves the base declaration of this method to RWHVBase.

Furthermore, SetBounds() can be ambiguous since: 1) it doesn't
necessarily set the size/position of the view, but maybe its top-level
window instead; 2) the coordinate system is that of the screen and not
the native view; 3) it's also acceptable for the platform implementation
to ignore the request in some circumstances. Therefore, this change
accounts for all this by renaming the method to
RequestTopLevelBoundsInScreen().

To be clear, there are no functional or behavior changes being made by
this change.

BUG=73362
TEST=Explicit unit test coverage from RWHVAura unit tests and SitePerProcess
browser tests; and implicit coverage via many others.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Remove RWHV::SetBounds() from the public API, and make comments clearer.

RenderWidgetHostView::SetBounds() is only being called from a single
call point within libcontent (within RWHImpl). It's purpose is to
position/size top-level windows for pop-up windows, in response to a
page calling window.resizeTo() or window.moveTo(). Therefore, this
change moves the base declaration of this method to RWHVBase.

Furthermore, SetBounds() can be ambiguous since: 1) it doesn't
necessarily set the size/position of the view, but maybe its top-level
window instead; 2) the coordinate system is that of the screen and not
the native view; 3) it's also acceptable for the platform implementation
to ignore the request in some circumstances. Therefore, this change
accounts for all this by renaming the method to
RequestTopLevelBoundsInScreen().

To be clear, there are no functional or behavior changes being made by
this change.

BUG=73362,693953
TEST=Explicit unit test coverage from RWHVAura unit tests and SitePerProcess
browser tests; and implicit coverage via many others.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[miu, 2017-02-18 23:08:13]

sky: Unfortunately, I had to revert the web_contents_resizer clean-up. tl;dr:
Some things were calling ResizeWebContents() before the RWHV was created. Full
details in http://crbug.com/693953.

[commit-bot: I haz the power, 2017-02-18 23:49:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-18 23:49:25]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[miu, 2017-02-28 01:01:07]

Ping: Everybody except sky@, PTAL.

[kenrb, 2017-02-28 21:44:44]

lgtm