Create a GraphicsContext from a DisplayList.

Create a GraphicsContext from a DisplayList.

And switch LinkHighlight over to using DisplayList.

The GraphicsContext change is needed for cases when we wish to record a DisplayList without
an existing GraphicsContext. The LinkHighlight case demonstrates that.

R=pdr@chromium.org,chrishtr@chromium.org,esprehn@chromium.org
BUG=410019
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=181835

[Stephen Chennney, 2014-09-04 21:19:53]

This depends on https://codereview.chromium.org/536573002/

For future use, such as LinkHighlight where we create the DisplayList in the
method that updates the geometry, outside of the painting code.\

Things like SVG-as-image and the SVGFEImage may be able to use this too, as the
stuff they render should not in theory depend on settings in the existing
context. Invalidation is simpler if you can update the list as soon as you know
it needs updating.

[Stephen Chennney, 2014-09-04 21:23:04]

CC test.

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

Can you do it along with the use case? That makes it easier to see why it makes
sense.

[Stephen Chennney, 2014-09-05 14:13:40]

schenney@chromium.org changed reviewers:
+ esprehn@chromium.org

[Stephen Chennney, 2014-09-05 14:16:23]

Updated patch. Need Source/web review for LinkHighlight.

[chrishtr, 2014-09-05 16:18:44]

The use case here is to create a GraphicsContext with no canvas, right? Because
all we're using
it for is to draw into an SkPicture inside of a DisplayList?

What about the approach of being able to pass 0 for the canvas to the existing
GraphicsContext constructor?

[esprehn, 2014-09-06 01:29:17]

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

https://codereview.chromium.org/541913002/diff/40001/Source/web/LinkHighlight...
Source/web/LinkHighlight.h:31: #include "platform/graphics/DisplayList.h"
Can we forward declare this instead?

[esprehn, 2014-09-06 05:18:11]

lgtm

[esprehn, 2014-09-06 05:18:27]

The CQ bit was checked by esprehn@chromium.org

[esprehn, 2014-09-06 05:18:31]

The CQ bit was unchecked by esprehn@chromium.org

[esprehn, 2014-09-06 05:18:31]

The CQ bit was unchecked by esprehn@chromium.org

[Stephen Chennney, 2014-09-08 17:42:09]

Chris, does this address your comment?

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

https://codereview.chromium.org/541913002/diff/40001/Source/web/LinkHighlight...
Source/web/LinkHighlight.h:31: #include "platform/graphics/DisplayList.h"
On 2014/09/06 01:29:17, esprehn wrote:
> Can we forward declare this instead?

Done.

[chrishtr, 2014-09-08 17:47:47]

On 2014/09/08 at 17:42:09, schenney wrote:
> Chris, does this address your comment?

My suggestion was to be able to pass 0 for the canvas arg to the constructor,
what about that option?

> 
>
https://codereview.chromium.org/541913002/diff/40001/Source/web/LinkHighlight.h
> File Source/web/LinkHighlight.h (right):
> 
>
https://codereview.chromium.org/541913002/diff/40001/Source/web/LinkHighlight...
> Source/web/LinkHighlight.h:31: #include "platform/graphics/DisplayList.h"
> On 2014/09/06 01:29:17, esprehn wrote:
> > Can we forward declare this instead?
> 
> Done.

[Stephen Chennney, 2014-09-08 17:49:19]

On 2014/09/08 17:47:47, chrishtr wrote:
> On 2014/09/08 at 17:42:09, schenney wrote:
> > Chris, does this address your comment?
> 
> My suggestion was to be able to pass 0 for the canvas arg to the constructor,
> what about that option?

We need the bounds and args to create the display list internally (i.e. the same
args we pass to beginRecording). So it's a completely different constructor
(which I like as there is less chance of an accidental mistake).

> 
> > 
> >
>
https://codereview.chromium.org/541913002/diff/40001/Source/web/LinkHighlight.h
> > File Source/web/LinkHighlight.h (right):
> > 
> >
>
https://codereview.chromium.org/541913002/diff/40001/Source/web/LinkHighlight...
> > Source/web/LinkHighlight.h:31: #include "platform/graphics/DisplayList.h"
> > On 2014/09/06 01:29:17, esprehn wrote:
> > > Can we forward declare this instead?
> > 
> > Done.

[chrishtr, 2014-09-08 17:56:13]

