[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 #2 from Darin Adler <darin at apple.com>  2010-07-07 09:49:30 PST ---
(From update of attachment 60672)
> +    , 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*.

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