Closed Bug 99163 Opened 23 years ago Closed 23 years ago

Freeze nsIObserver

Categories

(Core :: XPCOM, defect)

x86
Windows 2000
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.0

People

(Reporter: dougt, Assigned: dougt)

References

Details

Attachments

(3 files)

The topics should not be Unicode strings. They should be c-strings or atoms
Blocks: 98278
Summary: Freeze nsIObserver → Freeze nsIObserver
Target Milestone: --- → mozilla1.0
Patch freezes nsIObserver and nsIObserverService.  

Includes renaming of methods and changes the "topic" to be a C string.  No
backwards compatiblity support.
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.

very cool. 
sr=alecf
these changes look really good !!  (r/sr=rpotts@netscape.com)

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.

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.

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 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.)
Attachment #53786 - Flags: needs-work+
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.)
(Sorry for the dup comments; bugzilla was being unresponsive, and I thought I
had timed out.)
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.
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
rebuilding with these changes.  patch coming soon.

thanks.
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 on attachment 54206 [details] [diff] [review]
Proposed Patch v.2

sr=rpotts@netscape.com
Attachment #54206 - Flags: superreview+
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
- 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 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()?
Attachment #54206 - Flags: superreview+
- 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 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
Attachment #54206 - Flags: superreview+
changes have landed.  
Status: NEW → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Don't include a header all over the place just to get a contractID.  They're
human-readable strings for a reason.
> 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.
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?
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
patch coming up to back out this change.  
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
the "change" meaning #include "nsObserverService.h" all over the place.

Note - that there are other froze interfaces that include a #define in them.  
Which ones?  Why did they get frozen with #defines in the IDL?
( mike, yes, at least:
   nsIServiceManager
   nsIDirectoryService
   nsIGlobalHistory )


hmm.  that patch also include diffs for 106090 (small)
Comment on attachment 54536 [details] [diff] [review]
removed #include "nsObserver.h" from files.

r=shaver; I like this a lot better.
Attachment #54536 - Flags: review+
Comment on attachment 54536 [details] [diff] [review]
removed #include "nsObserver.h" from files.

sr=jband
Attachment #54536 - Flags: superreview+
fixed checked in.  Mailed security team their diffs.  Marking fixed.
Status: REOPENED → RESOLVED
Closed: 23 years ago23 years ago
Resolution: --- → FIXED
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?
The assertion was only a temporary measure.  Lets remove it.  File a new bug, I
will review and get a sr for a patch.
You need to log in before you can comment on or make changes to this bug.