Closed
Bug 1099128
Opened 10 years ago
Closed 9 years ago
Swap Loop to use rooms rather than call urls
Categories
(Hello (Loop) :: Client, defect, P1)
Hello (Loop)
Client
Tracking
(firefox35 verified, firefox36 verified)
backlog | Fx35+ |
People
(Reporter: standard8, Assigned: standard8)
References
Details
Attachments
(2 files, 2 obsolete files)
1.58 KB,
patch
|
standard8
:
review+
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
8.11 KB,
patch
|
NiKo
:
review+
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•10 years ago
|
||
Attachment #8522976 -
Flags: review?(mdeboer)
Assignee | ||
Comment 2•10 years ago
|
||
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 #8522979 -
Flags: review?(nperriault) → review+
Updated•10 years ago
|
Attachment #8522976 -
Flags: review?(mdeboer) → review+
Assignee | ||
Comment 3•10 years ago
|
||
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
Updated•10 years ago
|
Priority: -- → P1
Assignee | ||
Comment 4•10 years ago
|
||
Ready for landing.
Attachment #8522976 -
Attachment is obsolete: true
Attachment #8525129 -
Flags: review+
Assignee | ||
Comment 5•10 years ago
|
||
Landed directly on central with sheriff approval: https://hg.mozilla.org/mozilla-central/rev/d0d8c407efb5
Status: NEW → RESOLVED
Points: --- → 1
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Assignee | ||
Comment 6•10 years ago
|
||
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 → ---
Assignee | ||
Comment 7•9 years ago
|
||
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+
Assignee | ||
Comment 9•9 years ago
|
||
Follow-up landed so we can get it in for testing: https://hg.mozilla.org/integration/fx-team/rev/2baabecada38
Comment 10•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/2baabecada38
Status: REOPENED → RESOLVED
Closed: 10 years ago → 9 years ago
Resolution: --- → FIXED
Comment 11•9 years ago
|
||
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 12•9 years ago
|
||
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?
Updated•9 years ago
|
Attachment #8525129 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•9 years ago
|
Attachment #8527650 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 13•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/323dcdb9ff99 https://hg.mozilla.org/releases/mozilla-aurora/rev/b2c3adb502a6 https://hg.mozilla.org/releases/mozilla-aurora/rev/64f199adc34d
Updated•9 years ago
|
status-firefox35:
--- → fixed
status-firefox36:
--- → fixed
Comment 14•9 years ago
|
||
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
Assignee | ||
Updated•9 years ago
|
Attachment #8527650 -
Flags: feedback?(MattN+bmo)
You need to log in
before you can comment on or make changes to this bug.
Description
•