Closed
Bug 970821
Opened 10 years ago
Closed 10 years ago
Use an array of observer strings in ContentParent
Categories
(Core :: IPC, defect)
Core
IPC
Tracking
()
RESOLVED
FIXED
mozilla30
People
(Reporter: khuey, Assigned: lpy)
Details
(Whiteboard: [MemShrink] [mentor=jdm] [lang=c++] [good first bug])
Attachments
(1 file, 3 obsolete files)
4.85 KB,
patch
|
lpy
:
review+
|
Details | Diff | Splinter Review |
Updated•10 years ago
|
Whiteboard: [MemShrink]
Updated•10 years ago
|
Whiteboard: [MemShrink] → [MemShrink] [mentor=jdm] [lang=c++] [good first bug]
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → pylaurent1314
Assignee | ||
Comment 1•10 years ago
|
||
I wrote ugly "for iteration"
Attachment #8374224 -
Flags: review?(josh)
Assignee | ||
Updated•10 years ago
|
Attachment #8374224 -
Flags: review?(josh) → review-
Assignee | ||
Updated•10 years ago
|
Attachment #8374224 -
Flags: review-
Assignee | ||
Comment 2•10 years ago
|
||
Attachment #8374224 -
Attachment is obsolete: true
Attachment #8374267 -
Flags: review?(josh)
Comment 3•10 years ago
|
||
Comment on attachment 8374267 [details] [diff] [review] bug970821-V2.patch Review of attachment 8374267 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/ipc/ContentParent.h @@ +590,5 @@ > nsConsoleService* GetConsoleService(); > > nsDataHashtable<nsUint64HashKey, nsCOMPtr<ParentIdleListener> > mIdleListeners; > > + nsTArray<const char*> mObserverTopics; There's no need for a class member. Please use a static C++ array, like this: static const char* sObserverTopics = { "xpcom-shutdown", ... };
Attachment #8374267 -
Flags: review?(josh) → review-
Assignee | ||
Comment 4•10 years ago
|
||
Thank you.
Attachment #8374267 -
Attachment is obsolete: true
Attachment #8374655 -
Flags: review?(josh)
Comment 5•10 years ago
|
||
Comment on attachment 8374655 [details] [diff] [review] bug970821-V2.patch Review of attachment 8374655 [details] [diff] [review]: ----------------------------------------------------------------- Thanks! Please upload a new patch addressing the minor comments below. I can land it for you. ::: dom/ipc/ContentParent.cpp @@ +672,5 @@ > nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService(); > if (obs) { > + for (unsigned int observerTopicIndex = 0, observerTopicLength = ArrayLength(sObserverTopics); > + observerTopicIndex < observerTopicLength; ++observerTopicIndex) { > + obs->AddObserver(this, sObserverTopics[observerTopicIndex], false); Just use 'i' for the name of the index variable, instead of 'observerTopicIndex'. And I wouldn't bother creating the 'observerTopicLength' variable, or I'd call it something shorter like 'length'. @@ +1045,5 @@ > if (obs) { > + for (unsigned int observerTopicIndex = 0, observerTopicLength = ArrayLength(sObserverTopics); > + observerTopicIndex < observerTopicLength; ++observerTopicIndex) { > + obs->RemoveObserver(static_cast<nsIObserver*>(this), sObserverTopics[observerTopicIndex]); > + } Same here.
Attachment #8374655 -
Flags: review?(josh) → review+
Assignee | ||
Comment 6•10 years ago
|
||
Thank you :)
Attachment #8374655 -
Attachment is obsolete: true
Attachment #8374745 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 7•10 years ago
|
||
Thanks! https://hg.mozilla.org/integration/mozilla-inbound/rev/3a8fe7c942e3
Updated•10 years ago
|
Keywords: checkin-needed
Comment 8•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/3a8fe7c942e3
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in
before you can comment on or make changes to this bug.
Description
•