Remove GeckoInterface.isOfficial

RESOLVED FIXED in Firefox 55

Status

()

Firefox for Android
GeckoView
RESOLVED FIXED
a year ago
8 months ago

People

(Reporter: jchen, Unassigned)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 55
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(2 attachments)

(Reporter)

Description

a year ago
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.
(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.
(Reporter)

Updated

9 months ago
Blocks: 1362162
(Reporter)

Comment 2

9 months ago
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.
Attachment #8866499 - Flags: review?(snorp)
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.
Attachment #8866499 - Flags: review+
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.
Attachment #8866499 - Flags: review?(snorp) → review-
(Reporter)

Comment 5

9 months ago
(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) (jwillcox@mozilla.com) 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.
(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) (jwillcox@mozilla.com) 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?
Flags: needinfo?(snorp)
Flags: needinfo?(nchen)
(Reporter)

Comment 7

9 months ago
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`.
Flags: needinfo?(snorp)
Flags: needinfo?(nchen)
(Reporter)

Comment 8

9 months ago
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.
Attachment #8867278 - Flags: review?(rbarker)
Attachment #8867278 - Flags: review?(rbarker) → review+

Comment 9

8 months ago
Pushed by nchen@mozilla.com:
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

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b4549c97046b
https://hg.mozilla.org/mozilla-central/rev/06aae5032411
Status: NEW → RESOLVED
Last Resolved: 8 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
You need to log in before you can comment on or make changes to this bug.