<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - Selects should scale when rendering while zoomed"
href="https://bugs.webkit.org/show_bug.cgi?id=147868#c2">Comment # 2</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - Selects should scale when rendering while zoomed"
href="https://bugs.webkit.org/show_bug.cgi?id=147868">bug 147868</a>
from <span class="vcard"><a class="email" href="mailto:dbates@webkit.org" title="Daniel Bates <dbates@webkit.org>"> <span class="fn">Daniel Bates</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=258715&action=diff" name="attach_258715" title="Patch">attachment 258715</a> <a href="attachment.cgi?id=258715&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=258715&action=review">https://bugs.webkit.org/attachment.cgi?id=258715&action=review</a>
<span class="quote">> 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;</span >
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 <<a href="http://www.webkit.org/coding/coding-style.html#float-suffixes">http://www.webkit.org/coding/coding-style.html#float-suffixes</a>> since it is unnecessary to explicitly force floating point given the context).
<span class="quote">> Source/WebCore/rendering/RenderThemeMac.mm:954
> + if (ThemeMac::drawCellOrFocusRingWithViewIntoContext(popupButton, paintInfo.context, inflatedRect, view, true, shouldDrawFocusRing, shouldUseImageBuffer, deviceScaleFactor) && shouldDrawFocusRing)</span >
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.
<span class="quote">> Source/WebCore/rendering/RenderThemeMac.mm:955
> + renderer.document().page()->focusController().setFocusedElementNeedsRepaint();</span >
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.</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>