1618465 - Move pdf.js to toolkit

Status: RESOLVED FIXED

(Firefox :: PDF Viewer, task, P5)

Description

[Magnus Melin [:mkmelin]]

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?

Comment 1

[Brendan Dahl [:bdahl]]

I talked briefly with mossop, this sounds reasonable.

Comment 2

[Mark Banner (:standard8)]

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

(lots of orange due to issues on central)

Comment 3

[Mark Banner (:standard8)]

Attachment: Bug 1618465 - Move pdf.js to toolkit (main files). r?bdahl!

Comment 4

[Mark Banner (:standard8)]

Attachment: Bug 1618465 - Update references to pdf.js for the move to toolkit. r?bdahl!

Depends on D72532

Comment 5

[Axel Hecht [:Pike]]

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.

Comment 6

[Magnus Melin [:mkmelin]]

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

Comment 7

[Mark Banner (:standard8)]

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.

Comment 8

[Magnus Melin [:mkmelin]]

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.

Comment 9

[Brendan Dahl [:bdahl]]

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?

Comment 10

[Mark Banner (: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).

Comment 11

[Justin Dolske [:Dolske]]

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

Comment 12

[Mark Banner (:standard8)]

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?

Comment 13

[Axel Hecht [:Pike]]

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.

Comment 14

[Magnus Melin [:mkmelin]]

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

Comment 15

[Mark Banner (:standard8)]

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.

Comment 16

[Mark Banner (:standard8)]

Attachment: Bug 1618465, build and package pdfviewer l10n files from toolkit

Comment 17

[Mark Banner (:standard8)]

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

Comment 18

[Mark Banner (:standard8)]

(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

Comment 19

[Pulsebot]

Pushed by [e-mail]:
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

Comment 20

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/486e04aaf02d
https://hg.mozilla.org/mozilla-central/rev/4c13c9a66491
https://hg.mozilla.org/mozilla-central/rev/7563d04fc110