nsDTDUtils.cpp CObserverService::RegisterObservers() is called about 400 times on startup. It might be worth rethinking if we need to maintain all that data structure and kill it just to do MetaTag notification. Registering directly with the some parser observer service might be better.

Suggestion: I am not very particular about the names. You can do it if you think this makes the code more readable. * nsIContentSink::NotifyObservers -> NotifyTagObservers() Seems like a more intutive name. * nsIElementObserver::Notify() -> NotifyTagObservers() or something like that * nsIObserverEntry::GetObserverInterface() -> GetTopicObservers() ? * COtherDTD.cpp + static NS_DEFINE_IID(kParserServiceCID, NS_PARSERSERVICE_CID); why is this there ? * nsObserverEntry::AddObserver() Why nsVoidArray(0) ? How about using nsAutoVoidArray ? mostly there are going to be less than 4 observers per topic right ? AutoVoidArray can save the allocs at the cost of memory. * Add/Remove Observer() in nsObserverEntry() take nsISupports * Why ? Would it make sense to have them take an nsIElementObserver so you wont have to NS_STATIC_CAST() in your nsObserverEntry::Notify() method. Is there any specific benefit to keeping it nsISupports* ? * Using voidArray * in nsObserverEntry::mObservers is good. * If the number of people doing nsIParserService::RegisterObserver() is less and the number of Topics is less (4 right ?) I think not using a hash table is the right way. * nsViewSourceHTML.cpp +static NS_DEFINE_IID(kParserServiceCID, NS_PARSERSERVICE_CID); do we need this ? * I didnt' review the intl/

>I am not very particular about the names. You can do it if you think >this makes the code more readable. In fact I was thinking of renaming GetObserverInterface() to something else. But , Notify() -> NotifyTagObservers() did not strike me. Though, NotifyTagObservers is more intuitive. Will make that change. >COtherDTD.cpp > static NS_DEFINE_IID(kParserServiceCID, NS_PARSERSERVICE_CID); > nsViewSourceHTML.cpp > static NS_DEFINE_IID(kParserServiceCID, NS_PARSERSERVICE_CID); > do we need this ? We don't need these. I'll have to comment the code and will do the clean up then. >nsObserverEntry::AddObserver() > Why nsVoidArray(0) ? How about using nsAutoVoidArray ? mostly there are going > to be less than 4 observers per topic right ? AutoVoidArray can save the > allocs at the cost of memory. Will replace nsVoidArray with nsAutoVoidArray. >Add/Remove Observer() in nsObserverEntry() take nsISupports * > Why ? Would it make sense to have them take an nsIElementObserver so you > wont have to NS_STATIC_CAST() in your nsObserverEntry::Notify() method. Is > there any specific benefit to keeping it nsISupports* ? You're right. I was only particular in keeping parameters as nsISupports* at the interface level. > If the number of people doing nsIParserService::RegisterObserver() is less and > the number of Topics is less (4 right ?) I think not using a hash table is > the right way. Topics == mime type. There are lots of mime types. However, this mechanism is primarily for "text/html", "text/xhtml", and "application/xhtml+xml" ( i.e. flavors of html ). Note: This mechanism would not work for xml documents because there are no predefined tags in xml and therefore would require string comparisons (expensive), and change in data structure, to work correctly.

>Add/Remove Observer() in nsObserverEntry() take nsISupports * > Why ? Would it make sense to have them take an nsIElementObserver so you > wont have to NS_STATIC_CAST() in your nsObserverEntry::Notify() method. Is > there any specific benefit to keeping it nsISupports* ? I just realized that one cannot avoid casting when using nsVoidArray because the elements are void*.

Ah yes. nsISupports* -> void* -> nsIElementObserver* This using STATIC_CAST() might be buggy - depending on where in the inheritance hierarchy nsIElementObserver is. To get this right you would have to do: nsISupports -> void* -> nsISupports* -QI()-> nsIElementObserver* nsIElementObserver* -> void* -> nsIElementObserver* This is ok using STATIC_CAST().

