Clicking the missing plug-in indicator should call a delegate method. <rdar://problem/8132162>
Created attachment 60421 [details] Patch
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.
(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.
Created attachment 60446 [details] patch v2
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
(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.