Implement the interpolation of a URL like https://newtab.cdn.mozilla.net/v1/%CHANNEL%/%LOCALE%/index.html

Bug 1210478 - Add Meta URL resolution in RemoteNewTabLocation r?emtwo

https://reviewboard.mozilla.org/r/24269/#review22151 ::: browser/components/newtab/RemoteNewTabLocation.jsm:113 (Diff revision 1) > + if (url !== this._url) { You are doing an object comparison here, so they will always be different. Maybe: \`\`\`JS url.href \!=== this.\_url.href \`\`\` I would suggest that generateDefaultURL() return a the URL as a string (and that \`this.\_url\`). also be a string (argh, this review thing is really confusing)

Comment on attachment 8683300 [details] MozReview Request: Bug 1210478 - Add Meta URL resolution in RemoteNewTabLocation r=mconley Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24269/diff/1-2/

https://reviewboard.mozilla.org/r/24269/#review22151 > You are doing an object comparison here, so they will always be different. Maybe: > > \`\`\`JS > url.href \!=== this.\_url.href > \`\`\` > > I would suggest that generateDefaultURL() return a the URL as a string (and that \`this.\_url\`). also be a string > url.href !=== this._url.href Good catch! > I would suggest that generateDefaultURL() return a the URL as a string (and that `this._url`). also be a string Any reason why you suggest we store the string instead of the URL object? We'd need to generate the string for the href, and origin as well. Less state, less code seemed to be a plus for me.

https://reviewboard.mozilla.org/r/24269/#review22151 :marcosc, your comment revealed a bug in the code. I patched things up and added a few more tests to ensure correct behavior, and fixed linting errors.

https://reviewboard.mozilla.org/r/24269/#review22161 Some changes from the original patch described ::: browser/components/newtab/RemoteNewTabLocation.jsm:25 (Diff revisions 1 - 2) > + "v%VERSION%/%CHANNEL%/%LOCALE%/index.html"; Fixed incorrect URL here ::: browser/components/newtab/RemoteNewTabLocation.jsm:105 (Diff revisions 1 - 2) > - releaseFromUpdateChannel(channel) { > + _releaseFromUpdateChannel(channel) { Made some functions "private" using _ convention ::: browser/components/newtab/tests/xpcshell/test_RemoteNewTabLocation.js:43 (Diff revisions 1 - 2) > - Services.prefs.setBoolPref("intl.locale.matchOS", false); > + let expectedHref = "https://newtab.cdn.mozilla.net" + :marcosc's previous comment revealed a bug. Fixing tests and adding more below.

https://reviewboard.mozilla.org/r/24269/#review22151 > > url.href !=== this._url.href > > Good catch! > > > I would suggest that generateDefaultURL() return a the URL as a string (and that `this._url`). also be a string > > Any reason why you suggest we store the string instead of the URL object? > We'd need to generate the string for the href, and origin as well. Less state, less code seemed to be a plus for me. Here's a github PR if it makes it easier: https://github.com/mozilla/newtab-dev/pull/39/files

Comment on attachment 8683300 [details] MozReview Request: Bug 1210478 - Add Meta URL resolution in RemoteNewTabLocation r=mconley Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24269/diff/2-3/

https://reviewboard.mozilla.org/r/24267/#review22587 ::: browser/components/newtab/RemoteNewTabLocation.jsm:31 (Diff revision 3) > +let RemoteLocationProvider = function() { You only ever export the one instance of this, so is there a good reason to turn this into a JS "class", and create a new instance down on line 167, as opposed to keeping this as a vanilla Object? ::: browser/components/newtab/RemoteNewTabLocation.jsm:62 (Diff revision 3) > + get locale() { I think this functionality is already provided by toolkit/modules/Locale.jsm's getLocale() method. You should probably use that intead. ::: browser/components/newtab/RemoteNewTabLocation.jsm:132 (Diff revision 3) > + let releaseName = this._releaseFromUpdateChannel(UpdateUtils.UpdateChannel); > + let uri = DEFAULT_PAGE_LOCATION > + .replace("%VERSION%", this.version) > + .replace("%LOCALE%", this.locale) > + .replace("%CHANNEL%", releaseName); > + return new URL(uri); This is called all over the place, and it seems wasteful considering that (outside of your test) we will end up returning the same thing over and over again for the common case. Can we cache the URL, and invalidate it if we notice that the Locale has changed or something? ::: browser/components/newtab/RemoteNewTabLocation.jsm:144 (Diff revision 3) > + let url = new URL(newURL); You should probably use BrowserUtils.makeURI instead of importing URL. That's more consistent with what we do elsewhere in the tree, and importing globals always freaks me out. ::: browser/components/newtab/tests/xpcshell/test_RemoteNewTabLocation.js:10 (Diff revision 3) > -add_task(function* () { > +const defaultHref = RemoteNewTabLocation.href; Please ALL_CAPS this so that it's clear that this is a global const. ::: browser/components/newtab/tests/xpcshell/test_RemoteNewTabLocation.js:22 (Diff revision 3) > + let notificationPromise; > > - notificationPromise = changeNotificationPromise(testURL.href); > + notificationPromise = nextChangeNotificationPromise( > + testURL.href, "Remote Location should change"); Might as well merge these. ::: browser/components/newtab/tests/xpcshell/test_RemoteNewTabLocation.js:48 (Diff revision 3) > + Services.prefs.setBoolPref("intl.locale.matchOS", true); > + Services.prefs.setCharPref("general.useragent.locale", "en-GB"); If you can, you should use SpecialPowers.pushPrefEnv to change these prefs. That way, they're automatically reset once your test ends. DXR around for examples. ::: browser/components/newtab/tests/xpcshell/test_RemoteNewTabLocation.js:99 (Diff revision 3) > +add_task(function* test_release_names() { Can you briefly document what each of the add_tasks in this file tests? Doesn't need to be verbose - just to give a general idea for future reviewers.