QFY Run with my patch: ---------------------- Function : nsParserService::RegisterObserver Calls : 1 Function time: 0.09 usec (0.00% of Focus) F+D time : 49.00 usec (0.00% of Focus) Avg F time : 0.09 usec Min F time : 0.09 usec Max F time : 0.09 usec Measurement : Function ------------------------------------------------- Function :nsObserverEntry::Notify Calls :166 Function time:12.05 usec (0.00% of Focus) F+D time :318.44 usec (0.00% of Focus) Avg F time :0.07 usec Min F time :0.07 usec Max F time :0.65 usece) Measurement :Function -------------------------------------------------- Function :HTMLContentSink::NotifyObservers Calls :166 Function time:5.02 usec (0.00% of Focus) F+D time :323.46 usec (0.00% of Focus) Avg F time :0.03 usec Min F time :0.03 usec Max F time :0.03 usec Measurement :Function --------------------------------------------------- Function :nsParserService::GetEntry Calls :4 Function time:0.22 usec (0.00% of Focus) F+D time :2.18 usec (0.00% of Focus) Avg F time :0.06 usec Min F time :0.05 usec Max F time :0.06 usec Measurement :Function ---------------------------------------------------- Function : nsParserService::GetObserverInterface Calls : 3 Function time: 0.09 usec (0.00% of Focus) F+D time : 2.23 usec (0.00% of Focus) Avg F time : 0.03 usec Min F time : 0.03 usec Max F time : 0.03 usec Measurement : Function -----------------------------------------------------

Calling Notify() from CNavDTD::WillHandleStartTag() with the patch: -------------------------------------------------------------------- Function :CNavDTD::WillHandleStartTag Calls :223 Function time:22.87 usec (0.00% of Focus) F+D time :488.88 usec (0.00% of Focus) <<<<< NOTICE THIS <<<<< Avg F time :0.10 usec Min F time :0.09 usec Max F time :0.16 usec Measurement :Function Calling Notify() from CNavDTD::WillHandleStartTag() without the patch: ----------------------------------------------------------------------- Function :CNavDTD::WillHandleStartTag Calls :231 Function time:29.40 usec (0.00% of Focus) F+D time :58,508.89 usec (0.00% of Focus) <<<<< NOTICE THIS <<<<< Avg F time :0.13 usec Min F time :0.11 usec Max F time :0.18 usec Measurement :Function

Comment on attachment 50293 [details] [diff] [review] patch v1.2 [ includes dp's suggestions & a few editor code changes] r=dp

Comment on attachment 50293 [details] [diff] [review] patch v1.2 [ includes dp's suggestions & a few editor code changes] sr=rpotts@netscape.com

Comment on attachment 50293 [details] [diff] [review] patch v1.2 [ includes dp's suggestions & a few editor code changes] r=shanjian (for i18n related change)

Comments: + // Observer mechanism + NS_IMETHOD RegisterObserver(const nsString& aTopic, + nsIElementObserver* aObserver, + const eHTMLTags* aTags = nsnull) = 0; nsAString is the new, better string way, no? Here: + PRInt32 theAttrCount =aNode->GetAttributeCount(); + PRInt32 theObserversCount=theObservers->Count(); + if(0<theObserversCount){ and elsewhere, you're using non-standard spacing. Convention is spaces around '= ', after ',', around '<' etc. +} \ No newline at end of file fix this. +nsParserService::~nsParserService() +{ + nsObserverEntry *entry = nsnull; + while(entry = NS_STATIC_CAST(nsObserverEntry*,mEntries.Pop())) { + NS_RELEASE(entry); + } +} Is it really OK for the parser service to hold refs until it goes away? Does it need to hold any refs at all? I fear that this might introduce a risk of cycles. +class nsParserService : public nsIParserService { etc. It would be nice to see the nsIParserService interface use nsAStrings, so you can avoid this kind of stuff: + nsAutoString topic(NS_LITERAL_STRING("text/html")); Fix those and r=sfraser

> nsAString is the new, better string way, no? Didn't know that...will make it so if that's the case. >Is it really OK for the parser service to hold refs until it goes away? Does it >need to hold any refs at all? I fear that this might introduce a risk of cycles. ParserService holds a reference to the observer as long as the observer is resgistered with the service. nsParserService::~nsParserService() +{ + nsObserverEntry *entry = nsnull; + while(entry = NS_STATIC_CAST(nsObserverEntry*,mEntries.Pop())) { + NS_RELEASE(entry); + } +} nsObserverEntry contains the data structure where the observers are stored. When the sink get's constructed it requests, the parser service, for a topic observer interface through which the sink can notify observers. So, before handing over the interface to the sink the parser service ADDREFs it to avoid accidental deletion of the observer entry.

Fix is in. Marking FIXED.

verified fixed checked in cvs rev 3.115.