Closed Bug 1249642 Opened 4 years ago Closed 4 years ago

Enable environment selection in aboutNewTabService.js for remote URLs

Categories

(Firefox :: New Tab Page, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 47
Tracking Status
firefox47 --- fixed

People

(Reporter: oyiptong, Assigned: rrosario)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Given that we're locking down the remote URL for about:newtab, we still need to be able to change the URLs depending on "environments".

Use-cases:

1. development: where we want about:newtab to point to localhost.
2. test: we want to point to mochitest URL locations
3. staging: for CI to test pre-production resources
4. production: for all users

Given the update mechanisms, we also need to make the remote newtab versioning live in a pref. This allows for updates via addons, which will be needed after a signing key update or just an update of the page.

The environments are described in bug 1239118, so there should be some consolidation with the code there.

Note, this only affects remote newtab URLs and this codepath is only ever run when /browser's AboutRedirector is hit.
Assignee: nobody → rrosario
Comment on attachment 8724889 [details]
MozReview Request: Bug 1249642 - Implement remote modes in aboutNewTabService.js r?oyiptong

https://reviewboard.mozilla.org/r/37213/#review33761

Good stuff! r+ with a couple of nits

::: browser/components/newtab/NewTabRemoteResources.jsm:12
(Diff revision 1)
> +  "dev": {origin: "http://localhost"}

Maybe http://localhost:5000 or a port that isn't privileged might be more dev friendly

::: browser/components/newtab/aboutNewTabService.js:161
(Diff revision 1)
> +    if (!MODE_CHANNEL_MAP.hasOwnProperty(mode)) {

nit: please use the `in` operator:

```javascript
if (! mode in MODE_CHANNEL_MAP) {
  ...
}
```
Attachment #8724889 - Flags: review?(oyiptong) → review+
https://reviewboard.mozilla.org/r/37213/#review33761

> nit: please use the `in` operator:
> 
> ```javascript
> if (! mode in MODE_CHANNEL_MAP) {
>   ...
> }
> ```

That's what I had originally and it never goes inside the if statement (`mode in MODE_CHANNEL_MAP` always returns true). I just tried again and the tests blow up becuase it ends up trying a non existing mode. I also tried `if (! mode in Object.keys(MODE_CHANNEL_MAP)) {`. Puzzling.

```
 0:00.62 LOG: Thread-1 INFO "CONSOLE_MESSAGE: (error) [JavaScript Error: "TypeError: MODE_CHANNEL_MAP[mode] is undefined: generateRemoteURL@file:///Users/rlr/dev/newtab-dev/obj-ff-dbg/dist/NightlyDebug.app/Contents/Resources/browser/components/aboutNewTabService.js:164:5
_updateRemoteMaybe@file:///Users/rlr/dev/newtab-dev/obj-ff-dbg/dist/NightlyDebug.app/Contents/Resources/browser/components/aboutNewTabService.js:192:15
EventEmitter_emit@resource://devtools/shared/event-emitter.js:147:11
observe@resource:///modules/NewTabPrefsProvider.jsm:43:13
Preferences._set@resource://gre/modules/Preferences.jsm:123:9
Preferences.set@resource://gre/modules/Preferences.jsm:109:3
test_updates@/Users/rlr/dev/newtab-dev/obj-ff-dbg/_tests/xpcshell/browser/components/newtab/tests/xpcshell/test_AboutNewTabService.js:141:3
TaskImpl_run@resource://gre/modules/Task.jsm:319:40
_do_main@/Users/rlr/dev/newtab-dev/testing/xpcshell/head.js:209:5
_execute_test@/Users/rlr/dev/newtab-dev/testing/xpcshell/head.js:533:5
@-e:1:1
" {file: "resource://devtools/shared/event-emitter.js" line: 152}]
EventEmitter_emit@resource://devtools/shared/event-emitter.js:152:11
observe@resource:///modules/NewTabPrefsProvider.jsm:43:13
Preferences._set@resource://gre/modules/Preferences.jsm:123:9
Preferences.set@resource://gre/modules/Preferences.jsm:109:3
test_updates@/Users/rlr/dev/newtab-dev/obj-ff-dbg/_tests/xpcshell/browser/components/newtab/tests/xpcshell/test_AboutNewTabService.js:141:3
TaskImpl_run@resource://gre/modules/Task.jsm:319:40
_do_main@/Users/rlr/dev/newtab-dev/testing/xpcshell/head.js:209:5
_execute_test@/Users/rlr/dev/newtab-dev/testing/xpcshell/head.js:533:5
@-e:1:1
```
https://reviewboard.mozilla.org/r/37213/#review33761

> That's what I had originally and it never goes inside the if statement (`mode in MODE_CHANNEL_MAP` always returns true). I just tried again and the tests blow up becuase it ends up trying a non existing mode. I also tried `if (! mode in Object.keys(MODE_CHANNEL_MAP)) {`. Puzzling.
> 
> ```
>  0:00.62 LOG: Thread-1 INFO "CONSOLE_MESSAGE: (error) [JavaScript Error: "TypeError: MODE_CHANNEL_MAP[mode] is undefined: generateRemoteURL@file:///Users/rlr/dev/newtab-dev/obj-ff-dbg/dist/NightlyDebug.app/Contents/Resources/browser/components/aboutNewTabService.js:164:5
> _updateRemoteMaybe@file:///Users/rlr/dev/newtab-dev/obj-ff-dbg/dist/NightlyDebug.app/Contents/Resources/browser/components/aboutNewTabService.js:192:15
> EventEmitter_emit@resource://devtools/shared/event-emitter.js:147:11
> observe@resource:///modules/NewTabPrefsProvider.jsm:43:13
> Preferences._set@resource://gre/modules/Preferences.jsm:123:9
> Preferences.set@resource://gre/modules/Preferences.jsm:109:3
> test_updates@/Users/rlr/dev/newtab-dev/obj-ff-dbg/_tests/xpcshell/browser/components/newtab/tests/xpcshell/test_AboutNewTabService.js:141:3
> TaskImpl_run@resource://gre/modules/Task.jsm:319:40
> _do_main@/Users/rlr/dev/newtab-dev/testing/xpcshell/head.js:209:5
> _execute_test@/Users/rlr/dev/newtab-dev/testing/xpcshell/head.js:533:5
> @-e:1:1
> " {file: "resource://devtools/shared/event-emitter.js" line: 152}]
> EventEmitter_emit@resource://devtools/shared/event-emitter.js:152:11
> observe@resource:///modules/NewTabPrefsProvider.jsm:43:13
> Preferences._set@resource://gre/modules/Preferences.jsm:123:9
> Preferences.set@resource://gre/modules/Preferences.jsm:109:3
> test_updates@/Users/rlr/dev/newtab-dev/obj-ff-dbg/_tests/xpcshell/browser/components/newtab/tests/xpcshell/test_AboutNewTabService.js:141:3
> TaskImpl_run@resource://gre/modules/Task.jsm:319:40
> _do_main@/Users/rlr/dev/newtab-dev/testing/xpcshell/head.js:209:5
> _execute_test@/Users/rlr/dev/newtab-dev/testing/xpcshell/head.js:533:5
> @-e:1:1
> ```

Oops, using `in` with an `Array` doesn't do what I expected. But still puzzling that the `! mode in MODE_CHANNEL_MAP` isn't working.
Comment on attachment 8724889 [details]
MozReview Request: Bug 1249642 - Implement remote modes in aboutNewTabService.js r?oyiptong

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37213/diff/1-2/
Comment on attachment 8724889 [details]
MozReview Request: Bug 1249642 - Implement remote modes in aboutNewTabService.js r?oyiptong

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37213/diff/2-3/
https://reviewboard.mozilla.org/r/37213/#review33761

> Oops, using `in` with an `Array` doesn't do what I expected. But still puzzling that the `! mode in MODE_CHANNEL_MAP` isn't working.

Ahhh .I was missing a parens... `if (!(mode in MODE_CHANNEL_MAP)) {` works.
https://hg.mozilla.org/mozilla-central/rev/0010ab7a28fa
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
See Also: → 1256248
You need to log in before you can comment on or make changes to this bug.