There are a couple reasons: 1. the service listens to bookmarks changes to clean up tags, the implementer shouldn't rely on it being active since there's nothing enforcing that condition. if a bookmark is removed before the service is up, we don't properly cleanup tags 2. on startup it basically just adds the observer, this is the same as registering in the category the only downside is that someone using Places but not tags will have this component inited, though on startup it does nothing, so it barely pays the price of an additional loaded service.

the fix should be straight-forward, an xpcshell test is possible I guess. I'd only suggest to do a try run before pushing since some test may (wrongly) not expect tags notifications and it should be updated in such a case

The scope here is to modify the manifest where tagging service is defined, so that this service is registered in the bookmark-observers category, for example http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/toolkitplaces.manifest#20 Then we can remove all the code in function TaggingService() that so far is only registering it as an observer (it would be already) and registering a shutdown observer that is no more needed since the only thing it does is removing the observer.

Hi, I think this bug is still available so I would like to work on this.I have already built Firefox source code in windows but I'm new here could you guide me to start the things?

Hi sorry for late reply. If you already have the build, you can start searching for the code to change through services like http://dxr.mozilla.org/ or http://searchfox.org/ Comment 4 contains the description of the code changes: 1. modify the manifest so that tagging service is registered in the bookmark-observers category (see how PlacesCategoriesStarter does that http://searchfox.org/mozilla-central/source/toolkit/components/places/toolkitplaces.manifest#20) 2. remove code from nsTaggingService that registers it as a bookmarks observer 3. remove code from nsTaggingService that registers it as a shutdown observer (and the observing code as well)

I'm trying this as my first bug. As per your instructions, I edited the manifest and nsTaggingService.js files. Could you guide me further

well, there isn't much more to do after those changes. You should attach a patch, you can do either through moz-review (http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview-user.html) or just exporting a diff with a commit message and attaching it to the bug, adding a review flag on me.

Comment on attachment 8853063 [details] Fix to Bug 820876 - Use bookmarks-observer category for the tagging service [:mak] https://reviewboard.mozilla.org/r/125154/#review128442 it looks good, some code can still be removed, and let's see the results for the Try server build I just started. ::: toolkit/components/places/nsTaggingService.js:16 (Diff revision 1) > Components.utils.import("resource://gre/modules/Services.jsm"); > Components.utils.import("resource://gre/modules/PlacesUtils.jsm"); > XPCOMUtils.defineLazyModuleGetter(this, "Deprecated", > "resource://gre/modules/Deprecated.jsm"); > > const TOPIC_SHUTDOWN = "places-shutdown"; This can now be removed, as well as the whole "observe" method

Comment on attachment 8853063 [details] Fix to Bug 820876 - Use bookmarks-observer category for the tagging service [:mak] https://reviewboard.mozilla.org/r/125154/#review128444 ::: toolkit/components/places/nsTaggingService.js:462 (Diff revision 1) > _xpcom_factory: XPCOMUtils.generateSingletonFactory(TaggingService), > > QueryInterface: XPCOMUtils.generateQI([ > Ci.nsITaggingService > , Ci.nsINavBookmarkObserver > , Ci.nsIObserver Oh, also this Ci.nsIObserver can be removed

