725538 - make creating GeckoEvents sane

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 595586 [details] [diff] [review]
patch

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

I'd also like to see the event constants in GeckoEvent.java flipped from public to private, and the public GeckoEvent(int) constructor also made private. All event creations should be done through these static methods. r+ with that done.

::: mobile/android/base/GeckoAppShell.java
@@ +146,4 @@
>      private static native void reportJavaCrash(String stackTrace);
>  
>      public static void notifyUriVisited(String uri) {
> +        sendEventToGecko(GeckoEvent.createVisitedEvent( uri));

nit: space between "(" and "uri"

Comment 2

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

Kats, thanks for the review. I rushed that patch up while getting on the plane, here is the more complete version.

Comment 3

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 595665 [details] [diff] [review]
patch

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

Looks pretty good overall. Some nits and stuff but no real problems.

::: mobile/android/base/AutoCompletePopup.java
@@ +182,4 @@
>      public void hide() {
>          if (isShown()) {
>              setVisibility(View.GONE);
> +            GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("FormAssist:Closed", null));

We could add a createBroadcastEvent(String) overload as well, so the extra null parameter isn't needed.

::: mobile/android/base/AwesomeBar.java
@@ +230,4 @@
>          registerForContextMenu(mAwesomeTabs.findViewById(R.id.history_list));
>  
>          GeckoAppShell.registerGeckoEventListener("SearchEngines:Data", this);
> +        GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("SearchEngines:Get", null));

Ditto

::: mobile/android/base/ConfirmPreference.java
@@ +72,4 @@
>          } else if ("clear_private_data".equalsIgnoreCase(mAction)) {
>              GeckoAppShell.getHandler().post(new Runnable(){
>                  public void run() {
> +                    GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("Sanitize:ClearAll", null));

Ditto (and in a bunch more places..)

::: mobile/android/base/GeckoApp.java
@@ +2535,2 @@
>                                            "{\"ok\": true,  \"path\": \"" + file.getPath() + "\" }" :
>                                            "{\"ok\": false, \"path\": \"" + file.getPath() + "\" }");

Sure hope android doesn't allow file paths with quotes in them...

::: mobile/android/base/GeckoAppShell.java
@@ +146,4 @@
>      private static native void reportJavaCrash(String stackTrace);
>  
>      public static void notifyUriVisited(String uri) {
> +        sendEventToGecko(GeckoEvent.createVisitedEvent( uri));

nit: space between "(" and "uri"

::: mobile/android/base/GeckoEvent.java
@@ +259,5 @@
>      }
>  
> +    public static GeckoEvent createSensorEvent(SensorEvent s) {
> +        GeckoEvent event = new GeckoEvent(INVALID);
> +        event.initSensorEvent(s);

If you inline initSensorEvent() here, you can get rid of the INVALID constant, and also make mType final, both of which are Good Things (TM). Making mType final will allow the compiler to catch paths where it doesn't get initialized, or where the caller clobbers it for whatever reason.

Also, I'm not really sure why some of these have init functions and others do not. It only makes sense to have init functions where there are multiple functions that reuse that code that can't be collapsed.

@@ +308,3 @@
>      }
>  
>      private void InitIMERange(int action, int offset, int count,

InitIMERange now has a mType = IME_EVENT line that can be taken out. Or...

@@ +328,5 @@
> +        GeckoEvent event = new GeckoEvent(IME_EVENT);
> +        event.InitIMERange(IME_SET_TEXT, offset, count, rangeType, rangeStyles,
> +                           rangeForeColor, rangeBackColor);
> +        event.mCharacters = text;
> +        return event;

... you could replace the first two lines of this function with a call to the other createIMERangeEvent. That would allow inlining the InitIMERange function, which might be a bit cleaner.

Comment 5

[Brad Lassey [:blassey] (use needinfo?)]

Init functions eliminate a whole bunch of "event." where menu fields need to be set

Comment 6

[Brad Lassey [:blassey] (use needinfo?)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c42f64194c66

Comment 7

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/mozilla-central/rev/c42f64194c66