1448056 - Put BasicPromptDelegate in GeckoView AAR

Status: RESOLVED WONTFIX

(GeckoView :: General, enhancement, P2)

Description

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

It's part of geckoview_example now, but it's useful for all consumers.

Comment 1

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

Attachment: Bug 1448056 - Move BasicGeckoViewPrompt and ExamplePermissionDelegate into GeckoView

These two are conflated into one class, BasicPromptPermissionDelegate.
This makes it easy for an app to get reasonable behavior OOTB for web
prompts (alert(), etc) and permission prompts.

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

Comment 2

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

Comment on attachment 8971710 [details]
Bug 1448056 - Move BasicGeckoViewPrompt and ExamplePermissionDelegate into GeckoView

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

Comment 3

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

Comment on attachment 8971710 [details]
Bug 1448056 - Move BasicGeckoViewPrompt and ExamplePermissionDelegate into GeckoView

https://reviewboard.mozilla.org/r/240478/#review246274

r- because we don't want to ship strings with GeckoView, so we need a better solution there.

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/BasicPromptPermissionDelegate.java:1
(Diff revision 2)
> +/* -*- Mode: Java; c-basic-offset: 4; tab-width: 20; indent-tabs-mode: nil; -*-

Make mercurial recognize the file move

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/BasicPromptPermissionDelegate.java:52
(Diff revision 2)
> +import java.util.ArrayList;
> +import java.util.Calendar;
> +import java.util.Date;
> +import java.util.Locale;
> +
> +public class BasicPromptPermissionDelegate implements GeckoSession.PromptDelegate, GeckoSession.PermissionDelegate {

Wrap line, and add class javadoc (including explanation of need to set `filePickerRequestCode` and `androidPermissionRequestCode`)

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/BasicPromptPermissionDelegate.java:53
(Diff revision 2)
> +import java.util.Calendar;
> +import java.util.Date;
> +import java.util.Locale;
> +
> +public class BasicPromptPermissionDelegate implements GeckoSession.PromptDelegate, GeckoSession.PermissionDelegate {
> +    protected static final String LOGTAG = "BasicPromptPermissionDelegate";

"GeckoBasicPromptPermissionDelegate"

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/BasicPromptPermissionDelegate.java:66
(Diff revision 2)
> +     * The request code used for file picker responses. The app must implement
> +     * {@link Activity#onActivityResult(int, int, Intent)} and call
> +     * {@link #onFileCallbackResult(int, Intent)} if the request code matches this value. The app
> +     * is free to set its own value here to avoid conflicts with other result codes.
> +     */
> +    public int filePickerRequestCode = 1;

Default to an invalid value.

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/BasicPromptPermissionDelegate.java:74
(Diff revision 2)
> +     * The request cod used for Android permission requests. The app must implement
> +     * {@link Activity#onActivityResult(int, int, Intent)} and call
> +     * {@link #onRequestPermissionsResult(String[], int[])}  if the request code matches this value.
> +     * The app is free to set its own value here to avoid conflicts with other result codes.
> +     */
> +    public int androidPermissionRequestCode = 2;

Default to an invalid value.

::: mobile/android/geckoview_example/src/main/res/values/strings.xml
(Diff revision 2)
>  <resources>
> -    <string name="app_name">geckoview_example</string>

Ugh still need this in geckoview_example

Comment 4

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

Comment on attachment 8971710 [details]
Bug 1448056 - Move BasicGeckoViewPrompt and ExamplePermissionDelegate into GeckoView

https://reviewboard.mozilla.org/r/240478/#review246408

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/BasicPromptPermissionDelegate.java:1
(Diff revision 2)
> +/* -*- Mode: Java; c-basic-offset: 4; tab-width: 20; indent-tabs-mode: nil; -*-

I don't think I have much control over this since I'm using git, but I'll see what I can do.

Comment 5

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

Comment on attachment 8971710 [details]
Bug 1448056 - Move BasicGeckoViewPrompt and ExamplePermissionDelegate into GeckoView

https://reviewboard.mozilla.org/r/240478/#review246274

I don't think it's going to be avoidable. I mostly want to avoid strings because it's a headache, but in this case I think we would just punt on any localization issues and leave it up to the app. AFAIK, the app can easily override these strings with its own, and so we'd basically just be providing placeholders. This is still better than the app maintaining the entire delegate if it only needs "normal" prompt/permission functionality.

The gist here is that while we have strings, we won't be taking on the burden of string localization.

Comment 6

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

Comment on attachment 8971710 [details]
Bug 1448056 - Move BasicGeckoViewPrompt and ExamplePermissionDelegate into GeckoView

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

Comment 7

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

Comment on attachment 8971710 [details]
Bug 1448056 - Move BasicGeckoViewPrompt and ExamplePermissionDelegate into GeckoView

https://reviewboard.mozilla.org/r/240478/#review247312

I think we decided to keep this out of GV.

Comment 8

[[ex-Mozilla] Agi Sferro | :agi]

WONTFIX as per Comment 7.