1322579 - [geckoview] Add loadData and loadDataWithBaseUri

Status: RESOLVED FIXED

(GeckoView :: General, defect, P1)

Description

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

Android WebView has this, and it's pretty useful for loading a generated page.

Comment 1

[Emily Kager [:ekager]]

In Focus, focus:about and focus:rights (and in the future focus:debug) are generated with WebView with loadDataWithBaseUri. I have a GeckoView workaround working, but this method would be nice to have. 

Github issue: https://github.com/mozilla-mobile/focus-android/issues/2109

Comment 2

[Chris Peterson [:cpeterson]]

This is a nice-to-have for Klar+GeckoView.

Comment 3

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

Klar wants to use this for error pages, so moving up to a P1

Comment 4

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

Attachment: Bug 1322579 - Add flags to GeckoSession.loadUri()

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

Comment 5

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

Attachment: Bug 1322579 - Add GeckoSession.loadData() and loadString()

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

Comment 6

[Randall Barker [:rbarker]]

Comment on attachment 8960233 [details]
Bug 1322579 - Add GeckoSession.loadData() and loadString()

https://reviewboard.mozilla.org/r/228994/#review234730

Comment 7

[Eugen Sawin [:esawin]]

Comment on attachment 8960232 [details]
Bug 1322579 - Add flags to GeckoSession.loadUri()

https://reviewboard.mozilla.org/r/228992/#review234872

r=me with issued addressed.

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java:799
(Diff revision 1)
> +
> +    // These flags follow similarly named ones in Gecko's nsIWebNavigation.idl
> +    // https://searchfox.org/mozilla-central/source/docshell/base/nsIWebNavigation.idl
> +    //
> +    // We do not use the same values directly in order to insulate ourselves from
> +    // changes in Gecko. Instead, the flags are converted in GeckoViewNavigation.jsm.

