Adds DevTools commands for forced viewport override.

Adds DevTools commands for forced viewport 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 visible content area used in CompositedLayerMapping and disable clipping
to the visual viewport while the override is active.

BUG=625577
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/23d8f263fccca2ef163a9c3f1592d3a51e3a2d50
Cr-Commit-Position: refs/heads/master@{#420314}

[Eric Seckler, 2016-08-11 09:31:48]

Patchset #1 (id:1) has been deleted

[Eric Seckler, 2016-08-11 09:44:10]

Description was changed from

==========
Adds DevTools commands for composited area override.

BUG=625577
==========

to

==========
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
==========

[Eric Seckler, 2016-08-11 09:46:48]

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

[Eric Seckler, 2016-08-11 09:46:48]

Hey Sami,

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

Eric

[Sami, 2016-08-11 10:09:07]

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/Sour...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2237433004/diff/20001/third_party/WebKit/Sour...
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/Sour...
File third_party/WebKit/Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/2237433004/diff/20001/third_party/WebKit/Sour...
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/Sour...
third_party/WebKit/Source/core/frame/FrameView.h:457: IntSize
visibleContentSizeForRecording() const;
Unused?

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

https://codereview.chromium.org/2237433004/diff/20001/third_party/WebKit/Sour...
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 :)

[Eric Seckler, 2016-08-11 11:03:41]

Description was changed from

==========
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
==========

to

==========
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 visible content area used in CompositedLayerMapping and disable clipping
to the visual viewport while the override is active.


BUG=625577
==========

[Eric Seckler, 2016-08-11 11:33:13]

Description was changed from

==========
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 visible content area used in CompositedLayerMapping and disable clipping
to the visual viewport while the override is active.


BUG=625577
==========

to

==========
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 visible content area used in CompositedLayerMapping and disable clipping
to the visual viewport while the override is active.

BUG=625577
==========

[Eric Seckler, 2016-08-11 11:34:45]

Thanks!

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

https://codereview.chromium.org/2237433004/diff/20001/third_party/WebKit/Sour...
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/Sour...
File third_party/WebKit/Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/2237433004/diff/20001/third_party/WebKit/Sour...
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/Sour...
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/Sour...
File third_party/WebKit/Source/core/inspector/browser_protocol.json (right):

https://codereview.chromium.org/2237433004/diff/20001/third_party/WebKit/Sour...
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!

[Sami, 2016-08-11 13:24:24]

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

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

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

[Eric Seckler, 2016-08-11 15:12:37]

eseckler@chromium.org changed reviewers:
+ chrishtr@chromium.org, danakj@chromium.org, dgozman@chromium.org,
pfeldman@chromium.org

[Eric Seckler, 2016-08-11 15:12:38]

+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/Sour...
File third_party/WebKit/Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/2237433004/diff/60001/third_party/WebKit/Sour...
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 :)

[chrishtr, 2016-08-12 20:56:22]

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

https://codereview.chromium.org/2237433004/diff/80001/third_party/WebKit/Sour...
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?

[Eric Seckler, 2016-08-12 21:37:53]

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

https://codereview.chromium.org/2237433004/diff/80001/third_party/WebKit/Sour...
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.)

[chrishtr, 2016-08-12 21:41:36]

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

https://codereview.chromium.org/2237433004/diff/80001/third_party/WebKit/Sour...
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.

[Eric Seckler, 2016-08-12 21:55:37]

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

https://codereview.chromium.org/2237433004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/DevToolsEmulator.cpp:333:
m_webViewImpl->mainFrameImpl()->frameView()->setVisibleContentRectForPainting(enclosingIntRect(area));
On 2016/08/12 21:41:36, chrishtr wrote:
> 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.

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$20H...
.

[chrishtr, 2016-08-12 22:03:43]

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

https://codereview.chromium.org/2237433004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/DevToolsEmulator.cpp:333:
m_webViewImpl->mainFrameImpl()->frameView()->setVisibleContentRectForPainting(enclosingIntRect(area));
On 2016/08/12 at 21:55:36, Eric Seckler wrote:
> On 2016/08/12 21:41:36, chrishtr wrote:
> > 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.
> 
> 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$20H...
.

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

[Eric Seckler, 2016-08-12 22:15:46]

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

https://codereview.chromium.org/2237433004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/DevToolsEmulator.cpp:333:
m_webViewImpl->mainFrameImpl()->frameView()->setVisibleContentRectForPainting(enclosingIntRect(area));
On 2016/08/12 22:03:43, chrishtr wrote:
> On 2016/08/12 at 21:55:36, Eric Seckler wrote:
> > On 2016/08/12 21:41:36, chrishtr wrote:
> > > 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.
> > 
> > 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$20H...
> .
> 
> Is the objection to scrolling to the desired area that the site could do
> something adversarial when noticing we did that?

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 :)

[chrishtr, 2016-08-12 22:24:24]

lgtm

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

https://codereview.chromium.org/2237433004/diff/80001/third_party/WebKit/Sour...
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.

[Eric Seckler, 2016-08-15 16:18:11]

Thanks Chris.

Pavel / Dmitry, I added tests, PTAL :)

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

https://codereview.chromium.org/2237433004/diff/80001/third_party/WebKit/Sour...
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.

[Eric Seckler, 2016-08-18 15:33:44]

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

[commit-bot: I haz the power, 2016-08-18 15:34:07]

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

[commit-bot: I haz the power, 2016-08-18 15:43:22]

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

[commit-bot: I haz the power, 2016-08-18 15:43:23]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Eric Seckler, 2016-08-18 16:03:00]

Description was changed from

==========
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 visible content area used in CompositedLayerMapping and disable clipping
to the visual viewport while the override is active.

BUG=625577
==========

to

==========
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 visible content area used in CompositedLayerMapping and disable clipping
to the visual viewport while the override is active.

BUG=625577
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[Eric Seckler, 2016-08-18 16:25:50]

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

[commit-bot: I haz the power, 2016-08-18 16:26:16]

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

[commit-bot: I haz the power, 2016-08-18 17:07:19]

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

[commit-bot: I haz the power, 2016-08-18 17:07:21]

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

[dgozman, 2016-08-18 20:11:10]

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

https://codereview.chromium.org/2237433004/diff/120001/third_party/WebKit/Sou...
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/Sou...
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.

[Eric Seckler, 2016-08-18 22:04:28]

Thanks, Dmitry!

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

https://codereview.chromium.org/2237433004/diff/120001/third_party/WebKit/Sou...
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/Sou...
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.

