Blink side of changes to enable cache clearing without a flag.

Blink side of changes to enable cache clearing without a flag.

This is needed for rasterize_and_record future proofing against
the removal of the command line flag for cache clearing. It's
also cleaner and more transparent than setting a flag in the
benchmark.

Note that some code is temporary to allow the two-sided change
with the chromium repository.

R=pdr,chrishtr
BUG=451448
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=189548

[Stephen Chennney, 2015-02-03 17:15:24]

https://codereview.chromium.org/880653004/diff/1/Source/web/LinkHighlight.h
File Source/web/LinkHighlight.h (right):

https://codereview.chromium.org/880653004/diff/1/Source/web/LinkHighlight.h#n...
Source/web/LinkHighlight.h:61: WebContentLayerClient::GraphicsContextStatus =
WebContentLayerClient::GraphicsContextEnabled) override { }
Needed to add the extra default arg to allow for the default bool arg.

https://codereview.chromium.org/880653004/diff/1/public/platform/WebContentLa...
File public/platform/WebContentLayerClient.h (left):

https://codereview.chromium.org/880653004/diff/1/public/platform/WebContentLa...
public/platform/WebContentLayerClient.h:47: // optimizations.
This comment is referring to something that no longer exists. Some time ago I
removed the code but not the comment.

[chrishtr, 2015-02-03 17:21:49]

How does the continuous painting mode in devtools work? Is that an approach we
could reuse here?

[Stephen Chennney, 2015-02-03 17:30:04]

On 2015/02/03 17:21:49, chrishtr wrote:
> How does the continuous painting mode in devtools work? Is that an approach we
> could reuse here?

Continuous painting calls setNeedsPaint on the GraphicsLayers (or whatever it is
that uses that call to drive paint). It doesn't influence caching at all, and in
our pre-Slimming Paint architecture, calling setNeedsPaint with the entire
window rect causes us to record everything again. And R&R doesn't go through
that code path - it invokes the PaintContents methods directly.

Now I thought that continuous painting was causing cache clearing, but now I'm
not so sure. I'll have to look again. Regardless, it's independent of this code
path.

[Stephen Chennney, 2015-02-03 19:51:05]

On 2015/02/03 17:30:04, Stephen Chenney wrote:
> On 2015/02/03 17:21:49, chrishtr wrote:
> > How does the continuous painting mode in devtools work? Is that an approach
we
> > could reuse here?
> 
> Continuous painting calls setNeedsPaint on the GraphicsLayers (or whatever it
is
> that uses that call to drive paint). It doesn't influence caching at all, and
in
> our pre-Slimming Paint architecture, calling setNeedsPaint with the entire
> window rect causes us to record everything again. And R&R doesn't go through
> that code path - it invokes the PaintContents methods directly.
> 
> Now I thought that continuous painting was causing cache clearing, but now I'm
> not so sure. I'll have to look again. Regardless, it's independent of this
code
> path.

OK, after going down a rabbit hole I've got this figured out again. The
inspector web layer calls setContinuousPaintingEnabled on the WebViewImpl. That
flag turns on code in WebViewImpl::beginFrame that ultimately calls
GraphicsLayer::setNeedsDisplay which invalidates the display lists to defeat
caching. Note that it calls setNeedsDisplay which repaints the entire layer,
instead of setNeedsDisplayInRect which sets repaint for a sub-rect.

In R&R, we get to the GraphicsLayer paint methods via its ContentLayerDelegate
(which implements WebContentLayerClient) and there is no way to turn on
continuous repaint from that. And we don't want the side effect of painting the
whole layer; we want to be explicit in R&R about exactly what we are recording
and rasterizing.

[chrishtr, 2015-02-03 20:01:21]

Thanks for the clear explanation.

https://codereview.chromium.org/880653004/diff/1/public/platform/WebContentLa...
File public/platform/WebContentLayerClient.h (right):

https://codereview.chromium.org/880653004/diff/1/public/platform/WebContentLa...
public/platform/WebContentLayerClient.h:69: const bool clearCaches = false,
How about an enum { ShouldRespectPaintCache, ShouldIgnorePaintCache} instead of
the bool argument?

[Stephen Chennney, 2015-02-03 20:05:56]

