Closed
Bug 1127763
Opened 9 years ago
Closed 9 years ago
Some database might be missing the favicons guid index
Categories
(Toolkit :: Places, defect)
Toolkit
Places
Tracking
()
RESOLVED
FIXED
mozilla41
Tracking | Status | |
---|---|---|
firefox41 | --- | fixed |
People
(Reporter: mak, Assigned: tvaleev, Mentored)
References
Details
(Whiteboard: [good next bug][lang=cpp])
Attachments
(1 file, 1 obsolete file)
18.84 KB,
patch
|
mak
:
review+
|
Details | Diff | Splinter Review |
CREATE_IDX_MOZ_FAVICONS_GUI is invoked on migration, but not on creation, that means new profiles might be missing this index.
Reporter | ||
Updated•9 years ago
|
Points: --- → 2
Flags: qe-verify-
Flags: in-testsuite?
Flags: firefox-backlog+
Reporter | ||
Updated•9 years ago
|
Mentor: mak77
Whiteboard: [good next bug][lang=cpp]
Assignee | ||
Comment 1•9 years ago
|
||
Hello. Is it needed to add ExecuteSimpleSQL(CREATE_IDX_MOZ_FAVICONS_GUID) to Database::InitSchema method in the else conditional branch (when is creating all tables and indices for new database)?
Flags: needinfo?(mak77)
Reporter | ||
Comment 2•9 years ago
|
||
(In reply to Timur Valeev from comment #1) > Hello. > Is it needed to add ExecuteSimpleSQL(CREATE_IDX_MOZ_FAVICONS_GUID) to > Database::InitSchema method in the else conditional branch (when is creating > all tables and indices for new database)? Yes, that would fix the reported problem. But it's not enough, indeed we would still have databases in the wild missing this index. For that, we also need a migration step, that means * bump DATABASE_SCHEMA_VERSION in Database.h * add a MigrateV29Up method that tries to create the index to Database.h/cpp * bump CURRENT_SCHEMA_VERSION in toolkit/components/places/tests/head common.js * add a places_v29.sqlite file into toolkit/components/places/tests/migration/ (for this it's enough to create a new profile with the bumped up compiled version, then using either sqlite3.exe or Sqlite Manager Add-on run VACUUM on the places.sqlite file from that profile). Eventually I can take care of creating the db if that's a problem. * run tests with ./mach xpcshell-test toolkit/components/places/
Flags: needinfo?(mak77)
Assignee | ||
Comment 3•9 years ago
|
||
Thank you for the information. Some of these days I'll try to create the modifications.
Reporter | ||
Comment 4•9 years ago
|
||
ok, let me assign the bug so you can track it in the bugzilla dashboard.
Assignee: nobody → tvaleev
Assignee | ||
Comment 5•9 years ago
|
||
Hi. I created the patch. But I'm not sure about places_v29.sqlite file. I used "Compact database" command from Sqlite Manager Add-on to new places.sqlite file but the size remains the same.
Attachment #8610171 -
Flags: review?(mak77)
Reporter | ||
Comment 6•9 years ago
|
||
Comment on attachment 8610171 [details] [diff] [review] bug-1127763-fix-0.1.patch Review of attachment 8610171 [details] [diff] [review]: ----------------------------------------------------------------- The patch looks good. I have a couple minor comments: ::: toolkit/components/places/Database.cpp @@ +753,5 @@ > > + if (currentSchemaVersion < 29) { > + rv = MigrateV29Up(); > + NS_ENSURE_SUCCESS(rv, rv); > + } please add a // Firefox 41 uses schema version 29. comment just below this migration (see previous ones) @@ +1619,5 @@ > + NS_ENSURE_SUCCESS(rv, rv); > + > + return NS_OK; > + > +} please remove the newline before the closing brace
Attachment #8610171 -
Flags: review?(mak77) → feedback+
Reporter | ||
Comment 7•9 years ago
|
||
(In reply to Timur Valeev from comment #5) > I used "Compact database" command from Sqlite Manager Add-on to new > places.sqlite file but the size remains the same. The db file should be about 1MB after vacuuming it.
Reporter | ||
Comment 8•9 years ago
|
||
also, please edit toolkit/components/places/tests/migration/xpcshell.ini and add the places sqlite file to it
Assignee | ||
Comment 9•9 years ago
|
||
Ok. I will change my patch. Yes db file is about 1MB (like places_v27.sqlite or places_v28.sqlite).
Assignee | ||
Comment 10•9 years ago
|
||
New patch
Attachment #8610171 -
Attachment is obsolete: true
Attachment #8611182 -
Flags: review?(mak77)
Reporter | ||
Updated•9 years ago
|
Attachment #8611182 -
Flags: review?(mak77) → review+
Reporter | ||
Comment 11•9 years ago
|
||
Thank you for your contribution, much appreciated! https://treeherder.mozilla.org/#/jobs?repo=try&revision=1efb7424460c
Keywords: checkin-needed
Reporter | ||
Updated•9 years ago
|
Flags: in-testsuite? → in-testsuite-
Assignee | ||
Comment 12•9 years ago
|
||
Thank you too. Could you recommend some interesting issues? :)
Reporter | ||
Comment 13•9 years ago
|
||
Sure, thank you for your help! I'm looking at the list of bugs I'm mentoring, but you can find more mentored bugs at https://developer.mozilla.org/en-US/docs/Introduction#Step_2_-_Find_something_to_work_on if you can't see anything of your interest here. if you want something very involving (Will tak some time), there is: Bug 1039200 - Add Migration for new tag tables it has patches from another contributor that stopped working on it due to his real job taking time. The patches should likely be updated to the current codebase, the second part patch needs comments addressed and then it needs a third part as explained in https://bugzilla.mozilla.org/show_bug.cgi?id=1039200#c21 note: in the meanwhile we introduced new APIs, so the API built there might need a little bit of refreshing to make it coherent. This is still in cpp world, should not take much time: Bug 1166166 - use mAsyncExecutionThreadIsAlive to shrink memory on the async thread when possible. These should be easier, in js: Bug 1092607 - Intermittent test_PlacesUtils_asyncGetBookmarkIds.js Bug 1167229 - Make nsBrowserGlue instantiations use nsISupports where nsIBrowserGlue is not needed Bug 416611 - support importing tags from bookmarks.html exports (del.icio.us and others) Bug 820876 - Use bookmarks-observer category for the tagging service Bug 962999 - Because of encoding error, users can not import bookmarks which exported from 360 browser Apart the first one that might require to dedicate some more time, the others should be feasible in spare time.
Assignee | ||
Comment 14•9 years ago
|
||
Thank you for the information!
Comment 15•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/5670c0c09c2c
Keywords: checkin-needed
Comment 16•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/5670c0c09c2c
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox41:
--- → fixed
Flags: in-testsuite- → in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in
before you can comment on or make changes to this bug.
Description
•