Closed
Bug 901440
Opened 11 years ago
Closed 9 years ago
Add a maintenance task to remove binary annotations
Categories
(Toolkit :: Places, defect, P2)
Toolkit
Places
Tracking
()
RESOLVED
FIXED
mozilla46
Tracking | Status | |
---|---|---|
firefox46 | --- | fixed |
People
(Reporter: mak, Unassigned, Mentored)
References
(Blocks 1 open bug)
Details
(Keywords: perf, Whiteboard: [good first bug][lang=js])
Attachments
(1 file)
(deleted),
patch
|
mak
:
review+
|
Details | Diff | Splinter Review |
we should remove all existing binary annotations in a maintenance task.
Reporter | ||
Updated•11 years ago
|
Whiteboard: [mentor=mak][lang=js]
Reporter | ||
Updated•11 years ago
|
Blocks: PlacesDiet
Reporter | ||
Comment 1•10 years ago
|
||
The idea is to add a task to PlacesDBUtils.jsm that removes any existing binary annotation, and add such task to the idle maintenance tasks
Whiteboard: [mentor=mak][lang=js] → [good next bug][mentor=mak][lang=js]
Assignee | ||
Updated•10 years ago
|
Mentor: mak77
Whiteboard: [good next bug][mentor=mak][lang=js] → [good next bug][lang=js]
Reporter | ||
Updated•10 years ago
|
Whiteboard: [good next bug][lang=js] → [good second bug][lang=js]
Reporter | ||
Updated•10 years ago
|
Whiteboard: [good second bug][lang=js] → [good first bug][lang=js]
Hi~ I would like to work on this issue, but I only have some basic knowledge about JavaScript. Could you tell me more about this bug? Thank you very much! :)
Reporter | ||
Comment 3•10 years ago
|
||
Hi.
If you didn't yet, I suggest taking a look at these docs:
https://developer.mozilla.org/en-US/docs/Introduction
https://developer.mozilla.org/en-US/docs/Simple_Firefox_build
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ
https://developer.mozilla.org/en-US/docs/Mercurial_Queues
Now, let's look at what happens here.
Basically some time ago we removed an API that allowed to add binary data to a page in places.sqlite database. Even if we removed the API, we never removed the data eventually created by it.
The code we use to do maintenance on the database lives in PlacesDBUtils.jsm, it runs a series of "tasks" to cleanup a database.
you can look into _getBoundCoherenceStatements method where there is a list of SQL statements executed to do those cleanups.
Here, before "A.3" (see the code comments) we need to add a statement that will remove the binary annotations from moz_annos and moz_items_annos tables.
These annotations can be recognized cause they have type = 4 (writing a SQL for this removal should be trivial, but feel free to ask if you have doubts, you can use SQLITE Manager add-on to test queries, just don't do on your default profile!)
Then we need to write a test for this. The tests are in toolkit/components/places/tests/unit/test_preventive_maintenance.js you can copy the sub test A.2 and do the appropriate modifications, that is: in setup() create a fake binary annotation (type = 4) in both moz_annos and moz_items_annos(), finally in check() verify it has been removed.
Reporter | ||
Updated•10 years ago
|
Assignee: nobody → sabergeass
Status: NEW → ASSIGNED
(In reply to Marco Bonardo [::mak] from comment #3)
Thank you for your trust! :)
Last time when I rebuild mozilla source code I meet gcc-version problem. The system I use is Ubuntu 12.04 and gcc on it is default 4.6.3 but build mozilla need 4.8.2 or above :(
I will try it again after I find way to update my gcc and I comment to let you know I had work on this issue. :)
Reporter | ||
Comment 5•10 years ago
|
||
You might look here for some useful information https://developer.mozilla.org/en-US/docs/Simple_Firefox_build/Linux_and_MacOS_build_preparation
You might also ask on irc.mozilla.org on the channels #introduction or #build
(In reply to Marco Bonardo [::mak] from comment #5)
Sorry for the late reply, I must say my computer is too old to compiler Mozilla source code, I had try a lot of times these days but still have problem :(
So, I decide to leave this bug to other people who capable to fix that. Sorry :(
Oops.....I try it again and it works. Please forget I comment in comment #6 and I will commit my first ASAP :-)
(In reply to Marco Bonardo [::mak] from comment #3)
> Here, before "A.3" (see the code comments) we need to add a statement that
> will remove the binary annotations from moz_annos and moz_items_annos tables.
> These annotations can be recognized cause they have type = 4 (writing a SQL
> for this removal should be trivial, but feel free to ask if you have doubts,
> you can use SQLITE Manager add-on to test queries, just don't do on your
> default profile!)
Hi~ I don't know exactly type = 4 means. In my point of view, I should add some lines in deleteObsoleteAnnos and deleteObsoleteItemsAnnos functions to delete type 4 annotations. Could you tell me some more informations about how to pick up type 4 annotations? Thanks :)
Reporter | ||
Comment 9•10 years ago
|
||
If you open places.sqlite database using Sqlite Manager add-on, and you look at the moz_items_annos table, you see there's a type column with values (1, 2, 3 and 4). This is what type = 4 means.
The query to add should be something like:
"DELETE FROM moz_items_annos WHERE type = 4"
You should add this query after deleteObsoleteItemsAnnos, the code will look similar.
Comment 10•9 years ago
|
||
Hi Marco,
There are a bunch of things on my schedule and I'm afraid I can't finish this issue. Sorry about that :(
Please unassigned me from this bug. Thank you very much!
Reporter | ||
Comment 11•9 years ago
|
||
no worries, thank you for the notice.
The bug now has also more information for other contributors :)
Assignee: sabergeass → nobody
Status: ASSIGNED → NEW
Reporter | ||
Updated•9 years ago
|
Priority: -- → P2
Comment 12•9 years ago
|
||
I tried to fix the bug. This is my first bug hopefully this helps.
Attachment #8697873 -
Flags: review?(asaf)
Attachment #8697873 -
Flags: feedback?
Reporter | ||
Updated•9 years ago
|
Attachment #8697873 -
Flags: review?(mak77)
Attachment #8697873 -
Flags: review?(asaf)
Attachment #8697873 -
Flags: feedback?
Reporter | ||
Comment 13•9 years ago
|
||
Comment on attachment 8697873 [details] [diff] [review]
Added the lines to delete type 4
Review of attachment 8697873 [details] [diff] [review]:
-----------------------------------------------------------------
yep, it looks good, thanks
Attachment #8697873 -
Flags: review?(mak77) → review+
Comment 15•9 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox46:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in
before you can comment on or make changes to this bug.
Description
•