For clarity.

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.

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

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".

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

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 on attachment 725001 [details] [diff] [review] Part 2 - Move mMainHandler to ThreadUtils I plan on removing the trailing newlines on landing.

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 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 on attachment 725001 [details] [diff] [review] Part 2 - Move mMainHandler to ThreadUtils Really makes things more understandable! Nice removal of the unused GeckoHandler

(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.

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

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