Closed Bug 1437701 Opened 6 years ago Closed 6 years ago

Add DownloadListener.onDownloadStart support

Categories

(GeckoView :: General, enhancement, P1)

Product:
GeckoView
Component:
General
Type:
enhancement

Tracking

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

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox58 --- wontfix
firefox59 --- wontfix
firefox60 --- wontfix
firefox61 --- fixed

People

(Reporter: snorp, Assigned: snorp)

References

(Blocks 1 open bug)

Details

(Whiteboard: [geckoview:klar])

Attachments

(3 files)

Bug 1437701 - Add ContentDelegate test
59 bytes, text/x-review-board-request
esawin
: review+
Details
Bug 1437701 - Add GeckoBundle.getLong()
59 bytes, text/x-review-board-request
esawin
: review+
Details
Bug 1437701 - Add GeckoSession.ContentDelegate.onExternalResponse()
59 bytes, text/x-review-board-request
droeh
: review+
esawin
: review+
Details 
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
status-firefox58: --- → wontfix
status-firefox59: --- → wontfix
status-firefox60: --- → affected
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 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 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 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 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 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 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 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.
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
Product: Firefox for Android → GeckoView
Target Milestone: Firefox 61 → mozilla61
Regressions: 1553943
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: