[Webkit-unassigned] [Bug 10313] xsl:import and document() don't work in stylesheets loaded via XMLHttpRequest

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=10313


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #58970|review?                     |review-
               Flag|                            |




--- Comment #47 from Darin Adler <darin at apple.com>  2010-06-17 07:43:40 PST ---
(From update of attachment 58970)
> Index: WebCore/dom/DOMImplementation.h
> ===================================================================
> --- WebCore/dom/DOMImplementation.h	(revision 61177)
> +++ WebCore/dom/DOMImplementation.h	(working copy)
> @@ -24,6 +24,7 @@
>  #ifndef DOMImplementation_h
>  #define DOMImplementation_h
>  
> +#include <Document.h>
>  #include <wtf/PassRefPtr.h>
>  #include <wtf/RefCounted.h>

Why does DOMImplementation.h now need to include Document.h? I don't see new code that depends on Document, but rather ScriptExecutionContext.h. I think it would make sense to include "ScriptExecutionContext.h" here. Further, the syntax <Document.h> is incorrect, since we are in the same module. The include should be "Document.h". The patch should instead add an include of "ScriptExecutionContext.h" and we should remove the includes of PassRefPtr.h and RefCounted.h since ScriptExecutionContext.h already includes those.

But I'm wondering if it's right to store a RefPtr<ScriptExecutionContext> in DOMImplementation.h anyway. I think maybe we should be storing a RefPtr<Document>. If this is always going to be a Document and never a WorkerContext then perhaps we should do the conversion from ScriptExecutionContext at create time and store the pointer as a .

We could also avoid adding includes here by making the functions non-inline.

At minimum, this needs to be changed to "Document.h", but you could consider these other comments as well.

> -        m_implementation = DOMImplementation::create();
> +        m_implementation = DOMImplementation::create(scriptExecutionContext());

I think you can just pass "this" here. There's no need to explicitly call the scriptExecutionContext() function; a Document is a script execution context.

> +void Document::setOriginDocument(Document* originDocument)

A function that takes ownership of its argument should take a PassRefPtr<Document>, not a Document*.

> +Frame* Document::findFrame() const

Since Document already has a frame function it's important to name this in a way that makes it clear when you would call this instead of frame. I think we need some sort of adjective between the word "find" and the word "frame". This is the kind of thing that can make the class very confusing later if the name does not make it clear. How would you describe the difference between frame and findFrame in words? When would you use findFrame? This should inform a better name for findFrame. Maybe findFrameForSubresourceLoads?

I don't think it's really good to have this be a const member function. It doesn't make much sense to talk about a const Document anyway, since you can get to the frame and back to the document. This is not a simple value-type object where const is a really useful concept. I suggest omitting the const.

> +    const Document* document = this;
> +
> +    while (document) {
> +        if (document->frame())
> +            return document->frame();
> +        document = document->originDocument();
> +    }

I think this would read better as a for loop:

    for (Document* document = this; document; document = document->originDocument()) {
        if (Frame* frame = document->frame())
            return frame;
    }

> -    RefPtr<XSLTProcessor> processor = XSLTProcessor::create();
> +    RefPtr<XSLTProcessor> processor = XSLTProcessor::create(scriptExecutionContext());

Again, please just pass "this".

> +        RefPtr<ScriptExecutionContext> m_scriptExecutionContext;

As above in DOMImplementation, I think it might be better for the pointer in DOMParser to be a Reftr<Document> and convert from script execution context to document at create time instead of doing it at parse time.

ScriptExecutionContext is an abstraction, but we don't want to overdo the abstraction in the code. If we can get to a document, then it's best to do that so the concepts in the code are clearer.

> +#include "Frame.h"
> +#include "Node.h"
> +#include "ResourceError.h"
> +#include "TextResourceDecoder.h"
>  #include "XSLStyleSheet.h"

Are all of these includes needed? Maybe some of them include the others?

> +    XSLStyleSheet* rootXSL = 0;

I think rootXSL is an unclear name for this, because it's not an "XSL". You could name it rootXSLStyleSheet or even rootStyleSheet.

> +    Node* ownerNode = rootXSL->ownerNode();

This dereferences rootXSL without checking if it's non-zero, so will dereference the null pointer in that case. We need a test case covering that code path and the code needs to handle it correctly.

> +        // When parent stylesheet doesn't have a final url, use base url from the

In comments please call it a "URL" not a "url".

> +        // document the root stylesheet originated from. This can happen when the
> +        // stylesheet is created with DOMParser.parseFromString(), for example.
> +        if (Document* originDocument = ownerNode->document()->originDocument())
> +            absHref = KURL(originDocument->baseURL(), m_strHref).string();

Normally to resolve a URL against a document's base we would use the Document::completeURL function rather than using KURL directly. You should do that here unless there's a reason not to.

> +    if (ownerNode && ownerNode->nodeType() == Node::PROCESSING_INSTRUCTION_NODE)
> +        finishLoadingSheetAsynchronously(docLoader, absHref);
> +    else
> +        finishLoadingSheetSynchronously(docLoader, absHref);

This needs a comment. It's entirely unclear why you would do an asynchronous load when the node type is a processing instruction!

> +void XSLImportRule::finishLoadingSheetSynchronously(DocLoader* docLoader, const String& absHref)

Is there any way to share more code with the finishLoadingSheetAsynchronously function? This has an unpleasantly large amount of code.

> +    KURL url(ParsedURLString, absHref);

We already had this in a KURL in the function that calls this function. But then we turned it into a string and back into a KURL. Lets change the argument types so we don't have to convert in this fashion.

> +    if (error.isNull()) {
> +        RefPtr<TextResourceDecoder> decoder = TextResourceDecoder::create("text/xsl");
> +        String encoding = response.textEncodingName();
> +        if (!encoding.isNull())
> +            decoder->setEncoding(encoding, TextResourceDecoder::EncodingFromHTTPHeader);
> +
> +        String sheet(decoder->decode(data.data(), data.size()));
> +        setXSLStyleSheet(absHref, response.url(), sheet);
> +    }

The usual idiom for this sort of thing in WebKit is an early return. So you would write:

    if (error.isNotNull())
        return;

instead of nesting the non-error code inside an if statement.

> +    RefPtr<ScriptExecutionContext> m_scriptExecutionContext;

Same comment for XSLTProcessor as the other classes. I suggest converting to a Document at create time and using a RefPtr<Document> rather than a RefPtr<ScriptExecutionContext>.

review- because of the null dereference getting ownerNode. I am not confident that test cases cover all the code changes here and I'd like you to review the coverage of the tests you added. Does every new piece of code get tested at least once?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.