1210478 - Add Meta URL resolution in RemoteNewTabLocation

Status: RESOLVED FIXED

(Firefox :: New Tab Page, defect)

Description

[Olivier Yiptong [:oyiptong]]

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

Comment 1

[Olivier Yiptong [:oyiptong]]

Attachment: MozReview Request: Bug 1210478 - Add Meta URL resolution in RemoteNewTabLocation r=mconley

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

Comment 2

[Marcos Caceres [:marcosc]]

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 3

[Olivier Yiptong [:oyiptong]]

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/

Comment 4

[Olivier Yiptong [:oyiptong]]

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.

Comment 5

[Olivier Yiptong [:oyiptong]]

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.

Comment 6

[Olivier Yiptong [:oyiptong]]

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.

Comment 7

[Olivier Yiptong [:oyiptong]]

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 8

[Olivier Yiptong [:oyiptong]]

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/

Comment 9

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 10

[Olivier Yiptong [:oyiptong]]

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

Comment 11

[Marcos Caceres [:marcosc]]

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.

Comment 12

[Mike Conley (:mconley) (:⚙️)]

(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.

Comment 13

[Olivier Yiptong [:oyiptong]]

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

Comment 14

[Olivier Yiptong [:oyiptong]]

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 15

[Olivier Yiptong [:oyiptong]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9c65e91ae2ca

Comment 16

[Olivier Yiptong [:oyiptong]]

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 17

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 18

[Olivier Yiptong [:oyiptong]]

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 19

[Olivier Yiptong [:oyiptong]]

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/

Comment 20

[Olivier Yiptong [:oyiptong]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1413e8e2ccbd

Comment 21

[Olivier Yiptong [:oyiptong]]

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

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b7e674bbf37b