Closed Bug 880753 Opened 7 years ago Closed 7 years ago

Reintroduce public nsLookAndFeel::UseOverlayScrollbars method

Categories

(Core :: Widget: Cocoa, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla24
Tracking Status
firefox21 --- unaffected
firefox22 --- unaffected
firefox23 + fixed
firefox24 + fixed
firefox-esr17 --- unaffected

People

(Reporter: spohl, Assigned: spohl)

References

Details

(Whiteboard: [lion-scrollbars+])

Attachments

(1 file, 2 obsolete files)

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.
Attached patch Patch (obsolete) — Splinter Review
Attachment #759823 - Flags: review?(smichaud)
Attached patch Patch (obsolete) — Splinter Review
Added two missing '()'.
Attachment #759823 - Attachment is obsolete: true
Attachment #759823 - Flags: review?(smichaud)
Attachment #759825 - Flags: review?(smichaud)
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()?
Attachment #759825 - Flags: review?(smichaud) → review+
Attached patch PatchSplinter Review
> 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+.
Attachment #759825 - Attachment is obsolete: true
Attachment #759894 - Flags: review+
Target Milestone: mozilla23 → mozilla24
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/95b53f90183e
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
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
Attachment #759894 - Flags: approval-mozilla-aurora?
Attachment #759894 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Requesting checkin for aurora.
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.