Modifications to DisplayList for better Slimming Paint

Modifications to DisplayList for better Slimming Paint

Add clipping.
Add transform.
Change recording to be driven by GraphicsContext.
Update dependent code.
Move a method used only by SVGFEImage from AffineTransform to SVGFEImage

R=pdr,senorblanco
BUG=410019
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=181502

[Stephen Chennney, 2014-09-02 21:16:54]

A bunch of changes brought in from the experimental branch. These are needed to
enable the RenderImage and LinkHighlight caching.

[Stephen Chennney, 2014-09-02 21:17:20]

schenney@chromium.org changed reviewers:
+ senorblanco@chromium.org
- senorblanco@google.com

[chrishtr, 2014-09-02 22:05:56]

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

[chrishtr, 2014-09-02 22:05:58]

https://codereview.chromium.org/536573002/diff/1/Source/core/rendering/svg/Re...
File Source/core/rendering/svg/RenderSVGResourceFilter.cpp (right):

https://codereview.chromium.org/536573002/diff/1/Source/core/rendering/svg/Re...
Source/core/rendering/svg/RenderSVGResourceFilter.cpp:147: SourceGraphic*
sourceGraphic =
static_cast<SourceGraphic*>(filterData->builder->getEffectById(SourceGraphic::effectName()));
Typo in variable name.

https://codereview.chromium.org/536573002/diff/1/Source/core/rendering/svg/Re...
Source/core/rendering/svg/RenderSVGResourceFilter.cpp:149: RefPtr<DisplayList>
sourceGraphcisDisplayList = DisplayList::create(filterData->boundaries);
Typo in variable name.

https://codereview.chromium.org/536573002/diff/1/Source/core/svg/graphics/fil...
File Source/core/svg/graphics/filters/SVGFEImage.cpp (right):

https://codereview.chromium.org/536573002/diff/1/Source/core/svg/graphics/fil...
Source/core/svg/graphics/filters/SVGFEImage.cpp:75: AffineTransform
makeMapBetweenRects(const FloatRect& source, const FloatRect& dest)
Is this function called anywhere?

https://codereview.chromium.org/536573002/diff/1/Source/platform/graphics/Dis...
File Source/platform/graphics/DisplayList.h (right):

https://codereview.chromium.org/536573002/diff/1/Source/platform/graphics/Dis...
Source/platform/graphics/DisplayList.h:78: void setClip(const IntRect& rect) {
m_clip = rect; }
Can you sketch the use cases for clip and transform?

I'd ideally like to see these methods added only along with code that itself
provides a use case.

https://codereview.chromium.org/536573002/diff/1/Source/platform/graphics/Gra...
File Source/platform/graphics/GraphicsContext.cpp (right):

https://codereview.chromium.org/536573002/diff/1/Source/platform/graphics/Gra...
Source/platform/graphics/GraphicsContext.cpp:502: pDisplayList->clearPicture();
What's the use case of beginning a recording of a display list that is not
created from scratch just before?
Are you just changing this around to make clear the ownership of the pointer?

If it's always from scratch, we can change beginRecording() to return a fresh
PasRefPtr<DisplayList> that the caller can keep.
This simplifies the DisplayList API.

[Stephen Chennney, 2014-09-03 14:19:38]

Addressed comments. New patch real soon.

https://codereview.chromium.org/536573002/diff/1/Source/core/rendering/svg/Re...
File Source/core/rendering/svg/RenderSVGResourceFilter.cpp (right):

https://codereview.chromium.org/536573002/diff/1/Source/core/rendering/svg/Re...
Source/core/rendering/svg/RenderSVGResourceFilter.cpp:147: SourceGraphic*
sourceGraphic =
static_cast<SourceGraphic*>(filterData->builder->getEffectById(SourceGraphic::effectName()));
On 2014/09/02 22:05:58, chrishtr wrote:
> Typo in variable name.

Not here, right?

https://codereview.chromium.org/536573002/diff/1/Source/core/rendering/svg/Re...
Source/core/rendering/svg/RenderSVGResourceFilter.cpp:149: RefPtr<DisplayList>
sourceGraphcisDisplayList = DisplayList::create(filterData->boundaries);
On 2014/09/02 22:05:57, chrishtr wrote:
> Typo in variable name.
> 

