Summary: | [CSS Shapes] Add support for shape-outside gradient values | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Zoltan Horvath <zoltan> | ||||||||||||
Component: | CSS | Assignee: | Nobody <webkit-unassigned> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | commit-queue, esprehn+autocc, giles_joplin, glenn, gyuyoung.kim, kondapallykalyan, macpherson, menard, syoichi | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Bug Depends on: | |||||||||||||||
Bug Blocks: | 124173 | ||||||||||||||
Attachments: |
|
Description
Zoltan Horvath
2014-04-09 11:00:12 PDT
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. |