Bug 131444

Created attachment 229558 [details] Patch Added support for shape-outside gradient values.

Attachment 229558 [details] did not pass style-queue: ERROR: Source/WebCore/rendering/style/ShapeValue.h:71: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WebCore/rendering/shapes/ShapeOutsideInfo.cpp:151: Multi line control clauses should use braces. [whitespace/braces] [4] Total errors found: 2 in 9 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 229561 [details] Patch Fixed some styleOs.

Created attachment 229578 [details] Patch Removed a spurious ShapeOutsideInfo.cpp include.

Comment on attachment 229578 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=229578&action=review > Source/WebCore/rendering/shapes/ShapeOutsideInfo.cpp:123 > +static inline bool checkShapeImageOrigin(Document& document, const ShapeValue& shapeValue) Why take a ShapeValue instead of a StyleImage? > Source/WebCore/rendering/shapes/ShapeOutsideInfo.cpp:128 > + CachedImage& cachedImage = *(shapeValue.image()->cachedImage()); I'd ASSERT(shapeValue.image()->cachedImage()) first, just to be safe. > Source/WebCore/rendering/shapes/ShapeOutsideInfo.cpp:151 > + image = styleImage->image(const_cast<RenderBox*>(&renderBox), imageSize).get(); The const_cast makes me very uncomfortable. There isn't a better way to do this? Maybe we need to refactor StyleImage so it can take a const?

(In reply to comment #5) > (From update of attachment 229578 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=229578&action=review > > > Source/WebCore/rendering/shapes/ShapeOutsideInfo.cpp:123 > > +static inline bool checkShapeImageOrigin(Document& document, const ShapeValue& shapeValue) > > Why take a ShapeValue instead of a StyleImage? I've changed the parameter to StyleImage*. > > Source/WebCore/rendering/shapes/ShapeOutsideInfo.cpp:128 > > + CachedImage& cachedImage = *(shapeValue.image()->cachedImage()); > > I'd ASSERT(shapeValue.image()->cachedImage()) first, just to be safe. OK. > > > Source/WebCore/rendering/shapes/ShapeOutsideInfo.cpp:151 > > + image = styleImage->image(const_cast<RenderBox*>(&renderBox), imageSize).get(); > > The const_cast makes me very uncomfortable. There isn't a better way to do this? Maybe we need to refactor StyleImage so it can take a const? I did take a shot at that. The overall change would be much bigger than this patch. There are about 10 classes that implement this image() method and in some cases the not-const RenderElement is passed along to other methods. If it's worth tracking down the complete transitive closure of the change, it might be better to do it as a separate patch.

Created attachment 229662 [details] Patch Made the suggested changes.

Comment on attachment 229662 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=229662&action=review > Source/WebCore/rendering/shapes/ShapeOutsideInfo.cpp:125 > + ASSERT(styleImage); if this can't be null, why not pass a reference? (even better, can it be a const ref?) > Source/WebCore/rendering/shapes/ShapeOutsideInfo.cpp:153 > + image = styleImage->image(const_cast<RenderBox*>(&renderBox), imageSize).get(); It would be nice to see a follow-up patch that removes the need for the const_cast here.

Created attachment 229682 [details] Patch I've made the checkShapeImageOrigin() StyleImage* parameter a const StyleImage&. I'll attempt to make this StyleImage function's RenderElement* parameter, const RenderElement*, in a separate patch: virtual PassRefPtr<Image> image(RenderElement*, const FloatSize&) const = 0;

Comment on attachment 229682 [details] Patch r=me

Comment on attachment 229682 [details] Patch Clearing flags on attachment: 229682 Committed r167518: <http://trac.webkit.org/changeset/167518>

All reviewed patches have been landed. Closing bug.