[webkit-reviews] review granted: [Bug 102149] Change visual look of placeholder : [Attachment 174619] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Jon Lee <jonlee at apple.com>'s request
for review:
Bug 102149: Change visual look of placeholder
https://bugs.webkit.org/show_bug.cgi?id=102149

Attachment 174619: Patch
https://bugs.webkit.org/attachment.cgi?id=174619&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=174619&action=review


> Source/WebCore/rendering/RenderSnapshottedPlugIn.cpp:142
> +    LayoutUnit cWidth = contentWidth();
> +    LayoutUnit cHeight = contentHeight();
> +    if (!cWidth || !cHeight)
> +	   return;

Could we put this in a LayoutSize right from the start and use some function
that checks if it’s empty in either dimension instead of doing it by hand like
this? Would also eliminate the unpleasantly abbreviated cHeight and cWidth
names.

> Source/WebCore/rendering/RenderSnapshottedPlugIn.cpp:152
> +    GraphicsContext* context = paintInfo.context;
> +    GraphicsContextStateSaver saver(*context);

I’d put these lines of code next to the call to drawImage rather than putting
them before all the coordinate math. Also I do not think we need to save and
restore the graphics context state since we are only calling drawImage. So it
could all just be a one liner without these two local variables.

> Source/WebCore/rendering/RenderSnapshottedPlugIn.cpp:154
> +    LayoutSize borderAndPadding(borderLeft() + paddingLeft(), borderTop() +
paddingTop());

Is there really no way to get this more directly?

> Source/WebCore/rendering/RenderSnapshottedPlugIn.cpp:186
> +	   repaint();

Really unpleasant to repaint the entire thing just to make a small image appear
and disappear. I think you should call a repaintButton function even if it has
a FIXME and a call to repaint in it.

Also, this is OK for a temporary solution, but wrong longer term. The repaint
needs to be specifically when the hovered or active state changes. It’s not
right to do a repaint directly based on the mouse events, when the actual
drawing code uses the image’s hovered and active state. That assumes the two
are connected in a way that’s not right.

> Source/WebCore/rendering/RenderSnapshottedPlugIn.cpp:189
> +	   m_isMouseInButtonRect =
m_buttonRect.contains(IntPoint(mouseEvent->offsetX(), mouseEvent->offsetY()));
> +	   repaint();

Should not repaint here unless m_isMouseInButtonRect is changing, is that
right?

> Source/WebCore/rendering/RenderSnapshottedPlugIn.h:56
> +    void paintStart(PaintInfo&, const LayoutPoint&);

This should be called paintButton or perhaps paintStartButton, given the names
of the data members.