Bug 126205

Created attachment 222795 [details] Patch

Comment on attachment 222795 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=222795&action=review Some comments and questions to the patch. > Source/WebCore/rendering/RenderLayer.cpp:3784 > +template <class ReferenceBoxClipPathOperation> > +static inline LayoutRect computeReferenceBox(const RenderObject& renderer, const ReferenceBoxClipPathOperation& clippingPath, const LayoutRect& rootRelativeBounds) Two questions here: Why does it need to be a template function and why can't the code be added to ReferenceBoxClipPathOperation instead? > Source/WebCore/rendering/RenderLayer.cpp:3827 > + if (!rootRelativeBoundsComputed) { > + rootRelativeBounds = calculateLayerBounds(paintingInfo.rootLayer, &offsetFromRoot, 0); > + rootRelativeBoundsComputed = true; > + } Since we need this calculation for all cases now, it is ok to move it up here (was an initial reviewer request just to do it when absolutely necessary). But please remove it from the SVG clipPath code underneath as well. > Source/WebCore/rendering/RenderLayer.cpp:3844 > + // FIXME this does not properly compute the rounded corners as specified in all conditions Real sentence please. > Source/WebCore/rendering/shapes/ShapeInfo.cpp:95 > + // FIXME this does not properly compute the rounded corners as specified in all conditions Ditto.

Comment on attachment 222795 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=222795&action=review >> Source/WebCore/rendering/RenderLayer.cpp:3784 >> +static inline LayoutRect computeReferenceBox(const RenderObject& renderer, const ReferenceBoxClipPathOperation& clippingPath, const LayoutRect& rootRelativeBounds) > > Two questions here: Why does it need to be a template function and why can't the code be added to ReferenceBoxClipPathOperation instead? 1) There is no ReferenceBoxClipPathOperation class, that's just what I named the template parameter. I figured that creating the class and making the class hierarchy more complex wasn't worth it for this singe instance. 2) ClipPathOperation.h does not include RenderBox.h, and since it is only the header file, including it causes circular deps. I could create a ClipPathOperation.cpp and also a ReferenceBoxClipPathOperation class, if you think that is better than having this templated helper here. >> Source/WebCore/rendering/RenderLayer.cpp:3827 >> + } > > Since we need this calculation for all cases now, it is ok to move it up here (was an initial reviewer request just to do it when absolutely necessary). But please remove it from the SVG clipPath code underneath as well. Oops! I'll fix that. >> Source/WebCore/rendering/RenderLayer.cpp:3844 >> + // FIXME this does not properly compute the rounded corners as specified in all conditions > > Real sentence please. Ok. >> Source/WebCore/rendering/shapes/ShapeInfo.cpp:95 >> + // FIXME this does not properly compute the rounded corners as specified in all conditions > > Ditto. Ok.

Comment on attachment 222795 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=222795&action=review >> Source/WebCore/rendering/RenderLayer.cpp:3784 >> +static inline LayoutRect computeReferenceBox(const RenderObject& renderer, const ReferenceBoxClipPathOperation& clippingPath, const LayoutRect& rootRelativeBounds) > > Two questions here: Why does it need to be a template function and why can't the code be added to ReferenceBoxClipPathOperation instead? 1) There is no ReferenceBoxClipPathOperation class, that's just what I named the template parameter. I figured that creating the class and making the class hierarchy more complex wasn't worth it for this singe instance. 2) ClipPathOperation.h does not include RenderBox.h, and since it is only the header file, including it causes circular deps. I could create a ClipPathOperation.cpp and also a ReferenceBoxClipPathOperation class, if you think that is better than having this templated helper here. >> Source/WebCore/rendering/RenderLayer.cpp:3827 >> + } > > Since we need this calculation for all cases now, it is ok to move it up here (was an initial reviewer request just to do it when absolutely necessary). But please remove it from the SVG clipPath code underneath as well. Oops! I'll fix that. >> Source/WebCore/rendering/RenderLayer.cpp:3844 >> + // FIXME this does not properly compute the rounded corners as specified in all conditions > > Real sentence please. Ok. >> Source/WebCore/rendering/shapes/ShapeInfo.cpp:95 >> + // FIXME this does not properly compute the rounded corners as specified in all conditions > > Ditto. Ok.

Created attachment 222815 [details] Patch Fix comments and remove extra if statement

Comment on attachment 222815 [details] Patch r=me

Comment on attachment 222815 [details] Patch Clearing flags on attachment: 222815 Committed r163205: <http://trac.webkit.org/changeset/163205>

All reviewed patches have been landed. Closing bug.