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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sat Jun 12 18:47:05 PDT 2010


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


Darin Adler <darin at apple.com> changed:

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




--- Comment #44 from Darin Adler <darin at apple.com>  2010-06-12 18:47:01 PST ---
(From update of attachment 58131)
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

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


More information about the webkit-unassigned mailing list