[webkit-reviews] review denied: [Bug 10313] xsl:import and document() don't work in stylesheets loaded via XMLHttpRequest : [Attachment 58970] New proposed patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Andreas Wictor
<andreas.wictor at xcerion.com>'s request for review:
Bug 10313: xsl:import and document() don't work in stylesheets loaded via
XMLHttpRequest
https://bugs.webkit.org/show_bug.cgi?id=10313

Attachment 58970: New proposed patch
https://bugs.webkit.org/attachment.cgi?id=58970&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> 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?