Make PageScaleFactor work for oopif subframes.

Make PageScaleFactor work for oopif subframes.

Ooopif sub-frames render in a separate process, with a separate
LayerTree and compositor. The current CC framework expects page scale
to be a main-frame concept, but at present all the subframe really
needs is a correct value for the raster scale.

This can be accomplished in CC by always including the layer tree's
page scale factor in the raster scale if there's no PageScaleLayer,
which is always the case for an oopif sub-frame.

This CL includes the basic plumbing to push notification of page scale
changes, via the deltas from GesturePinchUpdate, to all the page's
RenderViews. It also includes the necessary changes to restore page
scale from the history on navigation back to a previously zoomed page.

Design doc:

https://docs.google.com/document/d/1dBuFglDPE1PoYN33IAR6O5BJ7u5Hityc6sM6jPF4a3M

BUG=654917
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

[wjmaclean, 2017-01-23 17:56:11]

Description was changed from

==========
Make PageScaleFactor work for oopif subframes.

Ooopif sub-frames render in a separate process, with a separate
LayerTree and compositor. The current CC framework expects page scale
to be a main-frame concept, but at present all the subframe really
needs is a correct value for the raster scale.

This can be accomplished in CC by always including the layer tree's
page scale factor in the raster scale if there's no PageScaleLayer,
which is always the case for an oopif sub-frame.

This CL includes the basic plumbing to push notification of page scale
changes, via the deltas from GesturePinchUpdate, to all the page's
RenderViews. It also includes the necessary changes to restore page
scale from the history on navigation back to a previously zoomed page.

BUG=654917
==========

to

==========
Make PageScaleFactor work for oopif subframes.

Ooopif sub-frames render in a separate process, with a separate
LayerTree and compositor. The current CC framework expects page scale
to be a main-frame concept, but at present all the subframe really
needs is a correct value for the raster scale.

This can be accomplished in CC by always including the layer tree's
page scale factor in the raster scale if there's no PageScaleLayer,
which is always the case for an oopif sub-frame.

This CL includes the basic plumbing to push notification of page scale
changes, via the deltas from GesturePinchUpdate, to all the page's
RenderViews. It also includes the necessary changes to restore page
scale from the history on navigation back to a previously zoomed page.

BUG=654917
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[wjmaclean, 2017-01-23 17:57:01]

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

[commit-bot: I haz the power, 2017-01-23 17:57:38]

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-01-23 18:38:22]

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

[commit-bot: I haz the power, 2017-01-23 18:38:23]

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

[wjmaclean, 2017-01-23 20:21:31]

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

[commit-bot: I haz the power, 2017-01-23 20:21:56]

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-01-23 22:57:13]

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

[commit-bot: I haz the power, 2017-01-23 22:57:14]

Dry run: This issue passed the CQ dry run.

[wjmaclean, 2017-01-24 14:20:19]

Description was changed from

==========
Make PageScaleFactor work for oopif subframes.

Ooopif sub-frames render in a separate process, with a separate
LayerTree and compositor. The current CC framework expects page scale
to be a main-frame concept, but at present all the subframe really
needs is a correct value for the raster scale.

This can be accomplished in CC by always including the layer tree's
page scale factor in the raster scale if there's no PageScaleLayer,
which is always the case for an oopif sub-frame.

This CL includes the basic plumbing to push notification of page scale
changes, via the deltas from GesturePinchUpdate, to all the page's
RenderViews. It also includes the necessary changes to restore page
scale from the history on navigation back to a previously zoomed page.

BUG=654917
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Make PageScaleFactor work for oopif subframes.

Ooopif sub-frames render in a separate process, with a separate
LayerTree and compositor. The current CC framework expects page scale
to be a main-frame concept, but at present all the subframe really
needs is a correct value for the raster scale.

This can be accomplished in CC by always including the layer tree's
page scale factor in the raster scale if there's no PageScaleLayer,
which is always the case for an oopif sub-frame.

