Closed Bug 778468 Opened 8 years ago Closed 7 years ago

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

Categories

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

ARM
Android
defect

Tracking

()

RESOLVED FIXED
Firefox 17
Tracking Status
firefox16 --- wontfix
firefox17 --- affected

People

(Reporter: cpeterson, Unassigned)

References

(Depends on 2 open bugs)

Details

Attachments

(10 files, 7 obsolete files)

6.84 KB, patch
mfinkle
: review+
cpeterson
: checkin+
Details | Diff | Splinter Review
4.13 KB, patch
mfinkle
: review+
cpeterson
: checkin+
Details | Diff | Splinter Review
4.73 KB, patch
mfinkle
: review+
cpeterson
: checkin+
Details | Diff | Splinter Review
8.55 KB, patch
mfinkle
: review+
cpeterson
: checkin+
Details | Diff | Splinter Review
13.24 KB, patch
mfinkle
: review+
cpeterson
: checkin+
Details | Diff | Splinter Review
3.95 KB, patch
blassey
: review+
cpeterson
: checkin+
Details | Diff | Splinter Review
19.01 KB, patch
blassey
: review+
cpeterson
: checkin+
Details | Diff | Splinter Review
18.81 KB, patch
blassey
: review+
kats
: checkin+
Details | Diff | Splinter Review
25.33 KB, patch
blassey
: review+
kats
: checkin+
Details | Diff | Splinter Review
22.10 KB, patch
blassey
: review+
kats
: checkin+
Details | Diff | Splinter Review
No description provided.
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".
Attachment #646886 - Flags: review?(mark.finkle)
Part 2: Rename GeckoJarReader to JarUtils.
Attachment #646887 - Flags: review?(mark.finkle)
Part 3: Rename GeckoBackgroundThread to BackgroundThread.
Attachment #646888 - Flags: review?(mark.finkle)
Part 4: Rename GeckoAsyncTask to BackgroundTask (and remove unused Progress parameter).
Attachment #646889 - Flags: review?(mark.finkle)
Attached patch part-5-compile-toolkit-jar.patch (obsolete) — Splinter Review
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.
Attachment #646890 - Flags: review?(khuey)
Attachment #646890 - Flags: feedback?(mark.finkle)
Attached patch part-6-util-FloatUtils.patch (obsolete) — Splinter Review
Part 6: Move FloatUtils to org.mozilla.gecko.util package.
Attachment #647772 - Flags: review?(mark.finkle)
Comment on attachment 647772 [details] [diff] [review]
part-6-util-FloatUtils.patch

I'm moving this patch to bug 779366.
Attachment #647772 - Attachment is obsolete: true
Attachment #647772 - Flags: review?(mark.finkle)
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 on attachment 646886 [details] [diff] [review]
part-1-create-toolkit-package.patch

Cancelling review because this patch has been obsoleted by my local changes.
Attachment #646886 - Flags: review?(mark.finkle)
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.
Attachment #646887 - Flags: review?(mark.finkle)
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.
Attachment #646888 - Flags: review?(mark.finkle)
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.
Attachment #646889 - Flags: review?(mark.finkle)
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.
Attachment #646890 - Flags: review?(khuey)
Attachment #646890 - Flags: feedback?(mark.finkle)
Summary: Create org.mozilla.gecko.toolkit package → Create org.mozilla.gecko.util package
Part 1: Move ActivityResultHandler to org.mozilla.gecko.util package.
Attachment #648438 - Flags: review?(mark.finkle)
Part 2: Move INIParser and INISection to org.mozilla.gecko.util package.
Attachment #648439 - Flags: review?(mark.finkle)
Part 3: Move GeckoJarReader to org.mozilla.gecko.util package.
Attachment #648440 - Flags: review?(mark.finkle)
Part 4: Move GeckoBackgroundThread to org.mozilla.gecko.util package.
Attachment #646886 - Attachment is obsolete: true
Attachment #646887 - Attachment is obsolete: true
Attachment #646888 - Attachment is obsolete: true
Part 5: Move GeckoAsyncTask to org.mozilla.gecko.util package.
Attachment #646889 - Attachment is obsolete: true
Attachment #646890 - Attachment is obsolete: true
Attachment #648457 - Flags: review?(mark.finkle)
Summary: Create org.mozilla.gecko.util package → Move classes into new namespaces to untangle org.mozilla.gecko class dependencies
Whiteboard: [leave open]
Depends on: 777591
Blocks: 777591
No longer depends on: 777591
Attachment #648441 - Flags: review?(mfinkle)
Attachment #648438 - Flags: review?(mfinkle) → review+
Attachment #648439 - Flags: review?(mfinkle) → review+
Attachment #648440 - Flags: review?(mfinkle) → review+
Attachment #648441 - Flags: review?(mfinkle) → review+
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.
Attachment #648457 - Flags: review?(mfinkle) → review+
(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.
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! :(
Blocks: 780276
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.
(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).
(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.
Part 6: Inject LayerView's InputConnectionHandler to remove GeckoInputConnection dependency.
Attachment #648959 - Flags: review?(blassey.bugs)
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.
Attachment #648960 - Flags: review?(blassey.bugs)
Part 8: Add EventDispatcher to remove gfx dependency on GeckoAppShell event registration.
Attachment #648961 - Flags: review?(blassey.bugs)
Attached patch part-9-more-EventListeners.patch (obsolete) — Splinter Review
Part 9: Use EventDispatcher to remove ui dependency on GeckoAppShell event registration.
Attachment #648962 - Flags: review?(blassey.bugs)
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.
Attachment #648962 - Attachment is obsolete: true
Attachment #648962 - Flags: review?(blassey.bugs)
Attachment #648968 - Flags: review?(blassey.bugs)
Part 10: Make all event registration go through EventDispatcher (except Robocop's dynamic class loader).
Attachment #648970 - Flags: review?(blassey.bugs)
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.
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.
(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.
Depends on: 780649
(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. :)
(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.
(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.
> > 
> > > 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.
That makes sense. I will update my local patches to make GeckoAppShell's EventDispatcher private.
(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.
(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?
(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.
(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.
Attachment #648959 - Flags: review?(blassey.bugs) → review+
Attachment #648960 - Flags: review?(blassey.bugs) → review+
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.
(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.
(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.
(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.
(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.
Attachment #648961 - Flags: review?(blassey.bugs) → review+
Attachment #648968 - Flags: review?(blassey.bugs) → review+
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.
Attachment #648970 - Flags: review?(blassey.bugs) → review+
Attachment #648438 - Flags: checkin+
Attachment #648439 - Flags: checkin+
Attachment #648440 - Flags: checkin+
Attachment #648441 - Flags: checkin+
Attachment #648960 - Flags: checkin+
Attachment #648959 - Flags: checkin+
Attachment #648457 - Flags: checkin+
Green tryserver run for patches 8-10:
https://tbpl.mozilla.org/?tree=Try&rev=1607ec98dfc4
Depends on: 783954
Assignee: cpeterson → nobody
Status: ASSIGNED → NEW
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.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [leave open]
You need to log in before you can comment on or make changes to this bug.