Adds DevTools commands for composited area override. (issue 2237433004 by eseckler@chromium.org)

[esec...@chromium.org]

Reviewers: Sami
CL: https://codereview.chromium.org/2237433004/

Message:
Hey Sami,

This is missing tests at the moment, but I thought you might want to have a
first look anyway :)

Eric

Description:
Adds DevTools commands for composited area override to the Emulation domain.
They are handled by the renderer's DevToolsEmulator.

This is a second step towards more flexible screenshots
(bit.ly/sized-screenshots): With this override, we can position a specific
(scaled) area of the page into the top-left corner of the frame. We can then
take a screenshot after resizing the frame (RWHV) in the browser to the area
size. We are adding DevTools commands for this in a separate patch:
https://codereview.chromium.org/2226323002/

We enforce the override through the root layer transform of WebViewImpl.
As the area coordinates are given relative to the page origin and a page
scale of 1.0, we take the current scroll offset and page scale into account
and update the transform when they change.

To ensure that all content in the area is recorded and shown, we override
the recording area used in CompositedLayerMapping and disable clipping to
the visual viewport while the override is active.


BUG=625577

Base URL: https://chromium.googlesource.com/chromium/src.git@master

Affected files (+236, -24 lines):
M third_party/WebKit/Source/core/frame/FrameView.h
M third_party/WebKit/Source/core/frame/FrameView.cpp
M third_party/WebKit/Source/core/frame/VisualViewport.cpp
M third_party/WebKit/Source/core/inspector/browser_protocol.json
M third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
M third_party/WebKit/Source/core/page/ChromeClient.h
M third_party/WebKit/Source/platform/graphics/GraphicsLayer.h
M third_party/WebKit/Source/platform/graphics/GraphicsLayer.cpp
M third_party/WebKit/Source/web/ChromeClientImpl.h
M third_party/WebKit/Source/web/ChromeClientImpl.cpp
M third_party/WebKit/Source/web/DevToolsEmulator.h
M third_party/WebKit/Source/web/DevToolsEmulator.cpp
M third_party/WebKit/Source/web/InspectorEmulationAgent.h
M third_party/WebKit/Source/web/InspectorEmulationAgent.cpp
M third_party/WebKit/Source/web/WebLocalFrameImpl.cpp
M third_party/WebKit/Source/web/WebViewImpl.h
M third_party/WebKit/Source/web/WebViewImpl.cpp
M third_party/WebKit/Source/web/tests/WebInputEventConversionTest.cpp

[skyostil@chromium.org via codereview.chromium.org]

I think this looks good. The compositor folks may have better ideas on how to
override the painted area, but this basically works too.


https://codereview.chromium.org/2237433004/diff/20001/third_party/WebKit/Source/core/frame/FrameView.cpp
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2237433004/diff/20001/third_party/WebKit/Source/core/frame/FrameView.cpp#newcode3412
third_party/WebKit/Source/core/frame/FrameView.cpp:3412:
frame().host()->chromeClient().mainFrameScrollOffsetChanged();
May need to guard this with m_frame->isMainFrame(). (I also wonder if we
could reuse the didChangeScrollOffset callback below?)

https://codereview.chromium.org/2237433004/diff/20001/third_party/WebKit/Source/core/frame/FrameView.h
File third_party/WebKit/Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/2237433004/diff/20001/third_party/WebKit/Source/core/frame/FrameView.h#newcode451
third_party/WebKit/Source/core/frame/FrameView.h:451: // Set a custom
recording area. Used for composited area overrides to ensure
I still think we should call this painting instead of recording, because
cc's recording step basically just means telling Blink to paint things.

https://codereview.chromium.org/2237433004/diff/20001/third_party/WebKit/Source/core/frame/FrameView.h#newcode457
third_party/WebKit/Source/core/frame/FrameView.h:457: IntSize
visibleContentSizeForRecording() const;
Unused?

https://codereview.chromium.org/2237433004/diff/20001/third_party/WebKit/Source/core/inspector/browser_protocol.json
File third_party/WebKit/Source/core/inspector/browser_protocol.json
(right):

https://codereview.chromium.org/2237433004/diff/20001/third_party/WebKit/Source/core/inspector/browser_protocol.json#newcode750
third_party/WebKit/Source/core/inspector/browser_protocol.json:750:
"name": "setCompositedAreaOverride",
Alternative name idea: setVisibleAreaOverride

Not sure which I like better so we could keep this one :)

https://codereview.chromium.org/2237433004/

[esec...@chromium.org]

Thanks!

https://codereview.chromium.org/2237433004/diff/20001/third_party/WebKit/Source/core/frame/FrameView.cpp
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2237433004/diff/20001/third_party/WebKit/Source/core/frame/FrameView.cpp#newcode3412
third_party/WebKit/Source/core/frame/FrameView.cpp:3412:
frame().host()->chromeClient().mainFrameScrollOffsetChanged();

On 2016/08/11 10:09:07, Sami wrote:
> May need to guard this with m_frame->isMainFrame(). (I also wonder if
we could
> reuse the didChangeScrollOffset callback below?)

