Bug 102149 - Change visual look of placeholder
Summary: Change visual look of placeholder
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.8
: P2 Normal
Assignee: Jon Lee
URL:
Keywords: InRadar
Depends on:
Blocks: 98318
  Show dependency treegraph
 
Reported: 2012-11-13 16:18 PST by Jon Lee
Modified: 2012-11-16 14:43 PST (History)
7 users (show)

See Also:


Attachments
Patch (23.53 KB, patch)
2012-11-14 22:22 PST, Jon Lee
no flags Details | Formatted Diff | Diff
Patch (37.15 KB, patch)
2012-11-16 00:11 PST, Jon Lee
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jon Lee 2012-11-13 16:18:27 PST
Remove the gradient and move the button.
Comment 1 Radar WebKit Bug Importer 2012-11-13 16:18:50 PST
<rdar://problem/12695566>
Comment 2 Jon Lee 2012-11-14 22:22:12 PST
Created attachment 174349 [details]
Patch
Comment 3 Jon Lee 2012-11-16 00:11:20 PST
Created attachment 174619 [details]
Patch
Comment 4 Darin Adler 2012-11-16 00:20:53 PST
Comment on attachment 174619 [details]
Patch

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.
Comment 5 Jon Lee 2012-11-16 01:44:54 PST
Comment on attachment 174619 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=174619&action=review

>> Source/WebCore/rendering/RenderSnapshottedPlugIn.cpp:142
>> +        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.

There is a LayoutSize.isEmpty() which I will use.

>> Source/WebCore/rendering/RenderSnapshottedPlugIn.cpp:152
>> +    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.

Done.

>> Source/WebCore/rendering/RenderSnapshottedPlugIn.cpp:154
>> +    LayoutSize borderAndPadding(borderLeft() + paddingLeft(), borderTop() + paddingTop());
> 
> Is there really no way to get this more directly?

Actually, combined with the content size above, I can just use contentBoxRect().

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

I'll add a repaintButton function and a FIXME.

>> Source/WebCore/rendering/RenderSnapshottedPlugIn.cpp:189
>> +        repaint();
> 
> Should not repaint here unless m_isMouseInButtonRect is changing, is that right?

Yes.

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

Renamed to paintButton.
Comment 6 Jon Lee 2012-11-16 14:43:59 PST
Committed r135003: <http://trac.webkit.org/changeset/135003>