Make Tabs.notifyListeners() run on UI thread

RESOLVED FIXED in Firefox 19

Status

()

defect
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: bnicholson, Assigned: bnicholson)

Tracking

unspecified
Firefox 19
ARM
Android
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

We do this in a lot of places:

GeckoAppShell.getMainHandler().post(new Runnable() {
    public void run() {
        Tabs.getInstance().notifyListeners(Tab.this, Tabs.TabEvents.MENU_UPDATED);
    }
});

This patch cleans things up a bit by moving the UI post to notifyListeners(), and it removes some references to mAppContext and GeckoAppShell in Tab. I also removed removeFromReadingList() from Tab, which is no longer used anywhere.
Attachment #674083 - Flags: review?(mark.finkle)
Comment on attachment 674083 [details] [diff] [review]
Make Tabs.notifyListeners() run on UI thread

>     private void updateBookmark() {

>-        (new GeckoAsyncTask<Void, Void, Void>(GeckoApp.mAppContext, GeckoAppShell.getHandler()) {
>-            @Override
>-            public Void doInBackground(Void... params) {
>-                if (url.equals(getURL())) {
>-                    mBookmark = BrowserDB.isBookmark(mContentResolver, url);
>-                    mReadingListItem = BrowserDB.isReadingListItem(mContentResolver, url);
>-                }
>-                return null;
>-            }
>+        if (url.equals(getURL())) {
>+            mBookmark = BrowserDB.isBookmark(mContentResolver, url);
>+            mReadingListItem = BrowserDB.isReadingListItem(mContentResolver, url);
>+        }

Is this code still being run in the background? Can we force the caller of updateBookmark to ensure it is run in the background?
Assignee

Comment 2

7 years ago
(In reply to Mark Finkle (:mfinkle) from comment #1)
> Is this code still being run in the background? Can we force the caller of
> updateBookmark to ensure it is run in the background?

updateBookmark() is called in two places: [1] and [2]. [1] is called on the GeckoBackgroundThread since we pass in the handler. [2], however, is apparently run on the Gecko thread. I always assumed our Gecko events were being parsed and dispatched on the GeckoBackgroundThread, but looks like I was wrong - I wonder if this hurting us anywhere else? Any long DB accesses here would block Gecko, which we obviously don't want.

To fix this, I posted the body of updateBookmark() to the GeckoBackgroundThread, which is the same thing we do for addBookmark(), removeBookmark(), updateHistory(), etc. Since addBookmark() is always run on the background thread, I also removed the redundant 'GeckoAppShell.getHandler()' we pass in at [1].

[1] http://hg.mozilla.org/integration/mozilla-inbound/file/8cc37887f621/mobile/android/base/Tab.java#l104
[2] http://hg.mozilla.org/integration/mozilla-inbound/file/8cc37887f621/mobile/android/base/Tab.java#l238
Attachment #674083 - Attachment is obsolete: true
Attachment #674083 - Flags: review?(mark.finkle)
Attachment #674307 - Flags: review?(mark.finkle)
(In reply to Brian Nicholson (:bnicholson) from comment #2)

> apparently run on the Gecko thread. I always assumed our Gecko events were
> being parsed and dispatched on the GeckoBackgroundThread, but looks like I
> was wrong - I wonder if this hurting us anywhere else? Any long DB accesses
> here would block Gecko, which we obviously don't want.

Right. We need to not to work on the Gecko thread. It will block Gecko work, like loading pages and cool stuff, that we want to happen fast. Is an audit in order?
Attachment #674307 - Flags: review?(mark.finkle) → review+
Assignee

Comment 4

7 years ago
(In reply to Mark Finkle (:mfinkle) from comment #3)
> (In reply to Brian Nicholson (:bnicholson) from comment #2)
> 
> > apparently run on the Gecko thread. I always assumed our Gecko events were
> > being parsed and dispatched on the GeckoBackgroundThread, but looks like I
> > was wrong - I wonder if this hurting us anywhere else? Any long DB accesses
> > here would block Gecko, which we obviously don't want.
> 
> Right. We need to not to work on the Gecko thread. It will block Gecko work,
> like loading pages and cool stuff, that we want to happen fast. Is an audit
> in order?

I ran some quick tests and found that most messages are done after 5ms or so (including the JSON parsing). Some did take longer, though; at startup, the Preferences:Data message takes around 40ms to finish.

http://hg.mozilla.org/integration/mozilla-inbound/rev/6ffa6c2557ae
https://hg.mozilla.org/mozilla-central/rev/6ffa6c2557ae
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → Firefox 19
You need to log in before you can comment on or make changes to this bug.