[dgozman, 2016-08-19 18:28:58]

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).

[Eric Seckler, 2016-08-22 18:04:13]

Patchset #7 (id:140001) has been deleted

[Eric Seckler, 2016-08-22 18:57:00]

Patchset #7 (id:160001) has been deleted

[Eric Seckler, 2016-08-22 18:58:33]

Description was changed from

==========
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 visible content area used in CompositedLayerMapping and disable clipping
to the visual viewport while the override is active.

BUG=625577
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Adds DevTools commands for forced viewport 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 visible content area used in CompositedLayerMapping and disable clipping
to the visual viewport while the override is active.

BUG=625577
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[Eric Seckler, 2016-08-22 19:04:14]

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?

[chrishtr, 2016-08-22 22:00:43]

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

https://codereview.chromium.org/2237433004/diff/180001/third_party/WebKit/Sou...
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/Sou...
File third_party/WebKit/Source/core/inspector/browser_protocol.json (right):

https://codereview.chromium.org/2237433004/diff/180001/third_party/WebKit/Sou...
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/Sou...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(right):

https://codereview.chromium.org/2237433004/diff/180001/third_party/WebKit/Sou...
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/Sou...
File third_party/WebKit/Source/web/DevToolsEmulator.h (right):

https://codereview.chromium.org/2237433004/diff/180001/third_party/WebKit/Sou...
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/Sou...
File third_party/WebKit/Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/2237433004/diff/180001/third_party/WebKit/Sou...
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/pub...
File third_party/WebKit/public/platform/WebLayerTreeView.h (right):

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

[dgozman, 2016-08-22 22:22:06]

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

https://codereview.chromium.org/2237433004/diff/180001/third_party/WebKit/Sou...
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/Sou...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(right):

https://codereview.chromium.org/2237433004/diff/180001/third_party/WebKit/Sou...
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/pub...
File third_party/WebKit/public/platform/WebLayerTreeView.h (right):

https://codereview.chromium.org/2237433004/diff/180001/third_party/WebKit/pub...
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.

[Eric Seckler, 2016-08-23 09:32:20]

Thank you for the prompt feedback! Comments addressed :)

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

https://codereview.chromium.org/2237433004/diff/180001/third_party/WebKit/Sou...
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/Sou...
File third_party/WebKit/Source/core/inspector/browser_protocol.json (right):

https://codereview.chromium.org/2237433004/diff/180001/third_party/WebKit/Sou...
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/Sou...
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/Sou...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(right):

https://codereview.chromium.org/2237433004/diff/180001/third_party/WebKit/Sou...
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/Sou...
File third_party/WebKit/Source/web/DevToolsEmulator.h (right):

https://codereview.chromium.org/2237433004/diff/180001/third_party/WebKit/Sou...
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/Sou...
File third_party/WebKit/Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/2237433004/diff/180001/third_party/WebKit/Sou...
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/pub...
File third_party/WebKit/public/platform/WebLayerTreeView.h (right):

https://codereview.chromium.org/2237433004/diff/180001/third_party/WebKit/pub...
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/pub...
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?

[Eric Seckler, 2016-08-23 09:34:26]

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

[commit-bot: I haz the power, 2016-08-23 09:35:03]

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

[commit-bot: I haz the power, 2016-08-23 10:43:30]

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

[commit-bot: I haz the power, 2016-08-23 10:43:31]

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

[Eric Seckler, 2016-08-23 13:58:05]

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

[commit-bot: I haz the power, 2016-08-23 13:58:27]

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

[commit-bot: I haz the power, 2016-08-23 15:44:29]

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

[commit-bot: I haz the power, 2016-08-23 15:44:31]

Dry run: This issue passed the CQ dry run.

[Eric Seckler, 2016-08-25 08:27:40]

eseckler@chromium.org changed reviewers:
- danakj@chromium.org, pfeldman@chromium.org

[Eric Seckler, 2016-08-25 08:27:41]

Chris, Dmitry, PTAL. Thanks again!

[chrishtr, 2016-08-25 15:16:21]

lgtm

[Eric Seckler, 2016-08-25 16:43:43]

eseckler@chromium.org changed reviewers:
+ jbauman@chromium.org, pdr@chromium.org

[Eric Seckler, 2016-08-25 16:43:44]

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

[chrishtr, 2016-08-25 16:46:23]

