Closed Bug 1099128 Opened 5 years ago Closed 5 years ago

Swap Loop to use rooms rather than call urls

Categories

(Hello (Loop) :: Client, defect, P1)

defect
Points:
1

Tracking

(firefox35 verified, firefox36 verified)

VERIFIED FIXED
mozilla36
Iteration:
36.3
Tracking Status
firefox35 --- verified
firefox36 --- verified
Blocking Flags:
backlog Fx35+

People

(Reporter: standard8, Assigned: standard8)

References

Details

Attachments

(2 files, 2 obsolete files)

There's a few bugs blocking this, but we know what they are. Getting this filed now so that we can land the patch on nightly as soon as we're ready.

Note: I'm assuming that we're going to end up being on permanently (i.e. follow trains, or uplifts) once we land in nightly, and we'll do something to explicitly disable if we really need to.
Attachment #8522976 - Flags: review?(mdeboer)
Just in case we don't get bug 1074694 and its dependent landed by the time we want to enable nightly builds, I'm filing this as a not-intending-to-land-unless-we-need-to-but-getting-review-just-in-case.
Attachment #8522979 - Flags: review?(nperriault)
Attachment #8522976 - Flags: review?(mdeboer) → review+
Comment on attachment 8522979 [details] [diff] [review]
Temporarily hide rename form input box in Loop room view until bug 1074694 is ready.

Bug 1074694 landed, so this is no longer necessary.
Attachment #8522979 - Attachment is obsolete: true
Priority: -- → P1
Landed directly on central with sheriff approval:

https://hg.mozilla.org/mozilla-central/rev/d0d8c407efb5
Status: NEW → RESOLVED
Points: --- → 1
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Somehow, this broke bc1 mochitests. I landed a temporary fix, reopening whilst I look at the real fix:

https://hg.mozilla.org/mozilla-central/rev/b4fbeba78a7d
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Depends on: 1101494
I swapped the tests back to running with rooms by default, the remaining issue is:

55 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/loop/test/mochitest/browser_fxa_login.js | A promise chain failed to handle a rejection:  - at chrome://browser/content/loop/shared/js/validate.js:77 - TypeError: invalid dependency: error; expected Error, got String
Stack trace:
    loop.validate</Validator.prototype._checkRequiredTypes/<@chrome://browser/content/loop/shared/js/validate.js:77:1
    loop.validate</Validator.prototype._checkRequiredTypes@chrome://browser/content/loop/shared/js/validate.js:73:1
    loop.validate</Validator.prototype.validate@chrome://browser/content/loop/shared/js/validate.js:61:7
    Action@chrome://browser/content/loop/shared/js/actions.js:20:25
    loop.store.RoomStore<.getAllRooms/<@chrome://browser/content/loop/shared/js/roomStore.js:312:20
    Tester_execTest@chrome://mochikit/content/browser-test.js:651:9
    Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:577:7
    SimpleTest.waitForFocus/maybeRunTests/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:683:49

I think the other issues had been fixed by other patches we've landed (better handling of cross-chrome/content parameter passing, and everything ensuring registration).

To fix this issue, I've made the roomStore action for GetAllRoomsError handle both type Object and Error for possible error types.

Additionally, I've made all the promises in MozLoopService return new errors, rather than strings. This should mean we're more likely to have a semi consistent error value of an Error or a Object (from the hawk request), rather than three possible types of Error, Object or String.

I think that's the best compromise to be had.
Attachment #8527650 - Flags: review?(nperriault)
Attachment #8527650 - Flags: feedback?(MattN+bmo)
Comment on attachment 8527650 [details] [diff] [review]
Follow-up to bug 1099128 - fix issues with error values not being correctly handled by the room store, and switch the tests back to running with rooms enabled by default

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

Looks good to me.

::: browser/components/loop/content/shared/js/actions.js
@@ +224,5 @@
>       * An error occured while trying to fetch the room list.
>       * XXX: should move to some roomActions module - refs bug 1079284
>       */
>      GetAllRoomsError: Action.define("getAllRoomsError", {
> +      error: [Error, Object]

Nit: I could see a comment added here, explaining why this is required.
Attachment #8527650 - Flags: review?(nperriault) → review+
Follow-up landed so we can get it in for testing:

https://hg.mozilla.org/integration/fx-team/rev/2baabecada38
https://hg.mozilla.org/mozilla-central/rev/2baabecada38
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Comment on attachment 8527650 [details] [diff] [review]
Follow-up to bug 1099128 - fix issues with error values not being correctly handled by the room store, and switch the tests back to running with rooms enabled by default

Approval Request Comment
[Feature/regressing bug #]: Enabling Rooms by default
[User impact if declined]:  This patch allows tests with the Room UI enabled by default to run properly
[Describe test coverage new/current, TBPL]: tbpl
[Risks and why]: The risk for this are very low and only to Hello tests (which would be caught quickly in tbpl)
[String/UUID change made/needed]: No strings
Attachment #8527650 - Flags: approval-mozilla-aurora?
Comment on attachment 8525129 [details] [diff] [review]
Swap Loop to use 2 person conversations (aka 'rooms') rather than call urls. r=mikedeboer,a=enabling-for-dogfooding

Approval Request Comment
[Feature/regressing bug #]: Enabling Rooms by default in Hello
[User impact if declined]: This patch enables "Rooms" by default.  Without this patch, Hello users would continue to use the "calls" URL for link-clicking
[Describe test coverage new/current, TBPL]: manual testing, unit test coverage in tbpl
[Risks and why]: There is a product and partner need to have Rooms in Fx35 for Hello.  This is a pref flip.  If there are problems, we can flip back;  it's a single line change (as shown in this patch).  Rooms has been on in Nightly for about a week.  
[String/UUID change made/needed]: No strings
Attachment #8525129 - Flags: approval-mozilla-aurora?
Attachment #8525129 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8527650 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: qe-verify+
Flags: in-moztrap-
Verified as fixed using the following environment:

FF 35.0b4
Build Id: 20141216120925
OS: Win 7 x64, Ubuntu 14.04 x64, Mac Os X 10.9.5
Attachment #8527650 - Flags: feedback?(MattN+bmo)
Verified fixed FF 36b1 Win 7
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.