Closed Bug 1143712 Opened 6 years ago Closed 5 years ago

Remove moz_bookmarks_roots


(Toolkit :: Places, defect, P2)




Tracking Status
firefox48 --- fixed


(Reporter: mak, Assigned: ceddesgranges, Mentored)


(Blocks 1 open bug)


(Whiteboard: [good first bug][lang=cpp])


(1 file, 3 obsolete files)

In Firefox 36 we expose constant guid roots, and thus we don't need anymore the roots table.
We will wait at least until Firefox 40 to do the removal though.
Depends on: 1071505
Priority: -- → P2
This requires changing
to remove references to moz_bookmarks_roots

the only thing that should not be modified is the MigrateV25Up() method.
Mentor: mak77
Whiteboard: [good first bug][lang=cpp]
Hi, I would like to work on this for my first contribution here.
I have already checked the files that require changing.

If you have any other advice, I'd be glad to listen to them.

Attached patch bug1143712.patch (obsolete) — Splinter Review
Thank you for the links.
I removed all the occurrences excepted the ones in MigrateV25Up method as asked.
Review ping ?
Oh, sorry, I missed this in the middle of bugmails, cause you didn't set the review flag.
When attaching a patch, please always set either review or feedback flag to the mentor.
When in need of information you can use the "Need more information" flag at the bottom of the bug.
Only one flag at a time is needed to simplify tracking.

I'll look at this now.
Comment on attachment 8724770 [details] [diff] [review]

Review of attachment 8724770 [details] [diff] [review]:

ok, there's one missing piece.
With these changes we will stop creating the table in new profiles, but we should also remove it from old profiles.

To do that, you need to bump DATABASE_SCHEMA_VERSION to 31, and add a MigrateV31Up() method Database.cpp/.h
In the method you should execute a "DROP TABLE IF EXISTS moz_bookmarks_roots"
then you should also bump CURRENT_SCHEMA_VERSION in toolkit/components/places/tests/head_common.js to 31

Finally, we need to create a new database file named placesV31.sqlite in toolkit\components\places\tests\migration. Ideally to do that one could just launch the build with your patch applied (./mach run after build succeeds with your patch), then copy the generated places.sqlite in the profile folder to placesV31.sqlite. At this point we need a last operation, that requires opening this db (can use Sqlite Manager add-on) and issuing the "VACUUM" query so it gets shrinked from 10MB to about 2MB.
But eventually I could take care of the file generation.

::: toolkit/components/places/Database.cpp
@@ +272,5 @@
>    rv = newRootStmt->BindUTF8StringByName(NS_LITERAL_CSTRING("guid"),
>                                           aGuid);
>    if (NS_FAILED(rv)) return rv;
>    rv = newRootStmt->Execute();
>    if (NS_FAILED(rv)) return rv;

you should also remove these bindings and execute, since they are related to newRootStmt that you removed.
Otherwise, this won't even compile.

If you already have a build you can rebuild using ./mach build binaries
Flags: needinfo?(ceddesgranges)
Attached patch bug1143712.patch (obsolete) — Splinter Review
Sorry about the fact that I didn't add the flag on my message, and thank you for all this help.
I updated my patch with all the new modifications.
I tried to VACUUM the places.sqlite using SQLite manager with SQL query or with the compact database option but it didn't work.
Attachment #8724770 - Attachment is obsolete: true
Flags: needinfo?(ceddesgranges)
Attachment #8730260 - Flags: review+
Comment on attachment 8730260 [details] [diff] [review]

Review of attachment 8730260 [details] [diff] [review]:

ok, but please set the review flag on a reviewer :)
Attachment #8730260 - Flags: review+ → review?(mak77)
Comment on attachment 8730260 [details] [diff] [review]

Review of attachment 8730260 [details] [diff] [review]:

Ok, there's only one mistake that is not bumping DATABASE_SCHEMA_VERSION in Database.h... but I can do that

I will add the missing db file and push to Try for you.
Attachment #8730260 - Flags: review?(mak77) → review+
Flags: needinfo?(mak77)
Attached patch 1143712.patchSplinter Review
Attachment #8730795 - Attachment is obsolete: true
Flags: needinfo?(mak77)
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Thank you Marco for your mentorship and your patience on my first bug fix.
thank you for your time and availability.
Feel free to pick up more bugs around, we don't lack them :)
You need to log in before you can comment on or make changes to this bug.