1186172 - Experiment with Loop as a system add-on for "go faster"

Status: RESOLVED FIXED

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

Description

[Mark Banner (:standard8)]

As part of the Go Faster effort, we want to experiment with Loop as a "system" add-on.

This bug is to track that effort.

Comment 1

[:shell escalante]

add etherpad with details.... correct me if i'm wrong - but this is a Fx44 activity for dev - and just discussion during Q3 while back end goes up?

Comment 2

[Morgan Phillips [:mrrrgn]]

(In reply to :shell escalante from comment #1)
> add etherpad with details.... correct me if i'm wrong - but this is a Fx44
> activity for dev - and just discussion during Q3 while back end goes up?

No, we need a prototype. The idea is to make sure that AMO can support the addon, before we go and build a back end for releasing the addon.

Comment 3

[Romain Testard [:RT]]

Mark, could you please help understand the amount of effort required for this and whether doing it would impact our current plans for 43. The original assumption was that Go Faster would not impact us for the 43 cycle - if it will we need to decide what the priorities are.

Comment 4

[Mark Banner (:standard8)]

Robert is currently working on the add-on so assigning to him.

Comment 5

[Robert Helmer [:rhelmer]]

Working on this prototype in https://github.com/rhelmer/loop-addon

Some notes:

* the current add-on installs and overrides Hello on a vanilla Firefox build, which is fine for prototyping but not really appropriate approach for shipping (more on that below). A better alternative might be to ship Firefox without the built-in feature and with a bundled add-on.

* loop uses about:loop* pages (https://dxr.mozilla.org/mozilla-central/source/browser/components/about/AboutRedirector.cpp#110-121) that point to chrome URIs.

* the add-on currently uses chrome overrides, which works (for content that doesn't depend on navigator.mozLoop or navigator.l10n) but chrome overrides cannot be recursive, so it's a very touchy way to do it (e.g. must override the exact versioned dependencies)

* using add-on SDK, but only to make bootstrapping and to make for easier tooling (e.g. jpm)

The reason I am taking this tack for the prototype is so the add-on can be installed on an existing Firefox install, and not need a special build with the Hello UI removed.

Currently working on overriding the Hello button (using existing code), and injecting the mozLoopAPI and l10n into the overridden URLs.

Comment 6

[Robert Helmer [:rhelmer]]

The chrome override approach in comment 5 is pretty brittle, and I don't think we would ever want to ship it that way - I've gone ahead with a more realistic approach (IMHO) and made a fork of mozilla-central with ./browser/components/loop/ unhooked from browser.xul/js, moved into ./browser/extensions/, and packagable as an extension (for update purposes).

We should be able to reuse the vast majority of existing code, from my experiments so far.

WIP will be up at https://bitbucket.org/rhelmer/mozilla-central when it finally finishes uploading ;)

Comment 7

[Robert Helmer [:rhelmer]]

(In reply to Robert Helmer [:rhelmer] from comment #6)
> The chrome override approach in comment 5 is pretty brittle, and I don't
> think we would ever want to ship it that way - I've gone ahead with a more
> realistic approach (IMHO) and made a fork of mozilla-central with
> ./browser/components/loop/ unhooked from browser.xul/js, moved into
> ./browser/extensions/, and packagable as an extension (for update purposes).
> 
> We should be able to reuse the vast majority of existing code, from my
> experiments so far.
> 
> WIP will be up at https://bitbucket.org/rhelmer/mozilla-central when it
> finally finishes uploading ;)

I have a working prototype in the above bitbucket repo now - you need to:

1) build the browser ("./mach build" with default config should be enough)
2) disable add-on signature checking (set xpinstall.signatures.required to false in about:config)
3) zip the contents of ./browser/extension/loop into loop.xpi
4) install loop.xpi (can do this from local filesystem via about:addons)

There are a few bits left behind with this approach, but (after discussion with Mark) nothing that we consider blockers right now:

* CSP check for about:loop* in platform
** https://dxr.mozilla.org/mozilla-central/source/dom/base/nsDocument.cpp#2759
** workaround for bug 663570 not being available
* special casing for about:loop* to avoid getUserMedia prompt
** https://dxr.mozilla.org/mozilla-central/source/browser/modules/webrtcUI.jsm#258
** https://dxr.mozilla.org/mozilla-central/source/dom/media/MediaManager.cpp

Registration of about:loop* pages and overriding the browser toolbar CSS are still in ./browser too, but I expect to be able to move these into the add-on shortly (working on that now), at that point I think we can consider the prototype to be "complete".

Comment 8

[Robert Helmer [:rhelmer]]

(In reply to Robert Helmer [:rhelmer] from comment #7)
> Registration of about:loop* pages and overriding the browser toolbar CSS are
> still in ./browser too, but I expect to be able to move these into the
> add-on shortly (working on that now), at that point I think we can consider
> the prototype to be "complete".

I have themes moved into the add-on now, just need to finish porting over the different overrides from the jar manifest to the add-on chrome manifest (there are icons for different OS versions etc). It's working on Mac at least now.

I am actual wondering if about:loop* page registration should stay in browser/components/about/AboutRedirector.cpp and not move into the add-on... since we have some hardcoded permissions in a few other places in Gecko for these URLs we may not want to allow add-ons to override these.

Comment 9

[Robert Helmer [:rhelmer]]

I've started to look at what it would take to make something like this shippable using our current release infra - I made a try build with my current patch:

https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-150066c728b3/

I am working on getting the loop add-on to pass the AMO validator and get signed, in the meantime I have an unsigned copy here:

https://people.mozilla.org/~rhelmer/[e-mail]

You'll need to toggle xpinstall.signatures.required in about:config if you test this. Note that the add-on requires the try builds above, it will install into a vanilla Nightly install but won't be functional due to the way about:loop* pages are registered, per previous comments.

Comment 10

[Robert Helmer [:rhelmer]]

The latest version of this code is on github (gecko-dev fork), in this PR:

https://github.com/mozilla/system-addons/pull/6

I've split the renames into a separate commit, for ease of reviewing.

Also - I did manage to get the loop add-on to pass the AMO validator and got a signed version, although it required a manual review for some of the issues it was finding.

Integrating the AMO add-on validator as part of any Loop pre-checkin tests would probably be helpful for catching potential issues before it gets to the point of signing the XPI.

Comment 11

[Mark Banner (:standard8)]

Ok, I've worked through some of the test issues today. As part of this, we decided to keep "loop-button" in the list of default placements in CustomizableUI.jsm, as it isn't easy for add-ons to alter that list (intentionally), but we still want Hello to be there as its a default feature.

There's now a fresh try push running with today's fixes, so we can check how much greener the tree gets:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=22a514bb37fa

Comment 12

[Mark Banner (:standard8)]

Try server run was good. Next up, rebase, and fix the remaining couple of issues.

Comment 13

[Mark Banner (:standard8)]

The "experimental" add-on is now complete apart from one minor thing - we don't currently unload the panel.

However, this is good enough that we can proceed with getting it landed, which we'll be working on in bug 1223573.

Hence marking this as fixed.