Issue 2096633002: Adds scroll position/scale emulation to DevTools.

Eric Seckler

Description was changed from ========== Adds scroll position/scale + visual viewport size emulation. BUG= ========== ...

4 years, 6 months ago (2016-06-23 15:12:22 UTC) #1

Eric Seckler

Hi Sami and David, This is to demonstrate a first approach to overriding the scroll ...

4 years, 6 months ago (2016-06-23 15:22:59 UTC) #2

Sami

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

4 years, 6 months ago (2016-06-23 17:34:02 UTC) #3

Sami

I think the integration points here look reasonable. The logic is kind of spread out ...

4 years, 6 months ago (2016-06-23 17:34:03 UTC) #4

Eric Seckler

https://codereview.chromium.org/2096633002/diff/1/third_party/WebKit/Source/core/frame/FrameView.cpp File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2096633002/diff/1/third_party/WebKit/Source/core/frame/FrameView.cpp#newcode3302 third_party/WebKit/Source/core/frame/FrameView.cpp:3302: if (m_scrollOverridePosition.x() >= 0) On 2016/06/23 at 17:34:03, Sami ...

4 years, 6 months ago (2016-06-23 18:12:43 UTC) #5

bokan

Here's my suggestion: Create a class called ScrollAndScaleOverride or similar, owned by FrameHost. When Blink ...

4 years, 6 months ago (2016-06-24 14:33:54 UTC) #7

Sami

https://codereview.chromium.org/2096633002/diff/1/third_party/WebKit/Source/core/frame/FrameView.cpp File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2096633002/diff/1/third_party/WebKit/Source/core/frame/FrameView.cpp#newcode3302 third_party/WebKit/Source/core/frame/FrameView.cpp:3302: if (m_scrollOverridePosition.x() >= 0) On 2016/06/23 18:12:43, Eric Seckler ...

4 years, 6 months ago (2016-06-24 14:48:26 UTC) #8

Eric Seckler

On 2016/06/24 at 14:33:54, bokan wrote: > Here's my suggestion: > > Create a class ...

4 years, 6 months ago (2016-06-24 15:07:49 UTC) #9

Eric Seckler

On 2016/06/24 at 15:07:49, Eric Seckler wrote: > On 2016/06/24 at 14:33:54, bokan wrote: > ...

4 years, 5 months ago (2016-06-27 17:32:43 UTC) #10

On 2016/06/24 at 15:07:49, Eric Seckler wrote:
> On 2016/06/24 at 14:33:54, bokan wrote:
> > Here's my suggestion:
> > 
> > Create a class called ScrollAndScaleOverride or similar, owned by FrameHost.
When Blink receives the override values from DevTools protocol, it sets them in
this class.
> > 
> > They way this class enforces scroll offsets is through FrameView's and
VisualViewport's maximumScrollPosition and minimumScrollPosition methods. The
FrameView/VisualViewport can ask the ScrollAndScaleOverride if it has an
override for it and return that instead of the real min/max. This way the rest
of the ScrollableArea machinery can run as usual but the offset will be clamped
to your override.
> > 
> > For page scale, ScrollAndScaleOverride can set the userAgentContraint in
FrameHost's PageScaleConstraintsSet (for e.g. see
WebViewImpl::setIgnoreViewportTagScaleLimits).
> > 
> > The advantage of doing it this way is that we're reusing most of the
existing machinery in Blink most of the new complexity is isolated in the
ScrollAndScaleOverride where it's cohesive and easy to document in detail. WDYT?
> 
> I like the idea and will work out a prototype. It looks like this should also
solve the user-resize issue as the RootFrameViewport uses the max/mins returned
by FrameView/VisualViewport :) Thanks!

Just uploaded a second patch with the new logic using min/max scroll positions
and PageScaleConstraints. Sadly, this doesn't quite work yet.

I'm running into issues when the visual viewport is smaller than the frame.
Fixing the visual viewport scroll position doesn't seem to work for user scrolls
in this case, and it also scrolls "out of" a fixed frame scroll position. It
seems, the positions are capped correctly in Blink
(VisualViewport/FrameView.scrollPosition() report capped positions), but scrolls
over the synthetic limits are somehow still applied in the compositor (using
--disable-threaded-scrolling).

Setting the scale seems to work, but it has some weird side effects on scroll
bar positions: If I zoom the visual viewport (same size as frame), the scroll
bars then appear in the middle of the viewport, scaled the opposite way. I will
investigate this further tomorrow, and if I get stuck will share some
screenshots :)

Eric Seckler

https://codereview.chromium.org/2096633002/diff/1/content/browser/devtools/protocol/emulation_handler.cc File content/browser/devtools/protocol/emulation_handler.cc (right): https://codereview.chromium.org/2096633002/diff/1/content/browser/devtools/protocol/emulation_handler.cc#newcode223 content/browser/devtools/protocol/emulation_handler.cc:223: "Scroll position coordinates must be non-negative, not greater than" ...

4 years, 5 months ago (2016-06-27 17:32:58 UTC) #11

Eric Seckler

On 2016/06/27 at 17:32:43, Eric Seckler wrote: > On 2016/06/24 at 15:07:49, Eric Seckler wrote: ...

4 years, 5 months ago (2016-06-28 18:02:50 UTC) #12

On 2016/06/27 at 17:32:43, Eric Seckler wrote:
> On 2016/06/24 at 15:07:49, Eric Seckler wrote:
> > On 2016/06/24 at 14:33:54, bokan wrote:
> > > Here's my suggestion:
> > > 
> > > Create a class called ScrollAndScaleOverride or similar, owned by
FrameHost. When Blink receives the override values from DevTools protocol, it
sets them in this class.
> > > 
> > > They way this class enforces scroll offsets is through FrameView's and
VisualViewport's maximumScrollPosition and minimumScrollPosition methods. The
FrameView/VisualViewport can ask the ScrollAndScaleOverride if it has an
override for it and return that instead of the real min/max. This way the rest
of the ScrollableArea machinery can run as usual but the offset will be clamped
to your override.
> > > 
> > > For page scale, ScrollAndScaleOverride can set the userAgentContraint in
FrameHost's PageScaleConstraintsSet (for e.g. see
WebViewImpl::setIgnoreViewportTagScaleLimits).
> > > 
> > > The advantage of doing it this way is that we're reusing most of the
existing machinery in Blink most of the new complexity is isolated in the
ScrollAndScaleOverride where it's cohesive and easy to document in detail. WDYT?
> > 
> > I like the idea and will work out a prototype. It looks like this should
also solve the user-resize issue as the RootFrameViewport uses the max/mins
returned by FrameView/VisualViewport :) Thanks!
> 
> Just uploaded a second patch with the new logic using min/max scroll positions
and PageScaleConstraints. Sadly, this doesn't quite work yet.
> 
> I'm running into issues when the visual viewport is smaller than the frame.
Fixing the visual viewport scroll position doesn't seem to work for user scrolls
in this case, and it also scrolls "out of" a fixed frame scroll position. It
seems, the positions are capped correctly in Blink
(VisualViewport/FrameView.scrollPosition() report capped positions), but scrolls
over the synthetic limits are somehow still applied in the compositor (using
--disable-threaded-scrolling).
> 
> Setting the scale seems to work, but it has some weird side effects on scroll
bar positions: If I zoom the visual viewport (same size as frame), the scroll
bars then appear in the middle of the viewport, scaled the opposite way. I will
investigate this further tomorrow, and if I get stuck will share some
screenshots :)

After another day of trouble shooting, here my findings.

