717428 - AndroidBrowserDB/BrowserProvider cleanup

Status: VERIFIED FIXED

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

Description

[Brian Nicholson (:bnicholson)]

We should store a reference to a page's favicon id rather than an actual favicon blob. If several pages use the same favicon (like http://www.google.com/m?q=foo and http://www.google.com/m?q=bar), this allows us to reuse the same favicon without duplicating it for each page entry in the database.

Comment 1

[Brian Nicholson (:bnicholson)]

The DB schema has changed a bit since this bug was filed, but the problem of storing duplicate favicons still exists. We create a duplicate entries in the images table for the same favicons. We can fix this by indexing the table based on the favicon URL itself rather than the URL of the page that uses it. This is what we do on desktop.

Comment 2

[Gian-Carlo Pascutto [:gcp]]

This would fix bug 716729 as well:

https://bugzilla.mozilla.org/show_bug.cgi?id=716729#c15

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

Lets start the investigation to make this happen

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

We decided that this bug is important and wanted, but not explicitly needed for the initial release

Comment 5

[Brian Nicholson (:bnicholson)]

Stopping work on this for now since it's no longer a blocker, but I'll add my WIP patches for when it get picked up again.

Comment 6

[Brian Nicholson (:bnicholson)]

Attachment: part 1: import favicon urls

We currently store favicon URLs in favicon_urls.db. This patch imports them into the images table and removes the obsolete favicon_urls.db.

Comment 7

[Brian Nicholson (:bnicholson)]

Attachment: part 1: import favicon urls

uploaded wrong patch

Comment 8

[Brian Nicholson (:bnicholson)]

Attachment: part 2: create favicons table

To prevents duplicates, we need a separate favicons table where each row is a unique favicon. The history table includes a new column, favicon_id, which allows each page to refer to a favicon entry.

Comment 9

[Brian Nicholson (:bnicholson)]

Attachment: part 3: import favicons

Imports favicon URLs/data from the images table into the new favicons table.

Comment 11

[Richard Newman [:rnewman]]

Note that we won't be able to sync favicons until the existing uniqueness constraint goes away, either by adjusting the current schema or applying this change. (Right now each page needs to have a different favicon GUID.)

Comment 12

[Brian Nicholson (:bnicholson)]

This change will create a favicons table, meaning the history/bookmarks tables will contain IDs pointing to these entries. This, in turn, requires factoring out history/bookmarks into a places DB, similar to what we do on desktop.

Comment 14

[Brian Nicholson (:bnicholson)]

Attachment: Part 1: Remove AndroidBrowserDB

I think AndroidBrowserDB is long gone, so we should go ahead and drop it.

Comment 15

[Brian Nicholson (:bnicholson)]

Attachment: Part 2: Add missing breaks in database upgrade logic

Seems like these typos here will be cause us to repeat steps our migration logic...

Comment 16

[Lucas Rocha (:lucasr)]

Comment on attachment 650355 [details] [diff] [review]
Part 1: Remove AndroidBrowserDB

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

Nice. I assume you grep'd the whole project to make sure there are references to AndroidBrowserDB anymore.

Comment 17

[Lucas Rocha (:lucasr)]

Comment on attachment 650356 [details] [diff] [review]
Part 2: Add missing breaks in database upgrade logic

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

Ouch.

Comment 18

[Brian Nicholson (:bnicholson)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/ab946a12992a
http://hg.mozilla.org/integration/mozilla-inbound/rev/8e5ce6580352

Comment 19

[Richard Newman [:rnewman]]

Maybe split this bug?

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ab946a12992a
https://hg.mozilla.org/mozilla-central/rev/8e5ce6580352

Comment 21

[Mark Finkle (:mfinkle) (use needinfo?)]

We want to make sure we get a full 6 weeks of testing on trunk before moving this change, so fx18 is the earliest vehicle for that.

Comment 22

[Brian Nicholson (:bnicholson)]

I should have put the patches here in a separate bug, so I've split this one and created bug 784086 for the DB schema refactor.

Comment 23

[Brian Nicholson (:bnicholson)]

Comment on attachment 650356 [details] [diff] [review]
Part 2: Add missing breaks in database upgrade logic

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: db migration could be slower
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): low risk
String or UUID changes made by this patch: none

We're missing some breaks when we do the database migration, meaning some steps are being repeated. This is likely hurting first run performance.

Comment 24

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 650356 [details] [diff] [review]
Part 2: Add missing breaks in database upgrade logic

Approving for branch uplift, low risk, mobile-only, improvements to first-run perf.

Comment 25

[Brian Nicholson (:bnicholson)]

http://hg.mozilla.org/releases/mozilla-aurora/rev/a09e54d947f2

Comment 26

[Brian Nicholson (:bnicholson)]

http://hg.mozilla.org/releases/mozilla-beta/rev/7d3fc54fb002

Comment 27

[Cristian Nicolae (:xti)]

Changes were applied for m-a and also for Firefox 15 and Firefox 16. Closing bug as verified fixed.

--
Device: Galaxy Note
OS: Android 4.0.4