Enable fast/images with slimming paint

Enable virtual/slimmingpaint/fast/images test suite.

- Refactor LayerTransparencyHelper to a more generic TransparencyRecorder.
- Fix an instance of raw clipping in HTMLCanvasPainter.
- Take clipping out of TransparencyDisplayItems.

NOTRY=true

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=185982

[leviw_travelin_and_unemployed, 2014-11-20 19:50:39]

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

[chrishtr, 2014-11-20 20:01:24]

Also, the raw clipping is in HTMLCanvasPainter, not BoxPainter.

https://codereview.chromium.org/744163002/diff/40001/Source/core/paint/Transp...
File Source/core/paint/TransparencyDisplayItem.h (right):

https://codereview.chromium.org/744163002/diff/40001/Source/core/paint/Transp...
Source/core/paint/TransparencyDisplayItem.h:14: // FIXME: Move this file to
TransparencyRecorder
There is no TransparencyRecorder in this patchset.

[leviw_travelin_and_unemployed, 2014-11-20 20:09:36]

> Also, the raw clipping is in HTMLCanvasPainter, not BoxPainter.

Good point :)

https://codereview.chromium.org/744163002/diff/40001/Source/core/paint/Transp...
File Source/core/paint/TransparencyDisplayItem.h (right):

https://codereview.chromium.org/744163002/diff/40001/Source/core/paint/Transp...
Source/core/paint/TransparencyDisplayItem.h:14: // FIXME: Move this file to
TransparencyRecorder
On 2014/11/20 at 20:01:24, chrishtr wrote:
> There is no TransparencyRecorder in this patchset.

False! It's right below this :p

[pdr., 2014-11-20 20:10:02]

LGTM modulo compile failure and Chris's comment about the change description.

[chrishtr, 2014-11-20 20:10:47]

https://codereview.chromium.org/744163002/diff/40001/Source/core/paint/Transp...
File Source/core/paint/TransparencyDisplayItem.h (right):

https://codereview.chromium.org/744163002/diff/40001/Source/core/paint/Transp...
Source/core/paint/TransparencyDisplayItem.h:14: // FIXME: Move this file to
TransparencyRecorder
On 2014/11/20 at 20:09:36, leviw wrote:
> On 2014/11/20 at 20:01:24, chrishtr wrote:
> > There is no TransparencyRecorder in this patchset.
> 
> False! It's right below this :p

Ah. LOL.

[chrishtr, 2014-11-20 20:10:55]

lgtm

[leviw_travelin_and_unemployed, 2014-11-20 20:14:49]

Thanks guys :)

[leviw_travelin_and_unemployed, 2014-11-20 21:01:34]

The CQ bit was checked by leviw@chromium.org

[commit-bot: I haz the power, 2014-11-20 21:14:37]

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

[commit-bot: I haz the power, 2014-11-20 21:27:57]

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

[commit-bot: I haz the power, 2014-11-20 21:27:58]

Try jobs failed on following builders:
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/31956)

[leviw_travelin_and_unemployed, 2014-11-20 21:36:45]

The CQ bit was checked by leviw@chromium.org

[commit-bot: I haz the power, 2014-11-20 21:37:49]

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

[commit-bot: I haz the power, 2014-11-20 22:43:01]

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

[commit-bot: I haz the power, 2014-11-20 22:43:02]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/3...)

[leviw_travelin_and_unemployed, 2014-11-20 23:15:12]

The CQ bit was checked by leviw@chromium.org

[commit-bot: I haz the power, 2014-11-20 23:16:51]

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

[commit-bot: I haz the power, 2014-11-21 00:07:37]

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

[commit-bot: I haz the power, 2014-11-21 00:07:38]

Try jobs failed on following builders:
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/31999)

[leviw_travelin_and_unemployed, 2014-11-21 01:33:57]

The CQ bit was checked by leviw@chromium.org

[commit-bot: I haz the power, 2014-11-21 01:34:56]

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

[commit-bot: I haz the power, 2014-11-21 03:18:44]

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

[commit-bot: I haz the power, 2014-11-21 03:18:45]

Try jobs failed on following builders:
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/32044)

[leviw_travelin_and_unemployed, 2014-11-21 20:01:09]

Damn... PTAL.

Turns out we don't always want clipping when we create a transparency layer. I
added an enum to control that behavior.

[chrishtr, 2014-11-21 20:08:26]

https://codereview.chromium.org/744163002/diff/120001/Source/core/paint/BoxPa...
File Source/core/paint/BoxPainter.cpp (right):

https://codereview.chromium.org/744163002/diff/120001/Source/core/paint/BoxPa...
Source/core/paint/BoxPainter.cpp:536:
paintInfo.context->setCompositeOperation(CompositeDestinationIn);
This line needs to be in a display item of some kind. Not this one?

[chrishtr, 2014-11-21 20:09:02]

https://codereview.chromium.org/744163002/diff/120001/Source/core/paint/Trans...
File Source/core/paint/TransparencyDisplayItem.cpp (right):

https://codereview.chromium.org/744163002/diff/120001/Source/core/paint/Trans...
Source/core/paint/TransparencyDisplayItem.cpp:17: if (m_clipBehavior ==
ClipToRect) {
Should we just create an extra ClipDisplayItem rather than having this code
here?

[pdr., 2014-11-21 20:09:05]

https://codereview.chromium.org/744163002/diff/120001/Source/core/paint/Layer...
File Source/core/paint/LayerPainter.cpp (right):

https://codereview.chromium.org/744163002/diff/120001/Source/core/paint/Layer...
Source/core/paint/LayerPainter.cpp:212: transparencyRecorder = adoptPtr(new
TransparencyRecorder(context, m_renderLayer.renderer(),
DisplayItem::BeginTransparency,
m_renderLayer.paintingExtent(paintingInfo.rootLayer,
paintingInfo.paintDirtyRect,
What do you think about emiting a clip here with the clip recorder instead of
baking it into the transparency recorder?

[leviw_travelin_and_unemployed, 2014-11-21 20:10:53]

On 2014/11/21 at 20:09:05, pdr wrote:
>
https://codereview.chromium.org/744163002/diff/120001/Source/core/paint/Layer...
> File Source/core/paint/LayerPainter.cpp (right):
> 
>
https://codereview.chromium.org/744163002/diff/120001/Source/core/paint/Layer...
> Source/core/paint/LayerPainter.cpp:212: transparencyRecorder = adoptPtr(new
TransparencyRecorder(context, m_renderLayer.renderer(),
DisplayItem::BeginTransparency,
m_renderLayer.paintingExtent(paintingInfo.rootLayer,
paintingInfo.paintDirtyRect,
> What do you think about emiting a clip here with the clip recorder instead of
baking it into the transparency recorder?

I think next time you should try to get your similar comments down to <30
seconds apart ;)

I'll make that change. I saw a bunch of other spots where we're doing similar
work (clip + transparency), but I agree it's probably better to keep them
separate.

[pdr., 2014-11-21 20:13:37]

On 2014/11/21 at 20:10:53, leviw wrote:
> On 2014/11/21 at 20:09:05, pdr wrote:
> >
https://codereview.chromium.org/744163002/diff/120001/Source/core/paint/Layer...
> > File Source/core/paint/LayerPainter.cpp (right):
> > 
> >
https://codereview.chromium.org/744163002/diff/120001/Source/core/paint/Layer...
> > Source/core/paint/LayerPainter.cpp:212: transparencyRecorder = adoptPtr(new
TransparencyRecorder(context, m_renderLayer.renderer(),
DisplayItem::BeginTransparency,
m_renderLayer.paintingExtent(paintingInfo.rootLayer,
paintingInfo.paintDirtyRect,
> > What do you think about emiting a clip here with the clip recorder instead
of baking it into the transparency recorder?
> 
> I think next time you should try to get your similar comments down to <30
seconds apart ;)

It was 4 seconds! Crazy :P

[leviw_travelin_and_unemployed, 2014-11-21 22:15:50]

https://codereview.chromium.org/744163002/diff/140001/Source/core/paint/Trans...
File Source/core/paint/TransparencyDisplayItem.cpp (right):

https://codereview.chromium.org/744163002/diff/140001/Source/core/paint/Trans...
Source/core/paint/TransparencyDisplayItem.cpp:26: context->fillRect(clipRect,
Color(0.0f, 0.0f, 0.5f, 0.2f));
How much do we care about this? If we delete this code, I can remove the "clip"
rect. WDYT?

[chrishtr, 2014-11-21 22:17:42]

https://codereview.chromium.org/744163002/diff/140001/Source/core/paint/Trans...
File Source/core/paint/TransparencyDisplayItem.cpp (right):

https://codereview.chromium.org/744163002/diff/140001/Source/core/paint/Trans...
Source/core/paint/TransparencyDisplayItem.cpp:26: context->fillRect(clipRect,
Color(0.0f, 0.0f, 0.5f, 0.2f));
On 2014/11/21 22:15:50, leviw wrote:
> How much do we care about this? If we delete this code, I can remove the
"clip"
> rect. WDYT?

Is this ever #defined? I can't find it in codesearch with a search
of "REVEAL_TRANSPARENCY_LAYERS"

[pdr., 2014-11-21 22:19:27]

On 2014/11/21 at 22:15:50, leviw wrote:
>
https://codereview.chromium.org/744163002/diff/140001/Source/core/paint/Trans...
> File Source/core/paint/TransparencyDisplayItem.cpp (right):
> 
>
https://codereview.chromium.org/744163002/diff/140001/Source/core/paint/Trans...
> Source/core/paint/TransparencyDisplayItem.cpp:26: context->fillRect(clipRect,
Color(0.0f, 0.0f, 0.5f, 0.2f));
> How much do we care about this? If we delete this code, I can remove the
"clip" rect. WDYT?

Wow :P Honey badger doesn't mind if we remove that.

[leviw_travelin_and_unemployed, 2014-11-21 22:34:05]

Chris, it's a debug tool you can turn on yourself if you wanna see 'em. I took
it out and there was much rejoicing.

Flags changed | before | after
              | r=+    | r=?

[chrishtr, 2014-11-21 22:35:30]

lgtm

[pdr., 2014-11-21 22:35:51]

On 2014/11/21 at 22:35:30, chrishtr wrote:
> lgtm

R=me

[leviw_travelin_and_unemployed, 2014-11-21 22:41:07]

The CQ bit was checked by leviw@chromium.org

[leviw_travelin_and_unemployed, 2014-11-21 22:41:08]

<3

[commit-bot: I haz the power, 2014-11-21 22:41:33]

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

[chrishtr, 2014-11-21 22:43:21]

lgtm

[chrishtr, 2014-11-21 22:43:25]

lgtm

[chrishtr, 2014-11-21 22:43:30]

lgtm

[leviw_travelin_and_unemployed, 2014-11-22 00:00:03]

Sigh... BoxClipper does a bit too much magic still and is bailing on properly
clipping canvases. I'm going to factor that magic out...

[commit-bot: I haz the power, 2014-11-22 00:17:36]

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

[commit-bot: I haz the power, 2014-11-22 00:17:37]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/3...)

[leviw_travelin_and_unemployed, 2014-11-24 23:00:30]

Alright... the tests pass this time, so it's just a question of whether you like
my patch!

I added a new HTMLCanvasClipper. I *almost* shoe-horned this into BoxClipper,
but decided against it. I kind of get the feeling we should make these clippers
dumber so they can be re-used instead of duplicated.

To do the above, I also moved the PaintPhase->ClipType mapping out of BoxClipper
and into a static function on DisplayItem. Let me know if you don't like any of
it.

[chrishtr, 2014-11-25 00:29:59]

https://codereview.chromium.org/744163002/diff/220001/Source/core/paint/HTMLC...
File Source/core/paint/HTMLCanvasClipper.cpp (right):

https://codereview.chromium.org/744163002/diff/220001/Source/core/paint/HTMLC...
Source/core/paint/HTMLCanvasClipper.cpp:17:
HTMLCanvasClipper::HTMLCanvasClipper(RenderHTMLCanvas& canvas, const PaintInfo&
paintInfo, const LayoutRect& clipRect)
Can this be easily unified with BoxClipper now that my BoxClipper refactor is
landed?

https://codereview.chromium.org/744163002/diff/220001/Source/core/paint/ViewD...
File Source/core/paint/ViewDisplayList.cpp (right):

https://codereview.chromium.org/744163002/diff/220001/Source/core/paint/ViewD...
Source/core/paint/ViewDisplayList.cpp:117: DisplayItem::Type
DisplayItem::paintPhaseToClipType(PaintPhase paintPhase)
IT's weird that this is in ViewDisplayList.cpp.

https://codereview.chromium.org/744163002/diff/220001/Source/core/paint/ViewD...
File Source/core/paint/ViewDisplayList.h (left):

https://codereview.chromium.org/744163002/diff/220001/Source/core/paint/ViewD...
Source/core/paint/ViewDisplayList.h:52: BeginTransparency = 30,
Let's get rid of the numbers that are not necessary here.

[leviw_travelin_and_unemployed, 2014-11-25 18:19:00]

ptal

[chrishtr, 2014-11-25 18:30:59]

https://codereview.chromium.org/744163002/diff/260001/Source/core/paint/ViewD...
File Source/core/paint/ViewDisplayList.cpp (right):

https://codereview.chromium.org/744163002/diff/260001/Source/core/paint/ViewD...
Source/core/paint/ViewDisplayList.cpp:117: DisplayItem::Type
DisplayItem::paintPhaseToClipType(PaintPhase paintPhase)
Please move to ClipRecorder.

[leviw_travelin_and_unemployed, 2014-11-25 18:38:23]

ptal

[chrishtr, 2014-11-25 18:59:28]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2014-11-25 18:59:29]

lgtm

[commit-bot: I haz the power, 2014-11-25 19:00:54]

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

[leviw_travelin_and_unemployed, 2014-11-25 19:29:37]

The CQ bit was checked by leviw@chromium.org

[commit-bot: I haz the power, 2014-11-25 19:29:46]

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

[commit-bot: I haz the power, 2014-11-25 20:07:54]

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

[commit-bot: I haz the power, 2014-11-25 20:07:56]

Try jobs failed on following builders:
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/32643)

[leviw_travelin_and_unemployed, 2014-11-25 21:21:27]

The CQ bit was checked by leviw@chromium.org

[commit-bot: I haz the power, 2014-11-25 21:21:58]

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

[commit-bot: I haz the power, 2014-11-25 21:35:48]

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

[commit-bot: I haz the power, 2014-11-25 21:35:50]

Try jobs failed on following builders:
  mac_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)

[leviw_travelin_and_unemployed, 2014-11-25 21:40:25]

The CQ bit was checked by leviw@chromium.org

[commit-bot: I haz the power, 2014-11-25 21:41:01]

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

[commit-bot: I haz the power, 2014-11-25 22:30:36]

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

[commit-bot: I haz the power, 2014-11-25 22:30:37]

Try jobs failed on following builders:
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/32663)

[leviw_travelin_and_unemployed, 2014-11-25 23:14:50]

The CQ bit was checked by leviw@chromium.org

[commit-bot: I haz the power, 2014-11-25 23:15:32]

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

[leviw_travelin_and_unemployed, 2014-11-25 23:17:07]

The CQ bit was unchecked by leviw@chromium.org

[leviw_travelin_and_unemployed, 2014-11-25 23:17:10]

The CQ bit was checked by leviw@chromium.org

[commit-bot: I haz the power, 2014-11-25 23:17:23]

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

[commit-bot: I haz the power, 2014-11-25 23:18:21]

Committed patchset #18 (id:340001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=185982

[pdr., 2014-11-26 03:49:20]

A revert of this CL (patchset #18 id:340001) has been created in
https://codereview.chromium.org/757183003/ by pdr@chromium.org.

The reason for reverting is: Boo, this broke transform-on-inline.html:
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=fas....

[chrishtr, 2014-11-26 17:20:59]

https://codereview.chromium.org/744163002/diff/340001/Source/core/paint/BoxPa...
File Source/core/paint/BoxPainter.cpp (right):

https://codereview.chromium.org/744163002/diff/340001/Source/core/paint/BoxPa...
Source/core/paint/BoxPainter.cpp:538: transparencyRecorder = adoptPtr(new
TransparencyRecorder(paintInfo.context, &m_renderBox,
DisplayItem::BeginTransparency, WebBlendModeNormal, 1));
On reflection, I think this was maybe wrong. The only reason to store separate
transparency display items is to separate pieces of painted state from the
current render object and its children, or if the transparency is a compositing
trigger. I don't think either is true in this case. Instead, we should allocate
a DrawingRecorder for the mask phase, in BoxPainter::paintMask(). I'm trying
this approach now.

https://codereview.chromium.org/744163002/diff/340001/Source/core/paint/HTMLC...
File Source/core/paint/HTMLCanvasPainter.cpp (left):

https://codereview.chromium.org/744163002/diff/340001/Source/core/paint/HTMLC...
Source/core/paint/HTMLCanvasPainter.cpp:25: if (clip) {
Same comment here. Shouldn't we just be allocating a DrawingRecorder at line 26
for all of the canvas in this phase?

[chrishtr, 2014-11-26 18:34:15]

https://codereview.chromium.org/744163002/diff/340001/Source/core/paint/HTMLC...
File Source/core/paint/HTMLCanvasPainter.cpp (left):

https://codereview.chromium.org/744163002/diff/340001/Source/core/paint/HTMLC...
Source/core/paint/HTMLCanvasPainter.cpp:25: if (clip) {
On 2014/11/26 at 17:20:59, chrishtr wrote:
> Same comment here. Shouldn't we just be allocating a DrawingRecorder at line
26 for all of the canvas in this phase?

Hmm. Maybe in this case, separating the clip is necessary, because the canvas
can have children that are clipped?

[leviw_travelin_and_unemployed, 2014-11-26 18:39:08]

https://codereview.chromium.org/744163002/diff/340001/Source/core/paint/HTMLC...
File Source/core/paint/HTMLCanvasPainter.cpp (left):

https://codereview.chromium.org/744163002/diff/340001/Source/core/paint/HTMLC...
Source/core/paint/HTMLCanvasPainter.cpp:25: if (clip) {
On 2014/11/26 18:34:15, chrishtr wrote:
> On 2014/11/26 at 17:20:59, chrishtr wrote:
> > Same comment here. Shouldn't we just be allocating a DrawingRecorder at line
> 26 for all of the canvas in this phase?
> 
> Hmm. Maybe in this case, separating the clip is necessary, because the canvas
> can have children that are clipped?

RenderReplaced and its ilk don't get child renderers, so maybe we don't need the
ClipRecorder.

[chrishtr, 2014-11-26 18:41:36]

On 2014/11/26 at 18:39:08, leviw wrote:
>
https://codereview.chromium.org/744163002/diff/340001/Source/core/paint/HTMLC...
> File Source/core/paint/HTMLCanvasPainter.cpp (left):
> 
>
https://codereview.chromium.org/744163002/diff/340001/Source/core/paint/HTMLC...
> Source/core/paint/HTMLCanvasPainter.cpp:25: if (clip) {
> On 2014/11/26 18:34:15, chrishtr wrote:
> > On 2014/11/26 at 17:20:59, chrishtr wrote:
> > > Same comment here. Shouldn't we just be allocating a DrawingRecorder at
line
> > 26 for all of the canvas in this phase?
> > 
> > Hmm. Maybe in this case, separating the clip is necessary, because the
canvas
> > can have children that are clipped?
> 
> RenderReplaced and its ilk don't get child renderers, so maybe we don't need
the ClipRecorder.

Great. Let's try that then.