Replace ambiguous booleans in GeckoView API with clearly named constants

RESOLVED FIXED in Firefox 64

Status

enhancement
P2
normal
RESOLVED FIXED
8 months ago
5 months ago

People

(Reporter: droeh, Assigned: droeh)

Tracking

(Blocks 1 bug)

53 Branch
mozilla64
All
Android

Firefox Tracking Flags

(firefox63 wontfix, firefox64 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

8 months ago
We have a few instances in the GV API where a boolean is used with rather ambiguous meaning -- eg, onLoadRequest expects a GeckoResult<Boolean> return but it's not entirely clear whether a result of true means that the load should continue or that it has been handled. Replacing them with clearly named constants (eg ALLOW/DENY) should clarify the API a bit.
(Assignee)

Comment 1

8 months ago
This replaces ambiguous booleans with an enum; yes, I know, enums are verboten, but as far as I can tell you can't meaningfully use @IntDefs in template params. I'm open to alternatives.
Attachment #9013832 - Flags: review?(snorp)
(Assignee)

Comment 2

8 months ago
This updates tests to use GeckoBoolean.

Also: GeckoBoolean is really not a great name, but I'm struggling to come up with something that encapsulates the concept.
Attachment #9013833 - Flags: review?(snorp)
Comment on attachment 9013832 [details] [diff] [review]
Replace ambiguous Booleans with an enum

Review of attachment 9013832 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, GeckoBoolean is not the best name. Let's try to think of something better.

If we put it in GeckoSession we can probably drop the Gecko prefix, which helps a bit with aesthetics. What about 'Decision'? So you'd have Decision.ALLOW, or Decision.DENY. Hmmm. Needs some more thought.
Making this a P2. A good semantic fix, good to get it done.
Priority: -- → P2
I think in the meeting last week we discussed something like:

public enum AllowOrDeny {
    ALLOW, DENY
};
(Assignee)

Comment 6

7 months ago
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #5)
> I think in the meeting last week we discussed something like:
> 
> public enum AllowOrDeny {
>     ALLOW, DENY
> };

We also considered adding something like GeckoResult.ALLOW()/GeckoResult.DENY(); static member functions that obscure the boolean. But that's harder to document clearly, I think, compared to just having an AllowOrDeny type. I also looked at a thesaurus for a bit trying to find something that captures the meaning a bit less verbosely, but no luck there. I'll update the existing patch to AllowOrDeny and put it up for review sometime today.
(Assignee)

Comment 7

7 months ago
Attachment #9013832 - Attachment is obsolete: true
Attachment #9013832 - Flags: review?(snorp)
Attachment #9016276 - Flags: review?(snorp)
Attachment #9016276 - Flags: review?(nchen)
(Assignee)

Comment 8

7 months ago
Attachment #9013833 - Attachment is obsolete: true
Attachment #9013833 - Flags: review?(snorp)
Attachment #9016277 - Flags: review?(snorp)
Comment on attachment 9016276 [details] [diff] [review]
Replace ambiguous booleans with AllowOrDeny

Review of attachment 9016276 [details] [diff] [review]:
-----------------------------------------------------------------

I really like this a lot better than the Boolean. Let's see one more patch with docs.

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java
@@ +66,5 @@
>      private static final int WINDOW_TRANSFER_OUT = 2; // Window has been transfer.
>      // Window has been set due to another session being transferred to this one.
>      private static final int WINDOW_TRANSFER_IN = 3;
>  
> +    public enum AllowOrDeny {

Let's make this a toplevel type, since we could easily imagine using it elsewhere. Also, docs!
Attachment #9016276 - Flags: review?(snorp) → review-
Attachment #9016277 - Flags: review?(snorp) → review+
(Assignee)

Comment 10

7 months ago
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #9) 
> Let's make this a toplevel type, since we could easily imagine using it
> elsewhere. Also, docs!

Do we want to keep the same name if it's top level? We discussed GeckoAllowOrDeny and decided the Gecko was redundant because it lived inside a Gecko* class... or did I misunderstand and the Gecko prefix is unnecessary for anything in o.m.gv?
Flags: needinfo?(snorp)
Comment on attachment 9016276 [details] [diff] [review]
Replace ambiguous booleans with AllowOrDeny

Review of attachment 9016276 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/java/org/mozilla/gecko/webapps/WebAppActivity.java
@@ +390,5 @@
>  
>          if (mManifest.isInScope(uri) && target != TARGET_WINDOW_NEW) {
>              // This is in scope and wants to load in the same frame, so
>              // let Gecko handle it.
> +            return GeckoResult.fromValue(AllowOrDeny.ALLOW);

I think this pattern is common enough that we should add a `GeckoResult.ALLOW` shortcut.

@@ +419,5 @@
>                  Log.w(LOGTAG, "No activity handler found for: " + urlStr);
>              }
>          }
>  
> +        return GeckoResult.fromValue(AllowOrDeny.DENY);

Same with a `GeckoResult.DENY` shortcut.

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java
@@ +66,5 @@
>      private static final int WINDOW_TRANSFER_OUT = 2; // Window has been transfer.
>      // Window has been set due to another session being transferred to this one.
>      private static final int WINDOW_TRANSFER_IN = 3;
>  
> +    public enum AllowOrDeny {

Yeah top-level with slight preference for `GeckoAllowOrDeny`
Attachment #9016276 - Flags: review?(nchen) → review+
(In reply to Dylan Roeh (:droeh) from comment #10)
> (In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #9) 
> > Let's make this a toplevel type, since we could easily imagine using it
> > elsewhere. Also, docs!
> 
> Do we want to keep the same name if it's top level? We discussed
> GeckoAllowOrDeny and decided the Gecko was redundant because it lived inside
> a Gecko* class... or did I misunderstand and the Gecko prefix is unnecessary
> for anything in o.m.gv?

IMHO dropping the Gecko prefix is ok for things that don't have obvious conflicts. It would be hard for `GeckoView` to just be `View` or `WebView`, for instance. I think `AllowOrDeny` is fine without a prefix, but I don't feel that strongly about it.
Flags: needinfo?(snorp)

Comment 13

7 months ago
Pushed by droeh@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/af27cdf736f5
Replace ambiguous Booleans with AllowOrDeny, an enum with clearly named values. r=snorp,jchen
https://hg.mozilla.org/integration/mozilla-inbound/rev/3939abb2b2c2
Update tests to use AllowOrDeny in lieu of Boolean. r=snorp

Comment 14

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/af27cdf736f5
https://hg.mozilla.org/mozilla-central/rev/3939abb2b2c2
Status: NEW → RESOLVED
Last Resolved: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64

Updated

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