Closed
Bug 374754
Opened 18 years ago
Closed 18 years ago
nsCategoryManager::AddCategoryEntry should notify about removal of the previous value
Categories
(Core :: XPCOM, defect)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
People
(Reporter: jwkbugzilla, Assigned: jwkbugzilla)
References
Details
Attachments
(1 file, 1 obsolete file)
3.79 KB,
patch
|
jwkbugzilla
:
review+
darin.moz
:
superreview+
|
Details | Diff | Splinter Review |
Right now if you use nsICategoryManager::AddCategoryEntry with aReplace parameter set it will only send notifications about the category entry added but not about the one removed/replaced. So for example, if you have a content policy registered and then autoregistration is re-run you end up with two instances of your content policy registered: one instance was already registered on startup and when autoregistration called nsICategoryManager::AddCategoryEntry another one has been added. I think we need this change:
if (NS_SUCCEEDED(rv)) {
+ if (*retval) {
+ NotifyObservers(NS_XPCOM_CATEGORY_ENTRY_REMOVED_OBSERVER_ID,
+ aCategoryName, *retval);
+ }
NotifyObservers(NS_XPCOM_CATEGORY_ENTRY_ADDED_OBSERVER_ID,
aCategoryName, aEntryName);
}
Will test it later.
Assignee | ||
Comment 1•18 years ago
|
||
Assignee | ||
Comment 2•18 years ago
|
||
Typo in the comment - "if the called" should be "if the caller" of course.
Assignee | ||
Updated•18 years ago
|
Attachment #264630 -
Flags: review?(cbiesinger) → review?(darin.moz)
Comment 3•18 years ago
|
||
Comment on attachment 264630 [details] [diff] [review]
Proposed patch and testcase
><html><body><pre style="word-wrap: break-word; white-space: pre-wrap;">Index: xpcom/components/nsCategoryManager.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/xpcom/components/nsCategoryManager.cpp,v
>retrieving revision 1.51
>diff -p -u -8 -r1.51 nsCategoryManager.cpp
>--- xpcom/components/nsCategoryManager.cpp 10 May 2006 17:30:08 -0000 1.51
>+++ xpcom/components/nsCategoryManager.cpp 13 May 2007 00:51:04 -0000
>@@ -619,24 +619,35 @@ nsCategoryManager::AddCategoryEntry( con
> char* categoryName = ArenaStrdup(aCategoryName, &mArena);
> mTable.Put(categoryName, category);
> }
> PR_Unlock(mLock);
>
> if (!category)
> return NS_ERROR_OUT_OF_MEMORY;
>
>+ // We will need the return value of AddLeaf even if the called doesn't want it
>+ char *oldEntry = nsnull;
>+ if (!_retval)
>+ _retval = &oldEntry;
>+
might be slightly cleaner to just use oldEntry directory, and then copy oldEntry to *_retval if _retval is non-null.
otherwise, this change looks fine to me.
> nsresult rv = category->AddLeaf(aEntryName,
> aValue,
> aPersist,
> aReplace,
> _retval,
> &mArena);
>
> if (NS_SUCCEEDED(rv)) {
>+ if (*_retval) {
>+ NotifyObservers(NS_XPCOM_CATEGORY_ENTRY_REMOVED_OBSERVER_ID,
>+ aCategoryName, *_retval);
>+ if (oldEntry)
>+ nsMemory::Free(oldEntry);
>+ }
> NotifyObservers(NS_XPCOM_CATEGORY_ENTRY_ADDED_OBSERVER_ID,
> aCategoryName, aEntryName);
> }
>
> return rv;
> }
>
> NS_IMETHODIMP
>Index: xpcom/tests/unit/test_bug374754.js
>===================================================================
>RCS file: xpcom/tests/unit/test_bug374754.js
>diff -N xpcom/tests/unit/test_bug374754.js
>--- /dev/null 1 Jan 1970 00:00:00 -0000
>+++ xpcom/tests/unit/test_bug374754.js 13 May 2007 00:44:44 -0000
>@@ -0,0 +1,58 @@
>+const Cc = Components.classes;
>+const Ci = Components.interfaces;
>+
>+var addedTopic = "xpcom-category-entry-added";
>+var removedTopic = "xpcom-category-entry-removed";
>+var testCategory = "bug-test-category";
>+var testEntry = "@mozilla.org/bug-test-entry;1";
>+
>+var result = "";
>+var expected = "add remove add remove ";
>+var timer;
>+
>+var observer = {
>+ QueryInterface: function(iid) {
>+ if (iid.equals(Ci.nsISupports) || iid.equals(Ci.nsIObserver))
>+ return this;
>+
>+ throw Components.results.NS_ERROR_NO_INTERFACE;
>+ },
>+
>+ observe: function(subject, topic, data) {
>+ if (topic == "timer-callback") {
>+ do_check_eq(result, expected);
>+
>+ var observerService = Cc["@mozilla.org/observer-service;1"].getService(Ci.nsIObserverService);
>+ observerService.removeObserver(this, addedTopic);
>+ observerService.removeObserver(this, removedTopic);
>+
>+ do_test_finished();
>+
>+ timer = null;
>+ }
>+
>+ if (subject.QueryInterface(Ci.nsISupportsCString).data != testEntry || data != testCategory)
>+ return;
>+
>+ if (topic == addedTopic)
>+ result += "add ";
>+ else if (topic == removedTopic)
>+ result += "remove ";
>+ }
>+};
>+
>+function run_test() {
>+ do_test_pending();
>+
>+ var observerService = Cc["@mozilla.org/observer-service;1"].getService(Ci.nsIObserverService);
>+ observerService.addObserver(observer, addedTopic, false);
>+ observerService.addObserver(observer, removedTopic, false);
>+
>+ var categoryManager = Cc["@mozilla.org/categorymanager;1"].getService(Ci.nsICategoryManager);
>+ categoryManager.addCategoryEntry(testCategory, testEntry, testEntry, false, true);
>+ categoryManager.addCategoryEntry(testCategory, testEntry, testEntry, false, true);
>+ categoryManager.deleteCategoryEntry(testCategory, testEntry, false);
>+
>+ timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
>+ timer.init(observer, 0, timer.TYPE_ONE_SHOT);
>+}
Attachment #264630 -
Flags: review?(darin.moz) → review+
Assignee | ||
Comment 4•18 years ago
|
||
Darin, can you sr this as well?
Attachment #264630 -
Attachment is obsolete: true
Attachment #266247 -
Flags: superreview?(darin.moz)
Attachment #266247 -
Flags: review+
Comment 5•18 years ago
|
||
Comment on attachment 266247 [details] [diff] [review]
Slightly cleaner patch
OK, sr=darin
Attachment #266247 -
Flags: superreview?(darin.moz) → superreview+
Assignee | ||
Updated•18 years ago
|
Whiteboard: [checkin needed]
Comment 6•18 years ago
|
||
xpcom/components/nsCategoryManager.cpp 1.52
xpcom/tests/unit/test_bug374754.js 1.1
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [checkin needed]
You need to log in
before you can comment on or make changes to this bug.
Description
•