Mike de Boer [:mikedeboer] Assignee
	Description • 10 years ago

The current approach is not e10s friendly. I suggest to change this interface to use cross-compartment messaging as implemented in bug 1022064.

There's WebChannel.jsm that neatly abstracts this and allows for RPC-style messaging.

I'll put up a WIP patch to demonstrate how that'll look like soon.

	Mike de Boer [:mikedeboer] Assignee
	Comment 1 • 10 years ago

Not taking this as it is unnecessary to do this work right now.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 2 • 10 years ago

Dave, you had some WebChannel alternative in a patch somewhere - did that land?

	Dave Townsend [:mossop]
	Comment 3 • 10 years ago

(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #2)
> Dave, you had some WebChannel alternative in a patch somewhere - did that
> land?

It's not landed yet, I still need to write up some tests for it. I was going to try to get around to that this week (bug 1068087).

	:shell escalante
	Comment 4 • 10 years ago

Hi Mike, this bug was found during triage of all the bugs that we haven't taken.  Do you know if it's still relevant.  If yes, what are the benefits/drawbacks so we can determine priorities

	Mike de Boer [:mikedeboer] Assignee
	Comment 5 • 10 years ago

(In reply to sescalante from comment #4)
> Do you know if it's still relevant.

Yes.

> If yes, what are the
> benefits/drawbacks so we can determine priorities

The benefit of having this would be that we'll conform to a more generic mechanism to pass messages between chrome and content, perhaps at the cost of a more verbose mechanism that is WebChannels.
Additionally, WebChannels have built-in e10s support, which is something that our current MozLoopAPI does not have.

I could be that the scope to implement this is too large, so this could serve as a meta bug.

	:shell escalante
	Comment 6 • 10 years ago

Hi GCP, Mike put in the details in comment 7 - how soon do we need this based on e10s plans?

	Gian-Carlo Pascutto [:gcp]
	Comment 7 • 10 years ago

I'm forwarding the needinfo since I know nothing about this.

	Mark Banner (:standard8)
	Comment 8 • 10 years ago

Gavin, who's best to ask about e10s plans? re comment 5 & 6.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 9 • 10 years ago

Not sure what the question is. We're enabling e10s by default on trunk today, for tomorrow's Nightly. We're not expecting it to ride the trains. Making Loop work with e10s should be someone's priority.

	Mike de Boer [:mikedeboer] Assignee
	Comment 10 • 10 years ago

To clarify: this only becomes an e10s issue once we enable remote docShells for in-content pages (pages like about:home, about:looppanel, etc.).

So the enabling of e10s on Nightly by default won't have any negative impact on Loop.

	Maire Reavy [:mreavy]
	Comment 11 • 10 years ago

Moving this to P2 based on our new priority definitions.

	Mike de Boer [:mikedeboer] Assignee
	Comment 12 • 9 years ago

This patch show off the following:

1) MozLoopAPI.jsm exports a LoopAPI object that exposes two methods:
    - `initialize`: sets up two RemotePages message listeners; one for
      about:looppanel and another for about:loopconversation.
    - `destroy`: clears all these listeners.
2) in MozLoopAPI.jsm a struct is defined that contains handlers for all the
   currently available methods on `navigator.mozLoop`:
    - The special LoopRooms, LoopCalls and LoopContacts object APIs are
      defined as wildcard message handlers. This means that a message sent
      as 'Loop:Contacts:Add' will invoke 'Contacts:*' handlers and in turn
      call `LoopRooms::add`, etc.
    - In this patch it contains handler stubs for _all_ mozLoop methods, but
      in practice we can implement handlers one-by-one to migrate gradually.
3) Calls that expect a return value - directly or as arguments to a callback
   function - should add a message listener for messages of the same name as
   the initial message that was sent to chrome.
4) To make dealing with 3) easier, a (React) mixin is introduces that exposes
   one function: `rpc` (name up for discussion!)
    - if no callback is passed as its last arguments, a message is sent to
      chrome without expecting a reply. I implemented one example for a
      'NotifyUITour' call.
    - if a callback is passed, the function will add a one-time listener for
      the same message name to come back from the handler on the chrome side.
      The callback is invoked with the data that is sent along with the message.

That is all! The goal here is to keep things simple - in fact, to simplify the mozLoopAPI.jsm code greatly - and keep the impact on the content pages minimal.

One thing to take into account is that _all_ the messaging is asynchronous; stuff we currently expect to be synchronous, like `getLoopPref()` and all getters like `userProfile`, will need to be converted to be async.
This is not something we can prevent with any architecture, I'm afraid.

	Mike de Boer [:mikedeboer] Assignee
	Comment 13 • 9 years ago

