Closed Bug 1174458 Opened 4 years ago Closed 4 years ago

Add WebChannel support to mobile/android

Categories

(Android Background Services Graveyard :: Firefox Accounts, defect)

defect
Not set

Tracking

(firefox42 fixed)

RESOLVED FIXED
Firefox 42
Tracking Status
firefox42 --- fixed

People

(Reporter: nalexander, Assigned: nalexander)

References

Details

Attachments

(7 files)

Bug 1022064 added the WebChannel abstraction to browser/.  This ticket tracks adding the same support to mobile/android/.
Testing may be tricky.  If we could enable the existing browser/ mochitests, that would be great.  mobile/android doesn't use the same JS, so I don't think the existing xpcshell tests will be helpful, but others with more knowledge can comment.

vladikoff: MattN: markh: ckarlof: see above, and I wanted to loop you into this.
Flags: needinfo?(vlad)
Flags: needinfo?(markh)
Flags: needinfo?(ckarlof)
Flags: needinfo?(MattN+bmo)
Bug 1174458 - Part 1: Copy-paste WebChannel message support from Desktop. r?MattN
Attachment #8622005 - Flags: review?(MattN+bmo)
(In reply to Nick Alexander :nalexander from comment #2)
> Created attachment 8622005 [details]
> MozReview Request: Bug 1174458 - Part 1: Copy-paste WebChannel message
> support from Desktop. r?MattN
> 
> Bug 1174458 - Part 1: Copy-paste WebChannel message support from Desktop.
> r?MattN

MattN, vladikoff, others: this seems like the little bit of integration glue I need to write a WebChannel-consuming add-on.  (It works fine locally.)  Are there particular security considerations that need verification in Fennec?

Also, I'd like to understand if this glue code could be simplified and shared between the two applications.  Perhaps we can lift more into toolkit/?  Guidance appreciated.
(In reply to Nick Alexander :nalexander from comment #1)
> Testing may be tricky.  If we could enable the existing browser/ mochitests,
> that would be great.  mobile/android doesn't use the same JS, so I don't
> think the existing xpcshell tests will be helpful, but others with more
> knowledge can comment.

Generally the xpcshell tests work cross-platform but I'm not sure which JS you're saying is different? One of the JSMs?
Flags: needinfo?(MattN+bmo)
(In reply to Matthew N. [:MattN] from comment #4)
> (In reply to Nick Alexander :nalexander from comment #1)
> > Testing may be tricky.  If we could enable the existing browser/ mochitests,
> > that would be great.  mobile/android doesn't use the same JS, so I don't
> > think the existing xpcshell tests will be helpful, but others with more
> > knowledge can comment.
> 
> Generally the xpcshell tests work cross-platform but I'm not sure which JS
> you're saying is different? One of the JSMs?

Hmm, I think I mis-interpreted.  I don't expect https://hg.mozilla.org/integration/fx-team/diff/bf5985c067af/services/fxaccounts/FxAccountsOAuthClient.jsm#l1.20 to do well in Fennec, since the FxA code doesn't work (shouldn't work!) in Fennec.  I thought the xpcshell tests were testing that code, but in fact they are testing the WebChannel.jsm broker.
Comment on attachment 8622005 [details]
MozReview Request: Bug 1174458 - Move WebChannel message support to toolkit/content. r?MattN

https://reviewboard.mozilla.org/r/11107/#review9703

(In reply to Nick Alexander :nalexander from comment #3)
> MattN, vladikoff, others: this seems like the little bit of integration glue
> I need to write a WebChannel-consuming add-on.  (It works fine locally.) 
> Are there particular security considerations that need verification in
> Fennec?

A few things to check off the top of my head:
* Make sure the permission manager works the same as desktop (I think it's shared code).
* Check that the correct principal is sent with messages
* Make sure subframes are handled properly

> Also, I'd like to understand if this glue code could be simplified and
> shared between the two applications.  Perhaps we can lift more into
> toolkit/?  Guidance appreciated.

Yes please. I think toolkit/content/browser-content.js or toolkit/content/process-content.js are good candidates for this code but I'm not sure if those are currently loaded on Android. Loading one of them on Android probably is straightforward.

Can you try to do that and see if it's straightforward?

Could you also try enabling the tests in toolkit/modules/tests/xpcshell/xpcshell.ini?
Flags: needinfo?(markh)
(In reply to Matthew N. [:MattN] from comment #6)
> Comment on attachment 8622005 [details]
> MozReview Request: Bug 1174458 - Part 1: Copy-paste WebChannel message
> support from Desktop. r?MattN
> 
> https://reviewboard.mozilla.org/r/11107/#review9703
> 
> (In reply to Nick Alexander :nalexander from comment #3)
> > MattN, vladikoff, others: this seems like the little bit of integration glue
> > I need to write a WebChannel-consuming add-on.  (It works fine locally.) 
> > Are there particular security considerations that need verification in
> > Fennec?
> 
> A few things to check off the top of my head:
> * Make sure the permission manager works the same as desktop (I think it's
> shared code).
> * Check that the correct principal is sent with messages
> * Make sure subframes are handled properly
> 
> > Also, I'd like to understand if this glue code could be simplified and
> > shared between the two applications.  Perhaps we can lift more into
> > toolkit/?  Guidance appreciated.
> 
> Yes please. I think toolkit/content/browser-content.js or
> toolkit/content/process-content.js are good candidates for this code but I'm
> not sure if those are currently loaded on Android. Loading one of them on
> Android probably is straightforward.
> 
> Can you try to do that and see if it's straightforward?

browser-content.js is definitely loaded in Fennec.

> Could you also try enabling the tests in
> toolkit/modules/tests/xpcshell/xpcshell.ini?

Looking at http://ftp.mozilla.org/pub/mozilla.org/mobile/tinderbox-builds/fx-team-android-api-11/1434403161/fx-team_ubuntu64_vm_armv7_mobile_test-xpcshell-3-bm118-tests1-linux64-build55.txt.gz, these tests are already running on mobile/android \o/

I'll try to make this happen.
(In reply to Nick Alexander :nalexander from comment #7)
> (In reply to Matthew N. [:MattN] from comment #6)
> > Could you also try enabling the tests in
> > toolkit/modules/tests/xpcshell/xpcshell.ini?
> 
> Looking at
> http://ftp.mozilla.org/pub/mozilla.org/mobile/tinderbox-builds/fx-team-
> android-api-11/1434403161/fx-team_ubuntu64_vm_armv7_mobile_test-xpcshell-3-
> bm118-tests1-linux64-build55.txt.gz, these tests are already running on
> mobile/android \o/

I don't regularly have to deal with `toolkit` vs. `os` in test manifests but I saw this in the file:
> skip-if = toolkit == 'android'
stomlinson has been working on this most recently in Desktop. He'd be a good person to include here.
Flags: needinfo?(ckarlof) → needinfo?(stomlinson)
Blocks: 1178378
https://reviewboard.mozilla.org/r/11105/#review10679

::: browser/base/content/content.js:314
(Diff revision 1)
> +// This should be kept in sync with /mobile/android/chrome/content/content.js. 

extra space after comment

::: browser/base/content/content.js:317
(Diff revision 1)
>    // if target is window then we want the document principal, otherwise fallback to target itself.

Nick could you fix / update this comment in both file. i.e it should be: 
`use nodePrincipal` if available, otherwise fallback to document` ?

::: mobile/android/chrome/content/content.js:109
(Diff revision 1)
> +// An event listener for custom "WebChannelMessageToChrome" events on pages.

I think we have tests for this in Desktop to make sure this event works correctly. Do we need to add new tests in Fennec or somehow reuse the existing tests?
I did my first review via mozreview, apologies if I submit it wrong or something.

Anyway, this looks pretty in sync with Desktop, adding tests that run for Fennec builds and test this event would be reassuring :)
Flags: needinfo?(vlad)
(In reply to Nick Alexander :nalexander from comment #10)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=3343e8d5f0fd

Worth noting that this try build has many 2.3 rc4 and 4.0 rc9 runs to verify testWebChannel, a Robocop test that duplicates the desktop browser-chrome test_web_channel.js.
Consider me notified, removing the needinfo request.
Flags: needinfo?(stomlinson)
(In reply to Nick Alexander :nalexander from comment #15)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=1644eb1c67c3

In api 11 opt X (http://ftp.mozilla.org/pub/mozilla.org/mobile/try-builds/nalexander@mozilla.com-8a6645645212/try-android-api-11/try_panda_android_test-xpcshell-bm102-tests1-panda-build2160.txt.gz), I see:

23:18:51     INFO -  TEST-START | toolkit/modules/tests/xpcshell/test_web_channel.js
23:18:52     INFO -  TEST-PASS | toolkit/modules/tests/xpcshell/test_web_channel.js | took 782ms
23:18:52     INFO -  mozdevice getting files in '/mnt/sdcard/tests/xpcshell/minidumps'
23:18:52     INFO -  TEST-START | toolkit/modules/tests/xpcshell/test_web_channel_broker.js
23:18:53     INFO -  TEST-PASS | toolkit/modules/tests/xpcshell/test_web_channel_broker.js | took 789ms
23:18:53     INFO -  mozdevice getting files in '/mnt/sdcard/tests/xpcshell/minidumps'

So it looks like we're running (and passing!).  But I don't understand why the minidumps are being fetched -- is this just noise?  gbrown, are these tests actually crashing?
Flags: needinfo?(gbrown)
Attachment #8622005 - Attachment description: MozReview Request: Bug 1174458 - Part 1: Copy-paste WebChannel message support from Desktop. r?MattN → MozReview Request: Bug 1174458 - Move WebChannel message support to toolkit/content. r?MattN
Attachment #8622005 - Flags: review?(MattN+bmo)
Comment on attachment 8622005 [details]
MozReview Request: Bug 1174458 - Move WebChannel message support to toolkit/content. r?MattN

Bug 1174458 - Move WebChannel message support to toolkit/content. r?MattN

This makes WebChannel support available to all XUL applications that
use toolkit/, including browser/ and mobile/android/.

The new Robocop tests are necessary because we can't run the existing
browser-chrome tests on Android (yet).
Just noise - no problem/crash. (We try to get files from minidumps simply to determine if there has been a crash. I'm not sure why, with all the log info we suppress, this message is displayed, but it is nothing to worry about.)
Flags: needinfo?(gbrown)
Bug 1174458 - Move WebChannel message support to toolkit/content. r?MattN
Attachment #8627922 - Flags: review?(MattN+bmo)
Bug 964854 - Reveal cause of exception. r=rnewman
Attachment #8627923 - Flags: review?(rnewman)
Bug 1177855: Fetch and show avatar image as preference icon -r=nalexander.
Attachment #8627925 - Flags: review?(nalexander)
Bug 1177855 : Miscellaneous improvements related to profile avatar caching -r=nalexander.
Attachment #8627926 - Flags: review?(nalexander)
Bug 1177855 - simplified avatar fetch logic r?nalexander.

* Generalized PicassoPreferenceIcon implementation

* Cleaned up profile caching and simplified the complex profile fetch rate limiter logic

* profile always de-serialized from account manager rather from a cache.

* UI always updated asynchronously through broadcast after sync. A defaul icon is used as
  fallback during the on going sync.
Attachment #8627927 - Flags: review?(nalexander)
Attachment #8627922 - Flags: review?(MattN+bmo)
Attachment #8627923 - Flags: review?(rnewman)
Attachment #8627925 - Flags: review?(nalexander)
Attachment #8627926 - Flags: review?(nalexander)
Attachment #8627927 - Flags: review?(nalexander)
Lots of commits hit the wrong ticket.  Sorry for the noise, folks.
Comment on attachment 8627923 [details]
MozReview Request: Bug 964854 - Reveal cause of exception. r=rnewman

https://reviewboard.mozilla.org/r/12329/#review10795

Ship It!
Attachment #8627923 - Flags: review+
Comment on attachment 8622005 [details]
MozReview Request: Bug 1174458 - Move WebChannel message support to toolkit/content. r?MattN

MattN is on PTO.  markh, can you look at this?
Attachment #8622005 - Flags: review?(MattN+bmo) → review?(markh)
Blocks: 1179949
Blocks: 1179978
Comment on attachment 8622005 [details]
MozReview Request: Bug 1174458 - Move WebChannel message support to toolkit/content. r?MattN

https://reviewboard.mozilla.org/r/11107/#review11047

Ship It!
Attachment #8622005 - Flags: review?(markh) → review+
url:        https://hg.mozilla.org/integration/fx-team/rev/419e99a6f242f4d64c9085b150aee1c182294a8b
changeset:  419e99a6f242f4d64c9085b150aee1c182294a8b
user:       Nick Alexander <nalexander@mozilla.com>
date:       Tue Jun 30 11:46:27 2015 -0700
description:
Bug 1174458 - Move WebChannel message support to toolkit/content. r=markh

This makes WebChannel support available to all XUL applications that
use toolkit/, including browser/ and mobile/android/.

The new Robocop tests are necessary because we can't run the existing
browser-chrome tests on Android (yet).
https://hg.mozilla.org/mozilla-central/rev/419e99a6f242
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
You need to log in before you can comment on or make changes to this bug.