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

[bugzilla-daemon at webkit.org]

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 196208: Patch
https://bugs.webkit.org/attachment.cgi?id=196208&action=review

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


> Source/WebCore/ChangeLog:6
> +	   Make embed, object, and applet elements support focus when
appropriate.

This description should appear after Reviewed by line. Also you need a much
more detailed explanation of your change
since this sentence simply repeats the bug title.

A good change log describes what caused the bug and how you fixed it.
See http://www.webkit.org/coding/contributing.html#changelogs for an example.

> Source/WebCore/ChangeLog:10
> +	   Test: plugins/focus.html, java/focus.html

Wrong format. See other entires with multiple tests.

> Source/WebCore/ChangeLog:14
> +	   * html/HTMLPlugInElement.h:

The change log is missing an entry for HTMLPlugInImageElement.
Please mention why you're removing supportsFocus there.

> Source/WebCore/html/HTMLPlugInElement.cpp:256
> +	   RenderEmbeddedObject* renderEmbed =
toRenderEmbeddedObject(renderer());
> +	   if (renderEmbed->showsUnavailablePluginIndicator())
> +	       return false;
> +	   return true;

We can simply rewrite these 4 lines as:
return !toRenderEmbeddedObject(renderer())->showsUnavailablePluginIndicator();
Remember to remove the curly braces once you've done that.

But really, this function needs to be explained in the change log.
e.g. why we don't focus when we use fallback content?

> LayoutTests/ChangeLog:7
> +	   Add/update testing to ensure embed, object, and applet tags now
support
> +	   focus.

Again, the description like this should appear after Reviewed by line.

> LayoutTests/java/focus.html:62
> +	       if (pluginElement.getAttribute("shouldFocus") === "true")
> +		   shouldBeEqual(pluginElement.id + " should get focus.",
pluginElement, realTarget);
> +	       else
> +		   shouldBeEqual(pluginElement.id + " should not get focus.",
undefined, realTarget);

Why don't we just use shouldBe? It seems like duplicating all of this logic is
unnecessary.

> LayoutTests/java/focus.html:68
> +	       if (pluginElement.getAttribute("shouldFocus") === "true")
> +		   shouldBeEqual(pluginElement.id + " should lose focus.",
pluginElement, realTarget);
> +	       else
> +		   shouldBeEqual(pluginElement.id + " should not lose focus.",
undefined, realTarget);

Ditto.

> LayoutTests/java/focus.html:76
> +if (window.layoutTestController)
> +    layoutTestController.dumpAsText();

layoutTestController has been renamed to testRunner.
Also, you don't need to call this function since js-test-pre automatically does
that.
r- because of this clearly wrong code.

> LayoutTests/plugins/focus.html:68
> +	       if (pluginElement.getAttribute("shouldFocus") === "true")
> +		   shouldBeEqual(pluginElement.id + " should get focus.",
pluginElement, realTarget);
> +	       else
> +		   shouldBeEqual(pluginElement.id + " should not get focus.",
undefined, realTarget);
> +	       realTarget = undefined;
> +	       pluginElement.blur();
> +	       if (pluginElement.getAttribute("shouldFocus") === "true")
> +		   shouldBeEqual(pluginElement.id + " should lose focus.",
pluginElement, realTarget);
> +	       else
> +		   shouldBeEqual(pluginElement.id + " should not lose focus.",
undefined, realTarget);

Ditto about using shouldBe.

> LayoutTests/plugins/focus.html:76
> +if (window.layoutTestController)
> +    layoutTestController.dumpAsText();

Ditto about layoutTestController.