Use an array of observer strings in ContentParent

RESOLVED FIXED in mozilla30

Status

()

Core
IPC
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: khuey, Assigned: lpy)

Tracking

unspecified
mozilla30
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink] [mentor=jdm] [lang=c++] [good first bug])

Attachments

(1 attachment, 3 obsolete attachments)

Whiteboard: [MemShrink]
Whiteboard: [MemShrink] → [MemShrink] [mentor=jdm] [lang=c++] [good first bug]
(Assignee)

Updated

4 years ago
Assignee: nobody → pylaurent1314
(Assignee)

Comment 1

4 years ago
Created attachment 8374224 [details] [diff] [review]
bug970821.patch

I wrote ugly "for iteration"
Attachment #8374224 - Flags: review?(josh)
(Assignee)

Updated

4 years ago
Attachment #8374224 - Flags: review?(josh) → review-
(Assignee)

Updated

4 years ago
Attachment #8374224 - Flags: review-
(Assignee)

Comment 2

4 years ago
Created attachment 8374267 [details] [diff] [review]
bug970821-V2.patch
Attachment #8374224 - Attachment is obsolete: true
Attachment #8374267 - Flags: review?(josh)
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

4 years ago
Created attachment 8374655 [details] [diff] [review]
bug970821-V2.patch

Thank you.
Attachment #8374267 - Attachment is obsolete: true
Attachment #8374655 - Flags: review?(josh)
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

4 years ago
Created attachment 8374745 [details] [diff] [review]
bug970821-V3.patch

Thank you :)
Attachment #8374655 - Attachment is obsolete: true
Attachment #8374745 - Flags: review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3a8fe7c942e3
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.