Currently the toolbox checks devtools.source-map.client-service.enabled when fetching the url service; but at least the inspector reacts to changes to its underlying pref. So, probably the service and the Frame component should listen as well and change their behavior when the pref changes.

I think this should be a precursor to switching over the inspector.

Comment on attachment 8876924 [details] Bug 1371852 - make Frame component listen for source-map pref changes; https://reviewboard.mozilla.org/r/148254/#review152978 Thanks! This looks reasonable in general, but I'd like to take another look. ::: devtools/client/framework/source-map-url-service.js:80 (Diff revision 1) > * The column number to map. > * @return Promise > * A promise resolving either to the original location, or null. > */ > SourceMapURLService.prototype.originalPositionFor = async function (url, line, column) { > + if (!Services.prefs.getBoolPref("devtools.source-map.client-service.enabled")) { Hmm, prefs are known to be somewhat slow, so I am bit reluctant to test this on every position request. Can you switch to a pref observer here instead? ::: devtools/client/shared/widgets/tooltip/EventTooltipHelper.js (Diff revision 1) > > let doc = this._tooltip.doc; > this.container = doc.createElementNS(XHTML_NS, "div"); > this.container.className = "devtools-tooltip-events-container"; > > - const sourceMapService = this._toolbox.sourceMapURLService; Seems like you can leave this as-is? Seems like extra work to read the service from the toolbox inside the loop, but not sure if there's an actual pref difference...

We discussed this on irc. A pref observer can't be used due to possible races when the ordering of notifications varies. Two other solutions were discussed: 1. moving all checking of the pref to the users of the api (currently the checks are done in both spots) 2. reimplementing the pub/sub model from the earlier (now deleted) service. I'm not sure where I lean. #1 seems like a bit of an abstraction violation, but on the other hand #2 will mean more work when it comes to bug 1179820, because the inspector already has its own pref observer. On the third hand, we're sort of already doing #1 anyway, so a bit more might not be so bad.

I went with #2. Is this really ok? Please push back if not.

Somehow I forgot to commit a hunk, new version in a sec.

Wait, the updated patch is approach #1 (check pref in API users) not #2, right?

Yes, sorry about the confusion.

Comment on attachment 8876924 [details] Bug 1371852 - make Frame component listen for source-map pref changes; https://reviewboard.mozilla.org/r/148254/#review158646 Thanks, I think it looks correct to me, but please check my comments below. :) ::: devtools/client/framework/toolbox.js:573 (Diff revision 3) > */ > get sourceMapURLService() { > if (this._sourceMapURLService) { > return this._sourceMapURLService; > } > - let sourceMaps = this.sourceMapService; > + // The source map URL service will check the pref at the right This comment is incorrect now, right? There's no pref checking inside the URL service, but instead we're expecting API consumers to do the checking... With this approach where each API consumer checks the pref, how many places would we end up with checking the pref in the end? Just thinking through if it becomes unwieldly, will new API consumers forget to do it, etc.

Comment on attachment 8876924 [details] Bug 1371852 - make Frame component listen for source-map pref changes; https://reviewboard.mozilla.org/r/148254/#review158646 > This comment is incorrect now, right? There's no pref checking inside the URL service, but instead we're expecting API consumers to do the checking... > > With this approach where each API consumer checks the pref, how many places would we end up with checking the pref in the end? Just thinking through if it becomes unwieldly, will new API consumers forget to do it, etc. I'll update the comment. I think there won't really be many users of this service -- currently just the frame component (my inspector patches use a new path, but may need to use this path depending on the results of some testing I have yet to do). So I think it should be ok this way.

Using a weak reference in the observer avoids a leak (at least according to local testing).

No, that was hilariously incorrect. Instead maybe I will have to switch to the pub/sub plan to avoid leaks, because having the Frame component implement nsISupportsWeakReference seems weird.

Comment on attachment 8876924 [details] Bug 1371852 - make Frame component listen for source-map pref changes; I ended up going with the subscription model, to avoid leaks. It's changed enough that it needs another review.

Comment on attachment 8876924 [details] Bug 1371852 - make Frame component listen for source-map pref changes; https://reviewboard.mozilla.org/r/148254/#review160400 Thanks, the approach seems solid overall! It seems like some changes are missing for `EventTooltipHelper`, so r- for now. ::: devtools/client/framework/source-map-url-service.js:144 (Diff revision 6) > + if (!subscriptionEntry.promise) { > + const {url, line, column} = subscriptionEntry; > + subscriptionEntry.promise = this.originalPositionFor(url, line, column); > + } > + > + subscriptionEntry.promise.then((resolvedLocation) => { Maybe use `await` here? ::: devtools/client/framework/source-map-url-service.js:178 (Diff revision 6) > + * location. If false, then source maps are disabled > + * and the generated location should be used; in this > + * case the remaining arguments should be ignored. > + */ > +SourceMapURLService.prototype.subscribe = function (url, line, column, callback) { > + if (this._subscriptions) { Maybe early return if `_subscriptions` is null here and elsewhere in the file, for one less indentation level? ::: devtools/client/framework/source-map-url-service.js:218 (Diff revision 6) > + if (this._subscriptions) { > + let key = JSON.stringify([url, line, column]); > + let subscriptionEntry = this._subscriptions.get(key); > + if (subscriptionEntry) { > + let index = subscriptionEntry.callbacks.indexOf(callback); > + if (index !== -1) { Probably good to remove the key from `_subscriptions` when the last callback is removed? ::: devtools/client/shared/widgets/tooltip/EventTooltipHelper.js:66 (Diff revision 6) > > let doc = this._tooltip.doc; > this.container = doc.createElementNS(XHTML_NS, "div"); > this.container.className = "devtools-tooltip-events-container"; > > - const sourceMapService = this._toolbox.sourceMapURLService; > + let sourceMapService = null; Doesn't this file need to use the new `subscribe` API?

Comment on attachment 8876924 [details] Bug 1371852 - make Frame component listen for source-map pref changes; https://reviewboard.mozilla.org/r/148254/#review160400 > Doesn't this file need to use the new `subscribe` API? I had intentionally not switched this code over, but it occurs to me now that the user could change the pref elsewhere and we'd still want the tooltip to react. So, yes, I'll switch it. Thanks for the comment.

Comment on attachment 8876924 [details] Bug 1371852 - make Frame component listen for source-map pref changes; https://reviewboard.mozilla.org/r/148254/#review160400 > I had intentionally not switched this code over, but it occurs to me now that the user could change the pref elsewhere and we'd still want the tooltip to react. So, yes, I'll switch it. Thanks for the comment. Ah, but now that I think about the behavior of the event tooltip... Isn't it only shown on hover, and the init code runs each time you show it? It seems like you can't have an event tooltip on screen while you are changing the pref, so perhaps it can be left as-is?

Comment on attachment 8876924 [details] Bug 1371852 - make Frame component listen for source-map pref changes; https://reviewboard.mozilla.org/r/148254/#review160400 > Ah, but now that I think about the behavior of the event tooltip... Isn't it only shown on hover, and the init code runs each time you show it? It seems like you can't have an event tooltip on screen while you are changing the pref, so perhaps it can be left as-is? For the record, you can open a second window, click to open the event tooltip, and then change the pref in that other window. So, I think it still needs to be changed.

Comment on attachment 8876924 [details] Bug 1371852 - make Frame component listen for source-map pref changes; https://reviewboard.mozilla.org/r/148254/#review160990 Thanks, this looks good to me! :)

I must have deleted the export during a rebase, leading to total breakage.

Pushed by ttromey@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/fb7a9c3916c6 make Frame component listen for source-map pref changes; r=jryans