Bug 121928 – ScrollbarPainterController should adopt the api to lock overlay scrollbar state

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Beth Dakin]

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.

[Beth Dakin]

Created attachment 212633 [details]
Patch

[Build Bot]

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]

Created attachment 212640 [details]
Patch

Oops, I forgot to include the WK2 parts of this patch before.

[Build Bot]

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]

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 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]

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]

Created attachment 212657 [details]
Patch

Okay, I think this should fix the failing tests. I was missing a null check.

[Build Bot]

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]

Created attachment 212727 [details]
Patch

This should fix the windows build.

[Sam Weinig]

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]

(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 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]

Thanks Andreas! I have incorporated all of that feedback.

[Build Bot]

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 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 on attachment 212727 [details]
Patch

r=me if you fix the things Andreas and I pointed out.

[Beth Dakin]

Thank you Sam and Andreas! http://trac.webkit.org/changeset/156558

[Timothy Hatcher]

This broke the Windows build. I landed an attempted fix.

http://trac.webkit.org/changeset/156565