99163 - Freeze nsIObserver

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Doug Turner (:dougt)]

The topics should not be Unicode strings. They should be c-strings or atoms

Comment 1

[Doug Turner (:dougt)]

Attachment: Proposed Patch v.1

Comment 2

[Doug Turner (:dougt)]

Patch freezes nsIObserver and nsIObserverService.  

Includes renaming of methods and changes the "topic" to be a C string.  No
backwards compatiblity support.

Comment 3

[Conrad Carlen (not reading bugmail)]

Looks good - makes the code more readable, smaller, and efficient. I don't see
any downsides to it. As long as it compiles, and the compiler should catch
things which need change, r=ccarlen.

Comment 4

[Alec Flett]

very cool. 
sr=alecf

Comment 5

[rpotts (gone)]

these changes look really good !!  (r/sr=[e-mail])

a couple of comments:

1. I believe/hope that NS_OBSERVER_CONTRACTID and NS_OBSERVER_CLASSNAME are 
obsolete and can be removed from nsIObserver.idl  :-)

2. Is there a reason that nsIObserverService.idl #includes 'nsIObserver.idl' and 
'nsIEnumerator.idl' rather than providing forward declarations?  I know that 
this file pre-dates this convention...

Also, having a forward declaration of nsISimpleEnumerator will allow us to *not* 
revisit this file when we split nsISimpleEnumerator out from nsIEnumerator.idl 
:-)

3. At some point (as a separate patch?) we need to create an nsObserverService.h 
file which describes the 'observer service' as we are going to freeze it.
ie. 
  - move contract-id from nsIObserverService.idl into this header
  - #include 'nsIObserver.h' and 'nsIObserverService.h'
  - document the observer 'topics' which are available/frozen for
    this version of the observer service.

Comment 6

[Doug Turner (:dougt)]

Rick, (1) and (2) fixed.  As for (3), I guess we could do this some other time.
 I was thinking that it would be a bad idea to have all of the topic listing in
one central place.  I would hope that xpcom, preferences, profiles, ect. defines
their own topics completely seperate from this observer service.

Comment 7

[Doug Turner (:dougt)]

Rick, actually, NS_OBSERVER_CONTRACTID and NS_OBSERVER_CLASSNAME
 are used in a macro inside nsXPComInit.cpp.  They are going to have to stay.

Comment 8

[Mike Shaver (:shaver emeritus)]

Comment on attachment 53786 [details] [diff] [review]
Proposed Patch v.1

>+  if (!nsCRT::strcmp(aTopic, NS_XPCOM_SHUTDOWN_OBSERVER_ID))
>     Shutdown();

Why favour nsCRT::strcmp over C library's (or compiler's)
platform-tuned one?

>+            categoryManager->GetCategoryEntry(aTopic, 
>                                     categoryEntry,
>                                     getter_Copies(contractId));

Your arguments mis-dangle.

>+#define NOTIFICATION_OPENED NS_LITERAL_CSTRING("domwindowopened")
>+#define NOTIFICATION_CLOSED NS_LITERAL_CSTRING("domwindowclosed")

Why do we need NS_LITERAL_CSTRING, exactly?

>-    rv = os->Notify(domwin, NOTIFICATION_OPENED.get(), 0);
>+    rv = os->NotifyObservers(domwin, NOTIFICATION_OPENED.get(), 0);

Can't that just be
+    rv = os->NotifyObservers(domwin, "domwindowclosed", 0);

?

(Or use a const char const[] if you want to share the space
between the multiple uses.)

>+    observerService->AddObserver(this, "profile-approve-change");
>+    observerService->AddObserver(this, "profile-change-teardown");
>+    observerService->AddObserver(this, "profile-after-change");

Perfect: literal, human-readable strings.

> %{C++
>-
> #define NS_OBSERVERSERVICE_CONTRACTID "@mozilla.org/observer-service;1"
>-
> #define NS_OBSERVERSERVICE_CLASSNAME "Observer Service"
>-
> %}