This CL includes the basic plumbing to push notification of page scale
changes, via the deltas from GesturePinchUpdate, to all the page's
RenderViews. It also includes the necessary changes to restore page
scale from the history on navigation back to a previously zoomed page.

Design doc:

https://docs.google.com/document/d/1dBuFglDPE1PoYN33IAR6O5BJ7u5Hityc6sM6jPF4a3M

BUG=654917
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[wjmaclean, 2017-01-24 15:42:39]

wjmaclean@chromium.org changed reviewers:
+ creis@chromium.org, enne@chromium.org, kenrb@chromium.org

[wjmaclean, 2017-01-24 15:50:30]

kenrb@ - Would you please do a pre-review on the non-cc parts of this?

enne@ - Would you please take a look at the cc/ parts?

Background:

There's a design doc in the description but here's a tl;dr

We need to update raster-scale for oopif subframes, but at present they don't
get page-scale information. Blindly plumbing page-scale to their LayerTrees
causes a lot of unwanted geometry changes, due to assumptions about page-scale
being a purely main-frame concept.

So this CL plumbs a distinct quantity, SubframePageScaleFactor, that only
affects raster scale for subframes, and updates via the same event flow as
page-scale does for the main frame (thus keeping in sync, throttling re-rasters,
etc.). Keeping a separate entity seems valuable because:
1) Salamander folks tell me that long-term subframe raster scale will be solved
by them, although that's a ways out ...
2) ... so this CL gives us a short-term solution that's easy to remove when
Salamander is ready.
3) The separate pathway avoids complicating existing geometry logic tied to
page-scale.

Thoughts?

[kenrb, 2017-01-24 17:13:27]

Thanks for doing this. I have some suggestions, and a couple of things I need
clarification on.

https://codereview.chromium.org/2652643004/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_input_event_router.cc
(right):

https://codereview.chromium.org/2652643004/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_input_event_router.cc:702: //
propagating page scaleg
nit: Typo.

https://codereview.chromium.org/2652643004/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_input_event_router.cc:706:
entry.second->ProcessGestureEvent(*event, latency);
Will this eventually get removed? It feels weird to be dispatching these events
to every process, but it would be fine if this won't be necessary after
Salamander ships.

https://codereview.chromium.org/2652643004/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_base.h (right):

https://codereview.chromium.org/2652643004/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_base.h:145: void
SetPageScaleFactorForSubframes(float page_scale);
Is this called anywhere? I can't find a call site.

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

https://codereview.chromium.org/2652643004/diff/20001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:762:
SetPageScaleFactorForSubframes)
This message should only ever come from the top-level frame, right?

Could we move the handler to RenderFrame, which can verify that it is the
correct frame to be receiving the message, and have it call the delegate method?