https://reviewboard.mozilla.org/r/24267/#review22587 > This is called all over the place, and it seems wasteful considering that (outside of your test) we will end up returning the same thing over and over again for the common case. > > Can we cache the URL, and invalidate it if we notice that the Locale has changed or something? this is cached in _url > You should probably use BrowserUtils.makeURI instead of importing URL. That's more consistent with what we do elsewhere in the tree, and importing globals always freaks me out. drop as per conversation

Hi Mike! > You should probably use BrowserUtils.makeURI instead of importing URL. That's more consistent with what we do elsewhere in the tree, and importing globals always freaks me out. Can you kindly clarify why this freaks you out? I use this a lot and I'm worried now :) Without knowing the freak-out rationale (and at the risk of making an arse of myself again), I'd like to voice my strong objection to using BrowserUtils.makeURI(). The non-standard legacy APIs are, IMHO, really f'ing bad: they are poorly documented, they produce undebuggable objects (NoInterfaceHelper or whatever shows up in the console), and just duplicate what the standard URL object does. As such, I would not be in favor of using makeURI (or where possibly, any of the legacy Gecko stuff even if it's used in old Gecko code) where there is now a standardized equivalent (as has now happened with native Promises). Using legacy/proprietary stuff just seems to add more technical debt and make it more difficult for new devs to get into Gecko code.

(In reply to Marcos Caceres [:marcosc] from comment #11) > Hi Mike! > > > You should probably use BrowserUtils.makeURI instead of importing URL. That's more consistent with what we do elsewhere in the tree, and importing globals always freaks me out. > > Can you kindly clarify why this freaks you out? I use this a lot and I'm > worried now :) > > Without knowing the freak-out rationale (and at the risk of making an arse > of myself again), I'd like to voice my strong objection to using > BrowserUtils.makeURI(). The non-standard legacy APIs are, IMHO, really f'ing > bad: they are poorly documented, they produce undebuggable objects > (NoInterfaceHelper or whatever shows up in the console), and just duplicate > what the standard URL object does. > > As such, I would not be in favor of using makeURI (or where possibly, any of > the legacy Gecko stuff even if it's used in old Gecko code) where there is > now a standardized equivalent (as has now happened with native Promises). > Using legacy/proprietary stuff just seems to add more technical debt and > make it more difficult for new devs to get into Gecko code. Hey Marcos! Yeah, after reading through what URL is, I think what you're saying makes sense. It's unfamiliar to me, but I agree that it's likely the most future-proof way forward - hence us dropping the issue. :) So we're all good.

https://reviewboard.mozilla.org/r/24267/#review22587 > If you can, you should use SpecialPowers.pushPrefEnv to change these prefs. That way, they're automatically reset once your test ends. DXR around for examples. I found that SpecialPowers are only for mochitests. Is there an xpcshell-test alternative? https://developer.mozilla.org/en-US/docs/SpecialPowers

https://reviewboard.mozilla.org/r/24267/#review22587 > You only ever export the one instance of this, so is there a good reason to turn this into a JS "class", and create a new instance down on line 167, as opposed to keeping this as a vanilla Object? I had made it so I would avoid an init() invocation in nsBrowserGlue. Perhaps that is warranted? Currently, the init is a side-effect... hmmm. > Might as well merge these. To the risk of sounding thick, can you please elaborate on what to merge?

Comment on attachment 8683300 [details] MozReview Request: Bug 1210478 - Add Meta URL resolution in RemoteNewTabLocation r=mconley Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24269/diff/3-4/

Comment on attachment 8683300 [details] MozReview Request: Bug 1210478 - Add Meta URL resolution in RemoteNewTabLocation r=mconley https://reviewboard.mozilla.org/r/24269/#review22699 This looks fine to me, modulo my question about initting within RemoteAboutNewTab.init, and uninitting there too. ::: browser/components/newtab/tests/xpcshell/test_RemoteNewTabLocation.js:29 (Diff revision 4) > + let notificationPromise; > > - notificationPromise = changeNotificationPromise(testURL.href); > + notificationPromise = nextChangeNotificationPromise( > + testURL.href, "Remote Location should change"); Nit - Combine these. ::: browser/components/newtab/tests/xpcshell/test_RemoteNewTabLocation.js:54 (Diff revision 4) > + let notificationPromise; Instead of declaring this so far away from its definition, just declare and define in the same place. ::: browser/components/nsBrowserGlue.js:856 (Diff revision 4) > RemoteAboutNewTab.init(); > + RemoteNewTabLocation.init(); Out of curiosity, why isn't RemoteNewTabLocation initted inside of RemoteAboutNewTab.init? Is there a reason to init it outside? ::: browser/components/nsBrowserGlue.js:1179 (Diff revision 4) > RemoteAboutNewTab.uninit(); > + RemoteNewTabLocation.uninit(); Ditto from above.

https://reviewboard.mozilla.org/r/24269/#review22699 > Nit - Combine these. As per our conversation, it's a coding convention. If the variable will be set multiple times, it is declared up top. > Instead of declaring this so far away from its definition, just declare and define in the same place. Ditto as above

Comment on attachment 8683300 [details] MozReview Request: Bug 1210478 - Add Meta URL resolution in RemoteNewTabLocation r=mconley Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24269/diff/4-5/

https://hg.mozilla.org/integration/mozilla-inbound/rev/b7e674bbf37ba9d12e5cf37b016c3877492f1824 Bug 1210478 - Add Meta URL resolution in RemoteNewTabLocation r=mconley