Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
18718002:
Use RefPtr to manage shaders in Gradient and Pattern. (Closed)
|
Created:
7 years, 5 months ago by jbroman Modified:
7 years, 5 months ago CC:
blink-reviews, jamesr, eae+blinkwatch, danakj, dglazkov+blink, Rik, jeez, pdr. Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionUse RefPtr to manage shaders in Gradient and Pattern.
BUG=254509
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=154260
Patch Set 1 #
Total comments: 2
Messages
Total messages: 13 (0 generated)
lgtm https://codereview.chromium.org/18718002/diff/1/Source/core/platform/graphics... File Source/core/platform/graphics/Gradient.cpp (right): https://codereview.chromium.org/18718002/diff/1/Source/core/platform/graphics... Source/core/platform/graphics/Gradient.cpp:333: m_gradient = adoptRef(SkGradientShader::CreateRadial(m_p1, m_r1, colors, pos, static_cast<int>(countUsed), tile, 0, shouldDrawInPMColorSpace)); In Blink, a "Create..." method would return a PassRefPtr. I presume that is not possible here because PassrefPtr is not known to SkGradientShader. Is that right?
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jbroman@chromium.org/18718002/1
https://codereview.chromium.org/18718002/diff/1/Source/core/platform/graphics... File Source/core/platform/graphics/Gradient.cpp (right): https://codereview.chromium.org/18718002/diff/1/Source/core/platform/graphics... Source/core/platform/graphics/Gradient.cpp:333: m_gradient = adoptRef(SkGradientShader::CreateRadial(m_p1, m_r1, colors, pos, static_cast<int>(countUsed), tile, 0, shouldDrawInPMColorSpace)); On 2013/07/08 13:55:11, Stephen Chenney wrote: > In Blink, a "Create..." method would return a PassRefPtr. I presume that is not > possible here because PassrefPtr is not known to SkGradientShader. Is that > right? Correct; these methods are defined in Skia, which does not know about the WTF smart pointers.
On 2013/07/08 13:59:19, jbroman wrote: > https://codereview.chromium.org/18718002/diff/1/Source/core/platform/graphics... > File Source/core/platform/graphics/Gradient.cpp (right): > > https://codereview.chromium.org/18718002/diff/1/Source/core/platform/graphics... > Source/core/platform/graphics/Gradient.cpp:333: m_gradient = > adoptRef(SkGradientShader::CreateRadial(m_p1, m_r1, colors, pos, > static_cast<int>(countUsed), tile, 0, shouldDrawInPMColorSpace)); > On 2013/07/08 13:55:11, Stephen Chenney wrote: > > In Blink, a "Create..." method would return a PassRefPtr. I presume that is > not > > possible here because PassrefPtr is not known to SkGradientShader. Is that > > right? > > Correct; these methods are defined in Skia, which does not know about the WTF > smart pointers. Just realized that these should be OwnPtr, because you are only using the RefPtr to control the automatic deletion. OwnPtr is for cases like this where the object is not shared.
On 2013/07/08 14:44:22, Stephen Chenney wrote: > On 2013/07/08 13:59:19, jbroman wrote: > > > https://codereview.chromium.org/18718002/diff/1/Source/core/platform/graphics... > > File Source/core/platform/graphics/Gradient.cpp (right): > > > > > https://codereview.chromium.org/18718002/diff/1/Source/core/platform/graphics... > > Source/core/platform/graphics/Gradient.cpp:333: m_gradient = > > adoptRef(SkGradientShader::CreateRadial(m_p1, m_r1, colors, pos, > > static_cast<int>(countUsed), tile, 0, shouldDrawInPMColorSpace)); > > On 2013/07/08 13:55:11, Stephen Chenney wrote: > > > In Blink, a "Create..." method would return a PassRefPtr. I presume that is > > not > > > possible here because PassrefPtr is not known to SkGradientShader. Is that > > > right? > > > > Correct; these methods are defined in Skia, which does not know about the WTF > > smart pointers. > > Just realized that these should be OwnPtr, because you are only using the RefPtr > to control the automatic deletion. OwnPtr is for cases like this where the > object is not shared. SkShader is a SkRefCnt subclass; objects in Skia expect to be able to extend its lifetime by adding a reference (e.g. when attaching the shader to a paint). If the lifetime of the SkPaint outlives this WebCore object, the SkPaint should not hold a dangling pointer.
On 2013/07/08 14:53:46, jbroman wrote: > On 2013/07/08 14:44:22, Stephen Chenney wrote: > > On 2013/07/08 13:59:19, jbroman wrote: > > > > > > https://codereview.chromium.org/18718002/diff/1/Source/core/platform/graphics... > > > File Source/core/platform/graphics/Gradient.cpp (right): > > > > > > > > > https://codereview.chromium.org/18718002/diff/1/Source/core/platform/graphics... > > > Source/core/platform/graphics/Gradient.cpp:333: m_gradient = > > > adoptRef(SkGradientShader::CreateRadial(m_p1, m_r1, colors, pos, > > > static_cast<int>(countUsed), tile, 0, shouldDrawInPMColorSpace)); > > > On 2013/07/08 13:55:11, Stephen Chenney wrote: > > > > In Blink, a "Create..." method would return a PassRefPtr. I presume that > is > > > not > > > > possible here because PassrefPtr is not known to SkGradientShader. Is that > > > > right? > > > > > > Correct; these methods are defined in Skia, which does not know about the > WTF > > > smart pointers. > > > > Just realized that these should be OwnPtr, because you are only using the > RefPtr > > to control the automatic deletion. OwnPtr is for cases like this where the > > object is not shared. > > SkShader is a SkRefCnt subclass; objects in Skia expect to be able to extend its > lifetime by adding a reference (e.g. when attaching the shader to a paint). If > the lifetime of the SkPaint outlives this WebCore object, the SkPaint should not > hold a dangling pointer. OK. Maybe a code comment to clarify the semantics for future developers?
On Mon, Jul 8, 2013 at 7:44 AM, <schenney@chromium.org> wrote: > On 2013/07/08 13:59:19, jbroman wrote: > > https://codereview.chromium.**org/18718002/diff/1/Source/** > core/platform/graphics/**Gradient.cpp<https://codereview.chromium.org/18718002/diff/1/Source/core/platform/graphics/Gradient.cpp> > >> File Source/core/platform/graphics/**Gradient.cpp (right): >> > > > https://codereview.chromium.**org/18718002/diff/1/Source/** > core/platform/graphics/**Gradient.cpp#newcode333<https://codereview.chromium.org/18718002/diff/1/Source/core/platform/graphics/Gradient.cpp#newcode333> > >> Source/core/platform/graphics/**Gradient.cpp:333: m_gradient = >> adoptRef(SkGradientShader::**CreateRadial(m_p1, m_r1, colors, pos, >> static_cast<int>(countUsed), tile, 0, shouldDrawInPMColorSpace)); >> On 2013/07/08 13:55:11, Stephen Chenney wrote: >> > In Blink, a "Create..." method would return a PassRefPtr. I presume >> that is >> not >> > possible here because PassrefPtr is not known to SkGradientShader. Is >> that >> > right? >> > > Correct; these methods are defined in Skia, which does not know about the >> WTF >> smart pointers. >> > > Just realized that these should be OwnPtr, because you are only using the > RefPtr > to control the automatic deletion. OwnPtr is for cases like this where the > object is not shared. > Sticking a ref-counted object into an OwnPtr is risky, and I think we should always avoid that. RefCounted objects should always be deleted by the last unref()/deref() call, not by calling delete on them directly, or as jeremy says you run the risk of deleting objects that have references still held on them. I just want to make the point that I think this should be a global rule for all refcounted objects, not a case-by-case basis. > > https://codereview.chromium.**org/18718002/<https://codereview.chromium.org/1... >
On 2013/07/08 15:38:14, danakj wrote: > On Mon, Jul 8, 2013 at 7:44 AM, <mailto:schenney@chromium.org> wrote: > > > On 2013/07/08 13:59:19, jbroman wrote: > > > > https://codereview.chromium.**org/18718002/diff/1/Source/** > > > core/platform/graphics/**Gradient.cpp<https://codereview.chromium.org/18718002/diff/1/Source/core/platform/graphics/Gradient.cpp> > > > >> File Source/core/platform/graphics/**Gradient.cpp (right): > >> > > > > > > https://codereview.chromium.**org/18718002/diff/1/Source/** > > > core/platform/graphics/**Gradient.cpp#newcode333<https://codereview.chromium.org/18718002/diff/1/Source/core/platform/graphics/Gradient.cpp#newcode333> > > > >> Source/core/platform/graphics/**Gradient.cpp:333: m_gradient = > >> adoptRef(SkGradientShader::**CreateRadial(m_p1, m_r1, colors, pos, > >> static_cast<int>(countUsed), tile, 0, shouldDrawInPMColorSpace)); > >> On 2013/07/08 13:55:11, Stephen Chenney wrote: > >> > In Blink, a "Create..." method would return a PassRefPtr. I presume > >> that is > >> not > >> > possible here because PassrefPtr is not known to SkGradientShader. Is > >> that > >> > right? > >> > > > > Correct; these methods are defined in Skia, which does not know about the > >> WTF > >> smart pointers. > >> > > > > Just realized that these should be OwnPtr, because you are only using the > > RefPtr > > to control the automatic deletion. OwnPtr is for cases like this where the > > object is not shared. > > > > Sticking a ref-counted object into an OwnPtr is risky, and I think we > should always avoid that. RefCounted objects should always be deleted by > the last unref()/deref() call, not by calling delete on them directly, or > as jeremy says you run the risk of deleting objects that have references > still held on them. I just want to make the point that I think this should > be a global rule for all refcounted objects, not a case-by-case basis. > > > > > > > https://codereview.chromium.**org/18718002/%3Chttps://codereview.chromium.org...> > > I'm inclined to agree with danakj – we don't otherwise say "Hey, this is refcounted!" everywhere we make a refcounted object. Any particular reason this case merits a comment?
On 2013/07/08 20:01:17, jbroman wrote: > On 2013/07/08 15:38:14, danakj wrote: > > On Mon, Jul 8, 2013 at 7:44 AM, <mailto:schenney@chromium.org> wrote: > > > > > On 2013/07/08 13:59:19, jbroman wrote: > > > > > > https://codereview.chromium.**org/18718002/diff/1/Source/** > > > > > > core/platform/graphics/**Gradient.cpp<https://codereview.chromium.org/18718002/diff/1/Source/core/platform/graphics/Gradient.cpp> > > > > > >> File Source/core/platform/graphics/**Gradient.cpp (right): > > >> > > > > > > > > > https://codereview.chromium.**org/18718002/diff/1/Source/** > > > > > > core/platform/graphics/**Gradient.cpp#newcode333<https://codereview.chromium.org/18718002/diff/1/Source/core/platform/graphics/Gradient.cpp#newcode333> > > > > > >> Source/core/platform/graphics/**Gradient.cpp:333: m_gradient = > > >> adoptRef(SkGradientShader::**CreateRadial(m_p1, m_r1, colors, pos, > > >> static_cast<int>(countUsed), tile, 0, shouldDrawInPMColorSpace)); > > >> On 2013/07/08 13:55:11, Stephen Chenney wrote: > > >> > In Blink, a "Create..." method would return a PassRefPtr. I presume > > >> that is > > >> not > > >> > possible here because PassrefPtr is not known to SkGradientShader. Is > > >> that > > >> > right? > > >> > > > > > > Correct; these methods are defined in Skia, which does not know about the > > >> WTF > > >> smart pointers. > > >> > > > > > > Just realized that these should be OwnPtr, because you are only using the > > > RefPtr > > > to control the automatic deletion. OwnPtr is for cases like this where the > > > object is not shared. > > > > > > > Sticking a ref-counted object into an OwnPtr is risky, and I think we > > should always avoid that. RefCounted objects should always be deleted by > > the last unref()/deref() call, not by calling delete on them directly, or > > as jeremy says you run the risk of deleting objects that have references > > still held on them. I just want to make the point that I think this should > > be a global rule for all refcounted objects, not a case-by-case basis. > > > > > > > > > > > > > https://codereview.chromium.**org/18718002/%253Chttps://codereview.chromium.o...> > > > > > I'm inclined to agree with danakj – we don't otherwise say "Hey, this is > refcounted!" everywhere we make a refcounted object. Any particular reason this > case merits a comment? ping for a comment on this line of reasoning before I submit
On 2013/07/15 20:07:18, jbroman wrote: > On 2013/07/08 20:01:17, jbroman wrote: > > On 2013/07/08 15:38:14, danakj wrote: > > > On Mon, Jul 8, 2013 at 7:44 AM, <mailto:schenney@chromium.org> wrote: > > > > > > > On 2013/07/08 13:59:19, jbroman wrote: > > > > > > > > https://codereview.chromium.**org/18718002/diff/1/Source/** > > > > > > > > > > core/platform/graphics/**Gradient.cpp<https://codereview.chromium.org/18718002/diff/1/Source/core/platform/graphics/Gradient.cpp> > > > > > > > >> File Source/core/platform/graphics/**Gradient.cpp (right): > > > >> > > > > > > > > > > > > https://codereview.chromium.**org/18718002/diff/1/Source/** > > > > > > > > > > core/platform/graphics/**Gradient.cpp#newcode333<https://codereview.chromium.org/18718002/diff/1/Source/core/platform/graphics/Gradient.cpp#newcode333> > > > > > > > >> Source/core/platform/graphics/**Gradient.cpp:333: m_gradient = > > > >> adoptRef(SkGradientShader::**CreateRadial(m_p1, m_r1, colors, pos, > > > >> static_cast<int>(countUsed), tile, 0, shouldDrawInPMColorSpace)); > > > >> On 2013/07/08 13:55:11, Stephen Chenney wrote: > > > >> > In Blink, a "Create..." method would return a PassRefPtr. I presume > > > >> that is > > > >> not > > > >> > possible here because PassrefPtr is not known to SkGradientShader. Is > > > >> that > > > >> > right? > > > >> > > > > > > > > Correct; these methods are defined in Skia, which does not know about the > > > >> WTF > > > >> smart pointers. > > > >> > > > > > > > > Just realized that these should be OwnPtr, because you are only using the > > > > RefPtr > > > > to control the automatic deletion. OwnPtr is for cases like this where the > > > > object is not shared. > > > > > > > > > > Sticking a ref-counted object into an OwnPtr is risky, and I think we > > > should always avoid that. RefCounted objects should always be deleted by > > > the last unref()/deref() call, not by calling delete on them directly, or > > > as jeremy says you run the risk of deleting objects that have references > > > still held on them. I just want to make the point that I think this should > > > be a global rule for all refcounted objects, not a case-by-case basis. > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.**org/18718002/%25253Chttps://codereview.chromium...> > > > > > > > > I'm inclined to agree with danakj – we don't otherwise say "Hey, this is > > refcounted!" everywhere we make a refcounted object. Any particular reason > this > > case merits a comment? > > ping for a comment on this line of reasoning before I submit Go for it without the comment. I'll reply on future code review to catch any misunderstandings about ref counted Skia objects in Blink.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jbroman@chromium.org/18718002/1
Message was sent while issue was closed.
Change committed as 154260 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
