Summary: | Split the extra logic out of RenderBlock::updateFirstLetter | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Igor Trindade Oliveira <igor.oliveira> | ||||||||||
Component: | Layout and Rendering | Assignee: | Igor Trindade Oliveira <igor.oliveira> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | dglazkov, enne, hyatt, jchaffraix, mitz, rniwa, simon.fraser, webkit.review.bot | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | All | ||||||||||||
OS: | All | ||||||||||||
Attachments: |
|
Description
Igor Trindade Oliveira
2012-03-10 16:08:19 PST
Created attachment 131193 [details]
Patch
Proposed patch.
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 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 Created attachment 131203 [details]
Patch
Fix failing test.
Comment on attachment 131203 [details]
Patch
Cleaning flags. I will submit a better patch.
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 Created attachment 131246 [details]
Patch
Lets try again. Proposed patch.
Created attachment 131247 [details]
Patch
patch.
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 :) Committed r110802 :http://trac.webkit.org/changeset/110802 |