845051 - Clean up the BrowserApp/BrowserToolbar onTabChanged handlers

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[:Margaret Leibovic]

Attachment: (Part 1) Don't post Runnables to the main thread from onTabChanged, since it's always called on the main thread

This first patch gets rid of calls to mMainHandler.post from inside onTabChanged. We don't need these, since onTabChanged is always called from a Runnable passed to mActivity.runOnUiThread.

Getting rid of this also gets rid of the need for some helper methods.

Comment 1

[:Margaret Leibovic]

Attachment: (Part 2) Handle tab events directly in BrowserToolbar, instead of going through BrowserApp

This patch gets rid of some indirection and lets us make some BrowserToolbar methods private (also we can get rid of mMainHandler.post calls for the same reason I mentioned above).

Comment 2

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

Comment on attachment 718084 [details] [diff] [review]
(Part 1) Don't post Runnables to the main thread from onTabChanged, since it's always called on the main thread

Add a comment here stating this is called on the main thread:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java#90

Comment 3

[Sriram Ramasubramanian [:sriram]]

In patch 1, the mMainHandler cannot be removed, for showTabs(). https://bugzilla.mozilla.org/show_bug.cgi?id=834399#c14

Comment 4

[Matt Brubeck (:mbrubeck)]

Comment on attachment 718084 [details] [diff] [review]
(Part 1) Don't post Runnables to the main thread from onTabChanged, since it's always called on the main thread

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

::: mobile/android/base/BrowserApp.java
@@ -109,5 @@
> -                                                : TabsPanel.Panel.NORMAL_TABS;
> -                    mMainHandler.post(new Runnable() {
> -                        public void run() {
> -                            if (areTabsShown() && mTabsPanel.getCurrentPanel() != panel)
> -                                showTabs(panel);

This is used not to run something on a particular thread, but to delay it so that it does not modify the mTabsChangedListeners array while we are still iterating through the array.  For details see bug 834399 comment 14.

(If we decide to keep this runnable, we should add a comment explaining it -- sorry for not doing that in my original patch.)

Comment 5

[:Margaret Leibovic]

Thanks Sriram and Matt. I updated the patch to keep the mMainHandler code and add a comment above that line.

https://hg.mozilla.org/integration/mozilla-inbound/rev/dc4f4e042b3f
https://hg.mozilla.org/integration/mozilla-inbound/rev/956f5c456463

Comment 6

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/dc4f4e042b3f
https://hg.mozilla.org/mozilla-central/rev/956f5c456463