https://codereview.chromium.org/2652643004/diff/20001/content/common/frame_me...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/2652643004/diff/20001/content/common/frame_me...
content/common/frame_messages.h:945:
IPC_MESSAGE_ROUTED1(FrameHostMsg_RestorePageScaleFactorOnLoad,
nit: This could use a comment.

https://codereview.chromium.org/2652643004/diff/20001/content/renderer/render...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/2652643004/diff/20001/content/renderer/render...
content/renderer/render_view_impl.cc:2383:
!webview()->mainFrame()->isWebLocalFrame()) {
It's cleaner to test the inverted conditions, and return early if they aren't
met.

https://codereview.chromium.org/2652643004/diff/20001/content/renderer/render...
content/renderer/render_view_impl.cc:2386: // every frame in a renderer.
That is technically true, but an alternative is to send a message individually
just to local frame roots. WebContentsImpl::GetRenderWidgetHostViewsInTree()
facilitates doing that. That is likely a better approach since there is no state
getting attached to a page-level object in the renderer.

https://codereview.chromium.org/2652643004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2652643004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1456:
client()->didRestorePageScaleFactorOnLoad(visualViewport.scale());
I am not clear on why this is necessary. Does this code path imply that we have
just completed a page load? Doesn't that mean that previously-existing OOPIFs
will all have been destroyed?

[wjmaclean, 2017-01-24 18:24:26]

Some early answers to kenrb@'s questions. I'll upload a new CL with the changes
soon.

https://codereview.chromium.org/2652643004/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_input_event_router.cc
(right):

https://codereview.chromium.org/2652643004/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_input_event_router.cc:702: //
propagating page scaleg
On 2017/01/24 17:13:27, kenrb wrote:
> nit: Typo.

Done.

https://codereview.chromium.org/2652643004/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_input_event_router.cc:706:
entry.second->ProcessGestureEvent(*event, latency);
On 2017/01/24 17:13:27, kenrb wrote:
> Will this eventually get removed? It feels weird to be dispatching these
events
> to every process, but it would be fine if this won't be necessary after
> Salamander ships.

Yes, this will disappear with Salamander.

https://codereview.chromium.org/2652643004/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_base.h (right):

https://codereview.chromium.org/2652643004/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_base.h:145: void
SetPageScaleFactorForSubframes(float page_scale);
On 2017/01/24 17:13:27, kenrb wrote:
> Is this called anywhere? I can't find a call site.

Hmm, no ... I must have copied a stale piece of code during the gardening
process. I think I had originally planned on having RWHIER propagate via
WebContentsImpl, but then switched to sending via the GesturePinchEvents, except
for the part about reloading on restore.

Removed.

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

https://codereview.chromium.org/2652643004/diff/20001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:762:
SetPageScaleFactorForSubframes)
On 2017/01/24 17:13:27, kenrb wrote:
> This message should only ever come from the top-level frame, right?
> 
> Could we move the handler to RenderFrame, which can verify that it is the
> correct frame to be receiving the message, and have it call the delegate
method?

Sure, I'll look into doing that and include it in the next CL.

