Closed Bug 894313 Opened 7 years ago Closed 6 years ago

GeckoThread should own its own static instance

Categories

(Firefox for Android :: General, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 26

People

(Reporter: blassey, Assigned: blassey)

Details

Attachments

(1 file, 3 obsolete files)

Attached patch patch (obsolete) — Splinter Review
I assert this is better, but happy to listen to other opinions.
Attachment #776292 - Flags: review?(bugmail.mozilla)
Comment on attachment 776292 [details] [diff] [review]
patch

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

Looks ok but would like to see an updated version with comments addressed.

::: mobile/android/base/GeckoApp.java
@@ +1190,4 @@
>          String args = intent.getStringExtra("args");
>          earlyStartJavaSampler(intent);
>  
> +

remove blank line

@@ +1243,4 @@
>              return;
>          }
>  
> +        if (GeckoThread.sGeckoThread != null) {

This seems like leaky abstraction to me. I would prefer if this was "if (!GeckoThread.isCreated())" or something similar. Basically, make sGeckoThread private and expose its nullness via a public method.

::: mobile/android/base/GeckoThread.java
@@ +39,5 @@
>  
> +    static GeckoThread sGeckoThread;
> +    static String sUri;
> +    static String sArgs;
> +    static String sAction;

All of these should be private.

@@ +47,5 @@
> +            sUri = uri;
> +        if (args != null)
> +            sArgs = args;
> +        if (action != null)
> +            sAction = action;

If the null checks here are really needed, add a comment as to why. From the code it looks like there might be some cases where this gets called multiple times and the null check will prevent future calls from clobbering the values set in older calls. Is that it?

@@ +127,5 @@
>  
>          Log.w(LOGTAG, "zerdatime " + SystemClock.uptimeMillis() + " - runGecko");
>  
> +        String args = addCustomProfileArg(sArgs);
> +        String type = getTypeFromAction(sAction != null ? sAction : 

trailing ws
Attachment #776292 - Flags: review?(bugmail.mozilla) → review-
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → blassey.bugs
Attachment #776292 - Attachment is obsolete: true
Attachment #777019 - Flags: review?(bugmail.mozilla)
Comment on attachment 777019 [details] [diff] [review]
patch

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

::: mobile/android/base/GeckoThread.java
@@ +44,3 @@
>  
> +    static void setStartupArgs(String uri, String args, String action) {
> +        // If a start up arg is non-null, set it

This comment is not useful. I can see that's what the code is doing, but the comment should explain *why* it only does it for non-null args. Why not just assign it unconditionally? The static members are initialized to null anyway so there's no harm in assigning a null to it again.
Attachment #777019 - Flags: review?(bugmail.mozilla) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #3)
> Comment on attachment 777019 [details] [diff] [review]
> patch
> 
> Review of attachment 777019 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/GeckoThread.java
> @@ +44,3 @@
> >  
> > +    static void setStartupArgs(String uri, String args, String action) {
> > +        // If a start up arg is non-null, set it
> 
> This comment is not useful. I can see that's what the code is doing, but the
> comment should explain *why* it only does it for non-null args. Why not just
> assign it unconditionally? The static members are initialized to null anyway
> so there's no harm in assigning a null to it again.

The intention is that if you want to set one of the start up args, but not another you can just call with nulls for things you don't know yet.

Alternatively, we could have 3 separate methods to set each arg.
(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #4)
> The intention is that if you want to set one of the start up args, but not
> another you can just call with nulls for things you don't know yet.

Yeah but you can just as easily drop the null checks and it will work the same. As long as setStartupArgs is called only once it makes no difference whether you do the null checks or not, as you'll just be putting a null in to sUri (for example) that was already null to begin with.
Attached patch gecko_thread.patch (obsolete) — Splinter Review
Attachment #777019 - Attachment is obsolete: true
Attachment #778170 - Flags: review?(bugmail.mozilla)
Comment on attachment 778170 [details] [diff] [review]
gecko_thread.patch

>+    static void setArgs(String args) {
>+            sArgs = args;
>+    }
>+
>+    static void setAction(String action) {
>+            sAction = action;
>+    }
>+

Fix indentation.
Attachment #778170 - Flags: review?(bugmail.mozilla) → review+
try run
Attachment #778170 - Attachment is obsolete: true
Attachment #784758 - Flags: review?
Attachment #784758 - Attachment is private: false
Attachment #784758 - Flags: review? → review?(bugmail.mozilla)
Comment 9 is private: false
looks like I forgot to include the link to the try run https://tbpl.mozilla.org/?tree=Try&rev=9a219cbb89c1
Comment on attachment 784758 [details] [diff] [review]
gecko_thread.patch

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

::: mobile/android/base/GeckoThread.java
@@ +45,4 @@
>      private final String mUri;
>  
> +    public static boolean ensureInit() {
> +        if (isCreated())

Potential multithreading hazard here. Might be a good idea to stick in a call to ThreadUtils.assertOnUiThread() here to ensure it doesn't get called from any other thread, that will ensure the hazard is never tripped.
Attachment #784758 - Flags: review?(bugmail.mozilla) → review+
Could we have this instance in GeckoApplication? Ideally Gecko should be running as long as GeckoApplication is up. In which case, GeckoApplication is a better place to take care of this thread -- as GeckoApplication is singleton, and can initialize Gecko. This would help initializing Gecko from Settings screen (if Settings is launched directly from notifications) and so on.
https://hg.mozilla.org/mozilla-central/rev/f93ac04d92ab
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
You need to log in before you can comment on or make changes to this bug.