Bug 128001 - Feature flag for shape-inside
Summary: Feature flag for shape-inside
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dean Jackson
URL:
Keywords: InRadar
Depends on: 128525
Blocks:
  Show dependency treegraph
 
Reported: 2014-01-31 10:57 PST by Dean Jackson
Modified: 2014-02-10 04:14 PST (History)
15 users (show)

See Also:


Attachments
Patch (69.32 KB, patch)
2014-02-03 11:47 PST, Dean Jackson
no flags Details | Formatted Diff | Diff
Patch (69.32 KB, patch)
2014-02-03 12:01 PST, Dean Jackson
no flags Details | Formatted Diff | Diff
Patch (73.41 KB, patch)
2014-02-03 14:42 PST, Dean Jackson
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dean Jackson 2014-01-31 10:57:14 PST
I'd like to separate the shape-inside implementation feature defines from the shape-outside ones.
Comment 1 Radar WebKit Bug Importer 2014-01-31 10:57:50 PST
<rdar://problem/15958487>
Comment 2 Dean Jackson 2014-02-03 11:47:45 PST
Created attachment 223004 [details]
Patch
Comment 3 Dean Jackson 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.
Comment 4 Dean Jackson 2014-02-03 12:01:00 PST
Created attachment 223005 [details]
Patch
Comment 5 Bear Travis 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.
Comment 6 Bem Jones-Bey 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!
Comment 7 Dean Jackson 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).
Comment 8 Dean Jackson 2014-02-03 14:42:42 PST
Created attachment 223022 [details]
Patch
Comment 9 Dean Jackson 2014-02-03 14:55:06 PST
I think this one is all good!
Comment 10 Bem Jones-Bey 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?)
Comment 11 Dean Jackson 2014-02-03 15:31:31 PST
Committed r163333: <http://trac.webkit.org/changeset/163333>
Comment 12 Darin Adler 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.