1291384 - [geckoview] Make GeckoProfile Fennec BrowserDB agnostic

Status: RESOLVED FIXED

(GeckoView :: General, defect)

Description

[Nick Alexander :nalexander [he/him] (PTO Feb 24-March 3)]

Right now, Fennec fetches a reference to its browser database from the
ambient GeckoProfile.  This ticket tracks inverting that control, so
that GeckoProfile doesn't have any knowledge of the (Fennec specific!)
browser databases.

See https://reviewboard.mozilla.org/r/60466/, which includes a simple
two-step commit process with IntelliJ automation commands for
achieving this.

There's additional clean-up possible here: the StubBrowserDB is
already outdated and can be removed entirely once the DB factory is
simplified.

Comment 1

[Nick Alexander :nalexander [he/him] (PTO Feb 24-March 3)]

jchen: I think this is the second thing to address, tied with Bug 1291383.

Comment 2

[Nick Alexander :nalexander [he/him] (PTO Feb 24-March 3)]

Attachment: Bug 1291384 - Part 1: Move BrowserDB access out of GeckoProfile into GeckoApplication.

This is the first half of the equation -- updating the callers.  This
was automated using IntelliJ's "Replace Structurally ..." operation, with
the template:

$Instance$.getDB() -> GeckoApplication.getDB($Instance$)

Review commit: https://reviewboard.mozilla.org/r/69344/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/69344/

Comment 3

[Nick Alexander :nalexander [he/him] (PTO Feb 24-March 3)]

Attachment: Bug 1291384 - Part 2: Inline BrowserDB factory into GeckoApplication.

These two patches will be folded together for landing.

Review commit: https://reviewboard.mozilla.org/r/69346/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/69346/

Comment 4

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

Comment on attachment 8777944 [details]
Bug 1291384 - Part 1: Move BrowserDB access out of GeckoProfile into GeckoApplication.

https://reviewboard.mozilla.org/r/69344/#review66748

::: mobile/android/base/java/org/mozilla/gecko/EditBookmarkDialog.java:26
(Diff revision 1)
>  import android.text.TextWatcher;
>  import android.view.LayoutInflater;
>  import android.view.View;
>  import android.widget.EditText;
>  
> +import static org.mozilla.gecko.GeckoProfile.get;

Hm why is IntelliJ doing this?

::: mobile/android/base/java/org/mozilla/gecko/EditBookmarkDialog.java:147
(Diff revision 1)
>       *            dialog will fail silently. If the url is bookmarked multiple times, this will only show
>       *            information about the first it finds.
>       */
>      public void show(final String url) {
>          final ContentResolver cr = mContext.getContentResolver();
> -        final BrowserDB db = GeckoProfile.get(mContext).getDB();
> +        final BrowserDB db = GeckoApplication.getDB(get(mContext));

We end up with these `get` calls that are confusing.

Comment 5

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

Comment on attachment 8777945 [details]
Bug 1291384 - Part 2: Inline BrowserDB factory into GeckoApplication.

https://reviewboard.mozilla.org/r/69346/#review66750

::: mobile/android/base/java/org/mozilla/gecko/GeckoApplication.java:221
(Diff revision 1)
> +        BrowserDB db = sDBCache.get(profileName);
> +        if (db != null) {
> +            return db;
> +        }
> +        db = new LocalBrowserDB(profileName);
> +        sDBCache.put(profileName, db);

You should use `sDBCache.putIfAbsent` to avoid a race condition where two threads call `sDBCache.put` at the same time, and end up returning different `LocalBrowserDB` instances for the same profile.

Comment 6

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

Still working on this, Nick? Or should I take it over?

Comment 7

[Nick Alexander :nalexander [he/him] (PTO Feb 24-March 3)]

(In reply to Jim Chen [:jchen] [:darchons] from comment #6)
> Still working on this, Nick? Or should I take it over?

Please, take over.

Comment 8

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

Attachment: 1. Take BrowserDB out of GeckoProfile (v1)

Remove references to BrowserDB and its factory from GeckoProfile.
Instead of keeping the DB in mDB, GeckoProfile now keeps an arbitrary
object in mData. Using a data object lets us avoid needing another map
to map profiles to DBs. This feature could be very useful for GeckoView
consumers as well.

The new way to get a BrowserDB from a profile/context is through
BrowserDB.from(Context) or BrowserDB.from(GeckoProfile), which takes
care of creating a local DB if necessary and associating the DB with the
profile.

Comment 9

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

Attachment: 2. Convert GeckoProfile.getDB calls to BrowserDB.from calls (v1)

Mass convert GeckoProfile.getDB() calls to BrowserDB.from() calls,
whether using Context or GeckoProfile.

Comment 10

[Nick Alexander :nalexander [he/him] (PTO Feb 24-March 3)]

Comment on attachment 8789843 [details] [diff] [review]
2. Convert GeckoProfile.getDB calls to BrowserDB.from calls (v1)

Stamp!

Comment 11

[Nick Alexander :nalexander [he/him] (PTO Feb 24-March 3)]

Comment on attachment 8789842 [details] [diff] [review]
1. Take BrowserDB out of GeckoProfile (v1)

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

I'm willing to take an Object for the associated data, but consider strongly paving the way for the next consumer by defining a Holder and extracting the BrowserDB out of that rather than casting data to BrowserDB directly.

This really needs comments on expected use and concurrency risks.  Perhaps setData should be harder to get at?

r+ since there's no real need for re-review.

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserDB.java
@@ +181,5 @@
> +    }
> +
> +    public static BrowserDB from(final GeckoProfile profile) {
> +        synchronized (profile.getLock()) {
> +            BrowserDB db = (BrowserDB) profile.getData();

Catch `ClassCastException` and do decent things, please.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoProfile.java
@@ +340,2 @@
>  
> +    public Object getData() {

This needs strong words about use, opacity, and locking.

@@ +343,3 @@
>      }
>  
> +    public void setData(final Object data) {

As does this.

Comment 12

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

(In reply to Nick Alexander :nalexander from comment #11)
> Comment on attachment 8789842 [details] [diff] [review]
> 1. Take BrowserDB out of GeckoProfile (v1)
> 
> Review of attachment 8789842 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm willing to take an Object for the associated data, but consider strongly
> paving the way for the next consumer by defining a Holder and extracting the
> BrowserDB out of that rather than casting data to BrowserDB directly.

IMO BrowserDB itself is already holder object. We already use it for a variety of things, and future code is likely to add to BrowserDB instead of using getData/setData directly.

> Catch `ClassCastException` and do decent things, please.

Currently, we only use BrowserDB with get/setData, so ClassCastException is not expected. If we do get a ClassCastException, we should crash and the crash should be easy enough to spot and fix.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/46f54a6f6aea
1. Take BrowserDB out of GeckoProfile; r=nalexander
https://hg.mozilla.org/integration/mozilla-inbound/rev/61e6762b20d0
2. Convert GeckoProfile.getDB calls to BrowserDB.from calls; r=nalexander

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/46f54a6f6aea
https://hg.mozilla.org/mozilla-central/rev/61e6762b20d0