Change Preferences::RegisterCallback(AndCall) to either default to ExactMatch for aMatchKind or have no default at all

RESOLVED FIXED in Firefox 55

Status

()

Core
Preferences: Backend
RESOLVED FIXED
7 months ago
7 months ago

People

(Reporter: bz, Assigned: bz)

Tracking

Trunk
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(3 attachments)

The PrefixMatch behavior is a footgun; see bug 1348215.

We have similar footguns right this second in nsJSContext::EnsureStatics for the same reasons.

In general, we only have 40-ish callsites of RegisterCallbackAndCall and 60-ish of RegisterCallback, so auditing them is not prohibitive.
Created attachment 8848699 [details] [diff] [review]
part 1.  Add RegisterPrefixCallback and RegisterPrefixCallbackAndCall APIs to Preferences

This will allow us to remove the PrefixMatch option from the public API version
of RegisterCallback/RegisterCallbackAndCall.

MozReview-Commit-ID: 6D0S35nv88Z
Attachment #8848699 - Flags: review?(nfroyd)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Created attachment 8848700 [details] [diff] [review]
part 2.  Switch Preferences::RegisterCallback/RegisterCallbackAndCall consumers that want prefix matches to the new RefisterPrefixCallback(AndCall) APIs

MozReview-Commit-ID: 2ebVZO4fN6i
Attachment #8848700 - Flags: review?(nfroyd)
Created attachment 8848701 [details] [diff] [review]
part 3.  Change Preferences::RegisterCallback/RegisterCallbackAndCall/UnregisterCallback to do exact matching on the pref name, not prefix matching

MozReview-Commit-ID: GY6J62yWkfk
Attachment #8848701 - Flags: review?(nfroyd)

Updated

7 months ago
Attachment #8848699 - Flags: review?(nfroyd) → review+
Comment on attachment 8848700 [details] [diff] [review]
part 2.  Switch Preferences::RegisterCallback/RegisterCallbackAndCall consumers that want prefix matches to the new RefisterPrefixCallback(AndCall) APIs

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

I think this is good.  I reviewed all the other RegisterCallback usages in tree and none of those looked like they needed to be changed.

::: dom/media/CubebUtils.cpp
@@ +418,5 @@
> +  Preferences::RegisterCallbackAndCall(PrefChanged, PREF_VOLUME_SCALE);
> +  Preferences::RegisterCallbackAndCall(PrefChanged, PREF_CUBEB_LATENCY_PLAYBACK);
> +  Preferences::RegisterCallbackAndCall(PrefChanged, PREF_CUBEB_LATENCY_MSG);
> +  Preferences::RegisterCallbackAndCall(PrefChanged, PREF_CUBEB_BACKEND);
> +  Preferences::RegisterCallbackAndCall(PrefChanged, PREF_CUBEB_LOG_LEVEL);

It looks like we didn't call the PrefChanged callback for this one...shouldn't this one be just ::RegisterCallback?  Or were you changing it because it seemed like a semi-obvious "this is what they really intended"?

::: js/xpconnect/src/XPCJSContext.cpp
@@ +1635,5 @@
> +                                          JS_OPTIONS_DOT_STR, this);
> +
> +#ifdef FUZZING
> +    Preferences::UnRegisterCallback(ReloadPrefsCallback, "fuzzing.enabled", this);
> +#endif    

Nit: trailing whitespace.
Attachment #8848700 - Flags: review?(nfroyd) → review+
Comment on attachment 8848701 [details] [diff] [review]
part 3.  Change Preferences::RegisterCallback/RegisterCallbackAndCall/UnregisterCallback to do exact matching on the pref name, not prefix matching

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

Please send an email to dev-platform about this API change; I'm sure everybody will enjoy the "wait, it worked how?!" moment and comm-central may appreciate the heads-up (even if comm-central DXR apparently has no RegisterCallback hits).
Attachment #8848701 - Flags: review?(nfroyd) → review+
> Or were you changing it because it seemed like a semi-obvious "this is what they really intended"?

The latter.  But you're right, I should get the author of that code to check that part.  I will do that.
Comment on attachment 8848700 [details] [diff] [review]
part 2.  Switch Preferences::RegisterCallback/RegisterCallbackAndCall consumers that want prefix matches to the new RefisterPrefixCallback(AndCall) APIs

Paul, could you look over the dom/media/CubebUtils.cpp changes here, please?  In particular, note the behavior change for PREF_CUBEB_LOG_LEVEL.
Attachment #8848700 - Flags: review?(padenot)
Comment on attachment 8848700 [details] [diff] [review]
part 2.  Switch Preferences::RegisterCallback/RegisterCallbackAndCall consumers that want prefix matches to the new RefisterPrefixCallback(AndCall) APIs

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

This can land. I'll remove it soon anyways, I wrote this because I was not aware we already had a mechanism for this ("logging.foo" -> "verbose" enables verbose logging for the module "foo").

Thanks !
Attachment #8848700 - Flags: review?(padenot) → review+

Comment 9

7 months ago
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4dd21d428e43
part 1.  Add RegisterPrefixCallback and RegisterPrefixCallbackAndCall APIs to Preferences.  r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/3526737363c6
part 2.  Switch Preferences::RegisterCallback/RegisterCallbackAndCall consumers that want prefix matches to the new RefisterPrefixCallback(AndCall) APIs.  r=froydnj,padenot
https://hg.mozilla.org/integration/mozilla-inbound/rev/0c4aa26a808b
part 3.  Change Preferences::RegisterCallback/RegisterCallbackAndCall/UnregisterCallback to do exact matching on the pref name, not prefix matching.  r=froydnj.
https://groups.google.com/d/msg/mozilla.dev.platform/8eBczwUJ9sQ/jBMox5J7CAAJ

Comment 11

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4dd21d428e43
https://hg.mozilla.org/mozilla-central/rev/3526737363c6
https://hg.mozilla.org/mozilla-central/rev/0c4aa26a808b
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.