Use SkLayerDrawLooper::Builder to construct SkLayerDrawLooper.

Use SkLayerDrawLooper::Builder to construct SkLayerDrawLooper.

The addLayer() methods of SkLayerDrawLooper will soon be removed. This CL
updates all uses of those methods with the corresponding methods in
SkLayerDrawLooper::Builder.

BUG=skia:2141
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=168041

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

[Dominik Grewe, 2014-02-24 12:41:36]

PTAL.
Wasn't sure whether to rename 'DrawLooper' to 'DrawLooperBuilder'. I left it for
now but I'm happy to change it. Wdyt?

[jbroman, 2014-02-24 15:23:32]

Unsolicited (non-owner) comments; hope you don't mind.

https://codereview.chromium.org/177473003/diff/1/Source/platform/graphics/Dra...
File Source/platform/graphics/DrawLooper.h (right):

https://codereview.chromium.org/177473003/diff/1/Source/platform/graphics/Dra...
Source/platform/graphics/DrawLooper.h:66: SkDrawLooper* skDrawLooper();
This method now destroys the WebCore::DrawLooper as well, and it's not at all
obvious that it has this effect. If, for example, a callee tried to keep a
DrawLooper instance around, this would not behave as one would expect.

I like that Skia is making draw loopers immutable, but I think the
platform/graphics/ API should make this clear. My suggestion would be to mirror
the change in the Skia API, and call this class "DrawLooperBuilder" or similar,
and rename this something that makes it obvious that it resets the builder, like
the method SkLayerDrawLooper::Builder has.

