AndroidBrowserDB/BrowserProvider cleanup

VERIFIED FIXED in Firefox 15

Status

()

Firefox for Android
General
P2
normal
VERIFIED FIXED
6 years ago
5 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)

(Assignee)

Description

6 years ago
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.

Updated

6 years ago
Assignee: nobody → bnicholson
tracking-fennec: --- → 12+
Priority: -- → P3
(Assignee)

Comment 1

5 years ago
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
(Assignee)

Updated

5 years ago
Depends on: 738875
We decided that this bug is important and wanted, but not explicitly needed for the initial release
blocking-fennec1.0: beta+ → -
(Assignee)

Comment 5

5 years ago
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.
(Assignee)

Comment 6

5 years ago
Created attachment 611668 [details] [diff] [review]
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.
(Assignee)

Comment 7

5 years ago
Created attachment 611669 [details] [diff] [review]
part 1: import favicon urls

uploaded wrong patch
Attachment #611668 - Attachment is obsolete: true
(Assignee)

Comment 8

5 years ago
Created attachment 611670 [details] [diff] [review]
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.
(Assignee)

Comment 9

5 years ago
Created attachment 611671 [details] [diff] [review]
part 3: import favicons

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
Blocks: 748342
tracking-fennec: 12+ → 16+
(Assignee)

Comment 12

5 years ago
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
(Assignee)

Comment 14

5 years ago
Created attachment 650355 [details] [diff] [review]
Part 1: Remove AndroidBrowserDB

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)
(Assignee)

Comment 15

5 years ago
Created attachment 650356 [details] [diff] [review]
Part 2: Add missing breaks in database upgrade logic

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+
(Assignee)

Comment 18

5 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/ab946a12992a
http://hg.mozilla.org/integration/mozilla-inbound/rev/8e5ce6580352
Whiteboard: [leave open]
Maybe split this bug?
Status: NEW → ASSIGNED
Target Milestone: --- → Firefox 17
https://hg.mozilla.org/mozilla-central/rev/ab946a12992a
https://hg.mozilla.org/mozilla-central/rev/8e5ce6580352
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+
(Assignee)

Updated

5 years ago
Blocks: 784086
(Assignee)

Comment 22

5 years ago
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]
(Assignee)

Updated

5 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Comment 23

5 years ago
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?
(Assignee)

Updated

5 years ago
status-firefox15: --- → affected
status-firefox16: --- → affected
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+
(Assignee)

Comment 25

5 years ago
http://hg.mozilla.org/releases/mozilla-aurora/rev/a09e54d947f2
status-firefox16: affected → fixed
(Assignee)

Comment 26

5 years ago
http://hg.mozilla.org/releases/mozilla-beta/rev/7d3fc54fb002
status-firefox15: affected → fixed
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
status-firefox15: fixed → verified
status-firefox16: fixed → verified
You need to log in before you can comment on or make changes to this bug.