You're right, guarding it now. I looked into the didChangeScrollOffset
callback, but that seems to be used to notify the embedder. I'm using
the ChromeClient path, because that's already used for similar things,
e.g. pageScaleFactorChanged() and layoutChanged().

https://codereview.chromium.org/2237433004/diff/20001/third_party/WebKit/Source/core/frame/FrameView.h
File third_party/WebKit/Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/2237433004/diff/20001/third_party/WebKit/Source/core/frame/FrameView.h#newcode451
third_party/WebKit/Source/core/frame/FrameView.h:451: // Set a custom
recording area. Used for composited area overrides to ensure

On 2016/08/11 10:09:07, Sami wrote:
> I still think we should call this painting instead of recording,
because cc's
> recording step basically just means telling Blink to paint things.

Alright :)

https://codereview.chromium.org/2237433004/diff/20001/third_party/WebKit/Source/core/frame/FrameView.h#newcode457
third_party/WebKit/Source/core/frame/FrameView.h:457: IntSize
visibleContentSizeForRecording() const;

On 2016/08/11 10:09:07, Sami wrote:
> Unused?

True, don't need it ATM.

https://codereview.chromium.org/2237433004/diff/20001/third_party/WebKit/Source/core/inspector/browser_protocol.json
File third_party/WebKit/Source/core/inspector/browser_protocol.json
(right):

https://codereview.chromium.org/2237433004/diff/20001/third_party/WebKit/Source/core/inspector/browser_protocol.json#newcode750
third_party/WebKit/Source/core/inspector/browser_protocol.json:750:
"name": "setCompositedAreaOverride",

On 2016/08/11 10:09:07, Sami wrote:
> Alternative name idea: setVisibleAreaOverride
>
> Not sure which I like better so we could keep this one :)

Renamed to VisualTransformOverride, Thanks!

https://codereview.chromium.org/2237433004/

[skyostil@chromium.org via codereview.chromium.org]

Looks great! Let's get some owner opinions on this before adding tests.


https://codereview.chromium.org/2237433004/diff/60001/third_party/WebKit/Source/core/frame/FrameView.h
File third_party/WebKit/Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/2237433004/diff/60001/third_party/WebKit/Source/core/frame/FrameView.h#newcode885
third_party/WebKit/Source/core/frame/FrameView.h:885:
std::unique_ptr<IntRect> m_visibleContentRectForPainting;
Could this be a WTF::Optional?

https://codereview.chromium.org/2237433004/

[esec...@chromium.org]

+chrishtr/danakj for feedback regarding the compositing parts: We're wondering
if changing the visibleContentRect used in CompositedLayerMapping + disabling
viewport clipping is a reasonable way to ensure that the whole area is
recorded/painted. Is it sufficient / is there a cleverer way? :)

+pfeldman/dgozman for the DevTools commands + integration with DevToolsEmulator.

Thank you for your help!

==========
Adds DevTools commands for visual transform override to the Emulation domain.

They are handled by the renderer's DevToolsEmulator.

This is a second step towards more flexible screenshots
(bit.ly/sized-screenshots): With this override, we can position a specific
(scaled) area of the page into the top-left corner of the frame. We can then

take a screenshot after resizing the frame (RWHV) in the browser to the scaled
area size. We are adding DevTools commands for the latter in a separate patch:

https://codereview.chromium.org/2226323002/

We enforce the override through the root layer transform of WebViewImpl.

As the given area coordinates are relative to the page origin and a page

scale of 1.0, we take the current scroll offset and page scale into account

and update the root transform when they change.

To ensure that all content in the area is recorded and shown, we override

the visibleContentRect used in CompositedLayerMapping and disable clipping

to the visual viewport while the override is active.

BUG=625577

==========

https://codereview.chromium.org/2237433004/diff/60001/third_party/WebKit/Source/core/frame/FrameView.h
File third_party/WebKit/Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/2237433004/diff/60001/third_party/WebKit/Source/core/frame/FrameView.h#newcode885
third_party/WebKit/Source/core/frame/FrameView.h:885:
std::unique_ptr<IntRect> m_visibleContentRectForPainting;

On 2016/08/11 13:24:24, Sami wrote:
> Could this be a WTF::Optional?

Indeed :)

https://codereview.chromium.org/2237433004/

[chri...@chromium.org]

https://codereview.chromium.org/2237433004/diff/80001/third_party/WebKit/Source/web/DevToolsEmulator.cpp
File third_party/WebKit/Source/web/DevToolsEmulator.cpp (right):

https://codereview.chromium.org/2237433004/diff/80001/third_party/WebKit/Source/web/DevToolsEmulator.cpp#newcode333
third_party/WebKit/Source/web/DevToolsEmulator.cpp:333:
m_webViewImpl->mainFrameImpl()->frameView()->setVisibleContentRectForPainting(enclosingIntRect(area));
Why can't this be accomplished by changing the width and height of the
frame?

https://codereview.chromium.org/2237433004/

[esec...@chromium.org]

https://codereview.chromium.org/2237433004/diff/80001/third_party/WebKit/Source/web/DevToolsEmulator.cpp
File third_party/WebKit/Source/web/DevToolsEmulator.cpp (right):