As reported before, fixing the scroll position of frame and visual viewport
works, but only within Blink. This works well for programmatic scrolls - and
also for user scrolls as long as frame and viewport have the same sizes. If the
visual viewport has a different size (overridden in RenderWidget by the
ScreenMetricsEmulator), user scroll events seem to be handled by cc::Viewport
instead of Blink's RootFrameViewport. (Not exactly sure why, I suspect this may
have to do with keyboard overlays and similar?) Thus, the min/max overrides of
the Blink-side aren't applied. David, do you know if there's a way around this?
Do we need to enforce the overrides in cc as well? One alternative I can see is
to re-apply Blink's capped scroll positions when the FrameView/VisualViewport
are notified of new positions by the compositor - at the moment, these
notifications are dropped because the capped scroll position seemingly doesn't
change (from Blink's point-of-view). Is there maybe a different way of syncing
the min/max scroll positions used by cc::Viewport with Blink's?

Regarding setting the scale via PageScaleConstraints, it seems that constraining
the minimum page scale factor means that the frame size changes as well. That's
probably why scrollbars appear in weird places (and, which I now discovered,
layouting also happens using the wrong frame size), screenshot here:
https://screenshot.googleplex.com/DUeq8cnkeqp.png . I think one of the culprits
is this one:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/WebViewImp...

Maybe PageScaleConstraints aren't what we're after afterall?

bokan

> After another day of trouble shooting, here my findings. > > As reported before, ...

4 years, 5 months ago (2016-06-28 18:41:40 UTC) #13

> After another day of trouble shooting, here my findings.
> 
> As reported before, fixing the scroll position of frame and visual viewport
> works, but only within Blink. This works well for programmatic scrolls - and
> also for user scrolls as long as frame and viewport have the same sizes. If
the
> visual viewport has a different size (overridden in RenderWidget by the
> ScreenMetricsEmulator), user scroll events seem to be handled by cc::Viewport
> instead of Blink's RootFrameViewport.

This is surprising and I'd like to find out why. Are you running with threaded
scrolling disabled? When --disable-threaded-scrolling is set, all the input
events should go to Blink rather than CC so cc::Viewport shouldn't ever get a
crack. I think you forgot to git add ScrollAndScaleEmulator so I can't verify,
but it's possible that the way you're resizing everything, the scroll on
cc::Viewport might be a clamp on the compositor side. Can you actually smoothly
scroll the viewport on the compositor with user input or does it just "jump"?

> (Not exactly sure why, I suspect this may
> have to do with keyboard overlays and similar?) Thus, the min/max overrides of
> the Blink-side aren't applied. David, do you know if there's a way around
this?
> Do we need to enforce the overrides in cc as well? One alternative I can see
is
> to re-apply Blink's capped scroll positions when the FrameView/VisualViewport
> are notified of new positions by the compositor - at the moment, these
> notifications are dropped because the capped scroll position seemingly doesn't
> change (from Blink's point-of-view). Is there maybe a different way of syncing
> the min/max scroll positions used by cc::Viewport with Blink's?

Re-clamping on compositor commits is a possibility, though I'd like to better
understand what's going on before we paper over it. Conceptually, if we set up
everything in Blink correctly, the compositor shouldn't be modifying anything.

> Regarding setting the scale via PageScaleConstraints, it seems that
constraining
> the minimum page scale factor means that the frame size changes as well.
That's
> probably why scrollbars appear in weird places (and, which I now discovered,
> layouting also happens using the wrong frame size), screenshot here:
> https://screenshot.googleplex.com/DUeq8cnkeqp.png . I think one of the
culprits
> is this one:
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/WebViewImp...
>
> Maybe PageScaleConstraints aren't what we're after after all?

Yes, sorry, I forgot about that, changing the minimum page scale resizes the
layout
viewport. Though, scrollbars should be fixed to the visual viewport so not sure
how to explain the screen shot. The layout size should also be unaffected since
that's set by pageScaleConstraints.layoutSize which is sized based on the
viewport
meta tag. You could add an override in WebViewImpl::mainFrameSize to prevent
resizing the layout viewport.

I think we can make this work but the question is how exactly this should all
behave. For e.g., where do you expect scrollbars to show up (if at all)? How
about
position: fixed elements? Or is this all configurable? We make a number of
assumptions in both Blink and CC about how the viewport sizes relate to each
other
and we may need to change those depending on your requirements. I can help you
debug some of this - just let me know how to run it and what the expectations
are.
Feel free to schedule a VC with me this week too.

Eric Seckler

On 2016/06/28 at 18:41:40, bokan wrote: > This is surprising and I'd like to find ...

4 years, 5 months ago (2016-06-28 19:58:38 UTC) #14

On 2016/06/28 at 18:41:40, bokan wrote:
> This is surprising and I'd like to find out why. Are you running with threaded
> scrolling disabled? When --disable-threaded-scrolling is set, all the input
> events should go to Blink rather than CC so cc::Viewport shouldn't ever get a
> crack. I think you forgot to git add ScrollAndScaleEmulator so I can't verify,
> but it's possible that the way you're resizing everything, the scroll on
> cc::Viewport might be a clamp on the compositor side. Can you actually
smoothly
> scroll the viewport on the compositor with user input or does it just "jump"?

Ups, sorry (bad local git-ignore to blame). Missing files should now be in the
patch.

Yes, I run with --disable-threaded-scrolling, and I do see those
"LayerImpl::TryScroll: Failed ShouldScrollOnMainThread" events in the tracer
consistently. It think it's smooth scrolling (there are animators involved in
the compositor), but given the debug build, it's a bit jumpy in the UI anyway.

The RFV is not involved at all in this case (where visual viewport size != frame
size), instead something in cc (somewhere below cc::Viewport) seems to create
the animations. The Blink-side FrameView/VisualViewport are "later notified"
through RenderWidget/WebViewImpl::ApplyViewportDeltas(), which eventually calls
FrameView::setScrollPosition() and VisualViewport::setScaleAndLocation()
directly. There, the provided deltas are capped and then dropped (because newPos
== oldPos).

> Re-clamping on compositor commits is a possibility, though I'd like to better
> understand what's going on before we paper over it. Conceptually, if we set up
> everything in Blink correctly, the compositor shouldn't be modifying anything.

Sounds good, let's see if there's something to fix for
--disable-threaded-scrolling here then.

> Yes, sorry, I forgot about that, changing the minimum page scale resizes the
layout
> viewport. Though, scrollbars should be fixed to the visual viewport so not
sure
> how to explain the screen shot. The layout size should also be unaffected
since
> that's set by pageScaleConstraints.layoutSize which is sized based on the
viewport
> meta tag. You could add an override in WebViewImpl::mainFrameSize to prevent
> resizing the layout viewport.

I'll try adding the override there. Let's see if that is the only point where
things go wrong when setting the minimum page scale.

I guess what I saw regarding the "changed" layout size is that fixed-pos
elements came up in different places, but that can be explained by the changed
layout-viewport size.

> I think we can make this work but the question is how exactly this should all
> behave. For e.g., where do you expect scrollbars to show up (if at all)? How
about
> position: fixed elements? Or is this all configurable? We make a number of
> assumptions in both Blink and CC about how the viewport sizes relate to each
other
> and we may need to change those depending on your requirements. I can help you
> debug some of this - just let me know how to run it and what the expectations
are.

The basic idea is to use the visual viewport soley to scale/crop an area within
the layout viewport to then take a screenshot of it. Thus, our assumption here
is that the visual viewport can be have any size and position contained within
the layout viewport. Regarding scrollbars, we'll eventually also want to add a
mechanism to hide them completely for headless mode, but if they aren't hidden,
they should appear on the frame as usual and not within the visual viewport.
Does that make sense? :)

> Feel free to schedule a VC with me this week too.

Thanks, I'll put one in for tomorrow morning your time!

Eric Seckler

Another update - I started a doc and described the issues here: https://docs.google.com/document/d/1XdhP0IWEliZHFjPONMHKDbZpcLFevLDFceliExB9yKw/edit#heading=h.igv4orepnkah After experimenting ...

4 years, 5 months ago (2016-06-29 14:01:42 UTC) #15

bokan

https://codereview.chromium.org/2096633002/diff/40001/third_party/WebKit/Source/web/WebViewImpl.cpp File third_party/WebKit/Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/2096633002/diff/40001/third_party/WebKit/Source/web/WebViewImpl.cpp#newcode3236 third_party/WebKit/Source/web/WebViewImpl.cpp:3236: } My recommendation for the mainFrameSize issue would be ...

4 years, 5 months ago (2016-06-29 15:15:49 UTC) #16

Eric Seckler

On 2016/06/29 at 15:15:49, bokan wrote: > https://codereview.chromium.org/2096633002/diff/40001/third_party/WebKit/Source/web/WebViewImpl.cpp > File third_party/WebKit/Source/web/WebViewImpl.cpp (right): > > https://codereview.chromium.org/2096633002/diff/40001/third_party/WebKit/Source/web/WebViewImpl.cpp#newcode3236 ...

4 years, 5 months ago (2016-06-29 17:35:02 UTC) #17

On 2016/06/29 at 15:15:49, bokan wrote:
>
https://codereview.chromium.org/2096633002/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/web/WebViewImpl.cpp (right):
> 
>
https://codereview.chromium.org/2096633002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/web/WebViewImpl.cpp:3236: }
> My recommendation for the mainFrameSize issue would be to add a similar
pattern to what you have in the minimum/maximumScrollPosition methods to
override the mainFrameSize. In the cases where the main frame size isn't
overridden you can save the current minimumPageScaleFactor here and just return
the frame size divided by this. Alternatively, you could get what the
minimumPageScaleFactor "should" be by clearing the UA constraints, calculating
the final constraints, then reapplying the UA constraints. A bit uglier but more
robust since it'll apply any changes made by the page (should be pretty rare).
Does that sound ok?

Thanks, David, that sounds reasonable. I think I like your proposed alternative
approach more (I guess neither are very pretty :)).

On 2016/06/29 at 14:01:42, Eric Seckler wrote:
> After experimenting a little more, I think that the cc parts are involved in
handling user scrolls (I'm testing with a mouse wheel) where the mouse is
outside the (smaller) visual viewport (but still inside the frame). Not sure if
there are any other situations.

Haven't yet discovered any additional situations for non-main-thread scrolling
with --disable-threaded-scrolling, but here's more insight into wheel-scrolling
outside the visual viewport:

LayerTreeHostImpl::ScrollBegin performs a hit-test
(FindLayerThatIsHitByPoint()), which returns |nullptr| since all layers are
clipped outside the visual viewport. Then,
LayerTreeHostImpl::FindScrollLayerForDeviceViewportPoint skips traversing the
scroll tree (and applying any potential main_thread_scrolling_reasons), and
insteads directly chooses the InnerViewportScrollLayer() as scroll layer, and
presumes it can be scrolled in the compositor. There's a comment above those
lines regarding overscroll - it seems that this is the only intended case in
which scroll events could ever occur outside all clipped layers - and it seems
to explicitly prevent main thread scrolling (maybe because overscroll should be
hidden from it?). See
https://cs.chromium.org/chromium/src/cc/trees/layer_tree_host_impl.cc?l=2583 .
If this turns out to be impossible to fix, maybe we'll just have to accept it?

bokan

On 2016/06/29 17:35:02, Eric Seckler wrote: > On 2016/06/29 at 15:15:49, bokan wrote: > > ...

4 years, 5 months ago (2016-06-29 17:41:57 UTC) #18

On 2016/06/29 17:35:02, Eric Seckler wrote:
> On 2016/06/29 at 15:15:49, bokan wrote:
> >
>
https://codereview.chromium.org/2096633002/diff/40001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/web/WebViewImpl.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2096633002/diff/40001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/web/WebViewImpl.cpp:3236: }
> > My recommendation for the mainFrameSize issue would be to add a similar
> pattern to what you have in the minimum/maximumScrollPosition methods to
> override the mainFrameSize. In the cases where the main frame size isn't
> overridden you can save the current minimumPageScaleFactor here and just
return
> the frame size divided by this. Alternatively, you could get what the
> minimumPageScaleFactor "should" be by clearing the UA constraints, calculating
> the final constraints, then reapplying the UA constraints. A bit uglier but
more
> robust since it'll apply any changes made by the page (should be pretty rare).
> Does that sound ok?
> 
> Thanks, David, that sounds reasonable. I think I like your proposed
alternative
> approach more (I guess neither are very pretty :)).
> 
> On 2016/06/29 at 14:01:42, Eric Seckler wrote:
> > After experimenting a little more, I think that the cc parts are involved in
> handling user scrolls (I'm testing with a mouse wheel) where the mouse is
> outside the (smaller) visual viewport (but still inside the frame). Not sure
if
> there are any other situations.
> 
> Haven't yet discovered any additional situations for non-main-thread scrolling
> with --disable-threaded-scrolling, but here's more insight into
wheel-scrolling
> outside the visual viewport:
> 
> LayerTreeHostImpl::ScrollBegin performs a hit-test
> (FindLayerThatIsHitByPoint()), which returns |nullptr| since all layers are
> clipped outside the visual viewport. Then,
> LayerTreeHostImpl::FindScrollLayerForDeviceViewportPoint skips traversing the
> scroll tree (and applying any potential main_thread_scrolling_reasons), and
> insteads directly chooses the InnerViewportScrollLayer() as scroll layer, and
> presumes it can be scrolled in the compositor. There's a comment above those
> lines regarding overscroll - it seems that this is the only intended case in
> which scroll events could ever occur outside all clipped layers - and it seems
> to explicitly prevent main thread scrolling (maybe because overscroll should
be
> hidden from it?). See
> https://cs.chromium.org/chromium/src/cc/trees/layer_tree_host_impl.cc?l=2583 .
> If this turns out to be impossible to fix, maybe we'll just have to accept it?

Ah, yes, I now I remember that. That code predates overscrolling support on the
main thread so routing those events to the main thread today should be ok. You'd
just have to figure out the proper way to return ScrollOnMainThread from that
situation; perhaps checking if one/both viewport layers have a main thread
scrolling reason?

Eric Seckler

On 2016/06/29 at 17:41:57, bokan wrote: > On 2016/06/29 17:35:02, Eric Seckler wrote: > > ...

4 years, 5 months ago (2016-06-29 19:37:17 UTC) #19

On 2016/06/29 at 17:41:57, bokan wrote:
> On 2016/06/29 17:35:02, Eric Seckler wrote:
> > On 2016/06/29 at 15:15:49, bokan wrote:
> > >
> >
https://codereview.chromium.org/2096633002/diff/40001/third_party/WebKit/Sour...
> > > File third_party/WebKit/Source/web/WebViewImpl.cpp (right):
> > > 
> > >
> >
https://codereview.chromium.org/2096633002/diff/40001/third_party/WebKit/Sour...
> > > third_party/WebKit/Source/web/WebViewImpl.cpp:3236: }
> > > My recommendation for the mainFrameSize issue would be to add a similar
> > pattern to what you have in the minimum/maximumScrollPosition methods to
> > override the mainFrameSize. In the cases where the main frame size isn't
> > overridden you can save the current minimumPageScaleFactor here and just
return
> > the frame size divided by this. Alternatively, you could get what the
> > minimumPageScaleFactor "should" be by clearing the UA constraints,
calculating
> > the final constraints, then reapplying the UA constraints. A bit uglier but
more
> > robust since it'll apply any changes made by the page (should be pretty
rare).
> > Does that sound ok?
> > 
> > Thanks, David, that sounds reasonable. I think I like your proposed
alternative
> > approach more (I guess neither are very pretty :)).
> > 
> > On 2016/06/29 at 14:01:42, Eric Seckler wrote:
> > > After experimenting a little more, I think that the cc parts are involved
in
> > handling user scrolls (I'm testing with a mouse wheel) where the mouse is
> > outside the (smaller) visual viewport (but still inside the frame). Not sure
if
> > there are any other situations.
> > 
> > Haven't yet discovered any additional situations for non-main-thread
scrolling
> > with --disable-threaded-scrolling, but here's more insight into
wheel-scrolling
> > outside the visual viewport:
> > 
> > LayerTreeHostImpl::ScrollBegin performs a hit-test
> > (FindLayerThatIsHitByPoint()), which returns |nullptr| since all layers are
> > clipped outside the visual viewport. Then,
> > LayerTreeHostImpl::FindScrollLayerForDeviceViewportPoint skips traversing
the
> > scroll tree (and applying any potential main_thread_scrolling_reasons), and
> > insteads directly chooses the InnerViewportScrollLayer() as scroll layer,
and
> > presumes it can be scrolled in the compositor. There's a comment above those
> > lines regarding overscroll - it seems that this is the only intended case in
> > which scroll events could ever occur outside all clipped layers - and it
seems
> > to explicitly prevent main thread scrolling (maybe because overscroll should
be
> > hidden from it?). See
> > https://cs.chromium.org/chromium/src/cc/trees/layer_tree_host_impl.cc?l=2583
.
> > If this turns out to be impossible to fix, maybe we'll just have to accept
it?
> 
> Ah, yes, I now I remember that. That code predates overscrolling support on
the main thread so routing those events to the main thread today should be ok.
You'd just have to figure out the proper way to return ScrollOnMainThread from
that situation; perhaps checking if one/both viewport layers have a main thread
scrolling reason?

That sounds good. I think the easiest way then might be to simply move the
fallback up before the tree traversal. Will look into it tomorrow :)