Done.

https://codereview.chromium.org/536573002/diff/1/Source/core/svg/graphics/fil...
File Source/core/svg/graphics/filters/SVGFEImage.cpp (right):

https://codereview.chromium.org/536573002/diff/1/Source/core/svg/graphics/fil...
Source/core/svg/graphics/filters/SVGFEImage.cpp:75: AffineTransform
makeMapBetweenRects(const FloatRect& source, const FloatRect& dest)
On 2014/09/02 22:05:58, chrishtr wrote:
> Is this function called anywhere?

Yep, 3 times in this file only. I wanted to replace it with the corresponding
SkMatrix method it because it does not really do what the name implies, but it
turns out it's hard to replicate due to some stupidity in
AffineTransform::scale. So I moved it here to prevent other code from pulling it
in.

https://codereview.chromium.org/536573002/diff/1/Source/platform/graphics/Dis...
File Source/platform/graphics/DisplayList.h (right):

https://codereview.chromium.org/536573002/diff/1/Source/platform/graphics/Dis...
Source/platform/graphics/DisplayList.h:78: void setClip(const IntRect& rect) {
m_clip = rect; }
On 2014/09/02 22:05:58, chrishtr wrote:
> Can you sketch the use cases for clip and transform?
> 
> I'd ideally like to see these methods added only along with code that itself
> provides a use case.

In 2 of the 3 cases I converted (LinkHighlight and RenderImage) a clip is
applied immediately to restrict drawing to the content area. 

And all of them need a transform because the paintOffset argument is ubiquitous
to paint methods and always needs to be applied.

Furthermore, Florin and I talked about the problem of balancing clips and
transforms and we both agreed that it is better to keep these out of the
SkPictures that are cached, so instead we put them on the DisplayList.

https://codereview.chromium.org/536573002/diff/1/Source/platform/graphics/Gra...
File Source/platform/graphics/GraphicsContext.cpp (right):

https://codereview.chromium.org/536573002/diff/1/Source/platform/graphics/Gra...
Source/platform/graphics/GraphicsContext.cpp:502: pDisplayList->clearPicture();
On 2014/09/02 22:05:58, chrishtr wrote:
> What's the use case of beginning a recording of a display list that is not
> created from scratch just before?

If the display list is invalidated we should be able to just re-record it
without creating another. Now that I think about it I don't think we take
advantage of this. I also want to clear the picture to maintain the invariant
stated in DisplayList: that the picture will be null when recording.

> Are you just changing this around to make clear the ownership of the pointer?

And that too.

> 
> If it's always from scratch, we can change beginRecording() to return a fresh
> PasRefPtr<DisplayList> that the caller can keep.
> This simplifies the DisplayList API.

[chrishtr, 2014-09-03 16:56:12]

https://codereview.chromium.org/536573002/diff/1/Source/core/rendering/svg/Re...
File Source/core/rendering/svg/RenderSVGResourceFilter.cpp (right):

https://codereview.chromium.org/536573002/diff/1/Source/core/rendering/svg/Re...
Source/core/rendering/svg/RenderSVGResourceFilter.cpp:147: SourceGraphic*
sourceGraphic =
static_cast<SourceGraphic*>(filterData->builder->getEffectById(SourceGraphic::effectName()));
On 2014/09/03 14:19:37, Stephen Chenney wrote:
> On 2014/09/02 22:05:58, chrishtr wrote:
> > Typo in variable name.
> 
> Not here, right?

Acknowledged.

https://codereview.chromium.org/536573002/diff/1/Source/platform/graphics/Gra...
File Source/platform/graphics/GraphicsContext.cpp (right):

https://codereview.chromium.org/536573002/diff/1/Source/platform/graphics/Gra...
Source/platform/graphics/GraphicsContext.cpp:502: pDisplayList->clearPicture();
On 2014/09/03 14:19:37, Stephen Chenney wrote:
> On 2014/09/02 22:05:58, chrishtr wrote:
> > What's the use case of beginning a recording of a display list that is not
> > created from scratch just before?
> 
> If the display list is invalidated we should be able to just re-record it
> without creating another.

What's the performance advantage of this though? There is a code correctness
downside risk to reusing.

