Closed Bug 1618465 Opened 9 months ago Closed 5 months ago

Move pdf.js to toolkit

Categories

(Firefox :: PDF Viewer, task, P5)

task

Tracking

()

RESOLVED FIXED
Firefox 80
Tracking Status
firefox80 --- fixed

People

(Reporter: mkmelin, Assigned: standard8)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

pdf.js is currently in browser/extensions/pdfjs

Thunderbird would also like to utilize pdf.js functionality (bug 810815). This bug is to consider moving it to toolkit/extensions/pdfjs. I think it would be relevant also for bug 938235.

Is this acceptable?

I talked briefly with mossop, this sounds reasonable.

Priority: -- → P5
Assignee: nobody → standard8
Status: NEW → ASSIGNED

From a cross-channel perspective, this is an hg copy. So, after this patch, we'll have two copies of these strings around, until we ditch 68 support in gecko-strings, and whatever normal release channel precedes this landing.
There's also https://github.com/mozilla/pdf.js/issues/10075, to convert pdf.js to Fluent. Which would add a third variant, depending on timing.

So my preferred way would be to avoid one of those copies and go straight from whatchamacallit-browser [0] to fluent-toolkit.

Having one way to localize pdf.js might help making changes to it, so asking dolske.

Magnus, I take it that you're interested in 78?

[0] pdf.js has some string bundle stuff, and apparently two implementations of webL10n, according to the linked issue.

Flags: needinfo?(dolske)

Yes, 78 would be the main target. Seems only two properties files to convert, so not too much if it was a normal conversion.

From my perspective, I've been moving attachment pdf viewing from Thunderbird Conversations into Thunderbird itself. This is a win from both sides as far as I'm concerned: Thunderbird gets nicer attachment viewing, Conversations doesn't need to try and support it (which is getting really hard to do in the WebExtension world). This is why I'd really like to get it into 78.

Unfortunately at this time, I don't think I could dedicate the extra time helping pdfjs move to fluent.

Someone from the Thunderbird team could handle the Fluent conversion. But I think the webL10n part Pike mentions in comment 5 needs someone to figure out first.

Blocks: 938235

Mark, I was hoping we could get this bug in first, but it looks like it will be hung up on l10n stuff. In the meantime, are you fine with me/RyanVM updating pdf.js in tree?

Flags: needinfo?(standard8)

(In reply to Brendan Dahl [:bdahl] from comment #9)

Mark, I was hoping we could get this bug in first, but it looks like it will be hung up on l10n stuff. In the meantime, are you fine with me/RyanVM updating pdf.js in tree?

That's fine, I don't think it is actually going to bitrot me, but if it does I can easily rebase (though remember we're in soft freeze now).

Flags: needinfo?(standard8)
No longer blocks: 1632623

I'm not super clear on what the question is, but AIUI the concern is that moving to toolkit triggers relocalization (as will the Fluent conversion), and so the ask is to only take 1 localization hit? I would support that to minimize costs to our l10n community.

I suppose you could do the move now but leave the .properties in /browser until someone does the Fluent conversion... That would temporarily sidestep the l10n concern but I'm not sure it's even useful for TB (obviously builds can't be done that way, but maybe if the rebase is painful?).

Flags: needinfo?(dolske)

The rebase of the patches is easy to do. Unfortunately we're now in soft freeze, so it is a bit late to do anything like this.

I fear we're going to have another year of nothing happening on the fluent side, and end up in the same position for next ESR. Axel, do your original objections go away once support for 68 ends, or do they start again because we'll then have 78 branched out?

Flags: needinfo?(l10n)

Yeah, the overlap in ESRs makes this a perpetual concern.

I've put up what's probably what dolske had in mind about build changes on https://treeherder.mozilla.org/#/jobs?repo=try&revision=f8386cf3f968e0de193a0ecec865ac878814dd35. AFAICT, I ran the tests for pdfjs, and I hope they'd break if I broke pdfjs ;-)

I've looked at langpacks for both firefox and thunderbird, and they work. I also did a cross check on cross-channel generation if that weird double-config is throwing it off, but it doesn't seem to.

Does that help?

PS: I'm not so much concerned about retranslation, I expect us (aka flod) to do that for them, via copies or migrations. What concerns me is that people that try to fix issues in their translation have no idea which file to fix, and where. And that localizations coming new to Firefox need to translate the content twice.

Flags: needinfo?(l10n)

What should we do here? Open a separate bug to move pdf.js to Fluent? And/or go with the change from comment 13?

I'm happy to merge in Axel's change from comment 13, I think that'll work, and the fluent change can happen later. I hope to find a bit of time this weekend for that.

(In reply to Mark Banner (:standard8) from comment #17)

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=590e2d39c69ce7ccdea88f38e98af7a56d42609a

That try build was bad because it was an artifact build, and the static module definitions are built into c++, so this full try build should work better:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6419b1caa51c22c66e8562f8c47ff6e59f191bdd

Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/486e04aaf02d
build and package pdfviewer l10n files from toolkit r=Pike
https://hg.mozilla.org/integration/autoland/rev/4c13c9a66491
Move pdf.js to toolkit (main files). r=bdahl,Pike
https://hg.mozilla.org/integration/autoland/rev/7563d04fc110
Update references to pdf.js for the move to toolkit. r=bdahl
Status: ASSIGNED → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 80
Regressions: 1651807
You need to log in before you can comment on or make changes to this bug.