Eric Seckler

Another day, another problem ;) It's about the scroll position override for the visual viewport ...

4 years, 5 months ago (2016-06-30 14:06:02 UTC) #20

Eric Seckler

On 2016/06/30 14:06:02, Eric Seckler wrote: > Another day, another problem ;) It's about the ...

4 years, 5 months ago (2016-06-30 14:53:43 UTC) #21

bokan

On 2016/06/30 14:53:43, Eric Seckler wrote: > On 2016/06/30 14:06:02, Eric Seckler wrote: > > ...

4 years, 5 months ago (2016-06-30 16:48:13 UTC) #22

Eric Seckler

On 2016/06/30 16:48:13, bokan wrote: > > > Can we get around this somehow, or ...

4 years, 5 months ago (2016-06-30 17:27:13 UTC) #23

On 2016/06/30 16:48:13, bokan wrote:
> > > Can we get around this somehow, or should we revert back to overriding the
> > > scroll position directly in VisualViewport::setScaleAndLocation(), without
> > using
> > > the min/max constraints?
> 
> Right, that's to support RTL scrolling somehow. VisualViewport's scroll
doesn't
> change for RTL so I can believe there's some missing machinery there. In any
> case, that line looks wrong, it seems like it should be adding
> ScrollableArea::scrollOrigin rather than subtracting minimumScrollPosition.
Try
> changing to that and see if it helps.
Ah, thanks. Changing it to scrollOrigin helps resolve my issue, but one of the
RTL tests fails:

[ RUN      ] ScrollingCoordinatorTest.rtlIframe
../../third_party/WebKit/Source/web/tests/ScrollingCoordinatorTest.cpp:557:
Failure
Value of: webScrollLayer->scrollPositionDouble().x
  Actual: -958
Expected: expectedScrollPosition
Which is: 958
[  FAILED  ] ScrollingCoordinatorTest.rtlIframe (97 ms)

I think this may be a bug in the test though. The data page (rtl-iframe.html)
used still renders correctly in the browser: 
https://screenshot.googleplex.com/1gfVeRdFK7d.png

> Anyway, I don't want to hold you back, if it turns out we just keep hitting
bugs
> then feel free to go back to the original approach.
I think we're close now, let's try to stick with this one for a little longer :)

> > Thinking about this, another issue with using min/max constraints for our
> > overrides (as in the latest patch) is that we can't easily clamp our
overrides
> > to the "real" min/max scroll positions. At least for scroll positions, this
> > would make sense, I think. WDYT?
> 
> That should be easy to do in maximum/minimumScrollPosition by clamping the
> returned override value to the previously calculated min/max (what you pass in
> to the override method).
The one thing I'm concerned about with doing this is that I can only clamp the
overridden maxPos to the real maxPos, not to the real minPos (and vice-versa for
the overridden minPos). Thus, I can run into a case like e.g. min = 200, max =
400, overridden = 100. Then, overriddenMin = 200, overriddenMax = 100.

A solution would be to add private methods for the "real" min/max and use those
to clamp the overridden ones. Is that a reasonable approach, or am I missing
something obvious? :)

bokan

On 2016/06/30 17:27:13, Eric Seckler wrote: > On 2016/06/30 16:48:13, bokan wrote: > > > ...

4 years, 5 months ago (2016-06-30 19:06:40 UTC) #24

On 2016/06/30 17:27:13, Eric Seckler wrote:
> On 2016/06/30 16:48:13, bokan wrote:
> > > > Can we get around this somehow, or should we revert back to overriding
the
> > > > scroll position directly in VisualViewport::setScaleAndLocation(),
without
> > > using
> > > > the min/max constraints?
> > 
> > Right, that's to support RTL scrolling somehow. VisualViewport's scroll
> doesn't
> > change for RTL so I can believe there's some missing machinery there. In any
> > case, that line looks wrong, it seems like it should be adding
> > ScrollableArea::scrollOrigin rather than subtracting minimumScrollPosition.
> Try
> > changing to that and see if it helps.
> Ah, thanks. Changing it to scrollOrigin helps resolve my issue, but one of the
> RTL tests fails:
> 
> [ RUN      ] ScrollingCoordinatorTest.rtlIframe
> ../../third_party/WebKit/Source/web/tests/ScrollingCoordinatorTest.cpp:557:
> Failure
> Value of: webScrollLayer->scrollPositionDouble().x
>   Actual: -958
> Expected: expectedScrollPosition
> Which is: 958
> [  FAILED  ] ScrollingCoordinatorTest.rtlIframe (97 ms)
> 
> I think this may be a bug in the test though. The data page (rtl-iframe.html)
> used still renders correctly in the browser: 
> https://screenshot.googleplex.com/1gfVeRdFK7d.png

Methinks you're subtracting the scroll origin? You have to add it and the test
passes for me.

> 
> > Anyway, I don't want to hold you back, if it turns out we just keep hitting
> bugs
> > then feel free to go back to the original approach.
> I think we're close now, let's try to stick with this one for a little longer
:)
> 
> > > Thinking about this, another issue with using min/max constraints for our
> > > overrides (as in the latest patch) is that we can't easily clamp our
> overrides
> > > to the "real" min/max scroll positions. At least for scroll positions,
this
> > > would make sense, I think. WDYT?
> > 
> > That should be easy to do in maximum/minimumScrollPosition by clamping the
> > returned override value to the previously calculated min/max (what you pass
in
> > to the override method).
> The one thing I'm concerned about with doing this is that I can only clamp the
> overridden maxPos to the real maxPos, not to the real minPos (and vice-versa
for
> the overridden minPos). Thus, I can run into a case like e.g. min = 200, max =
> 400, overridden = 100. Then, overriddenMin = 200, overriddenMax = 100.
> 
> A solution would be to add private methods for the "real" min/max and use
those
> to clamp the overridden ones. Is that a reasonable approach, or am I missing
> something obvious? :)

Yah, you're right. A private "physicalMax/Min" sounds fine to me.

Eric Seckler

Description was changed from ========== Adds scroll position/scale + visual viewport size overrides to DevTools ...

4 years, 5 months ago (2016-06-30 19:32:31 UTC) #25

Eric Seckler

On 2016/06/30 19:06:40, bokan wrote: > Methinks you're subtracting the scroll origin? You have to ...

4 years, 5 months ago (2016-06-30 20:01:28 UTC) #27

bokan

<cc'ing vollick@ for compositing question inline> Thanks for bearing with me. Overall approach is looking ...

4 years, 5 months ago (2016-07-01 21:05:13 UTC) #28

<cc'ing vollick@ for compositing question inline>

Thanks for bearing with me. Overall approach is looking good to me. Some
suggested moves below. In particular, WebViewImpl is kind of a dumping ground so
I'd prefer if we move as much of the functionality into the
ScrollAndScaleEmulator as we can.

Also, FYI (I only learned this recently myself), if you want to include related
patches you can designate them as an upstream branch:

git branch --set-upstream-to scrollingCoordinatorChange

And when you `git cl upload` reitvald will use scrollingCoordinatorChange as the
base, rather than including it in the diff.

