[webkit-reviews] review granted: [Bug 32292] Unable to focus on embedded plugins such as Flash via javascript focus() : [Attachment 196381] Patch

[bugzilla-daemon at webkit.org]

Dave Michael <dmichael at chromium.org> has granted  review:
Bug 32292: Unable to focus on embedded plugins such as Flash via javascript
focus()
https://bugs.webkit.org/show_bug.cgi?id=32292

Attachment 196381: Patch
https://bugs.webkit.org/attachment.cgi?id=196381&action=review

------- Additional Comments from Dave Michael <dmichael at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=196381&action=review


Thanks very much for the thoughtful and speedy reviews!

>> Source/WebCore/ChangeLog:6
>> +	    Reviewed by Ryosuke Niwa.
> 
> By the way, please don't replace NOBODY (OOPS!) by a reviewer's name until
the reviewer gave your patch r+.

Oops, sorry. Wasn't sure when I was supposed to do that; see now that I
shouldn't have.

>> Source/WebCore/html/HTMLPlugInElement.cpp:252
>> +	if (renderer() && renderer()->isEmbeddedObject())
> 
> Maybe we can combine these two conditions like this:
> if (useFallbackContent() || !renderer() || !renderer()->isEmbeddedObject())
>     return false;

Sure, done.

>> LayoutTests/ChangeLog:11
>> +	    * fast/events/resources/tabindex-focus-blur-all-frame1.html: Change
embed and object elements to reference an invalid plugin, to be consistent with
applet. These element types are not focusable when there is valid plugin
content.
> 
> Nit: please wrap this line at some point. It's too long.

Done.

>> LayoutTests/java/focus-expected.txt:9
>> +PASS pluginElement = document.getElementById("appletElem");
pluginElement.focus(); document.activeElement === pluginElement is
pluginElement.getAttribute("shouldFocus") === "true"
> 
> Nit: Perhaps we can define $ = function (id) { return
document.getElementById(id); } or something to shorten the line here.

I tried a slightly different approach that makes the output pretty short but
still readable, including the element id. Let me know what you think.

>> LayoutTests/plugins/focus.html:40
>> +			 'pluginElement.getAttribute("shouldFocus") ===
"true"');
> 
> Nit: wrong indentation. 'pluginElement.getAttribute' should be exactly 4
spaces to the right of shouldBe(.

Done.