1639069 - Add a context menu item in the Downloads Panel to "Open in system viewer" and "Always open in system viewer"

Status: VERIFIED FIXED

(Firefox :: Downloads Panel, defect, P2)

Description

[Sam Foster [:sfoster] (he/him)]

Each of the downloads views provides a context menu for the download list items to "Open Containing Folder" etc.
This menu should include options to "Open in system viewer" and "Always open in system viewer", when the download is PDF.

Comment 1

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 1639069 - Strings for new download context menu items. r?Jaws

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ed262a70a438
Strings for new download context menu items. r=jaws,fluent-reviewers

Comment 3

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/ed262a70a438

Comment 4

[Sam Foster [:sfoster] (he/him)]

There's more work to do here, I just forget to add leave-open when I landed that initial strings patch.

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

[Tracking Requested - why for this release]: Making it easier to access PDF.js will be a large part of Firefox 78 release and this bug tracks some of the work that just missed landing before the uplift from Nightly -> Beta.

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Bug 1639069 - WIP of adding context menu items.

Comment 7

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 1639069 - Add download context menu items to 'Use' and 'Always use' the system viewer to open the download. r?jaws

Depends on D79395

Comment 8

[Julien Cristau [:jcristau]]

Is this still targeting 78? We're now in the last week of beta.

Comment 9

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 1639069 - Provide helpers for getting a nsIMIMEInfo on DownloadsCommon, and confirming if a download is a given mime-type. r?jaws

Comment 10

[Sam Foster [:sfoster] (he/him)]

I noticed that currently (with/without these patches) if you download a PDF with contentType: "application/octet-stream" and a filename of something.pdf, the mimeInfo we get from mimeInfo = gMIMEService.getFromTypeAndExtension in DownloadIntegration#launchDownload will be for 'application/octet-stream', not 'application/pdf'. This means that mimeInfo.alwaysAskBeforeHandling will be true, so in the logic that determines if we should call DownloadUIHelper.loadFileIn for this file, the answer is always "no."

This also means that the context menu items we add here appear not to work with this kind of download; toggling "Always use system.." has no effect as despite resetting the preferredAction in the command, we're retrieving the wrong nsIMimeInfo and alwaysAskBeforeHandling value. I think what we want to happen in launchDownload is to follow the same logic now implemented in DownloadsCommon.getMimeInfo. Maybe that could move to Download (DownloadCore.jsm) where we can reuse it from DownloadIntegration.jsm

I think we should take stock and make a new plan as we are down to only a couple of days to land and uplift this.

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

Thanks for digging in to that. At this point I don't think we should uplift, we will want some extra time for this to bake and fix any regressions or missing support. It's okay if this isn't in 78. It would be great if we can do it "right" in 79.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Are we eventually going to want to do something for ESR78?

Comment 13

[Sam Foster [:sfoster] (he/him)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #12)

Are we eventually going to want to do something for ESR78?

Ideally yes, perhaps if there is a 78.1? We'll want to at least land and verify in nightly first.

Comment 14

[Sam Foster [:sfoster] (he/him)]

Status: I'm tracking down an issue where the context menu on PDF downloads in the all-downloads view alternates between having the system viewer menu items and not. This showed up as a test failure but is reproducable manually.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b06131994e4a
Provide helpers for getting a nsIMIMEInfo on DownloadsCommon, and confirming if a download is a given mime-type. r=jaws
https://hg.mozilla.org/integration/autoland/rev/d37373cfa435
Add download context menu items to 'Use' and 'Always use' the system viewer to open the download. r=jaws

Comment 16

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/b06131994e4a
https://hg.mozilla.org/mozilla-central/rev/d37373cfa435

Comment 17

[Catalin Sasca, Desktop Test Engineering [:csasca]]

Verified on Firefox 79.0b3 and Firefox 80.0a1 (2020-07-03) that these two new options are successfully implemented and they work as expected.

Tests were performed on macOS 10.15.5, Ubuntu 20.04 and Windows 10, using a pdf sample on google.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Is this ready for an ESR78 approval request?

Comment 19

[Sam Foster [:sfoster] (he/him)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #18)

Is this ready for an ESR78 approval request?

I'm still weighing the risks vs. benefits. This hasn't had long to bake for a feature that is not front-and-center for most people, so seeing no follow-up issues doesn't necessarily mean there are none. However, it has been verified by QA in 79 and 80, so its probably fair to say it is low (but not zero) risk.
The flip side is what is the risk of shipping ESR 78 without it? The idea with this feature is to provide choices and mitigation for when the changes in how we handle opening downloaded PDFs by default clash with users' workflows. So there is some potential risk in shipping one without the other.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Should we consider disabling the feature on ESR78 instead?

Comment 21

[Sam Foster [:sfoster] (he/him)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #20)

Should we consider disabling the feature on ESR78 instead?

I think the consensus is that ESR78 is fine without this patch at this point.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

OK, thanks.