[webkit-reviews] review denied: [Bug 41550] The missing plug-in indicator should be clickable : [Attachment 60421] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Jon Honeycutt <jhoneycutt at apple.com>'s
request for review:
Bug 41550: The missing plug-in indicator should be clickable
https://bugs.webkit.org/show_bug.cgi?id=41550

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

------- Additional Comments from Darin Adler <darin at apple.com>
> +void* HTMLPlugInElement::preDispatchEventHandler(Event* event)

Why does this code need to be in preDispatchEventHandler instead of
defaultEventHandler? This is not what preDispatchEventHandler was designed for.
It was only for very special cases in the <input> element. Do we really need to
use it here?

> +    String pluginPageURL = getAttribute("pluginspage");
> +
> +    ChromeClient* client = page->chrome()->client();
> +    client->missingPluginButtonClicked(serviceType(), pluginPageURL);


Why does the element need to call getAttribute and pass the URL? It seems the
client could get the pluginspage attribute itself as long as it got a reference
to the element. Same comment on service type. It seems the client could get
that using standard DOM APIs and it would be more flexible to pass a reference
to the element. The only downside to that I can think of is that it's not very
WebKit2-friendly.

I don’t see any code here to implement a different look for the Missing Plug-in
text when it's being clicked on, a pressed state. Isn't that part of what we
were planning?

review- because of the use of preDispatchEventHandler. Everything else seems OK
as is even though I do have the questions above.