https://codereview.chromium.org/2096633002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2096633002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:3185: if
(m_scrollAndScaleEmulator) {
Nit: no braces

https://codereview.chromium.org/2096633002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:3186: return
m_scrollAndScaleEmulator->applyFramePositionOverride(maximum, minimum, maximum);
I would remove the min and max parameters from applyFramePositionOverride and
clamp it to the physical limits after the override call.

https://codereview.chromium.org/2096633002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/ScrollAndScaleEmulator.cpp (right):

https://codereview.chromium.org/2096633002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/ScrollAndScaleEmulator.cpp:56: changed |=
true;
These can all just be `changed = true`

https://codereview.chromium.org/2096633002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/ScrollAndScaleEmulator.cpp:91:
m_threadedScrollingEnabledByDefault =
(page->settings().threadedScrollingEnabled());
nit: no need for wrapping parenthesis here

https://codereview.chromium.org/2096633002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/ScrollAndScaleEmulator.cpp:95:
page->scrollingCoordinator()->notifyGeometryChanged();
I suspect you'll also need to call setNeedsCompositingUpdate on the root element
but I'm not very familiar here. We should loop in someone who knows. +vollick@

https://codereview.chromium.org/2096633002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/VisualViewport.cpp (right):

https://codereview.chromium.org/2096633002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/VisualViewport.cpp:229:
setScaleAndLocation(scale(), location());
We should call setScaleAndLocation to clamp the scroll position from the caller
(inside ScrollAndScaleEmulator). Also, visual viewport has a clampToBoundaries
method you could use.

https://codereview.chromium.org/2096633002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/2096633002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebViewImpl.cpp:3236:
page()->frameHost().setUserAgentPageScaleConstraints(m_scrollAndScaleEmulator->pageScaleConstraints());
Give the ScrollAndScaleEmulator a ref to the frameHost and move all this code
(and the disableThreadedScrolling) and it's dual in the "clear" method inside
it.

https://codereview.chromium.org/2096633002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebViewImpl.cpp:3253:
page()->frameHost().setUserAgentPageScaleConstraints(PageScaleConstraints());
This will clear existing UA constraints. You should save the current UA
constraints when creating the override and restore to those instead (like you do
with the threaded scrolling flag).

https://codereview.chromium.org/2096633002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebViewImpl.cpp:3257: // ineffective, though.
I think you it doesn't work in the setter because it'll then skip
refreshPageScaleFactorAfterLayout in WebViewImpl::layoutUpdated.

I'm not sure why you'd need computeFinalConstraints here though...do you mean
that without it pinching in and out doesn't change from the overridden scale?
could you dig into it a little? setUserAgentPageScaleConstraints should
setNeedsLayout which should then eventually reach WebViewImpl::layoutUpdated
with dirty constraints. resetScrollAndScaleState below should also set the scale
(without respecting limits) to 1 as well as call setNeedsReset(true) which means
that layoutUpdated will reset to the minimum page scale. Not sure where this is
going wrong or why you'd need computeFinalConstraints here.

https://codereview.chromium.org/2096633002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebViewImpl.cpp:3405:
pageScaleConstraintsSet().setUserAgentConstraints(PageScaleConstraints());
I'd add a method to the scrollAndScaleEmulator that calculate the frame size to
use that if we have an Emulator. You can then move all this in there. Also, it's
probably cleaner to just copy the PageScaleConstraintsSet, remove the UA
constraint, and calculate the minimum from that rather than operating on the
live constraints set.

Eric Seckler

Thanks again, David! https://codereview.chromium.org/2096633002/diff/120001/third_party/WebKit/Source/core/frame/FrameView.cpp File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2096633002/diff/120001/third_party/WebKit/Source/core/frame/FrameView.cpp#newcode3185 third_party/WebKit/Source/core/frame/FrameView.cpp:3185: if (m_scrollAndScaleEmulator) { On 2016/07/01 21:05:12, ...

4 years, 5 months ago (2016-07-04 14:33:08 UTC) #29

Thanks again, David!

https://codereview.chromium.org/2096633002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2096633002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:3185: if
(m_scrollAndScaleEmulator) {
On 2016/07/01 21:05:12, bokan wrote:
> Nit: no braces

Done.

https://codereview.chromium.org/2096633002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:3186: return
m_scrollAndScaleEmulator->applyFramePositionOverride(maximum, minimum, maximum);
On 2016/07/01 21:05:12, bokan wrote:
> I would remove the min and max parameters from applyFramePositionOverride and
> clamp it to the physical limits after the override call.

Done.

https://codereview.chromium.org/2096633002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/ScrollAndScaleEmulator.cpp (right):

https://codereview.chromium.org/2096633002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/ScrollAndScaleEmulator.cpp:56: changed |=
true;
On 2016/07/01 21:05:12, bokan wrote:
> These can all just be `changed = true`

Done.

https://codereview.chromium.org/2096633002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/ScrollAndScaleEmulator.cpp:91:
m_threadedScrollingEnabledByDefault =
(page->settings().threadedScrollingEnabled());
On 2016/07/01 21:05:12, bokan wrote:
> nit: no need for wrapping parenthesis here

Done.

https://codereview.chromium.org/2096633002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/ScrollAndScaleEmulator.cpp:95:
page->scrollingCoordinator()->notifyGeometryChanged();
On 2016/07/01 21:05:12, bokan wrote:
> I suspect you'll also need to call setNeedsCompositingUpdate on the root
element
> but I'm not very familiar here. We should loop in someone who knows. +vollick@

I think you're right. We need to make sure that
ScrollingCoordinator::updateAfterCompositingChangeIfNeeded() is called
eventually. I've added a
setNeedsCompositingUpdate(CompositingUpdateAfterGeometryChange) call to the
layoutView's compositor. vollick@, please let me know if this is the right way
to enforce this :)

https://codereview.chromium.org/2096633002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/VisualViewport.cpp (right):

https://codereview.chromium.org/2096633002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/VisualViewport.cpp:229:
setScaleAndLocation(scale(), location());
On 2016/07/01 21:05:12, bokan wrote:
> We should call setScaleAndLocation to clamp the scroll position from the
caller
> (inside ScrollAndScaleEmulator). Also, visual viewport has a clampToBoundaries
> method you could use.

Done, using clampToBoundaries() now. (This boils down to the same
setScaleAndLocation() invocation.)

https://codereview.chromium.org/2096633002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/2096633002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebViewImpl.cpp:3236:
page()->frameHost().setUserAgentPageScaleConstraints(m_scrollAndScaleEmulator->pageScaleConstraints());
On 2016/07/01 21:05:12, bokan wrote:
> Give the ScrollAndScaleEmulator a ref to the frameHost and move all this code
> (and the disableThreadedScrolling) and it's dual in the "clear" method inside
> it.

Done. At the moment, I don't keep a reference to the page as member in the
emulator, but pass it in only for the methods as needed. (Primarily because I am
not sure about its lifetime.) Does that make sense, or can I safely store it? If
so, which of Blink's reference types would be the most sensible one? :)

https://codereview.chromium.org/2096633002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebViewImpl.cpp:3253:
page()->frameHost().setUserAgentPageScaleConstraints(PageScaleConstraints());
On 2016/07/01 21:05:12, bokan wrote:
> This will clear existing UA constraints. You should save the current UA
> constraints when creating the override and restore to those instead (like you
do
> with the threaded scrolling flag).

Done.

https://codereview.chromium.org/2096633002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebViewImpl.cpp:3257: // ineffective, though.
On 2016/07/01 21:05:13, bokan wrote:
> I think you it doesn't work in the setter because it'll then skip
> refreshPageScaleFactorAfterLayout in WebViewImpl::layoutUpdated.
> 
> I'm not sure why you'd need computeFinalConstraints here though...do you mean
> that without it pinching in and out doesn't change from the overridden scale?
> could you dig into it a little? setUserAgentPageScaleConstraints should
> setNeedsLayout which should then eventually reach WebViewImpl::layoutUpdated
> with dirty constraints. resetScrollAndScaleState below should also set the
scale
> (without respecting limits) to 1 as well as call setNeedsReset(true) which
means
> that layoutUpdated will reset to the minimum page scale. Not sure where this
is
> going wrong or why you'd need computeFinalConstraints here.

What I'm seeing is that the scale factor is not reset to 1 when the overrides
are cleared (without the call to computeFinalConstraints()). Instead, it stays
at the overridden value, but can be changed by e.g. pinch-zooming.

I now also know where this is coming from.
DevToolsEmulator::disableDeviceEmulation() is called in the process of handling
the "clearDeviceMetricsOverride" devtools command. There,
WebViewImpl::setPageScaleFactor() is called, which clamps to the old constraints
if computeFinalConstraints() is not called first.

I'm now fixing this by setting the page scale on the visual viewport directly
(circumventing clamping) from DevToolsEmulator. It also seems that the
DevToolsEmulator only modifies page scales when mobile emulation is active.
Thus, I also moved the call to setPageScaleFactor() from
disableDeviceEmulation() to disableMobileEmulation() and updated its test. Let
me know if this is reasonable :)

