RESOLVED FIXED 121928
ScrollbarPainterController should adopt the api to lock overlay scrollbar state
https://bugs.webkit.org/show_bug.cgi?id=121928
Summary ScrollbarPainterController should adopt the api to lock overlay scrollbar state
Beth Dakin
Reported 2013-09-25 15:43:37 PDT
ScrollbarPainterController should adopt the api to lock overlay scrollbar state. This will allow the ScrollbarPainterController and ScrollAnimatorMac to have control over knowing whether or not the scrollbars are currently active, which will allow us to remove a bunch of functions previously implemented in ScrollableArea subclasses to answer that question.
Attachments
Patch (28.27 KB, patch)
2013-09-25 16:07 PDT, Beth Dakin
buildbot: commit-queue-
Patch (30.62 KB, patch)
2013-09-25 16:47 PDT, Beth Dakin
buildbot: commit-queue-
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2 (643.46 KB, application/zip)
2013-09-25 17:52 PDT, Build Bot
no flags
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2 (639.23 KB, application/zip)
2013-09-25 18:50 PDT, Build Bot
no flags
Patch (30.64 KB, patch)
2013-09-25 20:17 PDT, Beth Dakin
buildbot: commit-queue-
Patch (31.75 KB, patch)
2013-09-26 11:12 PDT, Beth Dakin
sam: review+
buildbot: commit-queue-
Beth Dakin
Comment 1 2013-09-25 16:07:20 PDT
Build Bot
Comment 2 2013-09-25 16:44:23 PDT
Beth Dakin
Comment 3 2013-09-25 16:47:43 PDT
Created attachment 212640 [details] Patch Oops, I forgot to include the WK2 parts of this patch before.
Build Bot
Comment 4 2013-09-25 17:52:39 PDT
Comment on attachment 212640 [details] Patch Attachment 212640 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/2230064 New failing tests: fast/loader/image-in-page-cache.html loader/image-loader-adoptNode-assert.html fast/history/visited-generated-content-test.html platform/mac/editing/input/wrapped-line-char-rect.html fast/frames/frame-dead-region.html fast/harness/results.html fast/loader/stateobjects/pushstate-clears-forward-history.html fast/harness/user-preferred-language.html
Build Bot
Comment 5 2013-09-25 17:52:40 PDT
Created attachment 212644 [details] Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-09 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Build Bot
Comment 6 2013-09-25 18:50:09 PDT
Comment on attachment 212640 [details] Patch Attachment 212640 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/2264081 New failing tests: fast/loader/image-in-page-cache.html loader/image-loader-adoptNode-assert.html fast/history/visited-generated-content-test.html platform/mac/editing/input/wrapped-line-char-rect.html fast/frames/frame-dead-region.html fast/harness/results.html fast/loader/stateobjects/pushstate-clears-forward-history.html fast/harness/user-preferred-language.html
Build Bot
Comment 7 2013-09-25 18:50:10 PDT
Created attachment 212651 [details] Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-13 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Beth Dakin
Comment 8 2013-09-25 20:17:32 PDT
Created attachment 212657 [details] Patch Okay, I think this should fix the failing tests. I was missing a null check.
Build Bot
Comment 9 2013-09-25 20:56:35 PDT
Beth Dakin
Comment 10 2013-09-26 11:12:19 PDT
Created attachment 212727 [details] Patch This should fix the windows build.
Sam Weinig
Comment 11 2013-09-26 12:50:42 PDT
Comment on attachment 212727 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=212727&action=review > Source/WebCore/dom/Document.cpp:4072 > + page()->shouldLockOverlayScrollbarsToHidden(false); You should probably either null check page() here or ASSERT if there is some reason it can never be null.
Beth Dakin
Comment 12 2013-09-26 13:02:24 PDT
(In reply to comment #11) > (From update of attachment 212727 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=212727&action=review > > > Source/WebCore/dom/Document.cpp:4072 > > + page()->shouldLockOverlayScrollbarsToHidden(false); > > You should probably either null check page() here or ASSERT if there is some reason it can never be null. I'm pretty sure it will never be null here since I'm pretty sure documentDidResumeFromPageCache() is always called with a Page in place. But I will investigate a bit more before choosing between the two.
Andreas Kling
Comment 13 2013-09-26 13:16:19 PDT
Comment on attachment 212727 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=212727&action=review >>> Source/WebCore/dom/Document.cpp:4072 >>> + page()->shouldLockOverlayScrollbarsToHidden(false); >> >> You should probably either null check page() here or ASSERT if there is some reason it can never be null. > > I'm pretty sure it will never be null here since I'm pretty sure documentDidResumeFromPageCache() is always called with a Page in place. But I will investigate a bit more before choosing between the two. Beth: That is correct. An ASSERT(page()) will be sufficient. > Source/WebCore/page/Page.h:284 > + void shouldLockOverlayScrollbarsToHidden(bool lockOverlayScrollbars); The name of this setter sounds like a getter. > Source/WebCore/platform/mac/ScrollAnimatorMac.h:70 > + virtual bool scrollbarsCanBeActive() const; OVERRIDE FINAL > Source/WebCore/platform/mac/ScrollAnimatorMac.h:115 > + virtual void lockOverlayScrollbarStateToHidden(bool shouldLockState); OVERRIDE FINAL
Beth Dakin
Comment 14 2013-09-26 16:05:57 PDT
Thanks Andreas! I have incorporated all of that feedback.
Build Bot
Comment 15 2013-09-26 22:00:38 PDT
EFL EWS Bot
Comment 16 2013-09-27 02:59:24 PDT
Sam Weinig
Comment 17 2013-09-27 11:15:00 PDT
Comment on attachment 212727 [details] Patch r=me if you fix the things Andreas and I pointed out.
Beth Dakin
Comment 18 2013-09-27 11:34:28 PDT
Thank you Sam and Andreas! http://trac.webkit.org/changeset/156558
Timothy Hatcher
Comment 19 2013-09-27 12:22:34 PDT
This broke the Windows build. I landed an attempted fix. http://trac.webkit.org/changeset/156565
Note You need to log in before you can comment on or make changes to this bug.