[Webkit-unassigned] [Bug 41721] Missing plug-in indicator button should have a pressed state

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=41721





--- Comment #10 from Darin Adler <darin at apple.com>  2010-07-08 14:02:12 PST ---
(From update of attachment 60936)
> +    // 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

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.