https://codereview.chromium.org/2652643004/diff/20001/content/common/frame_me...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/2652643004/diff/20001/content/common/frame_me...
content/common/frame_messages.h:945:
IPC_MESSAGE_ROUTED1(FrameHostMsg_RestorePageScaleFactorOnLoad,
On 2017/01/24 17:13:27, kenrb wrote:
> nit: This could use a comment.

Done.

https://codereview.chromium.org/2652643004/diff/20001/content/renderer/render...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/2652643004/diff/20001/content/renderer/render...
content/renderer/render_view_impl.cc:2383:
!webview()->mainFrame()->isWebLocalFrame()) {
On 2017/01/24 17:13:27, kenrb wrote:
> It's cleaner to test the inverted conditions, and return early if they aren't
> met.

Done.

https://codereview.chromium.org/2652643004/diff/20001/content/renderer/render...
content/renderer/render_view_impl.cc:2386: // every frame in a renderer.
On 2017/01/24 17:13:27, kenrb wrote:
> That is technically true, but an alternative is to send a message individually
> just to local frame roots. WebContentsImpl::GetRenderWidgetHostViewsInTree()
> facilitates doing that. That is likely a better approach since there is no
state
> getting attached to a page-level object in the renderer.

Ok, I didn't know about that ... I'll look into that. Would this handler move to
RenderWidget in that case? I'm guessing 'yes' ...

https://codereview.chromium.org/2652643004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2652643004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1456:
client()->didRestorePageScaleFactorOnLoad(visualViewport.scale());
On 2017/01/24 17:13:27, kenrb wrote:
> I am not clear on why this is necessary. Does this code path imply that we
have
> just completed a page load? Doesn't that mean that previously-existing OOPIFs
> will all have been destroyed?

Hmm, I don't think so, otherwise one would not expect this to work on pages with
oopif subframes, and it seems to. Wouldn't it make sense that any previously
destroyed subframes would be re-created by the time the page load is completed?

I think any subframes that load later get the PSF from the WebContents as they
are loaded, but I'll have to verify that.

[kenrb, 2017-01-24 18:47:31]

https://codereview.chromium.org/2652643004/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_input_event_router.cc
(right):

https://codereview.chromium.org/2652643004/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_input_event_router.cc:706:
entry.second->ProcessGestureEvent(*event, latency);
On 2017/01/24 18:24:26, wjmaclean wrote:
> On 2017/01/24 17:13:27, kenrb wrote:
> > Will this eventually get removed? It feels weird to be dispatching these
> events
> > to every process, but it would be fine if this won't be necessary after
> > Salamander ships.
> 
> Yes, this will disappear with Salamander.

This warrants a TODO, marking it as temporary, then.

https://codereview.chromium.org/2652643004/diff/20001/content/renderer/render...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/2652643004/diff/20001/content/renderer/render...
content/renderer/render_view_impl.cc:2386: // every frame in a renderer.
On 2017/01/24 18:24:26, wjmaclean wrote:
> On 2017/01/24 17:13:27, kenrb wrote:
> > That is technically true, but an alternative is to send a message
individually
> > just to local frame roots. WebContentsImpl::GetRenderWidgetHostViewsInTree()
> > facilitates doing that. That is likely a better approach since there is no
> state
> > getting attached to a page-level object in the renderer.
> 
> Ok, I didn't know about that ... I'll look into that. Would this handler move
to
> RenderWidget in that case? I'm guessing 'yes' ...

Yes. Hypothetically it could go on RenderFrameImpl but RenderWidget makes more
sense.

https://codereview.chromium.org/2652643004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2652643004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1456:
client()->didRestorePageScaleFactorOnLoad(visualViewport.scale());
On 2017/01/24 18:24:26, wjmaclean wrote:
> On 2017/01/24 17:13:27, kenrb wrote:
> > I am not clear on why this is necessary. Does this code path imply that we
> have
> > just completed a page load? Doesn't that mean that previously-existing
OOPIFs
> > will all have been destroyed?
> 
> Hmm, I don't think so, otherwise one would not expect this to work on pages
with
> oopif subframes, and it seems to. Wouldn't it make sense that any previously
> destroyed subframes would be re-created by the time the page load is
completed?
> 
> I think any subframes that load later get the PSF from the WebContents as they
> are loaded, but I'll have to verify that.

What I meant was, if the frames have been destroyed and recreated, wouldn't they
have had their page scale factors reset anyway, because each would have a newly
initialized compositor?

[enne (OOO), 2017-01-24 18:58:47]

Just so I can understand this problem better, how do scale transforms on oopifs
from the parent affect the content scaling of that child renderer? It seems like
a very similar problem, so was curious if that was also an issue or if that was
somehow solved.

I can't comment on your doc, but I wonder if aelias@ meant CompositorFrameSink
(which used to be OutputSurface until danakj split them), in terms of just
looking at the CompositorFrameMetadata in the browser to get the page scale to
send directly to children instead of broadcasting input everywhere.  (Can these
get out of sync? Can the parent renderer pick a different page scale than what
the child renderer decides from the input? There are page scale max and mins
inside cc, for instance.)

I'm not totally sure I agree with your "modify page scale directly" conclusions.
 It seems like the only layer that you need in LayerTreeImpl is a page scale
layer.  There's a LayerTree::SetPageScaleFromImplSide that maybe could be
exposed instead of plumbing a separate value the whole way through.

[wjmaclean, 2017-01-24 21:43:07]

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

[commit-bot: I haz the power, 2017-01-24 21:43:28]

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

[wjmaclean, 2017-01-24 21:48:07]

On 2017/01/24 18:58:47, enne wrote:
> Just so I can understand this problem better, how do scale transforms on
oopifs
> from the parent affect the content scaling of that child renderer? It seems
like
> a very similar problem, so was curious if that was also an issue or if that
was
> somehow solved.

Do you mean CSS transforms? At present they do not appear to be handled.

> I can't comment on your doc, but I wonder if aelias@ meant CompositorFrameSink
> (which used to be OutputSurface until danakj split them), in terms of just
> looking at the CompositorFrameMetadata in the browser to get the page scale to
> send directly to children instead of broadcasting input everywhere.  (Can
these
> get out of sync? Can the parent renderer pick a different page scale than what
> the child renderer decides from the input? There are page scale max and mins
> inside cc, for instance.)

Thanks for the pointer to CompositorFrameMetaData ... I'll look at that and see
if it matches what aelias@ was describing, and if we can use that instead.

> I'm not totally sure I agree with your "modify page scale directly"
conclusions.
>  It seems like the only layer that you need in LayerTreeImpl is a page scale
> layer.  There's a LayerTree::SetPageScaleFromImplSide that maybe could be
> exposed instead of plumbing a separate value the whole way through.

It's been a while since I first created the doc/proof-of-concept CL, but I do
think I tried setting scale directly on the PageScaleLayer, without success
(though perhaps I was missing something in my implementation?).

I'll read through your comments again and try to work out alternatives that are
less invasive: I'd love to avoid the current design if something simpler is
available :-)

