Bug 143320 – [New Block-Inside-Inline Model] Implement the correct painting order for blocks inside inlines

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Dave Hyatt]

The painting order has to match the old continuation-based model.

[Dave Hyatt]

Created attachment 249944 [details]
Patch (just for ews)

[WebKit Commit Bot]

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


ERROR: Source/WebCore/rendering/InlineFlowBox.cpp:1135:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 14 files


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

[Dave Hyatt]

Created attachment 249984 [details]
Patch

[Darin Adler]

Comment on attachment 249984 [details]
Patch

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

Is this enough testing to cover all the code?

> Source/WebCore/rendering/EllipsisBox.cpp:143
> +bool EllipsisBox::nodeAtPoint(const HitTestRequest& request, HitTestResult& result, const HitTestLocation& locationInContainer, const LayoutPoint& accumulatedOffset, LayoutUnit lineTop, LayoutUnit lineBottom, HitTestAction hitTestAction)

Argument list here is getting out of hand. Can we further merge some of these intro structs and classes?

> Source/WebCore/rendering/InlineElementBox.cpp:74
> +        PaintPhase newPhase = (paintInfo.phase == PaintPhaseChildOutlines) ? PaintPhaseOutline : paintInfo.phase;
> +        newPhase = (newPhase == PaintPhaseChildBlockBackgrounds) ? PaintPhaseChildBlockBackground : newPhase;

This seems like a strange way to write it. How about a switch?

    PaintInfo subtreePaintInfo = paintInfo;
    switch (subtreePaintInfo.phase) {
    case PaintPhaseChildOutlines:
        subtreePaintInfo.phase = PaintPhaseOutline;
        break;
    case PaintPhaseChildBlockBackgrounds:
        subtreePaintInfo.phase = PaintPhaseChildBlockBackground;
        break;
    default:
        break;
    }

Or if you want it to look more compact, you can use this kind of formatting (and we can change the coding style guidelines to allow it).

    switch (subtreePaintInfo.phase) {
    case PaintPhaseChildOutlines: subtreePaintInfo.phase = PaintPhaseOutline; break;
    case PaintPhaseChildBlockBackgrounds: subtreePaintInfo.phase = PaintPhaseChildBlockBackground; break;
    default: break;
    }

The ? : seem harder to read.

[Dave Hyatt]

Fixed in r182279.