62686 2011-06-14 17:53:45 -0700 [Chromium] Switching between overlay and opaque scrollbars causes glitches 2011-08-31 16:05:14 -0700 1 1 1 Unclassified WebKit Platform 528+ (Nightly build) Mac Unspecified RESOLVED FIXED P2 Normal --- 0 sail sail bdakin jamesr thakis webkit.review.bot oldest_to_newest 420904 0 sail 2011-06-14 17:53:45 -0700 1. Run Chromium 2. Open a page with scrollbars 3. Switch between overlay and opaque scrollbars Expected: Scrollbars repaint and no glitches on screen Actual: There are lots of glitches. 421752 1 sail 2011-06-15 21:18:08 -0700 Looks like bdakin just fixed this bug for me :-) https://trac.webkit.org/changeset/88877 451963 2 104116 sail 2011-08-16 16:26:48 -0700 Created attachment 104116 Patch 451964 3 sail 2011-08-16 16:27:21 -0700 bdakin please take a look, thanks. 452088 4 sail 2011-08-16 20:24:17 -0700 change looks ok on try bots: http://build.chromium.org/p/tryserver.chromium/builders/mac_layout_rel/builds/271 http://build.chromium.org/p/tryserver.chromium/builders/win_layout/builds/1304 http://build.chromium.org/p/tryserver.chromium/builders/linux_layout/builds/1146 452104 5 104116 jamesr 2011-08-16 21:13:55 -0700 Comment on attachment 104116 Patch Where's the test? 452123 6 sail 2011-08-16 22:10:04 -0700 (In reply to comment #5) > (From update of attachment 104116 [details]) > Where's the test? Hey James, I'm not sure how to test this. All this code is only used if it's run on Lion. Are any bots running Lion right now? 453433 7 104414 sail 2011-08-18 15:43:45 -0700 Created attachment 104414 Patch 453434 8 sail 2011-08-18 15:44:21 -0700 (In reply to comment #6) > (In reply to comment #5) > > (From update of attachment 104116 [details] [details]) > > Where's the test? > > Hey James, I'm not sure how to test this. All this code is only used if it's run on Lion. Are any bots running Lion right now? I've filed bug 66504 to track the work to add tests for this case. 453435 9 thakis 2011-08-18 15:45:28 -0700 is scrollBarStyleChanged only called on 10.7? maybe you can trigger it by adding css to the scrollbar dynamically and use that to test? If so, just do it in this bug and close the one you just filed. 453441 10 sail 2011-08-18 15:47:15 -0700 (In reply to comment #9) > is scrollBarStyleChanged only called on 10.7? maybe you can trigger it by adding css to the scrollbar dynamically and use that to test? If so, just do it in this bug and close the one you just filed. It's only called on 10.7. It only called by the scroll animator when we get a callback that the scrollbar has switched from overlay to opaque mode. 453448 11 thakis 2011-08-18 15:54:23 -0700 Indeed. I wonder if this is really blocked on 10.7 – I guess we need to add a method "scrollbarStyleChanged" method to layoutTestController to tickle this anyway, and checking for invalidation doesn't require 10.7. Hooking this up to the layoutTestController should be fairly straightforward. jamesr, bdakin: Is the right thing to do here to add a method to layoutTestController, to not have a test, or something else? 453454 12 bdakin 2011-08-18 15:58:50 -0700 (In reply to comment #11) > Indeed. I wonder if this is really blocked on 10.7 – I guess we need to add a method "scrollbarStyleChanged" method to layoutTestController to tickle this anyway, and checking for invalidation doesn't require 10.7. Hooking this up to the layoutTestController should be fairly straightforward. > > jamesr, bdakin: Is the right thing to do here to add a method to layoutTestController, to not have a test, or something else? Hi Nico! I have not been adding tests for the many scrollbar-related bugs I have fixed, which is bad. I haven't been adding them because I couldn't come up with a way to test that was not blocked by 10.7. If you have an idea of how to extend LayoutTestController without relying on 10.7 bots and it works, then adding that would be great! 453456 13 jamesr 2011-08-18 15:59:55 -0700 There's a full complement of 10.7 bots for the apple mac port on build.webkit.org, so if you can come up with a test that works there you should be able to just add a test and put it in on the Skipped list for pre-10.7 platforms and get test coverage on the bots. 453472 14 sail 2011-08-18 16:16:38 -0700 (In reply to comment #12) > (In reply to comment #11) > > Indeed. I wonder if this is really blocked on 10.7 – I guess we need to add a method "scrollbarStyleChanged" method to layoutTestController to tickle this anyway, and checking for invalidation doesn't require 10.7. Hooking this up to the layoutTestController should be fairly straightforward. > > > > jamesr, bdakin: Is the right thing to do here to add a method to layoutTestController, to not have a test, or something else? > > Hi Nico! I have not been adding tests for the many scrollbar-related bugs I have fixed, which is bad. I haven't been adding them because I couldn't come up with a way to test that was not blocked by 10.7. If you have an idea of how to extend LayoutTestController without relying on 10.7 bots and it works, then adding that would be great! Hey Beth, jamesr and I discussed adding a hook into the scrollbar drawing code that would let the layout test choose which type of scrollbar it wants. This means making some changes to the ScrollbarTheme code. Are you ok with this idea? If so I don't mind making the change and adding the tests. Thanks, 453490 15 bdakin 2011-08-18 16:49:21 -0700 (In reply to comment #14) > Hey Beth, jamesr and I discussed adding a hook into the scrollbar drawing code that would let the layout test choose which type of scrollbar it wants. This means making some changes to the ScrollbarTheme code. > I think this is a good way to do it. Thanks! 453633 16 sail 2011-08-18 22:03:53 -0700 > I think this is a good way to do it. Thanks! Awesome. jamesr, can i have a review pleaseee 453644 17 thakis 2011-08-18 22:27:18 -0700 (In reply to comment #16) > Awesome. jamesr, can i have a review pleaseee Usually you include tests along with the fix, now that it looks like the tests aren't blocked on getting 10.7 bots (since a) this might be doable in a bot-agnostic way b) there are 10.7 bots for the mac port) 453806 18 sail 2011-08-19 07:14:21 -0700 (In reply to comment #17) > (In reply to comment #16) > > Awesome. jamesr, can i have a review pleaseee > > Usually you include tests along with the fix, now that it looks like the tests aren't blocked on getting 10.7 bots (since a) this might be doable in a bot-agnostic way b) there are 10.7 bots for the mac port) Writing up the test is going to require changing a lot of the drawing code. Currently all the drawing code looks up the scrollbar style from system settings. I think it's better to decouple that from this change. 456243 19 sail 2011-08-24 11:24:48 -0700 Ping! 460002 20 104414 bdakin 2011-08-31 15:49:51 -0700 Comment on attachment 104414 Patch r=me! 460017 21 104414 webkit.review.bot 2011-08-31 16:05:10 -0700 Comment on attachment 104414 Patch Clearing flags on attachment: 104414 Committed r94243: <http://trac.webkit.org/changeset/94243> 460018 22 webkit.review.bot 2011-08-31 16:05:14 -0700 All reviewed patches have been landed. Closing bug. 104116 2011-08-16 16:26:48 -0700 2011-08-18 15:43:41 -0700 Patch bug-62686-20110816162647.patch text/plain 1673 sail U3VidmVyc2lvbiBSZXZpc2lvbjogOTMwODEKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwppbmRleCA2MGU4ZmM1MzEzOThiMmNk YjZjMDk4YmY5NjBhYTczNzlhODhiYmVhLi42MjhjMjFiZTg4YmY2MDM4NzA2ZGQ0MzkwY2NkY2Q2 MzA1ZDU0OTg4IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvU291 cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTkgQEAKKzIwMTEtMDgtMTYgIFNhaWxl c2ggQWdyYXdhbCAgPHNhaWxAY2hyb21pdW0ub3JnPgorCisgICAgICAgIFtDaHJvbWl1bV0gU3dp dGNoaW5nIGJldHdlZW4gb3ZlcmxheSBhbmQgb3BhcXVlIHNjcm9sbGJhcnMgY2F1c2VzIGdsaXRj aGVzCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD02MjY4 NgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFdoZW4g c3dpdGNoaW5nIGZyb20gb3BhcXVlIHNjcm9sbGJhcnMgdG8gb3ZlcmxheSBzY3JvbGxiYXJzIHRo ZSBwYWdlIGNvbnRlbnQgc2l6ZSB3b3VsZG4ndCBjaGFuZ2UgbGVhdmluZyBhIGVtcHR5IHJlZ2lv biB1bmRlciB0aGUgb3ZlcmxheSBzY3JvbGxiYXIuCisKKyAgICAgICAgVGhlIHByb2JsZW0gd2Fz IHRoYXQgd2Ugd2VyZSByZWx5aW5nIG9uIFNjcm9sbFZpZXc6OnVwZGF0ZVNjcm9sbGJhcnMoKSB0 byBraWNrIG9mZiBhIGxheW91dC4gVW5mb3J0dW5hdGVseSB0aGlzIGRvZXNuJ3QgaGFwcGVuIGJl Y2F1c2UgdXBkYXRlU2Nyb2xsYmFycyBkb2Vzbid0IGRpc3Rpbmd1aXNoIGJldHdlZW4gb3Zlcmxh eSBhbmQgb3BhcXVlIHNjcm9sbGJhcnMuCisKKyAgICAgICAgVG8gZml4IHRoaXMgSSdtIGV4cGxp Y2l0bHkgaW52YWxpZGF0aW5nIHRoZSBsYXlvdXQgd2hlbiBTY3JvbGxWaWV3OjpzY3JvbGxiYXJT dHlsZUNoYW5nZWQoKSBpcyBjYWxsZWQuCisKKyAgICAgICAgKiBwbGF0Zm9ybS9TY3JvbGxWaWV3 LmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OlNjcm9sbFZpZXc6OnNjcm9sbGJhclN0eWxlQ2hhbmdl ZCk6CisKIDIwMTEtMDgtMTUgIEplZmZyZXkgUGZhdSAgPGpwZmF1QGFwcGxlLmNvbT4KIAogICAg ICAgICBOZXcgWE1MIHBhcnNlcjogYWRkIHhtbCBuYW1lc3BhY2UgdG8gdGhlIGRlZmF1bHQgbWFw IG9mIG5hbWVzcGFjZXMKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL1Njcm9s bFZpZXcuY3BwIGIvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vU2Nyb2xsVmlldy5jcHAKaW5kZXgg ODkwYzdmYzM1ODQ3NzEwY2U3M2ExZTc1Y2JhNmU0YWUwZDVhMzVhNy4uODI1ZTM5YWIzNzEwZjNh OTliMDMyMDk1YzMyODkyYzBmYzllYWQ2NiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvcGxh dGZvcm0vU2Nyb2xsVmlldy5jcHAKKysrIGIvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vU2Nyb2xs Vmlldy5jcHAKQEAgLTkzOSw2ICs5MzksNyBAQCBib29sIFNjcm9sbFZpZXc6OmlzU2Nyb2xsQ29y bmVyVmlzaWJsZSgpIGNvbnN0CiAKIHZvaWQgU2Nyb2xsVmlldzo6c2Nyb2xsYmFyU3R5bGVDaGFu Z2VkKCkKIHsKKyAgICBjb250ZW50c1Jlc2l6ZWQoKTsKICAgICB1cGRhdGVTY3JvbGxiYXJzKHNj cm9sbE9mZnNldCgpKTsKIH0KIAo= 104414 2011-08-18 15:43:45 -0700 2011-08-31 16:05:09 -0700 Patch bug-62686-20110818154344.patch text/plain 1809 sail U3VidmVyc2lvbiBSZXZpc2lvbjogOTMzMjgKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwppbmRleCBiMzFkZWY1NjM2NWM2Nzc1 MDk4ZGJjNTJiMzY3MGJkOGI0YmYzZTFkLi5mZmQ4NTAwNmQxMDQwYmU2NjRiZGE5ZjRkMWIyMmQ1 ODdlOWY5ZWZkIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvU291 cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMjEgQEAKKzIwMTEtMDgtMTggIFNhaWxl c2ggQWdyYXdhbCAgPHNhaWxAY2hyb21pdW0ub3JnPgorCisgICAgICAgIFtDaHJvbWl1bV0gU3dp dGNoaW5nIGJldHdlZW4gb3ZlcmxheSBhbmQgb3BhcXVlIHNjcm9sbGJhcnMgY2F1c2VzIGdsaXRj aGVzCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD02MjY4 NgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFdoZW4g c3dpdGNoaW5nIGZyb20gb3BhcXVlIHNjcm9sbGJhcnMgdG8gb3ZlcmxheSBzY3JvbGxiYXJzIHRo ZSBwYWdlIGNvbnRlbnQgc2l6ZSB3b3VsZG4ndCBjaGFuZ2UgbGVhdmluZyBhIGVtcHR5IHJlZ2lv biB1bmRlciB0aGUgb3ZlcmxheSBzY3JvbGxiYXIuCisKKyAgICAgICAgVGhlIHByb2JsZW0gd2Fz IHRoYXQgd2Ugd2VyZSByZWx5aW5nIG9uIFNjcm9sbFZpZXc6OnVwZGF0ZVNjcm9sbGJhcnMoKSB0 byBraWNrIG9mZiBhIGxheW91dC4gVW5mb3J0dW5hdGVseSB0aGlzIGRvZXNuJ3QgaGFwcGVuIGJl Y2F1c2UgdXBkYXRlU2Nyb2xsYmFycyBkb2Vzbid0IGRpc3Rpbmd1aXNoIGJldHdlZW4gb3Zlcmxh eSBhbmQgb3BhcXVlIHNjcm9sbGJhcnMuCisKKyAgICAgICAgVG8gZml4IHRoaXMgSSdtIGV4cGxp Y2l0bHkgaW52YWxpZGF0aW5nIHRoZSBsYXlvdXQgd2hlbiBTY3JvbGxWaWV3OjpzY3JvbGxiYXJT dHlsZUNoYW5nZWQoKSBpcyBjYWxsZWQuCisKKyAgICAgICAgTm8gdGVzdHMgYmVjYXVzZSB0aGVy ZSBhcmUgbm8gTWFjIDEwLjcgY2hyb21pdW0gYm90cyB5ZXQuIEJ1ZyA2NjUwNCB3aWxsIHRyYWNr IHRoZSB3b3JrIHRvIGFkZCB0aGVzZSB0ZXN0cyBvbmNlIHRoZSBib3RzIGFyZSBpbiBwbGFjZS4K KworICAgICAgICAqIHBsYXRmb3JtL1Njcm9sbFZpZXcuY3BwOgorICAgICAgICAoV2ViQ29yZTo6 U2Nyb2xsVmlldzo6c2Nyb2xsYmFyU3R5bGVDaGFuZ2VkKToKKwogMjAxMS0wOC0xOCAgTmljbyBX ZWJlciAgPHRoYWtpc0BjaHJvbWl1bS5vcmc+CiAKICAgICAgICAgRml4ICJtaXNzaW5nIHJldHVy biIgZ2NjIHdhcm5pbmcgaW4gU2Nyb2xsQW5pbWF0b3JOb25lCmRpZmYgLS1naXQgYS9Tb3VyY2Uv V2ViQ29yZS9wbGF0Zm9ybS9TY3JvbGxWaWV3LmNwcCBiL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3Jt L1Njcm9sbFZpZXcuY3BwCmluZGV4IDJlOTZmYzZhNTk2NDFjZGQ3Zjg0Njg0YmMxNGRmOGUyY2E2 MmRjZmMuLjA0MWUzODI3YzFmZWNjM2I4MGQzYzhhMjc5NDZmNDVlODJlODEwMDcgMTAwNjQ0Ci0t LSBhL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL1Njcm9sbFZpZXcuY3BwCisrKyBiL1NvdXJjZS9X ZWJDb3JlL3BsYXRmb3JtL1Njcm9sbFZpZXcuY3BwCkBAIC05MzksNiArOTM5LDcgQEAgYm9vbCBT Y3JvbGxWaWV3Ojppc1Njcm9sbENvcm5lclZpc2libGUoKSBjb25zdAogCiB2b2lkIFNjcm9sbFZp ZXc6OnNjcm9sbGJhclN0eWxlQ2hhbmdlZCgpCiB7CisgICAgY29udGVudHNSZXNpemVkKCk7CiAg ICAgdXBkYXRlU2Nyb2xsYmFycyhzY3JvbGxPZmZzZXQoKSk7CiB9CiAK