[webkit-reviews] review denied: [Bug 74723] REGRESSION (r94492): Text is shifted to the right in some buttons in the Mac App Store : [Attachment 120598] Patch

[bugzilla-daemon at webkit.org]

mitz at webkit.org has denied Robert Hogan <robert at webkit.org>'s request for
review:
Bug 74723: REGRESSION (r94492): Text is shifted to the right in some buttons in
the Mac App Store
https://bugs.webkit.org/show_bug.cgi?id=74723

Attachment 120598: Patch
https://bugs.webkit.org/attachment.cgi?id=120598&action=review

------- Additional Comments from mitz at webkit.org
View in context: https://bugs.webkit.org/attachment.cgi?id=120598&action=review


r- because of the incomplete change log and because the fix for the
hasTextDescendants() bug should be done separately and more efficiently.

> Source/WebCore/ChangeLog:16
> +	   * rendering/RenderBlockLineLayout.cpp:
> +	   (WebCore::RenderBlock::LineBreaker::skipLeadingWhitespace):
> +

The change log is incomplete: it doesn’t list the InlineFlowBox changes, the
LineInfo changes, and the constructLine change. You should include detailed
comments about each change.

>> Source/WebCore/rendering/InlineFlowBox.h:200
>> +	}
> 
> This part was necessary to ensure that when the positioned object was added
first, the root inline box was still aware of any subsequent text descendants
such as the BR added as children to its first child.

Good catch here, but this is a pre-existing issue and I think you should file a
bug about it and fix it separately before proceeding with this bug. Here is a
test case showing the bug:
    <span style="outline: 1px dotted blue;"> <span style="margin: 0
10px;"></span> text</span>

When you fix that bug, you should do it more efficiently. First, use a loop
instead of recursion. Secondly, check if the parent is already marked as having
text descendants (the overwhelmingly common case), and if so, stop.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:205
> +    void setPositionedObjectsInLeadingWhitespace(unsigned count) {
m_positionedObjectsInLeadingWhitespace = count; }

Since you’re only ever incrementing this by one, I’d define a function called
incrementPositionedObjectsInLeadingWhitespace() that bumps the counter by one.
No need for the caller to keep track of the count.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:488
> +	   int runCount = bidiRuns.runCount() -
lineInfo.positionedObjectsInLeadingWhitespace();

You should hoist this line out of the loop.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1881
> +    lineInfo.setPositionedObjectsInLeadingWhitespace(positionedObjects);

Is it necessary to reset this here? See my earlier comments about how to deal
with this counter.