Add DownloadListener.onDownloadStart support

RESOLVED FIXED in Firefox 61

Status

enhancement
P1
normal
RESOLVED FIXED
a year ago
5 months ago

People

(Reporter: snorp, Assigned: snorp)

Tracking

(Blocks 1 bug)

unspecified
mozilla61
Dependency tree / graph

Firefox Tracking Flags

(firefox58 wontfix, firefox59 wontfix, firefox60 wontfix, firefox61 fixed)

Details

(Whiteboard: [geckoview:klar])

Attachments

(3 attachments)

Focus currently uses the download support from WebView, and they would like to have it for GeckoView too. The relevant WebView API is here: https://developer.android.com/reference/android/webkit/DownloadListener.html
Whiteboard: [geckoview:klar]
snorp, cpeterson: is there a meta tracking WebView compatibility?
(In reply to Nick Alexander :nalexander from comment #1)
> snorp, cpeterson: is there a meta tracking WebView compatibility?

I just filed meta bug 1437983 (Bugzilla alias "WebView-compat").

I have been using the "[geckoview:klar]" whiteboard tag to track the GeckoView features needed for Firefox Focus/Klar, but that's not quite the same thing as compatibility with Chromium's WebView API and functionality.

The GeckoView wiki has a list of open [geckoview:klar] bugs:

https://wiki.mozilla.org/Mobile/GeckoView#Bugs
Summary: Add download support → Add DownloadListener.onDownloadStart support
Assignee: nobody → snorp
Comment hidden (mozreview-request)

Comment 6

a year ago
mozreview-review
Comment on attachment 8962890 [details]
Bug 1437701 - Add ContentDelegate test

https://reviewboard.mozilla.org/r/231692/#review237288


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: mobile/android/geckoview/src/androidTest/assets/www/titleChange.html:5
(Diff revision 1)
> +<html>
> +<header><title>Title1</title></header>
> +    <body>
> +        <script>
> +            addEventListener('load', function() {

Error: Strings must use doublequote. [eslint: quotes]

Comment 7

a year ago
mozreview-review
Comment on attachment 8962892 [details]
Bug 1437701 - Add GeckoSession.ContentDelegate.onExternalResponse()

https://reviewboard.mozilla.org/r/231696/#review237296


Code analysis found 6 defects in this patch:
 - 6 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: mobile/android/geckoview/src/androidTest/assets/www/download.html:5
(Diff revision 1)
> +<html>
> +<body>
> +    <script>
> +        const data = "Downloaded Data";
> +        const element = document.createElement('a');

Error: Strings must use doublequote. [eslint: quotes]

::: mobile/android/geckoview/src/androidTest/assets/www/download.html:6
(Diff revision 1)
> +<html>
> +<body>
> +    <script>
> +        const data = "Downloaded Data";
> +        const element = document.createElement('a');
> +        element.setAttribute('href', 'data:text/plain;charset=utf-8,' + encodeURIComponent(data));

Error: Strings must use doublequote. [eslint: quotes]

::: mobile/android/geckoview/src/androidTest/assets/www/download.html:6
(Diff revision 1)
> +<html>
> +<body>
> +    <script>
> +        const data = "Downloaded Data";
> +        const element = document.createElement('a');
> +        element.setAttribute('href', 'data:text/plain;charset=utf-8,' + encodeURIComponent(data));

Error: Strings must use doublequote. [eslint: quotes]

::: mobile/android/geckoview/src/androidTest/assets/www/download.html:7
(Diff revision 1)
> +<body>
> +    <script>
> +        const data = "Downloaded Data";
> +        const element = document.createElement('a');
> +        element.setAttribute('href', 'data:text/plain;charset=utf-8,' + encodeURIComponent(data));
> +        element.setAttribute('download', 'download.txt');

Error: Strings must use doublequote. [eslint: quotes]

::: mobile/android/geckoview/src/androidTest/assets/www/download.html:7
(Diff revision 1)
> +<body>
> +    <script>
> +        const data = "Downloaded Data";
> +        const element = document.createElement('a');
> +        element.setAttribute('href', 'data:text/plain;charset=utf-8,' + encodeURIComponent(data));
> +        element.setAttribute('download', 'download.txt');

Error: Strings must use doublequote. [eslint: quotes]

::: mobile/android/geckoview/src/androidTest/assets/www/download.html:8
(Diff revision 1)
> +    <script>
> +        const data = "Downloaded Data";
> +        const element = document.createElement('a');
> +        element.setAttribute('href', 'data:text/plain;charset=utf-8,' + encodeURIComponent(data));
> +        element.setAttribute('download', 'download.txt');
> +        element.style.display = 'none';

Error: Strings must use doublequote. [eslint: quotes]

Comment 8

a year ago
mozreview-review
Comment on attachment 8962890 [details]
Bug 1437701 - Add ContentDelegate test

https://reviewboard.mozilla.org/r/231692/#review237298

::: mobile/android/geckoview/src/androidTest/assets/www/titleChange.html:5
(Diff revision 1)
> +<html>
> +<header><title>Title1</title></header>
> +    <body>
> +        <script>
> +            addEventListener('load', function() {

I agree with the bot here.

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/ContentDelegateTest.kt:25
(Diff revision 1)
> +@RunWith(AndroidJUnit4::class)
> +@MediumTest
> +class ContentDelegateTest : BaseSessionTest() {
> +
> +    @Test fun titleChange() {
> +        sessionRule.session.loadTestPath("/assets/www/titleChange.html")

Should go into BaseSessionTest.

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/ContentDelegateTest.kt:31
(Diff revision 1)
> +
> +        sessionRule.waitUntilCalled(object : Callbacks.ContentDelegate {
> +            @AssertCalled(count = 2)
> +            override fun onTitleChange(session: GeckoSession, title: String) {
> +                var expected: String
> +                if (sessionRule.currentCall.counter == 1) {

That's a common pattern in our tests that I don't like.
I would like to see the results in an array or map and the counter could be used as the index of key.
That would remove most test results conditions.
Attachment #8962890 - Flags: review?(esawin) → review+

Comment 9

a year ago
mozreview-review
Comment on attachment 8962891 [details]
Bug 1437701 - Add GeckoBundle.getLong()

https://reviewboard.mozilla.org/r/231694/#review237310

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/util/GeckoBundle.java:294
(Diff revision 1)
> +     * @return long array value
> +     */
> +    public long[] getLongArray(final String key) {
> +        final Object value = mMap.get(key);
> +        return value == null ? null : Array.getLength(value) == 0 ? EMPTY_LONG_ARRAY :
> +                value instanceof double[] ? getLongArray((double[]) value) : (long[]) value;

Nested ternary operators are difficult to read, but apparently that's consistently used in this file.
Attachment #8962891 - Flags: review?(esawin) → review+

Comment 10

a year ago
mozreview-review
Comment on attachment 8962892 [details]
Bug 1437701 - Add GeckoSession.ContentDelegate.onExternalResponse()

https://reviewboard.mozilla.org/r/231696/#review237314

::: commit-message-26cd8:1
(Diff revision 1)
> +Bug 1437701 - Add GeckoSession.ContentDelegate.onExternalRequest() r=esawin,droeh

Yes, but why?

::: mobile/android/base/java/org/mozilla/gecko/GeckoApp.java:906
(Diff revision 1)
>                                final int screenY, final String uri,
>                                int elementType, final String elementSrc) {
>      }
>  
> +    @Override
> +    public void onExternalRequest(final GeckoSession session, GeckoSession.WebRequestInfo request) {

final

::: mobile/android/base/java/org/mozilla/gecko/customtabs/CustomTabsActivity.java:728
(Diff revision 1)
>              }
>          });
>      }
>  
> +    @Override
> +    public void onExternalRequest(GeckoSession session, GeckoSession.WebRequestInfo request) {

final, final

::: mobile/android/base/java/org/mozilla/gecko/webapps/WebAppActivity.java:368
(Diff revision 1)
>  
>          WebApps.openInFennec(validUri, WebAppActivity.this);
>      }
>  
>      @Override // GeckoSession.ContentDelegate
> +    public void onExternalRequest(GeckoSession session, GeckoSession.WebRequestInfo request) {

final, final

::: mobile/android/components/geckoview/GeckoViewExternalAppService.js:28
(Diff revision 1)
> +
> +ExternalAppService.prototype = {
> +  classID: Components.ID("{a89eeec6-6608-42ee-a4f8-04d425992f45}"),
> +  QueryInterface: XPCOMUtils.generateQI([Ci.nsIExternalHelperAppService]),
> +
> +  doContent(mimeType, request, context, forceSave) {

Please add debug logs.

::: mobile/android/components/geckoview/GeckoViewExternalAppService.js:44
(Diff revision 1)
> +
> +    request.cancel(Cr.NS_ERROR_ABORT);
> +    Components.returnCode = Cr.NS_ERROR_ABORT;
> +  },
> +
> +  applyDecodingForExtension(ext, encoding) {

Please add debug logs.

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/ContentDelegateTest.kt:41
(Diff revision 1)
>              }
>          })
>      }
>  
> +    @Test fun download() {
> +        sessionRule.session.loadTestPath("/assets/www/download.html")

Should go into BaseSessionTest.

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java:104
(Diff revision 1)
>                  "GeckoView:DOMTitleChanged",
>                  "GeckoView:DOMWindowFocus",
>                  "GeckoView:DOMWindowClose",
>                  "GeckoView:FullScreenEnter",
> -                "GeckoView:FullScreenExit"
> +                "GeckoView:FullScreenExit",
> +                "GeckoView:ExternalRequest"

Could go before :FullScreenEnter.

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java:1620
(Diff revision 1)
>              return ContentDelegate.ELEMENT_TYPE_AUDIO;
>          }
>          return ContentDelegate.ELEMENT_TYPE_NONE;
>      }
>  
> +    public class WebRequestInfo {

Docs.
Attachment #8962892 - Flags: review?(esawin) → review+

Comment 11

a year ago
mozreview-review
Comment on attachment 8962892 [details]
Bug 1437701 - Add GeckoSession.ContentDelegate.onExternalResponse()

https://reviewboard.mozilla.org/r/231696/#review237544

r+ with some changes to WebRequestInfo.

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java:1620
(Diff revision 1)
>              return ContentDelegate.ELEMENT_TYPE_AUDIO;
>          }
>          return ContentDelegate.ELEMENT_TYPE_NONE;
>      }
>  
> +    public class WebRequestInfo {

I think it would be more in line with what we do elsewhere (eg MediaSource, AuthOptions) to put this inside ContentDelegate.

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java:1621
(Diff revision 1)
>          }
>          return ContentDelegate.ELEMENT_TYPE_NONE;
>      }
>  
> +    public class WebRequestInfo {
> +        private String mUri;

Again, for consistency with similar classes, make the fields in this class public final and get rid of the getters.
Attachment #8962892 - Flags: review?(droeh) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 15

a year ago
mozreview-review-reply
Comment on attachment 8962892 [details]
Bug 1437701 - Add GeckoSession.ContentDelegate.onExternalResponse()

https://reviewboard.mozilla.org/r/231696/#review237544

> I think it would be more in line with what we do elsewhere (eg MediaSource, AuthOptions) to put this inside ContentDelegate.

Those are very specific to the delegate. This is a more generic class that I think we'll use in other places. Also, I've renamed it to WebResponseInfo since it's actually a response, not a request.

> Again, for consistency with similar classes, make the fields in this class public final and get rid of the getters.

Yeah I don't like those other classes. We should fix those to be more like this one. It doesn't make sense to allow those values to be changed by the app.
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #15)
> Comment on attachment 8962892 [details]
> Bug 1437701 - Add GeckoSession.ContentDelegate.onExternalResponse()
> 
> https://reviewboard.mozilla.org/r/231696/#review237544
> 
> > I think it would be more in line with what we do elsewhere (eg MediaSource, AuthOptions) to put this inside ContentDelegate.
> 
> Those are very specific to the delegate. This is a more generic class that I
> think we'll use in other places. Also, I've renamed it to WebResponseInfo
> since it's actually a response, not a request.

OK, if you're sure it will be used elsewhere then that's fine.

> > Again, for consistency with similar classes, make the fields in this class public final and get rid of the getters.
> 
> Yeah I don't like those other classes. We should fix those to be more like
> this one. It doesn't make sense to allow those values to be changed by the
> app.

Those values can't be changed by the app; they're final. As written, yours can't be changed after construction but nevertheless are not final (why?) -- and if you make them final, there's no real reason I can see for having them private w/ getters except to guard against possible (but very unlikely in my opinion) future changes.
(In reply to Dylan Roeh (:droeh) from comment #16)
> > > Again, for consistency with similar classes, make the fields in this class public final and get rid of the getters.
> > 
> > Yeah I don't like those other classes. We should fix those to be more like
> > this one. It doesn't make sense to allow those values to be changed by the
> > app.
> 
> Those values can't be changed by the app; they're final. As written, yours
> can't be changed after construction but nevertheless are not final (why?)

Yeah, they should be final.

> --
> and if you make them final, there's no real reason I can see for having them
> private w/ getters except to guard against possible (but very unlikely in my
> opinion) future changes.

Yeah, that's a good point, forgot about final. I'll get rid of the getters.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 21

a year ago
Pushed by jwillcox@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/698a2f48d8b6
Add ContentDelegate test r=esawin
https://hg.mozilla.org/integration/mozilla-inbound/rev/7de2ec5e9b04
Add GeckoBundle.getLong() r=esawin
https://hg.mozilla.org/integration/mozilla-inbound/rev/769685eb371b
Add GeckoSession.ContentDelegate.onExternalResponse() r=esawin,droeh

Comment 22

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/698a2f48d8b6
https://hg.mozilla.org/mozilla-central/rev/7de2ec5e9b04
https://hg.mozilla.org/mozilla-central/rev/769685eb371b
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61

Updated

5 months ago
Product: Firefox for Android → GeckoView
Target Milestone: Firefox 61 → mozilla61
You need to log in before you can comment on or make changes to this bug.