218837 - Share code between XML and HTML contentsinks

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Harshal Pradhan]

There's is a quite a bit of code in XML and HTML contentsink that really looks
very similar and should be relatively easy to factor out into a common place.

Comment 1

[Chase Tingley]

see bug 21771

Comment 2

[Harshal Pradhan]

Attachment: v1

Initial stab at making them inherit from a common base class. This throws a
bunch of random stuff including link handling, script element handling into a
class called nsContentSink. 

- The link stuff is mostly taken from the htmlsink. This means that prefetch
should now work for xml/xhtml. 
- The script element stuff is closer to the xmlsink. This will mean that the
scriptloader no longer holds on to a hard ref to the html contentsink. 
- Other than these, there really shouldn't be any major functional change. The
other stuff is minor. I've tried to stay away from the temptation to cleanup
some of the code that I am copying, except for straightforward nsCOMPtr
conversions, or when its needed because the code in the two places has
deviated.

One other thing that deserves a mention is that nsXMLContentSink had threadsafe
AddRef and Release. I couldnt see the point of that. Removed.

This is not really tested. I have just confirmed, I am able to bring up the
browser and browse around, load a few xml docs and xhtml pages.

Comment 3

[Harshal Pradhan]

Yeah, I know about bug 21771. This is a subset and and should probably block
that bug. I don't want to deal with XUL for now.

Adding some more ppl who might be interested.

Some more things I should have said about the patch ... 

- nsContentSink is probably not a good name for that class, since it clearly is
not a very generic contentsink implementation. I can't think of anything better
though. Suggestions welcome.
- I wanted nsContentSink to inherit from nsIContentSink, but ran into trouble
since nsIHTMLContentSink and nsIXMLContentSink inherit from nsIContentSink too.
Which means that we end up with diamond inheritance issues. I'm not sure we want
to use virtual inheritance etc. So I don't know how to proceed with that. Some
stuff is currently |#if 0|ed out as a result.
- This really is not ready for review. Actucally, it started out more as an
investigation of how much common code there really was. But I am looking for
feedback about whether people like the general direction in which this is going.

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

first off, i only looked at the patch briefly, but i'd definitly be interested
in a deeper look when i've got the time.

In general though this sounds like a very good first step and I agree with your
suggested relationship to bug 21771. (Btw, this takes care of the remainder of
bug 188729 too).

Wrt nsContentSink inhereting nsIContentSink. As you noted it won't really help
since the other contentsinks will need to implement it anyway. Which functions
do you want to implement in nsContentSink? You could always put the
implementation in nsContentSink and then forward the call in the html/xml sinks.

Comment 5

[Harshal Pradhan]

Attachment: fixed a few things. this seems to mostly work now.

Comment 6

[Harshal Pradhan]

Attachment: updated patch

Minor updates. Nothing much new.

