[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]

Ryosuke Niwa <rniwa at webkit.org> has granted Dave Michael
<dmichael at chromium.org>'s request for 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 Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=196381&action=review


> Source/WebCore/html/HTMLPlugInElement.cpp:252
> +    if (useFallbackContent())
> +	   return false;
> +    if (renderer() && renderer()->isEmbeddedObject())

Maybe we can combine these two conditions like this:
if (useFallbackContent() || !renderer() || !renderer()->isEmbeddedObject())
    return false;

> 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.

> 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.

> LayoutTests/plugins/focus.html:40
> +			'pluginElement.getAttribute("shouldFocus") ===
"true"');

Nit: wrong indentation. 'pluginElement.getAttribute' should be exactly 4 spaces
to the right of shouldBe(.