Closed Bug 1587544 Opened 2 years ago Closed 1 year ago

Migrate screenshots to fluent

Categories

(Firefox :: Screenshots, defect)

Product:
Firefox
Component:
Screenshots
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 74
Tracking Flags:
Tracking Status
firefox74 --- fixed

People

(Reporter: aswan, Assigned: aswan, NeedInfo)

References

(Blocks 2 open bugs)

Details

(Keywords: perf-alert, Whiteboard: [fxperf:p2])

Attachments

(2 files)

Bug 1587544 Part 2: Migrate screenshots to fluent
47 bytes, text/x-phabricator-request
Details | Review
Bug 1587544 Part 1: Remove screenshots onboarding
47 bytes, text/x-phabricator-request
Details | Review

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?

Flags: needinfo?(francesco.lodolo)

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/

Flags: needinfo?(francesco.lodolo)

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?

Flags: needinfo?(jhirsch)

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

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.

Flags: needinfo?(jhirsch) → needinfo?(ianb)

(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

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.

Depends on: 1457865
Summary: Unused strings in screenshots → Migrate screenshots to fluent

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

Flags: needinfo?(ianb)

(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!

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

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);
Whiteboard: [fxperf]

(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?

Flags: needinfo?(andrew.swan)
Whiteboard: [fxperf] → [fxperf:p2]

Hello! And sure, I can take this.

Assignee: nobody → andrew.swan
Flags: needinfo?(andrew.swan)

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

For more information, please visit auto_nag documentation.

Flags: needinfo?(ianb)
Attachment #9102617 - Attachment description: Bug 1587544 screenshots fluent migration wip → Bug 1587544 Migrate screenshots to fluent
Blocks: 1596667

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?

Flags: needinfo?(jhirsch)
Flags: needinfo?(dtownsend)

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?

Flags: needinfo?(jhirsch)

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

(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?

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

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

Flags: needinfo?(dtownsend)

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.

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

Any update on the status of this bug? Is it still blocked?

(In reply to Francesco Lodolo [:flod] (PTO Dec 20-29) from comment #22)

Any update on the status of this bug? Is it still blocked?

Its my fault, sorry.. I think it is unblocked and I started revising my patches to remove onboarding and haven't found time to finish it off. I'll try to get to it soon.

Screenshots is now part of the browser first-run experience so get rid of
the onboarding slides embedded in screenshots. This means fewer strings
that need to be ported to fluent.

Attachment #9102617 - Attachment description: Bug 1587544 Migrate screenshots to fluent → Bug 1587544 Part 2: Migrate screenshots to fluent

Just to provide some visibiility here, I've just given an R+ (with a few changes) to the first patch.

I still have to look at the other patch, which I'll do later today or tomorrow.

We should be sure to ping Cosmin for a manual test pass when the patches land.

Pushed by shindli@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/483a2f094c2b
Part 2: Migrate screenshots to fluent r=fluent-reviewers,_6a68,flod
Pushed by shindli@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/919eb56fa574
Part 1: Remove screenshots onboarding r=_6a68
Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/dbf4426797ee
Part 1: Remove screenshots onboarding r=_6a68
https://hg.mozilla.org/integration/autoland/rev/89dd3abfbb2a
Part 2: Migrate screenshots to fluent r=fluent-reviewers,_6a68,flod,Gijs
Flags: needinfo?(andrew.swan)

https://hg.mozilla.org/mozilla-central/rev/dbf4426797ee
https://hg.mozilla.org/mozilla-central/rev/89dd3abfbb2a

Status: NEW → RESOLVED
Closed: 1 year ago
status-firefox74: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 74
Regressions: 1611715

Hi Andrew, I am working on migration of Fluent to Rust and as part of it am debugging performance route of the startup path.

I noticed that the request for screenshots-context-menu is the very first request to Fluent before the main browser.xhtml load kicks in.
The other things is that you seem to use the document's context for it: https://searchfox.org/mozilla-central/source/browser/extensions/screenshots/background/startBackground.js#16 which in case of browser.xhtml is initially synchronous, and performs a sync load.

But really, you could use a standalone:

let loc = new Localization(["browser/screenshots.ftl", "browser/branding/brandings.ftl"]);

this.getStrings = loc.formatValues(ids}; // this is async

I don't think it has any dire consequences, but I wanted to flag you here in case you think we may be able to make it lazy and async.

Flags: needinfo?(andrew.swan)

Caveat: I don't know the screenshots code very well, I ported it from the webextensions i18n api to fluent, but I'm no expert in how it all works.

I noticed that the request for screenshots-context-menu is the very first request to Fluent before the main browser.xhtml load kicks in.
The other things is that you seem to use the document's context for it: https://searchfox.org/mozilla-central/source/browser/extensions/screenshots/background/startBackground.js#16 which in case of browser.xhtml is initially synchronous, and performs a sync load.

That looks to be using the extension's background page, what's the connection between that page and browser.xhtml?

In any case, if there's some improvement to be made here, filing a new bug (and tagging it fxperf if it looks like there's an opportunity to improve startup performance) would be helpful.

Flags: needinfo?(andrew.swan)
You need to log in before you can comment on or make changes to this bug.