[wjmaclean, 2017-01-24 21:48:47]

kenrb@ - I've taken a first pass at addressing your comments ... let me know if
I understood your comments correctly.

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

https://codereview.chromium.org/2652643004/diff/20001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:762:
SetPageScaleFactorForSubframes)
On 2017/01/24 17:13:27, kenrb wrote:
> This message should only ever come from the top-level frame, right?
> 
> Could we move the handler to RenderFrame, which can verify that it is the
> correct frame to be receiving the message, and have it call the delegate
method?

Done.

[enne (OOO), 2017-01-24 21:49:47]

On 2017/01/24 at 21:48:07, wjmaclean wrote:
> On 2017/01/24 18:58:47, enne wrote:
> > Just so I can understand this problem better, how do scale transforms on
oopifs
> > from the parent affect the content scaling of that child renderer? It seems
like
> > a very similar problem, so was curious if that was also an issue or if that
was
> > somehow solved.
> 
> Do you mean CSS transforms? At present they do not appear to be handled.

Are you planning on handling them?

[commit-bot: I haz the power, 2017-01-24 22:09:56]

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

[commit-bot: I haz the power, 2017-01-24 22:09:57]

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

[wjmaclean, 2017-01-25 14:15:40]

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

[commit-bot: I haz the power, 2017-01-25 14:15:54]

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

[wjmaclean, 2017-01-25 14:25:48]

On 2017/01/24 21:49:47, enne wrote:
> On 2017/01/24 at 21:48:07, wjmaclean wrote:
> > 
> > Do you mean CSS transforms? At present they do not appear to be handled.
> 
> Are you planning on handling them?

Short answer: Yes.

Longer answer: But I need to learn more to decide how that work might be related
to this CL (if at all).

I just discovered that they're not accounted for in OOPIF yesterday, so I have
to figure out what exactly needs to be done. Things like isotropic scale factors
could be included in a patch like this, but anisotropic ones are trickier, and
more general transforms might need modification before being passed off to the
child. I don't know what issues need to be handled in total, but raster scale is
definitely among them.

Full handling of CSS transforms might make more sense in the Salamander world,
where (I presume) there could be a single transform tree accessible to all
surfaces being rendered?

[commit-bot: I haz the power, 2017-01-25 16:28:21]

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

[commit-bot: I haz the power, 2017-01-25 16:28:22]

Dry run: This issue passed the CQ dry run.

[enne (OOO), 2017-01-25 18:53:39]

On 2017/01/25 at 14:25:48, wjmaclean wrote:
> On 2017/01/24 21:49:47, enne wrote:
> > On 2017/01/24 at 21:48:07, wjmaclean wrote:
> > > 
> > > Do you mean CSS transforms? At present they do not appear to be handled.
> > 
> > Are you planning on handling them?
> 
> Short answer: Yes.
> 
> Longer answer: But I need to learn more to decide how that work might be
related to this CL (if at all).
> 
> I just discovered that they're not accounted for in OOPIF yesterday, so I have
to figure out what exactly needs to be done. Things like isotropic scale factors
could be included in a patch like this, but anisotropic ones are trickier, and
more general transforms might need modification before being passed off to the
child. I don't know what issues need to be handled in total, but raster scale is
definitely among them.
> 
> Full handling of CSS transforms might make more sense in the Salamander world,
where (I presume) there could be a single transform tree accessible to all
surfaces being rendered?