> Now that I think about it I don't think we take
> advantage of this. I also want to clear the picture to maintain the invariant
> stated in DisplayList: that the picture will be null when recording.

If we don't take advantage of it, then beginRecording could return a new display
list rather than
using an existing one.

> 
> > Are you just changing this around to make clear the ownership of the
pointer?
> 
> And that too.
> 
> > 
> > If it's always from scratch, we can change beginRecording() to return a
fresh
> > PasRefPtr<DisplayList> that the caller can keep.
> > This simplifies the DisplayList API.
>

[chrishtr, 2014-09-03 17:02:24]

https://codereview.chromium.org/536573002/diff/1/Source/platform/graphics/Dis...
File Source/platform/graphics/DisplayList.h (right):

https://codereview.chromium.org/536573002/diff/1/Source/platform/graphics/Dis...
Source/platform/graphics/DisplayList.h:78: void setClip(const IntRect& rect) {
m_clip = rect; }
On 2014/09/03 14:19:37, Stephen Chenney wrote:
> On 2014/09/02 22:05:58, chrishtr wrote:
> > Can you sketch the use cases for clip and transform?
> > 
> > I'd ideally like to see these methods added only along with code that itself
> > provides a use case.
> 
> In 2 of the 3 cases I converted (LinkHighlight and RenderImage) a clip is
> applied immediately to restrict drawing to the content area. 
> 
> And all of them need a transform because the paintOffset argument is
ubiquitous
> to paint methods and always needs to be applied.
> 
> Furthermore, Florin and I talked about the problem of balancing clips and
> transforms and we both agreed that it is better to keep these out of the
> SkPictures that are cached, so instead we put them on the DisplayList.

Setting clip and transform externally to DisplayList is consistent
with the current plans.

[chrishtr, 2014-09-03 17:03:02]

https://codereview.chromium.org/536573002/diff/1/Source/platform/graphics/Dis...
File Source/platform/graphics/DisplayList.h (right):

https://codereview.chromium.org/536573002/diff/1/Source/platform/graphics/Dis...
Source/platform/graphics/DisplayList.h:78: void setClip(const IntRect& rect) {
m_clip = rect; }
On 2014/09/03 17:02:24, chrishtr wrote:
> On 2014/09/03 14:19:37, Stephen Chenney wrote:
> > On 2014/09/02 22:05:58, chrishtr wrote:
> > > Can you sketch the use cases for clip and transform?
> > > 
> > > I'd ideally like to see these methods added only along with code that
itself
> > > provides a use case.
> > 
> > In 2 of the 3 cases I converted (LinkHighlight and RenderImage) a clip is
> > applied immediately to restrict drawing to the content area. 
> > 
> > And all of them need a transform because the paintOffset argument is
> ubiquitous
> > to paint methods and always needs to be applied.
> > 
> > Furthermore, Florin and I talked about the problem of balancing clips and
> > transforms and we both agreed that it is better to keep these out of the
> > SkPictures that are cached, so instead we put them on the DisplayList.
> 
> Setting clip and transform externally to DisplayList is consistent
> with the current plans.
> 

Sent that comment too soon. Meant to say: therefore ok.

[Stephen Chennney, 2014-09-03 20:41:18]

So the only open issue is the re-use of DisplayLists for re-recording, right?

I actually copied the clearPicture at the start of recording from the existing
implementation. I'd like to leave that there.

Regarding the usefulness of re-use in general, creation of the DisplayList
outside of the GraphicsContext also allows a GraphicsContext constructor of this
form:

GraphicsContext(PassRefPtr<DisplayList>, DisabledMode);

that can be used to create a recording GraphicsContext that starts recoridng
immediately. I used that in LinkHighlight and we may want it elsewhere, such as
the filter code. I think I needed it in Linkhighlight because we create the
display list in that case outside of the paint method.

[Stephen Chennney, 2014-09-03 20:42:05]

