Register with the server for push notification updates to rooms

RESOLVED FIXED in Firefox 35

Status

enhancement
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: standard8, Assigned: pkerr)

Tracking

unspecified
mozilla36
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox35 fixed, firefox36 fixed)

Details

(Whiteboard: [rooms])

User Story

* Extend the push notification service as per the specification:

https://wiki.mozilla.org/Loop/Architecture/Rooms#Room_Owner_Notification

* Update push handler to register multiple ids for rooms and calls, both FxA and Guest
* Update registration handling to handle multiple channels
* Update notification callbacks to handle multiple channels

* TBD: Split existing Guest/FxA Calls push notification into two separate channels (is the server changing the ids?)

Attachments

(1 attachment, 11 obsolete attachments)

93.29 KB, patch
jesup
: review+
Details | Diff | Splinter Review
Reporter

Description

5 years ago
No description provided.
Reporter

Updated

5 years ago
User Story: (updated)
Reporter

Updated

5 years ago
Blocks: 1074665
Reporter

Updated

5 years ago
Severity: normal → enhancement
Assignee: nobody → pkerr
Assignee

Updated

5 years ago
Status: NEW → ASSIGNED
Assignee

Comment 1

5 years ago
Do you expect that the current usage of one channelID for the "calls" based Loop client to be replaced with using a separate channelID for FxA and Guest in this patch update? Or, is the change to be made to add the new "rooms" notification only?
Flags: needinfo?(adam)

Comment 2