I don't see much difference between css transforms and page scale in terms of a
parent changing the raster scale of a child.  I'm less concerned with
anisotropic scale factors, etc.  I would really just want to have a child frame
behave the same with respect to raster scale regardless of its oopif / inpif
status, if I had to pick some behavior.

The reason I ask is because if you want to handle both, then that might change
the direction of where you get the scale from.  Maybe you would need to do
something like store LayerImpl::GetIdealContentsScale on the SurfaceDrawQuad and
then fish it out later from the browser process to send to the renderer.  That
would handle both page scale and css transforms in a way that rerouting input
wouldn't.

[wjmaclean, 2017-01-25 19:15:40]

On 2017/01/25 18:53:39, enne wrote:
> 
> I don't see much difference between css transforms and page scale in terms of
a
> parent changing the raster scale of a child.  I'm less concerned with
> anisotropic scale factors, etc.  I would really just want to have a child
frame
> behave the same with respect to raster scale regardless of its oopif / inpif
> status, if I had to pick some behavior.

Yes, that's where my thinking has been heading.
 
> The reason I ask is because if you want to handle both, then that might change
> the direction of where you get the scale from.  Maybe you would need to do
> something like store LayerImpl::GetIdealContentsScale on the SurfaceDrawQuad
and
> then fish it out later from the browser process to send to the renderer.  That
> would handle both page scale and css transforms in a way that rerouting input
> wouldn't.

That's an interesting thought :-)

So so we'd be fishing the value provided by GetIdealContentsScale in the
top-level (mainframe) renderer ... and then re-send it to the child renderers?
That seems reasonable, but I have a few questions (to aid my understanding of
the idea):

1) Presumably the child renderers could lag by a frame in this case? Might we
have to force an extra draw on the child side then if we noticed the child's ICS
hasn't caught up?

2) ICS is a computed value on the impl thread ... so would we assume then that,
for subframes only, if we get a value flowing down from the browser that doesn't
match what we are currently computing for the layer, we consider it an override?

3) Or, instead of (2), would it just get changed on the child's SurfaceDrawQuad,
prior to rasterization, with no need for any extra plumbing (i.e. is that what
you intended by "... to send to the renderer"?) This might also make the point I
raised in (1) moot as well.

If (3) was possible it might have the nice property of the child never really
needing to know about the page scale.

[enne (OOO), 2017-01-25 19:24:10]

On 2017/01/25 at 19:15:40, wjmaclean wrote:

> 1) Presumably the child renderers could lag by a frame in this case? Might we
have to force an extra draw on the child side then if we noticed the child's ICS
hasn't caught up?

Yeah, I think they're going to lag by a frame.  I'm not sure how to get around
something like that unless you wanted to block the parent from sending a new
frame until the child was ready, like you'd do with resize.  fsamuel might have
some opinions here.  This is something that input routing would do better at, so
maybe you want to punt on css transforms to get a better page scale experience? 
I'm not sure.

> 2) ICS is a computed value on the impl thread ... so would we assume then
that, for subframes only, if we get a value flowing down from the browser that
doesn't match what we are currently computing for the layer, we consider it an
override?

I think you'd start with the device scale and then always use the value from the
browser as soon as the first one was delivered.  (Or send some initial value
from history including page scale and then always use the browser version.)

> 3) Or, instead of (2), would it just get changed on the child's
SurfaceDrawQuad, prior to rasterization, with no need for any extra plumbing
(i.e. is that what you intended by "... to send to the renderer"?) This might
also make the point I raised in (1) moot as well.
> 
> If (3) was possible it might have the nice property of the child never really
needing to know about the page scale.

No, you'd still need to inform the child oopif of this scale so that it could
raster resources at the right scale.  I don't think this would change anything
about the way the SurfaceDrawQuad was shown on screen.

