Bug 72987

Created attachment 116658 [details] Patch

Hi Julien, Can you take a look at this? Thanks!

Comment on attachment 116658 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=116658&action=review Robert, this is not an area of the code I know enough to review this patch. I just looked over the patch quickly. > Source/WebCore/rendering/RenderBlock.cpp:-5746 > - Unrelated change, it is also arguably better this way. > Source/WebCore/rendering/RenderBlock.h:477 > + RenderObject* firstLetterInChild(RenderObject* currChild, RenderObject* &firstLetterBlock); > + RenderObject* firstLetterInChildren(RenderObject* currChild, RenderObject* &firstLetterBlock); I find the 2 functions' name confusing. Especially since the second calls the first. Adding a verb would likely help. If those 2 functions had a really different name, it would also reduce the confusion between the 2.

Created attachment 134963 [details] Patch

Comment on attachment 134963 [details] Patch Attachment 134963 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12253863 New failing tests: fast/css/first-letter-capitalized-edit-select-crash.html

Created attachment 134969 [details] Archive of layout-test-results from ec2-cr-linux-03 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-03 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Created attachment 134998 [details] Patch

Comment on attachment 134998 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=134998&action=review > Source/WebCore/rendering/RenderBlock.cpp:6046 > +RenderObject* RenderBlock::firstLetterInSibling(RenderObject* currChild, RenderObject* &firstLetterBlock) I'm not sure "inSibling" is the right name here. This method seems to have little to do with being a sibling to anything. maybe InNode? or underNode? I also might call currChild "root" and have a separate currChild. Also, I wonder if we could pull the addFirstLetter call outside of this function? Could it be done in the caller, right before returning currChild? > Source/WebCore/rendering/RenderBlock.cpp:6055 > + // If we call addFirstLetter(), currChild will be destroyed. Always, or just could be? > Source/WebCore/rendering/RenderBlock.cpp:6067 > + if (currChild->style()->styleType() == FIRST_LETTER) { I'm confused what this check is for. > Source/WebCore/rendering/RenderBlock.cpp:6072 > + } else if (currChild->isReplaced() || currChild->isRenderButton() || currChild->isMenuList()) Is button and menu-list not replaced? Why do they need separate checks?

Created attachment 136967 [details] Patch

Comment on attachment 136967 [details] Patch Attachment 136967 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12390777 New failing tests: media/video-loop.html accessibility/aria-disabled.html http/tests/xmlhttprequest/xmlhttprequest-unsafe-redirect.html fast/canvas/webgl/shader-precision-format.html media/video-pause-immediately.html media/video-muted.html fast/loader/text-document-wrapping.html

Created attachment 137027 [details] Archive of layout-test-results from ec2-cr-linux-02 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-02 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

(In reply to comment #11) > Created an attachment (id=137027) [details] > Archive of layout-test-results from ec2-cr-linux-02 > > The attached test failures were seen while running run-webkit-tests on the chromium-ews. > Bot: ec2-cr-linux-02 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick These failures are unrelated to my patch - I don't see them locally.

(In reply to comment #8) > (From update of attachment 134998 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=134998&action=review > > > Source/WebCore/rendering/RenderBlock.cpp:6046 > > +RenderObject* RenderBlock::firstLetterInSibling(RenderObject* currChild, RenderObject* &firstLetterBlock) > > I'm not sure "inSibling" is the right name here. This method seems to have little to do with being a sibling to anything. maybe InNode? or underNode? I also might call currChild "root" and have a separate currChild. I've improved the names I think. > > Also, I wonder if we could pull the addFirstLetter call outside of this function? Could it be done in the caller, right before returning currChild? Not really, I think. I've refactored the patch quite a bit so I'm not sure if this is still a concern. > > > Source/WebCore/rendering/RenderBlock.cpp:6055 > > + // If we call addFirstLetter(), currChild will be destroyed. > > Always, or just could be? Refactoring has made this a non-issue. > > > Source/WebCore/rendering/RenderBlock.cpp:6067 > > + if (currChild->style()->styleType() == FIRST_LETTER) { > > I'm confused what this check is for. Added a comment. > > > Source/WebCore/rendering/RenderBlock.cpp:6072 > > + } else if (currChild->isReplaced() || currChild->isRenderButton() || currChild->isMenuList()) > > Is button and menu-list not replaced? Why do they need separate checks? This is existing code. As far as I can tell they will only be isReplaced() if they are inline, which I guess they will be most of the time, but not if display is set otherwise.

Comment on attachment 136967 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=136967&action=review This looks good as far as I can tell. > Source/WebCore/rendering/RenderBlock.cpp:5967 > + if (child->isText()) { Is this function hot? Do we need to worry about ordering the cases such that it's most likely to return early? > Source/WebCore/rendering/RenderBlock.cpp:5997 > + // We found a lower-level node with first-letter, which supersedes the higher-level style Should end with a period.

Committed r116438: <http://trac.webkit.org/changeset/116438>