https://codereview.chromium.org/2237433004/diff/80001/third_party/WebKit/Source/web/DevToolsEmulator.cpp#newcode333
third_party/WebKit/Source/web/DevToolsEmulator.cpp:333:
m_webViewImpl->mainFrameImpl()->frameView()->setVisibleContentRectForPainting(enclosingIntRect(area));

On 2016/08/12 20:56:22, chrishtr wrote:
> Why can't this be accomplished by changing the width and height of the
frame?

I don't think width&height would suffice, since we may be translating to
a different area of the page than the current scroll offset. Anyway, the
reason we discarded frame (or viewport) repositioning/resizing is that
we don't want the page to be able to observe or interfere with our
screenshotting process. Instead, we'd like to be able to set frame size
and position independently of the screenshot area, so that we can
control e.g. layout size and where fixed-pos elements turn up. AFAICT,
there's no way of changing frame size and/or scroll offset without it
affecting the page?

(We actually looked into realizing the screenshots through changing
frame+VisualViewport size/position and page scale first, and discarded
the idea because we'd have needed web-contract-breaking overrides for
these - plus the changes in size and scroll position would have been
visible to the page.)

https://codereview.chromium.org/2237433004/

[chri...@chromium.org]

https://codereview.chromium.org/2237433004/diff/80001/third_party/WebKit/Source/web/DevToolsEmulator.cpp
File third_party/WebKit/Source/web/DevToolsEmulator.cpp (right):

https://codereview.chromium.org/2237433004/diff/80001/third_party/WebKit/Source/web/DevToolsEmulator.cpp#newcode333
third_party/WebKit/Source/web/DevToolsEmulator.cpp:333:
m_webViewImpl->mainFrameImpl()->frameView()->setVisibleContentRectForPainting(enclosingIntRect(area));

On 2016/08/12 at 21:37:53, Eric Seckler wrote:
> On 2016/08/12 20:56:22, chrishtr wrote:
> > Why can't this be accomplished by changing the width and height of
the frame?
>
> I don't think width&height would suffice, since we may be translating
to a different area of the page than the current scroll offset. Anyway,
the reason we discarded frame (or viewport) repositioning/resizing is
that we don't want the page to be able to observe or interfere with our
screenshotting process. Instead, we'd like to be able to set frame size
and position independently of the screenshot area, so that we can
control e.g. layout size and where fixed-pos elements turn up. AFAICT,
there's no way of changing frame size and/or scroll offset without it
affecting the page?
>
> (We actually looked into realizing the screenshots through changing
frame+VisualViewport size/position and page scale first, and discarded
the idea because we'd have needed web-contract-breaking overrides for
these - plus the changes in size and scroll position would have been
visible to the page.)

Why would you want it to be different than the current scroll offset?
Sounds like you want to record somewhere in the document,
and render the entire document. Frame size and scroll offset are just
getting in your way?

Also, did you consider using a viewport size that is the entire
document? It's maybe not crazy to do that for just the display list
output of paint.

https://codereview.chromium.org/2237433004/

[esec...@chromium.org]

https://codereview.chromium.org/2237433004/diff/80001/third_party/WebKit/Source/web/DevToolsEmulator.cpp
File third_party/WebKit/Source/web/DevToolsEmulator.cpp (right):

https://codereview.chromium.org/2237433004/diff/80001/third_party/WebKit/Source/web/DevToolsEmulator.cpp#newcode333
third_party/WebKit/Source/web/DevToolsEmulator.cpp:333:
m_webViewImpl->mainFrameImpl()->frameView()->setVisibleContentRectForPainting(enclosingIntRect(area));

We want to record+render only the screenshot area, which could be
smaller than
the frame (e.g. a single advert) or larger (e.g. the whole page). We'd
like to avoid
having to paint the whole document to screenshot a small area though.

For more background on our use case, see bit.ly/sized-screenshots.
boliu@ also
mentioned that Android-WebView's record_whole_document can be rather
slow, so
we'd like to avoid something similar, see discussion on
https://groups.google.com/a/chromium.org/forum/#!search/Screenshots$20for$20Headless$20Chrome/headless-dev/cOLRWKTW1YA/lAxbwBpCBAAJ
.

https://codereview.chromium.org/2237433004/

[chri...@chromium.org]

https://codereview.chromium.org/2237433004/diff/80001/third_party/WebKit/Source/web/DevToolsEmulator.cpp
File third_party/WebKit/Source/web/DevToolsEmulator.cpp (right):

https://codereview.chromium.org/2237433004/diff/80001/third_party/WebKit/Source/web/DevToolsEmulator.cpp#newcode333
third_party/WebKit/Source/web/DevToolsEmulator.cpp:333:
m_webViewImpl->mainFrameImpl()->frameView()->setVisibleContentRectForPainting(enclosingIntRect(area));

Is the objection to scrolling to the desired area that the site could do
something adversarial when noticing we did that?

https://codereview.chromium.org/2237433004/

[esec...@chromium.org]

