Propagate pageScaleFactor to GuestViews

Propagate pageScaleFactor to GuestViews.

We make all changes to pageScaleFactor available to the
browser process, so that WebContentsObservers may observe
them.

Previously, we were only sending notifications to the browser
process when the PageScaleFactorIsOne changed. Now that we're
notifying whenever the pageScaleFactor changed, we get rid of
this extra message and have the browser process handle the
PageScaleFactorIsOne logic as part of its handling of all
pageScaleFactor changes.

We also replace WebContentsImpl::ResetPageScale with
WebContentsImpl::SetPageScale, so that the browser process can
set any pageScaleFactor.

BUG=445319
Committed: https://crrev.com/432e47d6fc9b1a154900d91ad4841632160af238
Cr-Commit-Position: refs/heads/master@{#358631}

[Kevin McNee - google account, 2015-10-19 15:31:08]

Description was changed from

==========
Propagate pageScaleFactor to GuestViews

BUG=
==========

to

==========
Propagate pageScaleFactor to GuestViews

BUG=
==========

[Kevin McNee - google account, 2015-10-19 15:31:08]

mcnee@google.com changed reviewers:
+ wjmaclean@chromium.org

[wjmaclean, 2015-10-19 15:43:29]

Here are some initial comments, including answers to your questions.

https://codereview.chromium.org/1408393003/diff/1/content/public/browser/web_...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/1408393003/diff/1/content/public/browser/web_...
content/public/browser/web_contents.h:585: virtual void ResetPageScale() = 0;
Perhaps find calls to this and replace with SetPageScale(1.f)?

https://codereview.chromium.org/1408393003/diff/1/content/public/browser/web_...
File content/public/browser/web_contents_observer.h (right):

https://codereview.chromium.org/1408393003/diff/1/content/public/browser/web_...
content/public/browser/web_contents_observer.h:421: virtual void
OnPageScaleFactorChanged(float page_scale_factor) {}
Add a comment to explain what this function is for.

https://codereview.chromium.org/1408393003/diff/1/content/renderer/render_vie...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/1408393003/diff/1/content/renderer/render_vie...
content/renderer/render_view_impl.cc:1303:
IPC_MESSAGE_HANDLER(ViewMsg_ResetPageScale, OnResetPageScale)
Ditto, perhaps we can remove this.

https://codereview.chromium.org/1408393003/diff/1/content/renderer/render_vie...
content/renderer/render_view_impl.cc:2476: void
RenderViewImpl::OnResetPageScale() {
This can probably go away too ...

https://codereview.chromium.org/1408393003/diff/1/content/renderer/render_vie...
content/renderer/render_view_impl.cc:3389: Send(new
ViewHostMsg_PageScaleFactorChanged(routing_id_,
Yeah, but for now this is here, so I guess we modify it here.

https://codereview.chromium.org/1408393003/diff/1/content/renderer/render_vie...
content/renderer/render_view_impl.cc:3393: // and have the browser handle this
logic?
Yes, remove the extra message.

[Kevin McNee - google account, 2015-10-20 15:13:44]

https://codereview.chromium.org/1408393003/diff/1/content/public/browser/web_...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/1408393003/diff/1/content/public/browser/web_...
content/public/browser/web_contents.h:585: virtual void ResetPageScale() = 0;
On 2015/10/19 15:43:28, wjmaclean wrote:
> Perhaps find calls to this and replace with SetPageScale(1.f)?

Done.

https://codereview.chromium.org/1408393003/diff/1/content/public/browser/web_...
File content/public/browser/web_contents_observer.h (right):

https://codereview.chromium.org/1408393003/diff/1/content/public/browser/web_...
content/public/browser/web_contents_observer.h:421: virtual void
OnPageScaleFactorChanged(float page_scale_factor) {}
On 2015/10/19 15:43:28, wjmaclean wrote:
> Add a comment to explain what this function is for.

Done.

https://codereview.chromium.org/1408393003/diff/1/content/renderer/render_vie...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/1408393003/diff/1/content/renderer/render_vie...
content/renderer/render_view_impl.cc:1303:
IPC_MESSAGE_HANDLER(ViewMsg_ResetPageScale, OnResetPageScale)
On 2015/10/19 15:43:28, wjmaclean wrote:
> Ditto, perhaps we can remove this.

Done.

https://codereview.chromium.org/1408393003/diff/1/content/renderer/render_vie...
content/renderer/render_view_impl.cc:2476: void
RenderViewImpl::OnResetPageScale() {
On 2015/10/19 15:43:28, wjmaclean wrote:
> This can probably go away too ...

Done.

https://codereview.chromium.org/1408393003/diff/1/content/renderer/render_vie...
content/renderer/render_view_impl.cc:3389: Send(new
ViewHostMsg_PageScaleFactorChanged(routing_id_,
On 2015/10/19 15:43:29, wjmaclean wrote:
> Yeah, but for now this is here, so I guess we modify it here.

Acknowledged.

https://codereview.chromium.org/1408393003/diff/1/content/renderer/render_vie...
content/renderer/render_view_impl.cc:3393: // and have the browser handle this
logic?
On 2015/10/19 15:43:28, wjmaclean wrote:
> Yes, remove the extra message.

Done.

[Kevin McNee - google account, 2015-10-20 15:34:50]

Description was changed from

==========
Propagate pageScaleFactor to GuestViews

BUG=
==========

to

==========
Propagate pageScaleFactor to GuestViews.

We make all changes to pageScaleFactor available to the browser process, so that
WebContentsObservers may observe them.

Previously, we were only sending notifications to the browser process when the
PageScaleFactorIsOne changed. Now that we're notifying whenever the
pageScaleFactor changed, we get rid of this extra message and have the browser
process handle the PageScaleFactorIsOne logic as part of its handling of all
pageScaleFactor changes.

We also replace WebContentsImpl::ResetPageScale with
WebContentsImpl::SetPageScale, so that the browser browser process can set any
pageScaleFactor.

BUG=
==========

[Kevin McNee - google account, 2015-10-20 15:36:01]

Description was changed from

==========
Propagate pageScaleFactor to GuestViews.

We make all changes to pageScaleFactor available to the browser process, so that
WebContentsObservers may observe them.

Previously, we were only sending notifications to the browser process when the
PageScaleFactorIsOne changed. Now that we're notifying whenever the
pageScaleFactor changed, we get rid of this extra message and have the browser
process handle the PageScaleFactorIsOne logic as part of its handling of all
pageScaleFactor changes.

We also replace WebContentsImpl::ResetPageScale with
WebContentsImpl::SetPageScale, so that the browser browser process can set any
pageScaleFactor.

BUG=
==========

to

==========
Propagate pageScaleFactor to GuestViews.

We make all changes to pageScaleFactor available to the browser process, so that
WebContentsObservers may observe them.

Previously, we were only sending notifications to the browser process when the
PageScaleFactorIsOne changed. Now that we're notifying whenever the
pageScaleFactor changed, we get rid of this extra message and have the browser
process handle the PageScaleFactorIsOne logic as part of its handling of all
pageScaleFactor changes.

We also replace WebContentsImpl::ResetPageScale with
WebContentsImpl::SetPageScale, so that the browser process can set any
pageScaleFactor.

BUG=
==========

[Kevin McNee - google account, 2015-10-20 15:37:34]

The CQ bit was checked by mcnee@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-10-20 15:37:40]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1408393003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1408393003/20001

[commit-bot: I haz the power, 2015-10-20 15:37:40]

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

[commit-bot: I haz the power, 2015-10-20 15:37:41]

Dry run: No L-G-T-M from a valid reviewer yet. Only full committers are
accepted.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[Kevin McNee - google account, 2015-10-21 14:49:47]

mcnee@google.com changed reviewers:
+ paulmeyer@chromium.org

[Kevin McNee - google account, 2015-10-21 14:51:00]

Adding Paul as reviewer.

[Kevin McNee - google account, 2015-10-21 19:26:14]

Added tests:
-WebContentsImplBrowserTest.ChangePageScale for verifying that
WebContentsObservers are now notified of page scale changes
-HostZoomMapBrowserTest.PageScaleIsOneChanged for the existing
PageScaleFactorIsOne logic

[wjmaclean, 2015-10-21 19:47:56]

lgtm

Can you please edit the issue description to limit the linewidth to 72 chars
please?

I would suggest creis@ to review content/ changes.

[Kevin McNee - google account, 2015-10-21 19:50:47]

Description was changed from

==========
Propagate pageScaleFactor to GuestViews.

We make all changes to pageScaleFactor available to the browser process, so that
WebContentsObservers may observe them.

Previously, we were only sending notifications to the browser process when the
PageScaleFactorIsOne changed. Now that we're notifying whenever the
pageScaleFactor changed, we get rid of this extra message and have the browser
process handle the PageScaleFactorIsOne logic as part of its handling of all
pageScaleFactor changes.

We also replace WebContentsImpl::ResetPageScale with
WebContentsImpl::SetPageScale, so that the browser process can set any
pageScaleFactor.

BUG=
==========

to

==========
Propagate pageScaleFactor to GuestViews.

We make all changes to pageScaleFactor available to the
browser process, so that WebContentsObservers may observe
them.

Previously, we were only sending notifications to the browser
process when the PageScaleFactorIsOne changed. Now that we're
notifying whenever the pageScaleFactor changed, we get rid of
this extra message and have the browser process handle the
PageScaleFactorIsOne logic as part of its handling of all
pageScaleFactor changes.

We also replace WebContentsImpl::ResetPageScale with
WebContentsImpl::SetPageScale, so that the browser process can
set any pageScaleFactor.

BUG=
==========

[Kevin McNee - google account, 2015-10-21 19:53:22]

mcnee@google.com changed reviewers:
+ creis@chromium.org

[Kevin McNee - google account, 2015-10-21 19:55:01]

Adding creis@ as a reviewer.

Hello there. Could you take a look at this change?

[Charlie Reis, 2015-10-21 21:37:09]

[+lfg, site-isolation-reviews]

Can you add a bug number to the CL description?

I'm noticing some places that this change doesn't work for out-of-process
iframes.  GuestView will be moving to OOPIFs at some point, and we're generally
looking for ways to solve problems like this that can benefit both
BrowserPlugin-based GuestViews and OOPIF-based GuestViews.  @wjmaclean: Are
there any ways to improve this CL for that?

We may want to just fix this bug and fix the OOPIF case later, but I want us to
consider it first.

https://codereview.chromium.org/1408393003/diff/40001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1408393003/diff/40001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:3130:
static_cast<HostZoomMapImpl*>(HostZoomMap::Get(GetSiteInstance()));
I'm concerned about this, because there isn't a single SiteInstance for
WebContents anymore (due to out-of-process iframes).  Things like
GetSiteInstance(), GetRenderProcessHost(), and GetRoutingID() generally
shouldn't be called on WebContents, since each RenderFrameHost can have a
different value for them.

In this case, the zoom logic hasn't been updated for OOPIFs yet.  wjmaclean@:
Have you thought about how it will have to change?  Here, it seems like each
RenderViewHost would have to be notified.

I could be ok with landing this change without fixing it for OOPIFs, but we need
a TODO mentioning a bug (maybe 528407?) to fix it.

https://codereview.chromium.org/1408393003/diff/40001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl_browsertest.cc (right):

https://codereview.chromium.org/1408393003/diff/40001/content/browser/web_con...
content/browser/web_contents/web_contents_impl_browsertest.cc:721: // See
WebContentsImplBrowserTest.ChangePageScale
nit: End sentence with a period.  (Same below on line 727.)

https://codereview.chromium.org/1408393003/diff/40001/content/browser/web_con...
content/browser/web_contents/web_contents_impl_browsertest.cc:755: // a
notification to the browser so that WebContentsObservers are notified.
nit: Move comment before test declaration and remove blank line below.

https://codereview.chromium.org/1408393003/diff/40001/content/browser/web_con...
content/browser/web_contents/web_contents_impl_browsertest.cc:757:
NavigateToURL(shell(), GURL(url::kAboutBlankURL));
You should probably use a real URL here.  about:blank is a special case that
behaves differently in a lot of ways, including for things like HostZoomMap.

A common test page is embedded_test_server()->GetURL("/title1.html").

https://codereview.chromium.org/1408393003/diff/40001/content/common/view_mes...
File content/common/view_messages.h (right):

https://codereview.chromium.org/1408393003/diff/40001/content/common/view_mes...
content/common/view_messages.h:1237: // Sent when the renderer changes its page
scale factor
nit: End with period.

[wjmaclean, 2015-10-22 14:47:30]

Description was changed from

==========
Propagate pageScaleFactor to GuestViews.

We make all changes to pageScaleFactor available to the
browser process, so that WebContentsObservers may observe
them.

Previously, we were only sending notifications to the browser
process when the PageScaleFactorIsOne changed. Now that we're
notifying whenever the pageScaleFactor changed, we get rid of
this extra message and have the browser process handle the
PageScaleFactorIsOne logic as part of its handling of all
pageScaleFactor changes.

We also replace WebContentsImpl::ResetPageScale with
WebContentsImpl::SetPageScale, so that the browser process can
set any pageScaleFactor.

BUG=
==========

to

==========
Propagate pageScaleFactor to GuestViews.

We make all changes to pageScaleFactor available to the
browser process, so that WebContentsObservers may observe
them.

Previously, we were only sending notifications to the browser
process when the PageScaleFactorIsOne changed. Now that we're
notifying whenever the pageScaleFactor changed, we get rid of
this extra message and have the browser process handle the
PageScaleFactorIsOne logic as part of its handling of all
pageScaleFactor changes.

We also replace WebContentsImpl::ResetPageScale with
WebContentsImpl::SetPageScale, so that the browser process can
set any pageScaleFactor.

BUG=445319
==========

[wjmaclean, 2015-10-22 15:03:15]

https://codereview.chromium.org/1408393003/diff/40001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1408393003/diff/40001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:3130:
static_cast<HostZoomMapImpl*>(HostZoomMap::Get(GetSiteInstance()));
On 2015/10/21 21:37:09, Charlie Reis wrote:
> I'm concerned about this, because there isn't a single SiteInstance for
> WebContents anymore (due to out-of-process iframes).  Things like
> GetSiteInstance(), GetRenderProcessHost(), and GetRoutingID() generally
> shouldn't be called on WebContents, since each RenderFrameHost can have a
> different value for them.
> 
> In this case, the zoom logic hasn't been updated for OOPIFs yet.  wjmaclean@:
> Have you thought about how it will have to change?  Here, it seems like each
> RenderViewHost would have to be notified.
> 
> I could be ok with landing this change without fixing it for OOPIFs, but we
need
> a TODO mentioning a bug (maybe 528407?) to fix it.

I would like to suggest that we do land this change, with a bug as you suggest.

The entire, current host-zoom infrastructure is predicated on tying zoom level
to a site instance, which related to the notion of an origin.

I suppose, going forward, we might expect the outermost (top-level?) frame's
origin/site-instance to set the zoom level for the entire page, though it's not
entirely obvious to me if that should always be the case. Maybe there will be
cases where we want a nested frame to render with a different zoom?

In all honesty, I haven't really thought about how HostZoomMap will need to
change for OOPIF beyond what I've said above: we need a plan.

All attempts to use methods like HostZoomMap::GetForWebContents() will need to
be revised as part of the change to OOPIF.

In the short term we can change the call here to use
HostZoomMap::GetForWebContents(this) so we will easily catch it when we update
the rest of these calls.

[Fady Samuel, 2015-10-22 15:09:01]

fsamuel@chromium.org changed reviewers:
+ fsamuel@chromium.org

[Fady Samuel, 2015-10-22 15:09:01]

https://codereview.chromium.org/1408393003/diff/40001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1408393003/diff/40001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:3130:
static_cast<HostZoomMapImpl*>(HostZoomMap::Get(GetSiteInstance()));
On 2015/10/21 21:37:09, Charlie Reis wrote:
> I'm concerned about this, because there isn't a single SiteInstance for
> WebContents anymore (due to out-of-process iframes).  Things like
> GetSiteInstance(), GetRenderProcessHost(), and GetRoutingID() generally
> shouldn't be called on WebContents, since each RenderFrameHost can have a
> different value for them.
> 
> In this case, the zoom logic hasn't been updated for OOPIFs yet.  wjmaclean@:
> Have you thought about how it will have to change?  Here, it seems like each
> RenderViewHost would have to be notified.
> 
> I could be ok with landing this change without fixing it for OOPIFs, but we
need
> a TODO mentioning a bug (maybe 528407?) to fix it.

I'm not sure this needs changing for OOPIFs.

Zoom is a property that applies for the whole page including iframes. Zoom is
also a property that stored on a per-origin (really per-SiteInstance) basis. The
zoom of the page is the zoom of the site instance of the top level frame. All
subframes inherit the same zoom level as the top level frame.

[wjmaclean, 2015-10-22 15:29:02]

> 
> I'm not sure this needs changing for OOPIFs.
> 
> Zoom is a property that applies for the whole page including iframes. Zoom is
> also a property that stored on a per-origin (really per-SiteInstance) basis.
The
> zoom of the page is the zoom of the site instance of the top level frame. All
> subframes inherit the same zoom level as the top level frame.

Certainly for apps we can imagine setting a different zoom for a WebView's
contents (e.g. BrowserSample might want to zoom tab contents in a manner similar
to how Chrome currently zooms ... Chrome UI elements don't zoom with the tab
contents, and Browser Zoom might want to mimic this behaviour).

Although it might be a headache for the (designers of a) frame containing an
iframe, I think it's worth at least considering the possibility that different
frames *could* set their zoom levels independently.

[Charlie Reis, 2015-10-22 19:09:25]

https://codereview.chromium.org/1408393003/diff/40001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1408393003/diff/40001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:3130:
static_cast<HostZoomMapImpl*>(HostZoomMap::Get(GetSiteInstance()));
On 2015/10/22 15:03:14, wjmaclean wrote:
> On 2015/10/21 21:37:09, Charlie Reis wrote:
> > I'm concerned about this, because there isn't a single SiteInstance for
> > WebContents anymore (due to out-of-process iframes).  Things like
> > GetSiteInstance(), GetRenderProcessHost(), and GetRoutingID() generally
> > shouldn't be called on WebContents, since each RenderFrameHost can have a
> > different value for them.
> > 
> > In this case, the zoom logic hasn't been updated for OOPIFs yet. 
wjmaclean@:
> > Have you thought about how it will have to change?  Here, it seems like each
> > RenderViewHost would have to be notified.
> > 
> > I could be ok with landing this change without fixing it for OOPIFs, but we
> need
> > a TODO mentioning a bug (maybe 528407?) to fix it.
> 
> I would like to suggest that we do land this change, with a bug as you
suggest.

Sure.

> The entire, current host-zoom infrastructure is predicated on tying zoom level
> to a site instance, which related to the notion of an origin.
> 
> I suppose, going forward, we might expect the outermost (top-level?) frame's
> origin/site-instance to set the zoom level for the entire page, though it's
not
> entirely obvious to me if that should always be the case. Maybe there will be
> cases where we want a nested frame to render with a different zoom?

I think you probably want the zoom to stay the same for all frames in a
WebContents.

> In all honesty, I haven't really thought about how HostZoomMap will need to
> change for OOPIF beyond what I've said above: we need a plan.

Yes, and I think that time is coming soon, since we're getting closer to turning
on OOPIFs in some cases.  I'll start a thread on site-isolation-dev@ for it.

> All attempts to use methods like HostZoomMap::GetForWebContents() will need to
> be revised as part of the change to OOPIF.
> 
> In the short term we can change the call here to use
> HostZoomMap::GetForWebContents(this) so we will easily catch it when we update
> the rest of these calls.

Works for me.

On 2015/10/22 15:09:01, Fady Samuel wrote:
> I'm not sure this needs changing for OOPIFs.
> 
> Zoom is a property that applies for the whole page including iframes. Zoom is
> also a property that stored on a per-origin (really per-SiteInstance) basis.
The
> zoom of the page is the zoom of the site instance of the top level frame. All
> subframes inherit the same zoom level as the top level frame.

I'm ok with this proposed behavior, but that's not how it works today.  If you
zoom a page with an OOPIF, the OOPIF doesn't match the zoom level of the page. 
We definitely need some changes to make it work.


On 2015/10/22 15:29:02, wjmaclean wrote:
> Certainly for apps we can imagine setting a different zoom for a WebView's
> contents (e.g. BrowserSample might want to zoom tab contents in a manner
similar
> to how Chrome currently zooms ... Chrome UI elements don't zoom with the tab
> contents, and Browser Zoom might want to mimic this behaviour).

That's a different case, though.  GuestViews will have a separate WebContents
from their embedder page, so they can have separate zoom levels if you want.  I
think you probably want to keep zoom as a WebContents-level setting.

> 
> Although it might be a headache for the (designers of a) frame containing an
> iframe, I think it's worth at least considering the possibility that different
> frames *could* set their zoom levels independently.

[wjmaclean, 2015-10-22 19:27:45]

> 
> That's a different case, though.  GuestViews will have a separate WebContents
> from their embedder page, so they can have separate zoom levels if you want. 
I
> think you probably want to keep zoom as a WebContents-level setting.
> 

In this case the modifications could be fairly simple:

1) Make sure we set/get zoom levels based on SiteInstance of top-level frame in
a WebContents, and
2) Propagate any changes to all the sub-frames in the frame tree.

At first glance that seems fairly straightforward.

[Kevin McNee - google account, 2015-10-22 20:28:48]

https://codereview.chromium.org/1408393003/diff/40001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1408393003/diff/40001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:3130:
static_cast<HostZoomMapImpl*>(HostZoomMap::Get(GetSiteInstance()));
On 2015/10/22 19:09:25, Charlie Reis wrote:
> On 2015/10/22 15:03:14, wjmaclean wrote:
> > On 2015/10/21 21:37:09, Charlie Reis wrote:
> > > I'm concerned about this, because there isn't a single SiteInstance for
> > > WebContents anymore (due to out-of-process iframes).  Things like
> > > GetSiteInstance(), GetRenderProcessHost(), and GetRoutingID() generally
> > > shouldn't be called on WebContents, since each RenderFrameHost can have a
> > > different value for them.
> > > 
> > > In this case, the zoom logic hasn't been updated for OOPIFs yet. 
> wjmaclean@:
> > > Have you thought about how it will have to change?  Here, it seems like
each
> > > RenderViewHost would have to be notified.
> > > 
> > > I could be ok with landing this change without fixing it for OOPIFs, but
we
> > need
> > > a TODO mentioning a bug (maybe 528407?) to fix it.
> > 
> > I would like to suggest that we do land this change, with a bug as you
> suggest.
> 
> Sure.
> 
> > The entire, current host-zoom infrastructure is predicated on tying zoom
level
> > to a site instance, which related to the notion of an origin.
> > 
> > I suppose, going forward, we might expect the outermost (top-level?) frame's
> > origin/site-instance to set the zoom level for the entire page, though it's
> not
> > entirely obvious to me if that should always be the case. Maybe there will
be
> > cases where we want a nested frame to render with a different zoom?
> 
> I think you probably want the zoom to stay the same for all frames in a
> WebContents.
> 
> > In all honesty, I haven't really thought about how HostZoomMap will need to
> > change for OOPIF beyond what I've said above: we need a plan.
> 
> Yes, and I think that time is coming soon, since we're getting closer to
turning
> on OOPIFs in some cases.  I'll start a thread on site-isolation-dev@ for it.
> 
> > All attempts to use methods like HostZoomMap::GetForWebContents() will need
to
> > be revised as part of the change to OOPIF.
> > 
> > In the short term we can change the call here to use
> > HostZoomMap::GetForWebContents(this) so we will easily catch it when we
update
> > the rest of these calls.
> 
> Works for me.
> 
> On 2015/10/22 15:09:01, Fady Samuel wrote:
> > I'm not sure this needs changing for OOPIFs.
> > 
> > Zoom is a property that applies for the whole page including iframes. Zoom
is
> > also a property that stored on a per-origin (really per-SiteInstance) basis.
> The
> > zoom of the page is the zoom of the site instance of the top level frame.
All
> > subframes inherit the same zoom level as the top level frame.
> 
> I'm ok with this proposed behavior, but that's not how it works today.  If you
> zoom a page with an OOPIF, the OOPIF doesn't match the zoom level of the page.

> We definitely need some changes to make it work.
> 
> 
> On 2015/10/22 15:29:02, wjmaclean wrote:
> > Certainly for apps we can imagine setting a different zoom for a WebView's
> > contents (e.g. BrowserSample might want to zoom tab contents in a manner
> similar
> > to how Chrome currently zooms ... Chrome UI elements don't zoom with the tab
> > contents, and Browser Zoom might want to mimic this behaviour).
> 
> That's a different case, though.  GuestViews will have a separate WebContents
> from their embedder page, so they can have separate zoom levels if you want. 
I
> think you probably want to keep zoom as a WebContents-level setting.
> 
> > 
> > Although it might be a headache for the (designers of a) frame containing an
> > iframe, I think it's worth at least considering the possibility that
different
> > frames *could* set their zoom levels independently.
> 

Done.

https://codereview.chromium.org/1408393003/diff/40001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl_browsertest.cc (right):

https://codereview.chromium.org/1408393003/diff/40001/content/browser/web_con...
content/browser/web_contents/web_contents_impl_browsertest.cc:721: // See
WebContentsImplBrowserTest.ChangePageScale
On 2015/10/21 21:37:09, Charlie Reis wrote:
> nit: End sentence with a period.  (Same below on line 727.)

Done.

https://codereview.chromium.org/1408393003/diff/40001/content/browser/web_con...
content/browser/web_contents/web_contents_impl_browsertest.cc:755: // a
notification to the browser so that WebContentsObservers are notified.
On 2015/10/21 21:37:10, Charlie Reis wrote:
> nit: Move comment before test declaration and remove blank line below.

Done.

https://codereview.chromium.org/1408393003/diff/40001/content/browser/web_con...
content/browser/web_contents/web_contents_impl_browsertest.cc:757:
NavigateToURL(shell(), GURL(url::kAboutBlankURL));
On 2015/10/21 21:37:09, Charlie Reis wrote:
> You should probably use a real URL here.  about:blank is a special case that
> behaves differently in a lot of ways, including for things like HostZoomMap.
> 
> A common test page is embedded_test_server()->GetURL("/title1.html").

Done.

https://codereview.chromium.org/1408393003/diff/40001/content/common/view_mes...
File content/common/view_messages.h (right):

https://codereview.chromium.org/1408393003/diff/40001/content/common/view_mes...
content/common/view_messages.h:1237: // Sent when the renderer changes its page
scale factor
On 2015/10/21 21:37:10, Charlie Reis wrote:
> nit: End with period.

Done.

[Charlie Reis, 2015-10-22 20:53:47]

Thanks, LGTM with nit.  You'll need an IPC owner for view_messages.h.

https://codereview.chromium.org/1408393003/diff/60001/content/public/browser/...
File content/public/browser/host_zoom_map.h (right):

https://codereview.chromium.org/1408393003/diff/60001/content/public/browser/...
content/public/browser/host_zoom_map.h:75: // TODO(crbug.com/528407) Update this
behaviour to work with OOPIF.
nit: Generally, TODOs include a username of someone who owns or knows about the
bug in the parentheses, rather than a bug number.  Something like:

TODO(wjmaclean): Update... See https://crbug.com/528407.

(This is also probably worth putting in the .cc file rather than in the public
.h file, since it likely won't affect the contract.)

[Kevin McNee - google account, 2015-10-22 20:54:33]

mcnee@google.com changed reviewers:
+ kenrb@chromium.org

[Kevin McNee - google account, 2015-10-22 20:54:33]

kenrb@chromium.org: Please review changes in 

content/common/view_messages.h

Thanks.

[Kevin McNee - google account, 2015-10-22 21:15:55]

https://codereview.chromium.org/1408393003/diff/60001/content/public/browser/...
File content/public/browser/host_zoom_map.h (right):

https://codereview.chromium.org/1408393003/diff/60001/content/public/browser/...
content/public/browser/host_zoom_map.h:75: // TODO(crbug.com/528407) Update this
behaviour to work with OOPIF.
On 2015/10/22 20:53:47, Charlie Reis wrote:
> nit: Generally, TODOs include a username of someone who owns or knows about
the
> bug in the parentheses, rather than a bug number.  Something like:
> 
> TODO(wjmaclean): Update... See https://crbug.com/528407.
> 
> (This is also probably worth putting in the .cc file rather than in the public
> .h file, since it likely won't affect the contract.)

Done.

[kenrb, 2015-10-23 18:21:50]

ipc lgtm

[Kevin McNee - google account, 2015-10-23 18:28:28]

The CQ bit was checked by mcnee@google.com

[Kevin McNee - google account, 2015-10-23 18:28:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
wjmaclean@chromium.org, creis@chromium.org
Link to the patchset: https://codereview.chromium.org/1408393003/#ps80001
(title: "Fix TODO")

[commit-bot: I haz the power, 2015-10-23 18:29:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1408393003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1408393003/80001

[commit-bot: I haz the power, 2015-10-23 19:09:18]

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

[commit-bot: I haz the power, 2015-10-23 19:09:20]

Try jobs failed on following builders:
  android_clang_dbg_recipe on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)

[Kevin McNee - google account, 2015-10-26 17:36:24]

mcnee@google.com changed reviewers:
+ sgurun@chromium.org

[Kevin McNee - google account, 2015-10-26 17:36:24]

sgurun@chromium.org: Please review changes in 

android_webview/

Page scale factor changes are now available via
WebContentsObserver::OnPageScaleFactorChanged, so we get rid of
android_webview's own implementation.

Thanks.

[sgurun-gerrit only, 2015-10-26 17:38:38]

sgurun@chromium.org changed reviewers:
+ mnaganov@chromium.org
- sgurun@chromium.org

[sgurun-gerrit only, 2015-10-26 17:39:00]

On 2015/10/26 17:36:24, mcnee wrote:
> mailto:sgurun@chromium.org: Please review changes in 
> 
> android_webview/
> 
> Page scale factor changes are now available via
> WebContentsObserver::OnPageScaleFactorChanged, so we get rid of
> android_webview's own implementation.
> 
> Thanks.

mikhail, would you mind looking at this?

[mnaganov (inactive), 2015-10-26 17:52:33]

android_webview/ LGTM

[Kevin McNee - google account, 2015-10-26 17:59:27]

The CQ bit was checked by mcnee@google.com

[Kevin McNee - google account, 2015-10-26 17:59:27]

The patchset sent to the CQ was uploaded after l-g-t-m from kenrb@chromium.org,
wjmaclean@chromium.org, creis@chromium.org
Link to the patchset: https://codereview.chromium.org/1408393003/#ps100001
(title: "Remove android_webview implementation of OnPageScaleFactorChanged")

[commit-bot: I haz the power, 2015-10-26 18:00:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1408393003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1408393003/100001

[commit-bot: I haz the power, 2015-10-26 20:07:25]

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

[commit-bot: I haz the power, 2015-10-26 20:07:26]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

[Kevin McNee - google account, 2015-10-29 18:31:32]

Description was changed from

==========
Propagate pageScaleFactor to GuestViews.

We make all changes to pageScaleFactor available to the
browser process, so that WebContentsObservers may observe
them.

Previously, we were only sending notifications to the browser
process when the PageScaleFactorIsOne changed. Now that we're
notifying whenever the pageScaleFactor changed, we get rid of
this extra message and have the browser process handle the
PageScaleFactorIsOne logic as part of its handling of all
pageScaleFactor changes.

We also replace WebContentsImpl::ResetPageScale with
WebContentsImpl::SetPageScale, so that the browser process can
set any pageScaleFactor.

BUG=445319
==========

to

==========
Propagate pageScaleFactor to GuestViews.

We make all changes to pageScaleFactor available to the
browser process, so that WebContentsObservers may observe
them. Notifications also occur upon navigation.

Previously, we were only sending notifications to the browser
process when the PageScaleFactorIsOne changed. Now that we're
notifying whenever the pageScaleFactor changed, we get rid of
this extra message and have the browser process handle the
PageScaleFactorIsOne logic as part of its handling of all
pageScaleFactor changes.

We also replace WebContentsImpl::ResetPageScale with
WebContentsImpl::SetPageScale, so that the browser process can
set any pageScaleFactor.

BUG=445319
==========

[Kevin McNee - google account, 2015-10-29 18:45:15]

I made a change to the behaviour of OnPageScaleFactorChanged so that
notifications are sent upon navigation (similar to the behaviour of zoom
changes).

This affects code that creis@ and kenrb@ previously LGTM'd. Do you find this
change acceptable?

[Kevin McNee - google account, 2015-10-30 13:27:45]

The CQ bit was checked by mcnee@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-10-30 13:28:12]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1408393003/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1408393003/120001

[commit-bot: I haz the power, 2015-10-30 14:34:01]

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

[commit-bot: I haz the power, 2015-10-30 14:34:02]

Dry run: This issue passed the CQ dry run.

[kenrb, 2015-10-30 17:29:10]

On 2015/10/29 18:45:15, mcnee wrote:
> I made a change to the behaviour of OnPageScaleFactorChanged so that
> notifications are sent upon navigation (similar to the behaviour of zoom
> changes).
> 
> This affects code that creis@ and kenrb@ previously LGTM'd. Do you find this
> change acceptable?

Still lgtm.

[Charlie Reis, 2015-10-30 17:33:36]

On 2015/10/30 17:29:10, kenrb wrote:
> On 2015/10/29 18:45:15, mcnee wrote:
> > I made a change to the behaviour of OnPageScaleFactorChanged so that
> > notifications are sent upon navigation (similar to the behaviour of zoom
> > changes).
> > 
> > This affects code that creis@ and kenrb@ previously LGTM'd. Do you find this
> > change acceptable?
> 
> Still lgtm.

Wait, this is a big change, with a new IPC message and implications for
navigation.  Let me take a minute to review it.

[Charlie Reis, 2015-10-30 18:42:58]

https://codereview.chromium.org/1408393003/diff/120001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1408393003/diff/120001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:2981: Send(new
ViewMsg_GetPageScale(GetRoutingID()));
This doesn't make sense to me.  The renderer process has just told us that it
navigated, and now the browser process is turning around and telling it to do
something extra.  (This method should be about notifications going up to other
things in the browser process to let them know the navigation occurred.)

Is there a reason we can't just call render_view_->pageScaleFactorChanged() from
somewhere in RenderFrameImpl when the commit happens?  At first glance, I'm
looking at something like this:

https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r...

[Kevin McNee - google account, 2015-10-30 21:41:31]

https://codereview.chromium.org/1408393003/diff/120001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1408393003/diff/120001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:2981: Send(new
ViewMsg_GetPageScale(GetRoutingID()));
On 2015/10/30 18:42:58, Charlie Reis wrote:
> This doesn't make sense to me.  The renderer process has just told us that it
> navigated, and now the browser process is turning around and telling it to do
> something extra.  (This method should be about notifications going up to other
> things in the browser process to let them know the navigation occurred.)
> 
> Is there a reason we can't just call render_view_->pageScaleFactorChanged()
from
> somewhere in RenderFrameImpl when the commit happens?  At first glance, I'm
> looking at something like this:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r...

Done.

Yeah, that works.

[Charlie Reis, 2015-10-30 22:29:08]

Sorry, I'm still not happy with this, though it's heading in the right
direction.  I'm trying to understand which navigation cases require this message
to be sent.

(Also, it looks like you might have rebased between patch sets 6 and 7, which
made it difficult to review the diff between patch sets.  In the future, it can
help to upload a separate rebase patch set from the one with your changes.)

https://codereview.chromium.org/1408393003/diff/140001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1408393003/diff/140001/content/renderer/rende...
content/renderer/render_frame_impl.cc:2837: if (is_main_frame_)
In patch 6, you had this operation inside a
details.is_navigation_to_different_page() block, which meant that it wouldn't
happen for in-page navigations.  I thought that was intentional, because page
scale wouldn't change for in-page navigations.

Here you're not checking for that.  Should you check for something like
!navigation_state->WasWithinSamePage(), such as the block on line 2812?  Is that
the correct check?  (I'm not sure when page scale can change on a navigation.)

I was also trying to suggest the
internal_data->must_reset_scroll_and_scale_state() block, just based on the
name.  Is there some proper way to know whether the page scale has changed, and
thus we need to send an update?

In that sense, I'm also wondering why Blink isn't calling
pageScaleFactorChanged() directly after a navigation, if it does change in some
cases.  If it did, we might not need to add code in content for this.

[Kevin McNee - google account, 2015-11-02 14:45:56]

On 2015/10/30 22:29:08, Charlie Reis (slow til 11-16) wrote:
> Sorry, I'm still not happy with this, though it's heading in the right
> direction.  I'm trying to understand which navigation cases require this
message
> to be sent.
> 
> (Also, it looks like you might have rebased between patch sets 6 and 7, which
> made it difficult to review the diff between patch sets.  In the future, it
can
> help to upload a separate rebase patch set from the one with your changes.)
> 
>
https://codereview.chromium.org/1408393003/diff/140001/content/renderer/rende...
> File content/renderer/render_frame_impl.cc (right):
> 
>
https://codereview.chromium.org/1408393003/diff/140001/content/renderer/rende...
> content/renderer/render_frame_impl.cc:2837: if (is_main_frame_)
> In patch 6, you had this operation inside a
> details.is_navigation_to_different_page() block, which meant that it wouldn't
> happen for in-page navigations.  I thought that was intentional, because page
> scale wouldn't change for in-page navigations.
> 
> Here you're not checking for that.  Should you check for something like
> !navigation_state->WasWithinSamePage(), such as the block on line 2812?  Is
that
> the correct check?  (I'm not sure when page scale can change on a navigation.)
> 
> I was also trying to suggest the
> internal_data->must_reset_scroll_and_scale_state() block, just based on the
> name.  Is there some proper way to know whether the page scale has changed,
and
> thus we need to send an update?
> 
> In that sense, I'm also wondering why Blink isn't calling
> pageScaleFactorChanged() directly after a navigation, if it does change in
some
> cases.  If it did, we might not need to add code in content for this.

Sorry about the rebasing. I'll keep that in mind.

Blink calls pageScaleFactorChanged when the page scale factor actually changes.
In the case of resetting it due to a navigation, the message is dropped because
the RenderView is swapped out. I could add the message to
SwappedOutMessages::CanSendWhileSwappedOut
(https://code.google.com/p/chromium/codesearch#chromium/src/content/common/swa...),
which would keep the renderer and browser consistent. Then it's just a matter of
what to do upon the initial navigation and navigation between pages at the
default page scale factor (Blink wouldn't notify because the page scale factor
didn't actually change). Perhaps we could just do nothing here and make
observers initialize themselves with the default page scale factor and to not
rely on pageScaleFactorChanged to indicate navigation when the page scale factor
didn't actually change.

[Charlie Reis, 2015-11-03 00:44:31]

On 2015/11/02 14:45:56, mcnee wrote:
> On 2015/10/30 22:29:08, Charlie Reis (slow til 11-16) wrote:
> > Sorry, I'm still not happy with this, though it's heading in the right
> > direction.  I'm trying to understand which navigation cases require this
> message
> > to be sent.
> > 
> > (Also, it looks like you might have rebased between patch sets 6 and 7,
which
> > made it difficult to review the diff between patch sets.  In the future, it
> can
> > help to upload a separate rebase patch set from the one with your changes.)
> > 
> >
>
https://codereview.chromium.org/1408393003/diff/140001/content/renderer/rende...
> > File content/renderer/render_frame_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/1408393003/diff/140001/content/renderer/rende...
> > content/renderer/render_frame_impl.cc:2837: if (is_main_frame_)
> > In patch 6, you had this operation inside a
> > details.is_navigation_to_different_page() block, which meant that it
wouldn't
> > happen for in-page navigations.  I thought that was intentional, because
page
> > scale wouldn't change for in-page navigations.
> > 
> > Here you're not checking for that.  Should you check for something like
> > !navigation_state->WasWithinSamePage(), such as the block on line 2812?  Is
> that
> > the correct check?  (I'm not sure when page scale can change on a
navigation.)
> > 
> > I was also trying to suggest the
> > internal_data->must_reset_scroll_and_scale_state() block, just based on the
> > name.  Is there some proper way to know whether the page scale has changed,
> and
> > thus we need to send an update?
> > 
> > In that sense, I'm also wondering why Blink isn't calling
> > pageScaleFactorChanged() directly after a navigation, if it does change in
> some
> > cases.  If it did, we might not need to add code in content for this.
> 
> Sorry about the rebasing. I'll keep that in mind.
> 
> Blink calls pageScaleFactorChanged when the page scale factor actually
changes.
> In the case of resetting it due to a navigation, the message is dropped
because
> the RenderView is swapped out.

Ah, interesting.  However, that's only in the case of cross-process navigations
(e.g., typing a cross-site URL in the omnibox).  Does it already handle
same-process navigations correctly (e.g., typing a same-site URL in the
omnibox)?

> I could add the message to
> SwappedOutMessages::CanSendWhileSwappedOut
>
(https://code.google.com/p/chromium/codesearch#chromium/src/content/common/swa...),
> which would keep the renderer and browser consistent.

This seems reasonable to me.  (And to make things better, the filtering will
likely go away once we finish killing swapped out mode for --site-per-process.)

> Then it's just a matter of
> what to do upon the initial navigation and navigation between pages at the
> default page scale factor (Blink wouldn't notify because the page scale factor
> didn't actually change). Perhaps we could just do nothing here and make
> observers initialize themselves with the default page scale factor and to not
> rely on pageScaleFactorChanged to indicate navigation when the page scale
factor
> didn't actually change.

Initializing with the correct value sounds like a good plan.  We shouldn't have
to send a notification on creation.

[Kevin McNee - google account, 2015-11-05 16:37:31]

Description was changed from

==========
Propagate pageScaleFactor to GuestViews.

We make all changes to pageScaleFactor available to the
browser process, so that WebContentsObservers may observe
them. Notifications also occur upon navigation.

Previously, we were only sending notifications to the browser
process when the PageScaleFactorIsOne changed. Now that we're
notifying whenever the pageScaleFactor changed, we get rid of
this extra message and have the browser process handle the
PageScaleFactorIsOne logic as part of its handling of all
pageScaleFactor changes.

We also replace WebContentsImpl::ResetPageScale with
WebContentsImpl::SetPageScale, so that the browser process can
set any pageScaleFactor.

BUG=445319
==========

to

==========
Propagate pageScaleFactor to GuestViews.

We make all changes to pageScaleFactor available to the
browser process, so that WebContentsObservers may observe
them.

Previously, we were only sending notifications to the browser
process when the PageScaleFactorIsOne changed. Now that we're
notifying whenever the pageScaleFactor changed, we get rid of
this extra message and have the browser process handle the
PageScaleFactorIsOne logic as part of its handling of all
pageScaleFactor changes.

We also replace WebContentsImpl::ResetPageScale with
WebContentsImpl::SetPageScale, so that the browser process can
set any pageScaleFactor.

BUG=445319
==========

[Kevin McNee - google account, 2015-11-05 17:00:49]

> > Blink calls pageScaleFactorChanged when the page scale factor actually
> changes.
> > In the case of resetting it due to a navigation, the message is dropped
> because
> > the RenderView is swapped out.
> 
> Ah, interesting.  However, that's only in the case of cross-process
navigations
> (e.g., typing a cross-site URL in the omnibox).  Does it already handle
> same-process navigations correctly (e.g., typing a same-site URL in the
> omnibox)?

Right.
 
> > I could add the message to
> > SwappedOutMessages::CanSendWhileSwappedOut
> >
>
(https://code.google.com/p/chromium/codesearch#chromium/src/content/common/swa...),
> > which would keep the renderer and browser consistent.
> 
> This seems reasonable to me.  (And to make things better, the filtering will
> likely go away once we finish killing swapped out mode for
--site-per-process.)

Done.
content/ is now back to what you originally LGTM'd plus the addition of the
message to CanSendWhileSwappedOut and the updated test.
Just to confirm: content/ LGTM?

Alright, now it's just a matter of making the android_webview tests happy with
this behaviour.

[Charlie Reis, 2015-11-06 05:10:54]

On 2015/11/05 17:00:49, mcnee wrote:
> > > Blink calls pageScaleFactorChanged when the page scale factor actually
> > changes.
> > > In the case of resetting it due to a navigation, the message is dropped
> > because
> > > the RenderView is swapped out.
> > 
> > Ah, interesting.  However, that's only in the case of cross-process
> navigations
> > (e.g., typing a cross-site URL in the omnibox).  Does it already handle
> > same-process navigations correctly (e.g., typing a same-site URL in the
> > omnibox)?
> 
> Right.
>  
> > > I could add the message to
> > > SwappedOutMessages::CanSendWhileSwappedOut
> > >
> >
>
(https://code.google.com/p/chromium/codesearch#chromium/src/content/common/swa...),
> > > which would keep the renderer and browser consistent.
> > 
> > This seems reasonable to me.  (And to make things better, the filtering will
> > likely go away once we finish killing swapped out mode for
> --site-per-process.)
> 
> Done.
> content/ is now back to what you originally LGTM'd plus the addition of the
> message to CanSendWhileSwappedOut and the updated test.
> Just to confirm: content/ LGTM?
> 

Yes, content/ LGTM again.

> Alright, now it's just a matter of making the android_webview tests happy with
> this behaviour.

[Kevin McNee - google account, 2015-11-06 15:56:15]

On 2015/10/26 17:52:33, mnaganov wrote:
> android_webview/ LGTM

mnaganov@, we're now only sending page scale factor notifications when the page
scale factor actually changes and expect observers to initialize themselves with
the correct value. There were a few tests that failed due to this, which were
fixed by initializing AwLayoutSizer's page scale factor to the correct value. Do
you approve of these changes?

Also, there are a bunch of calls to onPageScaleChanged in AwLayoutSizerTest.java
to do the initialization, which are presumably no longer necessary. Do you want
me to remove those?

[mnaganov (inactive), 2015-11-06 20:54:35]

On 2015/11/06 15:56:15, mcnee wrote:
> On 2015/10/26 17:52:33, mnaganov wrote:
> > android_webview/ LGTM
> 
> mnaganov@, we're now only sending page scale factor notifications when the
page
> scale factor actually changes and expect observers to initialize themselves
with
> the correct value. There were a few tests that failed due to this, which were
> fixed by initializing AwLayoutSizer's page scale factor to the correct value.
Do
> you approve of these changes?
> 

I think that the initial change of the page scale (from 0 to the real value)
isn't a strong contract -- at least I don't see any explicit test for that in
Android CTS tests for WebView. The tests only verify that page scale changes
after you zoom in / out, which will be preserved by your CL, I guess.

Thus it's fine to remove the expectations of scale change when the page
is initially displayed with a scale of 1.

> Also, there are a bunch of calls to onPageScaleChanged in
AwLayoutSizerTest.java
> to do the initialization, which are presumably no longer necessary. Do you
want
> me to remove those?

Yes, you can try removing them, thanks!

[Kevin McNee - google account, 2015-11-09 14:15:49]

> > Also, there are a bunch of calls to onPageScaleChanged in
> AwLayoutSizerTest.java
> > to do the initialization, which are presumably no longer necessary. Do you
> want
> > me to remove those?
> 
> Yes, you can try removing them, thanks!

mnaganov@:
Alright, it looks like the android tests are happy with that.

android_webview/ LGTM?

[mnaganov (inactive), 2015-11-09 18:15:52]

Yes, still LGTM.

[Kevin McNee - google account, 2015-11-09 18:21:37]

The CQ bit was checked by mcnee@google.com

[Kevin McNee - google account, 2015-11-09 18:21:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
wjmaclean@chromium.org, kenrb@chromium.org, creis@chromium.org
Link to the patchset: https://codereview.chromium.org/1408393003/#ps200001
(title: "Remove redundant initialization of AwLayoutSizer's page scale factor in
tests")

[commit-bot: I haz the power, 2015-11-09 18:22:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1408393003/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1408393003/200001

[commit-bot: I haz the power, 2015-11-09 19:37:53]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2015-11-09 19:39:29]

Patchset 11 (id:??) landed as
https://crrev.com/432e47d6fc9b1a154900d91ad4841632160af238
Cr-Commit-Position: refs/heads/master@{#358631}

[wjmaclean, 2015-11-18 16:37:37]

mcnee@ - I've looked this CL over to see if I can spot possible candidates for
the Android perf regression. I've included a couple of comments below.

https://codereview.chromium.org/1408393003/diff/200001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1408393003/diff/200001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:3157: void
WebContentsImpl::OnPageScaleFactorChanged(float page_scale_factor) {
is page_scale_factor always guaranteed to be different each time this function
is called?

https://codereview.chromium.org/1408393003/diff/200001/content/renderer/rende...
File content/renderer/render_view_impl.cc (left):

https://codereview.chromium.org/1408393003/diff/200001/content/renderer/rende...
content/renderer/render_view_impl.cc:3388: return;
Is this only meant to catch the case when the page scale factor changes from one
non-one value to another? Or is there the possibility that this function can be
called without pageScaleFactor having actually changed?

https://codereview.chromium.org/1408393003/diff/200001/content/renderer/rende...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/1408393003/diff/200001/content/renderer/rende...
content/renderer/render_view_impl.cc:3385: 
I notice that AwRenderViewExt always checks to see if the new page scale is
unique before sending the IPC ... should we do so here as well? Is there any
chance this is causing unnecessary layout/drawing?

[Kevin McNee - google account, 2015-11-18 19:41:34]

Here is the CL for the Android perf regression:
https://codereview.chromium.org/1457013002/

https://codereview.chromium.org/1408393003/diff/200001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1408393003/diff/200001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:3157: void
WebContentsImpl::OnPageScaleFactorChanged(float page_scale_factor) {
On 2015/11/18 16:37:37, wjmaclean wrote:
> is page_scale_factor always guaranteed to be different each time this function
> is called?

We only get notifications when the page scale factor actually changes. See
blink::VisualViewport::setScaleAndLocation.

https://codereview.chromium.org/1408393003/diff/200001/content/renderer/rende...
File content/renderer/render_view_impl.cc (left):

https://codereview.chromium.org/1408393003/diff/200001/content/renderer/rende...
content/renderer/render_view_impl.cc:3388: return;
On 2015/11/18 16:37:37, wjmaclean wrote:
> Is this only meant to catch the case when the page scale factor changes from
one
> non-one value to another? Or is there the possibility that this function can
be
> called without pageScaleFactor having actually changed?

The former. Previously the browser only cared about whether the page scale
factor was one.

See previous comment about no extra calls.

https://codereview.chromium.org/1408393003/diff/200001/content/renderer/rende...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/1408393003/diff/200001/content/renderer/rende...
content/renderer/render_view_impl.cc:3385: 
On 2015/11/18 16:37:37, wjmaclean wrote:
> I notice that AwRenderViewExt always checks to see if the new page scale is
> unique before sending the IPC ... should we do so here as well? Is there any
> chance this is causing unnecessary layout/drawing?

AwRenderViewExt was polling for changes to the page scale factor. Hence the need
to check that it changed.