Bug 89259 – [CSS Exclusions] Enable shape-inside for simple rectangles

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Bear Travis]

Enable shape-inside for rectangles (without corner radius)
See http://dev.w3.org/csswg/css3-exclusions/#shapes-from-svg-syntax

[Bear Travis]

Created attachment 147935 [details]
Prototype Patch

Initial prototype. Not yet ready for review / commit.

[Early Warning System Bot]

Comment on attachment 147935 [details]
Prototype Patch

Attachment 147935 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12974004

[Early Warning System Bot]

Comment on attachment 147935 [details]
Prototype Patch

Attachment 147935 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12974005

[WebKit Review Bot]

Comment on attachment 147935 [details]
Prototype Patch

Attachment 147935 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12967706

[Alexandru Chiculita]

Comment on attachment 147935 [details]
Prototype Patch

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

I've did a first pass on reviewing things. I will take a look at the actual layout code later today.

> Source/WebCore/rendering/InlineFlowBox.cpp:359
> +float InlineFlowBox::placeBoxesInInlineDirection(float logicalLeft, bool& needsWordSpacing, GlyphOverflowAndFallbackFontsMap& textBoxDataMap, const Vector<SegmentInfo>* segments)

It would look better to typedef Vector<SegmentInfo>.

> Source/WebCore/rendering/InlineFlowBox.cpp:376
> +            if (segments && segmentIndex < segments->size()) {

Is there any IFDEF that we need for this feature?

> Source/WebCore/rendering/InlineFlowBox.cpp:378
> +                if (segmentPosition.m_obj == text->renderer() && segmentPosition.m_pos == text->start()) {

Maybe we could move the whole if (segments) outside the isText() because I suppose we need to fix it for other objects as well.

> Source/WebCore/rendering/InlineFlowBox.cpp:379
> +                    logicalLeft = segments->at(segmentIndex).left;

Is there a chances that the old logical left would be larger then what we read from the segment.left?
Maybe an assert could catch bad behavior.

> Source/WebCore/rendering/InlineFlowBox.cpp:380
> +                    segmentIndex++;

>> logicalLeft = flow->placeBoxesInInlineDirection(logicalLeft, needsWordSpacing, textBoxDataMap);

Should we transport the segment index to the nested calls to placeBoxesInInlineDirection ?
Maybe we could wrap Vector<SegmentInfo> into a an object that we pass around.

> Source/WebCore/rendering/InlineFlowBox.h:173
> +    float placeBoxesInInlineDirection(float logicalLeft, bool& needsWordSpacing, GlyphOverflowAndFallbackFontsMap&, const Vector<SegmentInfo>* = 0);

Use typedef for the vector.

> Source/WebCore/rendering/RenderBlock.h:928
> +    RootInlineBox* createLineBoxesFromBidiRuns(BidiRunList<BidiRun>&, const InlineIterator& end, LineInfo&, VerticalPositionCache&, BidiRun* trailingSpaceRun, const Vector<SegmentInfo>&);

Use typedef for vector<>.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:82
> +    LineWidth(RenderBlock* block, bool isFirstLine, const SegmentInfo* segment)

Why not just make the first LineWidth constructor take a default segment = 0 ?

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:774
> +    float logicalLeft = segments.size() ? LayoutUnit(segments[0].left) : pixelSnappedLogicalLeftOffsetForLine(logicalHeight(), lineInfo.isFirstLine());

You are not taking into account the that are already on the line here, and floats may overlap your first segment completely, so you might need to look at next ones to catch the first pixel.
Also it seems like the other functions are called "pixelSnapped*", do you need to do something special about the segment.left to snap it to pixels?

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1240
> +static bool getSegmentsForLine(RenderBlock* block, Vector<SegmentInfo>& segments)

getSegmentsForLine might better be called compute segments for line.

Is there a reason this method is not actually RenderBlock::computeSegmentsForLine(Vector<SegmentInfo>& segments) instead?

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1245
> +    if (!block->isHorizontalWritingMode())

FIXME with bug number?

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1252
> +    if (!shape || shape->type() != CSSWrapShape::CSS_WRAP_SHAPE_RECTANGLE)

Can we make this a switch where we will add more types?

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1274
> +static bool getShapeBounds(RenderBlock* block, int& top, int& bottom)

I first thought this is going to do a complete bounds rect, but it is only computing the vertical bounds. What about computeShapeVerticalBounds?

Why not make this a member of the RenderBlock?

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1279
> +    if (!block->isHorizontalWritingMode())

FIXME?

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1286
> +    if (!shape || shape->type() != CSSWrapShape::CSS_WRAP_SHAPE_RECTANGLE)

Move this to a switch(shape->type()).

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1310
> +    int shapeTop, shapeBottom;
> +    bool hasShape = getShapeBounds(this, shapeTop, shapeBottom);

Having these two on the stack seems like we could have a helper class to do just that. I assume that when exclusions would come we would have a better structure to make this faster.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1333
> +            if (logicalHeight() < shapeTop) {

Should this stay outside the loop? It seems like we are only going to do it once.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1337
> +            if (logicalHeight() < shapeBottom)

Can we just make hasShape = false so that we don't bother to do anything after that?
What happens with the lines after the shapeBottom is hit? Should it just overflow? Where should it start to overflow: from the borderline?

> Source/WebCore/rendering/SegmentInfo.h:28
> +    int left;

Should it be LayoutUnit instead?

[Bear Travis]

Created attachment 149133 [details]
Revised patch

Incorporating feedback from Alexandru.
Main changes:
* Refactoring the line segment & shape calculations into the helper ShapeResolver class
* Adding support for the CSS_EXCLUSIONS compile flag
* Removing the changes to InlineFlowBox. These are only necessary when we start supporting multiple segments per line.

The patch does not yet deal with the interaction of line segments and floats (Bug 89261).

[Bear Travis]

Created attachment 149590 [details]
Revised patch

Revised patch, optimizing the allocation/deallocation of ShapeResolvers.

[WebKit Review Bot]

Attachment 149590 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
Source/WebCore/rendering/RenderBlockLineLayout.cpp:112:  Non-label code inside switch statements should be indented.  [whitespace/indent] [4]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Bear Travis]

Created attachment 149592 [details]
Revised patch

Fixing up the style failure

[Early Warning System Bot]

Comment on attachment 149592 [details]
Revised patch

Attachment 149592 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13093570

[Build Bot]

Comment on attachment 149592 [details]
Revised patch

Attachment 149592 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13093571

[Early Warning System Bot]

Comment on attachment 149592 [details]
Revised patch

Attachment 149592 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13095535

[Gyuyoung Kim]

Comment on attachment 149592 [details]
Revised patch

Attachment 149592 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13101523

[Bear Travis]

Created attachment 152369 [details]
Revised patch

Updating patch, fixing build failures, and modifying the changelogs

[Early Warning System Bot]

Comment on attachment 152369 [details]
Revised patch

Attachment 152369 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13242272

[Early Warning System Bot]

Comment on attachment 152369 [details]
Revised patch

Attachment 152369 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13242281

[Bear Travis]

Created attachment 152380 [details]
Revised patch

Fixing misspelled header file

[Bear Travis]

Created attachment 152662 [details]
Move to length based wrap shapes

Incorporating the changes from bug 89670, making the wrap shapes in RenderStyle be length based

[Alexandru Chiculita]

Comment on attachment 152662 [details]
Move to length based wrap shapes

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

Looks good, I've added some comments below. 

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:173
> +    if (gShapeResolverMap) {

nit: In WebKit we usually exit early:
if (!gShapeResolverMap)
   return;

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:266
> +        m_left = m_segment->left;

Don't you also need to take into account the logicalLeftOffsetForLine? It might be that a float would move this to the right. Same for m_right.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:478
> +

nit: remove empty changes.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:918
> +    if (hasShapeInside() && (shapeResolver = gShapeResolverMap->get(this)) && shapeResolver->hasSegments()) {

nit: Make a function for hasShapeInside() && (shapeResolver = gShapeResolverMap->get(this))

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:919
> +        logicalLeft = roundToInt(shapeResolver->segments()[0].left);

Same comment about the floats. If you don't support that in this patch, I think it's worth adding a bug link.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:927
> +    logicalLeft = pixelSnappedLogicalLeftOffsetForLine(logicalHeight(), lineInfo.isFirstLine(), lineLogicalHeight);

Can you rewrite this, so that we don't duplicate code? Anyway I think you need the logical left/right to avoid the floats, so you might better just let the initial code as is and remove both else branches.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1405
> +    ShapeResolver* shapeResolver = 0;

I think we can extract the whole code into ShapeResolver* RenderBlock::updateShapeResolverIfNeeded()

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1407
> +    if (ShapeResolver::shapeResolverNeeded(this))

Why don't you make shapeResolveNeeded a method of the RenderBlock instead? It seems like you have a middle man.
I would also name it the other way around: needsShapeResolver.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1412
> +    setHasShapeInside(shapeResolver);

setHasShapeInside should sit inside createOrUpdateShapeResolver and removeShapeResolver

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1414
> +    if (shapeResolver && logicalHeight() < shapeResolver->shapeTop())

I think this needs a comment saying that you jump directly to the first line inside the shape.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:2312
> +    if (m_block->hasShapeInside() && (shapeResolver = gShapeResolverMap->get(m_block)) && shapeResolver->hasSegments())

nit: ditto -> shapeResolver = gShapeResolverMap->get(m_block)

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:2313
> +        segment = &shapeResolver->segments()[0];

nit: FIXME about handling more segments here?

> LayoutTests/ChangeLog:11
> +        * fast/exclusions/shape-inside/shape-inside-inline-elements-expected.html: Added.

Can you also make a test where the text is changed dynamically? There's no code dealing with incremental layouts. I would add a bug for that and keep it in a different patch.

> LayoutTests/fast/exclusions/shape-inside/shape-inside-inline-elements-expected.html:1
> +<html>

nit: New tests should use HTML5 doctype.

> LayoutTests/fast/exclusions/shape-inside/shape-inside-inline-elements-expected.html:3
> +<style>

nit: There's no indentation.

> LayoutTests/fast/exclusions/shape-inside/shape-inside-inline-elements-expected.html:4
> +* {

nit: Why do you need this?

> LayoutTests/fast/exclusions/shape-inside/shape-inside-inline-elements.html:26
> +Should display a 2x2 grid of blue squares

There's no assert about the shape inside, meaning that I cannot tell by looking at the test if it works or not. Same applies for the other tests. Maybe you could add some border to make it easy to depict if works or not.

[Bear Travis]

Created attachment 153613 [details]
Updating patch from comments

Incorporating Alex's feedback. Inline content respects floats, but floats are not yet positioned using shape-inside (bug 91906). I have added a bug for testing incremental layout (bug 91879). The inline element test was meant to test wrapping on elements besides text, but I can remove the test if it seems superfluous.

[WebKit Review Bot]

Comment on attachment 153613 [details]
Updating patch from comments

Attachment 153613 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13301534

New failing tests:
fast/exclusions/shape-inside/shape-inside-floats-simple.html

[WebKit Review Bot]

Created attachment 153625 [details]
Archive of layout-test-results from gce-cr-linux-07

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-07  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Bear Travis]

Created attachment 153935 [details]
Updated patch

Incorporating additional feedback, mainly refactoring WrapShapeInfo into separate files

[Early Warning System Bot]

Comment on attachment 153935 [details]
Updated patch

Attachment 153935 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13324433

[Bear Travis]

Created attachment 153937 [details]
Adding makefile entries

Updating the build files for new WrapShapeInfo files

[WebKit Review Bot]

Comment on attachment 153937 [details]
Adding makefile entries

Attachment 153937 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13334175

New failing tests:
fast/exclusions/shape-inside/shape-inside-floats-simple.html

[WebKit Review Bot]

Created attachment 153944 [details]
Archive of layout-test-results from gce-cr-linux-05

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-05  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Alexandru Chiculita]

Comment on attachment 153937 [details]
Adding makefile entries

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

Looks better, small nits though.

> Source/WebCore/rendering/RenderBlock.cpp:324
> +#if ENABLE(CSS_EXCLUSIONS)

I think you also need to update RenderStyle::diff to trigger a layout not just a repaint for the shape-inside change.

Look for https://bugs.webkit.org/show_bug.cgi?id=62991 in RenderStyle.cpp

> Source/WebCore/rendering/RenderBlock.cpp:327
> +    if (style()->wrapShapeInside() != (oldStyle ? oldStyle->wrapShapeInside() : 0))

Don't you also need to compare the values, not just the pointers?

> Source/WebCore/rendering/RenderBlock.cpp:1374
> +    setLogicalHeight(MAX_LAYOUT_UNIT / 2);

I think this should actually be 0 instead.

If the height is auto and the shape uses percentages, it should resolve to 0. Do you have any tests for that?

> Source/WebCore/rendering/RenderBlock.cpp:7423
> +WrapShapeInfo* RenderBlock::wrapShapeInfo() const

I assume this would be called lots of times, so can you move it to the header and exit early when the bitfield is set to false? The compiler would inline that check.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:799
> +    if (wrapShapeInfo && wrapShapeInfo->wrapShapeInfoState() == WrapShapeInfo::INSIDE) {

I would change the name of the wrapShapeInfoState. Initially I thought you're only treating the shape-inside. Also WrapShapeInfo::INSIDE can be a little more verbose:
Maybe this would work better:
wrapShapeInfo->lineState() == WrapShapeInfo::LINE_INSIDE_SHAPE

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1308
> +        if (wrapShapeInfo)
> +            wrapShapeInfo->computeSegmentsForLine(logicalHeight());

What happens with the space between the bottom of the shape and the bottom of the border-box. Should it fill with text or not? I assume that we will have to skip that space. If so there's no code to do that.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:2179
> +    if (wrapShapeInfo && wrapShapeInfo->wrapShapeInfoState() == WrapShapeInfo::INSIDE) {

Ditto.

> Source/WebCore/rendering/WrapShapeInfo.cpp:87
> +        return (shape && shape->type() == WrapShape::WRAP_SHAPE_RECTANGLE);

nit: too many spaces here?

> Source/WebCore/rendering/WrapShapeInfo.cpp:103
> +    if (!s_wrapShapeInfoMap || !block->hasShapeInside())

Can you assert that block->hasShapeInside() is false when there is no s_wrapShapeInfoMap?

> Source/WebCore/rendering/WrapShapeInfo.cpp:120
> +    return m_lineTop >= m_shapeTop && m_lineTop < m_shapeTop + m_shapeHeight && m_segments.size();

You have wrapShapeInfoState. Why not use that? Or at least add some parentheses, it is a little hard to follow.

> Source/WebCore/rendering/WrapShapeInfo.cpp:157
> +    if (m_lineTop >= m_shapeTop && m_lineTop < m_shapeTop + m_shapeHeight) {

Ditto.

> Source/WebCore/rendering/WrapShapeInfo.cpp:166
> +WrapShapeInfo::WrapShapeInfoState WrapShapeInfo::wrapShapeInfoState()

Initially I thought it is related to the shape type. I would change this to lineState() instead. Also drop the the wrapShapeInfo prefix.

[Bear Travis]

Created attachment 154173 [details]
Updated patch

Incorporating feedback. Created bug 92164 to track potential optimizations with style diff, and bug 92165 to track overflow behavior with wrapshapes

[Bear Travis]

Created attachment 154191 [details]
Fixing project

References in the project needed cleanup

[Bear Travis]

Created attachment 155342 [details]
Updating to head

Merging with current version of the code

[Build Bot]

Comment on attachment 155342 [details]
Updating to head

Attachment 155342 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13390487

[Bear Travis]

Created attachment 155369 [details]
Fix windows project

Fixing the incorrect spacing in the new windows build entries

[Build Bot]

Comment on attachment 155369 [details]
Fix windows project

Attachment 155369 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13396499

[Julien Chaffraix]

Comment on attachment 155369 [details]
Fix windows project

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

First high level comment, the spec is pretty rough and should be updated before we implement it. shape-inside seems to be on the safe side so this patch is OK.

> Source/WebCore/rendering/RenderBlock.cpp:211
> +    WrapShapeInfo::removeWrapShapeInfoForRenderBlock(this);

It's annoying that we need a static function. Those tends to be bad and cause lots of pain (see for example counters).

> Source/WebCore/rendering/RenderObject.h:1044
> +        ADD_BOOLEAN_BITFIELD(hasShapeInside, HasShapeInside);

The bar for adding a new boolean to RenderObject is high and I don't think this meets the requirements. Your logic could be completely based on RenderStyle::shapeInside() with some modification to the architecture.

> Source/WebCore/rendering/WrapShapeInfo.cpp:75
> +static inline bool needsWrapShapeInfo(RenderBlock* block)

This function is badly named. It doesn't underline what you are trying to achieve (which is basically filtering the cases you can handle). Here is a better list of names:
* isWrapShapeInfoDisabledForBlock()
* isWrapShapeInfoEnabledForBlock()

As a whole I think this should be done in RenderStyle::wrapShapeInside(). This would simplify the code and remove a lot of the complexity.

> Source/WebCore/rendering/WrapShapeInfo.cpp:78
> +    if (!RuntimeEnabledFeatures::cssExclusionsEnabled())
> +        return false;

I don't think this is right: normally you *should* disable CSS parsing to avoid breaking feature detection in CSS. This means that this code cannot be executed if exclusion is disabled.

> Source/WebCore/rendering/WrapShapeInfo.cpp:89
> +void WrapShapeInfo::updateWrapShapeInfoShapeForRenderBlock(RenderBlock* block)

I am really not supportive of this naming. updateWrapShapeInfoShapeForRenderBlock doesn't really say anything about what it does and the function is doing too much stuff. It would be better if we had a create and destroy function and that's it. If you need an update then it should only dirty your function.

> Source/WebCore/rendering/WrapShapeInfo.cpp:102
> +    ASSERT(!s_wrapShapeInfoMap || !block->hasShapeInside());

I don't understand this ASSERT. This is the only place where you reset the |hasShapeInside| flag which means you will trigger this ASSERT.

> Source/WebCore/rendering/WrapShapeInfo.cpp:149
> +    default:

Better to avoid |default|, it defeats the compile time check that we handle all types.

> Source/WebCore/rendering/WrapShapeInfo.cpp:150
> +        ASSERT_NOT_REACHED();

AFAICT we can reach other shape here as the parsing is implemented. It's better to use notImplemented() in this case.

> Source/WebCore/rendering/WrapShapeInfo.h:48
> +typedef HashMap<const RenderBlock*, WrapShapeInfo*> WrapShapeInfoMap;

Better to use HashMap<const RenderBlock*, OwnPtr<WrapShareInfo> > here.

> Source/WebCore/rendering/WrapShapeInfo.h:61
> +    float shapeTop() { return m_shapeTop; }

You are not coherent here in terms of units. Most of the layout code uses LayoutUnit, yet you return a float here. As you are not touching line layout, AFAICT this should be only LayoutUnit.

> Source/WebCore/rendering/WrapShapeInfo.h:62
> +    float shapeBottom() { return m_shapeTop + m_shapeHeight; }

Unused method.

> Source/WebCore/rendering/WrapShapeInfo.h:64
> +    bool hasSegments();
> +    SegmentList& segments() { ASSERT(hasSegments()); return m_segments; }

Those getters should be |const|.

> Source/WebCore/rendering/WrapShapeInfo.h:77
> +    LayoutUnit m_shapeLeft, m_shapeTop, m_shapeWidth, m_shapeHeight;

Please one member per line.

[Bear Travis]

Created attachment 156528 [details]
Incorporating feedback

Incorporating feedback from Julien. Largest changes are removing the shape inside bitfield from RenderObject, and moving the hashmap to use OwnPtr instead of raw pointers. Some wrap shapes are supported in CSS parsing but not yet in layout, making isWrapShapeInfoEnabledForBlock a method that is currently necessary but may not be in the future.

[Build Bot]

Comment on attachment 156528 [details]
Incorporating feedback

Attachment 156528 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13435268

[Bear Travis]

Created attachment 156593 [details]
Conditionally wrapping WrapShapeInfo

Removing WrapShapeInfo code when CSS_EXCLUSIONS is disabled

[Julien Chaffraix]

Comment on attachment 156593 [details]
Conditionally wrapping WrapShapeInfo

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

I haven't looked at the inline layout part in details.

> Source/WebCore/ChangeLog:71
> +        (WebCore::RenderStyle::diff):

The function level entries should be filled in the final patch.

> Source/WebCore/rendering/RenderBlock.cpp:64
> +#if ENABLE(CSS_EXCLUSIONS)

As you are using runtime flags, you could drop the compile-time guard. It's a pity as it would make the code more readable.

Unfortunately, the parsing is guarded in a similar fashion so we have to keep the pattern.

> Source/WebCore/rendering/RenderBlock.cpp:211
> +    WrapShapeInfo::removeWrapShapeInfoForRenderBlock(this);

This is potentially dangerous. As long as removeWrapShapeInfoForRenderBlock is limited to removing the info in our map, it's fine. If it needs to call any virtual method, it would be bad though.

> Source/WebCore/rendering/RenderBlock.cpp:329
> +        WrapShapeInfo* wrapShapeInfo = this->wrapShapeInfo();
> +        if (WrapShapeInfo::isWrapShapeInfoEnabledForBlock(this)) {

I still think this is an anti-pattern to return a wrapShapeInfo that disable. I would just implement the filtering in wrapShapeInfo (I would do it in RenderStyle) to avoid an extra step.

> Source/WebCore/rendering/RenderBlock.cpp:331
> +                wrapShapeInfo->setNeedsUpdate(true);

Booleans should usually be avoided as it makes the call site less readable. For example, can you tell me what repaint(true) means in WebKit without looking?

Here the boolean is unneeded as you only ever set it to true anyway.

> Source/WebCore/rendering/RenderBlock.cpp:336
> +    }

Maybe better to move that to an updateWrapeShapeInfoAfterStyleChange (not a huge fan of update but at least it's consistent with some pattern we have).

> Source/WebCore/rendering/RenderBlock.cpp:1388
> +    setLogicalHeight(0);
> +    computeLogicalHeight();
> +    wrapShapeInfo->computeShapeSize(logicalWidth(), logicalHeight());
> +    setLogicalHeight(oldHeight);
> +    setLogicalTop(oldLogicalTop);

There is a pattern here where you do another computeLogicalHeight for each new feature we support. It's annoying that we have to recompute the logical height over and over. Couldn't we find a way to compute it once and for all *before* we layout our children.

> Source/WebCore/rendering/RenderBlock.h:396
> +    bool hasShapeInside() const { return style() && style()->wrapShapeInside(); }

style() is never NULL except the first time styleWillChange is called. We don't need to pay the price of the NULL-check for the common case. I would say the need for this function is fairly small as it doesn't add much.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:2188
> +    LineWidth width = LineWidth(m_block, lineInfo.isFirstLine(), segment);

Couldn't we let LineWidth constructor automatically handle this? That would remove the need to the constructor's signature.

> Source/WebCore/rendering/WrapShapeInfo.cpp:37
> +#include "RuntimeEnabledFeatures.h"

Unneeded #include.

> Source/WebCore/rendering/WrapShapeInfo.cpp:60
> +WrapShapeInfo* WrapShapeInfo::createWrapShapeInfoForRenderBlock(RenderBlock* block)

Nobody uses the returned value so it could be dropped. Not a huge fan of the 'create' in the method name but I didn't come up with something better.

> Source/WebCore/rendering/WrapShapeInfo.h:72
> +    void setNeedsUpdate(bool value) { m_needsUpdate = value; }

Generally the naming is poor. "update" means that the value is dirtied and needs to be recomputed and it would be nicer to use a more precise naming. My picks:
* dirtyCachedWrapShapeSize
* wrapShapeSizeNeedsRecomputing
* ...

[Bear Travis]

Created attachment 157367 [details]
Incorporating review feedback

Making changes suggested by jchaffraix

[Bear Travis]

(In reply to comment #39)
> > Source/WebCore/ChangeLog:71
> The function level entries should be filled in the final patch.
Added function level entries in the WebCore changelog

> > Source/WebCore/rendering/RenderBlock.cpp:64
> > +#if ENABLE(CSS_EXCLUSIONS)
Leaving the compile time guards in place. As mentioned, they are in the parsing code. Also, if disabled (say for a release), the goal is to remove as much of the exclusions code as possible.

> > Source/WebCore/rendering/RenderBlock.cpp:211
> > +    WrapShapeInfo::removeWrapShapeInfoForRenderBlock(this);
Leaving in place. This only removes the render block from the map, and does not call any virtual methods. I can alternately move this to willBeDestroyed, but ColumnInfo is also removed here.

> > Source/WebCore/rendering/RenderBlock.cpp:329
> > +        WrapShapeInfo* wrapShapeInfo = this->wrapShapeInfo();
> > +        if (WrapShapeInfo::isWrapShapeInfoEnabledForBlock(this)) { 
> I still think this is an anti-pattern...
Moved the isWrapShapeInfoEnabledForBlock check to createWrapShapeInfoForBlock.

> > Source/WebCore/rendering/RenderBlock.cpp:331
> > +                wrapShapeInfo->setNeedsUpdate(true);
> Booleans should usually be avoided as it makes the call site less readable.
Removed the boolean and renamed function to wrapShapeSizeNeedsRecomputing

> > Source/WebCore/rendering/RenderBlock.cpp:336
> > +    }
> 
> Maybe better to move that to an updateWrapeShapeInfoAfterStyleChange (not a huge fan of update but at least it's consistent with some pattern we have).
Done
 
> > Source/WebCore/rendering/RenderBlock.cpp:1388
> > +    setLogicalHeight(0);
> > +    ...
> > +    wrapShapeInfo->computeShapeSize(logicalWidth(), logicalHeight());
> > +    setLogicalHeight(oldHeight);
> > +    setLogicalTop(oldLogicalTop);
> 
> There is a pattern here where you do another computeLogicalHeight for each new feature we support. It's annoying that we have to recompute the logical height over and over. Couldn't we find a way to compute it once and for all *before* we layout our children.
Removed this extra function and just use logical height of 0 for now. Filed bug 93547 to resolve vertical height, making sure to do it in a single place. The refactoring will require touching the way regions does this, so I think it should be done as another patch.

> > Source/WebCore/rendering/RenderBlock.h:396
> > +    bool hasShapeInside() const { return style() && style()->wrapShapeInside(); }
> I would say the need for this function is fairly small as it doesn't add much.
Removed, along with the excessive style() check

> > Source/WebCore/rendering/RenderBlockLineLayout.cpp:2188
> > +    LineWidth width = LineWidth(m_block, lineInfo.isFirstLine(), segment);
> Couldn't we let LineWidth constructor automatically handle this? That would remove the need to the constructor's signature.
Done. This may need to be adjusted when we support multiple segments, but we can wait on that call.

> > Source/WebCore/rendering/WrapShapeInfo.cpp:37
> > +#include "RuntimeEnabledFeatures.h"
> Unneeded #include.
Removed

> > Source/WebCore/rendering/WrapShapeInfo.cpp:60
> > +WrapShapeInfo* WrapShapeInfo::createWrapShapeInfoForRenderBlock(RenderBlock* block)
> Nobody uses the returned value so it could be dropped. Not a huge fan of the 'create' in the method name but I didn't come up with something better.
Removed the return value. Left "create", though I could alternatively call it "reserve," "allocate," or "enter"? None of those seems better though.

> > Source/WebCore/rendering/WrapShapeInfo.h:72
> > +    void setNeedsUpdate(bool value) { m_needsUpdate = value; }
> Generally the naming is poor. 
Renamed to wrapShapeSizeNeedsRecomputing

Didn't do it in this patch, but what about changing the wrapShapeInfoMap to a static local, as in RenderCounter.cpp::counterMaps? I could potentially make it an OwnPtr. Either of these would remove the need for the delete call on the map.

[Bear Travis]

Created attachment 157500 [details]
Fixing buggy assert in WrapShapeInfo.cpp

[Bear Travis]

Created attachment 157809 [details]
updating patch

resolving merge conflict with a change in RenderBlockLineLayout

[Julien Chaffraix]

Comment on attachment 157809 [details]
updating patch

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

> Source/WebCore/rendering/RenderBlock.cpp:1376
> +    if (wrapShape != oldWrapShape) {

Early returns are preferred to nesting.

> Source/WebCore/rendering/RenderBlock.cpp:1382
> +            WrapShapeInfo* wrapShapeInfo = this->wrapShapeInfo();
> +            if (wrapShapeInfo)
> +                wrapShapeInfo->wrapShapeSizeNeedsRecomputing();
> +            else
> +                WrapShapeInfo::createWrapShapeInfoForRenderBlock(this);

I would rather see the following as it would simplify the code path:
WrapShapeInfo* wrapShapeInfo = ensureWrapShapeInfo();
wrapShapeInfo->wrapShapeSizeNeedsRecomputing();

> Source/WebCore/rendering/RenderBlock.cpp:1485
> +    // FIXME: 93547 resolve logical height for percentage based vertical lengths

// FIXME: Bug 93547: Resolve logical height for percentage based vertical lengths

(not repeated for all comment missing the word 'bug')

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:165
> +        computeAvailableWidthFromLeftAndRight();
> +        return;

Those 2 lines are unneeded as you would do exactly the same operations below. It's also better to avoid splitting our current code path.

> Source/WebCore/rendering/WrapShapeInfo.cpp:42
> +WrapShapeInfoMap* WrapShapeInfo::s_wrapShapeInfoMap = 0;

I completely agree with your proposal to move this to use DEFINE_STATIC_LOCAL. You wouldn't have to expose the internal getter and that would remove all the initialization / destruction / NULL checks below.

> Source/WebCore/rendering/WrapShapeInfo.cpp:61
> +    if (!isWrapShapeInfoEnabledForBlock(block)) {

Note that this doesn't work properly if you transition from a supported to an unsupported shape. This is why I was hinting at doing this check in RenderBlock::wrapShapeInfo and make all style changes use this method. This would ensure that we never use an unsupported shape.

Even if this code is temporary, it's important that it's somewhat bullet-proof.

> Source/WebCore/rendering/WrapShapeInfo.cpp:62
> +        removeWrapShapeInfoForRenderBlock(block);

This is unneeded AFAICT as you will never insert the shape if it's disabled.

> Source/WebCore/rendering/WrapShapeInfo.cpp:70
> +    WrapShapeInfo* wrapShapeInfo = new WrapShapeInfo(block);
> +    s_wrapShapeInfoMap->set(block, adoptPtr(wrapShapeInfo));

We usually have a pattern for creating an OwnPtr:

static PassOwnPtr<WrapShapeInfo> create(RenderBlock* block)
{
    return adoptPtr(new WrapShapeInfo(block));
}

This ensures that you never forget to call adoptPtr.

> Source/WebCore/rendering/WrapShapeInfo.cpp:97
> +    s_wrapShapeInfoMap->take(block);

remove is better as you don't need the return value, take is more if you want to do something with the value before it's destroyed.

> Source/WebCore/rendering/WrapShapeInfo.h:48
> +    LayoutUnit left;
> +    LayoutUnit right;

I would renamed those 2 to be logicalLeft and logicalRight to underline that they are not physical but logical coordinates.

> Source/WebCore/rendering/WrapShapeInfo.h:78
> +    static bool isWrapShapeInfoEnabledForBlock(const RenderBlock*);
> +    static WrapShapeInfoMap* s_wrapShapeInfoMap;

Anything that doesn't need to be known outside the implementation file doesn't need to be exposed here. Those 2, for example, could have a static binding inside WrapShapeInfo.cpp so that you never expose them.

> Source/WebCore/rendering/WrapShapeInfo.h:93
> +    // OwnPtr needs access to our destructor
> +    friend void WTF::deleteOwnedPtr<WrapShapeInfo>(WrapShapeInfo*);

Better to just expose the destructor then.

[Bear Travis]

Created attachment 158090 [details]
Incorporating feedback

Incorporating Julien's feedback. The WrapShapeInfo map has been moved to a static local in WrapShapeInfo.cpp. Fixme notes now include 'Bug #'. RenderBlock::wrapShapeInfo() now returns null when a wrap shape cannot (yet) be used for layout. Other small refactorings.

[WebKit Review Bot]

Comment on attachment 158090 [details]
Incorporating feedback

Attachment 158090 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13492270

New failing tests:
fast/forms/form-hides-table.html
fullscreen/fullscreen-child-not-allowed-crash.html

[WebKit Review Bot]

Created attachment 158122 [details]
Archive of layout-test-results from gce-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Bear Travis]

Created attachment 159203 [details]
Updating patch

Merging with current version of code

[WebKit Review Bot]

Comment on attachment 159203 [details]
Updating patch

Attachment 159203 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13518817

New failing tests:
fast/forms/form-hides-table.html
fullscreen/fullscreen-child-not-allowed-crash.html

[WebKit Review Bot]

Created attachment 159234 [details]
Archive of layout-test-results from gce-cr-linux-08

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-08  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Bear Travis]

Created attachment 159482 [details]
Fixing nullptr error on destruction

Moving wrapshapeinfo removal to willBeDestroyed, and checking that style exists before looking up style->wrapShapeInside.

[Daniel Bates]

Comment on attachment 159482 [details]
Fixing nullptr error on destruction

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

I didn't review this patch for correctness. I have some minor nits.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:6436
> +		FD748ABF15BF74ED0059CF0D /* WrapShapeInfo.cpp in Sources */ = {isa = PBXBuildFile; fileRef = FD748ABD15BF74ED0059CF0D /* WrapShapeInfo.cpp */; };
> +		FD748AC015BF74ED0059CF0D /* WrapShapeInfo.h in Headers */ = {isa = PBXBuildFile; fileRef = FD748ABE15BF74ED0059CF0D /* WrapShapeInfo.h */; settings = {ATTRIBUTES = (Private, ); }; };

Nit: Nit: Please add these files to the list such that the list is in alphabetical order.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:13836
> +		FD748ABD15BF74ED0059CF0D /* WrapShapeInfo.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = WrapShapeInfo.cpp; sourceTree = "<group>"; };
> +		FD748ABE15BF74ED0059CF0D /* WrapShapeInfo.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = WrapShapeInfo.h; sourceTree = "<group>"; };

Ditto.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:22158
> +				FD748AC015BF74ED0059CF0D /* WrapShapeInfo.h in Headers */,
>  				97BC69DB1505F076001B74AC /* AbstractDatabase.h in Headers */,
>  				41E1B1D10FF5986900576B3B /* AbstractWorker.h in Headers */,
>  				29A8122E0FBB9C1D00510293 /* AccessibilityARIAGridCell.h in Headers */,

Nit: Please add this file to the list such that the list is in alphabetical order.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:28511
> +				FD748ABF15BF74ED0059CF0D /* WrapShapeInfo.cpp in Sources */,

Ditto.

> Source/WebCore/rendering/WrapShapeInfo.cpp:56
> +    , m_shapeLeft(0)
> +    , m_shapeTop(0)
> +    , m_shapeWidth(0)
> +    , m_shapeHeight(0)
> +    , m_lineTop(0)

It's unnecessary to initialize these instance variables with 0 as the default constructor for FractionalLayoutUnit (*) defaults the value of new instances to 0.

(*) LayoutUnit is defined to be an alternative name for FractionalLayoutUnit by <http://trac.webkit.org/browser/trunk/Source/WebCore/rendering/LayoutTypes.h?rev=125167#L49>.

> Source/WebCore/rendering/WrapShapeInfo.cpp:65
> +WrapShapeInfo* WrapShapeInfo::ensureWrapShapeInfoForRenderBlock(RenderBlock* block)

Maybe "add", as in WrapShapeInfo::add(RenderBlock*), would be a more descriptive name for this function? This would make the name and functionality of this function consistent with other functions named add, including HashMap::add() (which is called below).

> Source/WebCore/rendering/WrapShapeInfo.h:64
> +    static PassOwnPtr<WrapShapeInfo> create(RenderBlock* block) { return adoptPtr(new WrapShapeInfo(block)); }
> +    static WrapShapeInfo* wrapShapeInfoForRenderBlock(const RenderBlock*);
> +    static WrapShapeInfo* ensureWrapShapeInfoForRenderBlock(RenderBlock*);
> +    static void removeWrapShapeInfoForRenderBlock(const RenderBlock*);

Do you intend to expand this class to support other render types (e.g. RenderInline)? Otherwise, you may want to consider removing the suffix "ForRenderBlock".

> Source/WebCore/rendering/WrapShapeInfo.h:65
> +    static bool isWrapShapeInfoEnabledForBlock(const RenderBlock*);

This is the only function that ends in the suffix "ForBlock".

> Source/WebCore/rendering/WrapShapeInfo.h:69
> +    const SegmentList& segments() const { ASSERT(hasSegments()); return m_segments; }

We tend to write each statement on its own line when an inline method definition contains more than one statement:

const SegmentList& segments() const
{
    ASSERT(hasSegments());
    return m_segments;
}

> Source/WebCore/rendering/WrapShapeInfo.h:73
> +    void wrapShapeSizeNeedsRecomputing() { m_wrapShapeSizeNeedsRecomputing = true; }

Maybe a better name for this function would be sizeDirty? or isSizeDirty? or needsSizeUpdate?

> LayoutTests/fast/exclusions/shape-inside/shape-inside-inline-elements-expected.html:32
> +    <div id="border"></div>
> +    <div class="inline"></div><div class="inline"></div><div class="inline"></div><div class="inline"></div>

For your consideration, I suggest indenting these lines so as to make it more straightforward to identify that the closing </div> (line 33) matches with the <div id="shape-inside"> (line 30). Notice that you indent the contents of <div id="shape-inside"> in other test included in this patch (e.g. LayoutTests/fast/exclusions/shape-inside/shape-inside-overflow-expected.html).

> LayoutTests/fast/exclusions/shape-inside/shape-inside-inline-elements.html:35
> +    <div id="border"></div>
> +    <div class="inline"></div><div class="inline"></div><div class="inline"></div><div class="inline"></div>

For your consideration, I suggest indenting these lines so as to make it more straightforward to identify that the closing </div> (line 36) matches with the <div id="shape-inside"> (line 33). Notice that you indent the contents of <div id="shape-inside"> in other test included in this patch (e.g. LayoutTests/fast/exclusions/shape-inside/shape-inside-overflow-expected.html).

[Julien Chaffraix]

Comment on attachment 159482 [details]
Fixing nullptr error on destruction

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

The change looks good. Some cosmetic comments. I just need an architectural check by someone more familiar with the line box architecture and I am fine with r+'ing it.

> Source/WebCore/ChangeLog:34
> +        (WebCore::RenderBlock::willBeDestroyed): remove WrapShapeInfo when destroyed

Nit: Sentences start with a capital and end with a dot.

> Source/WebCore/rendering/RenderBlock.cpp:1383
> +    if (wrapShape == oldWrapShape)

Because of how RenderStyle works, pointer inequality doesn't imply that the data are different. It's enough for now but you may want to mention that as a future optimization.

> Source/WebCore/rendering/RenderBlock.h:405
> +    };

Trailing ';'

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:163
> +        m_left = max<float>(m_segment->logicalLeft, m_left);
> +        m_right = min<float>(m_segment->logicalRight, m_right);

We should try to share this code with the one below. As you will need to support multiple segments, it would prevent any bug to sneak in.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1294
> +#endif

I really wonder if LineBreaker shouldn't hold part of this logic. Eric / Levi would have a more educated opinion on that.

> Source/WebCore/rendering/WrapShapeInfo.cpp:90
> +    if (!block->style() || !block->style()->wrapShapeInside())

block->style() is pretty much never NULL, are you sure the first check is needed?

> LayoutTests/fast/exclusions/shape-inside/shape-inside-floats-simple.html:36
> +        green squares.

Let's add a description here. That would make the intent of the test more obvious (in this case, I had to think a lot to answer this question). Not repeated for each test.

> LayoutTests/fast/exclusions/shape-inside/shape-inside-percentage-auto.html:16
> +        margin-top: -2px;
> +        margin-left: -2px;

Couldn't we get away without the negative margins here?

[Julien Chaffraix]

Comment on attachment 159482 [details]
Fixing nullptr error on destruction

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

Ooops review collision, let's have another round to cover Daniel's comments :)

>> Source/WebCore/rendering/WrapShapeInfo.cpp:56
>> +    , m_lineTop(0)
> 
> It's unnecessary to initialize these instance variables with 0 as the default constructor for FractionalLayoutUnit (*) defaults the value of new instances to 0.
> 
> (*) LayoutUnit is defined to be an alternative name for FractionalLayoutUnit by <http://trac.webkit.org/browser/trunk/Source/WebCore/rendering/LayoutTypes.h?rev=125167#L49>.

Good catch Daniel, the definition of LayoutUnit actually depends if you have enabled subpixel layout: it's FractionalLayoutUnit if you do, int if you didn't.

The preferred way is to initialize a LayoutUnit to "0" is ZERO_LAYOUT_UNIT and not 0.

>> Source/WebCore/rendering/WrapShapeInfo.cpp:65
>> +WrapShapeInfo* WrapShapeInfo::ensureWrapShapeInfoForRenderBlock(RenderBlock* block)
> 
> Maybe "add", as in WrapShapeInfo::add(RenderBlock*), would be a more descriptive name for this function? This would make the name and functionality of this function consistent with other functions named add, including HashMap::add() (which is called below).

We have a pattern of ensure* method in the dom/, this is just extending it here. I don't think add makes any sense as it's an implementation detail.

> Source/WebCore/rendering/WrapShapeInfo.h:86
> +    LayoutUnit m_logicalHeight;

Maybe we could get away with using floats here to match the line box units and not the RenderObject layout.

[Levi Weintraub]

Comment on attachment 159482 [details]
Fixing nullptr error on destruction

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

> Source/WebCore/ChangeLog:47
> +        (WebCore::RenderBlock::layoutRunsAndFloatsInRange): move to the top of the inside wrap shape before beginning inline layout, and update the line segments for each line

Nit: your line lengths are really inconsistent.

>> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1294
>> +#endif
> 
> I really wonder if LineBreaker shouldn't hold part of this logic. Eric / Levi would have a more educated opinion on that.

It seems a little weird to have LineBreaker setting the height of the RenderBlock. I feel like in general the relationship between LineBreaker and this function specifically could use some work, but I'm okay with this as it stands.

>>> Source/WebCore/rendering/WrapShapeInfo.cpp:56
>>> +    , m_lineTop(0)
>> 
>> It's unnecessary to initialize these instance variables with 0 as the default constructor for FractionalLayoutUnit (*) defaults the value of new instances to 0.
>> 
>> (*) LayoutUnit is defined to be an alternative name for FractionalLayoutUnit by <http://trac.webkit.org/browser/trunk/Source/WebCore/rendering/LayoutTypes.h?rev=125167#L49>.
> 
> Good catch Daniel, the definition of LayoutUnit actually depends if you have enabled subpixel layout: it's FractionalLayoutUnit if you do, int if you didn't.
> 
> The preferred way is to initialize a LayoutUnit to "0" is ZERO_LAYOUT_UNIT and not 0.

Actually, all ports now use FractionalLayoutUnits, and only the value of the fraction is determined by the Sub-pixel layout feature. You don't need to initialize these at all.

>> Source/WebCore/rendering/WrapShapeInfo.h:86
>> +    LayoutUnit m_logicalHeight;
> 
> Maybe we could get away with using floats here to match the line box units and not the RenderObject layout.

Sadly, I'd say LayoutUnits would be preferred if all ports enabled sub-pixel layout, but otherwise floats probably do make more sense :(

[Bear Travis]

Created attachment 160293 [details]
Incorporating feedback

Making suggested changes. All changes have been incorporated with the following exceptions. Certain areas of the pbx project are not alphabetized by filename (but by hash), so not all references were alphabetized. The 'ForRenderBlock' methods are keeping their names, but one is changing to 'isWrapSHapeInfoEnabledForRenderBlock'. Some test cases have hit a null block->style() reference, so the check is remaining in place. Each LineWidth should only refer to a single segment, so I left updateAvailableWidth as is.

[Levi Weintraub]

Comment on attachment 160293 [details]
Incorporating feedback

LGTM.

[Alexandru Chiculita]

Comment on attachment 160293 [details]
Incorporating feedback

Adding CQ+ for Bear.

[WebKit Review Bot]

Comment on attachment 160293 [details]
Incorporating feedback

Clearing flags on attachment: 160293

Committed r126605: <http://trac.webkit.org/changeset/126605>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.