https://codereview.chromium.org/2237433004/diff/80001/third_party/WebKit/Source/web/DevToolsEmulator.cpp
File third_party/WebKit/Source/web/DevToolsEmulator.cpp (right):

https://codereview.chromium.org/2237433004/diff/80001/third_party/WebKit/Source/web/DevToolsEmulator.cpp#newcode333
third_party/WebKit/Source/web/DevToolsEmulator.cpp:333:
m_webViewImpl->mainFrameImpl()->frameView()->setVisibleContentRectForPainting(enclosingIntRect(area));

Correct. Javascript can listen for scroll events and/or issue
window.scrollTo(), which is why we'd have to actually override things,
rather than set once. This affects both visual viewport & frame scroll
positions. If changing the frame size, fixed-pos elements will be placed
elsewhere (i.e. could obscure the screenshot area). The page can also
see changes in frame or visual viewport size/scale through
window.innerWidth/Height and react to them.

That's why we pivoted towards a solution that separates screenshot
translation/scale/size from frame/viewport state. We don't actually want
to change how/where elements in the frame get painted, just which ones,
so that they a) show up in the screenshot and b) we don't paint more
than necessary :)

https://codereview.chromium.org/2237433004/

[chri...@chromium.org]

lgtm




https://codereview.chromium.org/2237433004/diff/80001/third_party/WebKit/Source/core/frame/FrameView.h
File third_party/WebKit/Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/2237433004/diff/80001/third_party/WebKit/Source/core/frame/FrameView.h#newcode454
third_party/WebKit/Source/core/frame/FrameView.h:454: // set, the
painting area defaults to visibleContentRect().
Add a note that this should only be used by devtools overrides.

https://codereview.chromium.org/2237433004/

[esec...@chromium.org]

Thanks Chris.

Pavel / Dmitry, I added tests, PTAL :)

https://codereview.chromium.org/2237433004/diff/80001/third_party/WebKit/Source/core/frame/FrameView.h
File third_party/WebKit/Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/2237433004/diff/80001/third_party/WebKit/Source/core/frame/FrameView.h#newcode454
third_party/WebKit/Source/core/frame/FrameView.h:454: // set, the
painting area defaults to visibleContentRect().

On 2016/08/12 22:24:24, chrishtr - OOO wrote:
> Add a note that this should only be used by devtools overrides.

Done.

https://codereview.chromium.org/2237433004/

[dgo...@chromium.org]

https://codereview.chromium.org/2237433004/diff/120001/third_party/WebKit/Source/core/inspector/browser_protocol.json
File third_party/WebKit/Source/core/inspector/browser_protocol.json
(right):

https://codereview.chromium.org/2237433004/diff/120001/third_party/WebKit/Source/core/inspector/browser_protocol.json#newcode750
third_party/WebKit/Source/core/inspector/browser_protocol.json:750:
"name": "setVisualTransformOverride",
The overall approach sounds good, but I think we should use scale,
offsetX and offsetY in device metrics instead. They are meant to be used
exactly for that (e.g. centering/scaling the emulated page inside the
RWHV). If we miss something (like disabling viewport clipping), let's
modify existing handling to add that. WDYT?

https://codereview.chromium.org/2237433004/diff/120001/third_party/WebKit/Source/core/inspector/browser_protocol.json#newcode755
third_party/WebKit/Source/core/inspector/browser_protocol.json:755: {
"name": "width", "type": "number", "description": "Width of the area
(CSS pixels)." },
So, these are here only for optimization purpose? Can we use the
physical backing size in compositor instead (which you plan to set using
RWHV resize from another patch)? The less parameters we have the better.

https://codereview.chromium.org/2237433004/

[esec...@chromium.org]

Thanks, Dmitry!

https://codereview.chromium.org/2237433004/diff/120001/third_party/WebKit/Source/core/inspector/browser_protocol.json
File third_party/WebKit/Source/core/inspector/browser_protocol.json
(right):

https://codereview.chromium.org/2237433004/diff/120001/third_party/WebKit/Source/core/inspector/browser_protocol.json#newcode750
third_party/WebKit/Source/core/inspector/browser_protocol.json:750:
"name": "setVisualTransformOverride",

On 2016/08/18 20:11:10, dgozman wrote:
> The overall approach sounds good, but I think we should use scale,
offsetX and
> offsetY in device metrics instead. They are meant to be used exactly
for that
> (e.g. centering/scaling the emulated page inside the RWHV). If we miss
something
> (like disabling viewport clipping), let's modify existing handling to
add that.
> WDYT?

Hmm, the current parameters in the device metrics override are only
applied once, and they seem really more intended as an offset within the
frame at which to show the emulated page, as you've mentioned. We could
"abuse" them for our use of placing a specific area of the page at
coordinates (0,0) by supplying a negative offset. However, that doesn't
take current scroll offset / page scale into account. (For the visual
transform override, we're compensating for changes in scroll offset /
page scale to ensure we're actually showing the intended area at
intended scale at (0,0).) Plus, as you said, the other missing bits are
viewport clipping and setting the interest rect for painting/recording.
Thus, we'd need an additional boolean flag (or multiple) that, when
enabled:
- makes offsetX/Y relative to page origin, and scale relative to a 1.0
page scale.
- enables scroll/scale compensating behavior
- disables viewport clipping
- sets the recording rect according to physical backing size and
override offset/scale (see your other comment).

I was refraining from adding this to the device metrics override, since
you previously said that it's already getting rather
parameter/functionality-loaden. Plus, I felt this alternative behavior
of the offsetX/Y/scale parameters might be confusing. IMHO, having a
separate command feels a bit cleaner - e.g. the purpose & coordinate
space for the params is clearer in this case. But I can see there's some
potential overlap in parameter-reuse, at least if we slightly change
their meaning ;)

