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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Jul 8 14:02:12 PDT 2010


--- 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.


Extra space after printf. Could use puts here instead.


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.

More information about the webkit-unassigned mailing list