Closed Bug 1154478 Opened 9 years ago Closed 9 years ago

Force-enable event-regions when APZ is enabled

Categories

(Core :: Panning and Zooming, defect)

All
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: kats, Assigned: kats)

References

Details

Attachments

(2 files, 3 obsolete files)

+++ This bug was initially created as a clone of Bug #1137267 +++

When bug 1137267 was written it was great but before it landed bug 1139213 landed, and so another use of the event-regions pref snuck in.
Attached patch PatchSplinter Review
Attachment #8592485 - Flags: review?(tnikkel)
(I'll wait until bug 1154459 is landed and backed out before landing this, just so we don't add an extra variable at this point).
Attachment #8592485 - Flags: review?(tnikkel) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #2)
> (I'll wait until bug 1154459 is landed and backed out before landing this,
> just so we don't add an extra variable at this point).

What did you mean by "an extra variable" here?
The absence of this fix caused bug 1151667.

It occurs to me that in a case like this, where we want all callers of gfxPrefs::LayoutEventRegionsEnabled() to call nsDisplayListBuilder::IsBuildingLayerEventRegions() instead, it's a footgun for callers to be able to call gfxPrefs::LayoutEventRegionsEnabled() directly.

Here's an attempt to avoid such footguns in the future.

The declaration of the pref is changed to:

  DECL_POTENTIAL_FOOTGUN_GFX_PREF(Live, 
    "layout.event-regions.enabled", LayoutEventRegionsEnabled, bool, false);

and as a result, a call site like:

  gfxPrefs::LayoutEventRegionsEnabled()

produces the following compiler error:

 0:30.65 ../../dist/include/gfxPrefs.h:364:3: error: static_assert failed "Checking this pref directly is a potential footgun! If you really know what you're doing, pass 'gfxPrefs::YesIAmSure()' as an argument to the getter."
 0:30.65   DECL_POTENTIAL_FOOTGUN_GFX_PREF(Live, "layout.event-regions.enabled",          LayoutEventRegionsEnabled, bool, false);
 0:30.65   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 0:30.65 ../../dist/include/gfxPrefs.h:73:22: note: expanded from macro 'DECL_POTENTIAL_FOOTGUN_GFX_PREF'
 0:30.65 static Type Name() { static_assert(sizeof(T) == 0, "Checking this pref directly is a potential footgun! " \
 0:30.65                      ^             ~~~~~~~~~~~~~~
 0:30.65 /home/botond/dev/mozilla/central/layout/generic/nsGfxScrollFrame.cpp:3045:19: note: in instantiation of function template specialization 'gfxPrefs::LayoutEventRegionsEnabled<int>' requested here
 0:30.65         gfxPrefs::LayoutEventRegionsEnabled())
 0:30.65                   ^

As the error message explains, you can make the error go away by changing the call site to be:

  gfxPrefs::LayoutEventRegionsEnabled(gfxPrefs::YesIAmSure())

Kats, what do you think of this? If you like it, I'll clean it up a bit and post it for review.
Attachment #8596245 - Flags: feedback?(bugmail.mozilla)
https://hg.mozilla.org/mozilla-central/rev/48eaa5547dcc
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Here's an alternative approach to avoiding the footgun. On the one hand it's more direct and straightforward, on the other hand it sets a precedent for putting things in gfxPrefs that are not strictly getters and setters for prefs (and related infrastructure).

Kats, let me know which approach you prefer (with 'neither' also being a valid answer).
Attachment #8596354 - Flags: feedback?(bugmail.mozilla)
(In reply to Botond Ballo [:botond] from comment #6)
> (In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #2)
> > (I'll wait until bug 1154459 is landed and backed out before landing this,
> > just so we don't add an extra variable at this point).
> 
> What did you mean by "an extra variable" here?

Just that I knew dvander had done all his try pushes with the event-regions pref explicitly set to true, and I didn't want to introduce a change that might inadvertently break things in a way where it would be hard to tell if it was my change or APZ.
Comment on attachment 8596245 [details] [diff] [review]
Avoid this kind of footgun in the future

Review of attachment 8596245 [details] [diff] [review]:
-----------------------------------------------------------------

So although this is kind of neat I'm not a huge fan of it because this pref is not going to be around forever (we will remove it once APZ is on by default) and it seems unlikely that we will have other prefs in this particular scenario. We do have other prefs that are not meant to be used directly (see ProgressivePaint) but traditionally we have just tacked on DoNotUseDirectly to the name which is a simple low-tech solution that works just fine. I probably should have done that here when tying it to the APZ pref. If we do want a more general compile-time-safe approach to this I would want it to be general enough to work for the existing DoNotUseDirectly prefs which all have platform-specific behavior wrappers.
Attachment #8596245 - Flags: feedback?(bugmail.mozilla) → feedback-
Comment on attachment 8596354 [details] [diff] [review]
Footgun avoidance, alternative approach

Review of attachment 8596354 [details] [diff] [review]:
-----------------------------------------------------------------

IMO this approach is better because it is more generic and we can use it for progressive paint as well with some ifdef'ing
Attachment #8596354 - Flags: feedback?(bugmail.mozilla) → feedback+
Rather than mucking with the DECL_GFX_PREF macro in any way, I just added the DoNotUseDirectly prefix to LayoutEventRegionsEnabled. It's simple and gets the point across.
Attached file MozReview Request: bz://1154478/botond (obsolete) —
/r/7907 - Bug 1154478 - Rename gfxPrefs::LayoutEventRegionsEnabled to LayoutEventRegionsEnabledDoNotUseDirectly. r=kats

Pull down this commit:

hg pull -r d428ba048092270ad6e39c959040d7454945d169 https://reviewboard-hg.mozilla.org/gecko/
Attachment #8599632 - Flags: review?(bugmail.mozilla)
Attachment #8596354 - Attachment is obsolete: true
Attachment #8596245 - Attachment is obsolete: true
Attachment #8599632 - Flags: review?(bugmail.mozilla) → review+
Attachment #8599632 - Attachment is obsolete: true
Attachment #8620041 - Flags: review+
You need to log in before you can comment on or make changes to this bug.