Closed Bug 1451485 Opened 2 years ago Closed 11 months ago

Convert webcompat-reporter to a webextension

Categories

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

enhancement

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: aswan, Assigned: wisniewskit)

References

Details

Attachments

(1 file, 1 obsolete file)

No description provided.
We track webcompat-reporter bugs in Web Compat Tools::General, moving there. :)
Component: Go Faster → General
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).
Flags: needinfo?(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.
Flags: needinfo?(aswan)
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).
Flags: needinfo?(miket)
correction - Nightly code removal starts OCTOBER 22 (not Sept), sorry.
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.
Assignee: nobody → wisniewskit
Flags: needinfo?(miket)
Priority: P2 → P1
Attached patch wip.patch (obsolete) — Splinter Review
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 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.
Attachment #9010051 - Attachment is obsolete: true
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?
Flags: needinfo?(florian)
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.
Flags: needinfo?(florian)
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
(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.
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
convert webcompat-reporter to a webextension
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.
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.
Flags: needinfo?(l10n)
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.
Flags: needinfo?(l10n)
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.
(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 on attachment 9011293 [details]
Bug 1451485 - convert webcompat-reporter to a webextension; r?aswan

Andrew Swan [:aswan] has approved the revision.
Attachment #9011293 - Flags: review+
(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)?
Flags: needinfo?(francesco.lodolo)
Yes, we're basically blocked by supporting Fluent in WebExtension (comment 16 and 18, bug 1425104).
Flags: needinfo?(francesco.lodolo)
flod, how are formautofill and screenshots working around this (assuming they are?)
Flags: needinfo?(francesco.lodolo)
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).
Flags: needinfo?(francesco.lodolo)
Thanks, it looks like Bug 1425104 is getting pretty close to done. We should still be able to hit our 64 target.
Depends on: 1425104
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?
Is this a question for Flod? Or Tom?
Flags: needinfo?(aswan)
For Tom.  He and I just chatted on IRC and I think we have a plan...
Flags: needinfo?(aswan)
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.
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?
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.
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)
No longer depends on: 1425104
See Also: → 1425104
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.
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).
Flags: needinfo?(aswan)
No longer depends on: 1483172
: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).
Status: NEW → RESOLVED
Closed: Last year
Flags: needinfo?(l10n)
Resolution: --- → FIXED
Um, not sure how I resolved this as fixed... re-opening.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
The try push helps me, too. Commented in phabricator.
Flags: needinfo?(l10n)
Thanks! Here's hoping the try-runs give us some valuable insight.
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.
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.
Flags: needinfo?(aswan) → needinfo?(mconley)
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.
Flags: needinfo?(mconley)
See Also: → 1498027
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.
Keywords: checkin-needed
Pushed by rvandermeulen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/90b00e89e83e
convert webcompat-reporter to a webextension; r=aswan,Pike
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/90b00e89e83e
Status: REOPENED → RESOLVED
Closed: Last year11 months ago
Resolution: --- → FIXED
Depends on: 1498579
Depends on: 1498808
Duplicate of this bug: 1453493
Depends on: 1499062
No longer blocks: 1494282
You need to log in before you can comment on or make changes to this bug.