Summary: | [CSS Exclusions] Implement empty segments for multiple-segment shape-insides | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Bear Travis <betravis> | ||||||||||
Component: | CSS | Assignee: | Bear Travis <betravis> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | commit-queue, esprehn+autocc, giles_joplin | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Bug Depends on: | |||||||||||||
Bug Blocks: | 89256 | ||||||||||||
Attachments: |
|
Description
Bear Travis
2012-10-22 16:26:14 PDT
Created attachment 198452 [details]
Initial Patch
Source/WebCore/ChangeLog > Shape-insides do not require content in every segment. I'm not exactly sure what this means. Some line segments within a shape may be empty because because the interior of the shape does not completely overlap the line? Source/WebCore/rendering/RenderBlockLineLayout.cpp > float minWidth = 0; > for (size_t i = 0; i < wordMeasurements.size(); i++) { > minWidth = wordMeasurements[i].width; > if (minWidth > 0) > break; > } Maybe move this litte loop into an aptly named inline function, so: float minWidth = firstPositiveWidth(wordMeasurements); And I'm hoping that there's no such thing as negative WordMeasurement.width values. Note also: the cosmetic feedback I've been getting about this kind of loop is to use unsigned and ++i, as in: for (unsigned i = 0; i < wordMeasurements.size(); ++i) Created attachment 198757 [details]
Incorporating feedback
Comment on attachment 198757 [details] Incorporating feedback View in context: https://bugs.webkit.org/attachment.cgi?id=198757&action=review Some minor comments bellow. > Source/WebCore/rendering/RenderBlockLineLayout.cpp:1629 > + for (unsigned i = 0; i < wordMeasurements.size(); i++) { unsigned-> size_t i++ -> ++i > Source/WebCore/rendering/RenderBlockLineLayout.cpp:1715 > + ASSERT(minWidth || !wordMeasurements.size()); !wordMeasurements.size() -> wordMeasurements.isEmpty() Created attachment 198764 [details]
Updating patch
Comment on attachment 198764 [details] Updating patch View in context: https://bugs.webkit.org/attachment.cgi?id=198764&action=review r=me, although see comment. > Source/WebCore/rendering/RenderBlockLineLayout.cpp:1722 > + if (exclusionShapeInsideInfo && end == resolver.position()) { > + float minWidth = firstPositiveWidth(wordMeasurements); > + ASSERT(minWidth || wordMeasurements.isEmpty()); > + LayoutUnit nextLineHeight; > + if (minWidth > 0 && exclusionShapeInsideInfo->adjustLogicalLineTop(minWidth)) > + nextLineHeight = exclusionShapeInsideInfo->logicalLineTop() - absoluteLogicalTop; > + else > + nextLineHeight = exclusionShapeInsideInfo->shapeLogicalBottom() - absoluteLogicalTop; > + end = restartLayoutRunsAndFloatsInRange(logicalHeight(), nextLineHeight, lastFloatFromPreviousLine, resolver, oldEnd); > continue; I think it would be nice to put this into a helper function, even if it's just a static function that is up with firstPositiveWidth's definition. Created attachment 198915 [details]
Factoring out line adjustment code
Comment on attachment 198915 [details]
Factoring out line adjustment code
Adding to commit queue.
Comment on attachment 198915 [details] Factoring out line adjustment code Clearing flags on attachment: 198915 Committed r148781: <http://trac.webkit.org/changeset/148781> All reviewed patches have been landed. Closing bug. |