This is a good comment for devs, but shouldn't be in public docs, can we exclude stuff?

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java:862
(Diff revision 1)
> +        }
>          mEventDispatcher.dispatch("GeckoView:LoadUri", msg);
>      }
>  
>      /**
> -    * Load the given URI.
> +     * Load the given Uri.

The Uri is the type name, it's still a URI.

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java:872
(Diff revision 1)
> +    }
> +
> +    /**
> +     * Load the given Uri with the specified referrer and load type.
> +     * @param uri the Uri to load
> +     * @param flags the load flags to use, an ORed value of {@link #LOAD_FLAGS_NONE LOAD_FLAGS_*}

OR-ed maybe, this looks weird.

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java:885
(Diff revision 1)
> +     * @param uri the Uri to load
> +     * @param referrer the Uri to use as the referrer
> +     * @param flags the load flags to use, an ORed value of {@link #LOAD_FLAGS_NONE LOAD_FLAGS_*}
> -    */
> +     */
> -    public void loadUri(Uri uri) {
> -        loadUri(uri.toString());
> +    public void loadUri(@NonNull Uri uri, @Nullable Uri referrer, @LoadFlags int flags) {
> +        loadUri(uri.toString(), referrer.toString(), flags);

With referrer == null, we'll pass "null" as a non-null referrer.

::: mobile/android/modules/geckoview/GeckoViewNavigation.jsm:59
(Diff revision 1)
>          break;
>        case "GeckoView:GoForward":
>          this.browser.goForward();
>          break;
>        case "GeckoView:LoadUri":
> -        this.browser.loadURI(aData.uri, null, null, null);
> +        const { uri, referrer, flags } = aData;

Null-referrers are not added to the message.

Comment 8

[Eugen Sawin [:esawin]]

Comment on attachment 8960233 [details]
Bug 1322579 - Add GeckoSession.loadData() and loadString()

https://reviewboard.mozilla.org/r/228994/#review234884

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java:907
(Diff revision 1)
> +     *
> +     * @param data a String representing the data
> +     * @param mimeType the mime type of the data, e.g. "text/plain". Maybe be null, in
> +     *                 which case the type is guessed.
> +     */
> +    public void loadString(String data, String mimeType) {

@NonNull, final?

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java:950
(Diff revision 1)
> +     * @param mimeType optional mime type, e.g. text/plain
> +     * @return a URI String
> +     */
> +    public static String createDataUri(@NonNull byte[] bytes, @Nullable String mimeType) {
> +        return String.format("data:%s,%s", mimeType != null ? mimeType : "",
> +                             Base64.encodeToString(bytes, Base64.URL_SAFE | Base64.NO_WRAP));

I assume we don't need to call into encodeURIComponent with this?

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java:962
(Diff revision 1)
> +     * @return a URI String
> +     */
> +    public static String createDataUri(@NonNull String data, @Nullable String mimeType) {
> +        return String.format("data:%s,%s", mimeType != null ? mimeType : "",
> +                Base64.encodeToString(data.getBytes(Charset.forName("utf-8")),
> +                                 Base64.URL_SAFE | Base64.NO_WRAP));

Can't we just call into the byte[] version?

Comment 9

[Dylan Roeh (:droeh) (he/him)]

Comment on attachment 8960232 [details]
Bug 1322579 - Add flags to GeckoSession.loadUri()

https://reviewboard.mozilla.org/r/228992/#review234932

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java:791
(Diff revision 1)
>          // May be called on any thread.
>          return mTextInput;
>      }
>  
> +    @IntDef(flag=true,
> +            value = { LOAD_FLAGS_NONE, LOAD_FLAGS_BYPASS_CACHE, LOAD_FLAGS_BYPASS_PROXY,

Is it necessary/correct to include LOAD_FLAGS_NONE (which is, properly speaking, a combination of flags) in the set of values here? I can't really tell for sure from IntDef documentation, but I would expect 0 to always be a valid value to pass when flag=true. If that's the case I think it's better to remove it from the set of values (but leave the constant defined below).

Comment 10

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

Comment on attachment 8960232 [details]
Bug 1322579 - Add flags to GeckoSession.loadUri()

https://reviewboard.mozilla.org/r/228992/#review234872

> This is a good comment for devs, but shouldn't be in public docs, can we exclude stuff?

It's not formatted as a javadoc, so it doesn't show up in the docs.

> Null-referrers are not added to the message.

That's fine, it will be undefined in that case, and it gets normalized to null in the loadURIWithFlags() call.

Comment 11

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

Comment on attachment 8960233 [details]
Bug 1322579 - Add GeckoSession.loadData() and loadString()

https://reviewboard.mozilla.org/r/228994/#review234938

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java:950
(Diff revision 1)
> +     * @param mimeType optional mime type, e.g. text/plain
> +     * @return a URI String
> +     */
> +    public static String createDataUri(@NonNull byte[] bytes, @Nullable String mimeType) {
> +        return String.format("data:%s,%s", mimeType != null ? mimeType : "",
> +                             Base64.encodeToString(bytes, Base64.URL_SAFE | Base64.NO_WRAP));

Right, the URL_SAFE variation takes care of that for base64. I guess the mime type might not be URL encoded, but nothing we do there is going to really help, since AFAIK all valid mime types are URL safe.

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java:962
(Diff revision 1)
> +     * @return a URI String
> +     */
> +    public static String createDataUri(@NonNull String data, @Nullable String mimeType) {
> +        return String.format("data:%s,%s", mimeType != null ? mimeType : "",
> +                Base64.encodeToString(data.getBytes(Charset.forName("utf-8")),
> +                                 Base64.URL_SAFE | Base64.NO_WRAP));

Yup, for sure. Whoops.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/504dcbc210b9
Add flags to GeckoSession.loadUri() r=esawin,droeh
https://hg.mozilla.org/integration/mozilla-inbound/rev/b8761cfdec93
Add GeckoSession.loadData() and loadString() r=esawin,rbarker

Comment 13

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/504dcbc210b9
https://hg.mozilla.org/mozilla-central/rev/b8761cfdec93

Comment 14

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

(In reply to Dylan Roeh (:droeh) from comment #9)
> Comment on attachment 8960232 [details]
> Bug 1322579 - Add flags to GeckoSession.loadUri()
> 
> https://reviewboard.mozilla.org/r/228992/#review234932
> 
> :::
> mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.
> java:791
> (Diff revision 1)
> >          // May be called on any thread.
> >          return mTextInput;
> >      }
> >  
> > +    @IntDef(flag=true,
> > +            value = { LOAD_FLAGS_NONE, LOAD_FLAGS_BYPASS_CACHE, LOAD_FLAGS_BYPASS_PROXY,
> 
> Is it necessary/correct to include LOAD_FLAGS_NONE (which is, properly
> speaking, a combination of flags) in the set of values here? I can't really
> tell for sure from IntDef documentation, but I would expect 0 to always be a
> valid value to pass when flag=true. If that's the case I think it's better
> to remove it from the set of values (but leave the constant defined below).

Whoops, I missed this comment. AFAICT, people typically put all of the flags into the @IntDef, but I see your point.