778468 - Move classes into new namespaces to untangle org.mozilla.gecko class dependencies

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect, P2)

Comment 1

[Chris Peterson [:cpeterson]]

Attachment: part-1-create-toolkit-package.patch

Part 1: Move some utility classes to new org.mozilla.gecko.toolkit package.

If "toolkit" is not the dumping ground for generic utility code that I think it is, I can rename the "org.mozilla.gecko.toolkit" package to something like "org.mozilla.gecko.util".

Comment 2

[Chris Peterson [:cpeterson]]

Attachment: part-2-rename-GeckoJarReader-JarUtils.patch

Part 2: Rename GeckoJarReader to JarUtils.

Comment 3

[Chris Peterson [:cpeterson]]

Attachment: part-3-rename-GeckoBackgroundThread-BackgroundThread.patch

Part 3: Rename GeckoBackgroundThread to BackgroundThread.

Comment 4

[Chris Peterson [:cpeterson]]

Attachment: part-4-rename-GeckoAsyncTask-BackgroundTask.patch

Part 4: Rename GeckoAsyncTask to BackgroundTask (and remove unused Progress parameter).

Comment 5

[Chris Peterson [:cpeterson]]

Attachment: part-5-compile-toolkit-jar.patch

Part 5: Compile browser, toolkit, and sync into separate jar files (and fix deprecation warnings).

khuey, I was asked to run all Makefile changes by a build reviewer.

This patch compiles some Java .class files into separate .jar files to enforce inter-package dependencies at compile-time. In a later bug, I will move each package's sources into separate directories with their own Makefiles.

Comment 6

[Chris Peterson [:cpeterson]]

Attachment: part-6-util-FloatUtils.patch

Part 6: Move FloatUtils to org.mozilla.gecko.util package.

Comment 7

[Chris Peterson [:cpeterson]]

Comment on attachment 647772 [details] [diff] [review]
part-6-util-FloatUtils.patch

I'm moving this patch to bug 779366.

Comment 8

[Chris Peterson [:cpeterson]]

mfinkle, on further reflection, I think a `org.mozilla.gecko.util` package seems like a more appropriate home for these classes than `org.mozilla.gecko.toolkit`. I've changed the namespace name in my local patches.

Comment 9

[Chris Peterson [:cpeterson]]

Comment on attachment 646886 [details] [diff] [review]
part-1-create-toolkit-package.patch

Cancelling review because this patch has been obsoleted by my local changes.

Comment 10

[Chris Peterson [:cpeterson]]

Comment on attachment 646887 [details] [diff] [review]
part-2-rename-GeckoJarReader-JarUtils.patch

Cancelling review because this patch has been obsoleted by my local changes.

Comment 11

[Chris Peterson [:cpeterson]]

Comment on attachment 646888 [details] [diff] [review]
part-3-rename-GeckoBackgroundThread-BackgroundThread.patch

Cancelling review because this patch has been obsoleted by my local changes.

Comment 12

[Chris Peterson [:cpeterson]]

Comment on attachment 646889 [details] [diff] [review]
part-4-rename-GeckoAsyncTask-BackgroundTask.patch

Cancelling review because this patch has been obsoleted by my local changes.

Comment 13

[Chris Peterson [:cpeterson]]

Comment on attachment 646890 [details] [diff] [review]
part-5-compile-toolkit-jar.patch

Cancelling review because this patch has been obsoleted by my local changes.

Comment 14

[Chris Peterson [:cpeterson]]

Attachment: part-1-util-ActivityResultHandler.patch

Part 1: Move ActivityResultHandler to org.mozilla.gecko.util package.

Comment 15

[Chris Peterson [:cpeterson]]

Attachment: part-2-util-INIParser.patch

Part 2: Move INIParser and INISection to org.mozilla.gecko.util package.

Comment 16

[Chris Peterson [:cpeterson]]

Attachment: part-3-util-GeckoJarReader.patch

Part 3: Move GeckoJarReader to org.mozilla.gecko.util package.

Comment 17

[Chris Peterson [:cpeterson]]

Attachment: part-4-util-GeckoBackgroundThread.patch

Part 4: Move GeckoBackgroundThread to org.mozilla.gecko.util package.

Comment 18

[Chris Peterson [:cpeterson]]

Attachment: part-5-util-GeckoAsyncTask.patch

Part 5: Move GeckoAsyncTask to org.mozilla.gecko.util package.

Comment 19

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

Comment on attachment 648457 [details] [diff] [review]
part-5-util-GeckoAsyncTask.patch

