Bug 80772 - Split the extra logic out of RenderBlock::updateFirstLetter
Summary: Split the extra logic out of RenderBlock::updateFirstLetter
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Igor Trindade Oliveira
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-03-10 16:08 PST by Igor Trindade Oliveira
Modified: 2012-03-14 18:20 PDT (History)
8 users (show)

See Also:


Attachments
Patch (11.18 KB, patch)
2012-03-10 16:28 PST, Igor Trindade Oliveira
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Patch (12.53 KB, patch)
2012-03-10 21:56 PST, Igor Trindade Oliveira
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Patch (12.57 KB, patch)
2012-03-11 10:50 PDT, Igor Trindade Oliveira
no flags Details | Formatted Diff | Diff
Patch (12.57 KB, patch)
2012-03-11 10:52 PDT, Igor Trindade Oliveira
jchaffraix: review+
jchaffraix: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Igor Trindade Oliveira 2012-03-10 16:08:19 PST
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.
Comment 1 Igor Trindade Oliveira 2012-03-10 16:28:28 PST
Created attachment 131193 [details]
Patch

Proposed patch.
Comment 2 Igor Trindade Oliveira 2012-03-10 16:34:15 PST
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.
Comment 3 WebKit Review Bot 2012-03-10 17:22:58 PST
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
Comment 4 Igor Trindade Oliveira 2012-03-10 21:56:05 PST
Created attachment 131203 [details]
Patch

Fix failing test.
Comment 5 Igor Trindade Oliveira 2012-03-10 22:19:24 PST
Comment on attachment 131203 [details]
Patch

Cleaning flags. I will submit a better patch.
Comment 6 WebKit Review Bot 2012-03-10 22:34:10 PST
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
Comment 7 Igor Trindade Oliveira 2012-03-11 10:50:46 PDT
Created attachment 131246 [details]
Patch

Lets try again. Proposed patch.
Comment 8 Igor Trindade Oliveira 2012-03-11 10:52:37 PDT
Created attachment 131247 [details]
Patch

patch.
Comment 9 Julien Chaffraix 2012-03-12 12:01:21 PDT
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 :)
Comment 10 Igor Trindade Oliveira 2012-03-14 18:20:44 PDT
Committed r110802 :http://trac.webkit.org/changeset/110802