802130 - Rename GeckoAppShell.getHandler to GeckoAppShell.getBackgroundHandler

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

For clarity.

Comment 1

[Chris Peterson [:cpeterson]]

Attachment: part-1-add-getUiThreadHandler.patch

kats, here are some WIP patches I had sitting around. They are probably bitrotted and incomplete, but I'm including them for reference or feedback.

Comment 2

[Chris Peterson [:cpeterson]]

Attachment: part-2-replace-getMainHandler.patch

Comment 3

[Chris Peterson [:cpeterson]]

Attachment: part-3-replace-getHandler.patch

Comment 4

[Chris Peterson [:cpeterson]]

Attachment: part-4-updateThumbnail-already-on-background-thread.patch

Comment 5

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

Thanks for the patches! Were there any known issues you ran into?

Comment 6

[Chris Peterson [:cpeterson]]

I don't remember any issues, but I haven't tested the patches recently. They were a quick WIP, so feel free to cherry pick bits or rewrite them as you see fit.

I renamed references to "MainThread" to the (hopefully) less ambiguous "UiThread".

Comment 7

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

Attachment: Part 1 - Move assertOnThread functions to a new ThreadUtils class

Comment 8

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

Attachment: Part 2 - Move mMainHandler to ThreadUtils

Comment 9

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

Attachment: Part 3 - Move GeckoAppShell.getHandler() to ThreadUtils.getBackgroundHandler()

Comment 10

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

Attachment: Part 4 - Add ThreadUtils.isOnUiThread

Comment 11

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

Try build with these patches at https://tbpl.mozilla.org/?tree=Try&rev=981f1dc8303f

Comment 12

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

Comment on attachment 725000 [details] [diff] [review]
Part 1 - Move assertOnThread functions to a new ThreadUtils class

Try run looks green, and I don't think I screwed anything up based on local testing.

Comment 13

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

Comment on attachment 725001 [details] [diff] [review]
Part 2 - Move mMainHandler to ThreadUtils

I plan on removing the trailing newlines on landing.

Comment 14

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

Comment on attachment 725000 [details] [diff] [review]
Part 1 - Move assertOnThread functions to a new ThreadUtils class

>diff --git a/mobile/android/base/GeckoApp.java b/mobile/android/base/GeckoApp.java

>+        ThreadUtils.setUiThread(Thread.currentThread());

I am a bit ignorant on how threads and activities are associated, but I wonder if the thread could change in those situations where the activity is killed and restarted? Would it be simpler to just use: Looper.getMainLooper().getThread()  when we need to check against the main thread? 

Just asking, I have no idea really. Even if the thread doesn't change, maybe we could init the UI thread inside ThreadUtils using the Looper method and not need to pass it in.


>             sGeckoThread = new GeckoThread(intent, passedUri);
>+            ThreadUtils.setGeckoThread(sGeckoThread);

Ah, there is some symmetry in having setXxxThread methods. Should all access to the Gecko thread happen from ThreadUtils? Just wondering if we could remove it from GeckoApp

---

You might be able to remove the | import ...GeckoApp; | from some of these files. I didn't really check myself. Maybe Brian will using Eclipse!

Comment 15

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

Comment on attachment 725002 [details] [diff] [review]
Part 3 - Move GeckoAppShell.getHandler() to ThreadUtils.getBackgroundHandler()

Wow, we use the background thread a lot.

Maybe once we figure out these Sqlite locking issues, we could look at using a simple pool of background threads. I am getting ahead of myself though.

Comment 16

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

Comment on attachment 725001 [details] [diff] [review]
Part 2 - Move mMainHandler to ThreadUtils

Really makes things more understandable!

Nice removal of the unused GeckoHandler

Comment 17

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

(In reply to Mark Finkle (:mfinkle) from comment #14)
> >+        ThreadUtils.setUiThread(Thread.currentThread());
> 
> I am a bit ignorant on how threads and activities are associated, but I
> wonder if the thread could change in those situations where the activity is
> killed and restarted? Would it be simpler to just use:
> Looper.getMainLooper().getThread()  when we need to check against the main
> thread? 
>
> Just asking, I have no idea really. Even if the thread doesn't change, maybe
> we could init the UI thread inside ThreadUtils using the Looper method and
> not need to pass it in. 

I don't think the thread does change in practice, but even if it does change theoretically, setting it every time we execute onCreate(), which is what the patch does, should make sure it's always correct. I could probably also use Looper.getMainLooper().getThread() though - I might do that in another round of cleanups I'm thinking about.

> Ah, there is some symmetry in having setXxxThread methods. Should all access
> to the Gecko thread happen from ThreadUtils? Just wondering if we could
> remove it from GeckoApp

I would like to do that, but currently ThreadUtils is built in the util/ folder which is built before the rest of the code (except mozglue). So I can't reference GeckoThread directly in ThreadUtils, I can only reference it as "Thread". Moving sGeckoThread out of GeckoApp is also something I want to do in a future round of cleanups.

Comment 18

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/a08bb4602689
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ebda01226c8
https://hg.mozilla.org/integration/mozilla-inbound/rev/418eea6b1963
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ba5f40a0340

Comment 19

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/a08bb4602689
https://hg.mozilla.org/mozilla-central/rev/1ebda01226c8
https://hg.mozilla.org/mozilla-central/rev/418eea6b1963
https://hg.mozilla.org/mozilla-central/rev/0ba5f40a0340