859212 - Change AwesomeBar entry mode for stock gesture keyboard and TouchPal

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Keyboards and IME, defect)

Comment 1

[Chris Peterson [:cpeterson]]

Attachment: part-1-move-swype-check.patch

Part 1: Move AwesomeBar's Swype logic to InputMethods.

Comment 2

[Chris Peterson [:cpeterson]]

Attachment: part-2-add-stock-and-touchpal-keyboards.patch

Part 2: Add Android stock keyboard and TouchPal to list of gesture keyboards.

Comment 3

[Chris Peterson [:cpeterson]]

Attachment: part-3-remove-unused-ime-names.patch

Part 3: Remove unused InputMethod names. This commented out code has little value. If we need to add a special case for one of these input methods, we can recheck and document its IME name then. The TouchPal name, for example, was wrong.

Comment 4

[Chris Peterson [:cpeterson]]

Attachment: part-4-simplify-hkb-whitelist.patch

Part 4: Replace fancy static Java collection with simpler if statement.

The fancy static collection sHBKWhiteList is hard to read and causes unnecessary memory allocations during static initialization of the InputMethods class. I think the direct string equals() checks are easy to read and probably more efficient.

Comment 5

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

Comment on attachment 734482 [details] [diff] [review]
part-1-move-swype-check.patch

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

Thanks!

::: mobile/android/base/InputMethods.java
@@ +102,5 @@
> +
> +    public static boolean isGestureKeyboard(String inputMethod) {
> +        return inputMethod.equals(METHOD_SWYPE) ||
> +               inputMethod.equals(METHOD_SWYPE_BETA);
> +    }

I think you should just take a Context argument and get the current IME inside the method; see 'shouldDelayAwesomebarUpdate(Context context)' above.

Also, switch around the comparisons, i.e.
> return METHOD_SWYPE.equals(inputMethod) ||
>        METHOD_SWYPE_BETA.equals(inputMethod);

Comment 6

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

Comment on attachment 734483 [details] [diff] [review]
part-2-add-stock-and-touchpal-keyboards.patch

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

Thanks!

::: mobile/android/base/InputMethods.java
@@ +108,5 @@
> +        return (Build.VERSION.SDK_INT >= 17 && (inputMethod.equals(METHOD_GOOGLE_LATINIME) ||
> +                                                inputMethod.equals(METHOD_STOCK_LATINIME))) ||
> +               inputMethod.equals(METHOD_SWYPE) ||
> +               inputMethod.equals(METHOD_SWYPE_BETA) ||
> +               inputMethod.equals(METHOD_TOUCHPAL_KEYBOARD);

Don't forget to switch around comparisons :)

Comment 7

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

Comment on attachment 734485 [details] [diff] [review]
part-3-remove-unused-ime-names.patch

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

Thanks!

Comment 8

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

Comment on attachment 734487 [details] [diff] [review]
part-4-simplify-hkb-whitelist.patch

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

Thanks!

We are not actually using canUseInputMethodOnHKB(), but we can keep it.

Comment 9

[Chris Peterson [:cpeterson]]

Landed with the suggested changes:
* Changed isGestureKeyboard() to take Context
* Switched METHOD.equals() string comparisons
* Removed unused canUseInputMethodOnHKB()

https://hg.mozilla.org/integration/mozilla-inbound/rev/d678fbf4ca2e
https://hg.mozilla.org/integration/mozilla-inbound/rev/0c4b651f1a44
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1a739e3b5fa
https://hg.mozilla.org/integration/mozilla-inbound/rev/79c23403daee

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/d678fbf4ca2e
https://hg.mozilla.org/mozilla-central/rev/0c4b651f1a44
https://hg.mozilla.org/mozilla-central/rev/f1a739e3b5fa
https://hg.mozilla.org/mozilla-central/rev/79c23403daee

Comment 11

[Chris Peterson [:cpeterson]]

Comment on attachment 734483 [details] [diff] [review]
part-2-add-stock-and-touchpal-keyboards.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): N/A
User impact if declined: Users using Android's stock gesture keyboard or the TouchPal gesture keyboard will still need to manually enter spaces when "swyping" words in the address bar.
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky): Low risk. These patches just add Android's stock keyboard and TouchPal to the list of gesture keyboards. The consequence of being in this list only changes the keyboards' "text or URL?" mode which controls which keys are displayed and whether to auto-insert spaces between words.
String or IDL/UUID changes made by this patch: N/A

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3ddd71814580
https://hg.mozilla.org/releases/mozilla-aurora/rev/201d7b638887
https://hg.mozilla.org/releases/mozilla-aurora/rev/da1b15adb6a0
https://hg.mozilla.org/releases/mozilla-aurora/rev/b0fd16fe3162