Closed Bug 810815 Opened 12 years ago Closed 4 years ago

Integrate pdf.js to Thunderbird

Categories

(Thunderbird :: General, defect)

defect
Not set
normal

Tracking

(thunderbird_esr78 wontfix)

RESOLVED FIXED
81 Branch
Tracking Status
thunderbird_esr78 --- wontfix

People

(Reporter: smaug, Assigned: standard8)

References

(Depends on 2 open bugs, Blocks 2 open bugs, )

Details

Attachments

(1 file, 8 obsolete files)

47 bytes, text/x-phabricator-request
Details | Review
It would be very nice to be able read pdf files inline.
Whiteboard: dupme
I spent one our two nights trying to make that work out. I somehow managed to make double-clicking on an attachment open up a pdf.js content tab, but then pdf.js was unable to download the attachment because it's an imap:// URL and there's some hackery involved. Conversations bundle a pdf.js-based viewer, which does have the hackery, but conversely, does not have the full-fledged PDF.js UI, unfortunately.

All in all, I don't think it's that much work required, but it probably requires some pdf.js knowledge which I don't have. And also, it was quite some time ago :)
Is pdf.js even ready? It seems it was postponed from some FF versions. But I see it is enabled now in Aurora 18.
(In reply to :aceman from comment #2)
> Is pdf.js even ready? It seems it was postponed from some FF versions. But I
> see it is enabled now in Aurora 18.

It's stable enough to handle most of the PDFs I have to read (admittedly, mostly just academic papers, so not a strong indicator of general reliability).
IIRC pdf.js wasn't enabled in < FF18 because Gecko lacked some printing related features which
pdf.js needs.
From what I recall, pdf.js runs as a content script, but in the context of Thunderbird, you need to run with chrome privileges to download the contents of an attachment (which does indeed make a lot of sense!). So what is needed is to add an extra wrapper that fetches the contents of the attachment, and then passes it to pdf.js, for instance through a string in the URL, bypassing the default method for downloading a PDF (which is based on an XMLHttpRequest I think).
Do you have a patch for that WIP experiment?
Blocks: 977822
CC some pdf.js developers

(In reply to Magnus Melin from comment #6)
> Do you have a patch for that WIP experiment?
Ping Protz
Flags: needinfo?(jonathan.protzenko)
The code is in Conversations: https://github.com/protz/GMail-Conversation-View/tree/master/content/pdfviewer

There's an outer, chrome XUL window here: https://github.com/protz/GMail-Conversation-View/blob/master/content/pdfviewer/wrapper.js

It wraps the PDF contents as strings, which it then passes into the inner, HTML content window by calling its init function: https://github.com/protz/GMail-Conversation-View/blob/master/content/pdfviewer/viewer.js#L131

The inner HTML window then does the pdf.js incantations that allow the PDF to be rendered.

I completely gave up on adapting the addon "as-is"; I don't think I got very far anyway, and the code changed so much back then that it was pretty difficult to keep up with it. What I have now is a suboptimal viewer bundled with Conversations, but well, that does the job for now!
Flags: needinfo?(jonathan.protzenko)

This adds a copy of pdfjs into mail/extensions. The copy is taken from the existing version in m-c's browser/extensions.

It also adds the two pdf files.

This patch is enough that loading pdf files in web / special tabs works correctly. For example, from a WebExtension I can do browser.tabs.create({ "url": "http://.../mypdf.pdf" }); and it loads correctly.

I think preferences may be slightly broken here. Need to figure out how to add the "Preview in Thunderbird" option that's the same as the Firefox one (I'm currently looking at that), and at how the defaults are set for that option.

For opening a pdf from a message, I have briefly tried it and hit what may have been the same issue as Jonathan had previously. Not quite sure as I don't have the full details.

We could try doing that part here, or in bug 766901 - depends if it is worth trying to land the web page parts of the functionality first (e.g. maybe for WebExtensions to start using?).

Blocks: 766901

Instead of copying the files couldn't we directly reference them from browser in the jar.mn? Then we don't need to copy them every time they are updated in browser?

That's not how we do it, we only include directly if in toolkit. Maybe pdfjs should be in toolkit?

But it wouldn't be the first: https://searchfox.org/comm-central/search?q=..%2F..%2F..%2Fbrowser&case=false&regexp=false&path=mail and we've got automatically the newest version.

It seems that either will probably work. I'm not sure if m-c would move pdfjs into toolkit as I'm guessing it isn't really relevant to mobile.

I've changed the main patch to reference browser/ directly. I have no idea if it is possible with locale files, so I've just copied those across. If someone wants to play around with that, feel free to, but I'm not planning on it.

Assignee: nobody → standard8
Attachment #9119571 - Attachment is obsolete: true
Attachment #9119574 - Attachment is obsolete: true
Attachment #9121413 - Flags: review?(mkmelin+mozilla)

This hooks up pdfjs and gets the menu option working in preferences, but disables it by default. If you try to enable it and open a pdf from an email, then you get an error (as described previously).

In a WebExtension, I've been doing this to test it:

browser.tabs.create({
  url: "http://www.orimi.com/pdf-test.pdf",
});

(the url is just a random test pdf I found).

The code for the preferences is largely copied from the browser, adapted to non-fluent. I did consider porting some of the fluent work, but that's too much for what I want to do.

I also looked at porting browser/components/preferences/in-content/tests/browser_applications_selection.js, however again the lack of fluent made this difficult.

I'm planning to look into the error you get when opening a pdf from an email, but thought it is worth starting the review process, and possibly landing these so that extensions can start to at least use parts of it.

Attachment #9121415 - Flags: review?(mkmelin+mozilla)
Depends on: 1618465

I opened bug 1618465 about moving it to toolkit. I don't think we should reference browser/ directly, though we might need to reconsider in the future.

Comment on attachment 9121415 [details] [diff] [review]
Bug 810815 - Hook up pdfjs files so that web tabs can be opened with pdfs (disabled by default).

Review of attachment 9121415 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the delay. This looks alright except for how it's referencing browser

::: mail/components/MailGlue.jsm
@@ +19,5 @@
> +  MailMigrator: "resource:///modules/MailMigrator.jsm",
> +  PdfJs: "resource://pdf.js/PdfJs.jsm",
> +  RemoteSecuritySettings:
> +    "resource://gre/modules/psm/RemoteSecuritySettings.jsm",
> +  Services: "resource://gre/modules/Services.jsm",

I think we want to keep services imported normally, and looks like most of the others too will basically always be needed.
Attachment #9121415 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9121413 [details] [diff] [review]
Bug 810815 - Add pdfjs reader locale files to Thunderbird.

Shouldn't be needed if it's in toolkit.
Attachment #9121413 - Flags: review?(mkmelin+mozilla)
Status: NEW → ASSIGNED
OS: Linux → All
Hardware: x86 → All

I eventually worked out today why this wasn't fully working. The pdfjs loads itself, but pretends it is actually loading the url of the document. Since the document is based on the url of the message, we disable javascript on the docshell.

Ideally we shouldn't be doing that for this case, though I haven't yet found a way around it. Firefox has this issue when javascript is disabled globally as well (bug 866634), so it could be the fix needs to be in core somewhere.

I've been asking around in #dom on Matrix, hopefully might be able to get some help in getting that bit to work.

Depends on: 866634

This should work with the two patches in bug 866634 and bug 1631305.

Attached patch 810815.txt (obsolete) — Splinter Review

This is my current WIP. Still needs some work and clean up.

Attachment #9121415 - Attachment is obsolete: true

Thank you Emilio, it is working with those two patches applied :-)