On 2014/09/03 20:41:18, Stephen Chenney wrote:
> So the only open issue is the re-use of DisplayLists for re-recording, right?
> 
> I actually copied the clearPicture at the start of recording from the existing
> implementation. I'd like to leave that there.
> 
> Regarding the usefulness of re-use in general, creation of the DisplayList
> outside of the GraphicsContext also allows a GraphicsContext constructor of
this
> form:
> 
> GraphicsContext(PassRefPtr<DisplayList>, DisabledMode);
> 
> that can be used to create a recording GraphicsContext that starts recoridng
> immediately. I used that in LinkHighlight and we may want it elsewhere, such
as
> the filter code. I think I needed it in Linkhighlight because we create the
> display list in that case outside of the paint method.

I know, it allows you to create a GraphicsContext without an existing canvas.

[chrishtr, 2014-09-03 20:59:54]

On 2014/09/03 at 20:42:05, schenney wrote:
> On 2014/09/03 20:41:18, Stephen Chenney wrote:
> > So the only open issue is the re-use of DisplayLists for re-recording,
right?
> > 
> > I actually copied the clearPicture at the start of recording from the
existing
> > implementation. I'd like to leave that there.
> > 
> > Regarding the usefulness of re-use in general, creation of the DisplayList
> > outside of the GraphicsContext also allows a GraphicsContext constructor of
this
> > form:
> > 
> > GraphicsContext(PassRefPtr<DisplayList>, DisabledMode);
> > 
> > that can be used to create a recording GraphicsContext that starts recoridng
> > immediately. I used that in LinkHighlight and we may want it elsewhere, such
as
> > the filter code. I think I needed it in Linkhighlight because we create the
> > display list in that case outside of the paint method.
> 
> I know, it allows you to create a GraphicsContext without an existing canvas.

Is that necessary? In other words, are all of the use cases just caching a
DisplayList that was going to be recorded anyway? Simpler APIs are better,
especially if they keep us from shooting ourselves
in the foot. We can always generalize later if we have compelling use cases.

[Stephen Chennney, 2014-09-04 19:05:29]

On 2014/09/03 20:59:54, chrishtr wrote:
> On 2014/09/03 at 20:42:05, schenney wrote:
> > On 2014/09/03 20:41:18, Stephen Chenney wrote:
> > > So the only open issue is the re-use of DisplayLists for re-recording,
> right?
> > > 
> > > I actually copied the clearPicture at the start of recording from the
> existing
> > > implementation. I'd like to leave that there.
> > > 
> > > Regarding the usefulness of re-use in general, creation of the DisplayList
> > > outside of the GraphicsContext also allows a GraphicsContext constructor
of
> this
> > > form:
> > > 
> > > GraphicsContext(PassRefPtr<DisplayList>, DisabledMode);
> > > 
> > > that can be used to create a recording GraphicsContext that starts
recoridng
> > > immediately. I used that in LinkHighlight and we may want it elsewhere,
such
> as
> > > the filter code. I think I needed it in Linkhighlight because we create
the
> > > display list in that case outside of the paint method.
> > 
> > I know, it allows you to create a GraphicsContext without an existing
canvas.
> 
> Is that necessary? In other words, are all of the use cases just caching a
> DisplayList that was going to be recorded anyway? Simpler APIs are better,
> especially if they keep us from shooting ourselves
> in the foot. We can always generalize later if we have compelling use cases.

There are undoubtedly cases where we would like to create a DisplayList for
subsequent rendering without having a canvas on hand to create a
GraphicsContext, but we would still want to record into it. So the answer to
your questions are no and yes.

We might even want to update DisplayLists during layout, or in a distinct pass
that is separate from the compositor-driven paint calls we have today (the
compositor creates the canvas and passes it to Blink, because it knows all the
sizing information for the canvas). In such cases we have no canvas and we don't
want to allocate one. I believe I can re-implement my solution to that problem
while providing the API you're suggesting. So I'll go ahead and change things.

As a use case, consider LinkHighlight. The display list is invalidated when the
highlight geometry is invalidated, and rather than setting some flag to indicate
that the display list needs to be regenerated during the paint method, it was
much simpler to just regenerate directly when the new highlight geometry is
calculated. But we do record the list then. We just use it later.

[Stephen Chennney, 2014-09-04 20:43:49]

OK, I think Chris was right all along.

[chrishtr, 2014-09-05 01:15:34]

https://codereview.chromium.org/536573002/diff/40001/Source/platform/graphics...
File Source/platform/graphics/DisplayList.h (right):

