Bug 41550 – The missing plug-in indicator should be clickable

• Component: Plug-ins
• Status: RESOLVED FIXED

[Jon Honeycutt]

Clicking the missing plug-in indicator should call a delegate method.

<rdar://problem/8132162>

[Jon Honeycutt]

Created attachment 60421 [details]
Patch

[Darin Adler]

Comment on attachment 60421 [details]
Patch

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

[Jon Honeycutt]

(In reply to comment #2)
> (From update of attachment 60421 [details])
> > +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?

I added this to preDispatchEventHandler so that the delegate is called even if there is an onclick handler that calls event.preventDefault(). What should the correct behavior be in that case?

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

Kevin explained to me why he thought this was a better approach, but I don't remember what he said. I'll change the delegate to take a reference to the element.

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

I didn't include that in this patch, both to make this patch simpler and because the code to have a pressed look is not complete.

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

[Jon Honeycutt]

Created attachment 60446 [details]
patch v2

[Darin Adler]

Comment on attachment 60446 [details]
patch v2

This doesn't seem to do any hit testing. So clicking anywhere in the plug-in, even far away from the missing plug-in indicator, would trigger the function call.

> +        ChromeClient* client = page->chrome()->client();
> +        client->missingPluginButtonClicked(this);
> +
> +        return;

I'd write that more compactly like this:

    page->chrome()->client()->missingPluginButtonClicked(this);
    return;

Without the blank line or the local variable.

r=me

[Jon Honeycutt]

(In reply to comment #5)
> (From update of attachment 60446 [details])
> This doesn't seem to do any hit testing. So clicking anywhere in the plug-in, even far away from the missing plug-in indicator, would trigger the function call.

Yes, I'll file a bug about this.

> 
> > +        ChromeClient* client = page->chrome()->client();
> > +        client->missingPluginButtonClicked(this);
> > +
> > +        return;
> 
> I'd write that more compactly like this:
> 
>     page->chrome()->client()->missingPluginButtonClicked(this);
>     return;
> 
> Without the blank line or the local variable.

Fixed.

> 
> r=me

Thanks for the reviews! Landed in r62451.