AndroidBrowserDB/BrowserProvider cleanup

VERIFIED FIXED in Firefox 15

Status

()

defect
P2
normal
VERIFIED FIXED
8 years ago
7 years ago

People

(Reporter: bnicholson, Assigned: bnicholson)

Tracking

unspecified
Firefox 17
All
Android
Points:
---

Firefox Tracking Flags

(firefox15 verified, firefox16 verified, blocking-fennec1.0 -)

Details

Attachments

(2 attachments, 4 obsolete attachments)

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: --- → ?
Lets start the investigation to make this happen
blocking-fennec1.0: ? → beta+
Priority: P3 → P2
Depends on: 738875
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.
Posted patch part 1: import favicon urls (obsolete) — Splinter Review
We currently store favicon URLs in favicon_urls.db. This patch imports them into the images table and removes the obsolete favicon_urls.db.
Posted patch part 1: import favicon urls (obsolete) — Splinter Review
uploaded wrong patch
Attachment #611668 - Attachment is obsolete: true
Posted patch part 2: create favicons table (obsolete) — Splinter Review
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.
Posted patch part 3: import favicons (obsolete) — Splinter Review
Imports favicon URLs/data from the images table into the new favicons table.
Duplicate of this bug: 743330
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.)
Blocks: 709404
tracking-fennec: 12+ → 16+
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
Duplicate of this bug: 748342
I think AndroidBrowserDB is long gone, so we should go ahead and drop it.
Attachment #611669 - Attachment is obsolete: true
Attachment #611670 - Attachment is obsolete: true
Attachment #611671 - Attachment is obsolete: true
Attachment #650355 - Flags: review?(lucasr.at.mozilla)
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+
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+
Blocks: 784086
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.
No longer blocks: 709404, 784086, 748342
tracking-fennec: 18+ → ---
No longer depends on: 738875
Summary: Change DB schema to more closely resemble places → AndroidBrowserDB/BrowserProvider cleanup
Whiteboard: [leave open]
Status: ASSIGNED → RESOLVED
Closed: 7 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.
Attachment #650356 - Flags: approval-mozilla-beta?
Attachment #650356 - Flags: approval-mozilla-aurora?
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.
Attachment #650356 - Flags: approval-mozilla-beta?
Attachment #650356 - Flags: approval-mozilla-beta+
Attachment #650356 - Flags: approval-mozilla-aurora?
Attachment #650356 - Flags: approval-mozilla-aurora+
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.