On 2015/02/03 20:01:21, chrishtr wrote:
> Thanks for the clear explanation.
> 
>
https://codereview.chromium.org/880653004/diff/1/public/platform/WebContentLa...
> File public/platform/WebContentLayerClient.h (right):
> 
>
https://codereview.chromium.org/880653004/diff/1/public/platform/WebContentLa...
> public/platform/WebContentLayerClient.h:69: const bool clearCaches = false,
> How about an enum { ShouldRespectPaintCache, ShouldIgnorePaintCache} instead
of
> the bool argument?

Even better, how about we piggy back on the existing graphics context enum,
since in principle this is really a "recording behavior modifier" like the
existing graphics context status? It means the double sided patch would be
simpler too.

[chrishtr, 2015-02-03 20:06:25]

On 2015/02/03 at 20:05:56, schenney wrote:
> On 2015/02/03 20:01:21, chrishtr wrote:
> > Thanks for the clear explanation.
> > 
> >
https://codereview.chromium.org/880653004/diff/1/public/platform/WebContentLa...
> > File public/platform/WebContentLayerClient.h (right):
> > 
> >
https://codereview.chromium.org/880653004/diff/1/public/platform/WebContentLa...
> > public/platform/WebContentLayerClient.h:69: const bool clearCaches = false,
> > How about an enum { ShouldRespectPaintCache, ShouldIgnorePaintCache} instead
of
> > the bool argument?
> 
> Even better, how about we piggy back on the existing graphics context enum,
since in principle this is really a "recording behavior modifier" like the
existing graphics context status? It means the double sided patch would be
simpler too.

Sure that sounds promising.

[Stephen Chennney, 2015-02-03 21:38:24]

I've renamed the enum for controlling painting (leaving the old one in place to
bridge the chromium change) and provided modes that cover our existing use
cases. One of them is a perf case we might consider: running all the code to
create lists but not emitting the items. That would give us the closest thing we
could have to the existing GraphicsContextDisabled behavior.

[chrishtr, 2015-02-03 22:05:43]

https://codereview.chromium.org/880653004/diff/40001/Source/platform/graphics...
File Source/platform/graphics/ContentLayerDelegate.cpp (right):

https://codereview.chromium.org/880653004/diff/40001/Source/platform/graphics...
Source/platform/graphics/ContentLayerDelegate.cpp:62: GraphicsContext
context(canvas, m_painter->displayItemList(), paintingControl ==
WebContentLayerClient::PaintDefaultBehavior ? GraphicsContext::NothingDisabled :
GraphicsContext::FullyDisabled);
Don't you want to set FullyDisabled only with DisplayListConstructionDisabled?

[Stephen Chennney, 2015-02-03 22:11:33]

https://codereview.chromium.org/880653004/diff/40001/Source/platform/graphics...
File Source/platform/graphics/ContentLayerDelegate.cpp (right):

https://codereview.chromium.org/880653004/diff/40001/Source/platform/graphics...
Source/platform/graphics/ContentLayerDelegate.cpp:62: GraphicsContext
context(canvas, m_painter->displayItemList(), paintingControl ==
WebContentLayerClient::PaintDefaultBehavior ? GraphicsContext::NothingDisabled :
GraphicsContext::FullyDisabled);
On 2015/02/03 22:05:43, chrishtr wrote:
> Don't you want to set FullyDisabled only with DisplayListConstructionDisabled?

Yes, that makes more sense. We can then get 3 modes with Slimming Paint:
default, cache disabled, and nothing in SkPictures.

[chrishtr, 2015-02-03 22:13:36]

LGTM with that changed.

[Stephen Chennney, 2015-02-03 22:20:37]

New patchsets have been uploaded after l-g-t-m from chrishtr@chromium.org

[Stephen Chennney, 2015-02-03 22:21:23]

On 2015/02/03 22:13:36, chrishtr wrote:
> LGTM with that changed.

Thanks. I've done that. I won't commit until tomorrow morning when I can make
sure it works properly with the chromium side changes.

[Stephen Chennney, 2015-02-04 21:39:43]

The CQ bit was checked by schenney@chromium.org

[commit-bot: I haz the power, 2015-02-04 21:40:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/880653004/60001

[commit-bot: I haz the power, 2015-02-05 10:30:52]

Committed patchset #4 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=189548