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

RESOLVED FIXED

Status

defect
P1
normal
Rank:
1
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: standard8, Assigned: rhelmer)

Tracking

unspecified
Points:
---
Dependency tree / graph
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [go faster][github], URL)

User Story

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

Items to resolve:

- fix size of loop-button css, fix conversation window toolbar css (Standard8 working on)
- unloading panel on uninstall
(Reporter)

Description

4 years ago
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

4 years ago
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?
Rank: 33
Flags: needinfo?(sescalante)
Flags: firefox-backlog+
Priority: -- → P3
Whiteboard: [go faster]
(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.
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.
Flags: needinfo?(standard8)
(Reporter)

Comment 4

4 years ago
Robert is currently working on the add-on so assigning to him.
Assignee: nobody → rhelmer
Flags: needinfo?(standard8)
Flags: needinfo?(sescalante)
(Assignee)

Comment 5

4 years ago
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.
Status: NEW → ASSIGNED

Updated

4 years ago
Rank: 33 → 27
Priority: P3 → P2
(Assignee)

Comment 6

4 years ago
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 ;)
(Assignee)

Comment 7

4 years ago
(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".
(Assignee)

Comment 8

4 years ago
(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.
(Assignee)

Comment 9

4 years ago
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/rhelmer@mozilla.com-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/loop@testing.mozilla.org.xpi

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.
(Assignee)

Comment 10

4 years ago
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.
(Reporter)

Updated

4 years ago
Depends on: 1216371
Whiteboard: [go faster] → [go faster][github]
(Reporter)

Updated

4 years ago
Blocks: 1223573

Updated

4 years ago
Rank: 27 → 0
Priority: P2 → P1

Updated

4 years ago
Rank: 0 → 1
(Reporter)

Updated

4 years ago
Blocks: 1226706
(Reporter)

Updated

4 years ago
User Story: (updated)
(Reporter)

Updated

4 years ago
Depends on: 1227138
(Reporter)

Updated

4 years ago
User Story: (updated)
(Reporter)

Comment 11

4 years ago
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
User Story: (updated)
(Reporter)

Comment 12

4 years ago
Try server run was good. Next up, rebase, and fix the remaining couple of issues.
User Story: (updated)
(Reporter)

Comment 13

4 years ago
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.
Status: ASSIGNED → RESOLVED
Iteration: --- → 45.2 - Nov 30
Last Resolved: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.