Of course, reusing the offsetX/Y/scale params from the device metrics
override then makes it impossible to use them for their original purpose
at the same time. A separate visual transform override would allow for
that. Don't think we'd need that for headless though.

https://codereview.chromium.org/2237433004/diff/120001/third_party/WebKit/Source/core/inspector/browser_protocol.json#newcode755
third_party/WebKit/Source/core/inspector/browser_protocol.json:755: {
"name": "width", "type": "number", "description": "Width of the area
(CSS pixels)." },

On 2016/08/18 20:11:10, dgozman wrote:
> So, these are here only for optimization purpose? Can we use the
physical
> backing size in compositor instead (which you plan to set using RWHV
resize from
> another patch)? The less parameters we have the better.

We only use them to specify the interest area for painting/recording,
correct. You're right, we could probably get away with using the
physical backing size (scaled by the inverse of the supplied override
scale). The physical backing size update happens asynchronously, though,
so we'd need to make sure that this doesn't bite us.

https://codereview.chromium.org/2237433004/

[dgo...@chromium.org]

As discussed offline:
- remove offsetX/Y from device metrics;
- turn this method into forceViewport(scale, x, y);
- try to unify with record_whole_document by the means of clipping by physical
backing size instead of main frame size (currently controlled by
mainFrameClipsContent setting).

https://codereview.chromium.org/2237433004/

[esec...@chromium.org]

Dmitry, Chris, PTAL. Thanks! :)

On 2016/08/19 18:28:58, dgozman wrote:
> As discussed offline:
> - remove offsetX/Y from device metrics;

I got rid of offsetX/Y from the setDeviceMetricsOverride command, but I'm
leaving them in WebDeviceEmulationParams for the time being (they are still used
by fitToView). Maybe we can think about whether this needs further refactoring
separately?

> - turn this method into forceViewport(scale, x, y);

Done.

> - try to unify with record_whole_document by the means of clipping by physical
> backing size instead of main frame size (currently controlled by
> mainFrameClipsContent setting).

Now using the backing size for clipping the painting area. Clipping is now done
via FrameView/ChromeClient, which should enable Android WebView to eventually
use this as well (e.g. clipping to the canvas size instead of
record_whole_document). The details of which are probably also best thought
about separately?

https://codereview.chromium.org/2237433004/

[chri...@chromium.org]

https://codereview.chromium.org/2237433004/diff/180001/third_party/WebKit/Source/core/frame/FrameView.h
File third_party/WebKit/Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/2237433004/diff/180001/third_party/WebKit/Source/core/frame/FrameView.h#newcode457
third_party/WebKit/Source/core/frame/FrameView.h:457: void
clipPaintRect(FloatRect*) const;
Why provide a clipPaintRect method instead of just returning the rect to
clip by?

https://codereview.chromium.org/2237433004/diff/180001/third_party/WebKit/Source/core/inspector/browser_protocol.json
File third_party/WebKit/Source/core/inspector/browser_protocol.json
(right):

