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.

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

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

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

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

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

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