I've been running with this in my tree for a while now. Its ready for review.
(BTW, this will conflict a little bit with caillon's big security patch.)

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 131962 [details] [diff] [review]
updated patch

This is an awsome patch! The only think that i'm slightly worried about is
change to the linkhandling in the xml-sink. The code in the http-sink is
definitly better so you made a good choise, but I'm wondering if there might be
any xml-specific things (or general bugfixes) in the xmlsink that got lost?
What I would mostly like to hear is if/how you compared the differences between
the two?

Some of the things i've commented on are things in code that you've just
copied. I tried to stay away from it as much as possible so that we can keep
general cleanups in a separate patch, but I think a few snuck through anyway.


>Index: base/src/nsContentSink.cpp

>+nsContentSink::Init(nsIDocument* aDoc,
...
>+  nsCOMPtr<nsIScriptLoader> loader;
>+  nsresult rv = mDocument->GetScriptLoader(getter_AddRefs(loader));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  loader->AddObserver(proxy);

Please check the returnvalue from AddObserver


>+nsContentSink::ScriptAvailable(nsresult aResult,
...
>+  // Check if this is the element we were waiting for
>+  nsIDOMHTMLScriptElement* scriptElement = mScriptElements[count - 1];
>+  if (aElement != scriptElement) {
>+    return NS_OK;
>+  }

You can compare directly to mScriptElements[...] here. No need for the
temporary variable. Same in ScriptEvaluated


>+static const PRUnichar kSemiCh = PRUnichar(';');
>+static const PRUnichar kCommaCh = PRUnichar(',');
>+static const PRUnichar kEqualsCh = PRUnichar('=');
>+static const PRUnichar kLessThanCh = PRUnichar('<');
>+static const PRUnichar kGreaterThanCh = PRUnichar('>');

Does declaring |static| on a global do anything?


>+nsresult
>+nsContentSink::ProcessStyleLink(nsIContent* aElement,
>+                                const nsAString& aHref,
>+                                const PRBool aAlternate,
>+                                const nsAString& aTitle,
>+                                const nsAString& aType,
>+                                const nsAString& aMedia)
>+{
>+  nsresult result = NS_OK;
>+  PRBool isAlternate = PR_FALSE;

in the places further down in this function you should use aAlternate rather
then isAlternate (and of course remove isAlternate)

>+PRBool
>+nsContentSink::ScrollToRef(PRBool aReallyScroll)
>+{
>+  PRBool didScroll = PR_FALSE;
>+
>+  if (!mRef.IsEmpty()) {

Please make this function earlyreturn the way the HTMLSink one did.


>+    char* tmpstr = ToNewCString(mRef);
>+    if(! tmpstr) {
>+      return PR_FALSE;
>+    }
>+    nsUnescape(tmpstr);
>+    nsCAutoString unescapedRef;
>+    unescapedRef.Assign(tmpstr);
>+    nsMemory::Free(tmpstr);
>+
>+    nsresult rv = NS_ERROR_FAILURE;
>+    // We assume that the bytes are in UTF-8, as it says in the spec:
>+    // http://www.w3.org/TR/html4/appendix/notes.html#h-B.2.1
>+    NS_ConvertUTF8toUCS2 ref(unescapedRef);
>+
>+    PRInt32 i, ns = mDocument->GetNumberOfShells();
>+    for (i = 0; i < ns; i++) {
>+      nsCOMPtr<nsIPresShell> shell;
>+      mDocument->GetShellAt(i, getter_AddRefs(shell));
>+      if (shell) {
>+        // Scroll to the anchor
>+        if (aReallyScroll) {
>+          shell->FlushPendingNotifications(PR_FALSE);
>+        }
>+
>+        // Check an empty string which might be caused by the UTF-8 conversion
>+        if (!ref.IsEmpty())
>+          rv = shell->GoToAnchor(ref, aReallyScroll);
>+        else
>+          rv = NS_ERROR_FAILURE;

Please stick to a single '{' pattern. Most files nowadays use
if (foo) {
  ...
}
else {
  ...
}

Which also happens to be the one i prefer :).


>Index: html/document/src/nsHTMLContentSink.cpp
...
>-  NS_DECL_ISUPPORTS
>-  NS_DECL_NSISCRIPTLOADEROBSERVER
>+  NS_IMETHOD QueryInterface(REFNSIID aIID, void** aInstancePtr);
>+
>+  NS_IMETHOD_(nsrefcnt) AddRef(void);
>+  NS_IMETHOD_(nsrefcnt) Release(void);

Use NS_DECL_ISUPPORTS_INHERITED instead. Same in the xml contentsink.


>+NS_INTERFACE_MAP_BEGIN(HTMLContentSink)
>+  NS_INTERFACE_MAP_ENTRY(nsIContentSink)
>+  NS_INTERFACE_MAP_ENTRY(nsIHTMLContentSink)
>+  NS_INTERFACE_MAP_ENTRY(nsITimerCallback)
>+  NS_INTERFACE_MAP_ENTRY(nsIDocumentObserver)
>+  NS_INTERFACE_MAP_ENTRY(nsIDebugDumpContent)
>+NS_INTERFACE_MAP_END_INHERITING(nsContentSink)
> #else
>-NS_IMPL_ISUPPORTS6(HTMLContentSink,
>-                   nsIHTMLContentSink,
>-                   nsIContentSink,
>-                   nsIScriptLoaderObserver,
>-                   nsITimerCallback,
>-                   nsICSSLoaderObserver,
>-                   nsIDocumentObserver)
>+NS_INTERFACE_MAP_BEGIN(HTMLContentSink)
>+  NS_INTERFACE_MAP_ENTRY(nsIContentSink)
>+  NS_INTERFACE_MAP_ENTRY(nsIHTMLContentSink)
>+  NS_INTERFACE_MAP_ENTRY(nsITimerCallback)
>+  NS_INTERFACE_MAP_ENTRY(nsIDocumentObserver)
>+NS_INTERFACE_MAP_END_INHERITING(nsContentSink)

You can use NS_IMPL_ISUPPORTS_INHERITED here. Same in the XMLContentSink


> HTMLContentSink::Init(nsIDocument* aDoc,
>                       nsIURI* aURL,
>                       nsISupports* aContainer,
>                       nsIChannel* aChannel)
> {
>+  NS_ENSURE_TRUE(aContainer, NS_ERROR_NULL_POINTER);
>+
>+  nsresult rv = nsContentSink::Init(aDoc, aURL, aContainer, aChannel);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>   MOZ_TIMER_DEBUGLOG(("Reset and start: nsHTMLContentSink::Init(), this=%p\n",
>                       this));
>   MOZ_TIMER_RESET(mWatch);
>   MOZ_TIMER_START(mWatch);
> 
>-  if (!aDoc || !aURL || !aContainer) {
>-    NS_ERROR("Null ptr!");
>-
>-    MOZ_TIMER_DEBUGLOG(("Stop: nsHTMLContentSink::Init()\n"));
>-    MOZ_TIMER_STOP(mWatch);
>-
>-    return NS_ERROR_NULL_POINTER;
>-  }

It might be a good idea to do the logging before calling nsContentSinke::Init
and keep the |if| (though of course change the condition to NS_FAILED(rv)). I
honestly don't know how important it is though. Peterv?


> void
> HTMLContentSink::TryToScrollToRef()
> {
>   if (mRef.IsEmpty()) {
>     return;
>   }
> 
>   if (mScrolledToRefAlready) {
>     return;
>   }
> 
>-  ScrollToRef(PR_TRUE);
>-}
>-
>-void
>-HTMLContentSink::ScrollToRef(PRBool aReallyScroll)

You're moving the setting of mScrolledToRefAlready into TryToScrollToRef but
DidBuildModel calls ScrollToRef too. Do you need to add
mScrolledToRefAlready-code in there too?


>Index: xml/document/src/nsXMLContentSink.cpp

> nsXMLContentSink::ProcessStyleLink(nsIContent* aElement,
>-                                   const nsString& aHref,
>-                                   PRBool aAlternate,
>-                                   const nsString& aTitle,
>-                                   const nsString& aType,
>-                                   const nsString& aMedia)
>+                                   const nsAString& aHref,
>+                                   const PRBool aAlternate,
>+                                   const nsAString& aTitle,
>+                                   const nsAString& aType,
>+                                   const nsAString& aMedia)

just keep aAlternate as a |PRBool| that's what we do elsewhere and it doesn't
really make sense to use const on simple datatypes.

>-  if (aType.EqualsIgnoreCase(kXSLType) ||
>-      aType.EqualsIgnoreCase(kXMLTextContentType) ||
>-      aType.EqualsIgnoreCase(kXMLApplicationContentType)) {
>+  NS_ConvertUTF16toUTF8 type(aType);
>+  if (type.EqualsIgnoreCase(kXSLType) ||
>+      type.EqualsIgnoreCase(kXMLTextContentType) ||
>+      type.EqualsIgnoreCase(kXMLApplicationContentType)) {

Hmm.. why wasn't the conversion needed before?

>-  if (aType.Equals(NS_LITERAL_STRING("text/css"))) {
>-    nsCOMPtr<nsIURI> url;
>-    rv = NS_NewURI(getter_AddRefs(url), aHref, nsnull, mDocumentBaseURL);
>-    if (NS_FAILED(rv)) {
>-      return NS_OK; // The URL is bad, move along, don't propagate the error (for now)
>-    }
>-    PRBool doneLoading;
>-    rv = mCSSLoader->LoadStyleLink(aElement, url, aTitle, aMedia,
>-                                   ((!aAlternate) ? mParser : nsnull),
>-                                   doneLoading, 
>-                                   this);
>-    // XXX should probably use kBlockByDefault here, no?
>-    if (NS_SUCCEEDED(rv) && !doneLoading && !aAlternate && mParser) {
>-      mParser->BlockParser();
>-    }
>+  // Let nsContentSink deal with css.
>+  rv = nsContentSink::ProcessStyleLink(aElement, aHref, aAlternate,
>+                                       aTitle, aType, aMedia);
>+  if (rv = NS_ERROR_HTMLPARSER_BLOCK) {
>+    mParser->BlockParser();

should be |==|. And you need to check that mParser isn't null i think.

And the ending return should return NS_OK, not rv since otherwise a block will
be considered a failure in the caller of this function. (This is the reason why
I dislike functions endind with "return rv"). Though you need to deal with
other errorcodes then block and make sure that they return a failure.

r=sicking if you get peterv to sr this

Comment 8

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

From a quick glance, it looks like this causes the XML sink to handle style
without a TYPE attribute/pseudo-attribute (I'm not sure in what cases the code
is used) as CSS, whereas it didn't before.  (See changes to ProcessStyleLink.) 
I don't think we want to do that.

Comment 9

[Boris Zbarsky [:bzbarsky]]

More drive-by comments:

ProcessHeaderData for the setcookie header uses ToNewUTF8String in HTML and
NS_ConvertUCS2toUTF8().get() in XML.  I'd assume you would want to use the
latter... (especially since using nsCRT::free to deallocate the result of
ToNewUTF8String is wrong, iirc), whereas your patch uses the former.

And yes, what dbaron said.  It's OK to load a <link rel="stylesheet"> with no
type in HTML, but in XML we should load it only if it has type="text/css"...

Perhaps we should make that a quirks-vs-standards distinction?  David, what do
you think?

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I'm definitly for making that a quirks vs. standard thing. IMHO we should have
as few modes as possible to make things easy for authors. So the closer xml
behaves to standards mode the better IMHO. (if that means chaning the way strict
behaves or the way that xml behaves i don't have an oppinion on)

Comment 11

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

For the HTML link element, the type attribute is optional.  For the
xml-stylesheet PI, it's required.  (In both cases, it's advisory, and what the
server says wins.)  So we should do that.

Comment 12

[Boris Zbarsky [:bzbarsky]]

The code under discussion is actually neither for <link> elements nor for PIs. 
This is the code use for the HTTP header:

Link: <http://whatever>; type="text/css"; rel="stylesheet"

and the equivalent <meta> tag.  I _think_ the idea is that this should work like
an HTML <link> element, but...

Comment 13

[Hixie (not reading bugmail)]

The "type" attribute is optional on all of the above (<link>, Link:, <?
xml-stylesheet?>, etc). (For the XML Stylesheet PI, you have to read the errata 
to notice that it is optional.)

So in all cases, preferably by only coding this once and having them all go 
through the same codepath, we should be treating the optional type attribute as 
advisory, and treating a missing (or empty) value as a wildcard.

Note that an _empty_ type attribute for HTML is equivalent to a missing type 
attribute, and that since the others are all defined to have the same semantics 
as HTML, that goes for them too.

Comment 14

[Harshal Pradhan]

Comment on attachment 131962 [details] [diff] [review]
updated patch

Thanks for all the comments. I've got caught up in some other stuff. But
hopefully, I'll get time to go through the comments and post an updated patch
over the weekend. 

The <link> handling is clearly the most controversial part of this, and the
part that I  am least confident about. Maybe I should just first do a patch
that refactors in-place only those parts of the two sinks, to bring them
closer. I can then move the code into the base class as step 2?

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

No, i think we should go with this. I'll look through the differences again to
be extra sure that it all looks good.

Ian: so what you're saying is that all links should behave exactly the same
<meta>, http-header, <link> and <?xml-stylesheet?> ? Do we need any differences
between strict and quirks mode? Harshals patch uses the current html-code for
linkparsing in both html and xml, is there to your knowledge anything that it
does wrong?

Comment 16

[Hixie (not reading bugmail)]

I'm not familiar with the bugs in those areas at the moment. I'm sure there are 
some, there always are. :-)

Yes, as I understand it, those should all work in just the same way. You might 
want to run this past peterv, he knew this code quite well at one point IIRC.

Comment 17

[Harshal Pradhan]

I'll attach an updated patch shortly, but let me address a few comments
first. 

> The only think that i'm slightly worried about is
> change to the linkhandling in the xml-sink. The code in the http-sink is
> definitly better so you made a good choise, but I'm wondering if there might
> be any xml-specific things (or general bugfixes) in the xmlsink that got
> lost? What I would mostly like to hear is if/how you compared the
> differences between the two?

I looked at cvs blame and relevant bugs. I don't think that the xml code
has changed in any significant way since Heikki first introduced it in
the XMLSink in bug 93218. I couldn't find any bugfix since that would
needed to be carried over.

The HTMLSink version has had a lot more attention. Amongst other bugfixes,
it has been whacked significantly two times. First in bug 166647 by darin
for the prefetch stuff, and then by jst when he cleaned up a lot of string
and whitespace foo in bug 168371. 

So, I just went with the html version. Which means that, as far as I
know, I have not changed behavior in any situation in the html case.
In the xml case, I looked at what changed, and thought that in each
case the html behavior was more desirable. I know of two such things,
one of which dbaron pointed out. I'll shortly attach testcases for each.

Comment 18

[Harshal Pradhan]

Attachment: Testcase 1

This is what dbaron pointed out. With the current patch, all the four lines
here will be green. We currently make only the first three green.

I'm open to changing it back to keeping the current behavior if people
really want that, or making it a quirks thing or whatever. Let me know what
this should render like, and I will make it so.

For now, going by what Ian said in his last couple of comments, I've
kept this behavior. I looked this up, and, as Ian pointed out
http://www.w3.org/1999/06/REC-xml-stylesheet-19990629/errata
makes type optional for PI's. http://www.w3.org/Style/styling-XML
is resonably clear about this as well. Personally, I think it just
make things more confusing to require type only in the header form
and that too only for xml.

(Oh and I'm using some css from Ian's tests for these testcases, I didn't
want to have to attach tons of files here.)

Comment 19

[Harshal Pradhan]

Attachment: Testcase 2

A PI testcase for completeness sake. Just to show that we appear
to be doing the right thing per the errata, and that I did not change
that.

Comment 20

[Harshal Pradhan]

Attachment: Testcase 3

This is the only other case that I know of. 

The XMLSink currently doesn't have the bit of code over here
http://lxr.mozilla.org/mozilla/source/content/html/document/src/nsHTMLContentSink.cpp#4585


So, this testcase renders differently from its html counterpart. 

You have to go and pick the "a" in "Use Style" to make it go green as
it would have been if this were served as html. After the patch, the
"a" stylesheet will be picked up as the default, as happens in the html
case.

Frankly, I don't know if this is "correct" per the specs. Let me know.

Comment 21

[Harshal Pradhan]

Attachment: v4

>>+  loader->AddObserver(proxy);
>
>Please check the returnvalue from AddObserver

Done

>>+  nsIDOMHTMLScriptElement* scriptElement = mScriptElements[count - 1];
>>+  if (aElement != scriptElement) {
>>+    return NS_OK;
>>+  }
>
>You can compare directly to mScriptElements[...] here. No need for the
>temporary variable. Same in ScriptEvaluated

Done. 

[BTW, now that I think about it, this check is not strictly sufficent is it?
COM purists would QI to check the COM identity of the two pointers.
We can ignore that since we know the implementation. Right? ]


>>+static const PRUnichar kSemiCh = PRUnichar(';');
>>+static const PRUnichar kCommaCh = PRUnichar(',');
>>+static const PRUnichar kEqualsCh = PRUnichar('=');
>>+static const PRUnichar kLessThanCh = PRUnichar('<');
>>+static const PRUnichar kGreaterThanCh = PRUnichar('>');
>
>Does declaring |static| on a global do anything?

If I am not mistaken, it would make the variable "local to the translation
unit". In other words, theoretically, this should make static builds less
likely to break due to global variablename conflicts etc.

>>+{
>>+  nsresult result = NS_OK;
>>+  PRBool isAlternate = PR_FALSE;

in the places further down in this function you should use aAlternate rather
then isAlternate (and of course remove isAlternate)

Doh !

>>+PRBool
>>+nsContentSink::ScrollToRef(PRBool aReallyScroll)
>>+{
>>+  PRBool didScroll = PR_FALSE;
>>+
>>+  if (!mRef.IsEmpty()) {
>
>Please make this function earlyreturn the way the HTMLSink one did.

Done.

>Please stick to a single '{' pattern. Most files nowadays use

That code was just cut-pasted :-) Its easy enough to fix up. Done. 


>>+  NS_IMETHOD QueryInterface(REFNSIID aIID, void** aInstancePtr);
>>+
>>+  NS_IMETHOD_(nsrefcnt) AddRef(void);
>>+  NS_IMETHOD_(nsrefcnt) Release(void);
>
>Use NS_DECL_ISUPPORTS_INHERITED instead. Same in the xml contentsink.

Done.


>>+NS_INTERFACE_MAP_BEGIN(HTMLContentSink)
>>+  NS_INTERFACE_MAP_ENTRY(nsIContentSink)
>>+  NS_INTERFACE_MAP_ENTRY(nsIHTMLContentSink)
>>+  NS_INTERFACE_MAP_ENTRY(nsITimerCallback)
>>+  NS_INTERFACE_MAP_ENTRY(nsIDocumentObserver)
>>+NS_INTERFACE_MAP_END_INHERITING(nsContentSink)
>
>You can use NS_IMPL_ISUPPORTS_INHERITED here. Same in the XMLContentSink

I actually have a slight preference for the MAP version.
But ok. Done.



>>+
>>+  nsresult rv = nsContentSink::Init(aDoc, aURL, aContainer, aChannel);
>>+  NS_ENSURE_SUCCESS(rv, rv);
>>+
>>   MOZ_TIMER_DEBUGLOG(("Reset and start: nsHTMLContentSink::Init(),
this=%p\n",
>>			 this));
>>   MOZ_TIMER_RESET(mWatch);
>>   MOZ_TIMER_START(mWatch);
>> 
>>-  if (!aDoc || !aURL || !aContainer) {
>>-    NS_ERROR("Null ptr!");
>>-
>>-    MOZ_TIMER_DEBUGLOG(("Stop: nsHTMLContentSink::Init()\n"));
>>-    MOZ_TIMER_STOP(mWatch);
>>-
>>-    return NS_ERROR_NULL_POINTER;
>>-  }
>
>It might be a good idea to do the logging before calling nsContentSinke::Init
>and keep the |if| (though of course change the condition to NS_FAILED(rv)). I
>honestly don't know how important it is though. Peterv?

Done. 


>> void
>> HTMLContentSink::TryToScrollToRef()
>> {
>>   if (mRef.IsEmpty()) {
>>     return;
>>   }
>> 
>>   if (mScrolledToRefAlready) {
>>     return;
>>   }
>> 
>>-  ScrollToRef(PR_TRUE);
>>-}
>>-
>>-void
>>-HTMLContentSink::ScrollToRef(PRBool aReallyScroll)
>
>You're moving the setting of mScrolledToRefAlready into TryToScrollToRef but
>DidBuildModel calls ScrollToRef too. Do you need to add
>mScrolledToRefAlready-code in there too?

DidBuildModel() behavior did change there. Actually, I had concluded
that this didn't matter. We cancel the notification timer a bit further
up in DidBuildModel(). And the only place TryToScrollToRef() is called
is from that timer's callback. So, setting mScrolledToRefAlready should
really not matter beyond this point. However, its really trivial to 
do so, and I've tried to stay faithful to what was there earlier in
most other places, so I fixed this up it up.


>> nsXMLContentSink::ProcessStyleLink(nsIContent* aElement,
>>-				      const nsString& aHref,
>>-				      PRBool aAlternate,
>>-				      const nsString& aTitle,
>>-				      const nsString& aType,
>>-				      const nsString& aMedia)
>>+				      const nsAString& aHref,
>>+				      const PRBool aAlternate,
>>+				      const nsAString& aTitle,
>>+				      const nsAString& aType,
>>+				      const nsAString& aMedia)
>
>just keep aAlternate as a |PRBool| that's what we do elsewhere and it doesn't
>really make sense to use const on simple datatypes.

OK. Done. 

>>-  if (aType.EqualsIgnoreCase(kXSLType) ||
>>-	 aType.EqualsIgnoreCase(kXMLTextContentType) ||
>>-	 aType.EqualsIgnoreCase(kXMLApplicationContentType)) {
>>+  NS_ConvertUTF16toUTF8 type(aType);
>>+  if (type.EqualsIgnoreCase(kXSLType) ||
>>+	 type.EqualsIgnoreCase(kXMLTextContentType) ||
>>+	 type.EqualsIgnoreCase(kXMLApplicationContentType)) {
>
>Hmm.. why wasn't the conversion needed before?

Becuase the aType parameter in nsXMLContentSink::ProcessStyleLink()
was a |nsString| and not |nsAString|. |nsString| has an (evil)
EqualsIgnoreCase(const char*) method which will do a
conversion transparently. It may have been converting
to ASCII which maybe I should be doing too? Hmmmm....


>>-  if (aType.Equals(NS_LITERAL_STRING("text/css"))) {
>>-    nsCOMPtr<nsIURI> url;
>>-    rv = NS_NewURI(getter_AddRefs(url), aHref, nsnull, mDocumentBaseURL);
>>-    if (NS_FAILED(rv)) {
>>-	 return NS_OK; // The URL is bad, move along, don't propagate the error
(for now)
>>-    }
>>-    PRBool doneLoading;
>>-    rv = mCSSLoader->LoadStyleLink(aElement, url, aTitle, aMedia,
>>-				      ((!aAlternate) ? mParser : nsnull),
>>-				      doneLoading, 
>>-				      this);
>>-    // XXX should probably use kBlockByDefault here, no?
>>-    if (NS_SUCCEEDED(rv) && !doneLoading && !aAlternate && mParser) {
>>-	 mParser->BlockParser();
>>-    }
>>+  // Let nsContentSink deal with css.
>>+  rv = nsContentSink::ProcessStyleLink(aElement, aHref, aAlternate,
>>+					  aTitle, aType, aMedia);
>>+  if (rv = NS_ERROR_HTMLPARSER_BLOCK) {
>>+    mParser->BlockParser();
>
>should be |==|. And you need to check that mParser isn't null i think.

Doh! Fixed.

>And the ending return should return NS_OK, not rv since otherwise a block will

>be considered a failure in the caller of this function. (This is the reason
why
>I dislike functions endind with "return rv"). Though you need to deal with
>other errorcodes then block and make sure that they return a failure.

Fixed.


bz :

>ProcessHeaderData for the setcookie header uses ToNewUTF8String in HTML and
>NS_ConvertUCS2toUTF8().get() in XML.  I'd assume you would want to use the
>latter... (especially since using nsCRT::free to deallocate the result of
>ToNewUTF8String is wrong, iirc), whereas your patch uses the former.

Fixed. BTW, what is the right way to deallocate from ToNewUTF8String()?
nsMemory::Free()?

Comment 22

[Boris Zbarsky [:bzbarsky]]

> right way to deallocate from ToNewUTF8String()

Yes, nsMemory::Free()

Comment 23

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Ok, i feel more confident on the stylesheet issues now. Adding the url to the
link-spec for completness sake:
http://www.ietf.org/rfc/rfc2068  (section 19.6.2.4)

My r=sicking still stands (still with the requirement that you get peterv to sr)

Comment 24

[Harshal Pradhan]

Attachment: v4.1

Minor update over v4. (removed a hunk from unrelated patch, killed some
un-necessary #include's)

Comment 25

[Harshal Pradhan]

Comment on attachment 132511 [details] [diff] [review]
v4.1

This one is current. peterv, could you please have a look.

Comment 26

[Harshal Pradhan]

Comment on attachment 132299 [details] [diff] [review]
v4

stale request

Comment 27

[Peter Van der Beken [:peterv]]

Comment on attachment 132511 [details] [diff] [review]
v4.1

>Index: base/src/nsContentSink.cpp
>===================================================================

>+ * Version: NPL 1.1/GPL 2.0/LGPL 2.1

Make this MPL/GPL/LGPL

>+nsresult
>+nsContentSink::Init(nsIDocument* aDoc,
>+                    nsIURI* aURL,
>+                    nsISupports* aContainer,
>+                    nsIChannel* aChannel)
>+{
>+  NS_PRECONDITION(aDoc, "null ptr");
>+  NS_PRECONDITION(aURL, "null ptr");
>+
>+  if ((nsnull == aDoc) || (nsnull == aURL)) {

if (!aDoc || !aURL) {

>+  nsCOMPtr<nsIHTMLContentContainer> htmlContainer(do_QueryInterface(aDoc));
>+  if (htmlContainer) {
>+    htmlContainer->GetCSSLoader(*getter_AddRefs(mCSSLoader));
>+  }
>+
>+  // XXX this presumes HTTP header info is alread set in document

already

>+NS_IMETHODIMP
>+nsContentSink::ScriptEvaluated(nsresult aResult,
>+                               nsIDOMHTMLScriptElement *aElement,
>+                               PRBool aIsInline,
>+                               PRBool aWasPending)
>+{

>+  if (aElement != mScriptElements[count-1]) {

Spaces around -.

>+    return NS_OK;
>+  }
>+
>+  // Pop the script element stack
>+  mScriptElements.RemoveObjectAt(count-1); 

Spaces around -.

>+nsresult
>+nsContentSink::ProcessHTTPHeaders(nsIChannel* aChannel)
>+{

>+  while (*name) {
>+    rv = httpchannel->GetResponseHeader(nsDependentCString(*name), tmp);
>+    if (NS_SUCCEEDED(rv) && !tmp.IsEmpty()) {
>+      nsCOMPtr<nsIAtom> key = do_GetAtom(*name);
>+      ProcessHeaderData(key, NS_ConvertASCIItoUCS2(tmp));
>+    }
>+    name++;
>+  }
>+  
>+  return rv;

This return seems useless, we'll only return the result of the last
GetResponseHeader anyway. I'd just return NS_OK and declare rv inside the loop.
And use ++name.

>+nsresult
>+nsContentSink::ProcessHeaderData(nsIAtom* aHeader, const nsAString& aValue,
>+                                 nsIContent* aContent)
>+{

>+  } else if (aHeader == nsHTMLAtoms::setcookie) {
>+

Move the else to the next line and remove the blank line.

>+  } else if (aHeader == nsHTMLAtoms::link) {

Move the else to the next line.

>+nsresult
>+nsContentSink::ProcessLink(nsIContent* aElement,
>+                           const nsAString& aHref, const nsAString& aRel,
>+                           const nsAString& aTitle, const nsAString& aType,
>+                           const nsAString& aMedia)
>+{

>+  // is it a stylesheet link?
>+  if (linkTypes.IndexOf(NS_LITERAL_STRING("stylesheet")) != -1) {
>+    PRBool isAlternate = linkTypes.IndexOf(NS_LITERAL_STRING("alternate")) != -1;
>+    rv = ProcessStyleLink(aElement, aHref, isAlternate, aTitle, aType,
>+                          aMedia);
>+  }
>+
>+  return rv;

I'd return NS_OK for linkTypes.IndexOf(NS_LITERAL_STRING("stylesheet")) == -1
and return the result of ProcessStyleLink at the end.

>+}
>+
>+nsresult
>+nsContentSink::ProcessStyleLink(nsIContent* aElement,
>+                                const nsAString& aHref,
>+                                PRBool aAlternate,
>+                                const nsAString& aTitle,
>+                                const nsAString& aType,
>+                                const nsAString& aMedia)
>+{
>+  nsresult rv = NS_OK;
...
>+  nsCOMPtr<nsIURI> url;
>+  rv = NS_NewURI(getter_AddRefs(url), aHref, nsnull, mDocumentBaseURL);

Declare rv here.

>+nsresult
>+nsContentSink::RefreshIfEnabled(nsIViewManager* vm)
>+{
>+  if (vm) {

Return early on !vm

>+    nsCOMPtr<nsIContentViewer> contentViewer;
>+    nsresult rv = mDocShell->GetContentViewer(getter_AddRefs(contentViewer));
>+    if (NS_SUCCEEDED(rv) && contentViewer) {

Drop rv, just check contentViewer and return early.

>+      PRBool enabled;
>+      contentViewer->GetEnableRendering(&enabled);
>+      if (enabled) {
>+        vm->EnableRefresh(NS_VMREFRESH_IMMEDIATE);
>+      }
>+    }
>+  }
>+  return NS_OK;
>+}

>Index: base/src/nsContentSink.h
>===================================================================

>+ * Version: NPL 1.1/GPL 2.0/LGPL 2.1

MPL/GPL/LGPL

>Index: html/document/src/nsHTMLContentSink.cpp
>===================================================================

>+class HTMLContentSink : public nsContentSink,
>+                        public nsIHTMLContentSink,
>                         public nsITimerCallback,
>-                        public nsICSSLoaderObserver,
> #ifdef DEBUG
>                         public nsIDebugDumpContent,
> #endif
>                         public nsIDocumentObserver
> {

>-  nsresult RefreshIfEnabled(nsIViewManager* vm);
>+  virtual nsresult RefreshIfEnabled(nsIViewManager* vm);

This doesn't need to be virtual, does it?

>-  PRBool PreEvaluateScript();
>-  void PostEvaluateScript();
>+  virtual void PreEvaluateScript();
>+  virtual void PostEvaluateScript();

This doesn't need to be virtual, does it?

> nsresult
> HTMLContentSink::RefreshIfEnabled(nsIViewManager* vm)
> {
>   NS_ENSURE_TRUE(mDocShell, NS_ERROR_FAILURE);
> 
>+  return nsContentSink::RefreshIfEnabled(vm);

I wonder if we could just move the mDocShell check into
nsContentSink::RefreshIfEnabled and remove this.

This looks good, though I think we might want to freshen up at least
ProcessLinkHeader at some time in the future. I'm not worried about the changes
in link-handling for XML, I think you've actually fixed bugs by unifying the
implementations.

Comment 28

[Harshal Pradhan]

>Make this MPL/GPL/LGPL

Just a quick question about this.

This patch is primarily moving around code. The code going into the new files
came from files that are currently NPL/GPL/LGPL. Am I allowed to relicence this
code when I move it to a new file?

Comment 29

[Peter Van der Beken [:peterv]]

In this case yes, since Netscape granted permission to relicense its NPL code to
MPL.

Comment 30

[Peter Van der Beken [:peterv]]

Actually, since it's already triple-licensed we might want to keep it as is and
let the relicensing effort (does thats still exist?) figure it out.

Comment 31

[Brendan Eich [:brendan]]

The relicensing effort still exists, but don't wait for it.  Help it out,
relicense from N => M now, and with the latest tri-license versions all around.
The Mozilla Foundation has permission to do this, and we could use your help, to
reduce the global relicensing effort's "tail end" work.

/be

Comment 32

[Harshal Pradhan]

>>+  virtual void PreEvaluateScript();
>>+  virtual void PostEvaluateScript();
>
>This doesn't need to be virtual, does it?

These do need to be virtual. They are overridding the nsContentSink ones.
I put a comment there to that effect.

>> nsresult
>> HTMLContentSink::RefreshIfEnabled(nsIViewManager* vm)
>> {
>>   NS_ENSURE_TRUE(mDocShell, NS_ERROR_FAILURE);
>> 
>>+  return nsContentSink::RefreshIfEnabled(vm);
>
>I wonder if we could just move the mDocShell check into
>nsContentSink::RefreshIfEnabled and remove this.

That made total sense. I went ahead and made that change. 

I've changed the license to MPL/GPL/LGPL. All the other stuff you raised has
been fixed.

Comment 33

[Harshal Pradhan]

Attachment: Final patch

Comment 34

[Harshal Pradhan]

mZ dropped by about 6K on Linux and under 4K on Windows.

However, this seems to have regressed Tp by ~1%.

I tried backing out the ScrollToRef() changes from the htmlsink. It didn't help.
I tried backing out all the <script> stuff. That seems to have not helped
either. So I've checked the whole thing back in again for now. 

I'm puzzled. Other than these two bits, nothing much should have changed for
html. And Tp should only really be affected by what happens in the htmlsink.
(Right?) 

comet seems to be showing a minor increase in Ts too (~0.5%) . But luna doesn't
agree, it shows no change at all. So, I don't think this is that serious.

The Tp thing is the bigger problem. Does anyone have any idea about why this
patch might have affected Tp? I don't have a clue. 

Should I back the whole thing out?

Comment 35

[Boris Zbarsky [:bzbarsky]]

> Should I back the whole thing out?

No, let's not do that..

If we could get access to way to run the Tp tests reasonably, it would be
possible to start with a pre-patch build and then slowly drop in the patch a bit
at a time till we found the problem... Doing this with tinderbox via cvs seems
like overkill.

Would it be possible to coopt one of the Tp tinderboxen for a day or two for this? 

Comment 36

[Brendan Eich [:brendan]]

dbaron and bryner are on more intimate terms than I am with the tinderboxen, but
sure.  We have other machines moved into the Mozilla Foundation HQ but not yet
on the net (we're waiting an inordinate amount of time for Pac Bell to install a
T1). Any of those machines could be brought home by a volunteer in the area and
used temporarily to find out what regressed Tp here.

/be

Comment 37

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

I did a bit of testing on btek yesterday.  Most if not all of the performance
loss was related to the nsHTMLContentSink changes, not surprisingly.  Making
only the changes to make nsHTMLContentSink inherit from nsContentSink didn't
seem to cause any loss, but only eliminating the shadowing member variables from
nsHTMLContentSink so it used the nsCOMPtr/nsCOMArray member variables from
nsContentSink caused about half the performance loss.  At that point I gave up
investigating.

Comment 38

[Harshal Pradhan]

Thanks a lot for the testing! (I've been looking into setting up the pageload
tests locally, but it sure looked like that would turn into a minor "project" by
itself :-))

I just saw your comments and numbers on tinderbox. It's very surprising to me
that using the members from nsContentSink would have so much of an effect. That
is the last thing that I would have suspected. 

So, what does this really indicate? It doesnt look like the nsCOMArray of
scriptelements is accessed that often. So, nsCOMPtr's are for some reason much
slower to use than the corresponding raw ptr in this case? Is should really not
be the case right?

Should I try going back to using raw pointers and explicit addref/release for
the three members that are accessed most often? (mParser/ mDocument/
mNodeInfoManager)

Comment 39

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

My guess is that it's using nsCOMPtrs/nsCOMArrays that slows things down (though
at least nsCOMPtr really shouldn't). Try using the same datatypes as are in
nsHTMLContentSink now and see if that helps.

Other then that I can only think of that the order of the members would matter,
though i have no idea why it should. You can test this by moving the members
that were moved to the baseclass to be first in the class.

Comment 40

[Pete Zha]

nsContentSink.cpp cause bug 226700. Could someone take a look at that bug? Thanks!

Comment 41

[Harshal Pradhan]

Marking fixed since the patch landed a long time back. 

For the last couple of months, I've not been able to make time to deal with the
pref regression from this patch (or any other mozilla stuff). I think I will be
able to do so during 1.7a. I've filed bug 229163 for the regression.