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)
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.
Assignee | ||
Comment 1•9 years ago
|
||
Attachment #8592485 -
Flags: review?(tnikkel)
Assignee | ||
Comment 2•9 years ago
|
||
(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).
Updated•9 years ago
|
Attachment #8592485 -
Flags: review?(tnikkel) → review+
Assignee | ||
Comment 3•9 years ago
|
||
green try |
Try push https://treeherder.mozilla.org/#/jobs?repo=try&revision=05a8a21827fb
Comment 6•9 years ago
|
||
(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?
Comment 7•9 years ago
|
||
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
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Comment 9•9 years ago
|
||
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)
Assignee | ||
Comment 10•9 years ago
|
||
(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.
Assignee | ||
Comment 11•9 years ago
|
||
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-
Assignee | ||
Comment 12•9 years ago
|
||
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+
Comment 13•9 years ago
|
||
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.
Comment 14•9 years ago
|
||
/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)
Updated•9 years ago
|
Attachment #8596354 -
Attachment is obsolete: true
Updated•9 years ago
|
Attachment #8596245 -
Attachment is obsolete: true
Comment 15•9 years ago
|
||
Try push that includes this patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d46fdc69a50f
Assignee | ||
Comment 16•9 years ago
|
||
https://reviewboard.mozilla.org/r/7907/#review6669 Ship It!
Assignee | ||
Updated•9 years ago
|
Attachment #8599632 -
Flags: review?(bugmail.mozilla) → review+
Comment 19•9 years ago
|
||
Attachment #8599632 -
Attachment is obsolete: true
Attachment #8620041 -
Flags: review+
Comment 20•9 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•