Last Comment Bug 717428 - AndroidBrowserDB/BrowserProvider cleanup
: AndroidBrowserDB/BrowserProvider cleanup
Status: VERIFIED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: All Android
: P2 normal (vote)
: Firefox 17
Assigned To: Brian Nicholson (:bnicholson)
:
Mentors:
: 743330 748342 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-11 14:51 PST by Brian Nicholson (:bnicholson)
Modified: 2012-09-06 05:58 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
verified
verified
-


Attachments
part 1: import favicon urls (737 bytes, patch)
2012-04-02 17:45 PDT, Brian Nicholson (:bnicholson)
no flags Details | Diff | Splinter Review
part 1: import favicon urls (3.84 KB, patch)
2012-04-02 17:45 PDT, Brian Nicholson (:bnicholson)
no flags Details | Diff | Splinter Review
part 2: create favicons table (8.46 KB, patch)
2012-04-02 17:48 PDT, Brian Nicholson (:bnicholson)
no flags Details | Diff | Splinter Review
part 3: import favicons (6.68 KB, patch)
2012-04-02 17:49 PDT, Brian Nicholson (:bnicholson)
no flags Details | Diff | Splinter Review
Part 1: Remove AndroidBrowserDB (18.08 KB, patch)
2012-08-08 16:16 PDT, Brian Nicholson (:bnicholson)
lucasr.at.mozilla: review+
Details | Diff | Splinter Review
Part 2: Add missing breaks in database upgrade logic (1.09 KB, patch)
2012-08-08 16:19 PDT, Brian Nicholson (:bnicholson)
lucasr.at.mozilla: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Brian Nicholson (:bnicholson) 2012-01-11 14:51:05 PST
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) 2012-03-22 17:26:10 PDT
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] 2012-03-23 02:58:07 PDT
This would fix bug 716729 as well:

https://bugzilla.mozilla.org/show_bug.cgi?id=716729#c15
Comment 3 Mark Finkle (:mfinkle) (use needinfo?) 2012-03-23 11:37:12 PDT
Lets start the investigation to make this happen
Comment 4 Mark Finkle (:mfinkle) (use needinfo?) 2012-03-30 11:25:56 PDT
We decided that this bug is important and wanted, but not explicitly needed for the initial release
Comment 5 Brian Nicholson (:bnicholson) 2012-04-02 17:39:55 PDT
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) 2012-04-02 17:45:10 PDT
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.
Comment 7 Brian Nicholson (:bnicholson) 2012-04-02 17:45:50 PDT
Created attachment 611669 [details] [diff] [review]
part 1: import favicon urls

uploaded wrong patch
Comment 8 Brian Nicholson (:bnicholson) 2012-04-02 17:48:05 PDT
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.
Comment 9 Brian Nicholson (:bnicholson) 2012-04-02 17:49:32 PDT
Created attachment 611671 [details] [diff] [review]
part 3: import favicons

Imports favicon URLs/data from the images table into the new favicons table.
Comment 10 Gian-Carlo Pascutto [:gcp] 2012-04-09 04:35:13 PDT
*** Bug 743330 has been marked as a duplicate of this bug. ***
Comment 11 Richard Newman [:rnewman] 2012-04-17 10:04:59 PDT
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) 2012-07-30 13:30:02 PDT
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 13 Mark Finkle (:mfinkle) (use needinfo?) 2012-08-03 09:59:38 PDT
*** Bug 748342 has been marked as a duplicate of this bug. ***
Comment 14 Brian Nicholson (:bnicholson) 2012-08-08 16:16:24 PDT
Created attachment 650355 [details] [diff] [review]
Part 1: Remove AndroidBrowserDB

I think AndroidBrowserDB is long gone, so we should go ahead and drop it.
Comment 15 Brian Nicholson (:bnicholson) 2012-08-08 16:19:59 PDT
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...
Comment 16 Lucas Rocha (:lucasr) 2012-08-09 02:32:00 PDT
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) 2012-08-09 02:32:22 PDT
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 19 Richard Newman [:rnewman] 2012-08-09 11:33:49 PDT
Maybe split this bug?
Comment 21 Mark Finkle (:mfinkle) (use needinfo?) 2012-08-10 12:25:25 PDT
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) 2012-08-20 10:44:53 PDT
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) 2012-08-20 10:49:10 PDT
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 2012-08-20 11:51:32 PDT
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) 2012-08-20 21:32:49 PDT
http://hg.mozilla.org/releases/mozilla-aurora/rev/a09e54d947f2
Comment 26 Brian Nicholson (:bnicholson) 2012-08-20 21:34:54 PDT
http://hg.mozilla.org/releases/mozilla-beta/rev/7d3fc54fb002
Comment 27 Cristian Nicolae (:xti) 2012-09-06 05:58:03 PDT
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

Note You need to log in before you can comment on or make changes to this bug.