Skip to content
This repository has been archived by the owner. It is now read-only.

feat(client): Stringify WebChannel payloads in Fx Desktop >= 50. #4579

Merged
merged 3 commits into from Jan 6, 2017

Conversation

@shane-tomlinson
Copy link
Member

@shane-tomlinson shane-tomlinson commented Jan 3, 2017

Fx Desktop >= 50 expects to see the detail property of WebChannel
message payloads sent as strings. This makes it so.

fixes #4577

@rfk, @mhammond - r?

@mhammond - I was unsure whether Fennec >= 50 has the same behavior, so I only send strings for Fx Desktop at the moment.

* Send a WebChannel message.
*
* @param {String} command command name
* @param {Objet} data payload

This comment has been minimized.

@vladikoff

vladikoff Jan 3, 2017
Contributor

Objet -> Object

*/
function formatEventDetail(win, eventDetail) {
const userAgent = new UserAgent(win.navigator.userAgent);
// Firefox Desktop >= 50 expects the detail to be sent as a string.

This comment has been minimized.

@vladikoff

vladikoff Jan 3, 2017
Contributor

is it only FF Desktop or does this also apply to Fennec (Android)?

This comment has been minimized.

@shane-tomlinson

shane-tomlinson Jan 3, 2017
Author Member

That's what I asked @mhammond

This comment has been minimized.

@mhammond

mhammond Jan 4, 2017
Member

IIUC, Fennec uses the same WebChannel.jsm as desktop. What I don't know is what Fennec version it landed in :( I asked on #mobile and I'll let you know what response I get.

(and @thomcc ended up answering - it is 50 on Fennec too - see bug 1287884)

*
* @param {String} webChannelId ID of the receiving WebChannel
* @param {String} command command name
* @param {Objet} data payload

This comment has been minimized.

@vladikoff

vladikoff Jan 3, 2017
Contributor

Objet - > Object

}

describe('send', function () {
describe('Fx Dekstop >= 50', () => {

This comment has been minimized.

@vladikoff

vladikoff Jan 4, 2017
Contributor

Dekstop -> Desktop

Copy link
Member

@mhammond mhammond left a comment

This looks fine to me, notwithstanding the Fennec notes below (and the more general comment of either waiting a few months, or having a followup issue to remove the UA checks in a few months)

*/
function formatEventDetail(win, eventDetail) {
const userAgent = new UserAgent(win.navigator.userAgent);
// Firefox Desktop >= 50 expects the detail to be sent as a string.

This comment has been minimized.

@mhammond

mhammond Jan 4, 2017
Member

IIUC, Fennec uses the same WebChannel.jsm as desktop. What I don't know is what Fennec version it landed in :( I asked on #mobile and I'll let you know what response I get.

(and @thomcc ended up answering - it is 50 on Fennec too - see bug 1287884)

// Firefox Desktop >= 50 expects the detail to be sent as a string.
// See https://bugzilla.mozilla.org/show_bug.cgi?id=1275616 and
// https://bugzilla.mozilla.org/show_bug.cgi?id=1238128
if (userAgent.isFirefoxDesktop() && userAgent.browser.version >= 50) {

This comment has been minimized.

@mhammond

mhammond Jan 4, 2017
Member

As we mentioned in bugzilla, another alternative would be to just wait a few months and lose the UA check completely :)

@rfk
rfk approved these changes Jan 4, 2017
Copy link
Member

@rfk rfk left a comment

This LGTM, and it sounds like we can remove the isDesktop check as well. We must remember to test it with desktop and mobile while it's in dev.

re: waiting a couple of months, I don't have high hopes that the long tail of users noted in [1] will age out with any great speed, so I'm fine with the sniffing approach in order to close this out.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1275616#c9

Shane Tomlinson added 2 commits Jan 3, 2017
Fx Desktop >= 50 expects to see the `detail` property of WebChannel
message payloads sent as strings. This makes it so.

fixes #4577
@shane-tomlinson shane-tomlinson force-pushed the issue-4577-stringify-web-channels branch 2 times, most recently from 267730b to c64a1de Jan 6, 2017
@shane-tomlinson shane-tomlinson force-pushed the issue-4577-stringify-web-channels branch from c64a1de to 2dc3f4e Jan 6, 2017
@shane-tomlinson shane-tomlinson merged commit 4888e28 into master Jan 6, 2017
4 checks passed
4 checks passed
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.002%) to 98.528%
Details
@shane-tomlinson shane-tomlinson deleted the issue-4577-stringify-web-channels branch Jan 6, 2017
divyabiyani added a commit to divyabiyani/fxa-content-server that referenced this pull request Jan 6, 2017
…illa#4579) r=@mhammond, @rfk

Fx Desktop & Fennec >= 50 expect to see the `detail` property of WebChannel
message payloads sent as strings. This makes it so.

fixes mozilla#4577
@rfk rfk added this to the FxA-0: quality milestone Jan 25, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants