Bug 95490

Created attachment 161541 [details] Patch

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

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

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

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

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

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

Created attachment 161573 [details] Patch

(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.

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

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

(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.

Created attachment 161726 [details] Patch

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

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.

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

Created attachment 161764 [details] Patch Eliminated a dead code path in XShape::createXShape() that caused an error in the Windows build.

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