Closed Bug 1291384 Opened 3 years ago Closed 3 years ago

[geckoview] Make GeckoProfile Fennec BrowserDB agnostic

Categories

(GeckoView :: General, defect)

defect
Not set

Tracking

(firefox51 fixed)

RESOLVED FIXED
Tracking Status
firefox51 --- fixed

People

(Reporter: nalexander, Assigned: jchen)

References

Details

Attachments

(4 files)

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.
jchen: I think this is the second thing to address, tied with Bug 1291383.
Assignee: nobody → nchen
Status: NEW → ASSIGNED
Blocks: 1291385
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/
Attachment #8777944 - Flags: review?(nchen)
Attachment #8777945 - Flags: review?(nchen)
Attachment #8777944 - Flags: review?(nchen)
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 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.
Attachment #8777945 - Flags: review?(nchen)
Assignee: nchen → nalexander
Still working on this, Nick? Or should I take it over?
Flags: needinfo?(nalexander)
(In reply to Jim Chen [:jchen] [:darchons] from comment #6)
> Still working on this, Nick? Or should I take it over?

Please, take over.
Flags: needinfo?(nalexander)
Assignee: nalexander → nchen
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.
Attachment #8789842 - Flags: review?(s.kaspari)
Mass convert GeckoProfile.getDB() calls to BrowserDB.from() calls,
whether using Context or GeckoProfile.
Attachment #8789843 - Flags: review?(s.kaspari)
Attachment #8789842 - Flags: review?(s.kaspari) → review?(nalexander)
Attachment #8789843 - Flags: review?(s.kaspari) → review?(nalexander)
Comment on attachment 8789843 [details] [diff] [review]
2. Convert GeckoProfile.getDB calls to BrowserDB.from calls (v1)

Stamp!
Attachment #8789843 - Flags: review?(nalexander) → review+
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.
Attachment #8789842 - Flags: review?(nalexander) → review+
(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.
Pushed by nchen@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/46f54a6f6aea
https://hg.mozilla.org/mozilla-central/rev/61e6762b20d0
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Product: Firefox for Android → GeckoView
Target Milestone: Firefox 51 → ---
You need to log in before you can comment on or make changes to this bug.