https://codereview.chromium.org/536573002/diff/40001/Source/platform/graphics...
Source/platform/graphics/DisplayList.h:64: void setPicture(SkPicture* picture) {
m_picture = adoptRef(picture); }
This one is intended to be used only inside of GraphicsContext?

https://codereview.chromium.org/536573002/diff/40001/Source/platform/graphics...
Source/platform/graphics/DisplayList.h:65: void clearPicture() {
m_picture.clear(); }
Not used - remove?

https://codereview.chromium.org/536573002/diff/40001/Source/platform/graphics...
File Source/platform/graphics/GraphicsContext.h (right):

https://codereview.chromium.org/536573002/diff/40001/Source/platform/graphics...
Source/platform/graphics/GraphicsContext.h:346: void endRecording();
after endRecording() is called, the GraphicsContext promises to treat the
display list as a const object right? That deserves a comment.
How about adding a field on DisplayList such as displayList.lock() to enforce
that? Hmm..

[Stephen Chennney, 2014-09-05 13:50:50]

https://codereview.chromium.org/536573002/diff/40001/Source/platform/graphics...
File Source/platform/graphics/DisplayList.h (right):

https://codereview.chromium.org/536573002/diff/40001/Source/platform/graphics...
Source/platform/graphics/DisplayList.h:64: void setPicture(SkPicture* picture) {
m_picture = adoptRef(picture); }
On 2014/09/05 01:15:33, chrishtr wrote:
> This one is intended to be used only inside of GraphicsContext?

Yes, but I see no nice way of enforcing that. In reality it's not an invariant
of any kind either, so I don't see the harm in leaving it as is.

https://codereview.chromium.org/536573002/diff/40001/Source/platform/graphics...
Source/platform/graphics/DisplayList.h:65: void clearPicture() {
m_picture.clear(); }
On 2014/09/05 01:15:33, chrishtr wrote:
> Not used - remove?

Done.

https://codereview.chromium.org/536573002/diff/40001/Source/platform/graphics...
File Source/platform/graphics/GraphicsContext.h (right):

https://codereview.chromium.org/536573002/diff/40001/Source/platform/graphics...
Source/platform/graphics/GraphicsContext.h:346: void endRecording();
On 2014/09/05 01:15:34, chrishtr wrote:
> after endRecording() is called, the GraphicsContext promises to treat the
> display list as a const object right? That deserves a comment.
> How about adding a field on DisplayList such as displayList.lock() to enforce
> that? Hmm..

We can just assert that there is no picture before we start recording with the
list. If a picture exists, it's locked. I would put that in the follow on patch.

[chrishtr, 2014-09-05 15:47:32]

Since a DisplayList is only created when recording into it, how about we go back
to returning the DisplayList from endRecording()? Then we can get rid of
setPicture(). This would also I think remove the need for some of the other
changes you made for this CL, and the meat of the CL would be to add clip and
transform settings outside of SkPicture in the DisplayList.

[Stephen Chennney, 2014-09-05 17:55:03]

On 2014/09/05 15:47:32, chrishtr wrote:
> Since a DisplayList is only created when recording into it, how about we go
back
> to returning the DisplayList from endRecording()? Then we can get rid of
> setPicture(). This would also I think remove the need for some of the other
> changes you made for this CL, and the meat of the CL would be to add clip and
> transform settings outside of SkPicture in the DisplayList.

This moves the DisplayList return to endRecording.

We leave most other things in place, because we do not want every DisplayList
holding a recorder object, and we still have better DisplayList creation for ref
counting.

[chrishtr, 2014-09-05 18:28:13]

https://codereview.chromium.org/536573002/diff/80001/Source/platform/graphics...
File Source/platform/graphics/GraphicsContext.cpp (right):

https://codereview.chromium.org/536573002/diff/80001/Source/platform/graphics...
Source/platform/graphics/GraphicsContext.cpp:531:
recording.m_displayList->setPicture(recording.m_recorder->endRecording());
Why not create m_displayList here?

https://codereview.chromium.org/536573002/diff/80001/Source/platform/graphics...
Source/platform/graphics/GraphicsContext.cpp:534: delete recording.m_recorder;
Put m_recorder in an OwnPtr in the recording state?

