[webkit-reviews] review denied: [Bug 52952] Focus ring for anchor with inline image is incorrect : [Attachment 87072] updated patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied ChangSeok Oh <kevin.cs.oh at gmail.com>'s
request for review:
Bug 52952: Focus ring for anchor with inline image is incorrect
https://bugs.webkit.org/show_bug.cgi?id=52952

Attachment 87072: updated patch
https://bugs.webkit.org/attachment.cgi?id=87072&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=87072&action=review

I think we need another round and some questions answered.  I'm sorry I didn't
look at this sooner.  Thank you very much for the reminder email!

> Source/WebCore/ChangeLog:5
> +	   Wrong tab focused region

You might want to update the bug titles in the ChangeLogs.  Not important, but
the new title is definitely more descriptive than the old one. :)

> Source/WebCore/rendering/RenderInline.cpp:1039
> +	   for (InlineBox* child = curr->firstChild(); child; child =
child->next()) {

Hmm... Maybe we should be caching this information on the root line boxes
instead?

How is this affected by hyatt's recent change of removing many lineboxes from
the linebox tree?

> Source/WebCore/rendering/RenderInline.cpp:1041
> +	       if (!renderObj->isText()) {

I take it the text case is already handled somewhere else?

It seems we should be saving this information on the parent linebox as part of
layout()?  But I iadmit not to be a painting expert for the linebox tree yet.

> LayoutTests/fast/repaint/outline-focus-anchor-image.html:4
> +    <title>CSS 2.1 Test Suite: outline</title>

So is this a modified version of a CSS2.1 test?  Or is this just imported
directly from CSS 2.1's test suite? I believe we already have the css 2.1 test
suite imported into LayoutTests.  Have you run all the layout tests to see if
your change affects any of the rest of them?

> LayoutTests/fast/repaint/outline-focus-anchor-image.html:25
> +	   Text before image anchor <img src="resources/apple.jpg" width=100px
height=100px /><br>

apple.jpg seems to be missing from your patch.	Again, you might not even need
this test as it may already be in our repository, unless you modified an
existing CSS21 test?