123343 - XML output handler needs to handle base-tags

Status: VERIFIED FIXED

(Core :: XSLT, defect, P3)

Description

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

We need to do what the xml contentsink does and special-handle <base> tags

Comment 1

[Axel Hecht]

and here is a piece of spec:
http://www.w3.org/TR/html4/struct/links.html#h-12.4

Comment 2

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

Attachment: patch v1

this adds a bunch of html-handling to the output handler. I desided to break
out the handling from ::closePrevious and ::startElement since with the new
code i needed to add it just became too messy IMHO.

So now we have ::startHTMLElement which is called right after the element is
created, and ::endHTMLElement which is called when the the element is closed.
::startHTMLElement returns a boolean which indicates if the element is to be
inserted in the tree or if that should be held off until the elements children
are processed (for <script>).

The patch fixes:
 * support <base>
 * support <meta http-equiv="...">
 * keep track of stylesheets and specify stylesheetindex to speed up (and
   correct) cascading order

Comment 3

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

Attachment: patch v1 -w

Comment 4

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

my patch, my bug

Comment 5

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

I just noted that this patch also fixes the fact that all stylesheets in the
result document gets doubled.

Comment 6

[Peter Van der Beken [:peterv]]

Comment on attachment 71100 [details] [diff] [review]
patch v1 -w

>+PRBool txMozillaXMLOutput::startHTMLElement(nsIDOMElement* aElement)
>+{
>+    PRBool dontAppendNode = PR_FALSE;
>             nsCOMPtr<nsIAtom> atom;
>-            currentContent->GetTag(*getter_AddRefs(atom));
>-            if ((atom == txHTMLAtoms::title) && mTitleElement) {
>+    nsCOMPtr<nsIContent> content = do_QueryInterface(aElement);
>+    content->GetTag(*getter_AddRefs(atom));
>+
>+    if (atom == txHTMLAtoms::script) {
>+        dontAppendNode = PR_TRUE;
>+    }
>+
>+    nsCOMPtr<nsIStyleSheetLinkingElement> ssle =
>+        do_QueryInterface(aElement);
>+    if (ssle) {
>+        // XXX Trick nsCSSLoader into blocking/notifying us?
>+        //     We would need to implement nsIParser and
>+        //     pass ourselves as first parameter to
>+        //     InitStyleLinkElement. We would then be notified
>+        //     of stylesheet loads/load failures.
>+        ssle->InitStyleLinkElement(nsnull, PR_FALSE);
>+        ssle->SetEnableUpdates(PR_FALSE);
>+    }
>+    
>+    return dontAppendNode;
>+}

I think you could just do mDontAddCurrent = (atom == txHTMLAtoms::script);

>Index: source/xslt/txMozillaXMLOutput.h
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/transformiix/source/xslt/txMozillaXMLOutput.h,v
>retrieving revision 1.2
>diff -u -w -r1.2 txMozillaXMLOutput.h
>--- source/xslt/txMozillaXMLOutput.h	19 Feb 2002 22:48:07 -0000	1.2
>+++ source/xslt/txMozillaXMLOutput.h	23 Feb 2002 19:08:44 -0000
>@@ -160,7 +160,9 @@
> 
> private:
>     void closePrevious(PRInt8 aAction);
>-    void startHTMLElement(nsIDOMElement* aElement);
>+    PRBool startHTMLElement(nsIDOMElement* aElement);
>+    void endHTMLElement(nsIDOMElement* aElement, PRBool aXHTML);
>+    void ProcessHTTPEquiv(nsIAtom* aHeader, nsAReadableString& aValue);

processHTTPEquiv

Comment 7

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

Attachment: patch v2

addresses petervs comments and moves the set-refresh-timer code to endDocument
on Pikes request.

Comment 8

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

oops, the comment at the top of txMozillaXMLOutput::processHTTPEquiv is 
supposed to say

    // For now we only handle "refresh". There's a longer list in
    // HTMLContentSink::ProcessHeaderData

done locally

Comment 9

[Peter Van der Beken [:peterv]]

Comment on attachment 72843 [details] [diff] [review]
patch v2

>+    if (!mRefreshString.IsEmpty()) {
>+        nsCOMPtr<nsIDocument> doc = do_QueryInterface(mDocument);
>+        nsCOMPtr<nsIScriptGlobalObject> sgo;
>+        doc->GetScriptGlobalObject(getter_AddRefs(sgo));
>+        if(!sgo)

Space after if

>+            return;
>+        nsCOMPtr<nsIDocShell> docShell;
>+        sgo->GetDocShell(getter_AddRefs(docShell));
>+        if(!docShell)

Ditto

>+            return;
>+        nsCOMPtr<nsIRefreshURI> refURI = do_QueryInterface(docShell);
>+        if(!refURI)

Ditto

>@@ -311,7 +307,7 @@
>     closePrevious(eCloseElement | eFlushText);
> 
>     nsresult rv;
>-
>+    

Don't add the extra spaces.

>+        nsCOMPtr<nsIDocument> doc = do_QueryInterface(mDocument);
>+        NS_ASSERTION(doc, "document doesn't implement nsIDocument");
>+        nsAutoString value;
>+        content->GetAttr(kNameSpaceID_HTML, txHTMLAtoms::target, value);
>+        doc->SetBaseTarget(value);

kNameSpaceID_None

>+
>+        content->GetAttr(kNameSpaceID_HTML, txHTMLAtoms::href, value);

Ditto.

r=peterv.

Comment 10

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

Attachment: patch v3

addresses all petervs comments

Comment 11

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

Attachment: *this* is patch v3

oops

Comment 12

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

Comment on attachment 73212 [details] [diff] [review]
*this* is patch v3

bringing over petervs r=

Comment 13

[Johnny Stenback (:jst)]

Comment on attachment 73212 [details] [diff] [review]
*this* is patch v3

>Index: source/xslt/txMozillaXMLOutput.cpp
[...]
> void txMozillaXMLOutput::endDocument()
[...]
>+        nsCOMPtr<nsIDocShell> docShell;
>+        sgo->GetDocShell(getter_AddRefs(docShell));
>+        if (!docShell)
>+            return;

The above null check is not needed, the do_QueryInterface() below will do the
null check for you.

>+        nsCOMPtr<nsIRefreshURI> refURI = do_QueryInterface(docShell);
>+        if (!refURI)
>+            return;
>+        

I don't like the early return here, what if someone adds code to the end of
this method, then it won't get executed if there's no docshell? I'd add an if
check and put the code inside it in stead of returning early in this case.

>+        if (!mScriptElements)
>+            NS_NewISupportsArray(getter_AddRefs(mScriptElements));
>+        NS_ASSERTION(mScriptElements, "Can't create array");
>+        if (mScriptElements)
>+            mScriptElements->AppendElement(scriptElement);

Don't you want to return NS_ERROR_OUT_OF_MEMORY in cases like this if we can't
create a new nsISupportsArray?

Other than that, sr=jst

Comment 14

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

Attachment: patch v4

addresses all jsts comments except the last since the function returns void.

Got ok over irc

Comment 15

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

Comment on attachment 73612 [details] [diff] [review]
patch v4

bringing over reviews

Comment 16

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

Attachment: *this* is patch v4

i really must stop hacking at night. Saving before diffing makes so big
difference

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 73620 [details] [diff] [review]
*this* is patch v4

a=roc+moz

Comment 18

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

checked in

Comment 19

[Axel Hecht]

we didn't verify for a long time.
I really checked, so VERIFIED.