https://codereview.chromium.org/2096633002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebViewImpl.cpp:3405:
pageScaleConstraintsSet().setUserAgentConstraints(PageScaleConstraints());
On 2016/07/01 21:05:13, bokan wrote:
> I'd add a method to the scrollAndScaleEmulator that calculate the frame size
to
> use that if we have an Emulator. You can then move all this in there. Also,
it's
> probably cleaner to just copy the PageScaleConstraintsSet, remove the UA
> constraint, and calculate the minimum from that rather than operating on the
> live constraints set.

Done.

Ian Vollick

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

4 years, 5 months ago (2016-07-04 14:48:12 UTC) #30

Ian Vollick

https://codereview.chromium.org/2096633002/diff/120001/third_party/WebKit/Source/core/frame/ScrollAndScaleEmulator.cpp File third_party/WebKit/Source/core/frame/ScrollAndScaleEmulator.cpp (right): https://codereview.chromium.org/2096633002/diff/120001/third_party/WebKit/Source/core/frame/ScrollAndScaleEmulator.cpp#newcode95 third_party/WebKit/Source/core/frame/ScrollAndScaleEmulator.cpp:95: page->scrollingCoordinator()->notifyGeometryChanged(); On 2016/07/04 at 14:33:08, Eric Seckler wrote: > ...

4 years, 5 months ago (2016-07-04 14:48:13 UTC) #31

Eric Seckler

https://codereview.chromium.org/2096633002/diff/120001/third_party/WebKit/Source/core/frame/ScrollAndScaleEmulator.cpp File third_party/WebKit/Source/core/frame/ScrollAndScaleEmulator.cpp (right): https://codereview.chromium.org/2096633002/diff/120001/third_party/WebKit/Source/core/frame/ScrollAndScaleEmulator.cpp#newcode95 third_party/WebKit/Source/core/frame/ScrollAndScaleEmulator.cpp:95: page->scrollingCoordinator()->notifyGeometryChanged(); On 2016/07/04 14:48:13, vollick wrote: > On 2016/07/04 ...

4 years, 5 months ago (2016-07-04 15:06:12 UTC) #32

bokan

https://codereview.chromium.org/2096633002/diff/120001/third_party/WebKit/Source/web/WebViewImpl.cpp File third_party/WebKit/Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/2096633002/diff/120001/third_party/WebKit/Source/web/WebViewImpl.cpp#newcode3236 third_party/WebKit/Source/web/WebViewImpl.cpp:3236: page()->frameHost().setUserAgentPageScaleConstraints(m_scrollAndScaleEmulator->pageScaleConstraints()); On 2016/07/04 14:33:08, Eric Seckler wrote: > On ...

4 years, 5 months ago (2016-07-04 22:44:09 UTC) #33

https://codereview.chromium.org/2096633002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/2096633002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebViewImpl.cpp:3236:
page()->frameHost().setUserAgentPageScaleConstraints(m_scrollAndScaleEmulator->pageScaleConstraints());
On 2016/07/04 14:33:08, Eric Seckler wrote:
> On 2016/07/01 21:05:12, bokan wrote:
> > Give the ScrollAndScaleEmulator a ref to the frameHost and move all this
code
> > (and the disableThreadedScrolling) and it's dual in the "clear" method
inside
> > it.
> 
> Done. At the moment, I don't keep a reference to the page as member in the
> emulator, but pass it in only for the methods as needed. (Primarily because I
am
> not sure about its lifetime.) Does that make sense, or can I safely store it?
If
> so, which of Blink's reference types would be the most sensible one? :)

I would actually move the ScrollAndScaleEmulator object to be owned by
FrameHost. FrameHost is basically the same as Page but was split off for OOPIF
refactorings (Page will eventually disappear from Blink) and you can get a
pointer to Page from FrameHost and vice-versa. That way you don't have to worry
about lifetime issues and I think it makes more sense conceptually.

You can then store a back-pointer to FrameHost (as a Member<>) without worrying
about it being GC'd from under you.

https://codereview.chromium.org/2096633002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebViewImpl.cpp:3257: // ineffective, though.
On 2016/07/04 14:33:08, Eric Seckler wrote:
> On 2016/07/01 21:05:13, bokan wrote:
> > I think you it doesn't work in the setter because it'll then skip
> > refreshPageScaleFactorAfterLayout in WebViewImpl::layoutUpdated.
> > 
> > I'm not sure why you'd need computeFinalConstraints here though...do you
mean
> > that without it pinching in and out doesn't change from the overridden
scale?
> > could you dig into it a little? setUserAgentPageScaleConstraints should
> > setNeedsLayout which should then eventually reach WebViewImpl::layoutUpdated
> > with dirty constraints. resetScrollAndScaleState below should also set the
> scale
> > (without respecting limits) to 1 as well as call setNeedsReset(true) which
> means
> > that layoutUpdated will reset to the minimum page scale. Not sure where this
> is
> > going wrong or why you'd need computeFinalConstraints here.
> 
> What I'm seeing is that the scale factor is not reset to 1 when the overrides
> are cleared (without the call to computeFinalConstraints()). Instead, it stays
> at the overridden value, but can be changed by e.g. pinch-zooming.
> 
> I now also know where this is coming from.
> DevToolsEmulator::disableDeviceEmulation() is called in the process of
handling
> the "clearDeviceMetricsOverride" devtools command. There,
> WebViewImpl::setPageScaleFactor() is called, which clamps to the old
constraints
> if computeFinalConstraints() is not called first.
> 
> I'm now fixing this by setting the page scale on the visual viewport directly
> (circumventing clamping) from DevToolsEmulator. It also seems that the
> DevToolsEmulator only modifies page scales when mobile emulation is active.
> Thus, I also moved the call to setPageScaleFactor() from
> disableDeviceEmulation() to disableMobileEmulation() and updated its test. Let
> me know if this is reasonable :)

Thanks for tracking it down. See my reply in DevToolsEmulator.cpp.

https://codereview.chromium.org/2096633002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2096633002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:3320: IntPoint maximum =
calculateMaximumScrollPosition().expandedTo(minimum);
Move this outside this if and remove braces (make it look just like
maximumScrollPosition()).

https://codereview.chromium.org/2096633002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/2096633002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.h:878:
RefPtr<ScrollAndScaleEmulator> m_scrollAndScaleEmulator;
This should be Member<ScrollAndScaleEmulator> (see comment in
ScrollAndScaleEmulator.h)

https://codereview.chromium.org/2096633002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/ScrollAndScaleEmulator.cpp (right):

https://codereview.chromium.org/2096633002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/ScrollAndScaleEmulator.cpp:50: if
(!page->mainFrame() || !page->mainFrame()->isLocalFrame())
It might be more clear to make sure that we never create a
ScrollAndScaleEmulator from a remote iframe and simply ASSERT that the main
frame is local in the create method.

https://codereview.chromium.org/2096633002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/ScrollAndScaleEmulator.cpp:64:
emulator->disableThreadedScrolling(page);
Similarly to the UA constraints, store m_originalThreadedScrollingEnabled from
this method as well. As it is, calling disableThreadedScrolling twice would
overwrite that.

https://codereview.chromium.org/2096633002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/ScrollAndScaleEmulator.cpp:126:
constraints.setUserAgentConstraints(m_originalPageScaleConstraints);
Add a short comment as to why this is necessary.

https://codereview.chromium.org/2096633002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/ScrollAndScaleEmulator.cpp:148:
page->scrollingCoordinator()->notifyGeometryChanged();
Factor the notifyGeometryChanged and needsCompositingUpdate calls into a helper.

https://codereview.chromium.org/2096633002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/ScrollAndScaleEmulator.h (right):

https://codereview.chromium.org/2096633002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/ScrollAndScaleEmulator.h:2: * Copyright (C)
2016 Google Inc. All rights reserved.
Use shortened license block both here and in the cpp:
https://www.chromium.org/blink/coding-style#TOC-License

https://codereview.chromium.org/2096633002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/ScrollAndScaleEmulator.h:56: class
CORE_EXPORT ScrollAndScaleEmulator final : public
RefCounted<ScrollAndScaleEmulator> {
RefCounted is deprecated. Instead, inherit from GarbageCollected and store in a
Member<T>. See RootScrollerController for an example and
https://www.chromium.org/blink/blink-gc for the nitty gritty details.

https://codereview.chromium.org/2096633002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/inspector/browser_protocol.json (right):

https://codereview.chromium.org/2096633002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/browser_protocol.json:693: { "name":
"scrollPositionX", "type": "integer", "optional": true, "description": "Fix
horizontal scroll position of outer viewport (frame) to an override value
(device-independent pixel)." },
The `outer`/`inner` terminology is specific to the compositor. Outside it we
just use `layout` and `visual`.

https://codereview.chromium.org/2096633002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/DevToolsEmulator.cpp (right):

https://codereview.chromium.org/2096633002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/DevToolsEmulator.cpp:316:
m_webViewImpl->page()->frameHost().visualViewport().setScale(1.f);
I think this does actually belong in disableDeviceEmulation. We need this on
desktop emulation since it can include a touch screen which means the user can
zoom in themselves. We still want to reset the page scale in that case.

You may want to try calling setNeedsReset(true) instead and moving the layout
call below into disableDeviceEmulation as well. That way we'll reset the page
scale to the minimum after the layout.

https://codereview.chromium.org/2096633002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebViewImpl.h (right):

https://codereview.chromium.org/2096633002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebViewImpl.h:685: RefPtr<ScrollAndScaleEmulator>
m_scrollAndScaleEmulator;
I think this should move to FrameHost (see reply to comment on earlier
patchset).

Eric Seckler

Thanks once again! Will add some tests next. https://codereview.chromium.org/2096633002/diff/160001/third_party/WebKit/Source/core/frame/FrameView.cpp File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2096633002/diff/160001/third_party/WebKit/Source/core/frame/FrameView.cpp#newcode3320 third_party/WebKit/Source/core/frame/FrameView.cpp:3320: IntPoint ...

4 years, 5 months ago (2016-07-05 16:46:51 UTC) #34

bokan

Looks good, I'm happy with this patch. Just awaiting tests. Thanks! https://codereview.chromium.org/2096633002/diff/180001/third_party/WebKit/Source/core/frame/FrameHost.cpp File third_party/WebKit/Source/core/frame/FrameHost.cpp (right): ...

4 years, 5 months ago (2016-07-06 14:26:31 UTC) #35

Eric Seckler

Now with C++ tests for the override functionality in Blink. Will add layout tests for ...

4 years, 5 months ago (2016-07-11 10:38:44 UTC) #36

Eric Seckler

Description was changed from ========== Adds scroll position/scale + visual viewport size overrides to DevTools ...

4 years, 5 months ago (2016-07-11 10:58:35 UTC) #37

Eric Seckler

+cc pfeldman@, dgozman@ for devtools protocol changes. +cc auto-cc Adds scroll position/scale + visual viewport ...

4 years, 5 months ago (2016-07-11 11:57:07 UTC) #38

bokan

Thanks, tests look great! Blink-side changes LGTM https://codereview.chromium.org/2096633002/diff/200001/third_party/WebKit/Source/web/tests/WebViewTest.cpp File third_party/WebKit/Source/web/tests/WebViewTest.cpp (right): https://codereview.chromium.org/2096633002/diff/200001/third_party/WebKit/Source/web/tests/WebViewTest.cpp#newcode1146 third_party/WebKit/Source/web/tests/WebViewTest.cpp:1146: frameView->layout(); I ...

4 years, 5 months ago (2016-07-11 13:52:10 UTC) #39

Eric Seckler

Forked the VisualViewport::setScrollPosition() change off into https://codereview.chromium.org/2138823002/ . https://codereview.chromium.org/2096633002/diff/200001/third_party/WebKit/Source/web/tests/WebViewTest.cpp File third_party/WebKit/Source/web/tests/WebViewTest.cpp (right): https://codereview.chromium.org/2096633002/diff/200001/third_party/WebKit/Source/web/tests/WebViewTest.cpp#newcode1146 third_party/WebKit/Source/web/tests/WebViewTest.cpp:1146: frameView->layout(); ...

4 years, 5 months ago (2016-07-11 16:37:13 UTC) #40

Eric Seckler

eseckler@chromium.org changed reviewers: + dgozman@chromium.org

4 years, 5 months ago (2016-07-21 17:00:04 UTC) #41

Eric Seckler

PTAL. +r: dgozman@ As discussed with dgozman, I split the ScrollAndScale overrides off into a ...

4 years, 5 months ago (2016-07-21 17:00:05 UTC) #42

dgozman

Should we split this up into viewport override vs scroll/scale override? They are semantically different, ...

4 years, 5 months ago (2016-07-21 20:54:09 UTC) #43

Eric Seckler

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

4 years, 5 months ago (2016-07-22 12:46:24 UTC) #44

commit-bot: I haz the power

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

4 years, 5 months ago (2016-07-22 12:46:39 UTC) #45

commit-bot: I haz the power

Your CL relies on deprecated CQ feature(s): * Specifying master names in CQ_INCLUDE_TRYBOTS part of ...

4 years, 5 months ago (2016-07-22 12:46:41 UTC) #46

Eric Seckler

Description was changed from ========== Adds scroll position/scale + visual viewport size overrides to DevTools ...

4 years, 5 months ago (2016-07-22 12:54:18 UTC) #47

Eric Seckler

Thanks, Dmitry. I split off the size override into crrev.com/2173783002. This patch still depends on ...

4 years, 5 months ago (2016-07-22 14:44:48 UTC) #48

Thanks, Dmitry. I split off the size override into crrev.com/2173783002. This
patch still depends on it, though, since it's useful to test the scroll/scale
overrides in the layout tests.

https://codereview.chromium.org/2096633002/diff/300001/content/browser/devtoo...
File content/browser/devtools/protocol/emulation_handler.cc (right):

https://codereview.chromium.org/2096633002/diff/300001/content/browser/devtoo...
content/browser/devtools/protocol/emulation_handler.cc:217: "Visible_width and
visible_height values must be positive, not greater "
On 2016/07/21 20:54:08, dgozman wrote:
> Visual viewport dimensions must be positive, not greater...

Done.

https://codereview.chromium.org/2096633002/diff/300001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/inspector-protocol/emulation/device-emulation-visual-viewport-size.html
(right):

https://codereview.chromium.org/2096633002/diff/300001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector-protocol/emulation/device-emulation-visual-viewport-size.html:27:
layoutWidth: document.getElementById("layoutViewportDiv").clientWidth,
On 2016/07/21 20:54:08, dgozman wrote:
> I don't like that override we use is observable by the page. This defeats the
> purpose: let's say we focus on the specific region to take a screenshot of it,
> and then position:fixed element covers half of it because we made it
observable
> to the web.
> Since the whole purpose of the visible viewport override is performance, can
we
> do something else (like disabling compositing entirely)?

I noted some of this in the design doc. I think we actually want layout viewport
overrides (both size and scroll) to cause a reflow/layout of the page, so they
should always be observed by the page. I.e., the layout overrides should resize
the page, that's what we intend to use them for :)

For visual viewport overrides, we want to support both (i) page-observable
changes (e.g. to take a screenshot of "what does my page look like on a mobile
browser that zoomes in here"), but also (ii) the option to hide the visual
viewport overrides from the page (e.g. to take a screenshot of an ad - here we
don't want visual-viewport-fixed elements to pop up). 

I think it would be fine if we initially only support (i) and add (ii) later,
particularly since pos-fixed elements are layout-viewport relative and most of
the visual viewport isn't yet exposed to pages (VisualViewport API is still
experimental). We can later support (ii) by adding an option to hide the
overrides of the visual viewport from the page in some way.

Regarding disabling compositing, we're looking into suspending certain parts of
the pipeline (e.g. raster/paint) for headless at the moment. In that process, we
should also look into how often/when we want compositing to occur as well. For
us, it may actually rather be "compose only after overrides are set", or
"compose when we explicitly tell you to". I think it makes sense to look into
this separately, too.

https://codereview.chromium.org/2096633002/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2096633002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:3222: return
m_scrollAndScaleEmulator->applyFramePositionOverride(maximum).shrunkTo(maximum).expandedTo(minimum);
On 2016/07/21 20:54:08, dgozman wrote:
> Is this web-observable? E.g. what this snippet will produce when override is
on?
> 
> body.scrollTop = 100;
> console.log(body.scrollTop);
> 
> If it's observable, we break the web contract. I'm not sure that's good.

For the layout viewport (frame), the change of scroll position through the
override is observable by the page (again, we want that). The page won't be able
to change the position, though. So in your example, assuming overriden scroll
position is (0, 0):

body.scrollTop = 100; // Will be clamped to 0, so no change in position.
console.log(body.scrollTop); // Will log 0.

I'm not sure if this really breaks contracts, since setting body.scrollTop
outside min/max generally also clamps. We're just enforcing a different min/max,
which I'd argue is in the user-agent's right.

https://codereview.chromium.org/2096633002/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/DevToolsEmulator.cpp (left):

https://codereview.chromium.org/2096633002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/DevToolsEmulator.cpp:319:
m_webViewImpl->mainFrameImpl()->frameView()->layout();
On 2016/07/21 20:54:08, dgozman wrote:
> This is still needed when we toggle between mobile/desktop emulation (called
> from enableDeviceEmulation).

Right, thanks. I think it still makes sense to reset the scale through
resetScaleStateImmediately() in disableDeviceMetrics, which needs a layout. I
think a reasonable solution is to remove the layout() from
enableMobileEmulation, too, and add it to enableDeviceEmulation instead. Done.

https://codereview.chromium.org/2096633002/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/InspectorEmulationAgent.cpp (right):

https://codereview.chromium.org/2096633002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/InspectorEmulationAgent.cpp:58: if
(m_state->booleanProperty(EmulationAgentState::scrollAndScaleOverrideEnabled,
false)) {
On 2016/07/21 20:54:09, dgozman wrote:
> if (!m_state....)

Whoops, thanks for spotting :)

https://codereview.chromium.org/2096633002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/InspectorEmulationAgent.cpp:89: *errorString =
"Scroll position coordinates must be non-negative or -1 (disabled)";
On 2016/07/21 20:54:09, dgozman wrote:
> Let's not treat -1 as disabled. We have "optional" just for that.

Done. I was conflicted about this, too, since allowing -1 makes wrapper
functions for the command (e.g. in tests) easier. But it's cleaner without.

commit-bot: I haz the power

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

4 years, 5 months ago (2016-07-22 15:39:25 UTC) #49

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

4 years, 5 months ago (2016-07-22 15:39:29 UTC) #50

dgozman

4 years, 5 months ago (2016-07-25 22:26:17 UTC) #51

Sorry for delay, I was thinking and discussing this a bit.

>
https://codereview.chromium.org/2096633002/diff/300001/third_party/WebKit/Lay...
>
third_party/WebKit/LayoutTests/inspector-protocol/emulation/device-emulation-visual-viewport-size.html:27:
> layoutWidth: document.getElementById("layoutViewportDiv").clientWidth,
> On 2016/07/21 20:54:08, dgozman wrote:
> > I don't like that override we use is observable by the page. This defeats
the
> > purpose: let's say we focus on the specific region to take a screenshot of
it,
> > and then position:fixed element covers half of it because we made it
> observable
> > to the web.
> > Since the whole purpose of the visible viewport override is performance, can
> we
> > do something else (like disabling compositing entirely)?
> 
> I noted some of this in the design doc. I think we actually want layout
viewport
> overrides (both size and scroll) to cause a reflow/layout of the page, so they
> should always be observed by the page. I.e., the layout overrides should
resize
> the page, that's what we intend to use them for :)
> 
> For visual viewport overrides, we want to support both (i) page-observable
> changes (e.g. to take a screenshot of "what does my page look like on a mobile
> browser that zoomes in here"), but also (ii) the option to hide the visual
> viewport overrides from the page (e.g. to take a screenshot of an ad - here we
> don't want visual-viewport-fixed elements to pop up). 
> 
> I think it would be fine if we initially only support (i) and add (ii) later,
> particularly since pos-fixed elements are layout-viewport relative and most of
> the visual viewport isn't yet exposed to pages (VisualViewport API is still
> experimental). We can later support (ii) by adding an option to hide the
> overrides of the visual viewport from the page in some way.

For (i) we can just scroll/zoom as the user does it. For example, we can replace
Emulation.setPageScaleFactor with setVisualViewport(scale, scrollX, scrollY).
Won't that be good enough? Since it's going to trigger layout and be observable,
it might as well be regular interaction. WDYT?
And we can figure out (ii) later.

> For the layout viewport (frame), the change of scroll position through the
> override is observable by the page (again, we want that). The page won't be
able
> to change the position, though. So in your example, assuming overriden scroll
> position is (0, 0):
> 
> body.scrollTop = 100; // Will be clamped to 0, so no change in position.
> console.log(body.scrollTop); // Will log 0.
> 
> I'm not sure if this really breaks contracts, since setting body.scrollTop
> outside min/max generally also clamps. We're just enforcing a different
min/max,
> which I'd argue is in the user-agent's right.

I'm not so sure here. This introduces behavior which is "override mode only". We
try to avoid that as much as possible.
If the problem is to prevent page from doing any modifications, we can
separately achieve that by freezing it as we do for debugger pause. This code is
well-tested and will be reused, so we won't run into special cases, as we have
to with these overrides.

Eric Seckler

4 years, 3 months ago (2016-09-23 09:57:12 UTC) #52

Description was changed from

==========
Adds scroll position and scale overrides to DevTools Emulation
protocol.

This enables resizing and positioning of frame and viewport for
(headless) screenshots. For more information, see
crbug.com/625577 and the design doc: bit.ly/viewport-screenshots.

Depends on crrev.com/2173783002, which adds visual viewport
size overrides to DevTools.

BUG=625577
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel
==========

to

==========
(Obsolete.)

Adds scroll position and scale overrides to DevTools Emulation
protocol.

This enables resizing and positioning of frame and viewport for
(headless) screenshots. For more information, see
crbug.com/625577 and the design doc: bit.ly/viewport-screenshots.

Depends on crrev.com/2173783002, which adds visual viewport
size overrides to DevTools.

BUG=625577
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel
==========

Issue 2096633002: Adds scroll position/scale emulation to DevTools. (Closed)

Description

Patch Set 1 #

Patch Set 2 : Separate Emulator class, use min/max positions and PageScaleConstraints. #

Patch Set 3 : Add missing file, fix forbidden include. #

Patch Set 4 : Fixes issues with visual viewport scroll override, mainFrameSize on scale override, clamps position… #

Patch Set 5 : Fix style and comments. #

Patch Set 6 : Sync changes from patches 2111203004 + 2118773002. #

Patch Set 7 : Address reviewer comments, fix computeFinalConstraints() issue. #

Patch Set 8 : setNeedsCompositingUpdate now using InputChange. #

Patch Set 9 : Emulator using GC, moved to FrameHost, addressed other comments. #

Patch Set 10 : Add C++ tests, clamp visual viewport position. #

Patch Set 11 : Address reviewer comments. #

Patch Set 12 : add comment. #

Patch Set 13 : Re-sync. #

Patch Set 14 : Move scroll/scale overrides to separate DevTools command. #

Patch Set 15 : Sync, patch in 2169483002 (+ regression test), add DevTools tests. #

Patch Set 16 : Split off size override, address reviewer comments. #

Messages