Closed
Bug 374754
Opened 17 years ago
Closed 17 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•17 years ago
|
||
Assignee | ||
Comment 2•17 years ago
|
||
Typo in the comment - "if the called" should be "if the caller" of course.
Assignee | ||
Updated•17 years ago
|
Attachment #264630 -
Flags: review?(cbiesinger) → review?(darin.moz)
Comment 3•17 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•17 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•17 years ago
|
||
Comment on attachment 266247 [details] [diff] [review] Slightly cleaner patch OK, sr=darin
Attachment #266247 -
Flags: superreview?(darin.moz) → superreview+
Assignee | ||
Updated•17 years ago
|
Whiteboard: [checkin needed]
Comment 6•17 years ago
|
||
xpcom/components/nsCategoryManager.cpp 1.52 xpcom/tests/unit/test_bug374754.js 1.1
Status: ASSIGNED → RESOLVED
Closed: 17 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
•