https://codereview.chromium.org/2237433004/diff/180001/third_party/WebKit/Source/core/inspector/browser_protocol.json#newcode755
third_party/WebKit/Source/core/inspector/browser_protocol.json:755: {
"name": "scale", "type": "number", "optional": true, "description":
"Scale to apply to the area (relative to a page scale of 1.0). Defaults
to 1.0." }
What is the origin of this scale?

https://codereview.chromium.org/2237433004/diff/180001/third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(right):

https://codereview.chromium.org/2237433004/diff/180001/third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp#newcode2317
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:2317:
LayoutRect
graphicsLayerBoundsInScreenSpace(graphicsLayerBoundsInObjectSpace);
It's not screen space, it's the space of the root LayoutView. i.e. it
doesn't include scrolling.

https://codereview.chromium.org/2237433004/diff/180001/third_party/WebKit/Source/web/DevToolsEmulator.h
File third_party/WebKit/Source/web/DevToolsEmulator.h (right):

https://codereview.chromium.org/2237433004/diff/180001/third_party/WebKit/Source/web/DevToolsEmulator.h#newcode52
third_party/WebKit/Source/web/DevToolsEmulator.h:52: void
forceViewport(const WebFloatPoint& position, float scale);
Scale after position, or before? Units for position?

https://codereview.chromium.org/2237433004/diff/180001/third_party/WebKit/Source/web/WebViewImpl.cpp
File third_party/WebKit/Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/2237433004/diff/180001/third_party/WebKit/Source/web/WebViewImpl.cpp#newcode4095
third_party/WebKit/Source/web/WebViewImpl.cpp:4095:
m_devToolsEmulator->mainFrameScrollOrScaleChanged();
This seems new. Why was it not needed before?

https://codereview.chromium.org/2237433004/diff/180001/third_party/WebKit/public/platform/WebLayerTreeView.h
File third_party/WebKit/public/platform/WebLayerTreeView.h (right):

https://codereview.chromium.org/2237433004/diff/180001/third_party/WebKit/public/platform/WebLayerTreeView.h#newcode70
third_party/WebKit/public/platform/WebLayerTreeView.h:70: virtual
WebSize getViewportSize() const { return WebSize(); }
In what coordinate space? Not including the device scale factor?

https://codereview.chromium.org/2237433004/

[dgo...@chromium.org]

https://codereview.chromium.org/2237433004/diff/180001/third_party/WebKit/Source/core/inspector/browser_protocol.json
File third_party/WebKit/Source/core/inspector/browser_protocol.json
(right):

https://codereview.chromium.org/2237433004/diff/180001/third_party/WebKit/Source/core/inspector/browser_protocol.json#newcode733
third_party/WebKit/Source/core/inspector/browser_protocol.json:733: {
"name": "scale", "type": "number", "optional": true, "deprecated": true,
"description": "Not used." },
Please leave the scale, it's used by DevTools.

https://codereview.chromium.org/2237433004/diff/180001/third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(right):

https://codereview.chromium.org/2237433004/diff/180001/third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp#newcode2386
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:2386:
// Paint the whole layer if "mainFrameClipsContent" is false, meaning
that WebPreferences::record_whole_document is true.
I'd suggest to unify this setting with clipPaintRect being implemented
in FrameView in a separate patch before using it here. If I understand
things correctly, FrameView could clip to mainFrame when setting is
true, and to viewport size (as in this patch) when it's false. WDYT?

https://codereview.chromium.org/2237433004/diff/180001/third_party/WebKit/public/platform/WebLayerTreeView.h
File third_party/WebKit/public/platform/WebLayerTreeView.h (right):

https://codereview.chromium.org/2237433004/diff/180001/third_party/WebKit/public/platform/WebLayerTreeView.h#newcode73
third_party/WebKit/public/platform/WebLayerTreeView.h:73: virtual float
getDeviceScaleFactor() const { return 1.f; }
Having both set and get here looks strange. AFAIU, blink is supposed to
control WebLayerTreeView so it should already know the device scale
factor it set.

https://codereview.chromium.org/2237433004/

[esec...@chromium.org]

Thank you for the prompt feedback! Comments addressed :)

https://codereview.chromium.org/2237433004/diff/180001/third_party/WebKit/Source/core/frame/FrameView.h
File third_party/WebKit/Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/2237433004/diff/180001/third_party/WebKit/Source/core/frame/FrameView.h#newcode457
third_party/WebKit/Source/core/frame/FrameView.h:457: void
clipPaintRect(FloatRect*) const;

On 2016/08/22 22:00:42, chrishtr wrote:
> Why provide a clipPaintRect method instead of just returning the rect
to clip
> by?

Actually, because I was thinking we could move the
mainFrameClipsContent() check from CompositedLayerMapping there (and
simply not clip if it is false) for the time being. See also the comment
below about eventually unifying record_whole_document with clipping to
the device viewport.

On 2016/08/22 22:22:06, dgozman wrote:
> I'd suggest to unify this setting with clipPaintRect being implemented
in
> FrameView in a separate patch before using it here. If I understand
things
> correctly, FrameView could clip to mainFrame when setting is true, and
to
> viewport size (as in this patch) when it's false. WDYT?

I agree, that would be optimal - provided that we are correct in the
assumption that Android WebView sets the device viewport size to the
size of the canvas provided to WebView.onDraw(). In any case, I think
changing the behavior of record_whole_document will require some further
discussion with Android WebView folks, since it may require changes in
public WebView API (record_whole_document/enableSlowWholeDocumentDraw()
should probably be renamed / comments updated). If possible, I'd prefer
not to make this patch dependent on the outcome of that discussion, so
would suggest to tackle this as a follow-up. Does that sound reasonable?

https://codereview.chromium.org/2237433004/diff/180001/third_party/WebKit/Source/core/inspector/browser_protocol.json
File third_party/WebKit/Source/core/inspector/browser_protocol.json
(right):

https://codereview.chromium.org/2237433004/diff/180001/third_party/WebKit/Source/core/inspector/browser_protocol.json#newcode733
third_party/WebKit/Source/core/inspector/browser_protocol.json:733: {
"name": "scale", "type": "number", "optional": true, "deprecated": true,
"description": "Not used." },

On 2016/08/22 22:22:06, dgozman wrote:
> Please leave the scale, it's used by DevTools.

Ah, misunderstood you. Is it missing tests? (I didn't see anything under
inspector/ or inspector-protocol/ breaking.)

