Closed Bug 717428 Opened 11 years ago Closed 10 years ago
Browser DB/Browser Provider cleanup
18.08 KB, patch
|Details | Diff | Splinter Review|
1.09 KB, patch
|Details | Diff | Splinter Review|
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.
Assignee: nobody → bnicholson
tracking-fennec: --- → 12+
Priority: -- → P3
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.
blocking-fennec1.0: --- → ?
This would fix bug 716729 as well: https://bugzilla.mozilla.org/show_bug.cgi?id=716729#c15
Lets start the investigation to make this happen
blocking-fennec1.0: ? → beta+
Priority: P3 → P2
We decided that this bug is important and wanted, but not explicitly needed for the initial release
blocking-fennec1.0: beta+ → -
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.
We currently store favicon URLs in favicon_urls.db. This patch imports them into the images table and removes the obsolete favicon_urls.db.
uploaded wrong patch
Attachment #611668 - Attachment is obsolete: true
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.
Imports favicon URLs/data from the images table into the new favicons table.
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.)
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.
Summary: Reuse favicons in DB → Change DB schema to more closely resemble places
I think AndroidBrowserDB is long gone, so we should go ahead and drop it.
Seems like these typos here will be cause us to repeat steps our migration logic...
Attachment #650356 - Flags: review?(lucasr.at.mozilla)
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.
Attachment #650355 - Flags: review?(lucasr.at.mozilla) → review+
Comment on attachment 650356 [details] [diff] [review] Part 2: Add missing breaks in database upgrade logic Review of attachment 650356 [details] [diff] [review]: ----------------------------------------------------------------- Ouch.
Attachment #650356 - Flags: review?(lucasr.at.mozilla) → review+
Whiteboard: [leave open]
Maybe split this bug?
Status: NEW → ASSIGNED
Target Milestone: --- → Firefox 17
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.
tracking-fennec: 16+ → 18+
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.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
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 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.
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
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.