Bug 41721 - Missing plug-in indicator button should have a pressed state
Summary: Missing plug-in indicator button should have a pressed state
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: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-07-06 16:06 PDT by Adele Peterson
Modified: 2010-07-08 18:02 PDT (History)
3 users (show)

See Also:


Attachments
patch (23.53 KB, patch)
2010-07-06 19:06 PDT, Adele Peterson
no flags Details | Formatted Diff | Diff
patch (23.95 KB, patch)
2010-07-07 16:28 PDT, Adele Peterson
no flags Details | Formatted Diff | Diff
patch (27.88 KB, patch)
2010-07-07 21:52 PDT, Adele Peterson
no flags Details | Formatted Diff | Diff
patch (26.70 KB, patch)
2010-07-08 12:57 PDT, Adele Peterson
jhoneycutt: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adele Peterson 2010-07-06 16:06:07 PDT
Missing plug-in indicator button should have a pressed state
Comment 1 Adele Peterson 2010-07-06 19:06:34 PDT
Created attachment 60672 [details]
patch

Still needs a test and real ChangeLog comments
Comment 2 Darin Adler 2010-07-07 09:49:30 PDT
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*.
Comment 3 Adele Peterson 2010-07-07 16:28:56 PDT
Created attachment 60800 [details]
patch

same changelog issues as before, but addresses many of Darin's comments
Comment 4 Adele Peterson 2010-07-07 21:52:54 PDT
Created attachment 60834 [details]
patch

now with a test
Comment 5 Adam Roben (:aroben) 2010-07-08 09:15:17 PDT
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.
Comment 6 Adele Peterson 2010-07-08 10:28:53 PDT
Hmm I may post a new patch then.
Comment 7 Adele Peterson 2010-07-08 12:57:47 PDT
Created attachment 60936 [details]
patch
Comment 8 Adam Roben (:aroben) 2010-07-08 13:21:14 PDT
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--)
Comment 9 Jon Honeycutt 2010-07-08 13:25:16 PDT
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
Comment 10 Darin Adler 2010-07-08 14:02:12 PDT
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
Comment 11 Adele Peterson 2010-07-08 18:02:16 PDT
http://trac.webkit.org/changeset/62875