Create a WebChannel for receiving FxA profile change notifications

RESOLVED FIXED in Firefox 39

Status

()

RESOLVED FIXED
4 years ago
10 months ago

People

(Reporter: zaach, Assigned: zaach)

Tracking

unspecified
Firefox 39
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

4 years ago
In order to keep the browser UI in sync we should provide a mechanism to notify observers of changes to a user's profile image. For now, we can watch for WebChannel events from open tabs but in the future we may a more universal approach using push notifications.
(Assignee)

Updated

4 years ago
Iteration: 38.2 - 9 Feb → ---
(Assignee)

Comment 1

4 years ago
Created attachment 8571520 [details] [diff] [review]
Create a WebChannel for receiving FxA profile change notifications
Attachment #8571520 - Flags: review?(mhammond)
Comment on attachment 8571520 [details] [diff] [review]
Create a WebChannel for receiving FxA profile change notifications

Review of attachment 8571520 [details] [diff] [review]:
-----------------------------------------------------------------

I think this looks OK, but we really should have test coverage of the actual channel - any chance of adding a mochitest for this?  I'm also not sure how this is intended to actually be used in practice, and such a test might help clarify this for me.

::: services/fxaccounts/FxAccountsProfileChannel.jsm
@@ +50,5 @@
> +   * WebChannel origin, used to validate origin of messages
> +   */
> +  _webChannelOrigin: null,
> +  /**
> +   * Opens a tab at "this._fxaOAuthStartUrl"

It doesn't seem to open a tab!

@@ +54,5 @@
> +   * Opens a tab at "this._fxaOAuthStartUrl"
> +   * Registers a WebChannel listener and sets up a callback if needed
> +   */
> +  init: function () {
> +    if (!this._channelCallback) {

if init() ever called with _channelCallback non-null?  If not, maybe just throw if it is non-null?  Also, the ctor should probably call .init().

@@ +64,5 @@
> +   * Release all resources that are in use.
> +   */
> +  tearDown: function() {
> +    this._channel.stopListening();
> +    this._channel = null;

should we set this._channelCallback to null here?

@@ +75,5 @@
> +   */
> +  _configureChannel: function() {
> +    // if this.parameters.content_uri is present but not a valid URI, then this will throw an error.
> +    try {
> +      this._webChannelOrigin = Services.io.newURI(this.parameters.content_uri, null, null);

I think we should change 'content_uri' to 'content_url', as presumably it is a string rather than a URI object.  Also, this function should probably just be inline given it is only called in one place, and doesn't seem to configure a channel - it just creates a URI.

@@ +77,5 @@
> +    // if this.parameters.content_uri is present but not a valid URI, then this will throw an error.
> +    try {
> +      this._webChannelOrigin = Services.io.newURI(this.parameters.content_uri, null, null);
> +    } catch (e) {
> +      throw e;

This catch and throw seem pointless - a log.error() might make it valuable though.

@@ +109,5 @@
> +        }
> +      }
> +    };
> +
> +    this._channelCallback = listener.bind(this);

make listener a "fat arrow" function and you can avoid the .bind

::: services/fxaccounts/tests/xpcshell/test_profile_channel.js
@@ +32,5 @@
> +    do_check_eq(data, "foo");
> +    run_next_test();
> +  });
> +
> +  channel.init();

is there any reason .init() isn't called by the constructor in the module?  Seems a bit of a foot-gun that callers might forget to call it.

@@ +42,5 @@
> +  run_next_test();
> +}
> +
> +function makeObserver(aObserveTopic, aObserveFunc) {
> +  let observer = {

I'm pretty sure you can remove this object entirely and just pass the "observe" function below directly to addObserver (ie, there's no longer a hard requirement that the observer be an object implementing nsIObserver)
Attachment #8571520 - Flags: review?(mhammond) → feedback+
(Assignee)

Comment 3

4 years ago
Created attachment 8571698 [details] [diff] [review]
Create a WebChannel for receiving FxA profile change notifications

Thanks markh-- I've updated the patch and added a mochi test. It's ripped out of
a larger patch I'm working on. In that patch we do pass a URI object, sort of like
loop does for the FxAccountsOAuthClient that this profile channel was based on.
Attachment #8571520 - Attachment is obsolete: true
Attachment #8571698 - Flags: review?(mhammond)
Comment on attachment 8571698 [details] [diff] [review]
Create a WebChannel for receiving FxA profile change notifications

Review of attachment 8571698 [details] [diff] [review]:
-----------------------------------------------------------------

Awesome, thanks for the new test!

::: browser/base/content/test/general/browser_fxa_profile_channel.js
@@ +1,5 @@
> +/* Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/
> + */
> +
> +//thisTestLeaksUncaughtRejectionsAndShouldBeFixed("TypeError: this.docShell is null");

can we remove this?
Attachment #8571698 - Flags: review?(mhammond) → review+
(Assignee)

Updated

4 years ago
Blocks: 1139657
https://hg.mozilla.org/mozilla-central/rev/f74f22579856
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39

Updated

10 months ago
Product: Core → Firefox
Target Milestone: mozilla39 → Firefox 39
You need to log in before you can comment on or make changes to this bug.