Closed Bug 1004073 Opened 10 years ago Closed 10 years ago

Refactor EventDispatcher usages

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 32

People

(Reporter: jchen, Assigned: jchen)

References

Details

Attachments

(1 file, 11 obsolete files)

103.49 KB, patch
jchen
: review+
Details | Diff | Splinter Review
Right now different code use EventDispatcher in different ways, through GeckoAppShell.registerEventListener() and GeckoAppShell.getEventDispatcher().registerEventListener(). We should refactor them to all use GeckoAppShell.EVENT_DISPATCHER.registerGeckoThreadListener().
Attached patch a. Prepare for refactoring (v1) (obsolete) — Splinter Review
Switch to GeckoAppShell.EVENT_DISPATCHER and make registerGeckoThreadListener public.
Attachment #8415557 - Flags: review?(mark.finkle)
After all the refactoring patches, we can remove the methods that are now unused.
Attachment #8415571 - Flags: review?(mark.finkle)
Comment on attachment 8415563 [details] [diff] [review]
e. Refactor EventDispatcher usages in org.mozilla.gecko.health (v1)

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

Looks mechanical to me.

::: mobile/android/base/health/BrowserHealthReporter.java
@@ +41,5 @@
>  
>      protected final Context context;
>  
>      public BrowserHealthReporter() {
> +        GeckoAppShell.EVENT_DISPATCHER.registerGeckoThreadListener(this, EVENT_REQUEST);

Use GeckoAppShell.getEventDispatcher().

@@ +50,5 @@
>          }
>      }
>  
>      public void uninit() {
> +        GeckoAppShell.EVENT_DISPATCHER.unregisterGeckoThreadListener(this, EVENT_REQUEST);

Same.
Attachment #8415563 - Flags: review?(rnewman) → review+
Attachment #8415558 - Flags: review?(gbrown) → review+
Comment on attachment 8415564 [details] [diff] [review]
f. Refactor EventDispatcher usages in org.mozilla.gecko.home (v1)

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

LGTM, assuming the new APIs you're using exist :)
Attachment #8415564 - Flags: review?(margaret.leibovic) → review+
Attachment #8415565 - Flags: review?(margaret.leibovic) → review+
Attachment #8415557 - Flags: review?(mark.finkle) → review+
Comment on attachment 8415559 [details] [diff] [review]
c. Refactor EventDispatcher usages in org.mozilla.gecko (v1)

Any reason you are using GeckoAppShell.EVENT_DISPATCHER and not GeckoAppShell.getEventDispatcher() ?

Looks like everything is handled correctly.
Attachment #8415559 - Flags: review?(mark.finkle) → review+
Comment on attachment 8415571 [details] [diff] [review]
k. Remove now unused methods (v1)

OK, I see you are removing getEventDispatcher() in this patch. Think about whether we should keep it and use it. It might be a useful level of redirection and Proguard should inline it for us.
Attachment #8415571 - Flags: review?(mark.finkle) → review+
I'd really like GeckoAppShell to die if possible. If we're revamping these, any reason not to just use EventDispatcher.getInstance()? (or just make everything in EventDispatcher static?)
(In reply to Wesley Johnston (:wesj) from comment #16)
> I'd really like GeckoAppShell to die if possible. If we're revamping these,
> any reason not to just use EventDispatcher.getInstance()? (or just make
> everything in EventDispatcher static?)

+1 to EventDispatcher.getInstance().

(-1 to GeckoAppShell.EVENT_DISPATCHER! See Comment 12.)
Comment on attachment 8415568 [details] [diff] [review]
i. Refactor EventDispatcher usages in org.mozilla.gecko.toolbar (v1)

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

Looks good. I'm not a fan of the GeckoShellApp.EVENT_DISPATCHER API but I guess this is part of the wider API change discussion happening here.
Attachment #8415568 - Flags: review?(lucasr.at.mozilla) → review+
(In reply to Richard Newman [:rnewman] from comment #17)
> (In reply to Wesley Johnston (:wesj) from comment #16)
> > I'd really like GeckoAppShell to die if possible. If we're revamping these,
> > any reason not to just use EventDispatcher.getInstance()? (or just make
> > everything in EventDispatcher static?)
> 
> +1 to EventDispatcher.getInstance().
> 
> (-1 to GeckoAppShell.EVENT_DISPATCHER! See Comment 12.)

What wesj and rnewman said :-)
Attachment #8415561 - Flags: review?(snorp) → review+
EventDispatcher.getInstance() sounds reasonable.
Comment on attachment 8415567 [details] [diff] [review]
h. Refactor EventDispatcher usages in org.mozilla.gecko.prompts (v1)

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

+ with the move to EventDispatcher.getInstance().

Sidenote: I was using this the other day and hit issues passing a JSONArray to java. I glanced at the API and didn't see anyway to index into the array. I assume we'll need something like that before we really kill off the old API.
Attachment #8415567 - Flags: review?(wjohnston) → review+
(In reply to Wesley Johnston (:wesj) from comment #21)
> Comment on attachment 8415567 [details] [diff] [review]
> h. Refactor EventDispatcher usages in org.mozilla.gecko.prompts (v1)
> 
> Review of attachment 8415567 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> + with the move to EventDispatcher.getInstance().
> 
> Sidenote: I was using this the other day and hit issues passing a JSONArray
> to java. I glanced at the API and didn't see anyway to index into the array.
> I assume we'll need something like that before we really kill off the old
> API.

+1 to EventDispatcher.getInstance() and aiming to get rid of GeckoAppShell.
Attachment #8415569 - Flags: review?(mhaigh) → review+
Roll up patch with change from GeckoAppShell.EVENT_DISPATCHER to EventDispatcher.getInstance()
Attachment #8415557 - Attachment is obsolete: true
Attachment #8415558 - Attachment is obsolete: true
Attachment #8415559 - Attachment is obsolete: true
Attachment #8415561 - Attachment is obsolete: true
Attachment #8415563 - Attachment is obsolete: true
Attachment #8415564 - Attachment is obsolete: true
Attachment #8415565 - Attachment is obsolete: true
Attachment #8415567 - Attachment is obsolete: true
Attachment #8415568 - Attachment is obsolete: true
Attachment #8415569 - Attachment is obsolete: true
Attachment #8415571 - Attachment is obsolete: true
Attachment #8416652 - Flags: review+
Let's see if it sticks,

https://hg.mozilla.org/integration/fx-team/rev/36853e536d68
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/36853e536d68
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 32
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.