Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1710633002:
Pull up a subset of CanvasRenderingContext2D into BaseRenderingContext2D. (Closed)
|
Created:
4 years, 10 months ago by ikilpatrick Modified:
4 years, 10 months ago CC:
chromium-reviews, blink-reviews, ajuma+watch-canvas_chromium.org, Rik, dshwang Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPull up a subset of CanvasRenderingContext2D into BaseRenderingContext2D.
This will allow an additional sub-class of BaseRC2D called PaintRC2D.
BaseRC2D contains the subset of APIs needed for PaintRC2D.
See: https://drafts.css-houdini.org/css-paint-api/#paintrenderingcontext2d
BUG=578252
Committed: https://crrev.com/4333ef80f90f702a0571c030f15f0e317931ee7d
Cr-Commit-Position: refs/heads/master@{#377082}
Patch Set 1 #Patch Set 2 : fixup. #
Total comments: 8
Patch Set 3 : now with moar copy detection. #
Total comments: 6
Patch Set 4 : moved templated functions into .h file. #
Total comments: 2
Patch Set 5 : move draw* out of class definition. #Patch Set 6 : rebase #
Total comments: 1
Messages
Total messages: 28 (9 generated)
ikilpatrick@chromium.org changed reviewers: + junov@chromium.org
So this turned out to be easier than expected. There is a pretty nice subset of virtual methods now which aren't too crazy in BaseRC2D. I'm fine with this as a review, or I could pull things across more slowly in a piecemeal approach. Up to you!
This is great. It is exactly what I had in mind. lgtm with nits. https://codereview.chromium.org/1710633002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.h (right): https://codereview.chromium.org/1710633002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.h:66: class MODULES_EXPORT CanvasRenderingContext2D final : public CanvasRenderingContext, public BaseRenderingContext2D, public WebThread::TaskObserver, public SVGResourceClient { The svg resource client business needs to be figured out if you want expose canvas filters in PRC2D. Because asynchronous state changes are probably not relevant for PRC2D we'll probably need to special case the handling of filters that reference svg nodes. I am fine with solving that in another CL. Make sure to loop-in ajuma@ when you look into that. https://codereview.chromium.org/1710633002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.h:96: ImageData* createImageData(ImageData*) const; The image data stuff should live in Base IMO, even though you don't intend to expose them in PRC2D. Your call. If you don't do it now, we'll just do it in a later CL because we'll need it for offscreen canvas. https://codereview.chromium.org/1710633002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.h:179: void setShadow(const FloatSize& offset, double blur, RGBA32 color); Looks like I missed this guy in an earlier cleanup CL. Mind deleting it? https://codereview.chromium.org/1710633002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.h:203: bool is2d() const override { return true; } Should this go in Base? I'm not sure...
Description was changed from ========== [WIP] Pull up a subset of CanvasRenderingContext2D into BaseRenderingContext2D. This will allow an additional sub-class of BaseRC2D called PaintRC2D. BaseRC2D contains the subset of APIs needed for PaintRC2D. See: https://drafts.css-houdini.org/css-paint-api/#paintrenderingcontext2d BUG=578252 ========== to ========== Pull up a subset of CanvasRenderingContext2D into BaseRenderingContext2D. This will allow an additional sub-class of BaseRC2D called PaintRC2D. BaseRC2D contains the subset of APIs needed for PaintRC2D. See: https://drafts.css-houdini.org/css-paint-api/#paintrenderingcontext2d BUG=578252 ==========
Now with moar copy detection! https://codereview.chromium.org/1710633002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.h (right): https://codereview.chromium.org/1710633002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.h:66: class MODULES_EXPORT CanvasRenderingContext2D final : public CanvasRenderingContext, public BaseRenderingContext2D, public WebThread::TaskObserver, public SVGResourceClient { On 2016/02/18 03:52:09, Justin Novosad (slow) wrote: > The svg resource client business needs to be figured out if you want expose > canvas filters in PRC2D. Because asynchronous state changes are probably not > relevant for PRC2D we'll probably need to special case the handling of filters > that reference svg nodes. I am fine with solving that in another CL. Make sure > to loop-in ajuma@ when you look into that. Yeah lets do this in a separate CL. https://codereview.chromium.org/1710633002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.h:96: ImageData* createImageData(ImageData*) const; On 2016/02/18 03:52:09, Justin Novosad (slow) wrote: > The image data stuff should live in Base IMO, even though you don't intend to > expose them in PRC2D. Your call. If you don't do it now, we'll just do it in a > later CL because we'll need it for offscreen canvas. Done. https://codereview.chromium.org/1710633002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.h:179: void setShadow(const FloatSize& offset, double blur, RGBA32 color); On 2016/02/18 03:52:09, Justin Novosad (slow) wrote: > Looks like I missed this guy in an earlier cleanup CL. Mind deleting it? Done. https://codereview.chromium.org/1710633002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.h:203: bool is2d() const override { return true; } On 2016/02/18 03:52:09, Justin Novosad (slow) wrote: > Should this go in Base? I'm not sure... It's overriding CanvasRenderingContext which BaseRenderingContext2D doesn't inherit from, so no at the moment I think. This may need to change :). https://codereview.chromium.org/1710633002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.cpp (right): https://codereview.chromium.org/1710633002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.cpp:552: void BaseRenderingContext2D::drawForText(const Font& font, const TextRunPaintInfo& textRunPaintInfo, const FloatPoint& location, CanvasRenderingContext2DState::PaintType paintType) This fine? Needed as templates don't work across classes. https://codereview.chromium.org/1710633002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.h (left): https://codereview.chromium.org/1710633002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.h:315: WillBeHeapVector<OwnPtrWillBeMember<CanvasRenderingContext2DState>> m_stateStack; Worth putting accessors on these for sub-classes?
PTAL also.
https://codereview.chromium.org/1710633002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.cpp (right): https://codereview.chromium.org/1710633002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.cpp:552: void BaseRenderingContext2D::drawForText(const Font& font, const TextRunPaintInfo& textRunPaintInfo, const FloatPoint& location, CanvasRenderingContext2DState::PaintType paintType) On 2016/02/19 00:31:05, ikilpatrick wrote: > This fine? Needed as templates don't work across classes. Not true that templates don't work across classes. The problem is that implicit template instantiation requires the template definition to be in the same compilation unit as the call site that instantiates it. The solution to that is to move the definition of ::draw() into the .h file, or to use explicit template instancing. https://codereview.chromium.org/1710633002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.h (left): https://codereview.chromium.org/1710633002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.h:315: WillBeHeapVector<OwnPtrWillBeMember<CanvasRenderingContext2DState>> m_stateStack; On 2016/02/19 00:31:05, ikilpatrick wrote: > Worth putting accessors on these for sub-classes? I think 'protected' accessors would make sense if needed.
https://codereview.chromium.org/1710633002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.cpp (right): https://codereview.chromium.org/1710633002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.cpp:552: void BaseRenderingContext2D::drawForText(const Font& font, const TextRunPaintInfo& textRunPaintInfo, const FloatPoint& location, CanvasRenderingContext2DState::PaintType paintType) On 2016/02/22 19:55:13, Justin Novosad (slow) wrote: > On 2016/02/19 00:31:05, ikilpatrick wrote: > > This fine? Needed as templates don't work across classes. > > Not true that templates don't work across classes. The problem is that implicit > template instantiation requires the template definition to be in the same > compilation unit as the call site that instantiates it. The solution to that is > to move the definition of ::draw() into the .h file, or to use explicit template > instancing. Right, moved draw & compositedDraw into .h file. I could also move it into BaseRenderingContext2DInl.h (and include it at the end of BRC2D.h), but whatever you want. https://codereview.chromium.org/1710633002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.h (left): https://codereview.chromium.org/1710633002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.h:315: WillBeHeapVector<OwnPtrWillBeMember<CanvasRenderingContext2DState>> m_stateStack; On 2016/02/22 19:55:13, Justin Novosad (slow) wrote: > On 2016/02/19 00:31:05, ikilpatrick wrote: > > Worth putting accessors on these for sub-classes? > > I think 'protected' accessors would make sense if needed. I had another think about this, I don't think it makes it more readable.
lgtm with nit. https://codereview.chromium.org/1710633002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.h (right): https://codereview.chromium.org/1710633002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.h:235: void compositedDraw(const DrawFunc& drawFunc, SkCanvas* c, CanvasRenderingContext2DState::PaintType paintType, CanvasRenderingContext2DState::ImageType imageType) This is a pretty big method. I don't think a separate file is necessary, but you should at least move this outside of the class definition. Same for draw()
https://codereview.chromium.org/1710633002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.h (right): https://codereview.chromium.org/1710633002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.h:235: void compositedDraw(const DrawFunc& drawFunc, SkCanvas* c, CanvasRenderingContext2DState::PaintType paintType, CanvasRenderingContext2DState::ImageType imageType) On 2016/02/22 22:50:55, Justin Novosad (slow) wrote: > This is a pretty big method. I don't think a separate file is necessary, but you > should at least move this outside of the class definition. Same for draw() Done.
On 2016/02/22 23:19:10, ikilpatrick wrote: > https://codereview.chromium.org/1710633002/diff/60001/third_party/WebKit/Sour... > File third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.h > (right): > > https://codereview.chromium.org/1710633002/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.h:235: void > compositedDraw(const DrawFunc& drawFunc, SkCanvas* c, > CanvasRenderingContext2DState::PaintType paintType, > CanvasRenderingContext2DState::ImageType imageType) > On 2016/02/22 22:50:55, Justin Novosad (slow) wrote: > > This is a pretty big method. I don't think a separate file is necessary, but > you > > should at least move this outside of the class definition. Same for draw() > > Done. lgtm
The CQ bit was checked by ikilpatrick@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from junov@chromium.org Link to the patchset: https://codereview.chromium.org/1710633002/#ps100001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1710633002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1710633002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by ikilpatrick@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1710633002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1710633002/100001
Message was sent while issue was closed.
Description was changed from ========== Pull up a subset of CanvasRenderingContext2D into BaseRenderingContext2D. This will allow an additional sub-class of BaseRC2D called PaintRC2D. BaseRC2D contains the subset of APIs needed for PaintRC2D. See: https://drafts.css-houdini.org/css-paint-api/#paintrenderingcontext2d BUG=578252 ========== to ========== Pull up a subset of CanvasRenderingContext2D into BaseRenderingContext2D. This will allow an additional sub-class of BaseRC2D called PaintRC2D. BaseRC2D contains the subset of APIs needed for PaintRC2D. See: https://drafts.css-houdini.org/css-paint-api/#paintrenderingcontext2d BUG=578252 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Pull up a subset of CanvasRenderingContext2D into BaseRenderingContext2D. This will allow an additional sub-class of BaseRC2D called PaintRC2D. BaseRC2D contains the subset of APIs needed for PaintRC2D. See: https://drafts.css-houdini.org/css-paint-api/#paintrenderingcontext2d BUG=578252 ========== to ========== Pull up a subset of CanvasRenderingContext2D into BaseRenderingContext2D. This will allow an additional sub-class of BaseRC2D called PaintRC2D. BaseRC2D contains the subset of APIs needed for PaintRC2D. See: https://drafts.css-houdini.org/css-paint-api/#paintrenderingcontext2d BUG=578252 Committed: https://crrev.com/4333ef80f90f702a0571c030f15f0e317931ee7d Cr-Commit-Position: refs/heads/master@{#377082} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/4333ef80f90f702a0571c030f15f0e317931ee7d Cr-Commit-Position: refs/heads/master@{#377082}
Message was sent while issue was closed.
sigbjornf@opera.com changed reviewers: + sigbjornf@opera.com
Message was sent while issue was closed.
https://codereview.chromium.org/1710633002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.h (right): https://codereview.chromium.org/1710633002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.h:181: WillBeHeapVector<OwnPtrWillBeMember<CanvasRenderingContext2DState>> m_stateStack; This runs into MSVC compilation breakage non-Oilpan, https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win%20non-Oilp... i.e., MSVC attempting to instantiate code that ends up calling OwnPtr<T>(const OwnPtr<T>&) over uncopyable OwnPtrs. I can't readily make out what makes this particular Vector<OwnPtr<>> use unique..
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:100001) has been created in https://codereview.chromium.org/1732923002/ by engedy@chromium.org. The reason for reverting is: Speculative revert to fix WebKit Win non-Oilpan (dbg) breakage..
Message was sent while issue was closed.
On 2016/02/24 16:09:38, engedy wrote: > A revert of this CL (patchset #6 id:100001) has been created in > https://codereview.chromium.org/1732923002/ by mailto:engedy@chromium.org. > > The reason for reverting is: Speculative revert to fix WebKit Win non-Oilpan > (dbg) breakage.. I don't get why Vector<OwnPtr<xyz>> ever worked in the first place. In VectorTraits.h we have this: template <typename P> struct VectorTraits<OwnPtr<P>> : SimpleClassVectorTraits<OwnPtr<P>> { // OwnPtr -> PassOwnPtr has a very particular structure that tricks the // normal type traits into thinking that the class is "trivially copyable". static const bool canCopyWithMemcpy = false; }; That forces vector copying to go through the copy constructor... and in OwnPtr.h the copy constructor is made private by WTF_MAKE_NONCOPYABLE(OwnPtr);
Message was sent while issue was closed.
On 2016/02/24 16:51:08, Justin Novosad wrote: > On 2016/02/24 16:09:38, engedy wrote: > > A revert of this CL (patchset #6 id:100001) has been created in > > https://codereview.chromium.org/1732923002/ by mailto:engedy@chromium.org. > > > > The reason for reverting is: Speculative revert to fix WebKit Win non-Oilpan > > (dbg) breakage.. > > I don't get why Vector<OwnPtr<xyz>> ever worked in the first place. > > In VectorTraits.h we have this: > > template <typename P> > struct VectorTraits<OwnPtr<P>> : SimpleClassVectorTraits<OwnPtr<P>> { > // OwnPtr -> PassOwnPtr has a very particular structure that tricks the > // normal type traits into thinking that the class is "trivially copyable". > static const bool canCopyWithMemcpy = false; > }; > > That forces vector copying to go through the copy constructor... and in OwnPtr.h > the copy constructor is made private by WTF_MAKE_NONCOPYABLE(OwnPtr); Thanks for the revert. So this is pushing my c++ knowledge limits, but I've been looking at some similar issues. https://groups.google.com/a/chromium.org/forum/#!topic/cxx/lZ72n5oJhL8 I think the solution is to make CanvasRenderingContext2DState WTF_MAKE_NONCOPYABLE, but we'll find out.
Message was sent while issue was closed.
On 2016/02/24 18:16:57, ikilpatrick wrote: > On 2016/02/24 16:51:08, Justin Novosad wrote: > > On 2016/02/24 16:09:38, engedy wrote: > > > A revert of this CL (patchset #6 id:100001) has been created in > > > https://codereview.chromium.org/1732923002/ by mailto:engedy@chromium.org. > > > > > > The reason for reverting is: Speculative revert to fix WebKit Win non-Oilpan > > > (dbg) breakage.. > > > > I don't get why Vector<OwnPtr<xyz>> ever worked in the first place. > > > > In VectorTraits.h we have this: > > > > template <typename P> > > struct VectorTraits<OwnPtr<P>> : SimpleClassVectorTraits<OwnPtr<P>> { > > // OwnPtr -> PassOwnPtr has a very particular structure that tricks the > > // normal type traits into thinking that the class is "trivially > copyable". > > static const bool canCopyWithMemcpy = false; > > }; > > > > That forces vector copying to go through the copy constructor... and in > OwnPtr.h > > the copy constructor is made private by WTF_MAKE_NONCOPYABLE(OwnPtr); > > Thanks for the revert. > > So this is pushing my c++ knowledge limits, but I've been looking at some > similar issues. > https://groups.google.com/a/chromium.org/forum/#!topic/cxx/lZ72n5oJhL8 > > I think the solution is to make CanvasRenderingContext2DState > WTF_MAKE_NONCOPYABLE, but we'll find out. I think that's it (not that I fully understand why), thanks for the pointer. (WTF_MAKE_NONCOPYABLE(BaseRenderingContext2D) works out - tested locally.) |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