https://codereview.chromium.org/536573002/diff/80001/Source/platform/graphics...
File Source/platform/graphics/GraphicsContext.h (right):

https://codereview.chromium.org/536573002/diff/80001/Source/platform/graphics...
Source/platform/graphics/GraphicsContext.h:313: void clipOut(const Path&);
Why is this public?

https://codereview.chromium.org/536573002/diff/80001/Source/platform/graphics...
Source/platform/graphics/GraphicsContext.h:321: void canvasClip(const Path&,
WindRule = RULE_EVENODD);
Why is this public?

https://codereview.chromium.org/536573002/diff/80001/Source/platform/graphics...
File Source/platform/graphics/GraphicsContextTest.cpp (right):

https://codereview.chromium.org/536573002/diff/80001/Source/platform/graphics...
Source/platform/graphics/GraphicsContextTest.cpp:1156: 
Add a test with clips or transforms?

[chrishtr, 2014-09-05 18:28:51]

https://codereview.chromium.org/536573002/diff/80001/Source/platform/graphics...
File Source/platform/graphics/GraphicsContext.cpp (right):

https://codereview.chromium.org/536573002/diff/80001/Source/platform/graphics...
Source/platform/graphics/GraphicsContext.cpp:531:
recording.m_displayList->setPicture(recording.m_recorder->endRecording());
On 2014/09/05 18:28:13, chrishtr wrote:
> Why not create m_displayList here?

Sorry, that is a stale comment.

[Stephen Chennney, 2014-09-05 18:36:19]

https://codereview.chromium.org/536573002/diff/80001/Source/platform/graphics...
File Source/platform/graphics/GraphicsContext.cpp (right):

https://codereview.chromium.org/536573002/diff/80001/Source/platform/graphics...
Source/platform/graphics/GraphicsContext.cpp:534: delete recording.m_recorder;
On 2014/09/05 18:28:13, chrishtr wrote:
> Put m_recorder in an OwnPtr in the recording state?

Tried that. SkPictureRecorder is a Skia type that does no play with OwnPtr.

https://codereview.chromium.org/536573002/diff/80001/Source/platform/graphics...
File Source/platform/graphics/GraphicsContext.h (right):

https://codereview.chromium.org/536573002/diff/80001/Source/platform/graphics...
Source/platform/graphics/GraphicsContext.h:321: void canvasClip(const Path&,
WindRule = RULE_EVENODD);
On 2014/09/05 18:28:13, chrishtr wrote:
> Why is this public?

This, and clipOut, are used by external code. I just moved it up to be with the
other clipping methods. Part of the ongoing effort to clean up this header.

https://codereview.chromium.org/536573002/diff/80001/Source/platform/graphics...
File Source/platform/graphics/GraphicsContextTest.cpp (right):

https://codereview.chromium.org/536573002/diff/80001/Source/platform/graphics...
Source/platform/graphics/GraphicsContextTest.cpp:1156: 
On 2014/09/05 18:28:13, chrishtr wrote:
> Add a test with clips or transforms?

Can do, but not trivial because you can't inspect the contents of an SkPicture
or canvas. We need to generate a canvas, paint something, then examine pixels.
Tests for that do exist so I can copy the basic approach, but not quickly.

I should say, non-trivial to do a meaningful test. It's easy to test the getters
and setters but I would rather do it all in one go.

OK if I punt on it for now? I'll think some more about testing next week. It's
an open issue with the Skia team.

[chrishtr, 2014-09-05 18:40:58]

lgtm

https://codereview.chromium.org/536573002/diff/80001/Source/platform/graphics...
File Source/platform/graphics/GraphicsContext.h (right):

https://codereview.chromium.org/536573002/diff/80001/Source/platform/graphics...
Source/platform/graphics/GraphicsContext.h:321: void canvasClip(const Path&,
WindRule = RULE_EVENODD);
On 2014/09/05 18:36:19, Stephen Chenney wrote:
> On 2014/09/05 18:28:13, chrishtr wrote:
> > Why is this public?
> 
> This, and clipOut, are used by external code. I just moved it up to be with
the
> other clipping methods. Part of the ongoing effort to clean up this header.

