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

Tue Jan 10 14:46:47 PST 2012

Ryosuke Niwa <rniwa at webkit.org> has denied 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 121906: Patch
https://bugs.webkit.org/attachment.cgi?id=121906&action=review

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=121906&action=review

> Source/WebCore/ChangeLog:7
> +

You should explain what kind of changes you're making.

> Source/WebCore/html/HTMLAppletElement.h:46
> +    virtual bool useFallbackContent() const OVERRIDE { return false; }

Instead of making HTMLPluginElement:: useFallbackContent a pure virtual, you
can move the definition in HTMLPluginImageElement to HTMLPluginElement so that
you don't have to add new definition here.

> Source/WebCore/html/HTMLPlugInElement.cpp:192
> +    if (useFallbackContent())

You should also make sure HTMLFrameOwnerElement::supportsFocus() returns true.
e.g. if this plugin is showing a fallback content and has contenteditable
content attribute, then this element should be focusible (we need a test case
for this). r- because of this.

> Source/WebCore/html/HTMLPlugInElement.cpp:194
> +    RenderObject* r = renderer();

Please don't use one-letter variable. Also, renderer() is an inline function so
there's no need to cache it.

> Source/WebCore/html/HTMLPlugInElement.cpp:195
> +    if (!r || !(r->isEmbeddedObject()))

Redundant parenthesis around r->isEmbeddedObject(). This condition should be
re-written as:
!renderer() || !renderer()->isEmbeddedObject()

> Source/WebCore/html/HTMLPlugInElement.cpp:197
> +    RenderEmbeddedObject* renderEmbed(toRenderEmbeddedObject(r));

Please use operator= here: renderEmbed = toRenderEmbeddedObject(renderer()).

> Source/WebCore/html/HTMLPlugInElement.cpp:209
> +	   if (received)
> +	       page->focusController()->setFocusedNode(this, contentFrame());

Could you explain why we need this code? Also, maybe we can share code with
HTMLFrameElementBase::setFocus ?

> LayoutTests/ChangeLog:7
> +

Please explain what kind of a test you're adding.

> LayoutTests/fast/events/tabindex-focus-blur-all-expected.txt:1
> -333 focus / 333 blur events dispatched, and should be 333 / 333 PASSED
> +336 focus / 336 blur events dispatched, and should be 336 / 336 PASSED

Why is this change not listed on LayoutTests/ChangeLog ?

> LayoutTests/plugins/focus.html:1
> +<html>

Are you intentionally testing quirks mode? If not, we need <!DOCTYPE html>
here.

> LayoutTests/plugins/focus.html:29
> +	   var elem = owner.childNodes[i];

Please don't use abbreviations like elem.

> LayoutTests/plugins/focus.html:33
> +	       real_target = undefined;

Please use camelCase.

> LayoutTests/plugins/focus.html:41
> +	       if (elem.getAttribute("should_focus") === "true") {
> +		   if (elem !== real_target)
> +		       log(elem.id + " should have gotten focus but didn't.");
> +	       } else {
> +		   if (elem === real_target)
> +		       log(elem.id + " shouldn't have gotten focus but did.");
> +	       }

This test should be a script-test. (Use
http://trac.webkit.org/browser/trunk/Tools/Scripts/make-new-script-test).