On 2014/09/08 at 17:49:19, schenney wrote:
> On 2014/09/08 17:47:47, chrishtr wrote:
> > On 2014/09/08 at 17:42:09, schenney wrote:
> > > Chris, does this address your comment?
> > 
> > My suggestion was to be able to pass 0 for the canvas arg to the
constructor,
> > what about that option?
> 
> We need the bounds and args to create the display list internally (i.e. the
same args we pass to beginRecording). So it's a completely different constructor
(which I like as there is less chance of an accidental mistake).

What I'm suggesting is to use the regular constructor, then have the
LinkHighlight code call beginRecording. This approach looks cleaner to me,
because it doesn't introduce a new code execution
path in GraphicsContext.

> 
> > 
> > > 
> > >
> >
https://codereview.chromium.org/541913002/diff/40001/Source/web/LinkHighlight.h
> > > File Source/web/LinkHighlight.h (right):
> > > 
> > >
> >
https://codereview.chromium.org/541913002/diff/40001/Source/web/LinkHighlight...
> > > Source/web/LinkHighlight.h:31: #include "platform/graphics/DisplayList.h"
> > > On 2014/09/06 01:29:17, esprehn wrote:
> > > > Can we forward declare this instead?
> > > 
> > > Done.

[Stephen Chennney, 2014-09-08 18:02:37]

On 2014/09/08 17:56:13, chrishtr wrote:
> On 2014/09/08 at 17:49:19, schenney wrote:
> > On 2014/09/08 17:47:47, chrishtr wrote:
> > > On 2014/09/08 at 17:42:09, schenney wrote:
> > > > Chris, does this address your comment?
> > > 
> > > My suggestion was to be able to pass 0 for the canvas arg to the
> constructor,
> > > what about that option?
> > 
> > We need the bounds and args to create the display list internally (i.e. the
> same args we pass to beginRecording). So it's a completely different
constructor
> (which I like as there is less chance of an accidental mistake).
> 
> What I'm suggesting is to use the regular constructor, then have the
> LinkHighlight code call beginRecording. This approach looks cleaner to me,
> because it doesn't introduce a new code execution
> path in GraphicsContext.

But then we need to add lots and lots of null canvas checks in GraphicsContext
(or lots of assertions). Every operation that touches a canvas would need a
release assert or if check.

> 
> > 
> > > 
> > > > 
> > > >
> > >
>
https://codereview.chromium.org/541913002/diff/40001/Source/web/LinkHighlight.h
> > > > File Source/web/LinkHighlight.h (right):
> > > > 
> > > >
> > >
>
https://codereview.chromium.org/541913002/diff/40001/Source/web/LinkHighlight...
> > > > Source/web/LinkHighlight.h:31: #include
"platform/graphics/DisplayList.h"
> > > > On 2014/09/06 01:29:17, esprehn wrote:
> > > > > Can we forward declare this instead?
> > > > 
> > > > Done.

[chrishtr, 2014-09-08 18:08:48]

Doesn't the same risk occur when creating a GraphicsContext in the way you're
proposing? You're just setting m_canvas to 0.

[Stephen Chennney, 2014-09-08 18:30:44]

On 2014/09/08 18:08:48, chrishtr wrote:
> Doesn't the same risk occur when creating a GraphicsContext in the way you're
> proposing? You're just setting m_canvas to 0.

I see your point. After endRecording the canvas will be null. So I guess it's
back to assertions.

I don't think we need RELEASE_ASSERT here, as null pointer refs are not security
holes.

[Stephen Chennney, 2014-09-08 20:01:08]

I'll fix these too.

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?

[chrishtr, 2014-09-08 20:25:53]

Ok ping me when it's ready for re-review.

[Stephen Chennney, 2014-09-10 17:31:47]

New version now with null canvas support.

[chrishtr, 2014-09-10 20:21:45]

lgtm

[Stephen Chennney, 2014-09-11 16:10:00]

The CQ bit was checked by schenney@chromium.org

[commit-bot: I haz the power, 2014-09-11 16:10:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/541913002/120001

[commit-bot: I haz the power, 2014-09-11 16:13:52]

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

[commit-bot: I haz the power, 2014-09-11 16:13:53]

Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_chromium_gn_rel/bu...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/54704)

[Stephen Chennney, 2014-09-11 16:52:56]

The CQ bit was checked by schenney@chromium.org

[commit-bot: I haz the power, 2014-09-11 16:53:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/541913002/140001

[commit-bot: I haz the power, 2014-09-11 18:14:20]

Committed patchset #8 (id:140001) as 181835