723623 - Remove legacy Android Browser code, BrowserDB, etc.

Status: RESOLVED INCOMPLETE

(Firefox for Android Graveyard :: General, defect, P3)

Description

[Richard Newman [:rnewman]]

10:23:37 < rnewman> lucasr, margaret: last word I got from mfinkle is that there is no plan to ever support global db again
10:24:03 < rnewman> last word being Tuesday, IIRC
10:24:59 < rnewman> and of course, Sync won't be adding data to the global DB
10:25:10 < mfinkle> rnewman, lucasr, margaret: yeah - let's move away from android DB support
10:25:47 < lucasr> mfinkle, I'm all for that
10:25:58 < lucasr> rnewman, mfinkle, could you file a bug to remove it then?
10:26:10 < lucasr> we don't need that thin abstraction layer anymore if we drop android db

I'll let y'all set flags and mark this as blocking or dependent…

Comment 1

[Lucas Rocha (:lucasr)]

Attachment: Remove AndroidBrowserDB as it won't be used anymore

Comment 2

[Lucas Rocha (:lucasr)]

Attachment: Remove abstraction layer from BrowserDB

This patch pretty much moves LocalBrowserDB to be BrowserDB and replaces all uses of the cursor column abstraction (BrowserDB.URLColumns) with columns defined in BrowserContract.

Comment 3

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 599937 [details] [diff] [review]
Remove abstraction layer from BrowserDB

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

I don't see any advantage to removing the abstraction layer

Comment 4

[Lucas Rocha (:lucasr)]

(In reply to Brad Lassey [:blassey] from comment #3)
> Comment on attachment 599937 [details] [diff] [review]
> Remove abstraction layer from BrowserDB
> 
> Review of attachment 599937 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't see any advantage to removing the abstraction layer

Note that the BrowserDB API remains unchanged which means that we'd still be able to add abstractions behind the API if want to in the future. The only thing this patch does is turning LocalBrowserDB into BrowserDB itself.

There is some small performance advantages in doing that. For instance, we don't need to abstract cursors to map columns names to different DB schemas anymore.

Both ProfileMigrator and our bookmarks UI are already using our LocalBrowserDB and BrowserProvider directly. i.e. they're bypassing the abstraction layer because Android has no concept of bookmarks folder in releases older than Honeycomb. In the specific case of ProfileMigrator, using the current abstraction layer is not really an option for performance reasons (see bug 721352).

Comment 5

[Lucas Rocha (:lucasr)]

Just to be clear: when I say "abstraction layer" what I mean is "the ability to *dynamically* switch different implementations of BrowserDB". This is what I'm removing here.

If we ever decide to move away from SQLite in the future, it should be just about reimplementing the ContentProvider code to use the new storage system. The front-end code, the BrowserDB implementation (which is just a user of the ContentProvider) would remain unchanged.

Comment 6

[Lucas Rocha (:lucasr)]

Brad, ping?

Comment 7

[Gian-Carlo Pascutto [:gcp]]

>If we ever decide to move away from SQLite in the future, it should be just about 
>reimplementing the ContentProvider code to use the new storage system.

This is what terribly sucks in Android: you can't really do this unless the new ContentProvider makes great efforts to at least pretend to support SQL-like Syntax. This is of course easy if it's really SQLite, but a huge pain and impedance mismatch if it isn't.

The BrowserDB API didn't have this issue. I don't think we should have bypassed it as we did, architecturally speaking. (I understand there's other constraints at work - we need to get a product out of the door)

Comment 8

[Lucas Rocha (:lucasr)]

(In reply to Gian-Carlo Pascutto (:gcp) from comment #7)
> >If we ever decide to move away from SQLite in the future, it should be just about 
> >reimplementing the ContentProvider code to use the new storage system.
> 
> This is what terribly sucks in Android: you can't really do this unless the
> new ContentProvider makes great efforts to at least pretend to support
> SQL-like Syntax. This is of course easy if it's really SQLite, but a huge
> pain and impedance mismatch if it isn't.

Fennec UI only uses BrowserDB API and never interacts with the ContentProvider directly (with the exception of the profile migration AFAIK).
 
> The BrowserDB API didn't have this issue. I don't think we should have
> bypassed it as we did, architecturally speaking. (I understand there's other
> constraints at work - we need to get a product out of the door)

I'm not removing BrowserDB API. I'm simply making its implementation static because we don't care about dynamically switching implementations anymore.

What we're bypassing is the column names. We're using BrowserContract info directly in the bookmarks API because we couldn't really abstract this for both local and Android DBs. BrowserContract will still make sense even if we move away from SQLite in the future.

Comment 9

[:Margaret Leibovic]

It looks like the first patch from here is going to land as part of bug 717428. I think we should revisit landing the second patch in here (it will likely need to be rewritten at this point), or close this as a WONTFIX if we really want to keep both LocalBrowserDB and BrowserDB around. However, without AndroidBrowserDB, it really doesn't seem like there's any point to keep an extra abstraction layer.

Comment 10

[Richard Newman [:rnewman]]

(In reply to Margaret Leibovic [:margaret] from comment #9)
> It looks like the first patch from here is going to land as part of bug
> 717428. I think we should revisit landing the second patch in here (it will
> likely need to be rewritten at this point), or close this as a WONTFIX if we
> really want to keep both LocalBrowserDB and BrowserDB around. However,
> without AndroidBrowserDB, it really doesn't seem like there's any point to
> keep an extra abstraction layer.

I encourage you to keep BrowserDB.BrowserDBIface (albeit with a better name, and perhaps lifted to its own file).

Implementing the DB class on an interface will have positive design effects, and will make life much easier when you're trying to implement mocks or crazy Metro-esque things like writing to two profiles at once.

Comment 11

[Lucas Rocha (:lucasr)]

The Android backend has already been removed. What I think we should still do anyway is to get rid of BrowserDB.URLColumns and simply use BrowserContract interfaces directly. 

BrowserDB.URLColumns was only necessary while we supported the Android's system bookmarks database.

Comment 12

[Richard Newman [:rnewman]]

The first half of this bug was done elsewhere (I'm guessing Bug 717428).

The second half isn't exactly the direction we want to move (we need to have multiple LocalBrowserDB's to support the share activity in Bug 1044794), but will be useful as inspiration for Bug 1050034.

Closing as INCOMPLETE.