[geckoview] Move Fennec-specific GeckoProfile initialization out of GeckoView

RESOLVED FIXED in Firefox 51

Status

()

Core
Embedding: APIs
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: nalexander, Assigned: jchen)

Tracking

unspecified
mozilla51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed)

Details

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
After Bug 1258450, we have:


nalexander@chocho ~/M/gecko> java -cp mobile/android/build/classycle/classycle-1.4.1.jar classycle.dependency.DependencyChecker -mergeInnerClasses -dependencies=@mobile/android/base/geckoview.ddf objdir-droid/gradle/build/mobile/android/app/intermediates/packaged/local/debug/classes.jar
show onlyShortestPaths allResults
Set [lib] has 133 classes.
Set [app] has 715 classes.
check [lib] independentOf [app]
  Unexpected dependencies found:
  org.mozilla.gecko.GeckoProfile
    -> org.mozilla.gecko.GeckoApp
    -> org.mozilla.gecko.db.BrowserDB
    -> org.mozilla.gecko.distribution.Distribution
    -> org.mozilla.gecko.GuestSession
    -> org.mozilla.gecko.db.StubBrowserDB
    -> org.mozilla.gecko.TelemetryContract
    -> org.mozilla.gecko.Telemetry
    -> org.mozilla.gecko.db.LocalBrowserDB
    -> org.mozilla.gecko.preferences.DistroSharedPrefsImport
<snip>

There are two things Fennec-specific here.  One is that the GeckoProfile yields access to the BrowserDB factory and instances.  I think this can just be turned around: Rather than GeckoProfile.get().getDB(), call BrowserDB.getDB(GeckoProfile.get()).

The latter is that a bunch of Fennec-specific initialization with Telemetry and Distributions is tied into GeckoProfile creation.  I suggest we just make this explicit by having GeckoProfile hand itself to the ambient GeckoInterface, which can then complete whatever initialization it wants to do.

If we play this correctly, we may be able to get rid of https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/db/StubBrowserDB.java entirely.  This is because the GeckoView package won't reference Fennec DBs at all anymore!

Comment 1

2 years ago
Mass change of bugs in the "Embedding: GRE Core" component in preparation for archiving it. I believe that these are no longer relevant; but if they are, they should be reopened and moved into a component relevant to the code in question.
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → INCOMPLETE
Product: Core → Core Graveyard
Reopening and moving to Core::Embedding: APIs, where we'll continue to triage and track GeckoView issues.
Status: RESOLVED → REOPENED
Component: Embedding: GRE Core → Embedding: APIs
Product: Core Graveyard → Core
Resolution: INCOMPLETE → ---
(Assignee)

Updated

2 years ago
Depends on: 1291383
(Assignee)

Updated

2 years ago
Depends on: 1291384
(Assignee)

Comment 3

2 years ago
Created attachment 8789839 [details] [diff] [review]
1. Start first-run telemetry session outside of GeckoProfile (v1)

Move the first-run telemetry session from GeckoProfile to BrowserApp, so
there is no longer any dependency on Telemetry from inside GeckoProfile.
Attachment #8789839 - Flags: review?(s.kaspari)
(Assignee)

Comment 4

2 years ago
Created attachment 8789840 [details] [diff] [review]
2. Move guest mode state management to GeckoProfile (v1)

Because guest mode is intimately tied to the profile, it'd be hard to
keep guest mode out of geckoview code entirely, But we also don't want
any dependency on GuestSession from geckoview code, so this patch moves
the part of GuestSession that manages guest mode state to GeckoProfile.
Attachment #8789840 - Flags: review?(s.kaspari)
(Assignee)

Updated

2 years ago
Attachment #8789839 - Flags: review?(s.kaspari) → review?(nalexander)
(Assignee)

Updated

2 years ago
Attachment #8789840 - Flags: review?(s.kaspari) → review?(nalexander)
(Assignee)

Updated

2 years ago
Assignee: nobody → nchen
(Reporter)

Comment 5

2 years ago
Comment on attachment 8789839 [details] [diff] [review]
1. Start first-run telemetry session outside of GeckoProfile (v1)

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

Much better.
Attachment #8789839 - Flags: review?(nalexander) → review+
(Reporter)

Comment 6

2 years ago
Comment on attachment 8789840 [details] [diff] [review]
2. Move guest mode state management to GeckoProfile (v1)

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

Looks good.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoProfile.java
@@ +98,5 @@
>      private File mProfileDir;
>  
>      private Boolean mInGuestMode;
>  
> +    public static boolean shouldUseGuestMode(final Context context) {

I feel like these should be synchronized, but this whole approach could theoretically race since prefs are not atomic, so whatever :)
Attachment #8789840 - Flags: review?(nalexander) → review+

Comment 7

2 years ago
Pushed by nchen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/77797e21fa19
1. Start first-run telemetry session outside of GeckoProfile; r=nalexander
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d2e0c859f9e
2. Move guest mode state management to GeckoProfile; r=nalexander

Comment 8

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/77797e21fa19
https://hg.mozilla.org/mozilla-central/rev/5d2e0c859f9e
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.