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

Dave Michael <dmichael at chromium.org> has denied  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 Dave Michael <dmichael at chromium.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.


Done.

>> Source/WebCore/ChangeLog:10
>> +	    Test: plugins/focus.html, java/focus.html
> 
> Wrong format. See other entires with multiple tests.

Done.

>> Source/WebCore/ChangeLog:14
>> +	    * html/HTMLPlugInElement.h:
> 
> The change log is missing an entry for HTMLPlugInImageElement.
> Please mention why you're removing supportsFocus there.

Done (except I think you meant I should mention why I'm adding it?). Updated to
include HTMLPlugInImageElement.

>> Source/WebCore/html/HTMLPlugInElement.cpp:256
>> +	    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?

Done.

>> LayoutTests/ChangeLog:7
>> +	    focus.
> 
> Again, the description like this should appear after Reviewed by line.

Done.

>> LayoutTests/java/focus.html:62
>> +		    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.

shouldBe only accepts 2 strings that it evals, which has two consequences:
1) I have to make pluginElement a global (not a big problem, just annoying).
2) There's no "context" field, which means I lose useful information (the id
and whether we should or should not get focus).

Using shouldBe, I would get:
PASS pluginElement is realTarget
PASS pluginElement is realTarget
PASS undefined is realTarget
PASS undefined is realTarget
instead of something like:
PASS objectElemWithFallbackContents should get focus.
PASS objectElemWithFallbackContents should lose focus.
PASS noPluginEmbedElem should not get focus.
PASS noPluginEmbedElem should not lose focus.

I find the latter more readable and easier to debug.

>> LayoutTests/java/focus.html:76
>> +	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.

Sorry, I'd let this patch sit idle for a long time. Didn't realize this
changed. Done.

>> LayoutTests/plugins/focus.html:76
>> +	layoutTestController.dumpAsText();
> 
> Ditto about layoutTestController.

Done.