FYI I am an owner of third_party/WebKit/*

[Eric Seckler, 2016-08-25 16:53:11]

eseckler@chromium.org changed reviewers:
- pdr@chromium.org

[Eric Seckler, 2016-08-25 16:53:12]

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.

[pdr., 2016-08-25 17:30:41]

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!

[jbauman, 2016-08-25 20:21:01]

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

[dgozman, 2016-08-29 20:45:36]

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

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

https://codereview.chromium.org/2237433004/diff/240001/third_party/WebKit/Lay...
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/Sou...
File third_party/WebKit/Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/2237433004/diff/240001/third_party/WebKit/Sou...
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/Sou...
File third_party/WebKit/Source/core/inspector/browser_protocol.json (right):

https://codereview.chromium.org/2237433004/diff/240001/third_party/WebKit/Sou...
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/Sou...
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/Sou...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(right):

https://codereview.chromium.org/2237433004/diff/240001/third_party/WebKit/Sou...
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/Sou...
File third_party/WebKit/Source/web/DevToolsEmulator.cpp (left):

https://codereview.chromium.org/2237433004/diff/240001/third_party/WebKit/Sou...
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/Sou...
File third_party/WebKit/Source/web/DevToolsEmulator.h (right):

https://codereview.chromium.org/2237433004/diff/240001/third_party/WebKit/Sou...
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/Sou...
File third_party/WebKit/Source/web/InspectorEmulationAgent.cpp (right):

https://codereview.chromium.org/2237433004/diff/240001/third_party/WebKit/Sou...
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/Sou...
File third_party/WebKit/Source/web/tests/WebViewTest.cpp (left):

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

[Eric Seckler, 2016-09-12 13:51:54]

Patchset #11 (id:260001) has been deleted

[Eric Seckler, 2016-09-12 13:55:54]

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

[commit-bot: I haz the power, 2016-09-12 13:56:09]

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

[Eric Seckler, 2016-09-12 13:58:00]

Chris, Xianzhu, PTAL at the comment regarding CompositedLayerMapping. Thanks!

Dmitry, thanks for the thorough review :)

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

https://codereview.chromium.org/2237433004/diff/240001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector-protocol/emulation/device-emulation-test.js:206:
params.scale = 1;
On 2016/08/29 20:45:36, dgozman wrote:
> Please revert.

Done.

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

https://codereview.chromium.org/2237433004/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.h:893: WTF::Optional<IntRect>
m_visibleContentRectForPainting;
On 2016/08/29 20:45:36, dgozman wrote:
> Unused.

Done.

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

https://codereview.chromium.org/2237433004/diff/240001/third_party/WebKit/Sou...
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." }
On 2016/08/29 20:45:36, dgozman wrote:
> Do we need it to be optional? I think clients will always pass it.

Sure, we can make it required. Done.

https://codereview.chromium.org/2237433004/diff/240001/third_party/WebKit/Sou...
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.",
On 2016/08/29 20:45:36, dgozman wrote:
> nit: <code>forceViewport</code>

Done.

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

https://codereview.chromium.org/2237433004/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:2386:
if (!needsRepaint(*graphicsLayer) && previousInterestRect == wholeLayerRect)
On 2016/08/29 20:45:36, dgozman wrote:
> 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.

I think you're right, the logic for replacing old interest rects doesn't yet
account for the rect shrinking - it's assuming that it "moves" via scrolling. I
don't think it's a problem in this early return (this only seems to happen when
no repaint is needed anyway), but more so in
interestRectChangedEnoughToRepaint(), which doesn't update the rect if it
shrinks. So if we use forceViewport to set it to e.g. the whole page, it'd stick
to the full layers forever.

Two thoughts on this: First, this shouldn't break functionality, so keeping it
as-is would be fine except for potential performance implications (particularly
for headless this is unlikely to become an issue). Second, is this also an issue
for zooming out and then back into a page? It seems to me that this would also
extend the interest rect on zoom-out and later not shrink it on zoom-in.

I'd be happy to file a bug against this and postpone it until later. One
possible work-around would be to always update the interest rect if the layer
already needs repainting. That is, change the if statement below to:

if (needsRepaint(*graphicsLayer) ||
interestRectChangedEnoughToRepaint(previousInterestRect, newInterestRect,
expandedIntSize(graphicsLayer->size())))
    return newInterestRect;

However, this breaks a test which tries to ensure that this exact thing doesn't
happen (CompositedLayerMappingTest,
InterestRectShouldNotChangeOnPaintInvalidation). I'm not familiar with the
rationale behind this. (To me, it'd make sense to use the new interest rect when
you need to paint the layer anyway.)

Chris, Xianzhu, can you shed some light on this?

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

https://codereview.chromium.org/2237433004/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/DevToolsEmulator.cpp:202: &&
m_emulationParams.scale == params.scale) {
On 2016/08/29 20:45:36, dgozman wrote:
> Please revert this.

Done.

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

https://codereview.chromium.org/2237433004/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/DevToolsEmulator.h:27: class WEB_EXPORT
DevToolsEmulator final : public GarbageCollectedFinalized<DevToolsEmulator> {
On 2016/08/29 20:45:36, dgozman wrote:
> What are we exporting this for?

So we can call force/resetViewport on it from WebViewTest. Is that ok?

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

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

Done.

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

https://codereview.chromium.org/2237433004/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/WebViewTest.cpp:1081: params.offset =
WebFloatPoint();
On 2016/08/29 20:45:36, dgozman wrote:
> Please revert these.

Done.

[commit-bot: I haz the power, 2016-09-12 13:58:11]

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

[commit-bot: I haz the power, 2016-09-12 13:58:12]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Eric Seckler, 2016-09-12 13:58:59]

The CQ bit was unchecked by eseckler@chromium.org

[Eric Seckler, 2016-09-12 14:14:02]

Patchset #11 (id:280001) has been deleted

[chrishtr, 2016-09-13 00:49:17]

chrishtr@chromium.org changed reviewers:
+ pdr@chromium.org

[chrishtr, 2016-09-13 00:49:18]

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

https://codereview.chromium.org/2237433004/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:2386:
if (!needsRepaint(*graphicsLayer) && previousInterestRect == wholeLayerRect)
On 2016/09/12 at 13:58:00, Eric Seckler wrote:
> On 2016/08/29 20:45:36, dgozman wrote:
> > 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.
> 
> I think you're right, the logic for replacing old interest rects doesn't yet
account for the rect shrinking - it's assuming that it "moves" via scrolling. I
don't think it's a problem in this early return (this only seems to happen when
no repaint is needed anyway), but more so in
interestRectChangedEnoughToRepaint(), which doesn't update the rect if it
shrinks. So if we use forceViewport to set it to e.g. the whole page, it'd stick
to the full layers forever.
> 
> Two thoughts on this: First, this shouldn't break functionality, so keeping it
as-is would be fine except for potential performance implications (particularly
for headless this is unlikely to become an issue). Second, is this also an issue
for zooming out and then back into a page? It seems to me that this would also
extend the interest rect on zoom-out and later not shrink it on zoom-in.
> 
> I'd be happy to file a bug against this and postpone it until later. One
possible work-around would be to always update the interest rect if the layer
already needs repainting. That is, change the if statement below to:
> 
> if (needsRepaint(*graphicsLayer) ||
interestRectChangedEnoughToRepaint(previousInterestRect, newInterestRect,
expandedIntSize(graphicsLayer->size())))
>     return newInterestRect;
> 
> However, this breaks a test which tries to ensure that this exact thing
doesn't happen (CompositedLayerMappingTest,
InterestRectShouldNotChangeOnPaintInvalidation). I'm not familiar with the
rationale behind this. (To me, it'd make sense to use the new interest rect when
you need to paint the layer anyway.)
> 
> Chris, Xianzhu, can you shed some light on this?

https://codereview.chromium.org/1441973003 is what changed the code to the
current behavior. It notes inconsistent behavior would
occur otherwise, but I think that is not possibly any more because of cleanup on
the cc side that happened since then. I can't think
of a reason right now why we would need the current behavior when an
invalidation or repaint is needed. Xianzhu can you think of
one?

[Xianzhu, 2016-09-13 01:20:09]

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

[Xianzhu, 2016-09-13 01:20:11]

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

https://codereview.chromium.org/2237433004/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:2386:
if (!needsRepaint(*graphicsLayer) && previousInterestRect == wholeLayerRect)
On 2016/09/13 00:49:18, chrishtr wrote:
> On 2016/09/12 at 13:58:00, Eric Seckler wrote:
> > On 2016/08/29 20:45:36, dgozman wrote:
> > > 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.
> > 
> > I think you're right, the logic for replacing old interest rects doesn't yet
> account for the rect shrinking - it's assuming that it "moves" via scrolling.
I
> don't think it's a problem in this early return (this only seems to happen
when
> no repaint is needed anyway), but more so in
> interestRectChangedEnoughToRepaint(), which doesn't update the rect if it
> shrinks. So if we use forceViewport to set it to e.g. the whole page, it'd
stick
> to the full layers forever.
> > 
> > Two thoughts on this: First, this shouldn't break functionality, so keeping
it
> as-is would be fine except for potential performance implications
(particularly
> for headless this is unlikely to become an issue). Second, is this also an
issue
> for zooming out and then back into a page? It seems to me that this would also
> extend the interest rect on zoom-out and later not shrink it on zoom-in.
> > 
> > I'd be happy to file a bug against this and postpone it until later. One
> possible work-around would be to always update the interest rect if the layer
> already needs repainting. That is, change the if statement below to:
> > 
> > if (needsRepaint(*graphicsLayer) ||
> interestRectChangedEnoughToRepaint(previousInterestRect, newInterestRect,
> expandedIntSize(graphicsLayer->size())))
> >     return newInterestRect;
> > 
> > However, this breaks a test which tries to ensure that this exact thing
> doesn't happen (CompositedLayerMappingTest,
> InterestRectShouldNotChangeOnPaintInvalidation). I'm not familiar with the
> rationale behind this. (To me, it'd make sense to use the new interest rect
when
> you need to paint the layer anyway.)
> > 
> > Chris, Xianzhu, can you shed some light on this?
> 
> https://codereview.chromium.org/1441973003 is what changed the code to the
> current behavior. It notes inconsistent behavior would
> occur otherwise, but I think that is not possibly any more because of cleanup
on
> the cc side that happened since then. I can't think
> of a reason right now why we would need the current behavior when an
> invalidation or repaint is needed. Xianzhu can you think of
> one?

The behavior is mainly for performance. See
https://bugs.chromium.org/p/chromium/issues/detail?id=563496#c25. When the page
is nearly idle (visual update is scheduled for some reason, but no real visual
changes), we avoid the cost of recomputeInterestRect() to save battery.

I think we can force recomputeInterestRect() when switching between
forceViewport and non-forceViewport mode.

[Eric Seckler, 2016-09-13 08:40:00]

On 2016/09/13 01:20:11, Xianzhu wrote:
>
https://codereview.chromium.org/2237433004/diff/240001/third_party/WebKit/Sou...
> File
> third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
> (right):
> 
>
https://codereview.chromium.org/2237433004/diff/240001/third_party/WebKit/Sou...
>
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:2386:
> if (!needsRepaint(*graphicsLayer) && previousInterestRect == wholeLayerRect)
> On 2016/09/13 00:49:18, chrishtr wrote:
> > On 2016/09/12 at 13:58:00, Eric Seckler wrote:
> > > On 2016/08/29 20:45:36, dgozman wrote:
> > > > 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.
> > > 
> > > I think you're right, the logic for replacing old interest rects doesn't
yet
> > account for the rect shrinking - it's assuming that it "moves" via
scrolling.
> I
> > don't think it's a problem in this early return (this only seems to happen
> when
> > no repaint is needed anyway), but more so in
> > interestRectChangedEnoughToRepaint(), which doesn't update the rect if it
> > shrinks. So if we use forceViewport to set it to e.g. the whole page, it'd
> stick
> > to the full layers forever.
> > > 
> > > Two thoughts on this: First, this shouldn't break functionality, so
keeping
> it
> > as-is would be fine except for potential performance implications
> (particularly
> > for headless this is unlikely to become an issue). Second, is this also an
> issue
> > for zooming out and then back into a page? It seems to me that this would
also
> > extend the interest rect on zoom-out and later not shrink it on zoom-in.
> > > 
> > > I'd be happy to file a bug against this and postpone it until later. One
> > possible work-around would be to always update the interest rect if the
layer
> > already needs repainting. That is, change the if statement below to:
> > > 
> > > if (needsRepaint(*graphicsLayer) ||
> > interestRectChangedEnoughToRepaint(previousInterestRect, newInterestRect,
> > expandedIntSize(graphicsLayer->size())))
> > >     return newInterestRect;
> > > 
> > > However, this breaks a test which tries to ensure that this exact thing
> > doesn't happen (CompositedLayerMappingTest,
> > InterestRectShouldNotChangeOnPaintInvalidation). I'm not familiar with the
> > rationale behind this. (To me, it'd make sense to use the new interest rect
> when
> > you need to paint the layer anyway.)
> > > 
> > > Chris, Xianzhu, can you shed some light on this?
> > 
> > https://codereview.chromium.org/1441973003 is what changed the code to the
> > current behavior. It notes inconsistent behavior would
> > occur otherwise, but I think that is not possibly any more because of
cleanup
> on
> > the cc side that happened since then. I can't think
> > of a reason right now why we would need the current behavior when an
> > invalidation or repaint is needed. Xianzhu can you think of
> > one?
> 
> The behavior is mainly for performance. See
> https://bugs.chromium.org/p/chromium/issues/detail?id=563496#c25. When the
page
> is nearly idle (visual update is scheduled for some reason, but no real visual
> changes), we avoid the cost of recomputeInterestRect() to save battery.

I think this relates to the early-return, but not to cases where
interestRectChangedEnoughToRepaint() returns false, right? In those cases, you
need to go through recomputeInterestRect() anyway. Is there something else down
the line that saves battery if you do a repaint with the old interest rect
rather than a new one?

> I think we can force recomputeInterestRect() when switching between
> forceViewport and non-forceViewport mode.

The way I understand the logic at the moment is that a change in interest rect
can cause a repaint, which eats battery. Thus, you try to avoid changing the
rect for small changes to avoid causing too many repaints. For our case, when we
switch to a shrunken interest rect, it'd be fine to keep the previous rect to
avoid causing a repaint just because the rect shrinks, but it should be updated
for the next repaint that is caused by something else than the interest-rect
change (to avoid repainting with a larger-than-necessary rect). Thus my thinking
of skipping interestRectChangedEnoughToRepaint() if the layer needsRepaint()
anyway (and thus the early return is skipped too). That should ensure that
future repaints happen with the shrunken rect. This shouldn't need to be
specific to forceViewport-mode (and thus could also catch the zoom-out/zoom-in
case).

If we agree that the cc/blink inconsistencies mentioned in
https://codereview.chromium.org/1441973003 aren't a problem anymore, is there
anything else where this could cause issues?

[Eric Seckler, 2016-09-13 10:58:38]

On 2016/09/13 08:40:00, Eric Seckler wrote:
> On 2016/09/13 01:20:11, Xianzhu wrote:
> >
>
https://codereview.chromium.org/2237433004/diff/240001/third_party/WebKit/Sou...
> > File
> > third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
> > (right):
> > 
> >
>
https://codereview.chromium.org/2237433004/diff/240001/third_party/WebKit/Sou...
> >
>
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:2386:
> > if (!needsRepaint(*graphicsLayer) && previousInterestRect == wholeLayerRect)
> > On 2016/09/13 00:49:18, chrishtr wrote:
> > > On 2016/09/12 at 13:58:00, Eric Seckler wrote:
> > > > On 2016/08/29 20:45:36, dgozman wrote:
> > > > > 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.
> > > > 
> > > > I think you're right, the logic for replacing old interest rects doesn't
> yet
> > > account for the rect shrinking - it's assuming that it "moves" via
> scrolling.
> > I
> > > don't think it's a problem in this early return (this only seems to happen
> > when
> > > no repaint is needed anyway), but more so in
> > > interestRectChangedEnoughToRepaint(), which doesn't update the rect if it
> > > shrinks. So if we use forceViewport to set it to e.g. the whole page, it'd
> > stick
> > > to the full layers forever.
> > > > 
> > > > Two thoughts on this: First, this shouldn't break functionality, so
> keeping
> > it
> > > as-is would be fine except for potential performance implications
> > (particularly
> > > for headless this is unlikely to become an issue). Second, is this also an
> > issue
> > > for zooming out and then back into a page? It seems to me that this would
> also
> > > extend the interest rect on zoom-out and later not shrink it on zoom-in.
> > > > 
> > > > I'd be happy to file a bug against this and postpone it until later. One
> > > possible work-around would be to always update the interest rect if the
> layer
> > > already needs repainting. That is, change the if statement below to:
> > > > 
> > > > if (needsRepaint(*graphicsLayer) ||
> > > interestRectChangedEnoughToRepaint(previousInterestRect, newInterestRect,
> > > expandedIntSize(graphicsLayer->size())))
> > > >     return newInterestRect;
> > > > 
> > > > However, this breaks a test which tries to ensure that this exact thing
> > > doesn't happen (CompositedLayerMappingTest,
> > > InterestRectShouldNotChangeOnPaintInvalidation). I'm not familiar with the
> > > rationale behind this. (To me, it'd make sense to use the new interest
rect
> > when
> > > you need to paint the layer anyway.)
> > > > 
> > > > Chris, Xianzhu, can you shed some light on this?
> > > 
> > > https://codereview.chromium.org/1441973003 is what changed the code to the
> > > current behavior. It notes inconsistent behavior would
> > > occur otherwise, but I think that is not possibly any more because of
> cleanup
> > on
> > > the cc side that happened since then. I can't think
> > > of a reason right now why we would need the current behavior when an
> > > invalidation or repaint is needed. Xianzhu can you think of
> > > one?
> > 
> > The behavior is mainly for performance. See
> > https://bugs.chromium.org/p/chromium/issues/detail?id=563496#c25. When the
> page
> > is nearly idle (visual update is scheduled for some reason, but no real
visual
> > changes), we avoid the cost of recomputeInterestRect() to save battery.
> 
> I think this relates to the early-return, but not to cases where
> interestRectChangedEnoughToRepaint() returns false, right? In those cases, you
> need to go through recomputeInterestRect() anyway. Is there something else
down
> the line that saves battery if you do a repaint with the old interest rect
> rather than a new one?
> 
> > I think we can force recomputeInterestRect() when switching between
> > forceViewport and non-forceViewport mode.
> 
> The way I understand the logic at the moment is that a change in interest rect
> can cause a repaint, which eats battery. Thus, you try to avoid changing the
> rect for small changes to avoid causing too many repaints. For our case, when
we
> switch to a shrunken interest rect, it'd be fine to keep the previous rect to
> avoid causing a repaint just because the rect shrinks, but it should be
updated
> for the next repaint that is caused by something else than the interest-rect
> change (to avoid repainting with a larger-than-necessary rect). Thus my
thinking
> of skipping interestRectChangedEnoughToRepaint() if the layer needsRepaint()
> anyway (and thus the early return is skipped too). That should ensure that
> future repaints happen with the shrunken rect. This shouldn't need to be
> specific to forceViewport-mode (and thus could also catch the zoom-out/zoom-in
> case).
> 
> If we agree that the cc/blink inconsistencies mentioned in
> https://codereview.chromium.org/1441973003 aren't a problem anymore, is there
> anything else where this could cause issues?

FYI, apart from the mentioned unit test, nothing seems to break on the bots:
https://codereview.chromium.org/2332273003/

[Xianzhu, 2016-09-13 15:54:29]

On 2016/09/13 08:40:00, Eric Seckler wrote:
> On 2016/09/13 01:20:11, Xianzhu wrote:
> >
>
https://codereview.chromium.org/2237433004/diff/240001/third_party/WebKit/Sou...
> > File
> > third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
> > (right):
> > 
> >
>
https://codereview.chromium.org/2237433004/diff/240001/third_party/WebKit/Sou...
> >
>
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:2386:
> > if (!needsRepaint(*graphicsLayer) && previousInterestRect == wholeLayerRect)
> > On 2016/09/13 00:49:18, chrishtr wrote:
> > > On 2016/09/12 at 13:58:00, Eric Seckler wrote:
> > > > On 2016/08/29 20:45:36, dgozman wrote:
> > > > > 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.
> > > > 
> > > > I think you're right, the logic for replacing old interest rects doesn't
> yet
> > > account for the rect shrinking - it's assuming that it "moves" via
> scrolling.
> > I
> > > don't think it's a problem in this early return (this only seems to happen
> > when
> > > no repaint is needed anyway), but more so in
> > > interestRectChangedEnoughToRepaint(), which doesn't update the rect if it
> > > shrinks. So if we use forceViewport to set it to e.g. the whole page, it'd
> > stick
> > > to the full layers forever.
> > > > 
> > > > Two thoughts on this: First, this shouldn't break functionality, so
> keeping
> > it
> > > as-is would be fine except for potential performance implications
> > (particularly
> > > for headless this is unlikely to become an issue). Second, is this also an
> > issue
> > > for zooming out and then back into a page? It seems to me that this would
> also
> > > extend the interest rect on zoom-out and later not shrink it on zoom-in.
> > > > 
> > > > I'd be happy to file a bug against this and postpone it until later. One
> > > possible work-around would be to always update the interest rect if the
> layer
> > > already needs repainting. That is, change the if statement below to:
> > > > 
> > > > if (needsRepaint(*graphicsLayer) ||
> > > interestRectChangedEnoughToRepaint(previousInterestRect, newInterestRect,
> > > expandedIntSize(graphicsLayer->size())))
> > > >     return newInterestRect;
> > > > 
> > > > However, this breaks a test which tries to ensure that this exact thing
> > > doesn't happen (CompositedLayerMappingTest,
> > > InterestRectShouldNotChangeOnPaintInvalidation). I'm not familiar with the
> > > rationale behind this. (To me, it'd make sense to use the new interest
rect
> > when
> > > you need to paint the layer anyway.)
> > > > 
> > > > Chris, Xianzhu, can you shed some light on this?
> > > 
> > > https://codereview.chromium.org/1441973003 is what changed the code to the
> > > current behavior. It notes inconsistent behavior would
> > > occur otherwise, but I think that is not possibly any more because of
> cleanup
> > on
> > > the cc side that happened since then. I can't think
> > > of a reason right now why we would need the current behavior when an
> > > invalidation or repaint is needed. Xianzhu can you think of
> > > one?
> > 
> > The behavior is mainly for performance. See
> > https://bugs.chromium.org/p/chromium/issues/detail?id=563496#c25. When the
> page
> > is nearly idle (visual update is scheduled for some reason, but no real
visual
> > changes), we avoid the cost of recomputeInterestRect() to save battery.
> 
> I think this relates to the early-return, but not to cases where
> interestRectChangedEnoughToRepaint() returns false, right? In those cases, you
> need to go through recomputeInterestRect() anyway. Is there something else
down
> the line that saves battery if you do a repaint with the old interest rect
> rather than a new one?
> 
> > I think we can force recomputeInterestRect() when switching between
> > forceViewport and non-forceViewport mode.
> 
> The way I understand the logic at the moment is that a change in interest rect
> can cause a repaint, which eats battery. Thus, you try to avoid changing the
> rect for small changes to avoid causing too many repaints. For our case, when
we
> switch to a shrunken interest rect, it'd be fine to keep the previous rect to
> avoid causing a repaint just because the rect shrinks, but it should be
updated
> for the next repaint that is caused by something else than the interest-rect
> change (to avoid repainting with a larger-than-necessary rect). Thus my
thinking
> of skipping interestRectChangedEnoughToRepaint() if the layer needsRepaint()
> anyway (and thus the early return is skipped too). That should ensure that
> future repaints happen with the shrunken rect. This shouldn't need to be
> specific to forceViewport-mode (and thus could also catch the zoom-out/zoom-in
> case).
> 
> If we agree that the cc/blink inconsistencies mentioned in
> https://codereview.chromium.org/1441973003 aren't a problem anymore, is there
> anything else where this could cause issues?

recomputeInterestRect() itself also eats battery. The optimization is to avoid
recomputeInterestRect() if possible.

[Eric Seckler, 2016-09-13 16:01:16]

On 2016/09/13 15:54:29, Xianzhu wrote:
> On 2016/09/13 08:40:00, Eric Seckler wrote:
> > On 2016/09/13 01:20:11, Xianzhu wrote:
> > >
> >
>
https://codereview.chromium.org/2237433004/diff/240001/third_party/WebKit/Sou...
> > > File
> > >
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2237433004/diff/240001/third_party/WebKit/Sou...
> > >
> >
>
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:2386:
> > > if (!needsRepaint(*graphicsLayer) && previousInterestRect ==
wholeLayerRect)
> > > On 2016/09/13 00:49:18, chrishtr wrote:
> > > > On 2016/09/12 at 13:58:00, Eric Seckler wrote:
> > > > > On 2016/08/29 20:45:36, dgozman wrote:
> > > > > > 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.
> > > > > 
> > > > > I think you're right, the logic for replacing old interest rects
doesn't
> > yet
> > > > account for the rect shrinking - it's assuming that it "moves" via
> > scrolling.
> > > I
> > > > don't think it's a problem in this early return (this only seems to
happen
> > > when
> > > > no repaint is needed anyway), but more so in
> > > > interestRectChangedEnoughToRepaint(), which doesn't update the rect if
it
> > > > shrinks. So if we use forceViewport to set it to e.g. the whole page,
it'd
> > > stick
> > > > to the full layers forever.
> > > > > 
> > > > > Two thoughts on this: First, this shouldn't break functionality, so
> > keeping
> > > it
> > > > as-is would be fine except for potential performance implications
> > > (particularly
> > > > for headless this is unlikely to become an issue). Second, is this also
an
> > > issue
> > > > for zooming out and then back into a page? It seems to me that this
would
> > also
> > > > extend the interest rect on zoom-out and later not shrink it on zoom-in.
> > > > > 
> > > > > I'd be happy to file a bug against this and postpone it until later.
One
> > > > possible work-around would be to always update the interest rect if the
> > layer
> > > > already needs repainting. That is, change the if statement below to:
> > > > > 
> > > > > if (needsRepaint(*graphicsLayer) ||
> > > > interestRectChangedEnoughToRepaint(previousInterestRect,
newInterestRect,
> > > > expandedIntSize(graphicsLayer->size())))
> > > > >     return newInterestRect;
> > > > > 
> > > > > However, this breaks a test which tries to ensure that this exact
thing
> > > > doesn't happen (CompositedLayerMappingTest,
> > > > InterestRectShouldNotChangeOnPaintInvalidation). I'm not familiar with
the
> > > > rationale behind this. (To me, it'd make sense to use the new interest
> rect
> > > when
> > > > you need to paint the layer anyway.)
> > > > > 
> > > > > Chris, Xianzhu, can you shed some light on this?
> > > > 
> > > > https://codereview.chromium.org/1441973003 is what changed the code to
the
> > > > current behavior. It notes inconsistent behavior would
> > > > occur otherwise, but I think that is not possibly any more because of
> > cleanup
> > > on
> > > > the cc side that happened since then. I can't think
> > > > of a reason right now why we would need the current behavior when an
> > > > invalidation or repaint is needed. Xianzhu can you think of
> > > > one?
> > > 
> > > The behavior is mainly for performance. See
> > > https://bugs.chromium.org/p/chromium/issues/detail?id=563496#c25. When the
> > page
> > > is nearly idle (visual update is scheduled for some reason, but no real
> visual
> > > changes), we avoid the cost of recomputeInterestRect() to save battery.
> > 
> > I think this relates to the early-return, but not to cases where
> > interestRectChangedEnoughToRepaint() returns false, right? In those cases,
you
> > need to go through recomputeInterestRect() anyway. Is there something else
> down
> > the line that saves battery if you do a repaint with the old interest rect
> > rather than a new one?
> > 
> > > I think we can force recomputeInterestRect() when switching between
> > > forceViewport and non-forceViewport mode.
> > 
> > The way I understand the logic at the moment is that a change in interest
rect
> > can cause a repaint, which eats battery. Thus, you try to avoid changing the
> > rect for small changes to avoid causing too many repaints. For our case,
when
> we
> > switch to a shrunken interest rect, it'd be fine to keep the previous rect
to
> > avoid causing a repaint just because the rect shrinks, but it should be
> updated
> > for the next repaint that is caused by something else than the interest-rect
> > change (to avoid repainting with a larger-than-necessary rect). Thus my
> thinking
> > of skipping interestRectChangedEnoughToRepaint() if the layer needsRepaint()
> > anyway (and thus the early return is skipped too). That should ensure that
> > future repaints happen with the shrunken rect. This shouldn't need to be
> > specific to forceViewport-mode (and thus could also catch the
zoom-out/zoom-in
> > case).
> > 
> > If we agree that the cc/blink inconsistencies mentioned in
> > https://codereview.chromium.org/1441973003 aren't a problem anymore, is
there
> > anything else where this could cause issues?
> 
> recomputeInterestRect() itself also eats battery. The optimization is to avoid
> recomputeInterestRect() if possible.

Sure, I was saying that recomputeInterestRect() is always called when
needsRepaint() is true, since only the early return would avoid that, right? I'm
talking about changing the if statement below the call to
recomputeInterestRect(). Does that make sense? :)

[Xianzhu, 2016-09-13 16:09:58]

On 2016/09/13 16:01:16, Eric Seckler wrote:
> On 2016/09/13 15:54:29, Xianzhu wrote:
> > On 2016/09/13 08:40:00, Eric Seckler wrote:
> > > 
> > > The way I understand the logic at the moment is that a change in interest
> rect
> > > can cause a repaint, which eats battery. Thus, you try to avoid changing
the
> > > rect for small changes to avoid causing too many repaints. For our case,
> when
> > we
> > > switch to a shrunken interest rect, it'd be fine to keep the previous rect
> to
> > > avoid causing a repaint just because the rect shrinks, but it should be
> > updated
> > > for the next repaint that is caused by something else than the
interest-rect
> > > change (to avoid repainting with a larger-than-necessary rect). Thus my
> > thinking
> > > of skipping interestRectChangedEnoughToRepaint() if the layer
needsRepaint()
> > > anyway (and thus the early return is skipped too). That should ensure that
> > > future repaints happen with the shrunken rect. This shouldn't need to be
> > > specific to forceViewport-mode (and thus could also catch the
> zoom-out/zoom-in
> > > case).
> > > 
> > > If we agree that the cc/blink inconsistencies mentioned in
> > > https://codereview.chromium.org/1441973003 aren't a problem anymore, is
> there
> > > anything else where this could cause issues?
> > 
> > recomputeInterestRect() itself also eats battery. The optimization is to
avoid
> > recomputeInterestRect() if possible.
> 
> Sure, I was saying that recomputeInterestRect() is always called when
> needsRepaint() is true, since only the early return would avoid that, right?
I'm
> talking about changing the if statement below the call to
> recomputeInterestRect(). Does that make sense? :)

Can you add your proposed code here?

[Xianzhu, 2016-09-13 16:13:36]

On 2016/09/13 16:09:58, Xianzhu wrote:
> On 2016/09/13 16:01:16, Eric Seckler wrote:
> > On 2016/09/13 15:54:29, Xianzhu wrote:
> > > On 2016/09/13 08:40:00, Eric Seckler wrote:
> > > > 
> > > > The way I understand the logic at the moment is that a change in
interest
> > rect
> > > > can cause a repaint, which eats battery. Thus, you try to avoid changing
> the
> > > > rect for small changes to avoid causing too many repaints. For our case,
> > when
> > > we
> > > > switch to a shrunken interest rect, it'd be fine to keep the previous
rect
> > to
> > > > avoid causing a repaint just because the rect shrinks, but it should be
> > > updated
> > > > for the next repaint that is caused by something else than the
> interest-rect
> > > > change (to avoid repainting with a larger-than-necessary rect). Thus my
> > > thinking
> > > > of skipping interestRectChangedEnoughToRepaint() if the layer
> needsRepaint()
> > > > anyway (and thus the early return is skipped too). That should ensure
that
> > > > future repaints happen with the shrunken rect. This shouldn't need to be
> > > > specific to forceViewport-mode (and thus could also catch the
> > zoom-out/zoom-in
> > > > case).
> > > > 
> > > > If we agree that the cc/blink inconsistencies mentioned in
> > > > https://codereview.chromium.org/1441973003 aren't a problem anymore, is
> > there
> > > > anything else where this could cause issues?
> > > 
> > > recomputeInterestRect() itself also eats battery. The optimization is to
> avoid
> > > recomputeInterestRect() if possible.
> > 
> > Sure, I was saying that recomputeInterestRect() is always called when
> > needsRepaint() is true, since only the early return would avoid that, right?
> I'm
> > talking about changing the if statement below the call to
> > recomputeInterestRect(). Does that make sense? :)
> 
> Can you add your proposed code here?

Never mind. Now I understand your idea. It sgtm.

[chrishtr, 2016-09-13 16:39:29]

On 2016/09/13 at 16:13:36, wangxianzhu wrote:
> On 2016/09/13 16:09:58, Xianzhu wrote:
> > On 2016/09/13 16:01:16, Eric Seckler wrote:
> > > On 2016/09/13 15:54:29, Xianzhu wrote:
> > > > On 2016/09/13 08:40:00, Eric Seckler wrote:
> > > > > 
> > > > > The way I understand the logic at the moment is that a change in
interest
> > > rect
> > > > > can cause a repaint, which eats battery. Thus, you try to avoid
changing
> > the
> > > > > rect for small changes to avoid causing too many repaints. For our
case,
> > > when
> > > > we
> > > > > switch to a shrunken interest rect, it'd be fine to keep the previous
rect
> > > to
> > > > > avoid causing a repaint just because the rect shrinks, but it should
be
> > > > updated
> > > > > for the next repaint that is caused by something else than the
> > interest-rect
> > > > > change (to avoid repainting with a larger-than-necessary rect). Thus
my
> > > > thinking
> > > > > of skipping interestRectChangedEnoughToRepaint() if the layer
> > needsRepaint()
> > > > > anyway (and thus the early return is skipped too). That should ensure
that
> > > > > future repaints happen with the shrunken rect. This shouldn't need to
be
> > > > > specific to forceViewport-mode (and thus could also catch the
> > > zoom-out/zoom-in
> > > > > case).
> > > > > 
> > > > > If we agree that the cc/blink inconsistencies mentioned in
> > > > > https://codereview.chromium.org/1441973003 aren't a problem anymore,
is
> > > there
> > > > > anything else where this could cause issues?
> > > > 
> > > > recomputeInterestRect() itself also eats battery. The optimization is to
> > avoid
> > > > recomputeInterestRect() if possible.
> > > 
> > > Sure, I was saying that recomputeInterestRect() is always called when
> > > needsRepaint() is true, since only the early return would avoid that,
right?
> > I'm
> > > talking about changing the if statement below the call to
> > > recomputeInterestRect(). Does that make sense? :)
> > 
> > Can you add your proposed code here?
> 
> Never mind. Now I understand your idea. It sgtm.

+1. The optimizations after
https://bugs.chromium.org/p/chromium/issues/detail?id=563496#c25 will still
apply after thie proposed change in this CL.

[chrishtr, 2016-09-13 16:40:10]

eseckler: please send that change as a standalone patch, so we can isolate its
impact from the rest of this CL.

[Eric Seckler, 2016-09-13 16:49:45]

On 2016/09/13 16:40:10, chrishtr wrote:
> eseckler: please send that change as a standalone patch, so we can isolate its
> impact from the rest of this CL.

Thanks, done --> https://codereview.chromium.org/2332273003/

Dmitry, does this sound good to you as well? :)

[Eric Seckler, 2016-09-16 13:05:55]

Dmitry, PTAL - related patch is submitted, all comments here are now addressed.
Thanks!

[Eric Seckler, 2016-09-16 13:07:23]

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

[commit-bot: I haz the power, 2016-09-16 13:07:34]

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

[commit-bot: I haz the power, 2016-09-16 15:37:34]

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

[commit-bot: I haz the power, 2016-09-16 15:37:36]

Dry run: This issue passed the CQ dry run.

[dgozman, 2016-09-17 01:22:28]

Great job! lgtm

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

https://codereview.chromium.org/2237433004/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/DevToolsEmulator.cpp:340:
containerLayer->setMasksToBounds(false);
This line should be next to |originalVisualViewportMasking = ...|

https://codereview.chromium.org/2237433004/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/DevToolsEmulator.cpp:355: // Restore original
state.
nit: I don't think this comment brings anything.

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

https://codereview.chromium.org/2237433004/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/InspectorEmulationAgent.cpp:92: WebFloatPoint
position(x, y);
nit: can inline below as WebFloatPoint(x, y)

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

https://codereview.chromium.org/2237433004/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebViewImpl.cpp:4604: float
WebViewImpl::compositorDeviceScaleFactor() const
Let's inline this method into DevToolsEmulator, since it already knows the
override it forced.

[Eric Seckler, 2016-09-22 11:31:55]

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

[commit-bot: I haz the power, 2016-09-22 11:32:08]

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

[Eric Seckler, 2016-09-22 12:43:00]

Thanks!

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

https://codereview.chromium.org/2237433004/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/DevToolsEmulator.cpp:340:
containerLayer->setMasksToBounds(false);
On 2016/09/17 01:22:28, dgozman wrote:
> This line should be next to |originalVisualViewportMasking = ...|

Done.

https://codereview.chromium.org/2237433004/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/DevToolsEmulator.cpp:355: // Restore original
state.
On 2016/09/17 01:22:28, dgozman wrote:
> nit: I don't think this comment brings anything.

Done.

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

https://codereview.chromium.org/2237433004/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/InspectorEmulationAgent.cpp:92: WebFloatPoint
position(x, y);
On 2016/09/17 01:22:28, dgozman wrote:
> nit: can inline below as WebFloatPoint(x, y)

Done.

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

https://codereview.chromium.org/2237433004/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebViewImpl.cpp:4604: float
WebViewImpl::compositorDeviceScaleFactor() const
On 2016/09/17 01:22:28, dgozman wrote:
> Let's inline this method into DevToolsEmulator, since it already knows the
> override it forced.

Done.

[Eric Seckler, 2016-09-22 12:43:34]

The CQ bit was unchecked by eseckler@chromium.org

[Eric Seckler, 2016-09-22 12:43:35]

The CQ bit was checked by eseckler@chromium.org

[Eric Seckler, 2016-09-22 12:43:35]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrishtr@chromium.org, pdr@chromium.org, jbauman@chromium.org,
dgozman@chromium.org
Link to the patchset: https://codereview.chromium.org/2237433004/#ps320001
(title: "Adress Dmitry's comments + sync.")

[commit-bot: I haz the power, 2016-09-22 12:43:49]

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

[commit-bot: I haz the power, 2016-09-22 12:51:15]

Description was changed from

==========
Adds DevTools commands for forced viewport 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 visible content area used in CompositedLayerMapping and disable clipping
to the visual viewport while the override is active.

BUG=625577
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Adds DevTools commands for forced viewport 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 visible content area used in CompositedLayerMapping and disable clipping
to the visual viewport while the override is active.

BUG=625577
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[commit-bot: I haz the power, 2016-09-22 12:51:18]

Committed patchset #12 (id:320001)

[commit-bot: I haz the power, 2016-09-22 12:53:30]

Description was changed from

==========
Adds DevTools commands for forced viewport 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 visible content area used in CompositedLayerMapping and disable clipping
to the visual viewport while the override is active.

BUG=625577
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Adds DevTools commands for forced viewport 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 visible content area used in CompositedLayerMapping and disable clipping
to the visual viewport while the override is active.

BUG=625577
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/23d8f263fccca2ef163a9c3f1592d3a51e3a2d50
Cr-Commit-Position: refs/heads/master@{#420314}
==========

[commit-bot: I haz the power, 2016-09-22 12:53:33]

Patchset 12 (id:??) landed as
https://crrev.com/23d8f263fccca2ef163a9c3f1592d3a51e3a2d50
Cr-Commit-Position: refs/heads/master@{#420314}