Closed Bug 725538 Opened 9 years ago Closed 9 years ago

make creating GeckoEvents sane

Categories

(Firefox for Android Graveyard :: General, defect)

x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 13

People

(Reporter: blassey, Assigned: blassey)

Details

Attachments

(1 file, 2 obsolete files)

Attached patch patch (obsolete) — Splinter Review
No description provided.
Attachment #595586 - Flags: review?(bugmail.mozilla)
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"
Attachment #595586 - Flags: review?(bugmail.mozilla) → review+
Attached patch patch (obsolete) — Splinter Review
Kats, thanks for the review. I rushed that patch up while getting on the plane, here is the more complete version.
Assignee: nobody → blassey.bugs
Attachment #595586 - Attachment is obsolete: true
Attachment #595662 - Flags: review?(bugmail.mozilla)
Attached patch patchSplinter Review
Attachment #595662 - Attachment is obsolete: true
Attachment #595662 - Flags: review?(bugmail.mozilla)
Attachment #595665 - Flags: review?(bugmail.mozilla)
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.
Attachment #595665 - Flags: review?(bugmail.mozilla) → review+
Init functions eliminate a whole bunch of "event." where menu fields need to be set
https://hg.mozilla.org/mozilla-central/rev/c42f64194c66
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → Firefox 13
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.