Bug 41550 - The missing plug-in indicator should be clickable
Summary: The missing plug-in indicator should be clickable
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Plug-ins (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Jon Honeycutt
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2010-07-02 16:29 PDT by Jon Honeycutt
Modified: 2010-07-03 14:58 PDT (History)
0 users

See Also:


Attachments
Patch (17.66 KB, patch)
2010-07-02 16:58 PDT, Jon Honeycutt
darin: review-
Details | Formatted Diff | Diff
patch v2 (15.63 KB, patch)
2010-07-03 04:14 PDT, Jon Honeycutt
darin: review+
jhoneycutt: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jon Honeycutt 2010-07-02 16:29:25 PDT
Clicking the missing plug-in indicator should call a delegate method.

<rdar://problem/8132162>
Comment 1 Jon Honeycutt 2010-07-02 16:58:38 PDT
Created attachment 60421 [details]
Patch
Comment 2 Darin Adler 2010-07-02 17:11:47 PDT
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.
Comment 3 Jon Honeycutt 2010-07-02 17:39:32 PDT
(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.
Comment 4 Jon Honeycutt 2010-07-03 04:14:40 PDT
Created attachment 60446 [details]
patch v2
Comment 5 Darin Adler 2010-07-03 10:43:22 PDT
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
Comment 6 Jon Honeycutt 2010-07-03 14:58:25 PDT
(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.