1003029 - Desktop client loads additional files for OpenTok SDK from the server

Status: RESOLVED FIXED

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

Description

[Mark Banner (:standard8)]

Even though the main copy of the sdk is local for desktop, we're getting two files still loaded remotely. These will need packaging up (once we're able to put the references in the public repos):

http://static.opentok.com/webrtc/v2.2.4.1/css/ot.min.css
http://static.opentok.com/webrtc/v2.2.4.1/js/dynamic_config.min.js

Comment 1

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Attachment: https://github.com/mozilla/loop-client/pull/28

loop-client patch

Comment 2

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Attachment: https://github.com/adamroach/gecko-dev/pull/21

desktop client patch

Comment 3

[Mark Banner (:standard8)]

Comment on attachment 8418057 [details] [review]
https://github.com/adamroach/gecko-dev/pull/21

NiKo, sorry for the delay in getting to this. Could you rebase it against loop-service branch and regenerate the pull request please?

Comment 4

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

The loop-service branch is broken atm for me (after building); got:

- deriveHawkCredentials is not defined client.js:278
- "loop.hawk-session-token pref not found"

I don't know if that's expected, or if I should use another branch?

Comment 5

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Ayway, gecko-dev PR has been rebased on top of loop-service and pushed. I don't think these changes are the root cause for the two failures mentioned above.

Comment 6

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Attachment: https://github.com/adamroach/gecko-dev/pull/25

Replaces gecko-dev patch since I rebased on top of the loop-service branch.

Comment 7

[Dan Mosedale (:dmosedale, :dmose)]

It may be that the loop-service branch currently needs to be used together with loop-client/bug-976109-hawk-spike.

Comment 8

[Mark Banner (:standard8)]

My suspicion is that you were on the hawk credentials spike, and then moved off it, or you had miss-matched branches. Currently loop-service won't work until we get the hawk stuff running anyway, but loop-service is where we're going to be landing from.

Comment 9

[Rémy Hubscher (:natim)]

Ok lets upgrade the dev server to setup HAWK support tomorrow first thing in the morning then.

Comment 10

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to Dan Mosedale (:dmose) from comment #7)
> It may be that the loop-service branch currently needs to be used together
> with loop-client/bug-976109-hawk-spike.

Turns out this problem happens even in this configuration.  I'm currently suspecting something that happened across the rebase from hawk-spike to bug-976109-hawk-spike.

(In reply to Rémy Hubscher (:natim) from comment #9)
> Ok lets upgrade the dev server to setup HAWK support tomorrow first thing in
> the morning then.

Given that this will break all cookie builds, and the hawk code isn't really landed on the right branches yet, how does deploying before we've at least got the loop-service branch in the right state help us?

Comment 11

[Dan Mosedale (:dmosedale, :dmose)]

Niko, will happily review, but it will probably not happen until around Weds, I need to finish productionalizing the client-side Hawk stuff first.

Comment 12

[Rémy Hubscher (:natim)]

Ok then I will wait for your green light :)

Comment 13

[Alexis Metaireau (:alexis)]

The problem is *not* on the server side. It's possible to develop against a local web server, so no need to deploy it now. Also, TEF would need to update their client in case we release hawk on the server, which is not the case yet.

Comment 14

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8428669 [details] [review]
https://github.com/adamroach/gecko-dev/pull/25

I'll be OOO starting tomorrow, but I've talked to Standard8 about this, so it's on his radar.

Comment 15

[Mark Banner (:standard8)]

Comment on attachment 8418056 [details] [review]
https://github.com/mozilla/loop-client/pull/28

I believe Nicolas is going to come up with a new patch based on the new Elm repo for this.

Comment 16

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Annoyingly, it seems it's actually impossible to set these two asset urls without actually patching the sdk code itself… I'd expect this to be configurable, but it's not: 

- https://github.com/mozilla/gecko-projects/blob/elm/browser/components/loop/content/shared/libs/sdk.js#L2615-L2634
- https://github.com/mozilla/gecko-projects/blob/elm/browser/components/loop/content/shared/libs/sdk.js#L17659-L17667

I can't see any obvious way to hook from outside of that file to override the values. Thoughts? Should I patch this file? Should we ask TB for a configurable option?

Comment 17

[Adam Roach [:abr]]

Attachment: OpenTok library v2.2.5 with dependencies

The current advice from TB is that we simply need to patch the files locally. I *think* we can simply change cdnURL and cdnURLSSL at the top of the sdk.js file to be "otcdn", and then copy the contents of "webrtc/v2.2.5" tree from the attached zip file into browser/components/loop/content/shared/libs/otcdn.

We have explicit clearance from TokBox engineering to change these two variables as necessary to make things work. If we need more extensive modifications, let me know.

Longer term, we probably want something more elegant, like a reconfiguration function. I'll ask about that once we know that these minor modifications provide a viable solution.

Comment 18

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Attachment: bug-1003029.patch

Here's the patch implementing what :abr just described.

Comment 19

[Adam Roach [:abr]]

Comment on attachment 8432504 [details] [diff] [review]
bug-1003029.patch

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

General comment: I know we'd normally want to strip trailing WS; but, given that we'll be getting periodic updates of these files, I think we want to leave those as-is.

Comment 20

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Attachment: bug-1003029-2.patch

Following conversation with Standard8 on IRC, using slightly shorter paths for otcdn urls.

Comment 21

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Grmbl, missed :abr's comment. Will revert to keep using trailing spaces.

Comment 22

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Attachment: bug-1003029-3.patch

(sorry for bugspamming)

Suppressed whitespace changes for sdk.js.