https://codereview.chromium.org/2237433004/diff/180001/third_party/WebKit/Source/core/inspector/browser_protocol.json#newcode755
third_party/WebKit/Source/core/inspector/browser_protocol.json:755: {
"name": "scale", "type": "number", "optional": true, "description":
"Scale to apply to the area (relative to a page scale of 1.0). Defaults
to 1.0." }

On 2016/08/22 22:00:42, chrishtr wrote:
> What is the origin of this scale?

It's relative to a page scale of 1.0 and effectively overrides the page
scale. We'll be using this to e.g. create thumbnail screenshots (or e.g.
"zoomed" screenshots of elements on a page). The reason we're supplying
it here rather than setting it with via setPageScaleFactor is that
changes to the page scale factor are visible to the page (e.g. through
window.innerWidth/Height), so we're instead adapting to changes in the
page scale.

https://codereview.chromium.org/2237433004/diff/180001/third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(right):

https://codereview.chromium.org/2237433004/diff/180001/third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp#newcode2317
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:2317:
LayoutRect
graphicsLayerBoundsInScreenSpace(graphicsLayerBoundsInObjectSpace);

On 2016/08/22 22:00:42, chrishtr wrote:
> It's not screen space, it's the space of the root LayoutView. i.e. it
doesn't
> include scrolling.

Makes sense, updated the comments as well.

https://codereview.chromium.org/2237433004/diff/180001/third_party/WebKit/Source/web/DevToolsEmulator.h
File third_party/WebKit/Source/web/DevToolsEmulator.h (right):

https://codereview.chromium.org/2237433004/diff/180001/third_party/WebKit/Source/web/DevToolsEmulator.h#newcode52
third_party/WebKit/Source/web/DevToolsEmulator.h:52: void
forceViewport(const WebFloatPoint& position, float scale);

On 2016/08/22 22:00:42, chrishtr wrote:
> Scale after position, or before? Units for position?

Units are documented in the corresponding protocol message, I added a
comment here as well. Since position is given in CSS pixels (rather than
DIPs), I don't think we need an additional comment that scale is applied
after positioning.

https://codereview.chromium.org/2237433004/diff/180001/third_party/WebKit/Source/web/WebViewImpl.cpp
File third_party/WebKit/Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/2237433004/diff/180001/third_party/WebKit/Source/web/WebViewImpl.cpp#newcode4095
third_party/WebKit/Source/web/WebViewImpl.cpp:4095:
m_devToolsEmulator->mainFrameScrollOrScaleChanged();

On 2016/08/22 22:00:42, chrishtr wrote:
> This seems new. Why was it not needed before?

Should have been there in the previous version of the patch you looked
at, too. It's needed to notify the emulator about a page scale change,
so that it can update the viewport transform (to ensure the |scale|
param of forceViewport is enforced relative to a page scale of 1.0).

https://codereview.chromium.org/2237433004/diff/180001/third_party/WebKit/public/platform/WebLayerTreeView.h
File third_party/WebKit/public/platform/WebLayerTreeView.h (right):

https://codereview.chromium.org/2237433004/diff/180001/third_party/WebKit/public/platform/WebLayerTreeView.h#newcode70
third_party/WebKit/public/platform/WebLayerTreeView.h:70: virtual
WebSize getViewportSize() const { return WebSize(); }

On 2016/08/22 22:00:42, chrishtr wrote:
> In what coordinate space? Not including the device scale factor?

It's in physical pixels. Added a comment.

https://codereview.chromium.org/2237433004/diff/180001/third_party/WebKit/public/platform/WebLayerTreeView.h#newcode73
third_party/WebKit/public/platform/WebLayerTreeView.h:73: virtual float
getDeviceScaleFactor() const { return 1.f; }

On 2016/08/22 22:22:06, dgozman wrote:
> Having both set and get here looks strange. AFAIU, blink is supposed
to control
> WebLayerTreeView so it should already know the device scale factor it
set.

True, except that there could be a device_scale_factor override in place
within Blink, which is why I query it from the layerTreeView/compositor.
I got rid of this and instead added a compositorDeviceScaleFactor()
accessor to WebViewImpl that returns
|m_compositorDeviceScaleFactorOverride| if an override is active. Is
that more appropriate?

https://codereview.chromium.org/2237433004/

[esec...@chromium.org]

Chris, Dmitry, PTAL. Thanks again!

https://codereview.chromium.org/2237433004/

[chri...@chromium.org]

lgtm



https://codereview.chromium.org/2237433004/

[esec...@chromium.org]

+pdr for third_party/WebKit/Source/platform/testing/
+jbauman for content/renderer/gpu/

https://codereview.chromium.org/2237433004/

[chri...@chromium.org]

