Bug 128001

Summary: Feature flag for shape-inside
Product: WebKit Reporter: Dean Jackson <dino>
Component: CSSAssignee: Dean Jackson <dino>
Status: RESOLVED FIXED    
Severity: Normal CC: allan.jensen, betravis, bjonesbe, commit-queue, dbates, dstockwell, esprehn+autocc, glenn, gyuyoung.kim, kondapallykalyan, macpherson, menard, simon.fraser, webkit-bug-importer, zoltan
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 128525    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch simon.fraser: review+

Dean Jackson
Reported 2014-01-31 10:57:14 PST
I'd like to separate the shape-inside implementation feature defines from the shape-outside ones.
Attachments
Patch (69.32 KB, patch)
2014-02-03 11:47 PST, Dean Jackson 		no flags
DetailsFormatted DiffDiff
Patch (69.32 KB, patch)
2014-02-03 12:01 PST, Dean Jackson 		no flags
DetailsFormatted DiffDiff
Patch (73.41 KB, patch)
2014-02-03 14:42 PST, Dean Jackson 		simon.fraser: review+
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2014-01-31 10:57:50 PST
<rdar://problem/15958487>
Dean Jackson
Comment 2 2014-02-03 11:47:45 PST
Created attachment 223004 [details] Patch
Dean Jackson
Comment 3 2014-02-03 11:50:13 PST
Comment on attachment 223004 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=223004&action=review > Source/WebCore/css/CSSParser.cpp:5885 > - || (valueId == CSSValueOutsideShape && propId == CSSPropertyWebkitShapeInside)) { > + || (valueId == CSSValueOutsideShape > +#if ENABLE(CSS_SHAPE_INSIDE) > + && propId == CSSPropertyWebkitShapeInside > +#endif I'm not sure if this is correct.
Dean Jackson
Comment 4 2014-02-03 12:01:00 PST
Created attachment 223005 [details] Patch
Bear Travis
Comment 5 2014-02-03 13:24:48 PST
(In reply to comment #3) > (From update of attachment 223004 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=223004&action=review > > > Source/WebCore/css/CSSParser.cpp:5885 > > - || (valueId == CSSValueOutsideShape && propId == CSSPropertyWebkitShapeInside)) { > > + || (valueId == CSSValueOutsideShape > > +#if ENABLE(CSS_SHAPE_INSIDE) > > + && propId == CSSPropertyWebkitShapeInside > > +#endif > > I'm not sure if this is correct. The outside-shape value is only valid for shape-inside, so should be wrapped in the compile guard as well. Looks like you already got it in the second patch though. Thanks for doing this.
Bem Jones-Bey
Comment 6 2014-02-03 13:41:09 PST
Comment on attachment 223005 [details] Patch Two comments: 1) RenderBlock::imageChanged is used by both shape-inside and shape-outside. (It won't let me comment on that line because it's not part of the diff) 2) shape-padding is only used by shape-inside. Otherwise, looks good to me. Thanks!
Dean Jackson
Comment 7 2014-02-03 13:59:16 PST
(In reply to comment #6) > (From update of attachment 223005 [details]) > Two comments: > 1) RenderBlock::imageChanged is used by both shape-inside and shape-outside. (It won't let me comment on that line because it's not part of the diff) Great. Fixed this. > 2) shape-padding is only used by shape-inside. OK. I'll protect against it in the CSS parser, but I'll leave it in RenderStyle (otherwise it is a bit too intrusive, and I already hate how many #ifs this added).
Dean Jackson
Comment 8 2014-02-03 14:42:42 PST
Created attachment 223022 [details] Patch
Dean Jackson
Comment 9 2014-02-03 14:55:06 PST
I think this one is all good!
Bem Jones-Bey
Comment 10 2014-02-03 15:05:31 PST
(In reply to comment #9) > I think this one is all good! Yeah, it looks good. I should have asked this before, but have you tested that it compiles with the CSS_SHAPE_INSIDE flag turned off? (and maybe even that it passes the shape-outside tests in such a condition?)
Dean Jackson
Comment 11 2014-02-03 15:31:31 PST
Committed r163333: <http://trac.webkit.org/changeset/163333>
Darin Adler
Comment 12 2014-02-03 15:56:47 PST
Comment on attachment 223022 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=223022&action=review > Source/WebCore/css/CSSParser.cpp:5888 > + || (valueId == CSSValueOutsideShape > + && propId == CSSPropertyWebkitShapeInside) Why broken into two lines? This seems less readable than before.
Note You need to log in before you can comment on or make changes to this bug.