1004073 - Refactor EventDispatcher usages

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

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().

Comment 1

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

Attachment: a. Prepare for refactoring (v1)

Switch to GeckoAppShell.EVENT_DISPATCHER and make registerGeckoThreadListener public.

Comment 2

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

Attachment: b. Refactor EventDispatcher usages in robocop (v1)

Comment 3

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

Attachment: c. Refactor EventDispatcher usages in org.mozilla.gecko (v1)

Comment 4

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

Attachment: d. Refactor EventDispatcher usages in org.mozilla.gecko.gfx (v1)

Comment 5

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

Attachment: e. Refactor EventDispatcher usages in org.mozilla.gecko.health (v1)

Comment 6

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

Attachment: f. Refactor EventDispatcher usages in org.mozilla.gecko.home (v1)

Comment 7

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

Attachment: g. Refactor EventDispatcher usages in org.mozilla.gecko.preferences (v1)

Comment 8

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

Attachment: h. Refactor EventDispatcher usages in org.mozilla.gecko.prompts (v1)

Comment 9

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

Attachment: i. Refactor EventDispatcher usages in org.mozilla.gecko.toolbar (v1)

Comment 10

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

Attachment: j. Refactor EventDispatcher usages in org.mozilla.gecko.webapp (v1)

Comment 11

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

Attachment: k. Remove now unused methods (v1)

After all the refactoring patches, we can remove the methods that are now unused.

Comment 12

[Richard Newman [:rnewman]]

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.

Comment 13

[:Margaret Leibovic]

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 :)

Comment 14

[Mark Finkle (:mfinkle) (use needinfo?)]

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.

Comment 15

[Mark Finkle (:mfinkle) (use needinfo?)]

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.

Comment 16

[Wesley Johnston (:wesj)]

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?)

Comment 17

[Richard Newman [:rnewman]]

(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 18

[Lucas Rocha (:lucasr)]

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.

Comment 19

[Lucas Rocha (:lucasr)]

(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 :-)

Comment 20

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

EventDispatcher.getInstance() sounds reasonable.

Comment 21

[Wesley Johnston (:wesj)]

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.

Comment 22

[Martyn Haigh (:mhaigh)]

(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.

Comment 23

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

Attachment: Refactor EventDispatcher usages (v2)

Roll up patch with change from GeckoAppShell.EVENT_DISPATCHER to EventDispatcher.getInstance()

Comment 24

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

Let's see if it sticks,

https://hg.mozilla.org/integration/fx-team/rev/36853e536d68

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/36853e536d68