970821 - Use an array of observer strings in ContentParent

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

https://bugzilla.mozilla.org/show_bug.cgi?id=968536#c1

Comment 1

[Peiyong Lin[:lpy](UTC+8)]

Attachment: bug970821.patch

I wrote ugly "for iteration"

Comment 2

[Peiyong Lin[:lpy](UTC+8)]

Attachment: bug970821-V2.patch

Comment 3

[Nicholas Nethercote [inactive]]

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",
    ...
  };

Comment 4

[Peiyong Lin[:lpy](UTC+8)]

Attachment: bug970821-V2.patch

Thank you.

Comment 5

[Nicholas Nethercote [inactive]]

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.

Comment 6

[Peiyong Lin[:lpy](UTC+8)]

Attachment: bug970821-V3.patch

Thank you :)

Comment 7

[Nicholas Nethercote [inactive]]

Thanks!
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a8fe7c942e3

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3a8fe7c942e3