| Summary: | Change visual look of placeholder | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Jon Lee <jonlee> | ||||||
| Component: | WebKit Misc. | Assignee: | Jon Lee <jonlee> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | andersca, beidson, bweinstein, eric, mjs, webkit-bug-importer, webkit.review.bot | ||||||
| Priority: | P2 | Keywords: | InRadar | ||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | Mac | ||||||||
| OS: | OS X 10.8 | ||||||||
| Bug Depends on: | |||||||||
| Bug Blocks: | 98318 | ||||||||
| Attachments: |
|
||||||||
|
Description
Jon Lee
2012-11-13 16:18:27 PST
Created attachment 174349 [details]
Patch
Created attachment 174619 [details]
Patch
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 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. Committed r135003: <http://trac.webkit.org/changeset/135003> |