880753 - Reintroduce public nsLookAndFeel::UseOverlayScrollbars method

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Stephen A Pohl [:spohl]]

In bug 636564, the public nsLookAndFeel::UseOverlayScrollbars was introduced to conveniently check whether or not overlay scrollbars should be used on a system. It was later made protected because this method would ignore the settings in about:config when a user manually turned off overlay scrollbars by setting ui.useOverlayScrollbars (bug 868396). I'd like to reintroduce the public nsLookAndFeel::UseOverlayScrollbars and move the existing one to nsLookAndFeel::UseOverlayScrollbarsInternal to make it more convenient in widget code to make this check.

This patch also fixes a minor problem introduced in bug 873012: nsLookAndFeel::AllowOverlayScrollbarsOverlap() used to ignore a manual override via ui.useOverlayScrollbars in about:config (since it was calling nsLookAndFeel::UseOverlayScrollbars instead of GetInt(eIntID_UseOverlayScrollbars)). This didn't result in any visible misbehavior, but we may as well be consistent and correct here.

Comment 1

[Stephen A Pohl [:spohl]]

Attachment: Patch

Comment 2

[Stephen A Pohl [:spohl]]

Attachment: Patch

Added two missing '()'.

Comment 3

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 759825 [details] [diff] [review]
Patch

I agree with making UseOverlayScrollbars() once again a public method -- with having it be the method to call to fully answer the question of whether not to use overlay scrollbars (since it consults both the system setting and the about:config preference (if it exists)).  I agree that this is less confusing.

But I think we need to find a different name for what you've called UseOverlayScrollbarsInternal().  We need something to better indicate what that method actually does -- which is to find the system (0S) setting.  How about something like SystemWantsOverlayScrollbars()?

Comment 4

[Stephen A Pohl [:spohl]]

Attachment: Patch

> But I think we need to find a different name for what you've called
> UseOverlayScrollbarsInternal().  We need something to better indicate what
> that method actually does -- which is to find the system (0S) setting.  How
> about something like SystemWantsOverlayScrollbars()?

Thanks for pointing that out. Changed method name.

Carrying over r+.

Comment 5

[Stephen A Pohl [:spohl]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/95b53f90183e

Comment 6

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/95b53f90183e

Comment 7

[Stephen A Pohl [:spohl]]

Comment on attachment 759894 [details] [diff] [review]
Patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 868396
User impact if declined: No impact on users. Makes it easier for developers to check whether or not a system uses overlay scrollbars.
Testing completed (on m-c, etc.): Has been on m-c for a few days.
Risk to taking this patch (and alternatives if risky): very low
String or IDL/UUID changes made by this patch: none

Comment 8

[Stephen A Pohl [:spohl]]

Requesting checkin for aurora.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1a4e9188b06d