Adam Roach [:abr] Reporter
	Description • 10 years ago

Before we land in m-i, it is absolutely necessary that we have a pref that turns off Loop functionality (this is needed, at a minimum, to prevent the code from riding the train out into Aurora).

So, we need to add a "loop.enabled" pref. It should be on by default in Nightly, and must be off by default otherwise. When set to "false," it needs to at least suppress the Loop button, and prevent registration with the Loop server.

Mark: based on conversations with Maire, I'm assigning this to you. I'll let you add an estimate to the whiteboard. Thanks.

	Mark Banner (:standard8) Assignee
	Comment 1 • 10 years ago

Note that we already have a build-time switch in MOZ_LOOP that's enabled for nightly builds only:

http://hg.mozilla.org/mozilla-central/annotate/1e712b724d17//configure.in#l3907

This is probably sufficient for MLP/FF32, and I'm not sure we want the pref on until we've fixed bug 1015486, due to the comments from bug 1010325 (which basically don't want that code in releases until then).

Therefore targetting at FF33, and depending on bug 1015486. Please let me know if you disagree.

	Adam Roach [:abr] Reporter
	Comment 2 • 10 years ago

(In reply to Mark Banner (:standard8) from comment #1)
> Note that we already have a build-time switch in MOZ_LOOP that's enabled for
> nightly builds only
...
> Therefore targetting at FF33, and depending on bug 1015486. Please let me
> know if you disagree.

As long as no one else objects, I'm fine with this approach.

	Mark Banner (:standard8) Assignee
	Comment 3 • 10 years ago

This depends on the patch in bug 1015486.

This swaps us from the build-time config to a run-time option, ready for moving across to branches. I've confirmed with Maire that for now we'll be enabled by default on central & aurora. We can always toggle that later.

The main part of the patch is the browser-loop.js change, however I've also prevented the loop service starting up if the preference is disabled, just in case someone does something wrong.

Requesting general FF review, build config review and Jesup's review for Media Manager.

	Justin Dolske [:Dolske]
	Comment 4 • 10 years ago

Comment on attachment 8453783 [details] [diff] [review]
Loop needs an enabled pref.

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

r+ assuming adequate answers to my questions. I'd like Gijs to doublecheck the bits around having Loop's button there by default w/ loop.enabled = false interactions, since he knows CustomizableUI / bug 1023304 better. Otherwise this is just straightforward #ifdef removal.

::: browser/app/profile/firefox.js
@@ +1512,5 @@
>  pref("image.mem.max_decoded_image_kb", 256000);
>  
> +#ifndef RELEASE_BUILD
> +pref("loop.enabled", true);
> +#endif

I think "ifdef NIGHTLY_BUILD" is equivalent and certainly clearer.

Also, you'll probably want an |else| setting it to false, so code doesn't need to handle the case of missing prefs (ie, dealing with getBoolPref throwing).

::: browser/base/content/browser.xul
@@ +670,1 @@
>               defaultset="urlbar-container,search-container,webrtc-status-button,bookmarks-menu-button,downloads-button,home-button,loop-call-button,social-share-button,social-toolbar-item"

What happens right now when loop is enabled, but you're placing the loop-call-button into the default set? Seems like we don't want to be adding this to users' UI (even if invisible) until Loop is actually intended be enabled in that release.

(Similarly, what happens if we do add this, and later need to disable Loop?)

Should this remain #ifdef'd until bug 1023193 is sorted out, or is that completely unrelated?

@@ +804,5 @@
>          <!-- XXX Bug 1013989 will provide a label for the button -->
>          <!-- This uses badged to be compatible with the social api code it shares.
>               We may also want it to be badged in the future, for notification
>               purposes. -->
>          <toolbarbutton id="loop-call-button"

XXX

::: browser/components/build/nsModule.cpp
@@ +115,2 @@
>      { NS_ABOUT_MODULE_CONTRACTID_PREFIX "looppanel", &kNS_BROWSER_ABOUT_REDIRECTOR_CID },
>      { NS_ABOUT_MODULE_CONTRACTID_PREFIX "loopconversation", &kNS_BROWSER_ABOUT_REDIRECTOR_CID },

Do these about: pages do the right thing (whatever that is :) when loop.enabled == false?

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +203,1 @@
>          "loop-call-button",

Previous comment, ditto here.

	:Gijs (he/him)
	Comment 5 • 10 years ago

Comment on attachment 8453783 [details] [diff] [review]
Loop needs an enabled pref.

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

rs=me if my assumptions are right and you fix my comments :-)

::: browser/base/content/browser-loop.js
@@ +36,5 @@
>       * delayedStartup.
>       */
>      initialize: function() {
> +      if (!Services.prefs.getBoolPref("loop.enabled")) {
> +        document.getElementById("loop-call-button").hidden = true;

This is wrong, because that getElementById call will return null if the user has customized the button into the palette.

Instead, use:

CustomizableUI.getWidget("loop-call-button").forWindow(window).node

(which shouldn't be null for a XUL widget... although it might not hurt to check)

::: browser/base/content/browser.xul
@@ +670,1 @@
>               defaultset="urlbar-container,search-container,webrtc-status-button,bookmarks-menu-button,downloads-button,home-button,loop-call-button,social-share-button,social-toolbar-item"

This can be un-ifdef'd - in fact, otherwise things get confusing because the pref will control whether the button is functional, but if we ifdef the button itself then the code that hides/unhides it will break if we ifdef the button out.

Basically, I'm assuming that we've made a decision that we're OK shipping this in 33, behind a pref (and that inquisitive users might toggle that pref).

@@ +804,5 @@
>          <!-- XXX Bug 1013989 will provide a label for the button -->
>          <!-- This uses badged to be compatible with the social api code it shares.
>               We may also want it to be badged in the future, for notification
>               purposes. -->
>          <toolbarbutton id="loop-call-button"

Umm, so, I noticed this when I was looking at default widget placement stuff. This button isn't where the defaultset says it should be. And while CustomizableUI sorts that out for you, can you please, with an rs=me and an orthogonal patch, move this to where it should be? (after the home button, before the social share button)

For the purposes of bug 1023304 / bug 1023193, this should really move to CustomizableWidgets.jsm. Shouldn't really be too difficult, but yeah.

	Mike Hommey [:glandium]
	Comment 6 • 10 years ago

Comment on attachment 8453783 [details] [diff] [review]
Loop needs an enabled pref.

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

r+ for the build parts.

::: browser/app/profile/firefox.js
@@ +1512,5 @@
>  pref("image.mem.max_decoded_image_kb", 256000);
>  
> +#ifndef RELEASE_BUILD
> +pref("loop.enabled", true);
> +#endif

> I think "ifdef NIGHTLY_BUILD" is equivalent and certainly clearer.

NIGHTLY_BUILD is (essentially) mozilla-central
RELEASE_BUILD is beta and release

Neither include aurora. So you need to pick the one you actually mean.

> Also, you'll probably want an |else| setting it to false, so code
> doesn't need to handle the case of missing prefs (ie, dealing with
> getBoolPref throwing).

Note that if parts of loop are in toolkit, then the pref setting it to false should be in modules/libpref/src/init/all.js.

::: browser/components/about/AboutRedirector.cpp
@@ +103,5 @@
>      nsIAboutModule::HIDE_FROM_ABOUTABOUT },
>    { "looppanel", "chrome://browser/content/loop/panel.html",
>      nsIAboutModule::URI_SAFE_FOR_UNTRUSTED_CONTENT |
>      nsIAboutModule::ALLOW_SCRIPT |
>      nsIAboutModule::HIDE_FROM_ABOUTABOUT },

Would be better to add those if loop is enabled only, so that about:loop* says the address is not valid instead of the chrome:// file not being found.

::: dom/media/MediaManager.cpp
@@ +1487,5 @@
> +  rv = docURI->EqualsExceptRef(loopURI, &isLoop);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  if (isLoop) {
> +    aPrivileged = true;

Note that if an addon overrides about:loopconversation, that makes it privileged, whatever consequences that has. (but that's not a new problem, the existing code has that problem)

	Mark Banner (:standard8) Assignee
	Comment 7 • 10 years ago

(In reply to Mike Hommey [:glandium] from comment #6)
> > +#ifndef RELEASE_BUILD
> > +pref("loop.enabled", true);
> > +#endif
> 
> > I think "ifdef NIGHTLY_BUILD" is equivalent and certainly clearer.
> 
> NIGHTLY_BUILD is (essentially) mozilla-central
> RELEASE_BUILD is beta and release
> 
> Neither include aurora. So you need to pick the one you actually mean.

We want central and aurora for now. So this means !RELEASE_BUILD. I'll add a comment.

> > Also, you'll probably want an |else| setting it to false, so code
> > doesn't need to handle the case of missing prefs (ie, dealing with
> > getBoolPref throwing).
> 
> Note that if parts of loop are in toolkit, then the pref setting it to false
> should be in modules/libpref/src/init/all.js.

All of Loop is desktop specific at the moment.

> ::: browser/components/about/AboutRedirector.cpp
> @@ +103,5 @@
> >      nsIAboutModule::HIDE_FROM_ABOUTABOUT },
> >    { "looppanel", "chrome://browser/content/loop/panel.html",
> >      nsIAboutModule::URI_SAFE_FOR_UNTRUSTED_CONTENT |
> >      nsIAboutModule::ALLOW_SCRIPT |
> >      nsIAboutModule::HIDE_FROM_ABOUTABOUT },
> 
> Would be better to add those if loop is enabled only, so that about:loop*
> says the address is not valid instead of the chrome:// file not being found.

Not quite, as the file will be there regardless. Loading it in a tab will do what it does today - just a blank screen as the loop api isn't injected. Not sure if this really needs fixed, can always file a follow-up bug for doing something nicer, but seeing as not all about pages are guaranteed to do things, I'm not sure this is a major issue.

> ::: dom/media/MediaManager.cpp
> @@ +1487,5 @@
> > +  rv = docURI->EqualsExceptRef(loopURI, &isLoop);
> > +  NS_ENSURE_SUCCESS(rv, rv);
> > +
> > +  if (isLoop) {
> > +    aPrivileged = true;
> 
> Note that if an addon overrides about:loopconversation, that makes it
> privileged, whatever consequences that has. (but that's not a new problem,
> the existing code has that problem)

Yep, however an add-on can do anything it wants to anyway, so it could find other ways to get privs even without overriding about:loopconversation.

(In reply to Justin Dolske [:Dolske] from comment #4)
> @@ +804,5 @@
> >          <!-- XXX Bug 1013989 will provide a label for the button -->
> >          <!-- This uses badged to be compatible with the social api code it shares.
> >               We may also want it to be badged in the future, for notification
> >               purposes. -->
> >          <toolbarbutton id="loop-call-button"
> 
> XXX

Yep, we'll deal with it in the bug documented (we're still waiting on the label details).

(In reply to :Gijs Kruitbosch from comment #5)
> This is wrong, because that getElementById call will return null if the user
> has customized the button into the palette.
> 
> Instead, use:
> 
> CustomizableUI.getWidget("loop-call-button").forWindow(window).node
> 
> (which shouldn't be null for a XUL widget... although it might not hurt to
> check)

Yes that works.

> Basically, I'm assuming that we've made a decision that we're OK shipping
> this in 33, behind a pref (and that inquisitive users might toggle that
> pref).

Yes, we have.

> @@ +804,5 @@
> >          <!-- XXX Bug 1013989 will provide a label for the button -->
> >          <!-- This uses badged to be compatible with the social api code it shares.
> >               We may also want it to be badged in the future, for notification
> >               purposes. -->
> >          <toolbarbutton id="loop-call-button"
> 
> Umm, so, I noticed this when I was looking at default widget placement
> stuff. This button isn't where the defaultset says it should be. And while
> CustomizableUI sorts that out for you, can you please, with an rs=me and an
> orthogonal patch, move this to where it should be? (after the home button,
> before the social share button)

Ok, I'll do that change as well.

	Mark Banner (:standard8) Assignee
	Comment 8 • 10 years ago

Main patch:

https://hg.mozilla.org/integration/mozilla-inbound/rev/6dc734bab538

The additional change that Gijs requested:

https://hg.mozilla.org/integration/mozilla-inbound/rev/ceef79a9fcb9

	Mike Hommey [:glandium]
	Comment 9 • 10 years ago

(In reply to Mark Banner (:standard8) from comment #7)
> > ::: dom/media/MediaManager.cpp
> > @@ +1487,5 @@
> > > +  rv = docURI->EqualsExceptRef(loopURI, &isLoop);
> > > +  NS_ENSURE_SUCCESS(rv, rv);
> > > +
> > > +  if (isLoop) {
> > > +    aPrivileged = true;
> > 
> > Note that if an addon overrides about:loopconversation, that makes it
> > privileged, whatever consequences that has. (but that's not a new problem,
> > the existing code has that problem)
> 
> Yep, however an add-on can do anything it wants to anyway, so it could find
> other ways to get privs even without overriding about:loopconversation.

This strikes me as being odd. Why do we need a special hardcoded case in MediaManager.cpp, then?

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 10 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/6dc734bab538
https://hg.mozilla.org/mozilla-central/rev/ceef79a9fcb9

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 11 • 10 years ago

So when this merged back to fx-team from mozilla-central, there were some merge conflicts that snuck in.

At first, there was a problem where I somehow accidentally pasted a URL into browser.xul while I was resolving the conflict, which made an #endif get ignored, making the other half of browser.xul get excluded from the compiled build.

So I fixed that in https://hg.mozilla.org/integration/fx-team/rev/427b0299698b but then I realized that that entire toolbarbutton was now duplicated in browser.xul because the merge conflict resolution got confused by it being moved on one side and removed on the other. So I removed the original #ifdef'd version in https://hg.mozilla.org/integration/fx-team/rev/b8e40c01301a which should hopefully fix everything.

I wouldn't be opposed to someone who actually knows this code looking over that file to make sure everything's where it's supposed to be, though...

	Mark Banner (:standard8) Assignee
	Comment 12 • 10 years ago

(In reply to Wes Kocher (:KWierso) from comment #11)
> I wouldn't be opposed to someone who actually knows this code looking over
> that file to make sure everything's where it's supposed to be, though...

I've done a visual check of the diffs and the file and they appear to be fine. I've also run it up and had a play around with it and that seems fine as well.

Thanks.

	Carsten Book [:Tomcat]
	Comment 13 • 10 years ago

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

	u279076
	Comment 14 • 10 years ago

Verified that I can enabled/disable Loop with loop.enabled in about:config.