Bug 100049 - [CSS Exclusions] Implement empty segments for multiple-segment shape-insides
Summary: [CSS Exclusions] Implement empty segments for multiple-segment shape-insides
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Bear Travis
URL:
Keywords:
Depends on:
Blocks: 89256
  Show dependency treegraph
 
Reported: 2012-10-22 16:26 PDT by Bear Travis
Modified: 2013-04-19 16:27 PDT (History)
3 users (show)

See Also:


Attachments
Initial Patch (21.68 KB, patch)
2013-04-16 16:43 PDT, Bear Travis
no flags Details | Formatted Diff | Diff
Incorporating feedback (22.22 KB, patch)
2013-04-18 13:24 PDT, Bear Travis
no flags Details | Formatted Diff | Diff
Updating patch (22.22 KB, patch)
2013-04-18 14:43 PDT, Bear Travis
hyatt: review+
Details | Formatted Diff | Diff
Factoring out line adjustment code (22.54 KB, patch)
2013-04-19 14:40 PDT, Bear Travis
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Bear Travis 2012-10-22 16:26:14 PDT
We at least need several tests to cover these cases
Comment 1 Bear Travis 2013-04-16 16:43:19 PDT
Created attachment 198452 [details]
Initial Patch
Comment 2 Hans Muller 2013-04-17 16:42:32 PDT
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)
Comment 3 Bear Travis 2013-04-18 13:24:50 PDT
Created attachment 198757 [details]
Incorporating feedback
Comment 4 Benjamin Poulain 2013-04-18 14:00:49 PDT
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()
Comment 5 Bear Travis 2013-04-18 14:43:46 PDT
Created attachment 198764 [details]
Updating patch
Comment 6 Dave Hyatt 2013-04-19 10:48:32 PDT
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.
Comment 7 Bear Travis 2013-04-19 14:40:17 PDT
Created attachment 198915 [details]
Factoring out line adjustment code
Comment 8 Bear Travis 2013-04-19 15:45:21 PDT
Comment on attachment 198915 [details]
Factoring out line adjustment code

Adding to commit queue.
Comment 9 WebKit Commit Bot 2013-04-19 16:27:12 PDT
Comment on attachment 198915 [details]
Factoring out line adjustment code

Clearing flags on attachment: 198915

Committed r148781: <http://trac.webkit.org/changeset/148781>
Comment 10 WebKit Commit Bot 2013-04-19 16:27:14 PDT
All reviewed patches have been landed.  Closing bug.