Remove moz_bookmarks_roots

RESOLVED FIXED in Firefox 48

Status

()

Toolkit
Places
P2
normal
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: mak, Assigned: Cédric Desgranges, Mentored)

Tracking

(Blocks: 1 bug)

unspecified
mozilla48
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

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

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

3 years ago
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.
(Reporter)

Updated

3 years ago
Depends on: 1071505
(Reporter)

Updated

3 years ago
Priority: -- → P2
(Reporter)

Comment 1

2 years ago
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.
Mentor: mak77
Whiteboard: [good first bug][lang=cpp]
(Assignee)

Comment 2

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

Comment 4

2 years ago
Created attachment 8724770 [details] [diff] [review]
bug1143712.patch

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

Comment 5

2 years ago
Review ping ?
(Reporter)

Comment 6

2 years ago
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.
(Reporter)

Comment 7

2 years ago
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
(Reporter)

Updated

2 years ago
Flags: needinfo?(ceddesgranges)
(Assignee)

Comment 8

2 years ago
Created attachment 8730260 [details] [diff] [review]
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.
Attachment #8724770 - Attachment is obsolete: true
Flags: needinfo?(ceddesgranges)
Attachment #8730260 - Flags: review+
(Reporter)

Comment 9

2 years ago
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 :)
Attachment #8730260 - Flags: review+ → review?(mak77)
(Reporter)

Comment 10

2 years ago
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.
Attachment #8730260 - Flags: review?(mak77) → review+
(Reporter)

Updated

2 years ago
Flags: needinfo?(mak77)
(Reporter)

Comment 12

2 years ago
Created attachment 8731190 [details] [diff] [review]
1143712.patch
Attachment #8730795 - Attachment is obsolete: true
Flags: needinfo?(mak77)

Comment 14

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/db96206f80c8
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
(Assignee)

Comment 15

2 years ago
Thank you Marco for your mentorship and your patience on my first bug fix.
(Reporter)

Comment 16

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