Bug 41721 – Missing plug-in indicator button should have a pressed state

• Component: Plug-ins
• Status: RESOLVED FIXED

[Adele Peterson]

Missing plug-in indicator button should have a pressed state

[Adele Peterson]

Created attachment 60672 [details]
patch

Still needs a test and real ChangeLog comments

[Darin Adler]

Comment on attachment 60672 [details]
patch

> +    , m_capturingMouseEvents(0)

false, not 0.

> +    if (m_capturingMouseEvents)
> +        if (Frame* frame = document()->frame())
> +            frame->eventHandler()->setCapturingMouseEventsNode(0);      

Need braces around this. Trailing spaces after the setCapturingMouseEventsNode call. Should this set m_capturingMouseEvents to false?

> +    bool m_capturingMouseEvents;

Should be m_isCapturingMouseEvents.

> +    FloatPoint labelPoint(roundf(replacementTextRect.location().x() + (replacementTextRect.size().width() - textWidth) / 2),
> +                          roundf(replacementTextRect.location().y()+ (replacementTextRect.size().height() - font.height()) / 2 + font.ascent()));

For what it's worth, I never format things like this, with the second line indented to match the open parenthesis.

I think this might read better if you used two local variables for x and y. There's a missing space in "y()+".

> +    replacementTextRect.setLocation(FloatPoint((contentRect.size().width() / 2 - replacementTextRect.size().width() / 2) + contentRect.location().x(),
> +                                               (contentRect.size().height() / 2 - replacementTextRect.size().height() / 2) + contentRect.location().y()));

Same comment about indentation.

> +    FloatRect contentRect;
> +    Path path;
> +    FloatRect replacementTextRect;
> +    Font font;
> +    TextRun run("");
> +    float textWidth;
> +    if (!getReplacementTextGeometry(0, 0, contentRect, path, replacementTextRect, font, run, textWidth))
> +        return false;

Another way to do this is to create a struct to hold the six things that come out of this function.

> +    FloatPoint localPoint = absoluteToLocal(static_cast<MouseEvent*>(event)->absoluteLocation(), false, true);

Those boolean arguments stink! Not your job to fix that in this check-in, but still!

> +    if (Page* page = document()->page())
> +        if (!page->chrome()->client()->shouldMissingPluginMessageBeButton())
> +            return;

Missing braces here.

> +    void setMissingPluginIndicatorIsPressed(bool pressed) { m_missingPluginIndicatorIsPressed = pressed; }

Might want to make this handle the repaint automatically. Then you could use it in the three places that set this flag without explicit code to handle the repainting.

Also, I see no callers, so if you don't add callers, then I suggest getting rid of this. Also, should consider making this a private function.

> +    bool eventIsInMissingPluginIndicator(Event*);

Not sure the word "event" is needed in the function name.

Since you did end up checking isMouseEvent in handleMissingPluginIndicatorEvent, you could change this to take a MouseEvent*.

[Adele Peterson]

Created attachment 60800 [details]
patch

same changelog issues as before, but addresses many of Darin's comments

[Adele Peterson]

Created attachment 60834 [details]
patch

now with a test

[Adam Roben (:aroben)]

Comment on attachment 60834 [details]
patch

You need to update UIDelegate::QueryInterface to support querying to IWebUIDelegatePrivate3. You should probably support IWebUIDelegatePrivate2, too.

You could get away without adding shouldMissingPluginMessageBeButton on Windows. You could instead say "if the UI delegate implements IWebUIDelegatePrivate3, then the message should be a button". That would be good enough for Safari. It's not clear to me whether that's better than adding the new delegate function, though.

Other than that, the Windows changes look fine.

[Adele Peterson]

Hmm I may post a new patch then.

[Adele Peterson]

Created attachment 60936 [details]
patch

[Adam Roben (:aroben)]

Comment on attachment 60936 [details]
patch

I still think you should add support for IWebUIDelegatePrivate2 to UIDelegate::QueryInterface.

Other than that, the Windows changes look fine. Looks like you have an extra space in the Mac UIDelegate implementation.

I didn't read the rest of the patch (aroben--)

[Jon Honeycutt]

Comment on attachment 60936 [details]
patch

> ===================================================================
> Index: WebCore/plugins/PluginView.cpp
> ===================================================================
> --- WebCore/plugins/PluginView.cpp	(revision 62605)
> +++ WebCore/plugins/PluginView.cpp	(working copy)
> @@ -284,7 +284,9 @@ PluginView::~PluginView()
>  
>      ASSERT(!m_lifeSupportTimer.isActive());
>  
> -    instanceMap().remove(m_instance);
> +    // If the plug-in can't be found then m_instance is 0
> +    if (m_instance)
> +        instanceMap().remove(m_instance);
>  
>      if (m_isWaitingToStart)
>          m_parentFrame->document()->removeMediaCanStartListener(this);
> @@ -811,6 +813,7 @@ PluginView::PluginView(Frame* parentFram
>      , m_paramNames(0)
>      , m_paramValues(0)
>      , m_mimeType(mimeType)
> +    , m_instance(0)
>  #if defined(XP_MACOSX)
>      , m_isWindowed(false)
>  #else

This change isn't mentioned in your ChangeLog. This fix is also separate from your other changes and could be split out into its own change. Also, although your new test does test this on Windows, this fix could also have its own test that ran on all platforms that use PluginView.

r=me

[Darin Adler]

Comment on attachment 60936 [details]
patch

> +    // If the plug-in can't be found then m_instance is 0

Missing period on this comment.

> +void RenderEmbeddedObject::setMissingPluginIndicatorIsPressed(bool pressed)
> +{
> +    if (m_missingPluginIndicatorIsPressed != pressed) {
> +        m_missingPluginIndicatorIsPressed = pressed;
> +        repaint();
> +    }
> +}

I normally suggest early-return for this kind of function.

> +    path = Path::createRoundedRectangle(replacementTextRect, FloatSize(replacementTextRoundedRectRadius, replacementTextRoundedRectRadius));
> +    return true;

I think there should be a blank line there. No reason for the path-setting line to be grouped with the return statement, since it's only one of many things returned by the function.

> +    FloatPoint localPoint = absoluteToLocal(event->absoluteLocation(), false, true);
> +    return path.contains(localPoint);

I would collapse these two lines into one. I don't think the local variable name helps a lot, and the combined line is only three characters longer than the first line.

> +        bool shouldBePressed = m_mouseDownWasInMissingPluginIndicator && isInMissingPluginIndicator(mouseEvent);
> +        setMissingPluginIndicatorIsPressed(shouldBePressed);

I would collapse these two lines into one.

> +    void setMissingPluginIndicatorIsPressed(bool pressed);

I don't think you need this argument name here.

> +    // If the UI delegate implements IWebUIDelegatePrivate3, 
> +    // which contains didPressMissingPluginButton, then the message should be a button.
> +    COMPtr<IWebUIDelegatePrivate3> uiDelegatePrivate3(Query, uiDelegate);
> +    return uiDelegatePrivate3;

Seems like in the future some client might want to implement some other aspects of IWebUIDelegatePrivate3 but not have the missing plug-in text act as a button. So this is not perfect for the future.

> +    printf ("MISSING PLUGIN BUTTON PRESSED\n");

Extra space after printf. Could use puts here instead.

> +    printf("MISSING PLUGIN BUTTON PRESSED\n");

Could use puts here instead.

All of those comments are advisory and none should be considered as overriding Jon's r=me

[Adele Peterson]

http://trac.webkit.org/changeset/62875