1008122 - Adjust Loop content to adapt for split of Loop from social

Status: RESOLVED FIXED

(Hello (Loop) :: Client, defect)

Description

[Mark Banner (:standard8)]

Bug 1000007 is providing the split of Loop from the Social API. We need to adapt the existing content to cope with the new mozLoop API.

Comment 1

[Mark Banner (:standard8)]

Attachment: Loop content changes for the separation of Loop from Social

Most of this is changes for the API. I've also removed fxcom.js which is no longer used.

Markh asked for this to be separated out, hence the separate bug/patch.

Comment 2

[Mark Banner (:standard8)]

Comment on attachment 8420035 [details] [diff] [review]
Loop content changes for the separation of Loop from Social

Switching to Nicolas as Dan is busy.

Comment 3

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

Comment on attachment 8420035 [details] [diff] [review]
Loop content changes for the separation of Loop from Social

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

Definitely looks like an overall improvement. I keep thinking that passing the mozLoop object as a dependency is a better strategy regarding router testability, but this might be discussed.

::: browser/components/loop/content/js/conversation.js
@@ +83,5 @@
>        // XXX For now, we just kick the conversation straight away, bug 990678
>        // will implement the follow-ups.
>        this._conversation.set({loopVersion: loopVersion});
>        this._conversation.initiate({
> +        baseServerUrl: window.navigator.mozLoop.loopServer(),

Also, how about passing the whole mozLoop object as a dependency to the router object? Mostly to help testing by passing an easily stubbed object.

::: browser/components/loop/content/js/panel.js
@@ +53,5 @@
>          throw new Error("missing required notifier");
>        }
>        this.notifier = options.notifier;
>        this.client = new loop.shared.Client({
> +        baseServerUrl: window.navigator.mozLoop.loopServer()

I wasn't expecting a function call here; to me loopServer should be an object with a url property (we could alternatively rename the whole mozLoop property to loopServerUrl)

@@ +175,5 @@
> +
> +    // Notify the window that we've finished initalization and initial layout
> +    var evtObject = document.createEvent('Event');
> +    evtObject.initEvent('loopPanelInitialized', true, false);
> +    window.dispatchEvent(evtObject);

Thinking out loud: {sh|c}ouldn't this be triggered directly from the mozLoop object? Calling smthg like mozLoop.initialized() would be very meaningful to me, and would also ease testing a little.

::: browser/components/loop/content/libs/l10n.js
@@ +9,5 @@
>    var gLanguage = '';
>  
>    // fetch an l10n objects
>    function getL10nData(key) {
> +    var response = window.navigator.mozLoop.getStrings(key);

Not sure coupling mozLoop to mozL10n is a good idea, as it prevents further sharing of that file.

@@ +79,5 @@
>        translateElement(element);
>    }
>  
>    window.addEventListener('DOMContentLoaded', function() {
> +    gLanguage = window.navigator.mozLoop.getLocale();

Same remark as above; I suspect decoupling mozLoop from mozL10n would involve patching the original lib as well, so not sure what we should do here especially regarding the tight schedule…

::: browser/components/loop/test/desktop-local/conversation_test.js
@@ +35,5 @@
> +      // that somehow.
> +      getLocale: function() {
> +        return "en-US";
> +      }
> +    };

Same remark as above, I'd rather see a fake mozLoop object passed as a dependency to the tested router instances.

@@ +40,4 @@
>    });
>  
>    afterEach(function() {
> +    window.navigator.mozLoop = savedMozLoop;

Passing a fake mozLoop object as a dependency would avoid this step.

Comment 4

[Mark Banner (:standard8)]

(In reply to Nicolas Perriault (:NiKo`) from comment #3)
> Definitely looks like an overall improvement. I keep thinking that passing
> the mozLoop object as a dependency is a better strategy regarding router
> testability, but this might be discussed.

We discussed this and decided that for now, at least, it was better to keep the explicit window.navigator.mozLoop as this is a clearer indication of the fact we're accessing privileged code, plus it keeps the code slightly less complex.

> ::: browser/components/loop/content/js/panel.js
> @@ +53,5 @@
> >          throw new Error("missing required notifier");
> >        }
> >        this.notifier = options.notifier;
> >        this.client = new loop.shared.Client({
> > +        baseServerUrl: window.navigator.mozLoop.loopServer()
> 
> I wasn't expecting a function call here; to me loopServer should be an
> object with a url property (we could alternatively rename the whole mozLoop
> property to loopServerUrl)

Made into a getter and renamed serverUrl.

> > +    // Notify the window that we've finished initalization and initial layout
> > +    var evtObject = document.createEvent('Event');
> > +    evtObject.initEvent('loopPanelInitialized', true, false);
> > +    window.dispatchEvent(evtObject);
> 
> Thinking out loud: {sh|c}ouldn't this be triggered directly from the mozLoop
> object? Calling smthg like mozLoop.initialized() would be very meaningful to
> me, and would also ease testing a little.

Unfortunately, mozLoop isn't directly coupled to the code that handles this event (which is in browser-loop.js), so it seems best to leave it as an event.

> ::: browser/components/loop/content/libs/l10n.js
> @@ +9,5 @@
> >    var gLanguage = '';
> >  
> >    // fetch an l10n objects
> >    function getL10nData(key) {
> > +    var response = window.navigator.mozLoop.getStrings(key);
> 
> Not sure coupling mozLoop to mozL10n is a good idea, as it prevents further
> sharing of that file.

As discussed, I'll see if I can find a way to pass it in, as this would make it easier to share, and we might be able to get rid of the DOMContentLoaded handler, which would definitely make it easier for testing.

> Same remark as above; I suspect decoupling mozLoop from mozL10n would
> involve patching the original lib as well, so not sure what we should do
> here especially regarding the tight schedule…

For now, we keep them separate, but we can always handle merging them later.

Comment 5

[Mark Banner (:standard8)]

Attachment: Loop content changes for the separation of Loop from Social v2

Updated patch addressing Nicolas' comments, and removing some of the events from l10n.js to make it easier for testing.

Comment 6

[Mark Banner (:standard8)]

Attachment: Loop content changes for the separation of Loop from Social v3

Now including changes in conversation/panel.html to cope with the slightly different source urls when using about uris (the patch for those is in bug 976109).

Comment 7

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

Comment on attachment 8422238 [details] [diff] [review]
Loop content changes for the separation of Loop from Social v3

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

LGTM with minor nits you may want to address optionally.

::: browser/components/loop/content/js/conversation.js
@@ +121,5 @@
>     */
>    function init() {
> +    // Do the initial L10n setup, we do this before anything
> +    // else to ensure the L10n environment is setup correctly.
> +    document.mozL10n.initialize(window.navigator.mozLoop);

nit: mozL10n is directly available as one of the main IIFE argument

::: browser/components/loop/content/js/panel.js
@@ +168,5 @@
>     */
>    function init() {
> +    // Do the initial L10n setup, we do this before anything
> +    // else to ensure the L10n environment is setup correctly.
> +    document.mozL10n.initialize(window.navigator.mozLoop);

nit: mozL10n is directly available as one of the main IIFE argument; btw maybe we want to do the same for mozLoop? or remove the args passed to the IIFE if we want to quit that strategy, for consistency purpose.

Comment 8

[Mark Banner (:standard8)]

(In reply to Nicolas Perriault (:NiKo`) from comment #7)
> ::: browser/components/loop/content/js/panel.js
> @@ +168,5 @@
> >     */
> >    function init() {
> > +    // Do the initial L10n setup, we do this before anything
> > +    // else to ensure the L10n environment is setup correctly.
> > +    document.mozL10n.initialize(window.navigator.mozLoop);
> 
> nit: mozL10n is directly available as one of the main IIFE argument; btw
> maybe we want to do the same for mozLoop? or remove the args passed to the
> IIFE if we want to quit that strategy, for consistency purpose.

I think mozLoop is the special case for non IIFE as that's to highlight the security implications. We might want to change that as we go forward, but I suggest we see how it looks.

Comment 9

[Mark Banner (:standard8)]

Landed in gecko-dev:

https://github.com/adamroach/gecko-dev/commit/58f8d5a75f893e20c0fad2c75578b84e3b4d3491

Marking as fixed for tracking purposes, will update with mercurial urls later.

Comment 10

[Mark Banner (:standard8)]

https://hg.mozilla.org/projects/elm/rev/ac73e63109d8

Comment 11

[Mark Banner (:standard8)]

https://hg.mozilla.org/mozilla-central/rev/ac73e63109d8

Comment 12

[u279076]

Is there any QA testing required here?

Comment 13

[u279076]

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #12)
> Is there any QA testing required here?

Assuming this doesn't need any QE verification at this point. Please needinfo me to request testing.