WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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-
Details
Formatted Diff
Diff
Patch
(30.62 KB, patch)
2013-09-25 16:47 PDT
,
Beth Dakin
buildbot
: commit-queue-
Details
Formatted Diff
Diff
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
Details
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
Details
Patch
(30.64 KB, patch)
2013-09-25 20:17 PDT
,
Beth Dakin
buildbot
: commit-queue-
Details
Formatted Diff
Diff
Patch
(31.75 KB, patch)
2013-09-26 11:12 PDT
,
Beth Dakin
sam
: review+
buildbot
: commit-queue-
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Beth Dakin
Comment 1
2013-09-25 16:07:20 PDT
Created
attachment 212633
[details]
Patch
Build Bot
Comment 2
2013-09-25 16:44:23 PDT
Comment on
attachment 212633
[details]
Patch
Attachment 212633
[details]
did not pass mac-wk2-ews (mac-wk2): Output:
http://webkit-queues.appspot.com/results/2283053
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
Comment on
attachment 212657
[details]
Patch
Attachment 212657
[details]
did not pass win-ews (win): Output:
http://webkit-queues.appspot.com/results/2188126
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
Comment on
attachment 212727
[details]
Patch
Attachment 212727
[details]
did not pass win-ews (win): Output:
http://webkit-queues.appspot.com/results/2613056
EFL EWS Bot
Comment 16
2013-09-27 02:59:24 PDT
Comment on
attachment 212727
[details]
Patch
Attachment 212727
[details]
did not pass efl-wk2-ews (efl-wk2): Output:
http://webkit-queues.appspot.com/results/2654089
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.
Top of Page
Format For Printing
XML
Clone This Bug