[Fady Samuel, 2017-01-25 19:26:16]

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

[Fady Samuel, 2017-01-25 19:26:17]

Adding some mus folks to cc: rjkroege@, danakj@, sadrul@ for future reference
(just FYI for now).

[wjmaclean, 2017-02-06 21:31:20]

So I've spent a bit of time investigating the SurfaceDrawQuad angle, and it
seems like it will be fairly non-trivial in certain respects:

* RenderWidgetHostView{Aura|Mac|Android|ChildFrame}::OnSwapCompositorFrame()
would need to dig through CompositorFrame looking for SDQs, then ...
* ... figure out the right child renderer to send a message to (not easily done
with the current structure.
* ... and since any child frame could itself be the parent of another child,
every RWHVX needs the machinery

But it did make me think of an alternative, which I'll outline here:

1) RenderWidgetHostViewChildFrame is responsible for creating the surface the
child draws into, and (via CrossProcessFrameConnector) get the surface
information to both the parent and the child

2) What if we created a Mojo Message pipe in RenderWidgetHostViewChildFrame, in
the same pathway that sets up the surface, and have it pass the end-points for
that pipe into both renderers. Then the parent can send a single float value
representing combined scale directly to the child renderer.

There are more details to figure out, but is something like this worth exploring
in general? It might provide a principled way to directly chain the compositors
wihtout involving the browser process. And it keeps the scale mechanism tightly
bound to the existing surface pathway.

If this seems plausible, I'll come up with more details.

[wjmaclean, 2017-02-07 15:56:45]

On 2017/02/06 21:31:20, wjmaclean wrote:
> 
> If this seems plausible, I'll come up with more details.

I've started a design doc related to this idea:
https://docs.google.com/document/d/1xdGIjNZ89aCUqcrolKwlwU5X_yflqLbx1w3IB-i26...

[enne (OOO), 2017-02-07 19:51:23]

Yeah, I think this sounds plausible and it sounds like fsamuel's in favor of it
as well.  It does sound like looking through surface quads is a bit more
complicated than I thought.  Sorry for sending you down a bad path!

I think most of my questions are for fsamuel, in terms of understanding how
it'll interact with
https://docs.google.com/document/d/1TAc_6IAbmTlChOwqkW_lB8NAKBBktnuquPk_Y0jO6...
and direct parent->child notifications of surface ids and sizes.  Is page scale
something that could just land first and then get modified to include size and
id later? Should the receiving end of the proposed scale-sending mojo pipe get
connected further up in Blink (WebViewImpl?) where scale can still be passed to
LayerTreeHost but size can get used by Blink in the future?

[wjmaclean, 2017-02-08 16:35:18]

On 2017/02/07 19:51:23, enne wrote:
> Yeah, I think this sounds plausible and it sounds like fsamuel's in favor of
it
> as well.  It does sound like looking through surface quads is a bit more
> complicated than I thought.  Sorry for sending you down a bad path!

heh, no worries! One never knows until they look :-)

> I think most of my questions are for fsamuel, in terms of understanding how
> it'll interact with
>
https://docs.google.com/document/d/1TAc_6IAbmTlChOwqkW_lB8NAKBBktnuquPk_Y0jO6...
> and direct parent->child notifications of surface ids and sizes.  Is page
scale
> something that could just land first and then get modified to include size and
> id later? Should the receiving end of the proposed scale-sending mojo pipe get
> connected further up in Blink (WebViewImpl?) where scale can still be passed
to
> LayerTreeHost but size can get used by Blink in the future?

I'll coordinate locally with fsamuel@ to figure out how best to coordinate/stage
this. There are both similaritites and differences (e.g. if an OOPIF gets the
changed scale information a bit late it may display a bit fuzzy, but that's not
as critical as guttering I think ...).

The receiving end could be connected higher up, perhaps WebFrameWidgetImpl,
although this information could also be plumbed back up from the LayerTree via
RenderWidgetCompositor ... I'll try and figure out the various options, and the
pros/cons (if any) of each.