I'll see if I can sort out the move to toolkit and get the patch updated over the next few days.

Attachment #9121413 - Attachment is obsolete: true

With the patches in core that have landed, and the additional patch on bug 1618465, this now gets everything working for opening pdf attachments within Thunderbird using pdfjs.

Attachment #9141609 - Attachment is obsolete: true
Attachment #9143404 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9143404 [details] [diff] [review]
Bug 810815 - Enable opening pdf attachments within Thunderbird.

Review of attachment 9143404 [details] [diff] [review]:
-----------------------------------------------------------------

Looks reasonable, but something's broken so I can't test the actual feature, see below.

::: mail/app/profile/all-thunderbird.js
@@ +1247,5 @@
>  #endif
>  
>  pref("prompts.defaultModalType", 3);
> +
> +// Completely disable pdf.js as an option to preview pdfs within firefox.

Thunderbird

@@ +1251,5 @@
> +// Completely disable pdf.js as an option to preview pdfs within firefox.
> +// Note: if this is not disabled it does not necessarily mean pdf.js is the pdf
> +// handler just that it is an option.
> +pref("pdfjs.disabled", false);
> +// Used by pdf.js to know the first time firefox is run with it installed so it

Thunderbird

::: mail/components/MailGlue.jsm
@@ +31,5 @@
>  );
>  var { RemoteSecuritySettings } = ChromeUtils.import(
>    "resource://gre/modules/psm/RemoteSecuritySettings.jsm"
>  );
> +var { PdfJs } = ChromeUtils.import("resource://pdf.js/PdfJs.jsm");

