1143712 - Remove moz_bookmarks_roots

Status: RESOLVED FIXED

(Toolkit :: Places, defect, P2)

Description

[Marco Bonardo [:mak]]

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.

Comment 1

[Marco Bonardo [:mak]]

This requires changing
/toolkit/components/places/Database.cpp 
/toolkit/components/places/nsPlacesTables.h
to remove references to moz_bookmarks_roots

the only thing that should not be modified is the MigrateV25Up() method.

Comment 2

[Cédric Desgranges]

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.

Thanks

Comment 3

[Marco Bonardo [:mak]]

Hi, it shouldn't be too hard.
You can have a look at these articles:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build
http://mozilla-version-control-tools.readthedocs.org/en/latest/hgmozilla/index.html
https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Comment 4

[Cédric Desgranges]

Attachment: bug1143712.patch

Thank you for the links.
I removed all the occurrences excepted the ones in MigrateV25Up method as asked.

Comment 5

[Cédric Desgranges]

Review ping ?

Comment 6

[Marco Bonardo [:mak]]

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 7

[Marco Bonardo [:mak]]

Comment on attachment 8724770 [details] [diff] [review]
bug1143712.patch

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

Comment 8

[Cédric Desgranges]

Attachment: bug1143712.patch

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.

Comment 9

[Marco Bonardo [:mak]]

Comment on attachment 8730260 [details] [diff] [review]
bug1143712.patch

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

ok, but please set the review flag on a reviewer :)

Comment 10

[Marco Bonardo [:mak]]

Comment on attachment 8730260 [details] [diff] [review]
bug1143712.patch

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.

Comment 11

[Marco Bonardo [:mak]]

Attachment: 1143712.patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=44efe63c4e71

Comment 12

[Marco Bonardo [:mak]]

Attachment: 1143712.patch

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/db96206f80c8

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/db96206f80c8

Comment 15

[Cédric Desgranges]

Thank you Marco for your mentorship and your patience on my first bug fix.

Comment 16

[Marco Bonardo [:mak]]

thank you for your time and availability.
Feel free to pick up more bugs around, we don't lack them :)