Bug 117620

Summary: Clicking on snapshotting plug-ins does not restart them
Product: WebKit Reporter: Dean Jackson <dino>
Component: Plug-insAssignee: Dean Jackson <dino>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, esprehn+autocc, thorton
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch simon.fraser: review+

Description Dean Jackson 2013-06-13 17:58:21 PDT 
If a plug-in is snapshotting and the user clicks on it, it should immediately restart.

<rdar://problem/13821729>
Comment 1 Dean Jackson 2013-06-13 18:01:49 PDT 
Created attachment 204657 [details]
Patch
Comment 2 Dean Jackson 2013-06-13 18:05:58 PDT 
Comment on attachment 204657 [details]
Patch

Ignore this. I tried to sneak it by but it is bad coding.
Comment 3 Darin Adler 2013-06-13 18:08:41 PDT 
Comment on attachment 204657 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=204657&action=review

> Source/WebCore/html/HTMLPlugInElement.cpp:223
> +        if (displayState() == WaitingForSnapshot && event->type() == eventNames().clickEvent && isPlugInImageElement()) {
> +            MouseEvent* mouseEvent = static_cast<MouseEvent*>(event);
> +            if (mouseEvent->button() == LeftButton) {
> +                toHTMLPlugInImageElement(this)->userDidClickSnapshot(mouseEvent, false);
> +                event->setDefaultHandled();
> +                return;
> +            }
> +        }

defaultEventHandler is a virtual function. Why not put this code in the HTMLPlugInImageElement class in the first place instead of down casting?
Comment 4 Dean Jackson 2013-06-13 18:15:25 PDT 
Created attachment 204658 [details]
Patch
Comment 5 Dean Jackson 2013-06-13 18:17:59 PDT 
(In reply to comment #3)
> (From update of attachment 204657 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=204657&action=review
> 
> > Source/WebCore/html/HTMLPlugInElement.cpp:223
> > +        if (displayState() == WaitingForSnapshot && event->type() == eventNames().clickEvent && isPlugInImageElement()) {
> > +            MouseEvent* mouseEvent = static_cast<MouseEvent*>(event);
> > +            if (mouseEvent->button() == LeftButton) {
> > +                toHTMLPlugInImageElement(this)->userDidClickSnapshot(mouseEvent, false);
> > +                event->setDefaultHandled();
> > +                return;
> > +            }
> > +        }
> 
> defaultEventHandler is a virtual function. Why not put this code in the HTMLPlugInImageElement class in the first place instead of down casting?

Yeah... that's what I ended up doing. I'll even admit I thought of doing it up front :(
Comment 6 Simon Fraser (smfr) 2013-06-13 18:18:15 PDT 
Comment on attachment 204658 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=204658&action=review

> Source/WebCore/html/HTMLPlugInImageElement.cpp:716
> +        if (displayState() == WaitingForSnapshot && event->type() == eventNames().clickEvent && isPlugInImageElement()) {

I think this would be slightly clearer if you did the event->type() last.
Comment 7 Dean Jackson 2013-06-13 18:21:05 PDT 
Committed r151576: <http://trac.webkit.org/changeset/151576>
Comment 8 Darin Adler 2013-06-13 18:21:28 PDT 
Comment on attachment 204658 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=204658&action=review

> Source/WebCore/html/HTMLPlugInElement.h:83
> +    virtual void defaultEventHandler(Event*);

Should be protected, not public. And as long as you are touching it, should add OVERRIDE.

> Source/WebCore/html/HTMLPlugInImageElement.h:128
> +    virtual void defaultEventHandler(Event*);

Should add OVERRIDE. Could be private instead of protected.
Comment 9 Dean Jackson 2013-06-14 10:10:50 PDT 
Follow-up commit for Darin's comments: https://trac.webkit.org/r151599