(In reply to Mike de Boer [:mikedeboer] from comment #12)
>     - The special LoopRooms, LoopCalls and LoopContacts object APIs are
>       defined as wildcard message handlers. This means that a message sent
>       as 'Loop:Contacts:Add' will invoke 'Contacts:*' handlers and in turn
>       call `LoopRooms::add`, etc.

s/LoopRooms/LoopContacts/

	Mike de Boer [:mikedeboer] Assignee
	Comment 14 • 9 years ago

This is a patch that, when applied, shows off a fully functional Loop/ Hello panel _without_ using mozLoop directly. Just message passing.
With a little extra effort, the panel could be running in a remote browser.

	Mike de Boer [:mikedeboer] Assignee
	Comment 17 • 9 years ago

This patch is feature-complete, fixes the link-clicker (standalone) page(s).
injectLoopAPI and friends are no more.
Thing left to do: fix all the tests.

	Mark Banner (:standard8)
	Comment 18 • 9 years ago

Comment on attachment 8640593 [details] [diff] [review]
PoC: use message passing for loop about: pages

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

Reminder to provide feedback.

	Mark Banner (:standard8)
	Comment 19 • 9 years ago

Comment on attachment 8640593 [details] [diff] [review]
PoC: use message passing for loop about: pages

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

Dan and I had a chat about this. Generally the ideas here look good. We did think of a few things:

- We wondered about if we should switch to a promise based api (but not arrow functions). WebRTC is already switching to promises, and iirc, the browsers we supporting are promise capable (at least if we keep promises away from the standalone unsupported platform/browser screens).
- this.rpc/this.rpcBatch/this.onPush all feel a bit too low-level descriptions of what is happening. We discussed a few options, and came up with a few alternatives of things like this.loop.send, this.loop.onPush, this.loop.onNotification, this.loop.onMessage or this.browser.onMessage. Nothing firm, but just some ideas for how better to describe it without going into the protocol - we're communicating with the backend, but the general developer doesn't need to know its happening.
- The rpc functions in the mixins are very difficult to understand when you start reading them. Breaking them down into separate functions, and possibly adding more descriptions would likely help.

	Mark Banner (:standard8)
	Comment 20 • 9 years ago

Bumping up, this should be a priority after the visual refresh, given the current e10s shipping expectations.

	:shell escalante
	Comment 21 • 9 years ago

updated target for e10s going to release in whiteboard - Fx44.

	Romain Testard [:RT]
	Comment 22 • 9 years ago

My understanding is that e10s will make it to 43 Beta and that we have no ability to disable window/tab sharing on Hello for e10s builds.
Last week we discussed that this could be uplifted to 43 to allow beta users to not have a broken sharing experience (there will be a marketing push for Beta users so we need to ensure the experience is right) - Shell and Mike can you please confirm if these assumptions are still true, in which case it sounds like we need to address this in the sprint starting today? Disabling the tab/window sharing button for e10s users could be an option but is not preferred since marketing is willing to test different promo messages around the sharing value proposition.

	Mike de Boer [:mikedeboer] Assignee
	Comment 23 • 9 years ago

Romain, I think we answered your questions during the sprint planning meeting?

	Romain Testard [:RT]
	Comment 24 • 9 years ago

Yes, so:
- uplift is not possible
- We work towards having this fixed in 44
- In 43 we want to disable tab sharing if e10s is enabled - bug 1208402 now created to address this

	Mike de Boer [:mikedeboer] Assignee
	Comment 27 • 9 years ago

This patch is where the magic happens - navigator.mozLoop is gone and everything is message-passing style.

Please note that I haven't fixed/ updated the unit tests and the ui-showcase yet. That's for part 3.
Also note that `publish` and `publishMulti` are by no means meant to be the final names for the API. Mark mentioned `request` as an option... suggestions welcome!

	Mike de Boer [:mikedeboer] Assignee
	Comment 28 • 9 years ago

Unbitrotted version.

	Mark Banner (:standard8)
	Comment 30 • 9 years ago

Comment on attachment 8673612 [details] [diff] [review]
Patch 1.2: add a publish/ subscribe mechanism to sweeten chrome to content message passing

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

I'm still looking at this in a bit more detail, but here's some initial comments in the meantime.

Eslint says there's a couple of missing semi-colons.

Also, I realise we can't get 100% coverage because of `var loop = loop || {};` but could you please add tests for the branch conditions that aren't currently covered.

::: browser/components/loop/content/shared/js/pubsub.js
@@ +20,5 @@
> +   * Publish a message to chrome, running in the main process.
> +   *
> +   * @param {String} action Required name of the action to publish.
> +   * @return {Promise} Gets resolved with the result of the action, IF the chrome
> +   *                   script sent a reply. It never gets rejected.

I immediately wondered about the case where chrome doesn't send a response. However, it seems you handle timeout already, so maybe that needs documenting.

::: browser/components/loop/jar.mn
@@ +100,5 @@
>    content/browser/loop/shared/js/dispatcher.js          (content/shared/js/dispatcher.js)
>    content/browser/loop/shared/js/models.js              (content/shared/js/models.js)
>    content/browser/loop/shared/js/mixins.js              (content/shared/js/mixins.js)
>    content/browser/loop/shared/js/otSdkDriver.js         (content/shared/js/otSdkDriver.js)
> +  content/browser/loop/shared/js/pubsub.js              (content/shared/js/pubsub.js)

Although this is shared between panel and conversation window, I don't think its intended to be shared with standalone as well - so I think we should put this in the desktop specific files under content/js rather than the shared directory.

	Mark Banner (:standard8)
	Comment 31 • 9 years ago

Comment on attachment 8673612 [details] [diff] [review]
Patch 1.2: add a publish/ subscribe mechanism to sweeten chrome to content message passing

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

Ok, here's the rest of my comments. I think its generally not too bad but a few improvements could be made.

::: browser/components/loop/content/shared/js/pubsub.js
@@ +79,5 @@
> +
> +      var seq = ++loop._lastMessageID;
> +      var action = call.shift();
> +      // Normalize the action name to look like 'SomeAction'.
> +      action = action.charAt(0).toUpperCase() + action.substr(1);

The shift/change/unshift seems to be repeated especially compared to publish - I think this should be split out to its own function, which should also make what is happening a bit clearer.

::: browser/components/loop/test/shared/pubsub_test.js
@@ +38,5 @@
> +    });
> +
> +    it("should correct the action name", function() {
> +      loop.publish("getLoopPref", "enabled");
> +      sinon.assert.calledWithExactly(fakeWindow.sendAsyncMessage, "Loop:Message",

I think I've had issues doing this before. Although you expect loop.publish to call the items within the Promise before it returns, I don't think that's always guaranteed. Although its more verbose, I would suggest handling the promise return to ensure that we don't get any issues with intermittent failures.

@@ +56,5 @@
> +
> +      loop.publish("GetLoopPref", "enabled").then(function(result) {
> +        expect(result).to.eql("result");
> +        sinon.assert.calledOnce(fakeWindow.removeMessageListener);
> +        next();

As we have chai-as-promised, you should be able to change this to:

return loop.publish("GetLoopPref", "enabled").then(function(result) {
  expect(result).to.eql("result");
  sinon.assert.calledOnce(fakeWindow.removeMessageListener);
});

and then you don't need the async handling of `next`.

	Mark Banner (:standard8)
	Comment 32 • 9 years ago

Comment on attachment 8673610 [details] [diff] [review]
Patch 2.1: transition from the navigator.mozLoop API to the RemotePageManager API

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

I've had a brief look through, and the comment below is the one big thing that jumps out at me so far. I'm passing onto Dan for some further feedback as I'm out Monday/Tuesday.

::: browser/components/loop/content/js/panel.jsx
@@ +18,5 @@
>  
> +    componentWillMount: function() {
> +      loop.publish("GetLoopPref", "gettingStarted.seen").then(function(result) {
> +        this.setState({ seen: result });
> +      }.bind(this));

I'm a little concerned about these componentWill/DidMount etc. Setting the value outside of the initial mount could cause flicker or other temporarily random results.

They are certainly likely to affect performance.

The only way I can think of getting around it is to create a utility that pre-fetched most of the prefs we use.

	Dan Mosedale (:dmosedale, :dmose)
	Comment 33 • 9 years ago

I'll have a look at this on Monday..

	Mike de Boer [:mikedeboer] Assignee
	Comment 34 • 9 years ago

(In reply to Mark Banner (:standard8) from comment #32)
> I'm a little concerned about these componentWill/DidMount etc. Setting the
> value outside of the initial mount could cause flicker or other temporarily
> random results.
> 
> They are certainly likely to affect performance.
> 
> The only way I can think of getting around it is to create a utility that
> pre-fetched most of the prefs we use.

Yes! But I wanted to keep that as a phase-2 type thing to implement.

	Mike de Boer [:mikedeboer] Assignee
	Comment 35 • 9 years ago

(In reply to Mark Banner (:standard8) from comment #30)
> I immediately wondered about the case where chrome doesn't send a response.
> However, it seems you handle timeout already, so maybe that needs
> documenting.

Yes. For efficiency, the callee might not reply, because it doesn't say anything more than an 'ACK'. Perhaps that's useful enough to make it mandatory for each message to reply, but right now I assumed not.
I'll document the logic, regardless.

> Although this is shared between panel and conversation window, I don't think
> its intended to be shared with standalone as well - so I think we should put
> this in the desktop specific files under content/js rather than the shared
> directory.

Well, this mechanism will also be used in the areas where standaloneMozLoop is used (see patch 2). So that's why it's in shared.

	Dan Mosedale (:dmosedale, :dmose)
	Comment 36 • 9 years ago

Comment on attachment 8673612 [details] [diff] [review]
Patch 1.2: add a publish/ subscribe mechanism to sweeten chrome to content message passing

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

::: browser/components/loop/content/shared/js/pubsub.js
@@ +55,5 @@
> +  // This function should only be used in unit tests.
> +  loop.publish.stub = function(rootObj) { gRootObj = rootObj; };
> +
> +  /**
> +   * Publish multiple actions at once as a batch.

I'm assuming the "action" here is the same thing as the "message" in the publish method.  I'd suggest unifying on the "message" terminology, since so far we haven't overloaded the "action" stuff in the content space, and it'd be kinda nice to avoid overloading it right at the content/chrome boundary to avoid confusion.

	Dan Mosedale (:dmosedale, :dmose)
	Comment 37 • 9 years ago

Comment on attachment 8673610 [details] [diff] [review]
Patch 2.1: transition from the navigator.mozLoop API to the RemotePageManager API

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

From what I've looked at, this is looking good.  This has clearly been a lot work, thanks!  It's great not seeing the god object passed down everywhere. 

My primary concern so far is the pubsub wrapper doesn't feel like it quite matches the semantics of the client code is trying to do, but that probably doesn't want to block this landing. 

There's still about half of the patch that I haven't gone over.  I can continue on and do that tomorrow if it's useful, but if Mark gets to a point where he's comfortable moving this forward, I don't think that needs to block landing.  Thoughts on that are welcome.

::: browser/components/loop/content/js/conversation.jsx
@@ +97,5 @@
> +      ["GetLocale"],
> +      ["GetLoopPref", "ot.guid"],
> +      ["GetLoopPref", "textChat.enabled"],
> +      ["GetLoopPref", "feedback.periodSec"],
> +      ["GetLoopPref", "feedback.dateLastSeenSec"]

This feels odd in a readability sense, since if this were truly pub-sub, it seems like we'd be subscribing to most of these things rather than publishing a get request and then implicitly awaiting a response.

I'm having a hard time imagining it being worth the effort to shift the code around just to match that pattern (though I suppose it might make sense as part of a future patch).

But I'm also wondering if there's an easy way to massage the API introduced by patch 1.2 to make things clearer....

@@ +125,5 @@
> +        },
> +        set: function(guid, callback) {
> +          // See nsIPrefBranch
> +          const PREF_STRING = 32;
> +          sharedMixins.RPCMixin.rpc("SetLoopPref", "ot.guid", guid, PREF_STRING);

Is this left over from a previous iteration?

::: browser/components/loop/content/js/panel.jsx
@@ +985,5 @@
> +      ["GetAllConstants"],
> +      ["GetAllStrings"],
> +      ["GetLocale"],
> +      ["GetPluralRule"]
> +    ).then(function(results) {

It'd be nice if this function got broken up into 2/3 sub functions for more maintainability.

::: browser/components/loop/content/js/roomStore.js
@@ +124,5 @@
> +      loop.publish("Rooms:PushSubscription", ["add", "update", "delete", "refresh"]);
> +      loop.subscribe("Rooms:Add", this._onRoomAdded.bind(this));
> +      loop.subscribe("Rooms:Update", this._onRoomUpdated.bind(this));
> +      loop.subscribe("Rooms:Delete", this._onRoomRemoved.bind(this));
> +      loop.subscribe("Rooms:Refresh", this._onRoomsRefresh.bind(this));

It seems like this kind of API wants a wrapper that DRYs up calling code like this somewhat: it seems pretty easy to inadvertently get some piece of this code out of sync with some other piece (eg misspell something, remove or add something one place but not the other).

@@ +270,5 @@
>  
> +      loop.publish("Rooms:Create", roomCreationData).then(function(result) {
> +        var buckets = this._constants.ROOM_CREATE;
> +        if (result.isError) {
> +          this.rpc("TelemetryAddValue", "LOOP_ROOM_CREATE", buckets.CREATE_FAIL);

Another leftover, I think?

::: browser/components/loop/content/libs/l10n.js
@@ +15,5 @@
> +  // based on the plural rule number specified. The first element is the number
> +  // of plural forms and the second is the function to figure out the index.
> +  // NOTE: these rule functions are - unfortunately - a copy from the `gFunctions`
> +  //       array in intl/locale/PluralForm.jsm. An attempt should be made to keep
> +  //       this in sync with that source.

It would be helpful to explain why it's necessary to use a copy here, and under what conditions (if any) this copy could be avoided.

::: browser/components/loop/modules/LoopRooms.jsm
@@ +783,5 @@
>     * Refreshes a room
>     *
>     * @param {String} roomToken    The room token.
> +   * @param {String} sessionToken Optional. The session token for the session
> +   *                              that has been joined.

Probably handy to document why an API consumer would bother to pass a session token if it's optional.

::: browser/components/loop/standalone/content/js/standaloneMozLoop.js
@@ +118,5 @@
>       *                            is the response data.
>       */
>      _postToRoom: function(roomToken, sessionToken, roomData, expectedProps,
>                            async, callback) {
> +      var url = gBaseServerURL + "/rooms/" + roomToken;

Nit: the g and k hungarian-style prefixes are not generally used on the content side, so probably worth removing here.

	Dan Mosedale (:dmosedale, :dmose)
	Comment 38 • 9 years ago

(In reply to Dan Mosedale (:dmose) - use needinfo flag for response from comment #36)
> Comment on attachment 8673612 [details] [diff] [review]
> Patch 1.2: add a publish/ subscribe mechanism to sweeten chrome to content
> message passing
> 
> Review of attachment 8673612 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/loop/content/shared/js/pubsub.js
> @@ +55,5 @@
> > +  // This function should only be used in unit tests.
> > +  loop.publish.stub = function(rootObj) { gRootObj = rootObj; };
> > +
> > +  /**
> > +   * Publish multiple actions at once as a batch.
> 
> I'm assuming the "action" here is the same thing as the "message" in the
> publish method. 

OK, I was clearly confused here; never mind that.

> I'd suggest unifying on the "message" terminology, since so
> far we haven't overloaded the "action" stuff in the content space, and it'd
> be kinda nice to avoid overloading it right at the content/chrome boundary
> to avoid confusion.

It would still be nice to avoid overloading "action", but I don't think it's critical.  Maybe "command"?

	Dan Mosedale (:dmosedale, :dmose)
	Comment 39 • 9 years ago

Comment on attachment 8673610 [details] [diff] [review]
Patch 2.1: transition from the navigator.mozLoop API to the RemotePageManager API

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

browser-loop.js doesn't seem to have any automated tests, unlike many of the other browser/base/content/browser-*.  How about adding tests for at least the bits of code that you added/changed?

::: browser/components/loop/content/js/conversation.jsx
@@ +103,5 @@
> +      var constants = results[0];
> +      // Do the initial L10n setup, we do this before anything
> +      // else to ensure the L10n environment is setup correctly.
> +      var stringBundle = results[1];
> +      var locale = results[2];

Picking the results numerically out of an array feels fragile.  I'm not sure if it's worth trying to shift the api to make this easier to avoid.  One possibility that comes to mind is having requestMulti accept a hash of named operations to perform in a non-deterministic order, and returning a hash of results.  Thoughts on that or other ideas?

::: browser/components/loop/content/js/conversationAppStore.js
@@ +28,5 @@
>  
>      this._activeRoomStore = options.activeRoomStore;
>      this._dispatcher = options.dispatcher;
> +    this._feedbackPeriod = options.feedbackPeriod;
> +    this._feedbackTimestamp = options.feedbackTimestamp;

These are going to default to 0 if noone passes these in, which I suspect is not what we want.

On a related note, when Conversation.jsx is creating an instance of this object, it's not passing anything in for these options.  :-)

::: browser/components/loop/content/js/panel.jsx
@@ +848,4 @@
>        };
>      },
>  
> +    _serviceErrorToShow: function(callback) {

Looks like callback isn't used.

::: browser/components/loop/content/js/roomStore.js
@@ +141,4 @@
>       * @param {Object} addedRoomData The added room data.
>       */
> +    _onRoomAdded: function(addedRoomData) {
> +      addedRoomData = addedRoomData[0];

This and the other analogous changes feel like they want to either have the jsdoc changed to make it clear what's going on here, or a simple wrapper script that dereferences the real data out from the array and returns that.

@@ +377,5 @@
> +          case "mail.google.com":
> +            shareTitle = mozL10n.get("share_email_subject5", {
> +              clientShortname2: mozL10n.get("clientShortname2")
> +            });
> +            shareBody = mozL10n.get("share_email_body5", {

Both this and the title have changed from ending in 6 to ending in 5.  I've never seen this happen before.  Is it intentional?

::: browser/components/loop/content/libs/l10n.js
@@ +204,5 @@
>      initialize: function(l10nDetails) {
>        gL10nDetails = l10nDetails;
>        gLanguage = gL10nDetails.locale;
> +      // Fallback to a working - synchronous - implementation of retrieving the
> +      // plural form of a string.

So it's unclear to me why all this plural forms code change relates to the e10s changes.  Can you elaborate?

In fact, I seem to recall while reviewing the removal of the contacts code something along the lines of removing the only plural form usages that I could see.  Is it possible that this is no longer necessary?

::: browser/components/loop/content/shared/js/activeRoomStore.js
@@ +1088,5 @@
>        }
>  
> +      var calls = [
> +        ["SetScreenShareState", this.getStoreState().windowId, false]
> +      ];

If this is purely for performance, I'd suggest holding off until profiling suggests that we need it, as it reduces readability compared to just calling "loop.request" once for each call.c

::: browser/components/loop/content/shared/js/mixins.js
@@ +350,4 @@
>      },
>  
> +    _canPlay: function(callback) {
> +      return new Promise(function(resolve) {

JSDoc about why this behaves the way it does seems desirable.

::: browser/components/loop/content/shared/js/utils.js
@@ -417,5 @@
> -    if (typeof bucket === "undefined") {
> -      console.error("No URL sharing type bucket found for '" + from + "'");
> -      return;
> -    }
> -    mozLoop.telemetryAddValue("LOOP_SHARING_ROOM_URL", bucket);

This feels like it conceptually belongs in the store, where it was before.  But in this patch, it seems to have moved to the view.  Why?

::: browser/components/loop/modules/MozLoopService.jsm
@@ +1507,1 @@
>    },

If we really need to add a side-effect here, do we want to have this as a normal method named in a way that expresses what it does, rather than using a getter.

::: browser/components/loop/standalone/content/js/standaloneMozLoop.js
@@ +18,5 @@
> +  var kBatchMessage = "Batch";
> +
> +  var gBaseServerURL = null;
> +
> +  function cloneableError(err) {

Maybe just call this nativeErrorObject()?

@@ +298,3 @@
>    };
>  
> +  function handleBatchMessage(name, seq, data, reply) {

This wants jsdoc describing its semantics.

@@ +329,5 @@
> +      });
> +    });
> +  }
> +
> +  window.sendAsyncMessage = window.sendAsyncMessage || function(name, data, reply) {

Why is this test necessary?  To protect against multiple loads of this file?

I also wonder if a rootObject pattern might benefit testability.  I suppose you can do that if it comes up, though...

Same questions for the subsequent functions...

	Dan Mosedale (:dmosedale, :dmose)
	Comment 40 • 9 years ago

I've gone over everything except MozLoopAPI.jsm, which I'll admit that I'm kinda hoping Mark will be up for taking.  :-)

On the content side, one thing that keeps coming back to me as I've been reviewing is that the API doesn't feel very webby, and as a result has made the code somewhat harder to read.  I get that some of this is a side effect of us having switched a bunch of apis from synchronous to asynchronous, and that there is intrinsically a code complexity cost to that.  However, it feels like there part of the issue may also be a clash of API styles, e.g. that the DOM in general seems to manage asynchronicity without passing piles of arrays around.  It could be that this is really just a result of the batching API.  I don't have any fantastic suggestions here (other than perhaps avoiding batching until profiling shows us that we need it), but maybe one of you guys does!

When developing the unit tests, it'd be great to use the code-coverage tools on the content-side stuff to give us a sufficiently good chance catch edge-casey bugs before this lands.

Anyway, thanks again for doing all this hard work.

	Dan Mosedale (:dmosedale, :dmose)
	Comment 46 • 9 years ago

Does this patch continue to work in the non-e10s case?  What I'm wondering is, suppose we land this patch, subsequent client code depends on the new APIs, and then e10s sits turned off on beta for multiple release cycles... does Hello still work?

	Mike de Boer [:mikedeboer] Assignee
	Comment 47 • 9 years ago

(In reply to Dan Mosedale (:dmose) - use needinfo flag for response from comment #46)
> Does this patch continue to work in the non-e10s case?  What I'm wondering
> is, suppose we land this patch, subsequent client code depends on the new
> APIs, and then e10s sits turned off on beta for multiple release cycles...
> does Hello still work?

Yes.

	Mike de Boer [:mikedeboer] Assignee
	Comment 48 • 9 years ago

Unbitrotted version.

	Mark Banner (:standard8)
	Comment 49 • 9 years ago

Comment on attachment 8677518 [details] [diff] [review]
Patch 1.5: add a publish/ subscribe mechanism to sweeten chrome to content message passing

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

pubsub is looking a lot better. I think there's a few improvements to the tests we could make though.

::: browser/components/loop/content/shared/js/pubsub.js
@@ +161,5 @@
> +   * @param  {Function} callback Handler function
> +   */
> +  loop.unsubscribe = function unsubscribe(name, callback) {
> +    if (!gSubscriptionsMap[name]) {
> +      return;

I know its a small case, but please add a test to check that we don't throw assertions if this occurs.

::: browser/components/loop/test/shared/index.html
@@ +45,5 @@
>      mocha.setup({ui: 'bdd', timeout: 10000});
>    </script>
>  
>    <!-- App scripts -->
> +  <script src="../../content/shared/js/pubsub.js"></script>

This new file also needs adding to karma.coverage.shared_standalone.js otherwise the karma run fails.

::: browser/components/loop/test/shared/pubsub_test.js
@@ +15,5 @@
> +    fakeWindow = {
> +      addMessageListener: sinon.stub(),
> +      removeMessageListener: sinon.stub(),
> +      sendAsyncMessage: sinon.stub(),
> +      setTimeout: sinon.stub()

I'd prefer us to use `clock = sandbox.useFakeTimers()`. Whilst the stub works, it clearer in the test below to say clock.tick(delay) rather than using the callback (although admittedly, then we have to return the promise separately, but I think this is still a bit clearer overall).

@@ +27,5 @@
> +  });
> +
> +  describe("loop.request", function() {
> +    it("should send a message", function() {
> +      var res = loop.request("GetLoopPref", "enabled");

I think we should really be returning the promise from the `it` in all appropriate cases in this file, not just the ones where we're checking the callback.

Otherwise I think we run the risk of the initial promise not having been called/completed before the tests run, but also a risk that completes after the test has completed and interferes with other tests.

@@ +53,5 @@
> +      var promise = loop.request("GetLoopPref", "enabled").then(function(result) {
> +        expect(result).to.eql("result");
> +      });
> +
> +      loop.request.inspect()[loop._lastMessageID]("result");

This isn't testing what the it() says. We should be using the fakeWindow.addMessageListener stub and calling its listener argument. Otherwise we're not actually testing gListeningForMessages (which is shown in code coverage).

Please also make sure we test all the branches of gListeningForMessages.

@@ +115,5 @@
> +      ).then(function(result) {
> +        expect(result).to.eql(["result1", "result2"]);
> +      });
> +
> +      loop.request.inspect()[loop._lastMessageID]({

As above, this is just testing resolving the promise, not getting the response back from the window listener.

@@ +205,5 @@
> +      var handler = function() {};
> +      loop.subscribe("LoopStatusChanged", handler);
> +
> +      loop.unsubscribe("LoopStatusChanged", function() {});
> +      expect(loop.subscribe.inspect().LoopStatusChanged.length).to.eql(1);

This would be clearer split up into separate it statements. It took me a while to figure out that this was still 1 due to the listener being different - separate if statements would make that much quick to read.

	Mark Banner (:standard8)
	Comment 50 • 9 years ago

Comment on attachment 8679961 [details] [diff] [review]
Patch 2.4: transition from the navigator.mozLoop API to the RemotePageManager API

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

I've not yet gone reviewed MozLoopAPI.jsm yet, but there's a few comments for you to start looking at. I'll try and do that file in the morning.

It would also be useful to get the tests for next time around - I tend to review those side by side, so it'd help speed things if they were here. I'm also concerned there may be testability issues, so I'd like to see how it looks.

Also, there's some eslint issues raised with the patch so they'll need fixing as well.

::: browser/base/content/browser-loop.js
@@ +413,2 @@
>       */
> +    startBrowserSharing: function() {

jsdoc for this function needs updating - its not really adding/calling back a listener now.

@@ +429,2 @@
>       */
> +    stopBrowserSharing: function() {

jsdooc update needed

::: browser/components/loop/content/js/conversationAppStore.js
@@ +28,5 @@
>  
>      this._activeRoomStore = options.activeRoomStore;
>      this._dispatcher = options.dispatcher;
> +    this._feedbackPeriod = options.feedbackPeriod;
> +    this._feedbackTimestamp = options.feedbackTimestamp;

These don't appear to be being passed in. If you stick with passing them in, then I think we should add throws for if they aren't present.

::: browser/components/loop/content/js/roomStore.js
@@ +98,2 @@
>        this._notifications = options.notifications;
> +      this._constants = options.constants;

We need these, so we should check for them in the options.

Also the jsdoc needs updating to add info about the constants object.

@@ +123,5 @@
>      startListeningToRoomEvents: function() {
>        // Rooms event registration
> +      loop.request("Rooms:PushSubscription", ["add", "close", "delete", "open",
> +        "refresh", "update"]);
> +      loop.subscribe("Rooms:Add", this._onRoomAdded.bind(this));

This two-step process feels prone to error on a developer's part. Couldn't loop.subscribe automatically request the subscription?

If not, then I think we need to have much clearer comments that this may be required in the loop.subscribe jsdoc, and also we need to have some comments around here saying to add to both lists as otherwise it won't come through.

@@ +145,4 @@
>       * @param {Object} addedRoomData The added room data.
>       */
> +    _onRoomAdded: function(addedRoomData) {
> +      addedRoomData = addedRoomData[0];

Ok, now I've seen this, I don't think I like how loop.subscribe() is working. In loop.subscribe, rather than doing

cb(message.data[1])

why not do:

cb.apply(null, message.data[1])

?

This would simplify the callbacks so that we don't have to do argument translation on them all.

If not, then we need to extend the loop.subscribe docs & fix all the jsdocs of the functions where this is changing.

::: browser/components/loop/content/libs/l10n.js
@@ +13,5 @@
>    var gLanguage = '';
> +  // These are the available plural functions that give the appropriate index
> +  // based on the plural rule number specified. The first element is the number
> +  // of plural forms and the second is the function to figure out the index.
> +  // NOTE: these rule functions are - unfortunately - a copy from the `gFunctions`

Ok, I don't like this, but I can see why you're doing it. Given that L10n-gaia-*.js does something similar, we might want to file a follow-up bug.

Can you add an XXX reference here and file a bug to think about how we can address this please (and reference the bug in the code).

::: browser/components/loop/content/shared/js/views.jsx
@@ +295,5 @@
>       */
>      handleHelpEntry: function(event) {
>        event.preventDefault();
> +      loop.request("GetLoopPref", "support_url").then(function(helloSupportUrl) {
> +        loop.request("OpenURL", helloSupportUrl);

I'm half wondering for instances like this, if we shouldn't just define separate "OpenSupportPages" requests that just get the pref and open the url in one go.

This would save a round trip to the chrome process.

::: browser/components/loop/modules/MozLoopAPI.jsm
@@ +855,2 @@
>  
> +this.LoopAPI = Object.freeze({

Given we freeze the object (which I'm still not sure I like because of the way the globals work), is there really a need to go with separate external & internal interfaces, or can we just drop the redirection?

::: browser/components/loop/modules/MozLoopService.jsm
@@ +955,5 @@
>              chatbox = ref && ref.get() || chatbox;
>            } else if (eventName == "Loop:ChatWindowClosed") {
>              windowCloseCallback();
> +            if (conversationWindowData.type == "room") {
> +              LoopRooms.leave(conversationWindowData.roomToken);

This shouldn't be necessary because the unload handler should be handling it. If unload is no longer working the same, then we need to examine why as that's going to potentially give us issues with our logging and also with the opentok sdk.

::: intl/locale/PluralForm.jsm
@@ +96,2 @@
>      // Make the plural form get function and set it as the default get
> +    [PluralForm.get, PluralForm.numForms] = PluralForm.makeGetter(PluralForm.ruleNum);

I don't know why this change is here. If its an optimisation, I think it should belong to a different patch/bug as it doesn't seem related to this patch.

It'll should probably have review from an appropriate peer if we do need this change.

	Mike de Boer [:mikedeboer] Assignee
	Comment 52 • 9 years ago

Hmm, I haven't changed loop.subscribe with your request from comment 50. Please hold your review :)

	Mike de Boer [:mikedeboer] Assignee
	Comment 54 • 9 years ago

(In reply to Mark Banner (:standard8) from comment #50)
> These don't appear to be being passed in. If you stick with passing them in,
> then I think we should add throws for if they aren't present.

Whoops! Thanks for catching that!

> We need these, so we should check for them in the options.

The conversation window apparently doesn't need the `notifications` one, so I'll be omitting the check for that.

> Ok, I don't like this, but I can see why you're doing it. Given that
> L10n-gaia-*.js does something similar, we might want to file a follow-up bug.
> 
> Can you add an XXX reference here and file a bug to think about how we can
> address this please (and reference the bug in the code).

Sure, I'll create the bug before landing.

> I'm half wondering for instances like this, if we shouldn't just define
> separate "OpenSupportPages" requests that just get the pref and open the url
> in one go.
> 
> This would save a round trip to the chrome process.

Well sure, even though that optimization is not really needed right now. Would be good for follow-up fodder.

> Given we freeze the object (which I'm still not sure I like because of the
> way the globals work), is there really a need to go with separate external &
> internal interfaces, or can we just drop the redirection?

We might be able to. If sec-review comes and dislikes this approach, we can always put it back in.

> This shouldn't be necessary because the unload handler should be handling
> it. If unload is no longer working the same, then we need to examine why as
> that's going to potentially give us issues with our logging and also with
> the opentok sdk.

Well, it is necessary, because the unload handler doesn't give us the time to send a message across using messageManager - in fact, that interface is already destroyed once the unload handler is invoked.
So to make this work - and it works even more reliably than it does right now in the detached window state - we need to catch the 'window is closing' event on the chrome side and do the appropriate signalling.

> I don't know why this change is here. If its an optimisation, I think it
> should belong to a different patch/bug as it doesn't seem related to this
> patch.
> 
> It'll should probably have review from an appropriate peer if we do need
> this change.

I'm not really looking forward to that kind of overhead... I'm merely adding `ruleNum` to the exported object so that I can pass that along to the in-content l10n library.

	Mike de Boer [:mikedeboer] Assignee
	Comment 60 • 9 years ago

Mark, what do you think is the best way forward with browser_mozLoop_sharingListeners.js? Rewrite? (we have some intermittent failure history with this one, hence me asking).

	Mark Banner (:standard8)
	Comment 61 • 9 years ago

Comment on attachment 8682432 [details] [diff] [review]
Patch 3: make 'ruleNum' an object member to allow the property to be passed to content pages

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

I don't know why I thought this was just an optimisation before, but having the patch description for this as a separate thing makes it clear now!

	Mark Banner (:standard8)
	Comment 62 • 9 years ago

Comment on attachment 8682434 [details] [diff] [review]
Patch 5: ui-showcase adjustments to remove its dependency on navigator.mozLoop

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

I'm not sure I really like the getAllRooms override in the showcase, as it feels fragile. However, I can't really think of a better way of handling it at the moment, without some investigation and further architecture, so I think we'll leave it as it is.


The only reason for the r- is that we're now getting audio played whenever the page is reloaded. I think we should see if we can stub that out - it could get very annoying for developers very quickly.

	Mark Banner (:standard8)
	Comment 63 • 9 years ago

(In reply to Mike de Boer [:mikedeboer] from comment #60)
> Mark, what do you think is the best way forward with
> browser_mozLoop_sharingListeners.js? Rewrite? (we have some intermittent
> failure history with this one, hence me asking).

I think I'm easy either way here. A rewrite could help the intermittents, though it doesn't look like there's that many changes needed to get it working with the new code.

Do we know which areas the intermittents were in? Might just be worth rewriting those specific areas if it is specific enough.

	Mike de Boer [:mikedeboer] Assignee
	Comment 64 • 9 years ago

Unbitrot.

	Mike de Boer [:mikedeboer] Assignee
	Comment 65 • 9 years ago

Unbitrot.

	Mark Banner (:standard8)
	Comment 67 • 9 years ago

Comment on attachment 8682433 [details] [diff] [review]
Patch 4: update mocha tests and karma runs to not rely on mozLoop anymore

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

::: browser/components/loop/test/desktop-local/conversation_test.js
@@ +112,5 @@
>      it("should initialize L10n", function() {
>        loop.conversation.init();
>  
>        sinon.assert.calledOnce(document.mozL10n.initialize);
> +      expect(document.mozL10n.initialize.args[0][0].locale).to.eql("en-US");

I think you can make this:

sinon.assert.calledWith(document.mozL10n.initialize, sinon.match({ locale: "en-US" }));

That seems slightly cleaner to me.

Please update other instances where this is used as well (e.g. panel_test.js).

::: browser/components/loop/test/desktop-local/panel_test.js
@@ +421,3 @@
>          });
>  
> +        // it("should close dropdown menu", function() {

This test needs uncommenting & fixing.

::: browser/components/loop/test/desktop-local/roomStore_test.js
@@ +365,5 @@
>              }));
>          });
>      });
>  
> +    // describe("#createRoomError", function() {

This needs uncommenting. At a glance I can't see the reason for commenting it out - the code is still the same.

::: browser/components/loop/test/shared/loop_mocha_utils.js
@@ +237,5 @@
> +   *
> +   * @return {Promise}
> +   */
> +  function promiseNextTick() {
> +    return new Promise(function(resolve) { resolve(); });

This isn't getting used anywhere afaict.

::: browser/components/loop/test/standalone/index.html
@@ +10,5 @@
>  </head>
>  <body>
>    <div id="mocha">
>      <p><a href="../">Index</a></p>
> +    <p><a href="../shared/">Standalone/ Link-Clicker Tests</a></p>

That's not right, this is a link to the shared page. Given there's no other links on the other pages, lets drop this line and just have the index link.

	Mark Banner (:standard8)
	Comment 68 • 9 years ago

Comment on attachment 8684242 [details] [diff] [review]
Patch 4.1: update mocha tests and karma runs to not rely on mozLoop anymore

This is a bitrot update from patch 4, the interdiffs look fine, so please see my comments on that patch. My plan is to get you feedback on all of these today (hopefully!) and then do the r-/r+ on updated versions of the patches when I give them a thorough test together.

	Mark Banner (:standard8)
	Comment 69 • 9 years ago

Comment on attachment 8682430 [details] [diff] [review]
Patch 2.6: transition from the navigator.mozLoop API to the RemotePageManager API

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

::: browser/components/loop/content/shared/js/activeRoomStore.js
@@ +1085,5 @@
>        if (loop.standaloneMedia) {
>          loop.standaloneMedia.multiplexGum.reset();
>        }
>  
> +      var calls = [

nit: I think "requests" is a slightly better name here.

::: browser/components/loop/content/shared/js/dispatcher.js
@@ +18,5 @@
>    function Dispatcher() {
>      this._eventData = {};
>      this._actionQueue = [];
> +    this._debug = false;
> +    loop.shared.utils.getBoolPreference("debug.dispatcher").then(function(enabled) {

The fact this returns a promise is going to break IE 10/11 users from seeing the "browser not supported" page.

We can use promises in the rest of the flow, as all our "supported" browsers have promises, but we'll need to do something different here for the standalone I think.

::: browser/components/loop/content/shared/js/mixins.js
@@ +350,3 @@
>      },
>  
> +    _canPlay: function(callback) {

Callback is unused.

nit: Please add jsdoc.

::: browser/components/loop/content/shared/js/utils.js
@@ +119,5 @@
>      var options = { year: "numeric", month: "long", day: "numeric" };
>      return date.toLocaleDateString(navigator.language, options);
>    }
>  
> +  function isDesktop() {

nit: please add jsdoc.

@@ +121,5 @@
>    }
>  
> +  function isDesktop() {
> +    return rootObject.document.body.className.indexOf("standalone") === -1
> +      && ("sendAsyncMessage" in rootObject);

The "sendAsyncMessage" check here feels a bit bogus since standaloneMozLoop defines it as well...

::: browser/components/loop/modules/LoopRooms.jsm
@@ +795,5 @@
>     */
>    refreshMembership: function(roomToken, sessionToken, callback) {
> +    let room = this.rooms.get(roomToken);
> +    if (room) {
> +      room.sessionToken = sessionToken;

If we're making these optional, then we could just not pass them at all and use the data from the local copy of the room.

@@ +858,5 @@
>        };
>      }
> +    let room = this.rooms.get(roomToken);
> +    if (room) {
> +      room.sessionToken = sessionToken;

This shouldn't be set here - it isn't 100% guaranteed to be called before leave. It should be obtained during the _postToRoom that's called from the join.

That will ensure the most accurate version. Please add a note to the join()'s jsdoc that its storing the sessionToken for future use.

::: browser/components/loop/modules/MozLoopAPI.jsm
@@ +76,3 @@
>  
> +    // Mark the object as an Error, otherwise it won't be discernable from other,
> +  // regular objects.

nit: indentation

@@ +164,5 @@
> +    win.LoopUI.startBrowserSharing();
> +
> +    gBrowserSharingWindows.add(Cu.getWeakReference(win));
> +    ++gBrowserSharingListenerCount;
> +  },

This looks like it should have reply() called at the end of the function - at least to be consistent with the other functions in this file.

@@ +165,5 @@
> +
> +    gBrowserSharingWindows.add(Cu.getWeakReference(win));
> +    ++gBrowserSharingListenerCount;
> +  },
> +  /**

nit: Prefer a blank line after each function in here, like we have done with the prevailing style in LoopRooms, MozLoopService etc

@@ +172,5 @@
> +   * @param  {string}  windowId  The window id.
> +   * @param  {string}  sessionId OT session id.
> +   * @param  {string}  callId    The callId on the server.
> +   */
> +  AddConversationContext: function(message, reply) {

nit: all the jsdocs in this file need updating to reflect the "message" parameter that now has the appropriate attributes that were the original parameters.

@@ +267,5 @@
> +      gStringBundle[key] = value;
> +    }
> +    reply(gStringBundle);
> +  },
> +  GetAllConstants: function(message, reply) {

Please add jsdoc (bonus points for also adding jsdoc to the functions you've not created in this patch).

@@ +687,4 @@
>  };
>  
> +const LoopAPIInternal = {
> +  initialize: function() {

nit: please add jsdoc for the fuctions in LoopAPIInternal

@@ +855,2 @@
>  
> +this.LoopAPI = Object.freeze({

nit: please add jsdoc for these.

::: browser/components/loop/modules/MozLoopService.jsm
@@ +955,5 @@
>              chatbox = ref && ref.get() || chatbox;
>            } else if (eventName == "Loop:ChatWindowClosed") {
>              windowCloseCallback();
> +            if (conversationWindowData.type == "room") {
> +              LoopRooms.leave(conversationWindowData.roomToken);

Ok, I can see the reason for doing this. I think we also need to:

- Turn off screen share state for the windowId (as the current activeRoomStore#_leaveRoom does
- Add comments here, and to activeRoomStore#windowUnload and activeRoomStore#_leaveRoom to note that if we do something in close/unload, then we should make sure it is covered appropriately in both places.

::: browser/components/loop/standalone/content/js/standaloneMozLoop.js
@@ +17,5 @@
> +  var PUSH_SUBSCRIPTION = "pushSubscription";
> +  var BATCH_MESSAGE = "Batch";
> +  var MAX_LOOP_COUNT = 10;
> +
> +  var gBaseServerURL = null;

Since gBaseServerURL isn't used outside of StandaloneLoopRooms, I think it should be kept as a member of that, and set appropriately.

@@ +271,5 @@
>  
> +    /**
> +     * Stores a preference in the local storage for standalone.
> +     * Note: Some prefs are filtered out as they are not applicable
> +     * to the standalone UI.

Can you remove the note here, its not your fault, but we're clearly not doing filtering atm.

	Mark Banner (:standard8)
	Comment 70 • 9 years ago

Comment on attachment 8684244 [details] [diff] [review]
Patch 5.1: ui-showcase adjustments to remove its dependency on navigator.mozLoop

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

From the interdiffs this looks better, I'll test it fully once the other patches are updated.

	Mark Banner (:standard8)
	Comment 71 • 9 years ago

Comment on attachment 8682519 [details] [diff] [review]
Patch 6: make sure our mochi tests don't depend on mozLoopAPI

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

See comment 63 for my response to the test question. The changes here look good. How about a try push with the next round of updates to look for intermittents?

::: browser/components/loop/test/mochitest/browser.ini
@@ -24,1 @@
>  skip-if = buildapp == 'mulet'

The mulet line needs removing here as well.

	Mark Banner (:standard8)
	Comment 72 • 9 years ago

Dan just pointed out that we might also rename pubsub.js to something else as we're now using request/subscribe.

	Mike de Boer [:mikedeboer] Assignee
	Comment 73 • 9 years ago

(In reply to Mark Banner (:standard8) from comment #72)
> Dan just pointed out that we might also rename pubsub.js to something else
> as we're now using request/subscribe.

Thanks for all the comments! I'm thinking about 'loopapi-client.js', what do you think of that?

	Mark Banner (:standard8)
	Comment 74 • 9 years ago

(In reply to Mike de Boer [:mikedeboer] from comment #73)
> (In reply to Mark Banner (:standard8) from comment #72)
> > Dan just pointed out that we might also rename pubsub.js to something else
> > as we're now using request/subscribe.
> 
> Thanks for all the comments! I'm thinking about 'loopapi-client.js', what do
> you think of that?

Yeah, I'm not convinced by the "-client", but we've used that idea in the past, and its reasonable, so lets go for it.

	Dan Mosedale (:dmosedale, :dmose)
	Comment 75 • 9 years ago

loop-addon-api.js maybe?

	Mike de Boer [:mikedeboer] Assignee
	Comment 76 • 9 years ago

File rename. Carrying over r=Standard8.

	Mike de Boer [:mikedeboer] Assignee
	Comment 78 • 9 years ago

Forgot the requested LoopRooms changes.

	Mark Banner (:standard8)
	Comment 81 • 9 years ago

Comment on attachment 8687147 [details] [diff] [review]
Patch 2.9: transition from the navigator.mozLoop API to the RemotePageManager API

Much nicer. r=Standard8

	Mike de Boer [:mikedeboer] Assignee
	Comment 83 • 9 years ago

Carrying over r=Standard8

	Mike de Boer [:mikedeboer] Assignee
	Comment 85 • 9 years ago

Retry.

	Mike de Boer [:mikedeboer] Assignee
	Comment 86 • 9 years ago

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

	Mark Banner (:standard8)
	Comment 87 • 9 years ago

Comment on attachment 8687185 [details] [diff] [review]
Patch 5.2: ui-showcase adjustments to remove its dependency on navigator.mozLoop

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

Looks good. r=Standard8

	Mark Banner (:standard8)
	Comment 88 • 9 years ago

Comment on attachment 8687184 [details] [diff] [review]
Patch 4.2: update mocha tests and karma runs to not rely on mozLoop anymore

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

Looks good, r=Standard8

Note: need to fix the socialProviders -> gSocialProviders typo in MozLoopAPI in one of these patches.

	Mark Banner (:standard8)
	Comment 89 • 9 years ago

Comment on attachment 8688456 [details] [diff] [review]
Patch 6.2: make sure our mochi tests don't depend on mozLoopAPI

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

This looks good, however we still have the network issue to sort out in browser_mozLoop_socialShare:

FATAL ERROR: Non-local network connections are disabled and a connection attempt to activations.cdn.mozilla.net (68.232.34.191) was made.
You should only access hostnames available via the test networking proxy (if running mochitests) or from a test-specific httpd.js server (if running xpcshell tests). Browser services should be disabled or redirected to a local server.

I'm fairly sure that didn't happen for me when I was running a build alongside tests. If so, then this could be a timing issue exposed with the switch to doing things in an async fashion.

::: browser/components/uitour/test/browser.ini
@@ +41,5 @@
>  skip-if = e10s # Bug 1073247 - UITour.jsm not e10s friendly.
>  [browser_UITour_heartbeat.js]
>  skip-if = e10s # Bug 1073247 - UITour.jsm not e10s friendly.
> +#[browser_UITour_loop.js]
> +#skip-if = os == "linux" || e10s # Bug 1073247 - UITour.jsm not e10s friendly.

Please make this:

[browser_UITour_loop.js]
skip-if = true # Bug <number>

That way it still shows up as skipped, and there's a reference there. If we're going to assume bug 1073247 will fix this, then I think it might be worth a comment in that bug just noting that this one has disabled that bit fully.

	Mike de Boer [:mikedeboer] Assignee
	Comment 90 • 9 years ago

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

	Mike de Boer [:mikedeboer] Assignee
	Comment 91 • 9 years ago

https://hg.mozilla.org/integration/fx-team/rev/5688b4a4fa1b31aa54fd471efff4c13120a6ba70
Bug 1048850 - Part 1: add a client part that can be used to request data from the Loop API or subscribe to incoming push messages. r=Standard8

https://hg.mozilla.org/integration/fx-team/rev/0e07e1f79c76ebc643a28a1a78ca7e5298cc27e9
Bug 1048850 - Part 2: make 'ruleNum' an object member to allow the property to be passed to content pages. r=Standard8

https://hg.mozilla.org/integration/fx-team/rev/35e3dcc328d426b1f28309c907ff4fc10c9a638c
Bug 1048850 - Part 3: transition from the navigator.mozLoop API to the RemotePageManager API. r=Standard8

https://hg.mozilla.org/integration/fx-team/rev/af1b0a6879cc5d76cfd7e8bf2a0d4a5cf9fff84e
Bug 1048850 - Part 4: update mocha tests and karma runs to not rely on mozLoop anymore. r=Standard8

https://hg.mozilla.org/integration/fx-team/rev/b4c818050f8f773d61352cc23470074e81c7e73f
Bug 1048850 - Part 5: ui-showcase adjustments to remove its dependency on navigator.mozLoop. r=Standard8

https://hg.mozilla.org/integration/fx-team/rev/43075970a44ea086084135bb0a100245ecb3bde7
Bug 1048850 - Part 6: make sure our mochi tests don't depend on mozLoopAPI anymore and move relevant ones to xpcshell. r=Standard8

	Mike de Boer [:mikedeboer] Assignee
	Comment 92 • 9 years ago

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

	Mike de Boer [:mikedeboer] Assignee
	Comment 93 • 9 years ago

(In reply to Mike de Boer [:mikedeboer] from comment #92)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=aa762e1c9f35

Oops, wrong bug ;)

	Carsten Book [:Tomcat]
	Comment 94 • 9 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/5688b4a4fa1b
https://hg.mozilla.org/mozilla-central/rev/0e07e1f79c76
https://hg.mozilla.org/mozilla-central/rev/35e3dcc328d4
https://hg.mozilla.org/mozilla-central/rev/af1b0a6879cc
https://hg.mozilla.org/mozilla-central/rev/b4c818050f8f
https://hg.mozilla.org/mozilla-central/rev/43075970a44e