753217 - Move and refactor tab-specific listeners

Status: RESOLVED INVALID

(Firefox for Android Graveyard :: General, defect)

Description

[Brian Nicholson (:bnicholson)]

The coupling between GeckoApp/Tab/Tabs has become tighter and more complicated. This makes it more complicated to fix edge-case bugs, like bug 751650.

If we move tab-specific listeners to the Tab class, we can reduce the number of public method, especially setters/updaters, in Tab. This will help contain tab-specific implementation to the Tabs class and reduce the size of GeckoApp.

Comment 1

[Brian Nicholson (:bnicholson)]

Attachment: Part 1: Add handler to Tabs for tab-specific events

Simple refactor for existing listeners

Comment 2

[Brian Nicholson (:bnicholson)]

Attachment: Part 2: Move tab-specific listeners from GeckoApp to Tab class

Moves all tab-specific listeners and methods from GeckoApp to Tab. Removes leftover public methods that are no longer being used.

Comment 3

[Brian Nicholson (:bnicholson)]

Attachment: Part 3: Move isSelectedTab() logic to Tab class

Rather than having to use the Tabs singleton everywhere to determine whether a given tab is selected, we can contain this logic by moving it to Tab.

Comment 4

[Brian Nicholson (:bnicholson)]

Attachment: Part 4: Replace DOMWindowClose with Tab:Close

We're sending a DOMWindowClose event to Java that does the exact same thing as Tab:Close. This patch consolidates them.

Comment 5

[Brian Nicholson (:bnicholson)]

Attachment: Part 5: Remove setState from Tab class

Additional refactoring to remove Tab.setState().

Comment 6

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

Comment on attachment 622247 [details] [diff] [review]
Part 2: Move tab-specific listeners from GeckoApp to Tab class

The first thing that jumps out at me is all the GeckoApp.mXxx code that we are moving into Tab.java. We must not do that. We will very soon have code that runs in chrome and chromeless activities.

The biggest offender is GeckoApp.mBrowserToolbar which leads me to think that BrowserToolbar should start listening for these messages too. Does that make sense to you?

Comment 7

[Brian Nicholson (:bnicholson)]

Attachment: Part 6: Move toolbar update logic to Tabs listener

This patch creates an onTabUpdated() event which is called whenever some UI-related part of the tab is changed. This is handled in Tabs, which does a generic refresh.

Eventually, we probably want the browser toolbar updates to be handled in BrowserToolbar itself. Tabs.java already has lots of references to mBrowserToolbar, so it's easiest to just move it there for now, and we can replace all instances of mBrowserToolbar in Tabs separately.

Comment 8

[Brian Nicholson (:bnicholson)]

Attachment: Part 7: Change mTabChangedListeners to a LinkedList

Changes mTabsChangedListener to use a LinkedList, initializes it in the constructor, and removes some unnecessary null checks.

Comment 9

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

We have onTabUpdated and onTabChanged events. Should we use just one?

Comment 10

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

What here is still relevant?

Comment 11

[Brian Nicholson (:bnicholson)]

(In reply to Mark Finkle (:mfinkle) from comment #10)
> What here is still relevant?

Probably everything, though we'll have to essentially redo all of the patches from bitrot.

As you said in comment 6, some of this code introduces more mAppContext references to Tab.java. We should be slowly trying to kill off mAppContext, so I'll have to think about parts of this more.

Comment 12

[Brian Nicholson (:bnicholson)]

Parts of this have been fixed in other bugs, and other parts are extremely bitrotten. We could probably still use some cleanup in these areas, but we'd be best starting from scratch. Closing this bug since, as written, it is no longer valid.