[webkit-reviews] review denied: [Bug 10313] xsl:import and document() don't work in stylesheets loaded via XMLHttpRequest : [Attachment 59361] New proposed patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Jun 24 11:26:53 PDT 2010
Alexey Proskuryakov <ap at webkit.org> 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 59361: New proposed patch
https://bugs.webkit.org/attachment.cgi?id=59361&action=review
------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
+void Document::setOriginDocument(PassRefPtr<Document> originDocument)
+{
+ // This function stores a reference to the document where this document
+ // originated from. This is used for frameless documents, so that we can
I really dislike it that both Document and DOMImplementation get an
m_originDocument reference. It doesn't seem that these should ever be
different, but it raises all sorts of unpleasant questions about when exactly
they are different in practice.
+ // find a frame when we are loading subresources. Also, because frameless
+ // documents don't get a SecurityOrigin and a cookieURL when constructed,
+ // we need to set these here to be able to load subresources.
Please add assertions that there is no SecurityOrigin and cookieURL indeed.
m_responseXML->setURL(m_url);
+ m_responseXML->setOriginDocument(document());
This looks weird to me. The result of this code is that a script can decide
which security origin to use with the result of XMLHttpRequest -
req.responseXML will use one, DOMParser.parseFromString(req.responseText,
"application/xml") will use another. This sort of freedom looks highly
suspicious.
I'm not sure what exactly I'm asking for here - perhaps someone CC'ed on this
bug (Adam?) could weigh in.
if (!parentSheet->finalURL().isNull())
- // use parent styleheet's URL as the base URL
- absHref = KURL(parentSheet->finalURL(), m_strHref).string();
-
+ // Use parent styleheet's URL as the base URL.
+ absHref = KURL(parentSheet->finalURL(), m_strHref);
+ else if (originDocument)
+ // When parent stylesheet doesn't have a final URL, use base URL from
the
+ // document the root stylesheet originated from. This can happen when
the
+ // stylesheet is created with DOMParser.parseFromString(), for
example.
+ absHref = originDocument->completeURL(m_strHref);
Per WebKit coding style, these blocks should be in braces (comments do count).
+ else
+ absHref = KURL(ParsedURLString, m_strHref);
This doesn't seem right - what guarantees that m_strHref is indeed parsed? When
do we take this code path?
It seems that this should be ASSERT_NOT_REACHED plus an early return instead.
I'm going to say r- for the above comments, but there is also something I don't
understand in how subresources are loaded in general. There is that old ugly
code in XSLStyleSheet::loadChildSheets() that iterates over stylesheets to find
imports and includes - it it obsoleted by your code? How do they interact?
I'll try to figure this out myself later, but an explanation from you would be
appreciated, too.
+ void finishLoadingSheetAsynchronously(DocLoader*, const KURL&);
+ void finishLoadingSheetSynchronously(DocLoader*, const KURL&);
A synchronous load can be started while an asynchronous one is in progress. But
we're using global variables for loading, see globalProcessor and
globalDocLoader. Is there a race condition here?
More information about the webkit-reviews
mailing list