[webkit-reviews] review denied: [Bug 10313] xsl:import and document() don't work in stylesheets loaded via XMLHttpRequest : [Attachment 58131] 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 58131: proposed patch
https://bugs.webkit.org/attachment.cgi?id=58131&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
Thanks for taking this on.

I'm pretty sure this would work out better if it was a document that had
subdocuments rather than a frame that has subdocuments. Similarly, the owner
should be a document rather than a frame.

> -    return CREATE_DOM_OBJECT_WRAPPER(exec, jsConstructor->globalObject(),
XSLTProcessor, XSLTProcessor::create().get());
> +    RefPtr<XSLTProcessor> xsltProcessor = XSLTProcessor::create(context);
> +    return CREATE_DOM_OBJECT_WRAPPER(exec, jsConstructor->globalObject(),
XSLTProcessor, xsltProcessor.get());

Breaking this up into two lines doesn't seem like a big improvement. You could
have just added the "context" to the existing line and I think it would have
read more clearly.

As long as you are adding a local variable, I would suggest just calling it
processor; no need to give it the more awkward name xsltProcessor.

> +    if (m_scriptExecutionContext && m_scriptExecutionContext->isDocument())
{
> +	   Document* originDoc =
static_cast<Document*>(m_scriptExecutionContext);
> +	   if (originDoc->frame())
> +	       originDoc->frame()->registerSubDocument(doc.get());
> +    }

Please don't abbreviate "document" as "doc" in new code.

> -    DOMImplementation() { }
> +    DOMImplementation(ScriptExecutionContext* context)
> +	   : m_scriptExecutionContext(context) { }

Once we reach the point where the definition is not all on one line, we use a
different brace style. It would be like this:

    DOMImplementation(ScriptExecutionContext* context) :
m_scriptExecutionContext(context) { }

Or this:

    DOMImplementation(ScriptExecutionContext* context)
	: m_scriptExecutionContext(context)
    {
    }

> +    ScriptExecutionContext* m_scriptExecutionContext;

I don't understand why it's safe for a DOMImplementation object to hold a raw
pointer to a ScriptExecutionContext. What guarantees that the context won't be
deallocated? I think this needs to be a RefPtr.

> +void Document::setOriginFrame(Frame* frame, const KURL& url)

It's entirely unclear what the URL is here. I know you understand it, but you
need to leave something behind to make it clear what the URL is.

> +{
> +    m_originFrame = frame;
> +    if (m_originFrame) {
> +	   if (!url.isEmpty()) {
> +	       m_cookieURL = url;
> +	      
ScriptExecutionContext::setSecurityOrigin(SecurityOrigin::create(url));
> +	   } else if (Document* doc = m_originFrame->document()) {

Please don't use the abbreviation "doc" for "document" in new code.

Why is it OK to leave m_cookieURL set to the old value when the origin frame is
set to 0? Why is it OK to leave the security origin when the frame is set to 0.
It seems to me that the function setting the frame to 0 is really a different
one than the one setting the frame. All the call sites seem to be "always
setting the frame" and "always disconnecting the frame" so I suggest having two
separate functions for this.

> +    Frame* originFrame() const { return m_originFrame; } // can be NULL
> +    void setOriginFrame(Frame* frame, const KURL& url = KURL());

I find the name "origin frame" to be confusing. Is there some term used for
this in the HTML specification? Also, can this be a document instead of a
frame? The same frame is reused for successive documents, and so having a
pointer to a frame is more dangerous than a pointer to a document, because the
document is not reused.

The comment saying this can be null is not that useful. A more useful comment
would be one defining the term "origin frame".

> +void Frame::registerSubDocument(Document* doc, const KURL& url)

Since the word is "subdocument" and not the two words "sub document" it should
be registerSubdocument, unregisterSubdocument, and m_subdocuments.

> +    if (doc) {

Why is the check for null needed? Is it legal for someone to call this with a
null pointer?

> +void Frame::clearRegisteredSubDocuments()
> +{
> +    if (!m_subDocuments.isEmpty()) {

In WebKit we normally use early return for this sort of thing, rather than
nesting the function body inside an if.

> +	   for (HashSet<Document*>::iterator it = m_subDocuments.begin();
> +		it != m_subDocuments.end(); ++it)

This for statement should be on one line in the usual WebKit coding style.

> +	   void registerSubDocument(Document* doc, const KURL& url = KURL());
> +	   void unregisterSubDocument(Document* doc);

The argument names here should be omitted.

> +	   // use base url from originFrame's document when parent stylesheet
dont have an final url
> +	   // this can for example happen when the stylesheet is created with
DOMParser.parseFromString

Comments should be in sentence style with capital letter and period at the end.
Also worth fixing up the grammar.

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

These new functions don't have good names. For one thing, they don't do the
entire job of loading the sheet, since they don't do the other things that
loadSheet does, so the names are poor in that sense. Also, we normally use the
adverbs "synchronously" and "asynchronously" as you can see in various existing
function names, and you should do the same here. Maybe
finishLoadingSheetAsynchronously?

It’s also unfortunate that we have copied and pasted code now. Is there a way
to share more of the code?

> +    void loadSheetAsync(DocLoader* docLoader, const String& absHref);
> +    void loadSheetSync(DocLoader* docLoader, const String& absHref);

The argument names "docLoader" should be omitted here.

It does look like you added a good number of test cases, which is great.

review- largely because of the m_scriptExecutionContext raw pointer issue