It could either return an SkDrawLooper* or a new WebCore::DrawLooper type (which
would be a simple wrapper if we don't want to expose Skia types to core/).

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

https://codereview.chromium.org/177473003/diff/1/Source/platform/graphics/Gra...
Source/platform/graphics/GraphicsContext.h:323: void setDrawLooper(DrawLooper&);
Passing a non-const reference here seems odd. Especially now that the underlying
draw loopers are becoming immutable, it would be nice to pass something
immutable here too.

[Stephen Chennney, 2014-02-24 15:46:40]

As jbroman has said, I think there are some significant issues here with
const-ness and ownership that need to be explained.

[Dominik Grewe, 2014-02-24 15:47:28]

https://codereview.chromium.org/177473003/diff/1/Source/platform/graphics/Dra...
File Source/platform/graphics/DrawLooper.h (right):

https://codereview.chromium.org/177473003/diff/1/Source/platform/graphics/Dra...
Source/platform/graphics/DrawLooper.h:66: SkDrawLooper* skDrawLooper();
On 2014/02/24 15:23:33, jbroman wrote:
> This method now destroys the WebCore::DrawLooper as well, and it's not at all
> obvious that it has this effect. If, for example, a callee tried to keep a
> DrawLooper instance around, this would not behave as one would expect.
> 
> I like that Skia is making draw loopers immutable, but I think the
> platform/graphics/ API should make this clear. My suggestion would be to
mirror
> the change in the Skia API, and call this class "DrawLooperBuilder" or
similar,
> and rename this something that makes it obvious that it resets the builder,
like
> the method SkLayerDrawLooper::Builder has.
> 
> It could either return an SkDrawLooper* or a new WebCore::DrawLooper type
(which
> would be a simple wrapper if we don't want to expose Skia types to core/).

Makes sense. How about replicating the structure in Skia, i.e. keep the
'DrawLooper' class but move all the add* methods to an inner 'Builder' class
with a 'detachLooper()' method?

[Stephen Chennney, 2014-02-24 15:53:32]

On 2014/02/24 15:47:28, Dominik Grewe wrote:
>
https://codereview.chromium.org/177473003/diff/1/Source/platform/graphics/Dra...
> File Source/platform/graphics/DrawLooper.h (right):
> 
>
https://codereview.chromium.org/177473003/diff/1/Source/platform/graphics/Dra...
> Source/platform/graphics/DrawLooper.h:66: SkDrawLooper* skDrawLooper();
> On 2014/02/24 15:23:33, jbroman wrote:
> > This method now destroys the WebCore::DrawLooper as well, and it's not at
all
> > obvious that it has this effect. If, for example, a callee tried to keep a
> > DrawLooper instance around, this would not behave as one would expect.
> > 
> > I like that Skia is making draw loopers immutable, but I think the
> > platform/graphics/ API should make this clear. My suggestion would be to
> mirror
> > the change in the Skia API, and call this class "DrawLooperBuilder" or
> similar,
> > and rename this something that makes it obvious that it resets the builder,
> like
> > the method SkLayerDrawLooper::Builder has.
> > 
> > It could either return an SkDrawLooper* or a new WebCore::DrawLooper type
> (which
> > would be a simple wrapper if we don't want to expose Skia types to core/).
> 
> Makes sense. How about replicating the structure in Skia, i.e. keep the
> 'DrawLooper' class but move all the add* methods to an inner 'Builder' class
> with a 'detachLooper()' method?

I think that's a good idea. It will make it easier for those familiar with Skia
to figure out what's happening.

[Dominik Grewe, 2014-02-24 17:16:03]

I've added a Builder class to DrawLooper. I also made DrawLooper ref-counted so
we don't have to worry about explicitly destroying the object returned from
detachDrawLooper(). I'm not very familiar with Blink's ref counting so please
take a look.

[jbroman, 2014-02-24 17:47:44]

I'm liking this much better.

https://codereview.chromium.org/177473003/diff/120001/Source/core/rendering/E...
File Source/core/rendering/EllipsisBox.cpp (right):

https://codereview.chromium.org/177473003/diff/120001/Source/core/rendering/E...
Source/core/rendering/EllipsisBox.cpp:80:
context->setDrawLooper(*drawLooperBuilder.detachDrawLooper().get());
This seems awkward. If nothing else, you can dereference a PassRefPtr directly,
so the .get() is unnecessary.

I think my preference would be something like:

OwnPtr<DrawLooper> drawLooper = drawLooperBuilder.detachDrawLooper();
context->setDrawLooper(*drawLooper);

https://codereview.chromium.org/177473003/diff/120001/Source/platform/graphic...
File Source/platform/graphics/DrawLooper.cpp (right):

https://codereview.chromium.org/177473003/diff/120001/Source/platform/graphic...
Source/platform/graphics/DrawLooper.cpp:45: DrawLooper::DrawLooper(SkDrawLooper*
looper) : m_skDrawLooper(adoptRef(looper)) { }
Nit (see what schenney thinks):

My preference would be to accept a PassRefPtr<SkDrawLooper> here. Currently, the
only call site of this (private) constructor is below, but otherwise I wouldn't
expect something that takes a Foo* to adopt ownership; I'd expect to explicitly
release it then.

If you do this, you'll need to move the adoptRef call from here to the call
site.

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

https://codereview.chromium.org/177473003/diff/120001/Source/platform/graphic...
Source/platform/graphics/DrawLooper.h:47: class PLATFORM_EXPORT DrawLooper :
public RefCounted<DrawLooper> {
Why make this refcounted? The second level of refcounting seems redundant here,
especially now that this class is totally immutable.

(In fact, we might be able to remove the WTF_MAKE_NONCOPYABLE.)

[Dominik Grewe, 2014-02-24 18:11:56]

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

https://codereview.chromium.org/177473003/diff/120001/Source/platform/graphic...
Source/platform/graphics/DrawLooper.h:47: class PLATFORM_EXPORT DrawLooper :
public RefCounted<DrawLooper> {
On 2014/02/24 17:47:44, jbroman wrote:
> Why make this refcounted? The second level of refcounting seems redundant
here,
> especially now that this class is totally immutable.
> 
> (In fact, we might be able to remove the WTF_MAKE_NONCOPYABLE.)

If I remove it the compiler complains that it can't call deref() on DrawLooper.

[jbroman, 2014-02-24 18:18:52]

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

https://codereview.chromium.org/177473003/diff/120001/Source/platform/graphic...
Source/platform/graphics/DrawLooper.h:47: class PLATFORM_EXPORT DrawLooper :
public RefCounted<DrawLooper> {
On 2014/02/24 18:11:57, Dominik Grewe wrote:
> On 2014/02/24 17:47:44, jbroman wrote:
> > Why make this refcounted? The second level of refcounting seems redundant
> here,
> > especially now that this class is totally immutable.
> > 
> > (In fact, we might be able to remove the WTF_MAKE_NONCOPYABLE.)
> 
> If I remove it the compiler complains that it can't call deref() on
DrawLooper.

If you make this class not refcounted, you have to use
OwnPtr/PassOwnPtr/adoptPtr (which manages single-owner semantics) instead of
RefPtr/PassRefPtr/adoptRef to hold pointers to it.

[Dominik Grewe, 2014-02-24 18:34:39]

Using OwnPtr now. PTAL

https://codereview.chromium.org/177473003/diff/120001/Source/platform/graphic...
File Source/platform/graphics/DrawLooper.cpp (right):

https://codereview.chromium.org/177473003/diff/120001/Source/platform/graphic...
Source/platform/graphics/DrawLooper.cpp:45: DrawLooper::DrawLooper(SkDrawLooper*
looper) : m_skDrawLooper(adoptRef(looper)) { }
On 2014/02/24 17:47:44, jbroman wrote:
> Nit (see what schenney thinks):
> 
> My preference would be to accept a PassRefPtr<SkDrawLooper> here. Currently,
the
> only call site of this (private) constructor is below, but otherwise I
wouldn't
> expect something that takes a Foo* to adopt ownership; I'd expect to
explicitly
> release it then.
> 
> If you do this, you'll need to move the adoptRef call from here to the call
> site.

Done.

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

https://codereview.chromium.org/177473003/diff/120001/Source/platform/graphic...
Source/platform/graphics/DrawLooper.h:47: class PLATFORM_EXPORT DrawLooper :
public RefCounted<DrawLooper> {
On 2014/02/24 18:18:53, jbroman wrote:
> On 2014/02/24 18:11:57, Dominik Grewe wrote:
> > On 2014/02/24 17:47:44, jbroman wrote:
> > > Why make this refcounted? The second level of refcounting seems redundant
> > here,
> > > especially now that this class is totally immutable.
> > > 
> > > (In fact, we might be able to remove the WTF_MAKE_NONCOPYABLE.)
> > 
> > If I remove it the compiler complains that it can't call deref() on
> DrawLooper.
> 
> If you make this class not refcounted, you have to use
> OwnPtr/PassOwnPtr/adoptPtr (which manages single-owner semantics) instead of
> RefPtr/PassRefPtr/adoptRef to hold pointers to it.

Done.

[jbroman, 2014-02-24 18:53:37]

OwnPtr only for non-refcounted things, RefPtr for refcounted
(WebCore::RefCounted and SkRefCnt) things. Otherwise, looks good to me if
schenney agrees.

https://codereview.chromium.org/177473003/diff/210001/Source/platform/graphic...
File Source/platform/graphics/DrawLooper.h (right):

https://codereview.chromium.org/177473003/diff/210001/Source/platform/graphic...
Source/platform/graphics/DrawLooper.h:88: OwnPtr<SkDrawLooper> m_skDrawLooper;
I'm sorry I wasn't clear enough before, but while WebCore::DrawLooper is now not
refcounted (and so can be held with an OwnPtr), SkDrawLooper is still refcounted
(so it should be held with RefPtr here, and the constructor argument should be a
PassRefPtr).

[Dominik Grewe, 2014-02-24 19:41:02]

https://codereview.chromium.org/177473003/diff/210001/Source/platform/graphic...
File Source/platform/graphics/DrawLooper.h (right):

https://codereview.chromium.org/177473003/diff/210001/Source/platform/graphic...
Source/platform/graphics/DrawLooper.h:88: OwnPtr<SkDrawLooper> m_skDrawLooper;
On 2014/02/24 18:53:38, jbroman wrote:
> I'm sorry I wasn't clear enough before, but while WebCore::DrawLooper is now
not
> refcounted (and so can be held with an OwnPtr), SkDrawLooper is still
refcounted
> (so it should be held with RefPtr here, and the constructor argument should be
a
> PassRefPtr).

Of course, sorry. Wasn't thinking properly ;) Will fix it tomorrow.

[Dominik Grewe, 2014-02-25 10:29:57]

Okay, I should be using RefPtr/OwnPtr in the right places now. PTAL.

[jbroman, 2014-02-25 13:58:31]

Great work. LGTM.

[Stephen Chennney, 2014-02-25 17:26:43]

not lgtm. I would be far more comfortable if you modified
GraphicsContext::setDrawLooper to take a PassRefPtr<SkDrawLooper> instead of the
const reference. Then I think the ref counting would be clear.

[Stephen Chennney, 2014-02-25 17:36:56]

On 2014/02/25 17:26:43, Stephen Chenney wrote:
> not lgtm. I would be far more comfortable if you modified
> GraphicsContext::setDrawLooper to take a PassRefPtr<SkDrawLooper> instead of
the
> const reference. Then I think the ref counting would be clear.

And detachDrawLooper should also return a PassRefPtr in that case.
Finally, is a DrawLooper ever constructed by Blink now? Is so we can remove it
and just rename the entire class to DrawLooperBuilder.

[Dominik Grewe, 2014-02-25 18:15:08]

On 2014/02/25 17:36:56, Stephen Chenney wrote:
> On 2014/02/25 17:26:43, Stephen Chenney wrote:
> > not lgtm. I would be far more comfortable if you modified
> > GraphicsContext::setDrawLooper to take a PassRefPtr<SkDrawLooper> instead of
> the
> > const reference. Then I think the ref counting would be clear.
> 
> And detachDrawLooper should also return a PassRefPtr in that case.
> Finally, is a DrawLooper ever constructed by Blink now? Is so we can remove it
> and just rename the entire class to DrawLooperBuilder.

Sure, if GraphicsContext::setDrawLooper takes a PassRefPtr<SkDrawLooper> we
don't need DrawLooper at all. It was always just used as a builder class for
SkDrawLooper. In this patch it's really just a wrapper for SkDrawLooper. If we
don't need that anymore we can just have SkDrawLooperBuilder.

[Stephen Chennney, 2014-02-25 18:19:07]

On 2014/02/25 18:15:08, Dominik Grewe wrote:
> On 2014/02/25 17:36:56, Stephen Chenney wrote:
> > On 2014/02/25 17:26:43, Stephen Chenney wrote:
> > > not lgtm. I would be far more comfortable if you modified
> > > GraphicsContext::setDrawLooper to take a PassRefPtr<SkDrawLooper> instead
of
> > the
> > > const reference. Then I think the ref counting would be clear.
> > 
> > And detachDrawLooper should also return a PassRefPtr in that case.
> > Finally, is a DrawLooper ever constructed by Blink now? Is so we can remove
it
> > and just rename the entire class to DrawLooperBuilder.
> 
> Sure, if GraphicsContext::setDrawLooper takes a PassRefPtr<SkDrawLooper> we
> don't need DrawLooper at all. It was always just used as a builder class for
> SkDrawLooper. In this patch it's really just a wrapper for SkDrawLooper. If we
> don't need that anymore we can just have SkDrawLooperBuilder.

Great. Sorry to push you round in circles, but I think we've converged on the
best approach:
- Make DrawLooperBuilder standalone and remove DrawLooper
- Use RefPtr/PassRefPtr in GraphicsContext::setDrawLooper and
DrawLoopBuilder::detach...

[jbroman, 2014-02-25 18:19:31]

On 2014/02/25 17:26:43, Stephen Chenney wrote:
> not lgtm. I would be far more comfortable if you modified
> GraphicsContext::setDrawLooper to take a PassRefPtr<SkDrawLooper> instead of
the
> const reference. Then I think the ref counting would be clear.

Is there a clear policy about whether/where it is OK to use Skia types in core/
(and in particular, core/rendering/)? The Skia headers are still not permitted
by the DEPS file, and I've had reviewers ask me to use WebCore wrapper types to
prevent exposing Skia types outside platform/.

[Stephen Chennney, 2014-02-25 18:27:30]

On 2014/02/25 18:19:31, jbroman wrote:
> On 2014/02/25 17:26:43, Stephen Chenney wrote:
> > not lgtm. I would be far more comfortable if you modified
> > GraphicsContext::setDrawLooper to take a PassRefPtr<SkDrawLooper> instead of
> the
> > const reference. Then I think the ref counting would be clear.
> 
> Is there a clear policy about whether/where it is OK to use Skia types in
core/
> (and in particular, core/rendering/)? The Skia headers are still not permitted
> by the DEPS file, and I've had reviewers ask me to use WebCore wrapper types
to
> prevent exposing Skia types outside platform/.

We would never need to expose the type. We just call
gc->setDrawLooper(builder->detachLooper()). But I agree we would be allowing
people to get the Skia type in core.


We could really avoid the issue, and enforce the coding pattern, by defining the
GraphicsContext::setDrawLooper to take a DrawLooperBuilder and calling detach
within it. I rather like that approach.

[jbroman, 2014-02-25 18:35:05]

On 2014/02/25 18:27:30, Stephen Chenney wrote:
> On 2014/02/25 18:19:31, jbroman wrote:
> > On 2014/02/25 17:26:43, Stephen Chenney wrote:
> > > not lgtm. I would be far more comfortable if you modified
> > > GraphicsContext::setDrawLooper to take a PassRefPtr<SkDrawLooper> instead
of
> > the
> > > const reference. Then I think the ref counting would be clear.
> > 
> > Is there a clear policy about whether/where it is OK to use Skia types in
> core/
> > (and in particular, core/rendering/)? The Skia headers are still not
permitted
> > by the DEPS file, and I've had reviewers ask me to use WebCore wrapper types
> to
> > prevent exposing Skia types outside platform/.
> 
> We would never need to expose the type. We just call
> gc->setDrawLooper(builder->detachLooper()). But I agree we would be allowing
> people to get the Skia type in core.

I suppose that's fair, though it's awkward to expect that people will use the
API in a way that doesn't "say" the type.

> We could really avoid the issue, and enforce the coding pattern, by defining
the
> GraphicsContext::setDrawLooper to take a DrawLooperBuilder and calling detach
> within it. I rather like that approach.

Hmm. The main concern (not necessarily show-stoppers) I have about this is: This
reintroduces the weird "setter destroys its argument" behaviour. If we do this,
maybe there's some way to make it clear that it takes ownership of and/or
destroys the builder.

(As an aside, it might complicate things if we wanted to cache draw loopers
somewhere, but that's too speculative to make decisions about since it's equally
likely we would cache SkPaints or something.)

[Stephen Chennney, 2014-02-25 18:57:00]

On 2014/02/25 18:35:05, jbroman wrote:
> > We would never need to expose the type. We just call
> > gc->setDrawLooper(builder->detachLooper()). But I agree we would be allowing
> > people to get the Skia type in core.
> 
> I suppose that's fair, though it's awkward to expect that people will use the
> API in a way that doesn't "say" the type.
> 
> > We could really avoid the issue, and enforce the coding pattern, by defining
> the
> > GraphicsContext::setDrawLooper to take a DrawLooperBuilder and calling
detach
> > within it. I rather like that approach.
> 
> Hmm. The main concern (not necessarily show-stoppers) I have about this is:
This
> reintroduces the weird "setter destroys its argument" behaviour. If we do
this,
> maybe there's some way to make it clear that it takes ownership of and/or
> destroys the builder.

I see your point. I think we just suck it up and add a comment to the
setDrawLooper method, and also to the DrawLooperBuilder. Fortunately we're now
in Blink land where we can add large comment blocks if we need them.

> 
> (As an aside, it might complicate things if we wanted to cache draw loopers
> somewhere, but that's too speculative to make decisions about since it's
equally
> likely we would cache SkPaints or something.)

I believe that we'll cache paints if we cache anything, so we're reasonably safe
on this front. It's a good thing to consider, however.

[Dominik Grewe, 2014-02-26 10:38:45]

I now removed DrawLooper and we only have DrawLooperBuilder.
GraphicsContext::setDrawLooper() gets a PassOwnPtr<> and calls
detachDrawLooper(). I suppose that makes it clear that you can't use the builder
any more once you've passed it to setDrawLooper(). I still added a comment but
that may be unnecessary. PTAL.

[Stephen Chennney, 2014-02-26 19:35:17]

LGTM with one nit.

https://codereview.chromium.org/177473003/diff/390001/Source/platform/graphic...
File Source/platform/graphics/DrawLooperBuilder.h (right):

https://codereview.chromium.org/177473003/diff/390001/Source/platform/graphic...
Source/platform/graphics/DrawLooperBuilder.h:46: class PLATFORM_EXPORT
DrawLooperBuilder {
Add "final" keyword to prevent inheritance.
This is getting nit-picking, but it's worth doing it right.

[jbroman, 2014-02-26 19:46:08]

Still LGTM. One itty-bitty suggestion that you might want to consider, but I'm
quite happy with this.

https://codereview.chromium.org/177473003/diff/390001/Source/core/rendering/E...
File Source/core/rendering/EllipsisBox.cpp (right):

https://codereview.chromium.org/177473003/diff/390001/Source/core/rendering/E...
Source/core/rendering/EllipsisBox.cpp:70: OwnPtr<DrawLooperBuilder>
drawLooperBuilder = adoptPtr(new DrawLooperBuilder);
nit suggestion: to avoid adoptPtr(...) everywhere, we could create a static
method DrawLooperBuilder::create method returning a PassOwnPtr like we do
elsewhere in blink.

[Dominik Grewe, 2014-02-27 12:15:08]

Thanks!

https://codereview.chromium.org/177473003/diff/390001/Source/core/rendering/E...
File Source/core/rendering/EllipsisBox.cpp (right):

https://codereview.chromium.org/177473003/diff/390001/Source/core/rendering/E...
Source/core/rendering/EllipsisBox.cpp:70: OwnPtr<DrawLooperBuilder>
drawLooperBuilder = adoptPtr(new DrawLooperBuilder);
On 2014/02/26 19:46:09, jbroman wrote:
> nit suggestion: to avoid adoptPtr(...) everywhere, we could create a static
> method DrawLooperBuilder::create method returning a PassOwnPtr like we do
> elsewhere in blink.

Done.

https://codereview.chromium.org/177473003/diff/390001/Source/platform/graphic...
File Source/platform/graphics/DrawLooperBuilder.h (right):

https://codereview.chromium.org/177473003/diff/390001/Source/platform/graphic...
Source/platform/graphics/DrawLooperBuilder.h:46: class PLATFORM_EXPORT
DrawLooperBuilder {
On 2014/02/26 19:35:18, Stephen Chenney wrote:
> Add "final" keyword to prevent inheritance.
> This is getting nit-picking, but it's worth doing it right.

Done.

[Dominik Grewe, 2014-02-27 14:17:24]

The CQ bit was checked by dominikg@chromium.org

[commit-bot: I haz the power, 2014-02-27 14:17:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dominikg@chromium.org/177473003/410001

[commit-bot: I haz the power, 2014-02-27 17:12:43]

Change committed as 168041

[Dominik Grewe, 2014-03-01 10:02:55]

A revert of this CL has been created in
https://codereview.chromium.org/178113003/ by dominikg@chromium.org.

The reason for reverting is: crash in SkCanvas::getTotalMatrix
(crbug.com/348122)
Reverting this to see if it fixes it, otherwise we can reland..

[Stephen Chennney, 2014-03-03 13:40:47]

https://codereview.chromium.org/177473003/diff/410001/Source/platform/graphic...
File Source/platform/graphics/GraphicsContext.h (right):

https://codereview.chromium.org/177473003/diff/410001/Source/platform/graphic...
Source/platform/graphics/GraphicsContext.h:324: void
setDrawLooper(PassOwnPtr<DrawLooperBuilder>);
The problem was that this should be a PassRefPtr, I think, as the given argument
is then saved in a RefPtr.

[Dominik Grewe, 2014-03-03 13:45:23]

https://codereview.chromium.org/177473003/diff/410001/Source/platform/graphic...
File Source/platform/graphics/GraphicsContext.h (right):

https://codereview.chromium.org/177473003/diff/410001/Source/platform/graphic...
Source/platform/graphics/GraphicsContext.h:324: void
setDrawLooper(PassOwnPtr<DrawLooperBuilder>);
On 2014/03/03 13:40:48, Stephen Chenney wrote:
> The problem was that this should be a PassRefPtr, I think, as the given
argument
> is then saved in a RefPtr.

The result of calling detachDrawLooper on the argument (which returns
PassRefPtr) is saved in a RefPtr. The argument itself isn't stored anywhere and
gets destroyed once it's out of scope.

[Dominik Grewe, 2014-03-13 18:11:13]

Any ideas on how this patch could have caused the crashes reported in
crbug.com/348122 ? I don't really see it, but maybe I'm missing something.

[Stephen Chennney, 2014-03-13 18:23:45]

On 2014/03/13 18:11:13, Dominik Grewe wrote:
> Any ideas on how this patch could have caused the crashes reported in
> crbug.com/348122 ? I don't really see it, but maybe I'm missing something.

Reverting fixed the crash, right?

It has to be something about the pointer ownership, but I'm finding Rietveld
tough to work with here.

Maybe as a start, try rebasing it against the new version of GraphicsContext.

[Dominik Grewe, 2014-03-14 10:25:17]

On 2014/03/13 18:23:45, Stephen Chenney wrote:
> On 2014/03/13 18:11:13, Dominik Grewe wrote:
> > Any ideas on how this patch could have caused the crashes reported in
> > crbug.com/348122 ? I don't really see it, but maybe I'm missing something.
> 
> Reverting fixed the crash, right?

I'm not sure if the crash has been fixed. There are still similar crash reports,
even with the latest version afaict. I wasn't able to repro it.


> It has to be something about the pointer ownership, but I'm finding Rietveld
> tough to work with here.
> 
> Maybe as a start, try rebasing it against the new version of GraphicsContext.

Done.

[Stephen Chennney, 2014-03-14 23:13:41]

I looked through the crash logs too, and it does seem like we've been crashing
with null canvas objects for a while, long before and after your change was in.

I think this current issue is LGTM, so land it again. My guess is that the real
problem is in ref counting of Skia objects like the problems that Tom has
documented. Someone really needs to fix that, so maybe I'll finally take the
time to figure out how Skia ref counts things.

[Dominik Grewe, 2014-03-17 09:43:31]

The CQ bit was checked by dominikg@chromium.org

[commit-bot: I haz the power, 2014-03-17 09:43:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dominikg@chromium.org/177473003/430001

[commit-bot: I haz the power, 2014-03-17 11:01:39]

Change committed as 169335