Is there any way around passing GeckoApp.mAppContext and GeckoAppShell.getHandler() into every call. I thought we were trying to remove uses of mAppContext.

r+ for the move, but we might need a followup to do more cleaning/sanitizing.

Comment 20

[Sriram Ramasubramanian [:sriram]]

(In reply to Mark Finkle (:mfinkle) from comment #19)
> Comment on attachment 648457 [details] [diff] [review]
> part-5-util-GeckoAsyncTask.patch
> 
> Is there any way around passing GeckoApp.mAppContext and
> GeckoAppShell.getHandler() into every call. I thought we were trying to
> remove uses of mAppContext.
> 
> r+ for the move, but we might need a followup to do more cleaning/sanitizing.

Couple of comments:
1. mAppContext on GeckoApp is fine -- as it is its own member.
2. GeckoAsyncTask taking activity as param is good -- now its decoupled from GeckoApp.mAppContext.
3. AwesomeBar is its own activity. Why not pass AwesomeBar.this as the parameter to GeckoAsyncTask? Something that needs to be run on AwesomeBar's UI/Background thread doesn't need to know about GeckoApp.mAppContext.
4. Tab and TabsAccessor are tricky. Probably each tab should hold a reference to the activity. There by we can decouple it. I don't want it to have access to GeckoApp.mAppContext.

We should find a way to avoid dependency of Activity in Tab, Tabs, TabsAccessor -- and other standalone files. The better way would be that the activity implements interface of these and register itself with these classes. Then these classes doesn't need to know if its the activity or something else that's interested.

Comment 21

[Sriram Ramasubramanian [:sriram]]

Untangling a bit more..

 mActivity.runOnUiThread(new Runnable() { ... }); in GeckoAsyncTask

Ideally GeckoAsyncTask doesn't need to know about the activity, if its used only for a small background task. So, this can have a null check for mActivity, there by, I can do a small background task with the GeckoAsyncTask.

This will be helpful in Tab.java (and others). So,

updateBookmark() {
  ...
  (new GeckoAsyncTask(null, ...) {
     doInBackground() {
       ...
     }
   }).execute();
}

If it really wants a notification after completion, then GeckoAsyncTask will be like..

GeckoAsyncTask(args-list) {
  setUIHandler() {
    mUIHandler = GeckoAsyncTask.UIHandler;
  }

  doInBackground() { ... }

  run() {
    if (mUIHandler) { mUIHandler.notifyProcessDone(); }
}

and GeckoAsyncTask.UIHandler is just and interface.

This will help anything -- that's not an activity -- to make use of GeckoAsyncTask. Say, a BrowserToolbar wants to use GeckoAsyncTask,

  (new GeckoAsyncTask(args) {
    doInBackground() { ... }
  }).setUIHandler(new GeckoAsyncTask.UIHandler() {
     notifyProcessDone() { .. browser-toolbar's-ui-tasks }
  }).execute();

... aaahh.. that's finally Javascript! :(

Comment 22

[Chris Peterson [:cpeterson]]

Sriram, I like your ideas about GeckoAsyncTask. I opened bug 780276 with your suggestions.

Cleaning up our Java dependencies is a multiphase project. In this bug, I'm only trying to change code that affects which namespaces talk to which other namespaces.

Comment 23

[Chris Peterson [:cpeterson]]

(In reply to Sriram Ramasubramanian [:sriram] from comment #20)
> 3. AwesomeBar is its own activity. Why not pass AwesomeBar.this as the
> parameter to GeckoAsyncTask? Something that needs to be run on AwesomeBar's
> UI/Background thread doesn't need to know about GeckoApp.mAppContext.
> 4. Tab and TabsAccessor are tricky. Probably each tab should hold a
> reference to the activity. There by we can decouple it. I don't want it to
> have access to GeckoApp.mAppContext.

Sriram, do you think I should make these changes in this patch?

I was trying to preserve the original code's behavior while making the inter-package dependencies unidirectional (i.e. pass mAppContext as a parameter instead of calling GeckoAppShell.mAppContext from GeckoAsyncTask).

Comment 24

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

(In reply to Chris Peterson (:cpeterson) from comment #23)
> (In reply to Sriram Ramasubramanian [:sriram] from comment #20)
> > 3. AwesomeBar is its own activity. Why not pass AwesomeBar.this as the
> > parameter to GeckoAsyncTask? Something that needs to be run on AwesomeBar's
> > UI/Background thread doesn't need to know about GeckoApp.mAppContext.
> > 4. Tab and TabsAccessor are tricky. Probably each tab should hold a
> > reference to the activity. There by we can decouple it. I don't want it to
> > have access to GeckoApp.mAppContext.
> 
> Sriram, do you think I should make these changes in this patch?
> 
> I was trying to preserve the original code's behavior while making the
> inter-package dependencies unidirectional (i.e. pass mAppContext as a
> parameter instead of calling GeckoAppShell.mAppContext from GeckoAsyncTask).

I don't think so. Followups are fine IMO. Some of the possible changes, like #4, we might not want to impl as suggested.

Comment 25

[Chris Peterson [:cpeterson]]

Attachment: part-6-inject-InputConnectionHandler-dependency.patch

Part 6: Inject LayerView's InputConnectionHandler to remove GeckoInputConnection dependency.

Comment 26

[Chris Peterson [:cpeterson]]

Attachment: part-7-util-EventListener.patch

Part 7: Move GeckoEventListener to org.mozilla.gecko.util package.

This patch set is just fixing the namespace references. I plan to move the files to a separate package directory and rename GeckoEventListener class to just "EventListener" in a later patch.

Comment 27

[Chris Peterson [:cpeterson]]

Attachment: part-8-util-EventDispatcher.patch

Part 8: Add EventDispatcher to remove gfx dependency on GeckoAppShell event registration.

Comment 28

[Chris Peterson [:cpeterson]]

Attachment: part-9-more-EventListeners.patch

Part 9: Use EventDispatcher to remove ui dependency on GeckoAppShell event registration.

Comment 29

[Chris Peterson [:cpeterson]]

Attachment: part-9-more-EventListeners-v2.patch

part 9 v2: GeckoAppShell.registerGeckoEventListener() method must remain public because it is accessed from Robocop package.

We could probably make Robocop more robust if we had Gecko classes dedicated to  providing a stable API to thunk Robocop's dynamic class loader to GeckoAppShell and friends. Robocop currently relies on fragile implementation details of some Gecko method signatures that may change because they are used by other Gecko classes. Gecko signature changes would break the Robocop thunking classes at Fennec compile-time, not when running Robocop tests on the tryserver.

Comment 30

[Chris Peterson [:cpeterson]]

Attachment: part-10-even-more-EventListeners.patch

Part 10: Make all event registration go through EventDispatcher (except Robocop's dynamic class loader).

Comment 31

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 648961 [details] [diff] [review]
part-8-util-EventDispatcher.patch

mLayerClient = new GeckoLayerClient(this, GeckoAppShell.getEventDispatcher());

This should probably go into the constructor. Since GeckoAppShell is a static class, this can be set up inside the GeckoLayerClient's constructor and it would be clean.

Comment 32

[Sriram Ramasubramanian [:sriram]]

Couple of suggestions regarding the EventDispatcher:

1. I like the idea of moving the EventDispatcher out of GeckoAppShell. This is a clean approach :). And GeckoAppShell holding a reference to it is good.

2. I don't think there are going to be many EventDispatcher, unless we are going to support different types of events. I would suggest making it singleton -- and if we ever change our mind, we would change it in only one place -- GeckoAppShell.

3. I somehow don't like everyone knowing about GeckoAppShell's event dispatcher. The decoupling kind of fails there. GeckoAppShell should delegate the calls to it's event dispatcher.

GeckoAppShell.register("event", this); ---> mEventDispatcher.register("event", listener);

is a better approach than,

GeckoAppShell.getEventDispatcher().register("event", this);

So, this would make every constructor not take the addition argument. And it aligns with things like "TabsTray extends LinearLayout implements GeckoEventListener" whose constructor don't need it.

4. While making this change, please add a HashMap to EventDispatcher. EventDispatcher can hash all the strings, and expose the event names as a constant integer -- which is easier for switch cases. Also, when a new message comes in, it can search the hashmap and return this integer. Thereby, there is just "one place" for strings, and looks better. Also, EventDispatcher can additionally do this,

registerEventListener(int[] events, GEL listener);

Thereby we don't have to do registerEventListener too many times. We can pass in an array of event (integers), and the registerEventListener can loop through it and add it.

Comment 33

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

(In reply to Chris Peterson (:cpeterson) from comment #29)
> We could probably make Robocop more robust if we had Gecko classes dedicated
> to  providing a stable API to thunk Robocop's dynamic class loader to
> GeckoAppShell and friends. Robocop currently relies on fragile
> implementation details of some Gecko method signatures that may change
> because they are used by other Gecko classes. Gecko signature changes would
> break the Robocop thunking classes at Fennec compile-time, not when running
> Robocop tests on the tryserver.

+1. We should create a RobocopAPI class which is the only thing Robocop is allowed to access via reflection, and that can reach into the rest of the code as needed. Feel free to create a bug for that and throw it at me if you want.

Comment 34

[Chris Peterson [:cpeterson]]

(In reply to Kartikaya Gupta (:kats) (PTO aug 3) from comment #33)
> +1. We should create a RobocopAPI class which is the only thing Robocop is
> allowed to access via reflection, and that can reach into the rest of the
> code as needed. Feel free to create a bug for that and throw it at me if you
> want.

Kats, I created RobocopAPI bug 780650 for you. :)

Comment 35

[Chris Peterson [:cpeterson]]

(In reply to Sriram Ramasubramanian [:sriram] from comment #32)
> Couple of suggestions regarding the EventDispatcher:


> 2. I don't think there are going to be many EventDispatcher, unless we are
> going to support different types of events. I would suggest making it
> singleton -- and if we ever change our mind, we would change it in only one
> place -- GeckoAppShell.

There is only one instance of EventDispatcher; it's a member of GeckoAppShell. Singletons are just another way of saying static global variables. I thought you wanted to get rid of static variables because of the problems they caused with Activity lifetimes, Sriram? <:)

There is no reason we could not have multiple EventDispatchers. In fact, I would prefer a separate EventDispatcher for each event source. Then, event listeners could just register Runnable-like callbacks that are called directly for individual events. Our event listeners would not need to implement giant handleMessage() methods and its String parsing overhead.

Multiple event dispatchers is the event style used by JavaScript's EventTargets and C#'s Events/Delegates. When I created EventDispatcher.java, I debated naming it EventTarget.java and modeling it more closely on JavaScript's EventTarget.addEventListener() API. I still prefer that approach, but I wanted this (already big) patch to just move code with minimal changes to API and implementation.

https://developer.mozilla.org/en-US/docs/DOM/EventTarget


> 3. I somehow don't like everyone knowing about GeckoAppShell's event
> dispatcher. The decoupling kind of fails there. GeckoAppShell should
> delegate the calls to it's event dispatcher.
> 
> GeckoAppShell.register("event", this); --->
> mEventDispatcher.register("event", listener);
> 
> is a better approach than,
> 
> GeckoAppShell.getEventDispatcher().register("event", this);
> 
> So, this would make every constructor not take the addition argument. And it
> aligns with things like "TabsTray extends LinearLayout implements
> GeckoEventListener" whose constructor don't need it.

I think your first example more tightly couples the event listener and GeckoAppShell. The second example separates event registration from the owner of the particular EventDispatcher.

In the second example, the event listener registers directly with a EventDispatcher, which _just happens_ to come from GeckoAppShell for now. Changing the EventDispatcher to a method parameter or a member variable would be easier in the second example.

That said, I am not strongly attached to one approach or the other..


> 4. While making this change, please add a HashMap to EventDispatcher.
> EventDispatcher can hash all the strings, and expose the event names as a
> constant integer -- which is easier for switch cases. Also, when a new
> message comes in, it can search the hashmap and return this integer.
> Thereby, there is just "one place" for strings, and looks better. Also,
> EventDispatcher can additionally do this,
> 
> registerEventListener(int[] events, GEL listener);
> 
> Thereby we don't have to do registerEventListener too many times. We can
> pass in an array of event (integers), and the registerEventListener can loop
> through it and add it.

EventDispatcher's current implementation is just a cut/paste/encapsulate from GeckoAppShell. With this patch, I just wanted to move code with minimal changes to semantics and implementation. As I described above, if we had a separate EventDispatcher for each event, we would not need handleMessage() or any String switching.

Comment 36

[Sriram Ramasubramanian [:sriram]]

(In reply to Chris Peterson (:cpeterson) from comment #35)
> (In reply to Sriram Ramasubramanian [:sriram] from comment #32)
> > Couple of suggestions regarding the EventDispatcher:
> 
> 
> > 2. I don't think there are going to be many EventDispatcher, unless we are
> > going to support different types of events. I would suggest making it
> > singleton -- and if we ever change our mind, we would change it in only one
> > place -- GeckoAppShell.
> 
> There is only one instance of EventDispatcher; it's a member of
> GeckoAppShell. Singletons are just another way of saying static global
> variables. I thought you wanted to get rid of static variables because of
> the problems they caused with Activity lifetimes, Sriram? <:)
> 
> There is no reason we could not have multiple EventDispatchers. In fact, I
> would prefer a separate EventDispatcher for each event source. Then, event
> listeners could just register Runnable-like callbacks that are called
> directly for individual events. Our event listeners would not need to
> implement giant handleMessage() methods and its String parsing overhead.
> 
> Multiple event dispatchers is the event style used by JavaScript's
> EventTargets and C#'s Events/Delegates. When I created EventDispatcher.java,
> I debated naming it EventTarget.java and modeling it more closely on
> JavaScript's EventTarget.addEventListener() API. I still prefer that
> approach, but I wanted this (already big) patch to just move code with
> minimal changes to API and implementation.
> 
> https://developer.mozilla.org/en-US/docs/DOM/EventTarget

I love that approach and I started off with that approach 9 months back. However, at that time we weren't sure of how many events we would get and was asked to make it generic with a handleMessage(), instead of onTabLoaded(). I am not sure if now is the right time to do it. Mark?

> 
> 
> > 3. I somehow don't like everyone knowing about GeckoAppShell's event
> > dispatcher. The decoupling kind of fails there. GeckoAppShell should
> > delegate the calls to it's event dispatcher.
> > 
> > GeckoAppShell.register("event", this); --->
> > mEventDispatcher.register("event", listener);
> > 
> > is a better approach than,
> > 
> > GeckoAppShell.getEventDispatcher().register("event", this);
> > 
> > So, this would make every constructor not take the addition argument. And it
> > aligns with things like "TabsTray extends LinearLayout implements
> > GeckoEventListener" whose constructor don't need it.
> 
> I think your first example more tightly couples the event listener and
> GeckoAppShell. The second example separates event registration from the
> owner of the particular EventDispatcher.
> 
> In the second example, the event listener registers directly with a
> EventDispatcher, which _just happens_ to come from GeckoAppShell for now.
> Changing the EventDispatcher to a method parameter or a member variable
> would be easier in the second example.
> 
> That said, I am not strongly attached to one approach or the other..

My reason for the first approach is, the event listeners are tied to GeckoAppShell, and other modules -- basically UI ones, like GeckoApp or FormAssistantPopup, shouldn't really know what's happening under the hood. Hence, they should register with GeckoAppShell. GeckoAppShell can delegate it to an instance of EventDispatcher. That way, we don't have the need for 

GeckoAppShell.getEventDispatcher();

in many places in our code. Since GeckoAppShell is the entry point from Gecko, for all Gecko messages, we are not going to change that approach. Even if we move to many EventDispatchers approach, who will hold them? I think, GeckoAppShell should hide this member variable, and just allow other parts of the code to register with it, and it should delegate it to appropriate EventDispatcher (currently just one).

> 
> 
> > 4. While making this change, please add a HashMap to EventDispatcher.
> > EventDispatcher can hash all the strings, and expose the event names as a
> > constant integer -- which is easier for switch cases. Also, when a new
> > message comes in, it can search the hashmap and return this integer.
> > Thereby, there is just "one place" for strings, and looks better. Also,
> > EventDispatcher can additionally do this,
> > 
> > registerEventListener(int[] events, GEL listener);
> > 
> > Thereby we don't have to do registerEventListener too many times. We can
> > pass in an array of event (integers), and the registerEventListener can loop
> > through it and add it.
> 
> EventDispatcher's current implementation is just a cut/paste/encapsulate
> from GeckoAppShell. With this patch, I just wanted to move code with minimal
> changes to semantics and implementation. As I described above, if we had a
> separate EventDispatcher for each event, we would not need handleMessage()
> or any String switching.

Like I said, I'll leave to Mark to decide on that :)
If we still want a handleMessage() approach, I would suggest HashMap (as a followup), to make things better and faster.

Comment 37

[Sriram Ramasubramanian [:sriram]]

> > 
> > > 3. I somehow don't like everyone knowing about GeckoAppShell's event
> > > dispatcher. The decoupling kind of fails there. GeckoAppShell should
> > > delegate the calls to it's event dispatcher.
> > > 
> > > GeckoAppShell.register("event", this); --->
> > > mEventDispatcher.register("event", listener);
> > > 
> > > is a better approach than,
> > > 
> > > GeckoAppShell.getEventDispatcher().register("event", this);
> > > 
> > > So, this would make every constructor not take the addition argument. And it
> > > aligns with things like "TabsTray extends LinearLayout implements
> > > GeckoEventListener" whose constructor don't need it.
> > 
> > I think your first example more tightly couples the event listener and
> > GeckoAppShell. The second example separates event registration from the
> > owner of the particular EventDispatcher.
> > 
> > In the second example, the event listener registers directly with a
> > EventDispatcher, which _just happens_ to come from GeckoAppShell for now.
> > Changing the EventDispatcher to a method parameter or a member variable
> > would be easier in the second example.
> > 
> > That said, I am not strongly attached to one approach or the other..
> 
> My reason for the first approach is, the event listeners are tied to
> GeckoAppShell, and other modules -- basically UI ones, like GeckoApp or
> FormAssistantPopup, shouldn't really know what's happening under the hood.
> Hence, they should register with GeckoAppShell. GeckoAppShell can delegate
> it to an instance of EventDispatcher. That way, we don't have the need for 
> 
> GeckoAppShell.getEventDispatcher();
> 
> in many places in our code. Since GeckoAppShell is the entry point from
> Gecko, for all Gecko messages, we are not going to change that approach.
> Even if we move to many EventDispatchers approach, who will hold them? I
> think, GeckoAppShell should hide this member variable, and just allow other
> parts of the code to register with it, and it should delegate it to
> appropriate EventDispatcher (currently just one).
> 

Let's say we stick to handleMessage() now and later we want to support,
GeckoAppShell.registerTabEvent(this);
or
GeckoAppShell.register("Tab:Event", this);

which is handled by a EventDispatcher instance that handles _only_ Tab events.

By exposing GeckoAppShell.getEventDispatcher(), we are making things harder, and making more changes. With my approach, it's still the same. The logic under GeckoAppShell changes, based on the name of the event, and it automagically registers the listener to one of its EventDispatchers. Hence getEventDispatcher() is not exposed.

Comment 38

[Chris Peterson [:cpeterson]]

That makes sense. I will update my local patches to make GeckoAppShell's EventDispatcher private.

Comment 39

[Chris Peterson [:cpeterson]]

(In reply to Sriram Ramasubramanian [:sriram] from comment #31)
> Comment on attachment 648961 [details] [diff] [review]
> part-8-util-EventDispatcher.patch
> 
> mLayerClient = new GeckoLayerClient(this,
> GeckoAppShell.getEventDispatcher());
> 
> This should probably go into the constructor. Since GeckoAppShell is a
> static class, this can be set up inside the GeckoLayerClient's constructor
> and it would be clean.

I am trying to make org.mozilla.gecko.gfx a standalone package with no dependencies on GeckoAppShell and friends. Thus, GeckoLayerClient should not call GeckoAppShell and I inject the EventDispatcher dependency as a constructor parameter.

Comment 40

[Sriram Ramasubramanian [:sriram]]

(In reply to Chris Peterson (:cpeterson) from comment #39)
> (In reply to Sriram Ramasubramanian [:sriram] from comment #31)
> > Comment on attachment 648961 [details] [diff] [review]
> > part-8-util-EventDispatcher.patch
> > 
> > mLayerClient = new GeckoLayerClient(this,
> > GeckoAppShell.getEventDispatcher());
> > 
> > This should probably go into the constructor. Since GeckoAppShell is a
> > static class, this can be set up inside the GeckoLayerClient's constructor
> > and it would be clean.
> 
> I am trying to make org.mozilla.gecko.gfx a standalone package with no
> dependencies on GeckoAppShell and friends. Thus, GeckoLayerClient should not
> call GeckoAppShell and I inject the EventDispatcher dependency as a
> constructor parameter.

Oh wait for a second then. Me and Kats were talking about making mLayerView available in GeckoApp and it's layer controller initialized in its constructor. Which means, LayerView can attach itself to events from GeckoApp (or GeckoApp can register for LayerView's -- ideal way) and pass it to it. LayerView can delegate to its controller. Thus LayerView will not know about GeckoAppShell. Is that fine?

Comment 41

[Chris Peterson [:cpeterson]]

(In reply to Sriram Ramasubramanian [:sriram] from comment #40)
> Oh wait for a second then. Me and Kats were talking about making mLayerView
> available in GeckoApp and it's layer controller initialized in its
> constructor. Which means, LayerView can attach itself to events from
> GeckoApp (or GeckoApp can register for LayerView's -- ideal way) and pass it
> to it. LayerView can delegate to its controller. Thus LayerView will not
> know about GeckoAppShell. Is that fine?

LayerView and LayerController are in the gfx package, so they should not know about GeckoApp or GeckoAppShell.

As you suggested, GeckoApp could register with GeckoAppShell for events that LayerConstructor cares about. GeckoApp could then delegate those events to LayerView and LayerView could delegate those events to its LayerController. But I think that approach is much more complicated that simply injecting GeckoAppShell's EventDispatcher into LayerController's constructor and letting LayerConstructor registered for events it cares about and receive direct handleMessage() callbacks.

Comment 42

[Chris Peterson [:cpeterson]]

(In reply to Sriram Ramasubramanian [:sriram] from comment #37)
> By exposing GeckoAppShell.getEventDispatcher(), we are making things harder,
> and making more changes. With my approach, it's still the same. The logic
> under GeckoAppShell changes, based on the name of the event, and it
> automagically registers the listener to one of its EventDispatchers. Hence
> getEventDispatcher() is not exposed.

btw, I just experimented with making GeckoAppShell.getEventHandler() private, but I feel it makes the code more complicated.

As mentioned in comment 41, GeckoApp would not be able to inject GeckoAppShell's EventDispatcher into the gfx classes, so GeckoApp would need to know about every event that gfx classes care about because GeckoApp would need to register for those events with GeckoAppShell and delegate the callbacks into gfx classes.

I'm waiting for r?=blassey, so maybe he will have an opinion.

Comment 43

[Sriram Ramasubramanian [:sriram]]

It might feel complicated. But if you want GeckoAppShell to be a separate entity and mLayerController not know it, then it should not know about GeckoAppShell.getEventDispatcher() too :) The approach goes wrong there :)

Ideally, GeckoApp (as a UI controller), should talk to GeckoAppShell (Gecko's controller). Any event from GeckoAppShell should be given to GeckoApp or its views (and mLayerView is one such view). They should handle it. Also, org.mozilla.gecko.gfx package is just another "view" package -- like say, org.mozilla.gecko.our_own_custom_ui_widgets. And, I don't know why it shouldn't know about GeckoAppShell.

Comment 44

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

(In reply to Sriram Ramasubramanian [:sriram] from comment #43)
> It might feel complicated. But if you want GeckoAppShell to be a separate
> entity and mLayerController not know it, then it should not know about
> GeckoAppShell.getEventDispatcher() too :) The approach goes wrong there :)
> 
> Ideally, GeckoApp (as a UI controller), should talk to GeckoAppShell
> (Gecko's controller). Any event from GeckoAppShell should be given to
> GeckoApp or its views (and mLayerView is one such view). They should handle
> it. Also, org.mozilla.gecko.gfx package is just another "view" package --
> like say, org.mozilla.gecko.our_own_custom_ui_widgets. And, I don't know why
> it shouldn't know about GeckoAppShell.

When dealing with refactor patches, I like to avoid doing too much in any refactor phase. Chris' patches currently use the following:

EventDispatcher ed = GeckoAppShell.getEventDispatcher();
SomeClass sc = new SomeClass(ed);

and sc uses it's own reference of the event dispatcher to handle registering/unregistering. This seems fine to me for now. Personally, I don't want to make a really cool event dispatcher system, just one that works. So the current level of refactor is good enough for me.

I do like passing the EventDispatcher as a param to the constructor of the class. It could be a setter too, but this seems better. The classes are derived from GeckoEventListener so taking the dispatcher as a param in the constructor seems fine.

The only nit I saw was the use of adding registerEventListener/unregisterEventListener to every class as convenience methods. Seems like overkill to me, but meh.

My only concern now is spending _any_ additional time/effort working on making a bigger and better event dispatcher system. Just say no.

Comment 45

[Chris Peterson [:cpeterson]]

(In reply to Sriram Ramasubramanian [:sriram] from comment #43)
> It might feel complicated. But if you want GeckoAppShell to be a separate
> entity and mLayerController not know it, then it should not know about
> GeckoAppShell.getEventDispatcher() too :) The approach goes wrong there :)

LayerController doesn't know about GeckoAppShell.getEventDispatcher(). It only knows about some EventDispatcher instance, but doesn't know or care where it came from.


> Ideally, GeckoApp (as a UI controller), should talk to GeckoAppShell
> (Gecko's controller). Any event from GeckoAppShell should be given to
> GeckoApp or its views (and mLayerView is one such view). They should handle
> it. Also, org.mozilla.gecko.gfx package is just another "view" package --
> like say, org.mozilla.gecko.our_own_custom_ui_widgets. And, I don't know why
> it shouldn't know about GeckoAppShell.

If we rephrase the question, why _should_ gfx package's views know about GeckoAppShell? :)

EventDispatcher would allow GeckoAppShell to deliver events to views (including gfx) without GeckoAppShell having direct knowledge of those views and without those views having direct knowledge of GeckoAppShell. GeckoApp (as UI controller) can connects its views to GeckoAppShell's event dispatcher.

I don't have a strong opinion about whether GeckoApp acts as an event mediator when GeckoAppShell broadcasts events (to views). I just think it would add extra code to GeckoApp.

Comment 46

[Chris Peterson [:cpeterson]]

(In reply to Mark Finkle (:mfinkle) from comment #44)
> The only nit I saw was the use of adding
> registerEventListener/unregisterEventListener to every class as convenience
> methods. Seems like overkill to me, but meh.

I can remove the registerEventListener() convenience methods. I added them because:

1. They remove a bunch of copy/paste boilerplate code that obscures the interesting part (the event name string).

2. They encapsulate whether the class is registering events using a private mEventDispatcher reference or reaching out to use GeckoAppShell.getEventHandler(). This simplifies the event registration code and allows all event registration to follow a similar coding style.

Comment 47

[Chris Peterson [:cpeterson]]

Patches #1-6:
https://hg.mozilla.org/integration/mozilla-inbound/rev/800a810a9466
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f318daea7c3
https://hg.mozilla.org/integration/mozilla-inbound/rev/7bebc2480a5c
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac4bf00b43fe
https://hg.mozilla.org/integration/mozilla-inbound/rev/6fcc646aa521
https://hg.mozilla.org/integration/mozilla-inbound/rev/4129a9d0f887

Comment 48

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

(In reply to Chris Peterson (:cpeterson) from comment #46)

> I can remove the registerEventListener() convenience methods. I added them
> because:
> 
> 1. They remove a bunch of copy/paste boilerplate code that obscures the
> interesting part (the event name string).

Coming from a JS background, there is no single interesting part. "this" is not always the case.

> 2. They encapsulate whether the class is registering events using a private
> mEventDispatcher reference or reaching out to use
> GeckoAppShell.getEventHandler(). This simplifies the event registration code
> and allows all event registration to follow a similar coding style.

mEventDispatcher.register vs GeckoAppShell.getEventHandler().register : tells me the same thing and using the helper functions make it seem like the class itself is an EventDispatcher.

Just my thoughts and it's a nit. I wouldn't dream of blocking landing on it.

Comment 49

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/800a810a9466
https://hg.mozilla.org/mozilla-central/rev/8f318daea7c3
https://hg.mozilla.org/mozilla-central/rev/7bebc2480a5c
https://hg.mozilla.org/mozilla-central/rev/ac4bf00b43fe
https://hg.mozilla.org/mozilla-central/rev/6fcc646aa521
https://hg.mozilla.org/mozilla-central/rev/4129a9d0f887

Comment 50

[Chris Peterson [:cpeterson]]

Landed patch 7:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e10b948a6e63

Comment 51

[Matt Brubeck (:mbrubeck)]

Fixed a build error:
https://hg.mozilla.org/integration/mozilla-inbound/rev/28e537336f50

Comment 52

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/e10b948a6e63
https://hg.mozilla.org/mozilla-central/rev/28e537336f50

Comment 53

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

Comment on attachment 648970 [details] [diff] [review]
part-10-even-more-EventListeners.patch

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

::: mobile/android/base/GeckoAppShell.java
@@ +1908,5 @@
>       * will be invoked on future events of that type.
>       *
>       * This method is referenced by Robocop via reflection.
>       */
> +    public static void registerRobocopEventListener(String event, GeckoEventListener listener) {

naming nit, you're still registering a gecko event listener, you're just now only doing it for robocop.

Comment 54

[Chris Peterson [:cpeterson]]

Green tryserver run for patches 8-10:
https://tbpl.mozilla.org/?tree=Try&rev=1607ec98dfc4

Comment 55

[Chris Peterson [:cpeterson]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/db477a55f1cc
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc6ca8328180
https://hg.mozilla.org/integration/mozilla-inbound/rev/6c46136790a1

Comment 56

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/db477a55f1cc
https://hg.mozilla.org/mozilla-central/rev/fc6ca8328180
https://hg.mozilla.org/mozilla-central/rev/6c46136790a1

Comment 57

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

I think most or all of these patches landed in 17, and nobody is working on this any more. So I'm going to close this bug and if there is additional untangling to be done it should be done in a new bug.