RESOLVED FIXED 147868
Selects should scale when rendering while zoomed
https://bugs.webkit.org/show_bug.cgi?id=147868
Summary Selects should scale when rendering while zoomed
Wenson Hsieh
Reported 2015-08-10 22:57:43 PDT
When either the page scale or zoom factor is adjusted, selects should scale accordingly.
Attachments
Patch (2.46 KB, patch)
2015-08-11 07:26 PDT, Wenson Hsieh
no flags
Patch (2.40 KB, patch)
2015-08-13 10:15 PDT, Wenson Hsieh
dbates: review+
Wenson Hsieh
Comment 1 2015-08-11 07:26:32 PDT
Daniel Bates
Comment 2 2015-08-11 11:44:11 PDT
Comment on attachment 258715 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=258715&action=review > Source/WebCore/rendering/RenderThemeMac.mm:951 > + float pageScaleFactor = renderer.document().page() ? renderer.document().page()->pageScaleFactor() : 1.0f; > + float deviceScaleFactor = renderer.document().page() ? renderer.document().page()->deviceScaleFactor() : 1.0f; Can you elaborate on a case where this function is called and renderer.document().page() returns nullptr? I mean both the old code and new code assume renderer.document().page() is non-null after drawing the cell (well, the new code actually never calls renderer.document().page()->focusController().setFocusedElementNeedsRepaint() after drawing the cell per my remark below). 1.0f => 1.0 (by <http://www.webkit.org/coding/coding-style.html#float-suffixes> since it is unnecessary to explicitly force floating point given the context). > Source/WebCore/rendering/RenderThemeMac.mm:954 > + if (ThemeMac::drawCellOrFocusRingWithViewIntoContext(popupButton, paintInfo.context, inflatedRect, view, true, shouldDrawFocusRing, shouldUseImageBuffer, deviceScaleFactor) && shouldDrawFocusRing) I do not understand the purpose of the conjunct shouldDrawButtonCell. I mean, the condition of this if-statement will always evaluate to false because drawCellFocusRing(), called by ThemeMac::drawCellOrFocusRingWithViewIntoContext(), is hardcoded to return false (why?). Regardless, it should ThemeMac::drawCellOrFocusRingWithViewIntoContext() that tells the caller whether to the repaint On another note, it is unclear from looking at this line what is the purpose of the fifth argument to drawCellOrFocusRingWithViewIntoContext() without looking at its prototype. I suggest we define a local variable called shouldDrawButtonCell defined to be true and pass this variable as the fifth argument. > Source/WebCore/rendering/RenderThemeMac.mm:955 > + renderer.document().page()->focusController().setFocusedElementNeedsRepaint(); We repeatedly make use of the value of renderer.document().page(). Although the compiler is likely smart enough, I suggest that we cache the return value of renderer.document().page() in a local variable and make us of this local variable.
Daniel Bates
Comment 3 2015-08-11 11:49:22 PDT
(In reply to comment #2) > [...] > Regardless, it should ThemeMac::drawCellOrFocusRingWithViewIntoContext() that tells the caller whether to the repaint I meant to write: It is the responsibility of ThemeMac::drawCellOrFocusRingWithViewIntoContext() to tell the caller to repaint, if necessary, from looking at the implementation of this function.
Daniel Bates
Comment 4 2015-08-11 11:50:36 PDT
Comment on attachment 258715 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=258715&action=review > Source/WebCore/ChangeLog:3 > + Selects should scale when rendering while zoomed. Nit: Please remove the period at the end of this line so as to match the title of the Bugzilla bug.
Wenson Hsieh
Comment 5 2015-08-11 11:54:01 PDT
Comment on attachment 258715 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=258715&action=review >> Source/WebCore/ChangeLog:3 >> + Selects should scale when rendering while zoomed. > > Nit: Please remove the period at the end of this line so as to match the title of the Bugzilla bug. Oops -- nice catch! >> Source/WebCore/rendering/RenderThemeMac.mm:951 >> + float deviceScaleFactor = renderer.document().page() ? renderer.document().page()->deviceScaleFactor() : 1.0f; > > Can you elaborate on a case where this function is called and renderer.document().page() returns nullptr? I mean both the old code and new code assume renderer.document().page() is non-null after drawing the cell (well, the new code actually never calls renderer.document().page()->focusController().setFocusedElementNeedsRepaint() after drawing the cell per my remark below). > > 1.0f => 1.0 (by <http://www.webkit.org/coding/coding-style.html#float-suffixes> since it is unnecessary to explicitly force floating point given the context). Good point. We assume the page is non-null here, so I'm just being unnecessarily cautious. I'll take out these null checks. >> Source/WebCore/rendering/RenderThemeMac.mm:954 >> + if (ThemeMac::drawCellOrFocusRingWithViewIntoContext(popupButton, paintInfo.context, inflatedRect, view, true, shouldDrawFocusRing, shouldUseImageBuffer, deviceScaleFactor) && shouldDrawFocusRing) > > I do not understand the purpose of the conjunct shouldDrawButtonCell. I mean, the condition of this if-statement will always evaluate to false because drawCellFocusRing(), called by ThemeMac::drawCellOrFocusRingWithViewIntoContext(), is hardcoded to return false (why?). Regardless, it should ThemeMac::drawCellOrFocusRingWithViewIntoContext() that tells the caller whether to the repaint > > On another note, it is unclear from looking at this line what is the purpose of the fifth argument to drawCellOrFocusRingWithViewIntoContext() without looking at its prototype. I suggest we define a local variable called shouldDrawButtonCell defined to be true and pass this variable as the fifth argument. Even though the if statement will always return false in this situation, when we implement animated focus rings in the future, we'll be able to just tweak drawCellOrFocusRingWithViewIntoContext to return true if a repaint is required, and this code should handle animations correctly. We had the same situation in the old button rendering code from before I started the refactoring, so I decided to maintain this behavior. >> Source/WebCore/rendering/RenderThemeMac.mm:955 >> + renderer.document().page()->focusController().setFocusedElementNeedsRepaint(); > > We repeatedly make use of the value of renderer.document().page(). Although the compiler is likely smart enough, I suggest that we cache the return value of renderer.document().page() in a local variable and make us of this local variable. Good point. Will do.
Wenson Hsieh
Comment 6 2015-08-13 10:15:08 PDT
Daniel Bates
Comment 7 2015-08-13 12:09:58 PDT
Comment on attachment 258894 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=258894&action=review > Source/WebCore/rendering/RenderThemeMac.mm:950 > + Page *page = renderer.document().page(); Nit: The * should be on the left.
Wenson Hsieh
Comment 8 2015-08-13 12:22:12 PDT
Comment on attachment 258894 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=258894&action=review >> Source/WebCore/rendering/RenderThemeMac.mm:950 >> + Page *page = renderer.document().page(); > > Nit: The * should be on the left. Got it. Should I also change the NSView *view above to match this?
Wenson Hsieh
Comment 9 2015-08-13 13:30:56 PDT
Radar WebKit Bug Importer
Comment 10 2015-11-04 17:48:42 PST
Note You need to log in before you can comment on or make changes to this bug.