Closed
Bug 1325112
Opened 6 years ago
Closed 6 years ago
Remove GeckoInterface.isOfficial
Categories
(GeckoView :: General, defect)
Tracking
(firefox55 fixed)
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: jchen, Assigned: jchen)
References
Details
Attachments
(2 files)
10.90 KB,
patch
|
snorp
:
review-
nalexander
:
review+
|
Details | Diff | Splinter Review |
3.97 KB,
patch
|
rbarker
:
review+
|
Details | Diff | Splinter Review |
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•6 years ago
|
||
(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.
Assignee | ||
Comment 2•6 years ago
|
||
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 3•6 years ago
|
||
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-
Assignee | ||
Comment 5•6 years 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.
Comment 6•6 years ago
|
||
(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)
Assignee | ||
Comment 7•6 years 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)
Assignee | ||
Comment 8•6 years ago
|
||
Preload Gecko with the "-purgecaches" flag in debug builds of geckoview_example, similar to what we do for Fennec.
Attachment #8867278 -
Flags: review?(rbarker)
Updated•6 years ago
|
Attachment #8867278 -
Flags: review?(rbarker) → review+
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•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b4549c97046b https://hg.mozilla.org/mozilla-central/rev/06aae5032411
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Updated•4 years ago
|
Assignee: nobody → nchen
Updated•4 years ago
|
Product: Firefox for Android → GeckoView
Updated•4 years ago
|
Target Milestone: Firefox 55 → mozilla55
You need to log in
before you can comment on or make changes to this bug.
Description
•