Bug 80772 – Split the extra logic out of RenderBlock::updateFirstLetter

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

[Igor Trindade Oliveira]

Currently, updateFirstLetter has a complex logic, it creates and updates the first letter element. 
Splitting the extra logic out of RenderBlock::updateFirstLetter, makes it easier to understand.

[Igor Trindade Oliveira]

Created attachment 131193 [details]
Patch

Proposed patch.

[Igor Trindade Oliveira]

When working in the implementation of the generated content animations, i noticed that we could split out the logic of the updateFirstLetter. I am working in a similar patch for updateBeforeAfterContent.

[WebKit Review Bot]

Comment on attachment 131193 [details]
Patch

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

New failing tests:
fast/css/nested-first-letter-with-float-crash.html

[Igor Trindade Oliveira]

Created attachment 131203 [details]
Patch

Fix failing test.

[Igor Trindade Oliveira]

Comment on attachment 131203 [details]
Patch

Cleaning flags. I will submit a better patch.

[WebKit Review Bot]

Comment on attachment 131203 [details]
Patch

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

New failing tests:
fast/css/first-letter-inline-flow-split-crash.html
fast/css-generated-content/remove-div-from-flexible-box-with-floating-after-content-crash.html
fast/text/one-letter-transform-crash.html
fast/css/first-letter-set-text.html
fast/css/first-letter-block-form-controls-crash.html
fast/css/first-letter-inline-flow-split-table-crash.html
fast/css/first-letter-removed-added.html
fast/css-generated-content/float-first-letter-siblings-convert-to-inline.html

[Igor Trindade Oliveira]

Created attachment 131246 [details]
Patch

Lets try again. Proposed patch.

[Igor Trindade Oliveira]

Created attachment 131247 [details]
Patch

patch.

[Julien Chaffraix]

Comment on attachment 131247 [details]
Patch

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

> Source/WebCore/rendering/RenderBlock.cpp:6000
> +    RenderStyle* pseudoStyle = styleForFirstLetter(firstLetterBlock, firstLetterContainer);

Our only use of |firstLetterBlock| is for styleForFirstLetter in both functions. I wonder if this could not be pushed to the caller (not necessarily in this patch)

> Source/WebCore/rendering/RenderBlock.cpp:6149
> +    // to update it.

Missing first part of the comment:
// If the child already has style, then it has already been created, so we just want

> Source/WebCore/rendering/RenderBlock.cpp:6162
> +    createFirstLetter(firstLetterBlock, currChild);

Comment missing:

// Create our pseudo style now that we have our firstLetterContainer determined.

I wonder if we want to keep it though as it doesn't add much.

> Source/WebCore/rendering/RenderBlock.h:532
> +    void createFirstLetter(RenderObject* firstLetterBlock, RenderObject* currentChild);

I am not super happy with the naming of this function. How about createFirstLetterRenderer (though it doesn't return a RenderObject which is weird for a create function). I can't think of better so if you have better ideas feel free to throw them here :)

[Igor Trindade Oliveira]

Committed r110802 :http://trac.webkit.org/changeset/110802