Closed Bug 802130 Opened 9 years ago Closed 9 years ago

Rename GeckoAppShell.getHandler to GeckoAppShell.getBackgroundHandler

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 22

People

(Reporter: kats, Assigned: kats)

References

Details

Attachments

(4 files, 4 obsolete files)

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.
Attachment #671892 - Attachment is patch: true
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".
Attachment #671892 - Attachment is obsolete: true
Attachment #671893 - Attachment is obsolete: true
Attachment #671894 - Attachment is obsolete: true
Attachment #671895 - Attachment is obsolete: true
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.
Attachment #725000 - Flags: review?(mark.finkle)
Comment on attachment 725001 [details] [diff] [review]
Part 2 - Move mMainHandler to ThreadUtils

I plan on removing the trailing newlines on landing.
Attachment #725001 - Flags: review?(mark.finkle)
Attachment #725002 - Flags: review?(mark.finkle)
Attachment #725004 - Flags: review?(mark.finkle)
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!
Attachment #725000 - Flags: review?(mark.finkle) → review+
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.
Attachment #725002 - Flags: review?(mark.finkle) → review+
Attachment #725004 - Flags: review?(mark.finkle) → review+
Comment on attachment 725001 [details] [diff] [review]
Part 2 - Move mMainHandler to ThreadUtils

Really makes things more understandable!

Nice removal of the unused GeckoHandler
Attachment #725001 - Flags: review?(mark.finkle) → review+
(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.
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.