Bug 95490 – [CSS Exclusions] add support for the basic shapes

• Component: CSS
• Status: RESOLVED DUPLICATE

[Hans Muller]

Initial support for the shapes listed in the "Basic Shapes" section of the CSS Exclusions spec - http://dev.w3.org/csswg/css3-exclusions/#basic-shapes-from-svg-syntax.

[Hans Muller]

Created attachment 161541 [details]
Patch

[WebKit Review Bot]

Comment on attachment 161541 [details]
Patch

Attachment 161541 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13680864

[Early Warning System Bot]

Comment on attachment 161541 [details]
Patch

Attachment 161541 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13682855

[Gyuyoung Kim]

Comment on attachment 161541 [details]
Patch

Attachment 161541 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13680863

[Early Warning System Bot]

Comment on attachment 161541 [details]
Patch

Attachment 161541 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13681854

[Build Bot]

Comment on attachment 161541 [details]
Patch

Attachment 161541 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13694823

[Peter Beverloo (cr-android ews)]

Comment on attachment 161541 [details]
Patch

Attachment 161541 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/13699008

[Hans Muller]

Created attachment 161573 [details]
Patch

[Hans Muller]

(In reply to comment #8)
Added the new  filenames to five WebCore build files (they'd already been added to the XCode project).  Hopefully that's all of them.

[Build Bot]

Comment on attachment 161573 [details]
Patch

Attachment 161573 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13683938

[Bear Travis]

Here's a first pass over the code. I have not yet taken a really good look at the geometry algorithms, so comments on those will follow later.

> Source/WebCore/ChangeLog:31
> +        (WebCore::IntervalX1Comparator::operator()):

For a patch this large, you will probably need to write function-level comments (full-sentences) for each of the ChangeLog entries. You may be able to hold off a while if you anticipate large-scale architecture changes during the review process.

> Source/WebCore/platform/graphics/XSInterval.cpp:1
> +/*

I think XS is not the most descriptive prefix. ExclusionShape is probably a little too long. BasicShape, or just Shape? What classes are we most worried about naming collisions with?

> Source/WebCore/platform/graphics/XSInterval.cpp:37
> +struct IntervalX1Comparator {

Would the name XSIntervalComparator be more appropriate?

> Source/WebCore/platform/graphics/XSInterval.cpp:45
> +{  

WebKit code leans towards full-word variable names:
http://www.webkit.org/coding/coding-style.html#names-full-words
Perhaps other and result?

> Source/WebCore/platform/graphics/XSInterval.cpp:70
> +        for (size_t i = 0; i < v.size(); i++) {

Think I'm reading this right. Since interval is only null on the first pass, and v is guaranteed to have at least one element, I think this can change to:
ASSERT(v.size());
interval = &v[0];
for (size_t i = 1; ... ; ... )

> Source/WebCore/platform/graphics/XSInterval.cpp:71
> +            if (!interval)

If the above is correct, you can remove this case

> Source/WebCore/platform/graphics/XSInterval.cpp:81
> +        if (interval)

And this null-check, and just append the interval.

> Source/WebCore/platform/graphics/XSPolygon.h:1
> +/*

Because the patch is already large, and we aren't testing Polygons yet, I would recommend removing the Polygon files and add them in a subsequent patch.

> Source/WebCore/platform/graphics/XSRectangle.cpp:35
> +// See http://hansmuller-webkit.blogspot.com/2012/07/computing-horizonal-rounded-rectangle.html

I would remove the blog reference in the file, and place it in the bug or the ChangeLog.

> Source/WebCore/platform/graphics/XShape.cpp:103
> +    case BasicShape::BASIC_SHAPE_POLYGON: {

If you remove Polygon, this can be changed to notImplemented()

> Source/WebCore/platform/graphics/XShape.cpp:115
> +    default:

Removing the default case adds a compile-time check that each shape case is covered. The ASSERT_NOT_REACHED can then be below the switch statement.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1325
> +        if (wrapShapeInfo) {

I would change this back to the original form (no lineHeight), and add that in a subsequent patch.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1327
> +            LayoutUnit lineBottom = lineTop + lineHeight(true, isHorizontalWritingMode() ? HorizontalLine : VerticalLine, PositionOfInteriorLineBoxes);

"true" should be replaced with layoutState.lineInfo().isFirstLine()

> Source/WebCore/rendering/WrapShapeInfo.cpp:-108
> -

Whitespace removed

> Source/WebCore/rendering/WrapShapeInfo.cpp:114
> +bool WrapShapeInfo::computeSegmentsForLine(LayoutUnit lineTop, LayoutUnit lineBottom)

Leave the signature on this as just lineTop, until you use lineBottom

> Source/WebCore/rendering/WrapShapeInfo.h:81
> +    bool computeSegmentsForLine(LayoutUnit lineTop, LayoutUnit lineBottom);

Ditto

[Hans Muller]

(In reply to comment #11)
> > Source/WebCore/platform/graphics/XSInterval.cpp:1
> > +/*
> 
> I think XS is not the most descriptive prefix. ExclusionShape is probably a little too long. BasicShape, or just Shape? What classes are we most worried about naming collisions with?

Since the classes are intended to (just) support operations required by CSS Exclusions, I don't 
think a generic prefix like "Shape" would make sense.  BasicShape (nee WrapShape) has already 
been used as the name of the class that just encodes the CSS information.  ExclusionShape is 
what "XS" is short for.  I was reluctant replace the abbreviation because names like 
ExclusionShapeIntervalX1Comparator are pretty clunky.

> 
> > Source/WebCore/platform/graphics/XSInterval.cpp:37
> > +struct IntervalX1Comparator {
> 
> Would the name XSIntervalComparator be more appropriate?

No, that would obscure the important fact that XSInterval's "x1" field is what's being compared.


> > Source/WebCore/platform/graphics/XSInterval.cpp:45
> > +{  
> 
> WebKit code leans towards full-word variable names:
> http://www.webkit.org/coding/coding-style.html#names-full-words
> Perhaps other and result?

Yes, that would probably be more coding-style compliant.


> > Source/WebCore/platform/graphics/XSInterval.cpp:70
> > +        for (size_t i = 0; i < v.size(); i++) {
> 
> Think I'm reading this right. Since interval is only null on the first pass, and v is guaranteed to have at least one element, I think this can change to:
> ASSERT(v.size());
> interval = &v[0];
> for (size_t i = 1; ... ; ... )

What you mean is that v is guaranteed to have two elements.  In that case, yes the code could be refactored as you've suggested.

[Hans Muller]

Created attachment 161726 [details]
Patch

[Build Bot]

Comment on attachment 161726 [details]
Patch

Attachment 161726 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13719249

[Hans Muller]

Created attachment 161744 [details]
Patch

Fixed an uninitialized variable problem that the windows compiler was complaining about; removed extraneous trailing white space in the new files.

[Build Bot]

Comment on attachment 161744 [details]
Patch

Attachment 161744 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13698022

[Hans Muller]

Created attachment 161764 [details]
Patch

Eliminated a dead code path in XShape::createXShape() that caused an error in the Windows build.

[Hans Muller]

*** This bug has been marked as a duplicate of bug 95625 ***