WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
102149
Change visual look of placeholder
https://bugs.webkit.org/show_bug.cgi?id=102149
Summary
Change visual look of placeholder
Jon Lee
Reported
2012-11-13 16:18:27 PST
Remove the gradient and move the button.
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
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1
2012-11-13 16:18:50 PST
<
rdar://problem/12695566
>
Jon Lee
Comment 2
2012-11-14 22:22:12 PST
Created
attachment 174349
[details]
Patch
Jon Lee
Comment 3
2012-11-16 00:11:20 PST
Created
attachment 174619
[details]
Patch
Darin Adler
Comment 4
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.
Jon Lee
Comment 5
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.
Jon Lee
Comment 6
2012-11-16 14:43:59 PST
Committed
r135003
: <
http://trac.webkit.org/changeset/135003
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug