Add Meta URL resolution in RemoteNewTabLocation

RESOLVED FIXED in Firefox 45

Status

()

Firefox
New Tab Page
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: oyiptong, Assigned: oyiptong)

Tracking

(Blocks: 1 bug)

40 Branch
Firefox 45
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox45 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
Implement the interpolation of a URL like https://newtab.cdn.mozilla.net/v1/%CHANNEL%/%LOCALE%/index.html
(Assignee)

Updated

2 years ago
No longer blocks: 1210049
(Assignee)

Updated

2 years ago
Blocks: 1220227
(Assignee)

Comment 1

2 years ago
Created attachment 8683300 [details]
MozReview Request: Bug 1210478 - Add Meta URL resolution in RemoteNewTabLocation r=mconley

Bug 1210478 - Add Meta URL resolution in RemoteNewTabLocation r?emtwo
Attachment #8683300 - Flags: review?(msamuel)
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)
(Assignee)

Comment 3

2 years ago
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/
(Assignee)

Comment 4

2 years ago
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.
(Assignee)

Comment 5

2 years ago
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.
(Assignee)

Comment 6

2 years ago
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.
(Assignee)

Comment 7

2 years ago
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
(Assignee)

Comment 8

2 years ago
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/
Attachment #8683300 - Attachment description: MozReview Request: Bug 1210478 - Add Meta URL resolution in RemoteNewTabLocation r?emtwo → MozReview Request: Bug 1210478 - Add Meta URL resolution in RemoteNewTabLocation r?mconley
Attachment #8683300 - Flags: review?(msamuel) → review?(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.
(Assignee)

Comment 10

2 years ago
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.
Flags: needinfo?(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.
Flags: needinfo?(mconley)
(Assignee)

Comment 13

2 years ago
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
(Assignee)

Comment 14

2 years ago
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?
(Assignee)

Comment 15

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9c65e91ae2ca
(Assignee)

Comment 16

2 years ago
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/
Attachment #8683300 - Flags: review?(mconley) → review+
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.
(Assignee)

Comment 18

2 years ago
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
(Assignee)

Comment 19

2 years ago
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/
Attachment #8683300 - Attachment description: MozReview Request: Bug 1210478 - Add Meta URL resolution in RemoteNewTabLocation r?mconley → MozReview Request: Bug 1210478 - Add Meta URL resolution in RemoteNewTabLocation r=mconley
(Assignee)

Comment 20

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1413e8e2ccbd
(Assignee)

Comment 21

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

Comment 22

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b7e674bbf37b
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
(Assignee)

Updated

2 years ago
Blocks: 1227274
You need to log in before you can comment on or make changes to this bug.