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.
Looks like bdakin just fixed this bug for me :-) https://trac.webkit.org/changeset/88877
Created attachment 104116 [details] Patch
bdakin please take a look, thanks.
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
Comment on attachment 104116 [details] Patch Where's the test?
(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?
Created attachment 104414 [details] Patch
(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.
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.
(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.
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?
(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!
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.
(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,
(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!
> I think this is a good way to do it. Thanks! Awesome. jamesr, can i have a review pleaseee
(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)
(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.
Ping!
Comment on attachment 104414 [details] Patch r=me!
Comment on attachment 104414 [details] Patch Clearing flags on attachment: 104414 Committed r94243: <http://trac.webkit.org/changeset/94243>
All reviewed patches have been landed. Closing bug.