5 years ago
(In reply to Paul Kerr [:pkerr] from comment #1)
> Do you expect that the current usage of one channelID for the "calls" based
> Loop client to be replaced with using a separate channelID for FxA and Guest
> in this patch update? Or, is the change to be made to add the new "rooms"
> notification only?

tl;dr: Both.

We're not going to turn off the unaccounted direct calling immediately when rooms rolls out; we're just going to make it impossible to make new URLs. As the old ones age out, the feature will gradually remove itself from the system. This implies that the direct call handling logic for a logged in user will still have to check "GET /calls" for both the logged-in user and the browser instance. I'm concerned about the load impact on the Loop server of doing so.
Flags: needinfo?(adam)
Assignee

Comment 3

5 years ago
Actually, the pair of channelIDs I was asking about are the FXA_CALLS and GUEST_CALLS. The current code only uses one channelID for operation and checks both FxA and Guest accounts when a notification is received. (The channelID is the same as the one listed as GUEST_CALLS.) Is it your intention that this bug also change the "calls" behavior to use separate channelIDs for FXA and GUEST? (Besides the new "rooms" functions.)
Flags: needinfo?(adam)

Comment 4

5 years ago
(In reply to Paul Kerr [:pkerr] from comment #3)
> Actually, the pair of channelIDs I was asking about are the FXA_CALLS and
> GUEST_CALLS. The current code only uses one channelID for operation and
> checks both FxA and Guest accounts when a notification is received. (The
> channelID is the same as the one listed as GUEST_CALLS.) Is it your
> intention that this bug also change the "calls" behavior to use separate
> channelIDs for FXA and GUEST? (Besides the new "rooms" functions.)

Yes. That's what I was trying to say above. I don't want to have to hit the Loop server twice for every direct call.
Flags: needinfo?(adam)
Assignee

Comment 5

5 years ago
Posted patch WIP (obsolete) — Splinter Review
MozLoopPushHanler will handles multiple notification channels and multiple calls to register channels. Integrated with LoopRooms to register for rooms notification channels.
Assignee

Comment 6

5 years ago
Posted patch WIP (obsolete) — Splinter Review
MozLoopPushHandler will handles multiple notification channels and multiple calls to register channels. Integrated with LoopRooms to register for rooms notification channels.
Assignee

Updated

5 years ago
Attachment #8503424 - Attachment is obsolete: true
Assignee

Comment 7

5 years ago
Comment on attachment 8503437 [details] [diff] [review]
WIP

Refer to bug 1074664 for the registration performed by the LoopRooms code. I will need to integrate the rooms registration with the delay timer that registers the MozLoopService. Since LoopRooms needs to call MozLoopService, it needs to be outside of MozLoopService.
Attachment #8503437 - Flags: feedback?(standard8)
Assignee

Updated

5 years ago
Attachment #8503437 - Flags: feedback?(standard8)

Updated

5 years ago
backlog: --- → Fx35+
Assignee

Comment 8

5 years ago
Posted patch WIP (obsolete) — Splinter Review
Functional code. But, some tests are broken due to new registrations
Assignee

Updated

5 years ago
Attachment #8503437 - Attachment is obsolete: true
Assignee

Updated

5 years ago
Attachment #8504994 - Attachment is obsolete: true
Assignee

Updated

5 years ago
Attachment #8504996 - Flags: review?(standard8)
Assignee

Comment 10

5 years ago
Part 2 patch will fix the remainder of the broken xpcshell tests. I want to get feedback on the refactored code that handles multiple registrations and breaks out the calls handling code in a new module called LoopCalls.jsm (parallel to the new LoopRooms.jsm).
Assignee

Comment 11

5 years ago
removed a missed DEBUG temp change.
Assignee

Updated

5 years ago
Attachment #8504996 - Attachment is obsolete: true
Attachment #8504996 - Flags: review?(standard8)
Assignee

Updated

5 years ago
Attachment #8505024 - Flags: review?(standard8)
Reporter

Comment 12

5 years ago
Comment on attachment 8505024 [details] [diff] [review]
Part 1: Register rooms notification channels. Register new calls notification channels

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

The general idea behind this looks reasonable to me. Splitting out LoopCalls/LoopRooms is definitely a good idea.

I'm going to see if Matt has time to review this, as I need to look at other things today, and I know Matt was interested in improving the registration flow so may have some comments here.

::: browser/components/loop/LoopCalls.jsm
@@ +191,5 @@
> + * The registration is a two-part process. First we need to connect to
> + * and register with the push server. Then we need to take the result of that
> + * and register with the Loop server.
> + */
> +let LoopCallsInternal = {

Based on previous discussions with testing, I think we should probably not have a split of LoopCallInternal vs LoopCalls here, and just go with a single version, and clear indication (e.g. _) of private methods.
Attachment #8505024 - Flags: review?(standard8)
Attachment #8505024 - Flags: review?(MattN+bmo)
Attachment #8505024 - Flags: feedback+
Comment on attachment 8505024 [details] [diff] [review]
Part 1: Register rooms notification channels. Register new calls notification channels

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

I haven't had time to look at the registration changes yet.

::: browser/components/loop/LoopCalls.jsm
@@ +21,5 @@
> +XPCOMUtils.defineLazyModuleGetter(this, "LOOP_SESSION_TYPE",
> +                                  "resource:///modules/loop/MozLoopService.jsm");
> +
> +// Create a new instance of the ConsoleAPI so we can control the maxLogLevel with a pref.
> +XPCOMUtils.defineLazyGetter(this, "log", () => {

You can just access the logger from MozLoopService like MozLoopAPI does so you don't need to copy this code.

::: browser/components/loop/MozLoopService.jsm
@@ +24,1 @@
>  const PREF_LOG_LEVEL = "loop.debug.loglevel";

Want to |hg rename| this file to LoopService.jsm for consistency? :) I personally wouldn't mind.

@@ +311,4 @@
>      if (gRegisteredDeferred) {
>        return gRegisteredDeferred.promise;
>      }
> + 

Nit: whitespace
(In reply to Mark Banner (:standard8) from comment #12)
> ::: browser/components/loop/LoopCalls.jsm
> @@ +191,5 @@
> > + * The registration is a two-part process. First we need to connect to
> > + * and register with the push server. Then we need to take the result of that
> > + * and register with the Loop server.
> > + */
> > +let LoopCallsInternal = {
> 
> Based on previous discussions with testing, I think we should probably not
> have a split of LoopCallInternal vs LoopCalls here, and just go with a
> single version, and clear indication (e.g. _) of private methods.

I agree. A separation can also be made with a comment to keep the public method in one part of the object and private _-prefixed ones in another half.
Assignee

Updated

5 years ago
Attachment #8505024 - Attachment is obsolete: true
Attachment #8505024 - Flags: review?(MattN+bmo)
Assignee

Updated

5 years ago
Attachment #8507069 - Attachment is obsolete: true
Assignee

Updated

5 years ago
Attachment #8507599 - Flags: review?(MattN+bmo)
Assignee

Updated

5 years ago
Blocks: 1074664
Assignee

Comment 17

5 years ago
fixed some bit-rot
Assignee

Updated

5 years ago
Attachment #8507599 - Attachment is obsolete: true
Attachment #8507599 - Flags: review?(MattN+bmo)
Assignee

Updated

5 years ago
Attachment #8507599 - Flags: review?(MattN+bmo)
Assignee

Comment 18

5 years ago
I had made the MozLoopPushHandler.jsm response to having Services.io.offline set to true to be harsher than the original code: threw an exception. Reverted back to pringing a warning and returning.
Assignee

Updated

5 years ago
Attachment #8508234 - Attachment is obsolete: true
Assignee

Updated

5 years ago
Attachment #8508321 - Flags: review?(MattN+bmo)
Comment on attachment 8508321 [details] [diff] [review]
Register with PushServer for updates to rooms

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

Some initial review comments

::: browser/components/loop/LoopCalls.jsm
@@ +7,5 @@
> +const { classes: Cc, interfaces: Ci, utils: Cu, results: Cr } = Components;
> +
> +Cu.import("resource://gre/modules/Services.jsm");
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +Cu.import("resource://gre/modules/osfile.jsm", this);

What non-exported symbols are you using from osfile.jsm? i.e. why are you using the second argument?

@@ +178,5 @@
> + * The registration is a two-part process. First we need to connect to
> + * and register with the push server. Then we need to take the result of that
> + * and register with the Loop server.
> + */
> +let LoopCallsInternal = {

Please fold this into LoopCalls as mentioned earlier and prefix these members with "_"

::: browser/components/loop/MozLoopAPI.jsm
@@ +43,5 @@
>   */
>  const cloneErrorObject = function(error, targetWindow) {
>    let obj = new targetWindow.Error();
>    for (let prop of Object.getOwnPropertyNames(error)) {
> +    obj[prop] = String(error[prop]);

Note that this will convert numbers to strings too whereas the old code avoided that.

@@ -48,5 @@
> -      value = String(value);
> -    }
> -
> -    Object.defineProperty(Cu.waiveXrays(obj), prop, {
> -      configurable: false,

I'm not really sure about whether this code should be removed.

::: browser/components/loop/MozLoopPushHandler.jsm
@@ +24,5 @@
>    // This is the uri of the push server.
>    pushServerUri: undefined,
> +  // Records containing the registration and notification callbacks indexed by channelID.
> +  // Each channel will be registered with the PushServer.
> +  channels: {},

JS |Map| objects should be used instead of JS objects as the API is made for this use case and it avoids the default keys. See https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Map#Objects_and_maps_compared

@@ +73,3 @@
>      *                                 the websocket to be mocked for tests.
>      */
> +  initialize: function(options = {}) {

I think the JSDoc is out-of-date.

@@ +91,5 @@
>      this._openSocket();
> +    return true;
> +  },
> +
> +  register: function(channelID, onRegistered, onNotification) {

Could you add a JSDoc comment?

@@ +127,5 @@
>      // If a uaID has already been assigned, assume this is a re-connect
>      // and send the uaID in order to re-synch with the
>      // PushServer. If a registration has been completed, send the channelID.
> +    let helloMsg =
> +        { messageType: "hello",

Nit: keep the { on the same line as the = and align the }; with the beginning of the |let|.

::: browser/components/loop/test/xpcshell/head.js
@@ +77,5 @@
>    // This sets the registration result to be returned when initialize
>    // is called. By default, it is equivalent to success.
>    registrationResult: null,
> +  registrationPushURL: null,
> +  notificationCallback: {},

Use a map here too
Assignee

Comment 20

5 years ago
fixed mochitests
Assignee

Updated

5 years ago
Attachment #8508321 - Attachment is obsolete: true
Attachment #8508321 - Flags: review?(MattN+bmo)
Assignee

Comment 21

5 years ago
Try run: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=3f4412091172
Multi-platform run of xpcshell, marionette and mochi tests.
One orange for video control test on e10s.
Assignee

Comment 22

5 years ago
>::: browser/components/loop/MozLoopPushHandler.jsm
>@@ +24,5 @@
>>    // This is the uri of the push server.
>>    pushServerUri: undefined,
>> +  // Records containing the registration and notification callbacks indexed by channelID.
>> +  // Each channel will be registered with the PushServer.
>> +  channels: {},
>
>JS |Map| objects should be used instead of JS objects as the API is made for this use case and it avoids >the default keys. See https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Map#Objects_and_maps_compared

If I change this to Map now, I will need to make changes in many locations in the browser_fxa_login.js mochitest that uses direct access to this property on the mockPushHandler.
Assignee

Comment 23

5 years ago
I suggest that I create a bug to address the transformation of the Object to Map implementation and move forward. Several rooms bugs are blocked waiting for functional registration code to land.
Assignee

Comment 24

5 years ago
incorporate feedback
Assignee

Updated

5 years ago
Attachment #8509318 - Attachment is obsolete: true
Assignee

Updated

5 years ago
Attachment #8509673 - Flags: review?(MattN+bmo)
Comment on attachment 8509673 [details] [diff] [review]
Register with PushServer for updates to rooms

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

Here are some comments on MozLoopService:

I thought this patch was going to cleanup registration while adding more stuff to it.

The main change I think that should be made to the flow is rename promiseRegisteredWithServers to promiseRegisteredForPush and have it stop at setting gFxAOPushUrls. MozLoopService.register would then do the Loop server registration after promiseRegisteredForPush resolves.

::: browser/components/loop/MozLoopService.jsm
@@ +319,5 @@
>  
> +    this._mocks.webSocket = mockWebSocket;
> +    this._mocks.pushHandler = mockPushHandler;
> +
> +    // Wrap push notification regisration call-back in a Promise.

typo: s/regisration/registration/

@@ +320,5 @@
> +    this._mocks.webSocket = mockWebSocket;
> +    this._mocks.pushHandler = mockPushHandler;
> +
> +    // Wrap push notification regisration call-back in a Promise.
> +    let registerForNotification = (channelID, onNotification) => {

You can use traditional function syntax here to make it more clear this is a function when skimming:
function registerForNotification(channelID, onNotification) {

I wonder if it would be cleaner outside here and perhaps tests want to use it

@@ +328,5 @@
> +            reject(Error(error));
> +          } else {
> +            resolve(pushUrl);
> +          }
> +        }

Nit: missing semicolon

@@ +331,5 @@
> +          }
> +        }
> +        gPushHandler.register(channelID, onRegistered, onNotification);
> +      });
> +    }

Nit: missing semicolon

@@ +353,5 @@
> +                                              LoopCalls.onNotification);
> +
> +    let roomsRegFxA = registerForNotification(LoopRooms.channelIDs.FxA,
> +                                              LoopRooms.onNotification);
> +    Promise.all([callsRegGuest, roomsRegGuest, callsRegFxA, roomsRegFxA])

I like the simplicity of Promise.all but it does mean that one failure takes the whole ship down with it. I guess it's fine since I don't know why some would fail but not others (other than intermittent failures).

@@ +357,5 @@
> +    Promise.all([callsRegGuest, roomsRegGuest, callsRegFxA, roomsRegFxA])
> +    .then((pushUrls) => {
> +      gFxAOPushUrls = {calls: pushUrls[2], rooms: pushUrls[3]};
> +      return this.registerWithLoopServer(LOOP_SESSION_TYPE.GUEST,
> +                                         {calls: pushUrls[0], rooms: pushUrls[1]})})

Nit: Put spaces on the inside of { and } for objects (x2) like https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#JavaScript_objects

Also, you're missing a semicolon after the return value.

@@ -695,5 @@
>      let unregisterURL = "/registration?simplePushURL=" + encodeURIComponent(pushURL);
>      return this.hawkRequest(sessionType, unregisterURL, "DELETE")
>        .then(() => {
>          log.debug("Successfully unregistered from server for sessionType", sessionType);
> -        MozLoopServiceInternal.clearSessionToken(sessionType);

I'll have to look at the removal of these two clearSessionToken calls more closely.
Assignee

Comment 26

5 years ago
(In reply to Matthew N. [:MattN] (behind on reviews - focused on Loop) from comment #25)
> 
> @@ -695,5 @@
> >      let unregisterURL = "/registration?simplePushURL=" + encodeURIComponent(pushURL);
> >      return this.hawkRequest(sessionType, unregisterURL, "DELETE")
> >        .then(() => {
> >          log.debug("Successfully unregistered from server for sessionType", sessionType);
> > -        MozLoopServiceInternal.clearSessionToken(sessionType);
> 
> I'll have to look at the removal of these two clearSessionToken calls more
> closely.

unregisterFromLoopServer() is only called with a sessionType of FXA. Since the pushUrl is encoded as a query parameter, only one registration can be canceled per call. (The DELETE /registration does not use the new {simplePushURLs: {calls: ... rooms: ...}} format used by registration.) In the new multi-channel scheme, unregisterFromLoopServer needs to be called twice. In its old form, it would clear the session token on the first call and the second call would fail with a 400 Bad Request. Moving the clearSessionToken to the logoutFromFxA function wraps up the processing whether it succeeds or fails, which is the expected result in the mochitest.
Comment on attachment 8509673 [details] [diff] [review]
Register with PushServer for updates to rooms

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

::: browser/components/loop/MozLoopAPI.jsm
@@ +47,5 @@
>      let value = error[prop];
>      if (typeof value != "string" && typeof value != "number") {
>        value = String(value);
>      }
> +    

Nit: whitespace

::: browser/components/loop/MozLoopService.jsm
@@ +112,5 @@
>  let gInitializeTimer = null;
>  let gFxAEnabled = true;
>  let gFxAOAuthClientPromise = null;
>  let gFxAOAuthClient = null;
> +let gFxAOPushUrls = {};

This is now unused

@@ +1325,5 @@
> +          LOOP_SESSION_TYPE.FXA, roomsPushUrl);
> +      }
> +    } 
> +    catch (error) {throw error} 
> +    finally {

} catch (error) {
  throw error;
} finally {

::: browser/components/loop/test/mochitest/browser_fxa_login.js
@@ +121,5 @@
>        "Check FxA hawk token is not set");
>  });
>  
>  add_task(function* params_nonJSON() {
> +  Services.prefs.setCharPref("loop.server", "https://localhost:3000/invalid");

Is localhost:3000 a live server? If so, this is changing the test and I'm not sure why

@@ +256,5 @@
>  
>    // Normally the same pushUrl would be registered but we change it in the test
>    // to be able to check for success on the second registration.
> +  mockPushHandler.registeredChannels[LoopCalls.channelIDs.FxA] = "https://localhost/pushUrl/fxa";
> +  mockPushHandler.registeredChannels[LoopRooms.channelIDs.FxA] = "https://localhost/pushUrl/fxa";

Ideally these should be two different URLs so we are testing they don't get mixed up.

@@ +283,5 @@
>  
>    let registrationResponse = yield promiseOAuthGetRegistration(BASE_URL);
> +  ise(registrationResponse.response.simplePushURLs.calls, "https://localhost/pushUrl/fxa",
> +      "Check registered push URL");
> +  ise(registrationResponse.response.simplePushURLs.rooms, "https://localhost/pushUrl/fxa",

Smae here. Use different URLs

::: browser/components/loop/test/mochitest/head.js
@@ +6,5 @@
>    LOOP_SESSION_TYPE,
>    MozLoopServiceInternal,
>  } = Cu.import("resource:///modules/loop/MozLoopService.jsm", {});
> +const {LoopCalls} = Cu.import("resource:///modules/loop/LoopCalls.jsm", {});
> +const {LoopRooms} = Cu.import("resource:///modules/loop/LoopRooms.jsm", {});

Why not the simpler version?:
Cu.import("resource:///modules/loop/LoopCalls.jsm");

::: browser/components/loop/test/xpcshell/test_loopservice_registration.js
@@ +23,3 @@
>      // 404 is an expected failure indicated by the lack of route being set
>      // up on the Loop server mock. This is added in the next test.
> +    Assert.equal(err.message, "404", "Expected no errors in websocket registration");

Why the change from errno?
Attachment #8509673 - Flags: review?(MattN+bmo) → review+
Assignee

Comment 28

5 years ago
(In reply to Matthew N. [:MattN] (behind on reviews - focused on Loop) from comment #19)
> Comment on attachment 8508321 [details] [diff] [review]
> @@ +178,5 @@
> > + * The registration is a two-part process. First we need to connect to
> > + * and register with the push server. Then we need to take the result of that
> > + * and register with the Loop server.
> > + */
> > +let LoopCallsInternal = {
> 
> Please fold this into LoopCalls as mentioned earlier and prefix these
> members with "_"
> 
Per our discussion, I am going to leave this split as-is for now pending further discussion within the group. No test code currently uses direct access to LoopCallsInternal.
Assignee

Comment 29

5 years ago
>::: browser/components/loop/test/mochitest/browser_fxa_login.js
>@@ +121,5 @@
>>        "Check FxA hawk token is not set");
>>  });
>>  
>>  add_task(function* params_nonJSON() {
>> +  Services.prefs.setCharPref("loop.server", "https://localhost:3000/invalid");
>
>Is localhost:3000 a live server? If so, this is changing the test and I'm not sure whyFATAL ERROR: >Non-local network connections are disabled and a connection attempt to loop.invalid (198.105.244.228) was m
ade.

Matt: when loop.server is set to http://loop.invalid I get the following on my system when running the tests:
"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."
The hawkRequest is not going to the mock server and a fatal error is thrown. The localhost is not live and serves the purpose as a null sink but does not trigger the error.
Assignee

Comment 30

5 years ago
::: browser/components/loop/test/mochitest/head.js
>@@ +6,5 @@
>>    LOOP_SESSION_TYPE,
>>    MozLoopServiceInternal,
>>  } = Cu.import("resource:///modules/loop/MozLoopService.jsm", {});
>> +const {LoopCalls} = Cu.import("resource:///modules/loop/LoopCalls.jsm", {});
>> +const {LoopRooms} = Cu.import("resource:///modules/loop/LoopRooms.jsm", {});
>
>Why not the simpler version?:
>Cu.import("resource:///modules/loop/LoopCalls.jsm");

This form avoids triggering a test the explicitly looks for global properties.

146 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/loop/test/mochitest/browser_CardDavImporter.js | leaked window property: LOOP_SESSION_TYPE - expected PASS
147 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/loop/test/mochitest/browser_CardDavImporter.js | leaked window property: LoopCalls - expected PASS
148 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/loop/test/mochitest/browser_CardDavImporter.js | leaked window property: LoopRooms - expected PASS
Assignee

Comment 31

5 years ago
incorporate review comments and nits
Assignee

Updated

5 years ago
Attachment #8509673 - Attachment is obsolete: true
Assignee

Comment 32

5 years ago
::: browser/components/loop/test/xpcshell/test_loopservice_registration.js
@@ +23,3 @@
>      // 404 is an expected failure indicated by the lack of route being set
>      // up on the Loop server mock. This is added in the next test.
> +    Assert.equal(err.message, "404", "Expected no errors in websocket registration");

Why the change from errno?

Promise versions of new pushserver registration callbacks now pass an Error object with the message set to 404.
Assignee

Comment 33

5 years ago
Carrying forward r+ from MattN.
Assignee

Updated

5 years ago
Keywords: checkin-needed
Attachment #8510059 - Attachment description: Register with PushServer for updates to rooms → [landed on fx-team] Register with PushServer for updates to rooms
Assignee

Updated

5 years ago
Blocks: 1088230
https://hg.mozilla.org/mozilla-central/rev/5c8fc27aa8d4
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
No longer blocks: 1074665

Updated

5 years ago
Iteration: --- → 36.1
What are the user-facing implications of this change? Does this need manual QE verification? Does this have in-testsuite coverage?
Flags: qe-verify?
Flags: in-testsuite?
Comment on attachment 8510059 [details] [diff] [review]
[landed on fx-team] Register with PushServer for updates to rooms

Approval Request Comment
Landed on aurora per IRC with lsblakk with a=loop-only
Attachment #8510059 - Flags: review+
Attachment #8510059 - Flags: approval-mozilla-aurora?
Attachment #8510059 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 1102806
Reporter

Comment 39

4 years ago
This is covered by unit tests.
Flags: qe-verify?
Flags: qe-verify-
Flags: in-testsuite?
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.