Bug 117620 – Clicking on snapshotting plug-ins does not restart them

• Status: RESOLVED
• Resolution: FIXED
• Component: Plug-ins

[Dean Jackson]

If a plug-in is snapshotting and the user clicks on it, it should immediately restart.

<rdar://problem/13821729>

[Dean Jackson]

Created attachment 204657 [details]
Patch

[Dean Jackson]

Comment on attachment 204657 [details]
Patch

Ignore this. I tried to sneak it by but it is bad coding.

[Darin Adler]

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?

[Dean Jackson]

Created attachment 204658 [details]
Patch

[Dean Jackson]

(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 :(

[Simon Fraser (smfr)]

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.

[Dean Jackson]

Committed r151576: <http://trac.webkit.org/changeset/151576>

[Darin Adler]

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.

[Dean Jackson]

Follow-up commit for Darin's comments: https://trac.webkit.org/r151599