1138590 - Create a WebChannel for receiving FxA profile change notifications

Status: RESOLVED FIXED

(Firefox :: Firefox Accounts, defect)

Description

[Zachary Carter [:zaach]]

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.

Comment 1

[Zachary Carter [:zaach]]

Attachment: Create a WebChannel for receiving FxA profile change notifications

Comment 2

[Mark Hammond [:markh] [: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)

Comment 3

[Zachary Carter [:zaach]]

Attachment: 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.

Comment 4

[Mark Hammond [:markh] [: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?

Comment 5

[Carsten Book [:Tomcat]]

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