this throws an NS_ERROR_NOT_AVAILABLE

Which makes lots of other things not work. I get a tb with missing Get messages button and opening the compose window doesn't work.

::: mailnews/base/util/nsMsgProtocol.cpp
@@ +51,1 @@
>                    nsIRequestObserver, nsIRequest, nsITransportEventSink)

Looks like this needs a clang-format adjustment
Attachment #9143404 - Flags: review?(mkmelin+mozilla) → review-
Comment on attachment 9143404 [details] [diff] [review]
Bug 810815 - Enable opening pdf attachments within Thunderbird.

Review of attachment 9143404 [details] [diff] [review]:
-----------------------------------------------------------------

Oh I see, bug 1618465 hasn't landed yet
Attachment #9143404 - Flags: review- → review?(mkmelin+mozilla)
Comment on attachment 9143404 [details] [diff] [review]
Bug 810815 - Enable opening pdf attachments within Thunderbird.

Clearing request until bug 1618465 lands
Attachment #9143404 - Flags: review?(mkmelin+mozilla)

Updated patch now that the move has landed. I've not run this through try server, but it seems to work fine.

Attachment #9143404 - Attachment is obsolete: true
Attachment #9161019 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9161019 [details] [diff] [review]
Bug 810815 - Enable opening pdf attachments within Thunderbird.

Review of attachment 9161019 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks for pulling it through! r=mkmelin
I've fixed the below nits and sent it off to try: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=8db22b78a08d76fce855f3a49feb9a17d230eee7

::: mail/app/profile/all-thunderbird.js
@@ +1258,5 @@
>  
>  // The URL for the privacy policy related to recommended extensions.
>  pref("extensions.recommendations.privacyPolicyUrl", "https://www.mozilla.org/en-US/privacy/thunderbird/#addons");
> +
> +// Completely disable pdf.js as an option to preview pdfs within firefox.

Thunderbird.

@@ +1262,5 @@
> +// Completely disable pdf.js as an option to preview pdfs within firefox.
> +// Note: if this is not disabled it does not necessarily mean pdf.js is the pdf
> +// handler just that it is an option.
> +pref("pdfjs.disabled", false);
> +// Used by pdf.js to know the first time firefox is run with it installed so it

Thunderbird.
Attachment #9161019 - Flags: review?(mkmelin+mozilla) → review+

The try shows bustage detected by static analysis.

Adding this should show it locally too:
ac_add_options --enable-clang-plugin

Updated patch, fixed the refcnt issues (the NS_DECL_* macros needed adjusting).

Attachment #9161019 - Attachment is obsolete: true

I can land this, though tree is closed at the moment, so flagging for landing when it re-opens (unless I get there first!).

Attachment #9169066 - Attachment is patch: true
Whiteboard: dupme
Attachment #9169066 - Attachment is obsolete: true

Comment on attachment 9169141 [details]
Bug 810815 - Enable opening pdf attachments within Thunderbird. r=mkmelin!

Revision D86538 was moved to bug 810814. Setting attachment 9169141 [details] to obsolete.

Attachment #9169141 - Attachment is obsolete: true

patch had the wrong bug number. Patch is in bug 810814.

Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 81 Branch
Attachment #9169141 - Attachment is obsolete: false
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/comm-central/rev/f5690ed26dbe
Add a dummy gBrowser._insertBrowser test function to let mochitests pass. r=bustage-fix
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/comm-central/rev/f8922777d979
Test fix for browser_policy_passwordmanager.js and for debug tests. r=bustage-fix
Blocks: 1660603
Regressions: 1690467
Regressions: 1663904
Depends on: 1663427
Depends on: 1724627
Depends on: 1726134
Depends on: 1726136
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: