1393128 - Custom tabs: make downloads work

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, enhancement)

Comment 1

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Bug 1393128 - 1. Make IntentHelper available globally;

Instead of initializing IntentHelper in GeckoApp, initialize it in
GeckoApplication, and make it available globally. This will let custom
tabs / PWA use IntentHelper-provided services.

Review commit: https://reviewboard.mozilla.org/r/171948/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/171948/

Comment 2

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Bug 1393128 - 2. Support showing Snackbars globally;

Right now we have two Snackbar handlers in GeckoApp and
GeckoPreferences, respectively. This patch combines the two handlers
into one inside GeckoApplication, and lets us display Snackbars in other
Activities like custom tabs and PWA.

Review commit: https://reviewboard.mozilla.org/r/171950/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/171950/

Comment 3

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Bug 1393128 - 3. Initialize DownloadNotifications outside of browser.js;

In order to show download notifications, we need to initialize the
DownloadNotifications module outside of browser.js. This patch moves
initialization to BrowserCLH.js, and includes a refactoring of the
`addObserverScripts` function. The "chrome-document-interactive"
notification is used to trigger initialization because it is roughly
equivalent to where we used to initialize the module inside browser.js.

Review commit: https://reviewboard.mozilla.org/r/171952/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/171952/

Comment 4

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8900552 [details]
Bug 1393128 - 1. Make IntentHelper available globally;

https://reviewboard.mozilla.org/r/171948/#review177302

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java
(Diff revision 1)
>          if (mFormAssistPopup != null)
>              mFormAssistPopup.destroy();
>          if (mTextSelection != null)
>              mTextSelection.destroy();
>          NotificationHelper.destroy();
> -        IntentHelper.destroy();

Now that this is a global singleton I guess we can remove destroy() completely?

Comment 5

[Eugen Sawin [:esawin]]

Comment on attachment 8900554 [details]
Bug 1393128 - 3. Initialize DownloadNotifications outside of browser.js;

https://reviewboard.mozilla.org/r/171952/#review177392

Comment 6

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8900552 [details]
Bug 1393128 - 1. Make IntentHelper available globally;

https://reviewboard.mozilla.org/r/171948/#review177414

Comment 7

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8900553 [details]
Bug 1393128 - 2. Support showing Snackbars globally;

https://reviewboard.mozilla.org/r/171950/#review178574

Comment 8

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8900552 [details]
Bug 1393128 - 1. Make IntentHelper available globally;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/171948/diff/1-2/

Comment 9

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8900553 [details]
Bug 1393128 - 2. Support showing Snackbars globally;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/171950/diff/1-2/

Comment 10

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8900554 [details]
Bug 1393128 - 3. Initialize DownloadNotifications outside of browser.js;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/171952/diff/1-2/

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fd1b29a99163
1. Make IntentHelper available globally; r=nechen,sebastian
https://hg.mozilla.org/integration/autoland/rev/bfc42d1135a7
2. Support showing Snackbars globally; r=sebastian
https://hg.mozilla.org/integration/autoland/rev/b89c2fffc332
3. Initialize DownloadNotifications outside of browser.js; r=esawin

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/fd1b29a99163
https://hg.mozilla.org/mozilla-central/rev/bfc42d1135a7
https://hg.mozilla.org/mozilla-central/rev/b89c2fffc332

Comment 14

[Sorina Florean [:sflorean]]

Hi all, 
Tested today for pre-Release SignOff and the behavior of this bug is odd.
I have a page opened in Custom tab and If I press on PDF file to download, nothing happen. Also, if I want to download a picture or a video, like in Firefox, I'm unable to do that. 
My question is, what behavior do we expect here? Thanks

Comment 15

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Sorina Florean [:sorina] from comment #14)
> Hi all, 
> Tested today for pre-Release SignOff and the behavior of this bug is odd.
> I have a page opened in Custom tab and If I press on PDF file to download,
> nothing happen.

PDF works fine here for me. If I search for 'pdf example' in Google and click one of the PDF results, it opens in the Google Drive PDF reader. There does seem to be something going on with other downloads, though. If I try https://httpbin.org/bytes/1024, the page load seems to just hang and I don't get a download prompt.

Comment 16

[(inactive) Jim Chen [:jchen] [:darchons]]

Yeah looks like there are still some prompts that we're not showing correctly.

Comment 17

[James Willcox (:snorp) ([e-mail]) (he/him)]

Ah, I wonder if this is fallout from bug 1329154. Eugen?

Comment 18

[(inactive) Jim Chen [:jchen] [:darchons]]

Probably not. I think just I missed a few spots in the original patch.

Comment 19

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: 4. Add window to all Prompt usages (v1)

Add a `window` parameter to all Prompt.jsm usages, so the prompt will
appear in the correct window. This includes HelperAppDialog.js, which
was preventing the download chooser dialog from appearing in a custom
tab window.

The patch also moves `getActiveDispatcher` from GeckoViewPermission to
GeckoViewUtils, and makes several improvements to `getChromeWindow` and
`getDispatcherForWindow`. Prompt.jsm now uses the active
WindowDispatcher in the fallback scenario where we don't have a window.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e45f48ecf55
4. Add window to all Prompt usages; r=snorp

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9e45f48ecf55

Comment 22

[Sorina Florean [:sflorean]]

Verified as fixed on latest Nightly build, 59.0b13 and release with Nexus 6(Android 6.0.1), HTC 10(Android 8.1.0) and Samsung Galaxy S8(Android 7.0).