Ok, my mistake then.

https://codereview.chromium.org/536573002/diff/80001/Source/platform/graphics...
File Source/platform/graphics/GraphicsContextTest.cpp (right):

https://codereview.chromium.org/536573002/diff/80001/Source/platform/graphics...
Source/platform/graphics/GraphicsContextTest.cpp:1156: 
On 2014/09/05 18:36:19, Stephen Chenney wrote:
> On 2014/09/05 18:28:13, chrishtr wrote:
> > Add a test with clips or transforms?
> 
> Can do, but not trivial because you can't inspect the contents of an SkPicture
> or canvas. We need to generate a canvas, paint something, then examine pixels.
> Tests for that do exist so I can copy the basic approach, but not quickly.
> 
> I should say, non-trivial to do a meaningful test. It's easy to test the
getters
> and setters but I would rather do it all in one go.
> 
> OK if I punt on it for now? I'll think some more about testing next week. It's
> an open issue with the Skia team.

Ok. For now we can test that clips & transforms are correct in layout tests.
Please add a FIXME though.

[Stephen Chennney, 2014-09-05 18:46:25]

The CQ bit was checked by schenney@chromium.org

[commit-bot: I haz the power, 2014-09-05 18:46:49]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/schenney@chromium.org/536573002/120001

[commit-bot: I haz the power, 2014-09-05 23:12:02]

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

[commit-bot: I haz the power, 2014-09-05 23:12:03]

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

[Stephen Chennney, 2014-09-05 23:15:47]

The CQ bit was checked by schenney@chromium.org

[commit-bot: I haz the power, 2014-09-05 23:17:10]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/schenney@chromium.org/536573002/120001

[commit-bot: I haz the power, 2014-09-06 06:47:41]

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

[commit-bot: I haz the power, 2014-09-06 06:47:42]

Exceeded time limit waiting for builds to trigger.

[Stephen Chennney, 2014-09-06 17:54:10]

The CQ bit was checked by schenney@chromium.org

[commit-bot: I haz the power, 2014-09-06 17:55:06]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/schenney@chromium.org/536573002/120001

[commit-bot: I haz the power, 2014-09-07 08:14:16]

Committed patchset #7 (id:120001) as 181502

[Stephen White, 2014-09-08 19:51:01]

Vacation-delayed comments.

https://codereview.chromium.org/536573002/diff/120001/Source/core/rendering/s...
File Source/core/rendering/svg/RenderSVGResourceFilter.cpp (right):

https://codereview.chromium.org/536573002/diff/120001/Source/core/rendering/s...
Source/core/rendering/svg/RenderSVGResourceFilter.cpp:147: 
Nit: unnecessary whitespace change.

https://codereview.chromium.org/536573002/diff/120001/Source/platform/graphic...
File Source/platform/graphics/DisplayList.h (right):

https://codereview.chromium.org/536573002/diff/120001/Source/platform/graphic...
Source/platform/graphics/DisplayList.h:60: // midst of recording (i.e., between
a beginRecording/endRecording pair)
beginRecording()/endRecording() seem to be gone; should this comment be modified
also?

[Stephen Chennney, 2014-09-08 20:00:32]

On 2014/09/08 19:51:01, Stephen White wrote:
> Vacation-delayed comments.
> 
>
https://codereview.chromium.org/536573002/diff/120001/Source/core/rendering/s...
> File Source/core/rendering/svg/RenderSVGResourceFilter.cpp (right):
> 
>
https://codereview.chromium.org/536573002/diff/120001/Source/core/rendering/s...
> Source/core/rendering/svg/RenderSVGResourceFilter.cpp:147: 
> Nit: unnecessary whitespace change.
> 
>
https://codereview.chromium.org/536573002/diff/120001/Source/platform/graphic...
> File Source/platform/graphics/DisplayList.h (right):
> 
>
https://codereview.chromium.org/536573002/diff/120001/Source/platform/graphic...
> Source/platform/graphics/DisplayList.h:60: // midst of recording (i.e.,
between
> a beginRecording/endRecording pair)
> beginRecording()/endRecording() seem to be gone; should this comment be
modified
> also?

Careful reviewing! I'll fix these and clean them up in the follow-on patches.