Please, no.  C++ defines don't belong in IDL files.
Document these, and have people use the literal string.
%{C++ was a long-term mistake that we needed for short-term
migration.

Fixing nsXPComInit.cpp to use a literal string seems simple,
am I missing something?

>-NS_NAMED_LITERAL_STRING(gSkinSelectedTopic, "skin-selected");
>-NS_NAMED_LITERAL_STRING(gLocaleSelectedTopic, "locale-selected");
>-NS_NAMED_LITERAL_STRING(gInstallRestartTopic, "xpinstall-restart");
>+#define gSkinSelectedTopic "skin-selected"
>+#define gLocaleSelectedTopic "locale-selected"
>+#define gInstallRestartTopic "xpinstall-restart"

(See?  People using documented or well-known literal strings,
just like they do in JS or Python.  It's a beautiful thing.)

Comment 9

[Mike Shaver (:shaver emeritus)]

Comment on attachment 53786 [details] [diff] [review]
Proposed Patch v.1

>+  if (!nsCRT::strcmp(aTopic, NS_XPCOM_SHUTDOWN_OBSERVER_ID))
>     Shutdown();

Why favour nsCRT::strcmp over C library's (or compiler's)
platform-tuned one?

>+            categoryManager->GetCategoryEntry(aTopic, 
>                                     categoryEntry,
>                                     getter_Copies(contractId));

Your arguments mis-dangle.

>+#define NOTIFICATION_OPENED NS_LITERAL_CSTRING("domwindowopened")
>+#define NOTIFICATION_CLOSED NS_LITERAL_CSTRING("domwindowclosed")

Why do we need NS_LITERAL_CSTRING, exactly?

>-    rv = os->Notify(domwin, NOTIFICATION_OPENED.get(), 0);
>+    rv = os->NotifyObservers(domwin, NOTIFICATION_OPENED.get(), 0);

Can't that just be
+    rv = os->NotifyObservers(domwin, "domwindowclosed", 0);

?

(Or use a const char const[] if you want to share the space
between the multiple uses.)

>+    observerService->AddObserver(this, "profile-approve-change");
>+    observerService->AddObserver(this, "profile-change-teardown");
>+    observerService->AddObserver(this, "profile-after-change");

Perfect: literal, human-readable strings.

> %{C++
>-
> #define NS_OBSERVERSERVICE_CONTRACTID "@mozilla.org/observer-service;1"
>-
> #define NS_OBSERVERSERVICE_CLASSNAME "Observer Service"
>-
> %}

Please, no.  C++ defines don't belong in IDL files.
Document these, and have people use the literal string.
%{C++ was a long-term mistake that we needed for short-term
migration.

Fixing nsXPComInit.cpp to use a literal string seems simple,
am I missing something?

>-NS_NAMED_LITERAL_STRING(gSkinSelectedTopic, "skin-selected");
>-NS_NAMED_LITERAL_STRING(gLocaleSelectedTopic, "locale-selected");
>-NS_NAMED_LITERAL_STRING(gInstallRestartTopic, "xpinstall-restart");
>+#define gSkinSelectedTopic "skin-selected"
>+#define gLocaleSelectedTopic "locale-selected"
>+#define gInstallRestartTopic "xpinstall-restart"

(See?  People using documented or well-known literal strings,
just like they do in JS or Python.  It's a beautiful thing.)

Comment 10

[Mike Shaver (:shaver emeritus)]

(Sorry for the dup comments; bugzilla was being unresponsive, and I thought I
had timed out.)

Comment 11

[rpotts (gone)]

hey doug,

i think it is fine that NS_OBSERVER_CONTRACTID and NS_OBSERVER_CLASSNAME are
used internally...  however, that does *not* mean that we must expose them as
public/frozen components.

we need to find a different place for these #defines to live.  Not in the public
IDL file for nsIObserver.

Comment 12

[rpotts (gone)]

hey doug,

the issue of where observer 'topics' are documented is orthogonal to the issue
that  the CONTRACTID and CLASSID for nsIObserverService *cannot* live in the IDL
file for the interface.

these definitions must be moved out of the IDL file before we can call the
interface frozen.

-- rick

Comment 13

[Doug Turner (:dougt)]

rebuilding with these changes.  patch coming soon.

thanks.

Comment 14

[Doug Turner (:dougt)]

Attachment: Proposed Patch v.2

Comment 15

[Doug Turner (:dougt)]

Changes included in last patch:

1.  Added boolean to AddObserver method for weakRef similar to what
nsIPrefBranch is doing.
2.  Rewrote nsObserverService and removed nsIObserverList.  No more using an
abstract class to manager our nsISupportArray. :-)
3.  Fixed crashing bug in nsArrayEnumerator when in array is nsnull.
4.  Fixed js callers to use the lower case methods

can I get a r/sr please? 

Comment 16

[rpotts (gone)]

Comment on attachment 54206 [details] [diff] [review]
Proposed Patch v.2

sr=[e-mail]

Comment 17

[Alec Flett]

Comment on attachment 54206 [details] [diff] [review]
Proposed Patch v.2

- in InspectorSidebar.js, you forgot the 3rd parameter to addobserver
I think that file may not be part of the build anymore so it might not matter. 
- ns nsIAppStartupNotifier, can you just get rid of the extra APPSTARTUP_CATEGORY? We used to have two because we needed a const char* and a PRUnichar*

with those changes, sr=alecf

Comment 18

[Doug Turner (:dougt)]

- in InspectorSidebar.js, you forgot the 3rd parameter to addobserver
I think that file may not be part of the build anymore so it might not matter.

FIXED
 
- ns nsIAppStartupNotifier, can you just get rid of the extra
APPSTARTUP_CATEGORY? We used to have two because we needed a const char* and a
PRUnichar*

Lets file a seperate bug to clean this up.

Comment 19

[Alec Flett]

Comment on attachment 54206 [details] [diff] [review]
Proposed Patch v.2

wait, I retract that. Why are we #including "nsObserverService.h" all over the place? 

Also, I think that having a method GetObserverList inside class nsObserverList is confusing - how about GetObservers or GetObserverEnumerator

Lastly, I'm wondering why we need to go through the nsISimpleEnumerator in order to notify observers? 

We already know that there is an nsISupportsArray of these observers.. how 
about instead we get rid
of nsObserverList entirely, and directly 
access the nsISupportsArray. Then, we could use nsISupportsArray::EnumerateForwards, which would give us direct
 access to the elements, avoid the overhead of creating an enumerator, and avoid the extra AddRef
  from GetNext()?

Comment 20

[Doug Turner (:dougt)]

- Why are we #including "nsObserverService.h" all over the place? 

For the progid.

- GetObserverList inside class nsObserverList is confusing - how about
GetObservers or GetObserverEnumerator

The nsObserverList is private.  It doesn't matter what the name is.

- I'm wondering why we need to go through the nsISimpleEnumerator in order to
notify observers? 

I don't follow?  The nsObserverService is basically a hash to a list of
nsIObservers.  The list is implemented by the a thread safe version
nsISupportsArray called nsObserverList.  Notification happen, we need to
enumerate the list of observers. What is the problem?  (did you see what we did
before...yick)
 
> We already know that there is an nsISupportsArray of these observers.. how
about instead we get rid of nsObserverList entirely, and directly access the
nsISupportsArray. 

Oh... the array has a critical section that needs to protected.  We get the
nsObserverList from the hash atomically, but access to the list must also remain
threadsafe.  You could have two accessors to this list at the same time.  

Comment 21

[Alec Flett]

Comment on attachment 54206 [details] [diff] [review]
Proposed Patch v.2

talked with dougt over AIM about this - as this is more of an implementation issue, 
we'll let this patch land and then fix the rest as a part of bug 105719

Comment 22

[Doug Turner (:dougt)]

changes have landed.  

Comment 23

[Mike Shaver (:shaver emeritus)]

Don't include a header all over the place just to get a contractID.  They're
human-readable strings for a reason.

Comment 24

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

> Don't include a header all over the place just to get a contractID.  They're
> human-readable strings for a reason.

But the compiler won't tell you if you make a typo, which it will if you use a
macro that's equivalent to it.

Comment 25

[Mike Shaver (:shaver emeritus)]

I'm not sure that we should be designing our APIs around typo-resistance.  The
simplest of testing will tell you if you're getting the right contract ID.

We should put these strings in language-neutral documentation, so that JS and
Python hackers don't have to grovel through C++ ick to find what they're looking
for.

Isn't this the conclusion we came to in .xpcom months ago?

Comment 26

[Brendan Eich [:brendan]]

shaver's right.  If you are typo-prone and must use a .h file among some set of
.cpp files, don't make that header be the private FooImpl.h file -- make a
small, separate .h file.

/be

Comment 27

[Doug Turner (:dougt)]

patch coming up to back out this change.  

Comment 28

[Doug Turner (:dougt)]

the "change" meaning #include "nsObserverService.h" all over the place.

Note - that there are other froze interfaces that include a #define in them.  

Comment 29

[Mike Shaver (:shaver emeritus)]

Which ones?  Why did they get frozen with #defines in the IDL?

Comment 30

[Doug Turner (:dougt)]

( mike, yes, at least:
   nsIServiceManager
   nsIDirectoryService
   nsIGlobalHistory )

Comment 31

[Doug Turner (:dougt)]

Attachment: removed #include "nsObserver.h" from files.

Comment 32

[Doug Turner (:dougt)]

hmm.  that patch also include diffs for 106090 (small)

Comment 33

[Mike Shaver (:shaver emeritus)]

Argh.

Comment 34

[Mike Shaver (:shaver emeritus)]

Comment on attachment 54536 [details] [diff] [review]
removed #include "nsObserver.h" from files.

r=shaver; I like this a lot better.

Comment 35

[John Bandhauer]

Comment on attachment 54536 [details] [diff] [review]
removed #include "nsObserver.h" from files.

sr=jband

Comment 36

[Doug Turner (:dougt)]

fixed checked in.  Mailed security team their diffs.  Marking fixed.

Comment 37

[Mark Hammond [:markh] [:mhammond]]

I have struck a minor issuewith this checkin.  

Specifically, the assertion at 
http://lxr.mozilla.org/seamonkey/source/xpcom/ds/nsObserverList.cpp#85 is 
failing.  This is a debug only check that tries to enforce an assumption that 
if your object supports weak references, you *always* want to add an observer 
as a weak reference.

This is less than optimal for Python - *all* PyXPCOM implemented objects 
support weak references.  Thus, it is impossible to ever pass |false| as 
the "use weak reference" parameter to addObserver.  However, there are good 
reasons why you do want to use a strong reference.

Technically this is not a regression, as in the past you did not get a choice.  
However, this does seem a little pointless, as we are allowing a choice when 
that choice is never valid (ie, always asserts!).  By allowing us to say "do 
not use a weak reference", we can clean up some nasty hacks in our code.

Any chance this assertion can be removed?  Should I open a new bug on this?

Comment 38

[Doug Turner (:dougt)]

The assertion was only a temporary measure.  Lets remove it.  File a new bug, I
will review and get a sr for a patch.