Comment 23

[Mark Banner (:standard8)]

Comment on attachment 8432601 [details] [diff] [review]
bug-1003029-3.patch

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

I don't see the shortened paths.

Comment 24

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

> I don't see the shortened paths.

Well I can see them here https://bugzilla.mozilla.org/attachment.cgi?id=8432601&action=diff#a/browser/components/loop/jar.mn_sec2

I didn't manage to achieve something shorter because the sdk expects to load paths like `cdnUrl + "/webrtc/v2.2.5/images/rtc/audioonly-subscriber.png"`… so for this one the shortest meaningful local path I could come with is `content/browser/loop/otcdn/webrtc/v2.2.5/images/rtc/audioonly-subscriber.png` (which is admittedly quite long), which is composed from:

- `content/browser/` the loop url root
- `loop/otcdn` the cdnUrl value modified by this patch
- `/webrtc/v2.2.5/images/rtc/audioonly-subscriber.png` the hardcoded path loaded by the sdk

I feel like I'm possibly missing obvious, but what?

Comment 25

[Mark Banner (:standard8)]

Ok, I've just tried what I was thinking of on one file:

Moved the file on disk to 'browser/components/loop/content/shared/libs/sdk/2.2.5/dynamic_config.min.js'

In jar.mn:

content/browser/loop/otcdn/webrtc/v2.2.5/js/dynamic_config.min.js                   (content/shared/libs/sdk/2.2.5/dynamic_config.min.js)

This seems to work, and keeps the directory lengths down a bit more (~22 chars)

Comment 26

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Ah, a kind of url rewriting. Makes sense, even though I think it might possibly introduce further complexity when mentally mapping the alias to the actual resource, though this is no big issue, and the shorter paths are definitely prettier. 

Will update the patch, thanks for the feedback :)

Comment 27

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Attachment: bug-1003029-4.patch

Updated patch to shorten sdk url paths as proposed by :standard8.

Comment 28

[Mark Banner (:standard8)]

Comment on attachment 8433318 [details] [diff] [review]
bug-1003029-4.patch

Looks good, I can't see other files being loaded now.

Comment 29

[Mark Banner (:standard8)]

https://hg.mozilla.org/projects/elm/rev/3540ecf33808

Closing for tracking purposes.

Comment 30

[Mark Banner (:standard8)]

Backed out due to bc1 & dt test failures:

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/general/browser_parsable_css.js | Got error message for jar:file:///builds/slave/test/build/application/firefox/browser/omni.ja!/chrome/browser/content/browser/loop/otcdn/webrtc/v2.2.5/css/ot.min.css: Unknown property '-moz-border-radius'. Declaration dropped.
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/general/browser_parsable_css.js | Got error message for jar:file:///builds/slave/test/build/application/firefox/browser/omni.ja!/chrome/browser/content/browser/loop/otcdn/webrtc/v2.2.5/css/ot.min.css: Unknown property '-moz-box-shadow'. Declaration dropped.

(plus more of the -moz-box-shadow).

https://hg.mozilla.org/projects/elm/rev/1631aeaf0d7e

Comment 31

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Attachment: bug-1003029-5.patch

Added sdk css file to css test whitelist.

Comment 32

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Attachment: bug-1003029-6.patch

Reverted to patch version 3 with whitelist added to fix Try build: https://tbpl.mozilla.org/?tree=Try&rev=18309f428679

Comment 33

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Attachment: bug-1003029-7.patch

Updated with a better whitelist comment, sorry for bugspamming.

Comment 34

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Attachment: bug-1003029-8.patch

This is not a bad joke, I really suck at exporting patch it seems. Sorry.

Comment 35

[Mark Banner (:standard8)]

Comment on attachment 8434943 [details] [diff] [review]
bug-1003029-8.patch

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

I'm happy with this from the loop perspective, but we need sign-off on the browser_parsable_css.js from a browser peer. I think Mike might be able to help us here, as he reviewed the first versions of this test.

Mike, there's a try server build with this patch: https://tbpl.mozilla.org/?tree=Try&rev=18309f428679

Comment 36

[:Gijs (he/him)]

Comment on attachment 8434943 [details] [diff] [review]
bug-1003029-8.patch

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

rs=me for the test update.

Comment 37

[Mark Banner (:standard8)]

Attachment: Use local versions of OT assets.

Updated to include license headers for the additional styles from the sdk.

Comment 38

[Adam Roach [:abr]]

Comment on attachment 8435020 [details] [diff] [review]
Use local versions of OT assets.

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

Looks good -- let's get this landed.

Comment 39

[Mark Banner (:standard8)]

https://hg.mozilla.org/projects/elm/rev/9ef473b3d67f

Closing for tracking purposes.

Comment 40

[Mark Banner (:standard8)]

https://hg.mozilla.org/mozilla-central/rev/9ef473b3d67f

Comment 41

[u279076]

Does this need automated and/or manual testing?

Comment 42

[Mark Banner (:standard8)]

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #41)
> Does this need automated and/or manual testing?

This should be able to be manually tested by inspection. I'm not sure of a way to test this automatically, unless we do some sort of inspection of functional tests to look for unexpected network accesses. I guess we might be able to port what mochitest has although, I suspect that involves modifying the browser which we might not be able to do.

Dan, any ideas?

Comment 43

[Dan Mosedale (:dmosedale, :dmose)]

@Standard8: At some level, we could do something like this with a trivial HTTP proxy server.  Or even hooks into Necko, perhaps.

Comment 44

[u279076]

Untracking for QA. Please needinfo me to request testing.