Bug 98676 - [CSS Exclusions] shape-outside on floats for polygon shapes
Summary: [CSS Exclusions] shape-outside on floats for polygon shapes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Bem Jones-Bey
URL: http://dev.w3.org/csswg/css3-exclusio...
Keywords:
Depends on:
Blocks: 98664
  Show dependency treegraph
 
Reported: 2012-10-08 11:42 PDT by Bem Jones-Bey
Modified: 2013-03-05 10:21 PST (History)
5 users (show)

See Also:


Attachments
Patch (35.19 KB, patch)
2013-03-01 11:12 PST, Bem Jones-Bey
no flags Details | Formatted Diff | Diff
Updated Patch (35.35 KB, patch)
2013-03-01 13:40 PST, Bem Jones-Bey
no flags Details | Formatted Diff | Diff
Updated Patch (35.53 KB, patch)
2013-03-01 14:36 PST, Bem Jones-Bey
no flags Details | Formatted Diff | Diff
Updated Patch (36.65 KB, patch)
2013-03-04 21:20 PST, Bem Jones-Bey
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Bem Jones-Bey 2012-10-08 11:42:16 PDT
This bug tracks the implementation of shape-outside on floats for polygon shape outsides.
Comment 1 Bem Jones-Bey 2013-03-01 11:12:07 PST
Created attachment 190995 [details]
Patch

Initial fix
Comment 2 WebKit Review Bot 2013-03-01 11:55:09 PST
Comment on attachment 190995 [details]
Patch

Attachment 190995 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/16857091
Comment 3 WebKit Review Bot 2013-03-01 12:09:33 PST
Comment on attachment 190995 [details]
Patch

Attachment 190995 [details] did not pass cr-linux-debug-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/16882002
Comment 4 Bem Jones-Bey 2013-03-01 13:40:48 PST
Created attachment 191027 [details]
Updated Patch

Maybe this will make the Linux Chromium bots happy?
Comment 5 Bem Jones-Bey 2013-03-01 14:36:44 PST
Created attachment 191038 [details]
Updated Patch

The last one had bad luck, lets try again to make the Linux Chromium bots happy?
Comment 6 Dave Hyatt 2013-03-04 14:52:59 PST
Comment on attachment 191038 [details]
Updated Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=191038&action=review

r- for the writing mode issue, since that looks wrong to me.

> Source/WebCore/rendering/ExclusionShapeInfo.h:92
> -    LayoutUnit shapeLogicalRight() const { return computedShape()->shapeLogicalBoundingBox().y() + logicalLeftOffset(); }
> +    LayoutUnit shapeLogicalRight() const { return computedShape()->shapeLogicalBoundingBox().maxX() + logicalLeftOffset(); }

LOL.

> Source/WebCore/rendering/RenderBlock.h:1143
> +        const FloatingObject* lastFloat() const { return m_last; }

I think it is worth a comment explaining what this is.

> Source/WebCore/rendering/RenderBlock.h:1151
> +        mutable const FloatingObject* m_last;

Ditto.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:181
> +        shapeOutsideInfo->computeSegmentsForLine(m_block->logicalHeight() - newFloat->y(), logicalHeightForLine(m_block, m_isFirstLine));

This looks wrong. I suspect you mean m_block->logicalTopForFloat(newFloat) rather than newFloat->y().

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:188
> +        if (shapeOutsideInfo)
> +            newLeft += shapeOutsideInfo->logicalRightOffsetForLine();

This looks confusing to me. The only reason we're getting the right edge of the float is that is where the left edge of the line begins (i.e., after the right edge of the float). 

It reads very confusingly that you are calling a method named logicalRightOffsetForLine. If it's a delta applied to the float's right edge, then this method name seems very strange.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:199
> +#if ENABLE(CSS_EXCLUSIONS)
> +        if (shapeOutsideInfo)
> +            newRight += shapeOutsideInfo->logicalLeftOffsetForLine();
> +#endif

Same here. This seems very confusing.
Comment 7 Bem Jones-Bey 2013-03-04 21:20:16 PST
Created attachment 191394 [details]
Updated Patch

Fix writing mode issues, add explanatory comments, change method name to hopefully make more sense.
Comment 8 Dave Hyatt 2013-03-05 09:47:34 PST
Comment on attachment 191394 [details]
Updated Patch

r=me
Comment 9 WebKit Review Bot 2013-03-05 10:21:41 PST
Comment on attachment 191394 [details]
Updated Patch

Clearing flags on attachment: 191394

Committed r144776: <http://trac.webkit.org/changeset/144776>
Comment 10 WebKit Review Bot 2013-03-05 10:21:44 PST
All reviewed patches have been landed.  Closing bug.