There are some failures to handle. TEST-UNEXPECTED-FAIL | toolkit/components/places/tests/bookmarks/test_bookmarks_notifications.js | remove_bookmark_tag_notification - [remove_bookmark_tag_notification : 500] [{"name":"onItemRemoved","arguments":[23,4,1,2,null,"t1XlwHyLFXtx","tags________",0]},{"name":"onItemRemoved","arguments":[24,23 deepEqual [{"name":"onItemRemoved","arguments":[24,23,0,1,"http://untag.example.com/","ZwZJ-fAEMQHw","t1XlwHyLFXtx",0]},{"name":"onItemCha TEST-UNEXPECTED-FAIL | toolkit/components/places/tests/bookmarks/test_nsINavBookmarkObserver.js | onItemChanged_tags_bookmark - [onItemChanged_tags_bookmark : 22] onItemRemoved(args[3]: itemType) - false == true It may be an order of notifications problem, since now the tagging service registers earlier, but it's more plausible it's due to the tagging service removing a tag folder (and thus generating an additional onItemRemoved) when all the tagged uris for a given tag are removed. You can run these tests using ./mach test path_to_test

to be clear, the tests should be fixed in this case.

Added tagging-service as bookmarks-observer in toolkitplaces.manifest. Removed code that adds taggingService as a bookmarks-observer and shutdown-observer from nsTagggingService.js. Removed the Components.utils.import code that imports Services.jsm, PlacesUtils.jsm, and Deprecated.jsm and the constant referencing `places-shutdown`. Removed the observe method for nsIObserver and its corresponding reference in QueryInterface: XPCOMUtils.generateQI()

Comment on attachment 8882205 [details] [diff] [review] Use bookmarks-observer category for the tagging service (In reply to Marco Bonardo [::mak] from comment #13) > to be clear, the tests should be fixed in this case. So I'm supposed to work on the toolkit/components/places/tests/bookmarks/test_bookmarks_notifications.js file? This bit wasn't so clear to me.

(In reply to Leni Kadali from comment #15) > So I'm supposed to work on the > toolkit/components/places/tests/bookmarks/test_bookmarks_notifications.js > file? This bit wasn't so clear to me. yes. and test_nsINavBookmarkObserver.js.

Comment on attachment 8882205 [details] [diff] [review] Use bookmarks-observer category for the tagging service Review of attachment 8882205 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/places/nsTaggingService.js @@ -10,5 @@ > Components.utils.import("resource://gre/modules/XPCOMUtils.jsm"); > -Components.utils.import("resource://gre/modules/Services.jsm"); > -Components.utils.import("resource://gre/modules/PlacesUtils.jsm"); > -XPCOMUtils.defineLazyModuleGetter(this, "Deprecated", > - "resource://gre/modules/Deprecated.jsm"); why did you remove Services.jsm, PlacesUtils.jsm and Deprecated.jsm? They are still used, even after your removals. PlacesUtils should just probably be changed to use defineLazyModuleGetter.

(In reply to Marco Bonardo [::mak] from comment #17) > ::: toolkit/components/places/nsTaggingService.js > @@ -10,5 @@ > > Components.utils.import("resource://gre/modules/XPCOMUtils.jsm"); > > -Components.utils.import("resource://gre/modules/Services.jsm"); > > -Components.utils.import("resource://gre/modules/PlacesUtils.jsm"); > > -XPCOMUtils.defineLazyModuleGetter(this, "Deprecated", > > - "resource://gre/modules/Deprecated.jsm"); > > why did you remove Services.jsm, PlacesUtils.jsm and Deprecated.jsm? They > are still used, even after your removals. Are they supposed to be used or removing the other mentions in this file is part of the bug as well?

(In reply to Leni Kadali from comment #18) > > why did you remove Services.jsm, PlacesUtils.jsm and Deprecated.jsm? They > > are still used, even after your removals. > Are they supposed to be used or removing the other mentions in this file is > part of the bug as well? They are supposed to be used yet.

Added tagging-service as bookmarks-observer in toolkitplaces.manifest. Removed code that adds taggingService as a bookmarks-observer and shutdown-observer from nsTagggingService.js. Removed `const TOPIC_SHUTDOWN = "places-shutdown";` and the whole observe method. Removed nsIObserver and its corresponding reference in QueryInterface: XPCOMUtils.generateQI()

Comment on attachment 8885842 [details] [diff] [review] Use bookmarks-observer category for the tagging service I hope this is the correct version of the patch where I haven't deleted anything unnecessarily. If this is the case, then I will move to working on the tests `test_nsINavBookmarkObserver.js` and `test_bookmarks_notifications.js`.

(In reply to Marco Bonardo [::mak] from comment #17) > PlacesUtils should just probably be changed to use defineLazyModuleGetter. How would this be done? Something along the lines of `PlacesUtils.defineLazyModuleGetter()` like XPCOMUtils? If so, which module would need to be referenced?

(In reply to Leni Kadali from comment #22) > (In reply to Marco Bonardo [::mak] from comment #17) > > PlacesUtils should just probably be changed to use defineLazyModuleGetter. > How would this be done? Something along the lines of > `PlacesUtils.defineLazyModuleGetter()` like XPCOMUtils? If so, which module > would need to be referenced? XPCOMUtils.defineLazyModuleGetter(this, "PlacesUtils", "resource://gre/modules/PlacesUtils.jsm");

Comment on attachment 8885842 [details] [diff] [review] Use bookmarks-observer category for the tagging service Review of attachment 8885842 [details] [diff] [review]: ----------------------------------------------------------------- This one looks OK, it would be nice to change PlacesUtils to be a lazy getter, but it's not critical. Now, there are still the test failures in comment 12 to handle, likely by changing the order of the notifications. You can run the tests using ./mach test path_to_test

(In reply to Marco Bonardo [::mak] from comment #24) > Now, there are still the test failures in comment 12 to handle, likely by > changing the order of the notifications. How exactly do I go about changing the order of notifications? I tried moving them around but the error persists. According to the last bit of the test output here: 0:05.42 TEST_STATUS: Thread-3 remove_bookmark_tag_notification FAIL [remove_bookmark_tag_notification : 550] [{"name":"onItemRemoved","arguments":[23,4,1,2,null,"UeJ8d_ss3J1O","tags________",0]},{"name":"onItemRemoved","arguments":[24,23 deepEqual [{"name":"onItemRemoved","arguments":[24,23,0,1,"http://untag.example.com/","xRjIRB6ZFYUz","UeJ8d_ss3J1O",0]},{"name":"onItemCha /home/herabus/src/mozilla-central/obj-x86_64-pc-linux-gnu/_tests/xpcshell/toolkit/components/places/tests/bookmarks/test_bookmarks_notifications.js:expectNotifications/get/<:550 /home/herabus/src/mozilla-central/obj-x86_64-pc-linux-gnu/_tests/xpcshell/toolkit/components/places/tests/bookmarks/test_bookmarks_notifications.js:remove_bookmark_tag_notification:321 0:05.42 LOG: Thread-3 INFO exiting test 0:05.43 LOG: Thread-3 ERROR Unexpected exception 2147500036 It seems to me that the error lies within the `observer.check` method, not the order of notifications as I understand it, which would point to the cause being the tagging service removing a tag folder (and thus generating an additional onItemRemoved) when all the tagged uris for a given tag are removed as you said in comment 12

(In reply to Leni Kadali [:leni1] [GMT+3] from comment #25) > It seems to me that the error lies within the `observer.check` method, not > the order of notifications as I understand it, which would point to the > cause being the tagging service removing a tag folder (and thus generating > an additional onItemRemoved) when all the tagged uris for a given tag are > removed as you said in comment 12 Mine was a guess, your suggestion is plausible if the test before was not initing the tagging service for some reason, otherwise in both cases we should have a notification about the folder going away when it contains no tags. I'd suggest to print out in the check function the whole notifications array, compare how it was earlier and how it is now, and compare with the db contents through dump_table("moz_bookmarks"); Then you should be able to explain what changes before/after. If those changes make sense, you can just update the expected array to the new one.

Let's wontfix this, we should merge tags into bookmarks in the future, and the categories have been removed.