FYI I am an owner of third_party/WebKit/*

https://codereview.chromium.org/2237433004/

[esec...@chromium.org]

On 2016/08/25 16:46:23, chrishtr wrote:
> FYI I am an owner of third_party/WebKit/*

Ah, my bad. I was trusting chromite butler's red squares and not thinking ;)
pdr@, no need to review.

https://codereview.chromium.org/2237433004/

[p...@chromium.org]

On 2016/08/25 at 16:53:12, eseckler wrote:
> On 2016/08/25 16:46:23, chrishtr wrote:
> > FYI I am an owner of third_party/WebKit/*
>
> Ah, my bad. I was trusting chromite butler's red squares and not thinking ;)
> pdr@, no need to review.

LGTM anyway. Take that!

https://codereview.chromium.org/2237433004/

[jba...@chromium.org]

On 2016/08/25 16:43:44, Eric Seckler wrote:
> +pdr for third_party/WebKit/Source/platform/testing/
> +jbauman for content/renderer/gpu/

content/renderer/gpu lgtm

https://codereview.chromium.org/2237433004/

[dgo...@chromium.org]

Sorry for long review, but the patch is quite large :-)


https://codereview.chromium.org/2237433004/diff/240001/third_party/WebKit/LayoutTests/inspector-protocol/emulation/device-emulation-test.js
File
third_party/WebKit/LayoutTests/inspector-protocol/emulation/device-emulation-test.js
(left):

https://codereview.chromium.org/2237433004/diff/240001/third_party/WebKit/LayoutTests/inspector-protocol/emulation/device-emulation-test.js#oldcode206
third_party/WebKit/LayoutTests/inspector-protocol/emulation/device-emulation-test.js:206:
params.scale = 1;
Please revert.

https://codereview.chromium.org/2237433004/diff/240001/third_party/WebKit/Source/core/frame/FrameView.h
File third_party/WebKit/Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/2237433004/diff/240001/third_party/WebKit/Source/core/frame/FrameView.h#newcode893
third_party/WebKit/Source/core/frame/FrameView.h:893:
WTF::Optional<IntRect> m_visibleContentRectForPainting;
Unused.

https://codereview.chromium.org/2237433004/diff/240001/third_party/WebKit/Source/core/inspector/browser_protocol.json
File third_party/WebKit/Source/core/inspector/browser_protocol.json
(right):

https://codereview.chromium.org/2237433004/diff/240001/third_party/WebKit/Source/core/inspector/browser_protocol.json#newcode751
third_party/WebKit/Source/core/inspector/browser_protocol.json:751: {

"name": "scale", "type": "number", "optional": true, "description":
"Scale to apply to the area (relative to a page scale of 1.0). Defaults
to 1.0." }

Do we need it to be optional? I think clients will always pass it.

https://codereview.chromium.org/2237433004/diff/240001/third_party/WebKit/Source/core/inspector/browser_protocol.json#newcode756
third_party/WebKit/Source/core/inspector/browser_protocol.json:756:
"description": "Resets the visible area of the page to the original
viewport, undoing any effects of the forceViewport command.",
nit: <code>forceViewport</code>

https://codereview.chromium.org/2237433004/diff/240001/third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(right):

https://codereview.chromium.org/2237433004/diff/240001/third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp#newcode2386
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:2386:
if (!needsRepaint(*graphicsLayer) && previousInterestRect ==
wholeLayerRect)
I'm curios what happens if you forceViewport to cover the whole rect
(e.g. trying to make a screenshot of everything), then resetViewport and
stumble upon this early return.

https://codereview.chromium.org/2237433004/diff/240001/third_party/WebKit/Source/web/DevToolsEmulator.cpp
File third_party/WebKit/Source/web/DevToolsEmulator.cpp (left):

https://codereview.chromium.org/2237433004/diff/240001/third_party/WebKit/Source/web/DevToolsEmulator.cpp#oldcode202
third_party/WebKit/Source/web/DevToolsEmulator.cpp:202: &&
m_emulationParams.scale == params.scale) {
Please revert this.

https://codereview.chromium.org/2237433004/diff/240001/third_party/WebKit/Source/web/DevToolsEmulator.h
File third_party/WebKit/Source/web/DevToolsEmulator.h (right):

https://codereview.chromium.org/2237433004/diff/240001/third_party/WebKit/Source/web/DevToolsEmulator.h#newcode27
third_party/WebKit/Source/web/DevToolsEmulator.h:27: class WEB_EXPORT
DevToolsEmulator final : public
GarbageCollectedFinalized<DevToolsEmulator> {
What are we exporting this for?

https://codereview.chromium.org/2237433004/diff/240001/third_party/WebKit/Source/web/InspectorEmulationAgent.cpp
File third_party/WebKit/Source/web/InspectorEmulationAgent.cpp (right):

https://codereview.chromium.org/2237433004/diff/240001/third_party/WebKit/Source/web/InspectorEmulationAgent.cpp#newcode82
third_party/WebKit/Source/web/InspectorEmulationAgent.cpp:82:
webViewImpl()->devToolsEmulator()->forceViewport(position,
appliedScale);
We should store the values in |m_state| and restore from them in
|restore()|. This ensures we preserve the values upon cross-process
navigation.

https://codereview.chromium.org/2237433004/diff/240001/third_party/WebKit/Source/web/tests/WebViewTest.cpp
File third_party/WebKit/Source/web/tests/WebViewTest.cpp (left):

https://codereview.chromium.org/2237433004/diff/240001/third_party/WebKit/Source/web/tests/WebViewTest.cpp#oldcode1081
third_party/WebKit/Source/web/tests/WebViewTest.cpp:1081: params.offset
= WebFloatPoint();
Please revert these.

https://codereview.chromium.org/2237433004/