1325112 - Remove GeckoInterface.isOfficial

Status: RESOLVED FIXED

Description

[(inactive) Jim Chen [:jchen] [:darchons]]

I think it's okay to move the two points in GeckoThread where we use GeckoInterface.isOfficial to GeckoApp, so that we no longer need GeckoInterface.isOfficial.

Comment 1

[Nick Alexander :nalexander [he/him]]

(In reply to Jim Chen [:jchen] [:darchons] from comment #0)
> I think it's okay to move the two points in GeckoThread where we use
> GeckoInterface.isOfficial to GeckoApp, so that we no longer need
> GeckoInterface.isOfficial.

Yay!  While you're doing this, can you get rid of the EventDispatcher tests for RELEASE_OR_BETA?  I meant to file this earlier but haven't gotten to it.  This would make https://bugzilla.mozilla.org/show_bug.cgi?id=1322175 the final blocker to removing AppConstants from GeckoView entirely, IIRC.

Comment 2

[(inactive) Jim Chen [:jchen] [:darchons]]

Created attachment 8866499 [details] [diff] [review]
Remove GeckoInterface.isOfficial (v1)

There are two callers of isOfficial() in GeckoThread. For purging the
startup cache, the code to add the extra argument is moved to
GeckoApplication/GeckoApp/GeckoService. For logging the arguments, the
"debug" flag is used instead of the "official" flag.

Comment 3

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8866499 [details] [diff] [review]
Remove GeckoInterface.isOfficial (v1)

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

Happy to see you chewing up all of these GeckoInterface improvements, darchons!

::: mobile/android/base/java/org/mozilla/gecko/GeckoApplication.java
@@ +81,5 @@
>      }
>  
> +    public static String addDefaultGeckoArgs(String args) {
> +        if (!AppConstants.MOZILLA_OFFICIAL) {
> +            // In un-official builds, we want to load Javascript resources fresh

Will Gecko or the JS engine do something silly when reloading file:// or android_asset:// JavaScript resources when used in GeckoView?  I hope not, in which case this can safely be Fennec-specific.

Comment 4

[James Willcox (:snorp) ([e-mail])]

Comment on attachment 8866499 [details] [diff] [review]
Remove GeckoInterface.isOfficial (v1)

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

::: mobile/android/base/java/org/mozilla/gecko/GeckoApplication.java
@@ +81,5 @@
>      }
>  
> +    public static String addDefaultGeckoArgs(String args) {
> +        if (!AppConstants.MOZILLA_OFFICIAL) {
> +            // In un-official builds, we want to load Javascript resources fresh

Yeah, this is a behavior change, because BaseGeckoInterface always return false for isOfficial(). That means GeckoThread always added -purgecaches. This patch invert that for GV. I think we want to keep this logic in GeckoThread, but use AppConstants as we do here.

Comment 5

[(inactive) Jim Chen [:jchen] [:darchons]]

(In reply to Nick Alexander :nalexander from comment #3)
> Comment on attachment 8866499 [details] [diff] [review]
> Remove GeckoInterface.isOfficial (v1)
> 
> > +    public static String addDefaultGeckoArgs(String args) {
> > +        if (!AppConstants.MOZILLA_OFFICIAL) {
> > +            // In un-official builds, we want to load Javascript resources fresh
> 
> Will Gecko or the JS engine do something silly when reloading file:// or
> android_asset:// JavaScript resources when used in GeckoView?  I hope not,
> in which case this can safely be Fennec-specific.

Right, the patch makes it Fennec-specific by moving this code from GeckoThread (under GeckoView) to GeckoApplication (under Fennec). In official builds and in GeckoView (by-default), we don't pass -purgecaches, and therefore take advantage of the startup cache.

(In reply to James Willcox (:snorp) ([e-mail]) from comment #4)
> Comment on attachment 8866499 [details] [diff] [review]
> Remove GeckoInterface.isOfficial (v1)
> 
> Review of attachment 8866499 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/java/org/mozilla/gecko/GeckoApplication.java
> @@ +81,5 @@
> >      }
> >  
> > +    public static String addDefaultGeckoArgs(String args) {
> > +        if (!AppConstants.MOZILLA_OFFICIAL) {
> > +            // In un-official builds, we want to load Javascript resources fresh
> 
> Yeah, this is a behavior change, because BaseGeckoInterface always return
> false for isOfficial(). That means GeckoThread always added -purgecaches.
> This patch invert that for GV. I think we want to keep this logic in
> GeckoThread, but use AppConstants as we do here.

But we don't want -purgecaches for GeckoView? It's a perf hit that's meant for developer local builds only.

Comment 6

[Nick Alexander :nalexander [he/him]]

(In reply to Jim Chen [:jchen] [:darchons] from comment #5)
> (In reply to Nick Alexander :nalexander from comment #3)
> > Comment on attachment 8866499 [details] [diff] [review]
> > Remove GeckoInterface.isOfficial (v1)
> > 
> > > +    public static String addDefaultGeckoArgs(String args) {
> > > +        if (!AppConstants.MOZILLA_OFFICIAL) {
> > > +            // In un-official builds, we want to load Javascript resources fresh
> > 
> > Will Gecko or the JS engine do something silly when reloading file:// or
> > android_asset:// JavaScript resources when used in GeckoView?  I hope not,
> > in which case this can safely be Fennec-specific.
> 
> Right, the patch makes it Fennec-specific by moving this code from
> GeckoThread (under GeckoView) to GeckoApplication (under Fennec). In
> official builds and in GeckoView (by-default), we don't pass -purgecaches,
> and therefore take advantage of the startup cache.
> 
> (In reply to James Willcox (:snorp) ([e-mail]) from comment #4)
> > Comment on attachment 8866499 [details] [diff] [review]
> > Remove GeckoInterface.isOfficial (v1)
> > 
> > Review of attachment 8866499 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: mobile/android/base/java/org/mozilla/gecko/GeckoApplication.java
> > @@ +81,5 @@
> > >      }
> > >  
> > > +    public static String addDefaultGeckoArgs(String args) {
> > > +        if (!AppConstants.MOZILLA_OFFICIAL) {
> > > +            // In un-official builds, we want to load Javascript resources fresh
> > 
> > Yeah, this is a behavior change, because BaseGeckoInterface always return
> > false for isOfficial(). That means GeckoThread always added -purgecaches.
> > This patch invert that for GV. I think we want to keep this logic in
> > GeckoThread, but use AppConstants as we do here.
> 
> But we don't want -purgecaches for GeckoView? It's a perf hit that's meant
> for developer local builds only.

To me it's not a question of performance, it's a question about correctness while developing.  What I witnessed as a developer was:

* I would change code locally
* |mach build mobile/android && mach package && mach install|
* the cached version would be used in favour of my local changes.

This happened only because:

* the code I changed locally was in omni.ja and therefore eligible to be cached;
* the cache is evicted based on MOZ_BUILDID, which isn't advanced by local rebuilds or packaging.

For GeckoView _consumers_, this isn't a problem:

* the code they load should be from file:// or android://asset/... protocols.  It shouldn't be getting (startup) cached, and if it is and the cache isn't getting evicted based on timestamps we have another serious bug.
* each consumed release of GeckoView should advance the MOZ_BUILDID correctly, evicting the cache appropriately.

For GeckoView _developers_, they will still have this problem for code in the omni.ja; but, like for Fennec, they can work-around the issue (for example, in geckoview_example itself).

That's why I reasoned that this patch was sensible.  snorp, jchen: am I making incorrect assertions or assumptions?

Comment 7

[(inactive) Jim Chen [:jchen] [:darchons]]

That sounds right. On IRC, we decided to make geckoview_example act like Fennec and pass in `-purgecaches` if it's a debug build (based on BuildConfig.DEBUG, which is set by `withGeckoBinaries` because it includes `initWith debug` in build.gradle). GeckoThread will not have any special handling for `-purgecaches`.

Comment 8

[(inactive) Jim Chen [:jchen] [:darchons]]

Created attachment 8867278 [details] [diff] [review]
2. Purge caches in debug geckoview_example (v1)

Preload Gecko with the "-purgecaches" flag in debug builds of
geckoview_example, similar to what we do for Fennec.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b4549c97046b
1. Remove GeckoInterface.isOfficial; r=nalexander
https://hg.mozilla.org/integration/mozilla-inbound/rev/06aae5032411
2. Purge caches in debug geckoview_example; r=rbarker

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b4549c97046b
https://hg.mozilla.org/mozilla-central/rev/06aae5032411