Closed Bug 1338586 Opened 3 years ago Closed Last year

Ship the GoFaster addon 2.0.1 to Fennec

Categories

(Web Compatibility :: Interventions, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: miketaylr, Assigned: denschub)

References

Details

Attachments

(2 files, 1 obsolete file)

We need this running in Fennec, to one day replace our ua-overrides mechanism, and benefit from the new capabilities.

Some questions to figure out.

Does gofaster work in Fennec?

Do we need to just copy the files into the mobile tree and tweak the right build configs?

Do we need to generate and distribute two xpis when pushing updates?
Depends on: 1383989
Summary: Figure out how to get the go faster addon running in Fennec → Ship the Go Gaster addon in Fennec
Blocks: 1386807
We had some discussion about whether or not to duplicate the sources between Firefox for Android and Desktop, so I had a discussion with Robert Helmer, the current tech lead for Go Faster. Basically, it boils down to this:

(some additional lines removed since they didn't add too much)
> <rhelmer> hm, yeah things that are shared across browser and mobile are tricky... usually that sort of thing goes into toolkit/ but I don't know if that's appropriate here.
> <DenSchub> it looks like we can't build system extensions from toolkit/ yet, so that'd require some build toolchain modifications, probably, yeah
> <rhelmer> yeah. I think for sure anything done here to dedup will require some build system work.
> [...]
> <rhelmer> I'd take a shot at de-duping... toolkit/extensions/ or something along those lines... it'll be confusing no matter what I think :(
> <rhelmer> that said I don't think the duplicated sources is the worst thing, could do the dedup as a followup.
> <rhelmer> I'd be surprised if they don't fork at some point
> <rhelmer> pretty cool if not I guess
> <rhelmer> depends on how urgent you need to land it and what else you need to do :)

Some interesting points here: If we see a chance that we ever end up forking the extension for one platform, it would be easy to rule out deduping straight away. However, I currently see no reason why we need to fork - overriding UAs works just fine and even if we need APIs that only work on one platform, we always can check where we're running at and only load the modules that we need.

That being said, Firefox for Android also usually likes to be a complete independent project and reduce the amount of shared code if possible (also noted by Robert), so this decision is actually pretty hard. Also, working on the build tools could turn out to be hard, especially with building XPIs and stuff.

For now, I'd probably try to land the sources as a duplication. If we get a negative review for duping, we know that we should dedup. If we can land it, we should file a follow up bug to dedup the sources. In that bug, we can have an extensive discussion with Fennec people and build automation people to see if that's worth the effort. Do you agree? (not ni? anyone yet, will do that if landing it comes closer)
No longer blocks: 1386807
Note: The lack of reviewer on this patch is currently on purpose. I had some issues building Fenenc, and although it worked out in the end, I want to have a try build running as an additional sanity check before I ask anyone for review. Try push is at https://treeherder.mozilla.org/#/jobs?repo=try&revision=d810925c4e63c3b36a81c2f72c79028b8811a2b2
Summary: Ship the Go Gaster addon in Fennec → Ship the Go Gaster addon 2.0.1 in Fennec
Summary: Ship the Go Gaster addon 2.0.1 in Fennec → Ship the GoFaster addon 2.0.1 to Fennec
Adding the actual overrides as dependencies.
Assignee: nobody → dschubert
Depends on: 755590, 1480710
QA Contact: igor.lazar
Attachment #8997394 - Attachment is obsolete: true
Attached file Unsigned XPI, rev. 1
Attached is an unsigned XPI of the current sources for WebCompat GoFaster v2.0.1 for testing.
Robert, as discussed on IRC, I would really appreciate your review here.

For background: The WebCompat GoFaster addon is, in short, an extension that we use to fix broken websites. We do that by either spoofing the User Agent string to get around UA blocks, or by injecting ContentScripts to alter the site to work around issues. More info can be found at [0].

An earlier version of this is already landed in Desktop for some months, see bug 1386807. This means that large parts of this extension (basically everything except for files inside `webextension/injections` and one small change to the `registerContentScripts()` function) has been reviewed by Firefox peers before. Now, we want to have this extension in Fenenc as well, and have it ride the train for now.

In addition to the extension sources, this patch also includes some actual content scripts (aka "site patches") inside the embedded webextension. If you want to test those sites and the effects of the contentScripts, we have a Google Doc with QA testing steps at [1].

This patch also includes contentScripts that will never run on Firefox for Android, as well as user agent spoofs that will never get applied. We do this deliberately: we maintain the sources for both platforms in one GitHub repo, and, in future, will update the extension via GoFaster, so we need to cover both platforms with the same extension.

If you have any questions, please let me know.

[0]: https://wiki.mozilla.org/Compatibility/Go_Faster_Addon
[1]: https://docs.google.com/document/d/1J2Tg6yCPBpa-3LxMVxrNvgl2B9PMSopJu4TzQObQNv8
For the record, here is a new try push, which also includes the last-minute site patch, and a fresh rebase: https://treeherder.mozilla.org/#/jobs?repo=try&revision=27f750bd917fec489cb868b314594e6dc1a98061
Comment on attachment 8998825 [details]
Bug 1338586 - Ship the WebCompat GoFaster extension to Fennec. r=rhelmer

Robert Helmer [:rhelmer] has approved the revision.
Attachment #8998825 - Flags: review+
(In reply to Robert Helmer [:rhelmer] from comment #9)
> Comment on attachment 8998825 [details]
> Bug 1338586 - Ship the WebCompat GoFaster extension to Fennec
> 
> Robert Helmer [:rhelmer] has approved the revision.

Hm, so I am pretty new to phabricator, does it not reflect review comments into Bugzilla?

Please see the inline review comments for some important notes on deprecated features that you are going to run into in a few releases (Firefox 64) as well as a few small nits.
Thanks so much for your quick review! Indeed, phabricator does not mirror review flags besides r+ and comments to Bugzilla, by design[0], but I received emails for every single comment, so that's good. I addressed your remarks and replied to your note about legacy extensions, thanks for that. For the record, and for people CC'ed here:

> Yes, we want to keep the extension past 64, and we already have a a bug filed for porting the UA overriding logic to the WebExtension part and dropping the legacy stuff. As far as I can tell right now, we won't even need any non-standard APIs. Porting is the next task I will be working on, but for now, getting some sites fixes is more important for us, so we are happy with shipping this for now, and have the replacement ready when we need.
> 
> Thank you for highlighting it, though! :)

[0]: https://wiki.mozilla.org/Phabricator/FAQ#Why_don.27t_we_mirror_.27r.3F.27_and_other_review_states_to_Bugzilla.2C_why_just_.27r.2B.27.3F
QA Contact: igor.lazar → stefan.deiac
Attachment #8998825 - Attachment description: Bug 1338586 - Ship the WebCompat GoFaster extension to Fennec → Bug 1338586 - Ship the WebCompat GoFaster extension to Fennec. r=rhelmer
After chatting with QA people, I decided to land this in Fennec without explicit approval. I had a chat with them, and they usually only test features already landed in Nightly, and as the tests are green and we already have almost the same code landed in Desktop, I see no reason to wait with landing.

Leaving this bug open after landing until we have a green result from QA, just so this stays on our table. :)
Pushed by rvandermeulen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/18b999f1a4c1
Ship the WebCompat GoFaster extension to Fennec. r=rhelmer
Keywords: checkin-needed
Depends on: 1484706
No longer depends on: 1484706
This has been merged three months ago, there are no blocking regressions, I am not sure why this is still open. :)
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Ah, it was open because I wrote Comment 12... QA was fine with the release, but I never came back to close this issue. Mea culpa.
Keywords: leave-open
You need to log in before you can comment on or make changes to this bug.