147868 2015-08-10 22:57:43 -0700 Selects should scale when rendering while zoomed 2015-11-04 17:48:42 -0800 1 1 1 Unclassified WebKit WebKit Misc. 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 wenson_hsieh webkit-unassigned commit-queue dbates dino esprehn+autocc glenn jonlee kondapallykalyan webkit-bug-importer oldest_to_newest 1116496 0 wenson_hsieh 2015-08-10 22:57:43 -0700 When either the page scale or zoom factor is adjusted, selects should scale accordingly. 1116530 1 258715 wenson_hsieh 2015-08-11 07:26:32 -0700 Created attachment 258715 Patch 1116611 2 258715 dbates 2015-08-11 11:44:11 -0700 Comment on attachment 258715 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. 1116616 3 dbates 2015-08-11 11:49:22 -0700 (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. 1116618 4 258715 dbates 2015-08-11 11:50:36 -0700 Comment on attachment 258715 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. 1116620 5 258715 wenson_hsieh 2015-08-11 11:54:01 -0700 Comment on attachment 258715 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. 1117259 6 258894 wenson_hsieh 2015-08-13 10:15:08 -0700 Created attachment 258894 Patch 1117310 7 258894 dbates 2015-08-13 12:09:58 -0700 Comment on attachment 258894 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. 1117318 8 258894 wenson_hsieh 2015-08-13 12:22:12 -0700 Comment on attachment 258894 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? 1117358 9 wenson_hsieh 2015-08-13 13:30:56 -0700 Committed r188399: <http://trac.webkit.org/changeset/188399> 1139509 10 webkit-bug-importer 2015-11-04 17:48:42 -0800 <rdar://problem/23405529> 258715 2015-08-11 07:26:32 -0700 2015-08-13 10:15:04 -0700 Patch bug-147868-20150811072616.patch text/plain 2520 wenson_hsieh U3VidmVyc2lvbiBSZXZpc2lvbjogMTg4MjQyCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggMzBmZmQ2OGQxZTIzOTkx YjRlODhhMzhiNjM5MWVmNWY2NmEyNGNjNy4uNjU1MzBmZmFkOTM5M2VhM2E5ODE1OTBmODA2MzJk YmVkNzJhNWRlMyAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE1IEBACisyMDE1LTA4LTExICBXZW5z b24gSHNpZWggIDx3ZW5zb25faHNpZWhAYXBwbGUuY29tPgorCisgICAgICAgIFNlbGVjdHMgc2hv dWxkIHNjYWxlIHdoZW4gcmVuZGVyaW5nIHdoaWxlIHpvb21lZC4KKyAgICAgICAgaHR0cHM6Ly9i dWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTE0Nzg2OAorCisgICAgICAgIFdoZW4gcmVu ZGVyaW5nIHpvb21lZCA8c2VsZWN0PiBlbGVtZW50cywgZHJhdyB0byBhbiBpbWFnZSBidWZmZXIg aW5zdGVhZCBvZiBkcmF3aW5nIGRpcmVjdGx5CisgICAgICAgIGludG8gdGhlIGNvbnRleHQuIFRo aXMgYWxsb3dzIHVzIHRvIHNjYWxlIHRoZSBpbWFnZSBidWZmZXIgdXAgYmVmb3JlIHJlbmRlcmlu Zy4KKworICAgICAgICAqIHJlbmRlcmluZy9SZW5kZXJUaGVtZU1hYy5tbToKKyAgICAgICAgKFdl YkNvcmU6OlJlbmRlclRoZW1lTWFjOjpwYWludE1lbnVMaXN0KTogVXNlIFRoZW1lTWFjOjpkcmF3 Q2VsbE9yRm9jdXNSaW5nV2l0aFZpZXdJbnRvQ29udGV4dAorICAgICAgICAgICAgdG8gcmVuZGVy IHNlYXJjaCBmaWVsZHMsIHV0aWxpemluZyBhbiBvZmZzY3JlZW4gaW1hZ2UgYnVmZmVyIG9ubHkg d2hlbiBuZWNlc3NhcnkuCisKIDIwMTUtMDgtMTAgIEFsZXggQ2hyaXN0ZW5zZW4gIDxhY2hyaXN0 ZW5zZW5Ad2Via2l0Lm9yZz4KIAogICAgICAgICBCdWlsZCBUZXN0V2ViS2l0QVBJIHdpdGggQ01h a2Ugb24gV2luZG93cwpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUvcmVuZGVyaW5nL1JlbmRl clRoZW1lTWFjLm1tIGIvU291cmNlL1dlYkNvcmUvcmVuZGVyaW5nL1JlbmRlclRoZW1lTWFjLm1t CmluZGV4IDk0MWUwYTRjYmQ0ZWIwM2EyMDRjOWFjOTMzNmIyMmQ2ZTVkOTI4NTQuLmYzNjFiZTcw YzQ1YzEzMTkyMDRhY2ZjMzhiNWJlMWQ1YTJkYWY1MTEgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJD b3JlL3JlbmRlcmluZy9SZW5kZXJUaGVtZU1hYy5tbQorKysgYi9Tb3VyY2UvV2ViQ29yZS9yZW5k ZXJpbmcvUmVuZGVyVGhlbWVNYWMubW0KQEAgLTk0NywxMSArOTQ3LDEyIEBAIGJvb2wgUmVuZGVy VGhlbWVNYWM6OnBhaW50TWVudUxpc3QoY29uc3QgUmVuZGVyT2JqZWN0JiByZW5kZXJlciwgY29u c3QgUGFpbnRJbmZvCiAgICAgfQogCiAgICAgTlNWaWV3ICp2aWV3ID0gZG9jdW1lbnRWaWV3Rm9y KHJlbmRlcmVyKTsKLSAgICBbcG9wdXBCdXR0b24gZHJhd1dpdGhGcmFtZTppbmZsYXRlZFJlY3Qg aW5WaWV3OnZpZXddOwotICAgIGlmIChpc0ZvY3VzZWQocmVuZGVyZXIpICYmIHJlbmRlcmVyLnN0 eWxlKCkub3V0bGluZVN0eWxlSXNBdXRvKCkpIHsKLSAgICAgICAgaWYgKHdrRHJhd0NlbGxGb2N1 c1JpbmdXaXRoRnJhbWVBdFRpbWUocG9wdXBCdXR0b24sIGluZmxhdGVkUmVjdCwgdmlldywgc3Rk OjpudW1lcmljX2xpbWl0czxkb3VibGU+OjptYXgoKSkpCi0gICAgICAgICAgICByZW5kZXJlci5k b2N1bWVudCgpLnBhZ2UoKS0+Zm9jdXNDb250cm9sbGVyKCkuc2V0Rm9jdXNlZEVsZW1lbnROZWVk c1JlcGFpbnQoKTsKLSAgICB9CisgICAgZmxvYXQgcGFnZVNjYWxlRmFjdG9yID0gcmVuZGVyZXIu ZG9jdW1lbnQoKS5wYWdlKCkgPyByZW5kZXJlci5kb2N1bWVudCgpLnBhZ2UoKS0+cGFnZVNjYWxl RmFjdG9yKCkgOiAxLjBmOworICAgIGZsb2F0IGRldmljZVNjYWxlRmFjdG9yID0gcmVuZGVyZXIu ZG9jdW1lbnQoKS5wYWdlKCkgPyByZW5kZXJlci5kb2N1bWVudCgpLnBhZ2UoKS0+ZGV2aWNlU2Nh bGVGYWN0b3IoKSA6IDEuMGY7CisgICAgYm9vbCBzaG91bGREcmF3Rm9jdXNSaW5nID0gaXNGb2N1 c2VkKHJlbmRlcmVyKSAmJiByZW5kZXJlci5zdHlsZSgpLm91dGxpbmVTdHlsZUlzQXV0bygpOwor ICAgIGJvb2wgc2hvdWxkVXNlSW1hZ2VCdWZmZXIgPSB6b29tTGV2ZWwgIT0gMS4wZiB8fCBwYWdl U2NhbGVGYWN0b3IgIT0gMS4wZjsKKyAgICBpZiAoVGhlbWVNYWM6OmRyYXdDZWxsT3JGb2N1c1Jp bmdXaXRoVmlld0ludG9Db250ZXh0KHBvcHVwQnV0dG9uLCBwYWludEluZm8uY29udGV4dCwgaW5m bGF0ZWRSZWN0LCB2aWV3LCB0cnVlLCBzaG91bGREcmF3Rm9jdXNSaW5nLCBzaG91bGRVc2VJbWFn ZUJ1ZmZlciwgZGV2aWNlU2NhbGVGYWN0b3IpICYmIHNob3VsZERyYXdGb2N1c1JpbmcpCisgICAg ICAgIHJlbmRlcmVyLmRvY3VtZW50KCkucGFnZSgpLT5mb2N1c0NvbnRyb2xsZXIoKS5zZXRGb2N1 c2VkRWxlbWVudE5lZWRzUmVwYWludCgpOwogCiAgICAgW3BvcHVwQnV0dG9uIHNldENvbnRyb2xW aWV3Om5pbF07CiAK 258894 2015-08-13 10:15:08 -0700 2015-08-13 12:09:58 -0700 Patch bug-147868-20150813101451.patch text/plain 2458 wenson_hsieh U3VidmVyc2lvbiBSZXZpc2lvbjogMTg4MjU4CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggZTg5NzY0NjQ4Nzc4OWU2 N2I0MDYyMTkzMzlkYzBlMDM4MWZlMDIxMC4uODFlMWY4NmEyMDk2MDY4N2JjODdjM2M4NmRhNGNm YTYzMzg0NjdkZiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE1IEBACisyMDE1LTA4LTExICBXZW5z b24gSHNpZWggIDx3ZW5zb25faHNpZWhAYXBwbGUuY29tPgorCisgICAgICAgIFNlbGVjdHMgc2hv dWxkIHNjYWxlIHdoZW4gcmVuZGVyaW5nIHdoaWxlIHpvb21lZAorICAgICAgICBodHRwczovL2J1 Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTQ3ODY4CisKKyAgICAgICAgV2hlbiByZW5k ZXJpbmcgem9vbWVkIDxzZWxlY3Q+IGVsZW1lbnRzLCBkcmF3IHRvIGFuIGltYWdlIGJ1ZmZlciBp bnN0ZWFkIG9mIGRyYXdpbmcgZGlyZWN0bHkKKyAgICAgICAgaW50byB0aGUgY29udGV4dC4gVGhp cyBhbGxvd3MgdXMgdG8gc2NhbGUgdGhlIGltYWdlIGJ1ZmZlciB1cCBiZWZvcmUgcmVuZGVyaW5n LgorCisgICAgICAgICogcmVuZGVyaW5nL1JlbmRlclRoZW1lTWFjLm1tOgorICAgICAgICAoV2Vi Q29yZTo6UmVuZGVyVGhlbWVNYWM6OnBhaW50TWVudUxpc3QpOiBVc2UgVGhlbWVNYWM6OmRyYXdD ZWxsT3JGb2N1c1JpbmdXaXRoVmlld0ludG9Db250ZXh0CisgICAgICAgICAgICB0byByZW5kZXIg c2VhcmNoIGZpZWxkcywgdXRpbGl6aW5nIGFuIG9mZnNjcmVlbiBpbWFnZSBidWZmZXIgb25seSB3 aGVuIG5lY2Vzc2FyeS4KKwogMjAxNS0wOC0xMSAgQ2hyaXMgRHVtZXogIDxjZHVtZXpAYXBwbGUu Y29tPgogCiAgICAgICAgIFtXZWJJRExdIEFsbCBpbnRlcmZhY2Ugb2JqZWN0cyBtdXN0IGhhdmUg YSBwcm9wZXJ0eSBuYW1lZCAibmFtZSIKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL3JlbmRl cmluZy9SZW5kZXJUaGVtZU1hYy5tbSBiL1NvdXJjZS9XZWJDb3JlL3JlbmRlcmluZy9SZW5kZXJU aGVtZU1hYy5tbQppbmRleCA5NDFlMGE0Y2JkNGViMDNhMjA0YzlhYzkzMzZiMjJkNmU1ZDkyODU0 Li45OWI0N2UxYWRhNTlmNzVhNjViMTY3ZDk5ODNlZGQzNmM1ZWNhM2Q0IDEwMDY0NAotLS0gYS9T b3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcvUmVuZGVyVGhlbWVNYWMubW0KKysrIGIvU291cmNlL1dl YkNvcmUvcmVuZGVyaW5nL1JlbmRlclRoZW1lTWFjLm1tCkBAIC05NDcsMTEgKzk0NywxNCBAQCBi b29sIFJlbmRlclRoZW1lTWFjOjpwYWludE1lbnVMaXN0KGNvbnN0IFJlbmRlck9iamVjdCYgcmVu ZGVyZXIsIGNvbnN0IFBhaW50SW5mbwogICAgIH0KIAogICAgIE5TVmlldyAqdmlldyA9IGRvY3Vt ZW50Vmlld0ZvcihyZW5kZXJlcik7Ci0gICAgW3BvcHVwQnV0dG9uIGRyYXdXaXRoRnJhbWU6aW5m bGF0ZWRSZWN0IGluVmlldzp2aWV3XTsKLSAgICBpZiAoaXNGb2N1c2VkKHJlbmRlcmVyKSAmJiBy ZW5kZXJlci5zdHlsZSgpLm91dGxpbmVTdHlsZUlzQXV0bygpKSB7Ci0gICAgICAgIGlmICh3a0Ry YXdDZWxsRm9jdXNSaW5nV2l0aEZyYW1lQXRUaW1lKHBvcHVwQnV0dG9uLCBpbmZsYXRlZFJlY3Qs IHZpZXcsIHN0ZDo6bnVtZXJpY19saW1pdHM8ZG91YmxlPjo6bWF4KCkpKQotICAgICAgICAgICAg cmVuZGVyZXIuZG9jdW1lbnQoKS5wYWdlKCktPmZvY3VzQ29udHJvbGxlcigpLnNldEZvY3VzZWRF bGVtZW50TmVlZHNSZXBhaW50KCk7Ci0gICAgfQorICAgIFBhZ2UgKnBhZ2UgPSByZW5kZXJlci5k b2N1bWVudCgpLnBhZ2UoKTsKKyAgICBmbG9hdCBwYWdlU2NhbGVGYWN0b3IgPSBwYWdlLT5wYWdl U2NhbGVGYWN0b3IoKTsKKyAgICBmbG9hdCBkZXZpY2VTY2FsZUZhY3RvciA9IHBhZ2UtPmRldmlj ZVNjYWxlRmFjdG9yKCk7CisgICAgYm9vbCBzaG91bGREcmF3Rm9jdXNSaW5nID0gaXNGb2N1c2Vk KHJlbmRlcmVyKSAmJiByZW5kZXJlci5zdHlsZSgpLm91dGxpbmVTdHlsZUlzQXV0bygpOworICAg IGJvb2wgc2hvdWxkVXNlSW1hZ2VCdWZmZXIgPSB6b29tTGV2ZWwgIT0gMS4wZiB8fCBwYWdlU2Nh bGVGYWN0b3IgIT0gMS4wZjsKKyAgICBib29sIHNob3VsZERyYXdDZWxsID0gdHJ1ZTsKKyAgICBp ZiAoVGhlbWVNYWM6OmRyYXdDZWxsT3JGb2N1c1JpbmdXaXRoVmlld0ludG9Db250ZXh0KHBvcHVw QnV0dG9uLCBwYWludEluZm8uY29udGV4dCwgaW5mbGF0ZWRSZWN0LCB2aWV3LCBzaG91bGREcmF3 Q2VsbCwgc2hvdWxkRHJhd0ZvY3VzUmluZywgc2hvdWxkVXNlSW1hZ2VCdWZmZXIsIGRldmljZVNj YWxlRmFjdG9yKSkKKyAgICAgICAgcGFnZS0+Zm9jdXNDb250cm9sbGVyKCkuc2V0Rm9jdXNlZEVs ZW1lbnROZWVkc1JlcGFpbnQoKTsKIAogICAgIFtwb3B1cEJ1dHRvbiBzZXRDb250cm9sVmlldzpu aWxdOwogCg==