93218 - XMLContentSink doesn't have a ProcessHTTPHeaders() method

Status: VERIFIED FIXED

(Core :: XML, defect, P3)

Description

[Hixie (not reading bugmail)]

Harish: When you fixed bug 3248 for HTML, you omitted to fix it for XML too.
So XML currently ignores all the pretty HTTP headers that HTML currently 
accepts. See bug 3248 for a reminder of the problem.

Comment 1

[harishd]

setting to m0.9.4

Comment 2

[harishd]

Ian: could you please attach a test case? Thanx

Comment 3

[harishd]

Giving bug to heikki!

I tried to copy the code into XMLContentSink but then got stuck with the
difference in ProcessStyleLink() signature :-(

Comment 4

[Hixie (not reading bugmail)]

HTML case, works: http://www.hixie.ch/tests/adhoc/http/link/001.html
XML case, doesn't work: http://www.hixie.ch/tests/adhoc/http/link/002.xml

Comment 5

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: WIP, does not work

Comment 6

[scottputterman]

ADT3 per ADT triage.

Comment 7

[Heikki Toivonen (remove -bugzilla when emailing directly)]

*** Bug 120836 has been marked as a duplicate of this bug. ***

Comment 8

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: Proposed fix, mostly copy from HTML

Comment 9

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Harish, can you review?

This makes us process the HTTP headers pretty much the same way we do them for
HTML. Most of the code has been copied from HTML content sink. *shiver* Horrible
looking code...

Notice that we do this only for XML documents that are loaded normally for
viewing (also when they are loaded as data) but not for XSLT style sheets or
XBL. XSLT and XBL reuse the XML content sink so they would get this benefit if
they passed in a channel to the XML content sink Init() method, or if they
create a sink with NS_NewXMLContentSink() and pass in a channel. 

XBL is currently loaded from local disc so there is no reason to do it. 

I believe it would be really rare to see an XML document have XSLT stylesheet
that had HTTP header supplying XSLT stylesheet for the first XSLT stylesheet - I
am not even sure if we have the architecture for that in the XSLT engine. It
looks like there would be a little bit of work to get a channel from XSLT in any
case. If we want this, a new bug should be filed.

I don't know if XUL supports HTTP headers, but if it doesn't and we want it to
we could use this approach there as well. We should file a new bug on that.

Comment 10

[Hixie (not reading bugmail)]

heikki: Wouldn't the bug for that be the one that asks for us to merge the XML
and XUL content sinks? :-)

Comment 11

[Heikki Toivonen (remove -bugzilla when emailing directly)]

That is one way of fixing it, but not the only one.

Comment 12

[Darin Fisher]

Comment on attachment 75500 [details] [diff] [review]
Proposed fix, mostly copy from HTML

sr=darin, i hate seeing so much duplicated code :(  hopefully, only temporary.

Comment 13

[harishd]

Comment on attachment 75500 [details] [diff] [review]
Proposed fix, mostly copy from HTML

>+  } else 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, kNameSpaceID_Unknown,
>+                                   mStyleSheetCount++, 
>+                                   ((!aAlternate) ? mParser : nsnull),
>+                                   doneLoading, 
>+                                   this);
>+    if (NS_SUCCEEDED(rv) || (rv == NS_ERROR_HTMLPARSER_BLOCK)) {
>+      if (rv == NS_ERROR_HTMLPARSER_BLOCK && mParser) {
>+        mParser->BlockParser();
>+      }
>+      mStyleSheetCount++;
>+    }

How is this related to supporting HTTP headers in XML?

>+static IsAlternateHTTPStyleSheetHeader(const nsAString& aRel)
>+{

How did this work without any return type?

>+nsresult 
>+nsXMLContentSink::ProcessLink(nsIHTMLContent* aElement, const nsAReadableString& aLinkData)
>+{
> nsXMLContentSink::ProcessHeaderData(nsIAtom* aHeader,const nsAReadableString& aValue,nsIHTMLContent* aContent)
> {
>+nsXMLContentSink::ProcessHTTPHeaders(nsIChannel* aChannel) {

Are these methods a verbatim of HTMLContentSink? If they are then I don't have
to
review it. right? :)

Comment 14

[Heikki Toivonen (remove -bugzilla when emailing directly)]

That part that you questioned is executed if a HTTP header specifies a CSS
stylesheet.

Good catch about the missing return type, I'll fix it. Still, it works because
by default a return type is |int|.

They are nearly verbatim - see them side by side and there may be a difference
in how some function is called, or such a thing, because some functions these
functions are calling may have different signatures between the sinks.

Comment 15

[harishd]

Comment on attachment 75500 [details] [diff] [review]
Proposed fix, mostly copy from HTML

>+        result = ProcessStyleLink(
>+          aElement, 
>+          href, 
>+          !title.IsEmpty() && IsAlternateHTTPStyleSheetHeader(rel), 
>+          title, 
>+          type, 
>+          media);

OPTIONAL :

Could you write this as
result = 
  ProcessStyleLink(aElement, 
		   href, 
		   !title.IsEmpty() && IsAlternateHTTPStyleSheetHeader(rel), 
		   title, 
		   type, 
		   media);

Comment 16

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

Comment on attachment 75500 [details] [diff] [review]
Proposed fix, mostly copy from HTML

a=dbaron for trunk checkin

Comment 17

[Johnny Stenback (:jst)]

Fix checked in. Marking FIXED.

Comment 18

[Chris Petersen]

Changing QA contact

Comment 19

[Hixie (not reading bugmail)]

VERIFIED with test case in comment 4.

Thanks!!!