1587544 - Migrate screenshots to fluent

Status: NEW

(Firefox :: Screenshots, defect)

Description

[Andrew Swan [:aswan]]

It looks like the messages.json files included with screenshots include some strings that aren't actually used by the extension.
The unused strings have the following ids:

downloadOnlyNotice
downloadOnlyDetails
downloadOnlyDetailsPrivate
downloadOnlyDetailsThirdParty
downloadOnlyDetailsNeverRemember
downloadOnlyDetailsESR
downloadOnlyDetailsNoUploadPref
imageCropPopupWarning

:flod, assuming that removing these to save time and effort for translators is useful, is just removing them from m-c enough or does something else need to happen?

Comment 1

[Francesco Lodolo [:flod] (limited availability until Dec 11)]

That won't be enough. What's in mozilla-central is a dump (e.g. bug 1521168.) of external content. BTW, that should be updated, since it hasn't received updates in a long time.

Code is here, not sure who's active there these days
https://github.com/mozilla-services/screenshots/

Comment 2

[Andrew Swan [:aswan]]

I'm pretty sure nobody is actively working on that github repo these days.
I stumbled across this while looking at switching screenshots over to use Fluent, with strings coming from langpacks instead of being bundled with the extension.
Maybe a better strategy is just to make that migration, dropping the unused strings along the way. Then l10n updates can happen the same way they happen for other parts of Firefox?
(I realize that there will be a different set of issues with switching over to Fluent, I'll open a separate bug and put up the patches I have in progress, but just looking for consensus on the basic plan here.)
Jared, does the above sound reasonable?

Comment 3

[Andrew Swan [:aswan]]

Jared, one more question: it looks like initLibraryButton() in the experimental api inside the screenshots extension is unused? Can it just be removed?

Comment 4

[Jared Hirsch [:_6a68] [:jhirsch] (Needinfo please)]

Ian's been doing the planning for final server decommission, including removing server-related dead code from the webextension. I don't know if there's a plan to migrate strings from pontoon to the system used by desktop. Passing the ni over to Ian, who may have answers.

Comment 5

[Francesco Lodolo [:flod] (limited availability until Dec 11)]

(In reply to Andrew Swan [:aswan] from comment #2)

Maybe a better strategy is just to make that migration, dropping the unused strings along the way. Then l10n updates can happen the same way they happen for other parts of Firefox?

We would migrate the entire files from the external repository, but we need to clean up the en-US file beforehand.

There are plenty of languages not enabled in that repository, or incomplete ones, that will suddenly start translating useless strings the moment they're exposed as part of Firefox.

If we could migrate to Fluent directly, it would be even better. Otherwise, we'd need to :

1. Copy the .properties file into each l10n repository, and start using that content.
2. Migrate to Fluent in a second step, with the potential of languages translating twice the same content, if they weren't up to speed with translation on step 1

Comment 6

[Andrew Swan [:aswan]]

Okay, I have some patches locally that migrate screenshots directly to using fluent strings from language packs. There's just one snag, described in bug 1457865, but I can fix that if the extensions folks are okay with the proposed approach.
I'm going to re-purpose this bug for the fluent migration, I'll put my patches up here and we can continue the discussion once 1457865 gets fixed.

Comment 7

[Jared Hirsch [:_6a68] [:jhirsch] (Needinfo please)]

After thinking it over a bit, I feel bad about Screenshots becoming abandonware. :aswan, I can help with the migration.

Comment 8

[Andrew Swan [:aswan]]

(In reply to Jared Hirsch [:_6a68] [:jhirsch] (Needinfo please) from comment #7)

After thinking it over a bit, I feel bad about Screenshots becoming abandonware. :aswan, I can help with the migration.

Didn't mean to guilt you into doing unwanted work but thanks :)

I'll push my work-in-progress patches, some notes:

• This is obviously going to need to be coordinated with l10n folks, presumably :flod. In the patch, I've migrated the en-US strings to an ftl file under browser/locales/... I assume we would synchronize so the work described in comment 5 would happen around the same time this patch lands. Please let me know if anything needs to be done differently (or in addition) inside mozilla-central
• This depends on bug 1457865 but if you apply the patch on that bug in addition to this one, things mostly work
• For the most part, screenshots can now just use the dom l10n api and things generally get a lot simpler
• There's still a glitch with the selector ui that I haven't had a chance to dig into

Jared, I'm willing to finish this off (between the code freeze, the dependency on bug 1457865, synchronization with l10n, etc. it can't land any time soon) but if you want to take a look at the actual screenshots code changes and give feedback, that would be great, thanks!

Comment 9

[Andrew Swan [:aswan]]

Attachment: Bug 1587544 Migrate screenshots to fluent

This is not ready for merging or reviewing, just a work-in-progress snapshot.
See discussion on the bug.

Comment 10

[Andrew Swan [:aswan]]

For the performance team: I used the patch at the bottom of this comment for ad hoc measurement of the time we spend in checkForChanges(). Time in this function is negligible when starting in an existing profile, but we spend a lot of time in this function on the first startup in a new profile. I don't have access to a reference device but on a mid-range laptop (2017 MacBook Pro), switching screenshots to fluent drops the time in checkForChanges() from around 400ms to consistently under 300ms. From memory, we were spending something like 2-3 seconds in checkForChanges() on the reference hardware so some really hand-wavy math suggests this could cut somewhere from 0.5-1 second from first startup on the reference device. I think bug 1568270 could give us even more savings, but this patch takes care of some relatively low-hanging fruit.

--- a/toolkit/mozapps/extensions/internal/XPIProvider.jsm
+++ b/toolkit/mozapps/extensions/internal/XPIProvider.jsm
@@ -2381,7 +2381,12 @@ var XPIProvider = {
       Services.prefs.addObserver(PREF_ALLOW_LEGACY, this);
       Services.obs.addObserver(this, NOTIFICATION_FLUSH_PERMISSIONS);
 
-      this.checkForChanges(aAppChanged, aOldAppVersion, aOldPlatformVersion);
+      {
+        let start = Cu.now();
+        this.checkForChanges(aAppChanged, aOldAppVersion, aOldPlatformVersion);
+        let end = Cu.now();
+        dump(`CHECK took ${end-start}\n`);
+      }
 
       AddonManagerPrivate.markProviderSafe(this);

Comment 11

[Mike Conley (:mconley) (:⚙️) (Wayyyy behind on needinfos)]

(In reply to Andrew Swan [:aswan] from comment #10)

Hi aswan! :)

This looks amazing. I love it when we can kill two birds (improve startup perf, migrate to Fluent) with one stone. Thanks for working on this! Can I go ahead and assign this to you?

Comment 12

[Andrew Swan [:aswan]]

Hello! And sure, I can take this.

Comment 13

[Release mgmt bot [:sylvestre / :calixte / :marco for bugbug]]

The priority flag is not set for this bug.
:ianbicking, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 14

[Andrew Swan [:aswan]]

I've run into a snag with the dom l10n api. Screenshots does this thing where it loads an empty document, then separately parses a bunch of html with DOMParser and injects that into the empty document:
https://searchfox.org/mozilla-central/rev/131338e5017bc0283d86fb73844407b9a2155c98/browser/extensions/screenshots/onboarding/slides.js#20-69

I have no idea why screenshots works this way (I asked at https://bugzilla.mozilla.org/show_bug.cgi?id=1585588#c6 but never got an answer). In any case, rewriting it appears to me to be a big change that I'm not really equipped to do right now (and I would be reluctant to start without understanding why screenshots was written this way in the first place).

The html that gets parsed/injected is defined here:
https://searchfox.org/mozilla-central/rev/131338e5017bc0283d86fb73844407b9a2155c98/browser/extensions/screenshots/build/onboardingHtml.js
In the patch on phabricator, this html includes a <link rel="localization" ...> element that refers to screenshots.ftl. In response to review feedback, I tried to update this so that strings in screenshots.ftl reference terms defined in separate ftl files (branding/brand.ftl and browser/branding/branding.ftl) and then add additional <link> elements from onboardingHtml to load those files. This is where the bad interaction with the dom l10n api starts.

I'm a little shaky on exactly what's happening but it appears to me that when the parsed html is being adopted, Document::LocalizationLinkAdded() is called once for each <link> element. However, since the actual document is already loaded and we're just adopting new content, we end up here on the first <link>:
https://searchfox.org/mozilla-central/rev/131338e5017bc0283d86fb73844407b9a2155c98/dom/base/Document.cpp#3654-3659

On the second and third <link>s we take this branch:
https://searchfox.org/mozilla-central/rev/131338e5017bc0283d86fb73844407b9a2155c98/dom/base/Document.cpp#3650-3652

But note that the document translation takes place after the first <link>, so we don't have all the strings and terms available at that time.

I'm not sure what the right fix is here. It looks like there's some support in Document for batching up resource ids (mL10nResources, Document::OnL10nResourceContainerParsed()), but I don't know the platform bits well enough to know if its practical to invoke that path when adopting an entirely new <head>. Mossop, I think you were the original author of this api, any thoughts?

Also, Jared, can you shine any light on why screenshots works this way in the first place?

Comment 15

[Jared Hirsch [:_6a68] [:jhirsch] (Needinfo please)]

I'm pretty sure we're doing away with the onboarding slides, in favor of an introduction in the UI Tour (or whatever that tour is called now.) I'll double-check with jgruen.

Not that it matters too much at this point, but IIRC the onboarding slides were originally injected into the underlying third-party page, back when Page Shot was an addon. I don't think there was significant change other than injecting the slides into a special iframe instead, for security purposes. But I've been away from this long enough to forget a lot of context.

Assuming the onboarding slides are gone, are there any other major problems to address?

Comment 16

[Jared Hirsch [:_6a68] [:jhirsch] (Needinfo please)]

Yup, confirmed with jgruen in Slack that the onboarding slides should go away. Let me know if there are other issues to address, or if you need a hand (though my Q4 has gotten awfully busy).

Comment 17

[Andrew Swan [:aswan]]

(In reply to Jared Hirsch [:_6a68] [:jhirsch] (Needinfo please) from comment #16)

Yup, confirmed with jgruen in Slack that the onboarding slides should go away.

I think that will solve the immediate problem. Do you want a separate issue for handling that or should I just roll it into this bug?

Comment 18

[Andrew Swan [:aswan]]

(In reply to Jared Hirsch [:_6a68] [:jhirsch] (Needinfo please) from comment #15)

Not that it matters too much at this point, but IIRC the onboarding slides were originally injected into the underlying third-party page, back when Page Shot was an addon. I don't think there was significant change other than injecting the slides into a special iframe instead, for security purposes. But I've been away from this long enough to forget a lot of context.

Also, it sounds like maybe the answer has been lost to time, but I was wondering why not just create slides.html and load that in an iframe rather than the whole blank.html in the first link in comment 14. Its not obvious to me how the two are functionally different.

Comment 19

[Dave Townsend [:mossop] (he/him)]

(In reply to Andrew Swan [:aswan] from comment #14)

I've run into a snag with the dom l10n api. Screenshots does this thing where it loads an empty document, then separately parses a bunch of html with DOMParser and injects that into the empty document:
https://searchfox.org/mozilla-central/rev/131338e5017bc0283d86fb73844407b9a2155c98/browser/extensions/screenshots/onboarding/slides.js#20-69

I have no idea why screenshots works this way (I asked at https://bugzilla.mozilla.org/show_bug.cgi?id=1585588#c6 but never got an answer). In any case, rewriting it appears to me to be a big change that I'm not really equipped to do right now (and I would be reluctant to start without understanding why screenshots was written this way in the first place).

The html that gets parsed/injected is defined here:
https://searchfox.org/mozilla-central/rev/131338e5017bc0283d86fb73844407b9a2155c98/browser/extensions/screenshots/build/onboardingHtml.js
In the patch on phabricator, this html includes a <link rel="localization" ...> element that refers to screenshots.ftl. In response to review feedback, I tried to update this so that strings in screenshots.ftl reference terms defined in separate ftl files (branding/brand.ftl and browser/branding/branding.ftl) and then add additional <link> elements from onboardingHtml to load those files. This is where the bad interaction with the dom l10n api starts.

I'm a little shaky on exactly what's happening but it appears to me that when the parsed html is being adopted, Document::LocalizationLinkAdded() is called once for each <link> element. However, since the actual document is already loaded and we're just adopting new content, we end up here on the first <link>:
https://searchfox.org/mozilla-central/rev/131338e5017bc0283d86fb73844407b9a2155c98/dom/base/Document.cpp#3654-3659

On the second and third <link>s we take this branch:
https://searchfox.org/mozilla-central/rev/131338e5017bc0283d86fb73844407b9a2155c98/dom/base/Document.cpp#3650-3652

But note that the document translation takes place after the first <link>, so we don't have all the strings and terms available at that time.

I'm not sure what the right fix is here. It looks like there's some support in Document for batching up resource ids (mL10nResources, Document::OnL10nResourceContainerParsed()), but I don't know the platform bits well enough to know if its practical to invoke that path when adopting an entirely new <head>. Mossop, I think you were the original author of this api, any thoughts?

Sorry I dropped this. What I would expect to happen here is that we do the localization when the first link is inserted and then relocalize everytime a new link is added so by the end we should have a correctly localized document. But even if that works (and I don't think we test something like that so I can believe that it doesn't) that is horribly inefficient. I wonder if there is some flag we can use to detect that a batch insert is happening and delay until after, this is a bit past me though.

Comment 20

[Zibi Braniecki [:zbraniecki][:gandalf]]

This is the place where we inject a new link after the document has been initially translated: https://searchfox.org/mozilla-central/source/dom/base/Document.cpp#3692-3694

We could probably try to batch here and only submit the batch to Localization once per frame or sth? I'm not an expert in what's the best batching mechanism here to use.

Comment 21

[Andrew Swan [:aswan]]

I think we're all (Mossop, gandalf, and me anyway) on the same page about what's desirable but also not really knowing how to get there.
This is no longer a blocker for screenshots (comment 15 and comment 16) so I filed bug 1601509 so this can be tracked separately.
I don't think its a particularly high priority (at least until somebody suddenly needs it for something...)