1451485 - Convert webcompat-reporter to a webextension

Status: RESOLVED FIXED

(Web Compatibility :: Tooling & Investigations, enhancement, P1)

Comment 1

[Dennis Schubert [:denschub]]

We track webcompat-reporter bugs in Web Compat Tools::General, moving there. :)

Comment 2

[Mike Taylor [:miketaylr]]

Andrew, will it be possible to somehow get preference values as a webextension? e.g., https://dxr.mozilla.org/mozilla-central/source/browser/extensions/webcompat-reporter/content/WebCompatReporter.jsm#21-28

(we also maintain webextension-based addons here https://github.com/webcompat/webcompat-reporter-extensions, but they're less powerful).

Comment 3

[Andrew Swan [:aswan]]

Its not possible to get arbitrary prefs from a general-purpose webextension, but we can either add a new API that's restricted to webcompat-reporter or webcompat-reporter can bundle some privileged code to get at the preferences it needs.
I'd be happy to walk you through how this works on irc or vidyo or something some time if that would help.

Comment 4

[Mike Taylor [:miketaylr]]

I'm gonna try to port this over to a bundle web extension experiment, hopefully within this quarter!

References:

https://firefox-source-docs.mozilla.org/toolkit/components/extensions/webextensions/index.html
https://github.com/rhelmer/webext-experiment-crashme/tree/bundled-experiment

Comment 5

[:shell escalante]

Hi Mike,  Checking in on if this bug has an owner. 

I'm not sure which channels webcompat-reporter is used against.

Timing wise bootstrapped code will start being removed from 65 Nightly after Sept 22 (completed before Dec 11).  Bootstrapped add-ons won't work in 65 Beta (on Dec 11) or 65 Release (on Jan 29th).

Comment 6

[:shell escalante]

correction - Nightly code removal starts OCTOBER 22 (not Sept), sorry.

Comment 7

[Mike Taylor [:miketaylr]]

Heya Shell. Yeah, Tom will be working on this -- our goal is to get this finished by the end of Q3, so I think we should be in good shape. AFAIU, he has local patches, which is good...given that the end of the quarter is quickly approaching. When he's back from PTO next week he'll pick this up.

Comment 8

[Thomas Wisniewski [:twisniewski]]

Attachment: wip.patch

Here's a work-in-progress patch that just removes the old addon's files, adds in my webextension version's files, and updates moz.build accordingly.

Unfortunately, it's not working when I actually build it - the page action doesn't appear, and nothing related is logged in the browser console. I don't see any build warnings, either.

Whereas if I load the webextension from the patched directory in about:debugging in a running browser, it does work fine.

I'm not sure what I'm doing wrong here.

Comment 9

[Thomas Wisniewski [:twisniewski]]

Comment on attachment 9010051 [details] [diff] [review]
wip.patch

Ah, it turns out that the patch *was* working, I just had a stale temp profile when testing with mach run (thanks to aswan for helping me figure that out!)

I'm marking the patch I uploaded here as obsolete, as I still have to get the mochitests passing anyway.

Comment 10

[Thomas Wisniewski [:twisniewski]]

I'm running into test failures with the webextension version that I'm not sure how to handle: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8a54431f1152eeae7a2324eab6cf1f23f4c15c61&selectedJob=200230911

Basically, the webextension version is causing resource://gre/modules/WebNavigationFrames.jsm to be loaded early enough to trip browser_startup_content.js, which isn't expecting that module to be loaded early.

But the webextension seems to be loaded after browser-delayed-startup-finished, since if I add a listener for browser-delayed-startup-finished, it never hears that event.

florian, aswan suggested that I check with you on whether to just add webNavigationFrames.jsm to the whitelists in the tests, or if there's something else we should potentially be doing here?

Comment 11

[Thomas Wisniewski [:twisniewski]]

Actually, nevermind. It turns out that using webNavigation in any way seems to cause a lot of very strange breakage, and so since I only needed webNavigation to detect when the page action needs to be updated, and page actions have an internal onLocationChanged API, I just stopped using webNavigation entirely in favor of that internal API (via an extension experiment, since I need a bunch of those anyhow). So far this approach seems to have fixed the weird failures in my try-run, so I'm optimistic that I'm almost done here.

Comment 12

[Thomas Wisniewski [:twisniewski]]

Heh. Should have known better than to feel optimistic. Looks like there are still odd issues to work out: https://treeherder.mozilla.org/#/jobs?repo=try&revision=584c7dd86131e92987edb59f8dcfbabd108947ab

Comment 13

[Florian Quèze [:florian]]

(In reply to Thomas Wisniewski [:twisniewski] from comment #10)

> Basically, the webextension version is causing
> resource://gre/modules/WebNavigationFrames.jsm to be loaded early enough to
> trip browser_startup_content.js, which isn't expecting that module to be
> loaded early.
> 
> But the webextension seems to be loaded after
> browser-delayed-startup-finished, since if I add a listener for
> browser-delayed-startup-finished, it never hears that event.

Note: browser_startup_content.js is testing the code loaded in new content processes. browser-delayed-startup-finished is a notification in the parent process, so you shouldn't expect it in content processes.

Comment 14

[Thomas Wisniewski [:twisniewski]]

Thankfully it seems as though I've finally managed to figure out the test failures, as only known intermittents are showing up now in my try-runs: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e3766b40b6cad574b71dba85bb6989e911e7c2a3

Comment 15

[Thomas Wisniewski [:twisniewski]]

Attachment: Bug 1451485 - convert webcompat-reporter to a webextension; r?aswan

convert webcompat-reporter to a webextension

Comment 16

[Axel Hecht [:Pike]]

Full disclosure, we don't have plans to add support for https://developer.chrome.com/extensions/i18n l10n for Firefox, and instead would like to see Fluent support for web extensions. If that's becoming time-pressing, we could use some help on bug 1425104.

Comment 17

[Thomas Wisniewski [:twisniewski]]

But Firefox *does* support that API (otherwise how is this webextension version of the system addon using it and getting localized strings?)

Of course once we have a Fluent-based API I don't mind migrating the addon over to it. But for now, this appears to do the job in time for our bootstrapped addon deprecation date.

Or was there something I should already be doing instead? I'm not against changing something to ease an eventual transition.

Comment 18

[Axel Hecht [:Pike]]

Right, we support the API, but that's not enough to get Firefox localized.

To use this scheme in Firefox itself, we'd have to support it in the full stack, from l10n repacks, to the logic that extracts strings from m-c to cross-channel, we might need changes in pontoon, we don't have dashboard support.

We'd also effectively loose all current translations. We do have code to migrate to Fluent, but extending that to other file formats is all but straight-forward.

Comment 19

[Thomas Wisniewski [:twisniewski]]

Fair enough. Then I'll just leave the code as-is for now (pending review), and when the time comes and we have a better API, we'll convert it to use that instead.

Comment 20

[Francesco Lodolo [:flod]]

(In reply to Thomas Wisniewski [:twisniewski] from comment #19)
> Fair enough. Then I'll just leave the code as-is for now (pending review),
> and when the time comes and we have a better API, we'll convert it to use
> that instead.

This means shipping untranslated content in Firefox, doesn't it? I'd be OK with this in Nightly, not so much in Beta (which, I think, was enabled recently).

Comment 21

[Andrew Swan [:aswan]]

Comment on attachment 9011293 [details]
Bug 1451485 - convert webcompat-reporter to a webextension; r?aswan

Andrew Swan [:aswan] has approved the revision.

Comment 22

[Mike Taylor [:miketaylr]]

(In reply to Francesco Lodolo [:flod] from comment #20)
> (In reply to Thomas Wisniewski [:twisniewski] from comment #19)
> > Fair enough. Then I'll just leave the code as-is for now (pending review),
> > and when the time comes and we have a better API, we'll convert it to use
> > that instead.
> 
> This means shipping untranslated content in Firefox, doesn't it? I'd be OK
> with this in Nightly, not so much in Beta (which, I think, was enabled
> recently).

Erm yeah. We don't want to send English strings in Beta. What should we be doing to prevent that (are we blocked on some web extension API)?

Comment 23

[Francesco Lodolo [:flod]]

Yes, we're basically blocked by supporting Fluent in WebExtension (comment 16 and 18, bug 1425104).

Comment 24

[Thomas Wisniewski [:twisniewski]]

flod, how are formautofill and screenshots working around this (assuming they are?)

Comment 25

[Francesco Lodolo [:flod]]

Screenshots is not localized in mozilla-central, and code+translations are periodically dumped into m-c.

I believe they convert from .properties (the format used for localization) to JSON, using a npm package (pontoon-to-webext)
https://searchfox.org/mozilla-central/source/browser/extensions/screenshots/webextension/_locales
https://github.com/mozilla-services/screenshots/blob/010867ebe909788bd5a3cf2b5bb2915777404b8b/Makefile#L125

I really don't like the idea to use this approach for more projects, for one because it doesn't scale.

Form Autofill is localized in mozilla-central (.properties), but I don't know exactly what they're doing. Seems to be a WebExtension since bug 1449055, with recent issues in packaging (bug 1484325).

Comment 26

[Mike Taylor [:miketaylr]]

Thanks, it looks like Bug 1425104 is getting pretty close to done. We should still be able to hit our 64 target.

Comment 27

[Andrew Swan [:aswan]]

I'm not confident that bug 1425104 is going to get done for 64, can we continue to use .properties for now (since the whole l10n pipeline can support it) and add another experiment api to shim those strings up to the webextension for now?

Comment 28

[Mike Taylor [:miketaylr]]

Is this a question for Flod? Or Tom?

Comment 29

[Andrew Swan [:aswan]]

For Tom.  He and I just chatted on IRC and I think we have a plan...

Comment 30

[Thomas Wisniewski [:twisniewski]]

Alright, I think I'm on track to have a patch update on Monday (I'm away tomorrow).

As I discussed with aswan, I'm using a similar approach to formautofill to load the chrome.manifest that the bootstrapped version of this addon used, and have an experimetal API read strings from it using Services.strings.createBundle/GetStringFromName.

A proof-of-concept is already working, and now I just have to hook the strings up properly.

Comment 31

[Axel Hecht [:Pike]]

Thank you. Feel free to request review on the l10n build bits, if there's still gonna be some. It'd be great if we could use the existing .properties files as is, where they are in the tree now. Then the build bits would also be zero, I guess?

Comment 32

[Thomas Wisniewski [:twisniewski]]

Yes, my plan is to just keep the build bits working as they were for the bootstrapped addon. That is, the webcompat.properties file will continue to be used in the same way. I'll see if I can't get a candidate patch up here before I leave for the day.

Comment 33

[Thomas Wisniewski [:twisniewski]]

Alright, I've updated the patchset accordingly: https://phabricator.services.mozilla.com/D6575?vs=19171&id=19595&whitespace=ignore-most#toc

It seems to be working fine for me in a local build, but I haven't yet verified if there's any test breakage (I'm doing a try-run now: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6b21cb9a85c5d1b98aff1ba88b4f0b32754909b9)

Comment 34

[Thomas Wisniewski [:twisniewski]]

I've addressed the latest review feedback: https://phabricator.services.mozilla.com/D6575?vs=19595&id=19891&whitespace=ignore-most#toc

Let me know if there are any other concerns.

Comment 35

[Thomas Wisniewski [:twisniewski]]

I'm doing a fresh try-run here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7c96f8096e2e99de60b858b2e6c8b85a70962122

Also a Talos run: https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&newProject=try&newRevision=7c96f8096e2e99de60b858b2e6c8b85a70962122&framework=1&showOnlyImportant=1&showOnlyConfident=1&selectedTimeRange=172800

Comment 36

[Thomas Wisniewski [:twisniewski]]

Andrew, I'm not sure what to do about the Talos regressions. As we discussed on IRC they seem similar to what Screenshots is getting in their own webextension porting effort, and I'm not familiar enough with the issues I see in bug 1491997 to know where to go from here. I'll follow that bug just in case there's something I can do to help, but beyond that I think we're just blocked on those issues being fixed (or a decision being made to soak up the regressions for now).

Comment 37

[Thomas Wisniewski [:twisniewski]]

:pike, could you give this a review ASAP? I didn't realize your go-ahead was still needed on Phabricator: https://phabricator.services.mozilla.com/D6575

(In the end I did what I suggested in comment 30, so I'm not sure what there will be for you to actually review).

Comment 38

[Thomas Wisniewski [:twisniewski]]

Um, not sure how I resolved this as fixed... re-opening.

Comment 39

[Thomas Wisniewski [:twisniewski]]

I just started a couple of try-runs to hopefully help measure the Talos regressions;

With this patch applied: https://treeherder.mozilla.org/#/jobs?repo=try&revision=be99436c021f5dd043a9624df3801e65de59fd23

Without this patch applied: https://treeherder.mozilla.org/#/jobs?repo=try&revision=99668a2d21348d5064727f2f1d03800a2ee3d91f

Comment 40

[Axel Hecht [:Pike]]

The try push helps me, too. Commented in phabricator.

Comment 41

[Thomas Wisniewski [:twisniewski]]

Thanks! Here's hoping the try-runs give us some valuable insight.

Comment 42

[Andrew Swan [:aswan]]

Hm, the profiles for this job is an empty zip file :/
https://treeherder.mozilla.org/#/jobs?repo=try&revision=be99436c021f5dd043a9624df3801e65de59fd23&selectedJob=204289740

Comment 43

[Thomas Wisniewski [:twisniewski]]

Darn. I retriggered that job in the hopes that the second time would be a charm, but it too spit out a useless 22-byte zip file :S

If you think it's worth trying a completely fresh try-run in the hopes for a better result, let me know and I'll send it off.

Comment 44

[Andrew Swan [:aswan]]

I tried comparing the windows sessionrestore profiles and got myself thoroughly confused.  I started with the script here:
https://github.com/devtools-html/perf.html/issues/470#issuecomment-404571370

and tried to hack it up a bit to work for this test (since the script relies on user timing events which the talos sessionrestore test doesn't generate).  I tried creating a diff of the profiler samples between the sessionRestoreInit and sessionRestored events but then I noticed that in the profiles those events are ~600 ms apart but the talos figures for this test are ~300 ms.  

And sure enough, talos uses some logic that I don't really understand to compute the time:
https://searchfox.org/mozilla-central/source/testing/talos/talos/startup_test/sessionrestore/addon/bootstrap.js#87-90

From a quick glance, StartupPerformance.latestRestoredTimestamp is derived from SSTabRestored events, which aren't visible in the profiles.  Argh.

Mike, you have any advice to offer?  The short background story is that there's a (pretty small) sessionrestore regression with webcompat-reporter running as a webextension (see comment 35).  Tom gathered some profiles (comment 39) and I wanted to compare them to identify the cause of the regression, which led to the confusion described at the start of this commment.

Comment 45

[Thomas Wisniewski [:twisniewski]]

aswan has spent the day looking at Talos reports related to this, and has decided that it seems likely that the only regressions of note are related to bug 1498027. As he told me on IRC, the regressions are very small and should go away as that bug is fixed, so he's okay with us landing this patch.

Comment 46

[Thomas Wisniewski [:twisniewski]]

And for posterity, here's a link to a Talos comparison for the regression being investigated: https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&newProject=try&newRevision=cac770e62f9263675e2f5676207a6e28838be6ff&framework=1&filter=sessionrestore&selectedTimeRange=172800

Comment 47

[Thomas Wisniewski [:twisniewski]]

Alright, since there doesn't seem to be any blocking issues left, I just did a trivial rebase and one last try-run which confirms that things are still green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=16f7b0a775607933527db6314e33f6b039b9c124

Requesting check-in.

Comment 48

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/90b00e89e83e
convert webcompat-reporter to a webextension; r=aswan,Pike

Comment 49

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/90b00e89e83e