| Summary: | [CSS Shapes] Use the float height to determine position in shape-inside | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Bem Jones-Bey <bjonesbe> | ||||||||
| Component: | CSS | Assignee: | Zoltan Horvath <zoltan> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | bjonesbe, commit-queue, donggwan.kim, esprehn+autocc, glenn, kondapallykalyan | ||||||||
| Priority: | P2 | ||||||||||
| Version: | 528+ (Nightly build) | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| URL: | http://dev.w3.org/csswg/css3-exclusions/ | ||||||||||
| Bug Depends on: | |||||||||||
| Bug Blocks: | 89256, 119274, 121615, 121616 | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Bem Jones-Bey
2012-11-20 15:14:58 PST
Created attachment 207764 [details]
proposed patch
Created attachment 211916 [details]
proposed patch
Comment on attachment 211916 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=211916&action=review > Source/WebCore/rendering/LineWidth.cpp:210 > +void LineWidth::updateCurrentLineSegment() > +{ > + if (ShapeInsideInfo* shapeInsideInfo = m_block.layoutShapeInsideInfo()) > + m_segment = shapeInsideInfo->currentSegment(); > +} Does this need to go inside #if ENABLE(CSS_SHAPES)? > Source/WebCore/rendering/RenderBlock.cpp:3387 > + // FIXME Bug 102949: Add support for shapes with multipe segments. multiple rather than multipe > Source/WebCore/rendering/RenderBlockLineLayout.cpp:2580 > + bool lineOverlapsWithFloat = lastFloatFromPreviousLine->y() < (block->logicalHeight() + lineLogicalHeight) && (block->logicalHeight() < lastFloatFromPreviousLine->maxY()); Parentheses here make this expression confusing. I, at least, would be able to read it bette without those parens. If you really do want parens, I would suggest one more set around the expression before the &&. > Source/WebCore/rendering/RenderBlockLineLayout.cpp:2587 > + if ((block->width() - lastFloatFromPreviousLine->maxX()) < minWidth) Again, I find the parentheses make it harder to read this expression. Comment on attachment 211916 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=211916&action=review Here's the feedback we talked about in person. > LayoutTests/fast/shapes/shape-inside/shape-inside-left-float-in-lower-left-triangle-inline-content-expected.html:1 > +<html> You should put <!DOCTYPE html> at the top of all of your test files, unless you have a good reason not to. (Same feedback on all of the test files) > Source/WebCore/rendering/LineWidth.cpp:206 > +void LineWidth::updateCurrentLineSegment() I think that updateCurrentShapeSegment would be a clearer name. > Source/WebCore/rendering/RenderBlockLineLayout.cpp:2574 > +static void adjustLineTopAndBlockLogicalHeightForShapeInsideWithFloats(RenderBlock* block, const FloatingObject* lastFloatFromPreviousLine, const WordMeasurements& wordMeasurements, LineWidth& width, bool isFirstLine) This name makes me think that this method is trying to do too many things. Maybe there's a way to break it up into multiple methiods? Comment on attachment 211916 [details] proposed patch Attachment 211916 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/1877799 Created attachment 211932 [details] incorporating comments (In reply to comment #3) > (From update of attachment 211916 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=211916&action=review > Does this need to go inside #if ENABLE(CSS_SHAPES)? You're right! I added the missing guard to the patch. > > Source/WebCore/rendering/RenderBlock.cpp:3387 > > + // FIXME Bug 102949: Add support for shapes with multipe segments. > > multiple rather than multipe Fixed. > > Source/WebCore/rendering/RenderBlockLineLayout.cpp:2580 > > + bool lineOverlapsWithFloat = lastFloatFromPreviousLine->y() < (block->logicalHeight() + lineLogicalHeight) && (block->logicalHeight() < lastFloatFromPreviousLine->maxY()); > > Parentheses here make this expression confusing. I, at least, would be able to read it bette without those parens. If you really do want parens, I would suggest one more set around the expression before the &&. Fixed. > > Source/WebCore/rendering/RenderBlockLineLayout.cpp:2587 > > + if ((block->width() - lastFloatFromPreviousLine->maxX()) < minWidth) > > Again, I find the parentheses make it harder to read this expression. Fixed too. (In reply to comment #4) > (From update of attachment 211916 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=211916&action=review > > You should put <!DOCTYPE html> at the top of all of your test files, unless you have a good reason not to. (Same feedback on all of the test files) Done. > > Source/WebCore/rendering/LineWidth.cpp:206 > > +void LineWidth::updateCurrentLineSegment() > > I think that updateCurrentShapeSegment would be a clearer name. I updated the function name. > > Source/WebCore/rendering/RenderBlockLineLayout.cpp:2574 > > +static void adjustLineTopAndBlockLogicalHeightForShapeInsideWithFloats(RenderBlock* block, const FloatingObject* lastFloatFromPreviousLine, const WordMeasurements& wordMeasurements, LineWidth& width, bool isFirstLine) > > This name makes me think that this method is trying to do too many things. Maybe there's a way to break it up into multiple methiods? I renamed the function. I'll probably split the method up in one of the following floats-shape-inside patches. Thank you guys to look at the patch. I'm giving it to the EWS now, then land. Comment on attachment 211932 [details] incorporating comments Clearing flags on attachment: 211932 Committed r156022: <http://trac.webkit.org/changeset/156022> All reviewed patches have been landed. Closing bug. |