Refactoring XSLT

Refactoring XSLT

- added DocumentXSLT, document's supplement.
- moved transformSourceDocument and applyXSLTransform from Document to DocumentXSLT.
- moved StyleEngine's XSLT code to DocumentXSLT by using EventListener.

BUG=367689
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=185643

[tasak, 2014-11-17 09:35:57]

tasak@google.com changed reviewers:
+ haraken@chromium.org

[tasak, 2014-11-17 09:35:57]

PTAL?

[haraken, 2014-11-17 14:18:32]

https://codereview.chromium.org/730003002/diff/1/Source/core/dom/ProcessingIn...
File Source/core/dom/ProcessingInstruction.cpp (right):

https://codereview.chromium.org/730003002/diff/1/Source/core/dom/ProcessingIn...
Source/core/dom/ProcessingInstruction.cpp:267: // No need to register
XSLStyleSheet with StyleEngine.

with => to

https://codereview.chromium.org/730003002/diff/1/Source/core/dom/ProcessingIn...
Source/core/dom/ProcessingInstruction.cpp:313:
visitor->trace(m_listenerForXSLT);

You shouldn't trace m_listenerForXSLT, since m_listenerForXSLT is not a Member.

https://codereview.chromium.org/730003002/diff/1/Source/core/xml/DocumentXSLT...
File Source/core/xml/DocumentXSLT.cpp (right):

https://codereview.chromium.org/730003002/diff/1/Source/core/xml/DocumentXSLT...
Source/core/xml/DocumentXSLT.cpp:1: #include "config.h"

Add a copyright.

https://codereview.chromium.org/730003002/diff/1/Source/core/xml/DocumentXSLT...
Source/core/xml/DocumentXSLT.cpp:20: namespace {

What is this namespace for?

https://codereview.chromium.org/730003002/diff/1/Source/core/xml/DocumentXSLT...
Source/core/xml/DocumentXSLT.cpp:37: // Don't apply XSL transforms to already
transformed documents -- <rdar://problem/4132806>

Remove the rdar part.

https://codereview.chromium.org/730003002/diff/1/Source/core/xml/DocumentXSLT...
Source/core/xml/DocumentXSLT.cpp:65: RefPtr<ProcessingInstruction>
m_processingInstruction;

This should be RefPtrWillBePersistent. Let's confirm that this CL builds with
enable_oilpan=1.

https://codereview.chromium.org/730003002/diff/1/Source/core/xml/DocumentXSLT...
Source/core/xml/DocumentXSLT.cpp:84: v8::Local<v8::Context> context =
toV8Context(document.frame(), DOMWrapperWorld::mainWorld());

Is it guaranteed that this is for the main world?

https://codereview.chromium.org/730003002/diff/1/Source/core/xml/DocumentXSLT...
Source/core/xml/DocumentXSLT.cpp:85: ScriptState* scriptState =
ScriptState::from(context);

You can use ScriptState::forMainWorld(document.frame()). Then you won't need to
create a HandleScope.

https://codereview.chromium.org/730003002/diff/1/Source/core/xml/DocumentXSLT.h
File Source/core/xml/DocumentXSLT.h (right):

https://codereview.chromium.org/730003002/diff/1/Source/core/xml/DocumentXSLT...
Source/core/xml/DocumentXSLT.h:1: #ifndef DocumentXSLT_h

Add a copyright.

https://codereview.chromium.org/730003002/diff/1/Source/core/xml/DocumentXSLT...
Source/core/xml/DocumentXSLT.h:35: static bool
hasTransformSourceDocument(Document&);

We don't want to add these static methods to supplement classes. Instead, the
caller sites should call:

  DocumentXSLT::from(*document).sheetLoaded()

https://codereview.chromium.org/730003002/diff/1/Source/core/xml/XSLTProcesso...
File Source/core/xml/XSLTProcessor.cpp (right):

https://codereview.chromium.org/730003002/diff/1/Source/core/xml/XSLTProcesso...
Source/core/xml/XSLTProcessor.cpp:93: result->initContentSecurityPolicy(csp);

Just help me understand: Why do we need to copy the ContentSecurityPolicy?

https://codereview.chromium.org/730003002/diff/1/Source/core/xml/parser/XMLDo...
File Source/core/xml/parser/XMLDocumentParser.cpp (right):

https://codereview.chromium.org/730003002/diff/1/Source/core/xml/parser/XMLDo...
Source/core/xml/parser/XMLDocumentParser.cpp:106: if
(document->sawElementsInKnownNamespaces() ||
DocumentXSLT::hasTransformSourceDocument(*document))

We want to write this as:

  DocumentXSLT::from(*document).hasTransformSourceDocument()

https://codereview.chromium.org/730003002/diff/1/Source/core/xml/parser/XMLDo...
Source/core/xml/parser/XMLDocumentParser.cpp:1169: if (m_sawXSLTransform &&
!DocumentXSLT::hasTransformSourceDocument(*document())) {

Ditto.

https://codereview.chromium.org/730003002/diff/1/Source/core/xml/parser/XMLEr...
File Source/core/xml/parser/XMLErrors.cpp (right):

https://codereview.chromium.org/730003002/diff/1/Source/core/xml/parser/XMLEr...
Source/core/xml/parser/XMLErrors.cpp:155: if
(DocumentXSLT::hasTransformSourceDocument(*m_document)) {

Ditto.

[tasak, 2014-11-19 08:23:17]

Thank you for review.

https://codereview.chromium.org/730003002/diff/1/Source/core/dom/ProcessingIn...
File Source/core/dom/ProcessingInstruction.cpp (right):

https://codereview.chromium.org/730003002/diff/1/Source/core/dom/ProcessingIn...
Source/core/dom/ProcessingInstruction.cpp:267: // No need to register
XSLStyleSheet with StyleEngine.
On 2014/11/17 14:18:31, haraken wrote:
> 
> with => to

Done.

https://codereview.chromium.org/730003002/diff/1/Source/core/dom/ProcessingIn...
Source/core/dom/ProcessingInstruction.cpp:313:
visitor->trace(m_listenerForXSLT);
On 2014/11/17 14:18:31, haraken wrote:
> 
> You shouldn't trace m_listenerForXSLT, since m_listenerForXSLT is not a
Member.

Done.

https://codereview.chromium.org/730003002/diff/1/Source/core/xml/DocumentXSLT...
File Source/core/xml/DocumentXSLT.cpp (right):

https://codereview.chromium.org/730003002/diff/1/Source/core/xml/DocumentXSLT...
Source/core/xml/DocumentXSLT.cpp:1: #include "config.h"
On 2014/11/17 14:18:31, haraken wrote:
> 
> Add a copyright.

Done.

https://codereview.chromium.org/730003002/diff/1/Source/core/xml/DocumentXSLT...
Source/core/xml/DocumentXSLT.cpp:20: namespace {
On 2014/11/17 14:18:31, haraken wrote:
> 
> What is this namespace for?

Done.

https://codereview.chromium.org/730003002/diff/1/Source/core/xml/DocumentXSLT...
Source/core/xml/DocumentXSLT.cpp:37: // Don't apply XSL transforms to already
transformed documents -- <rdar://problem/4132806>
On 2014/11/17 14:18:31, haraken wrote:
> 
> Remove the rdar part.

Done.

https://codereview.chromium.org/730003002/diff/1/Source/core/xml/DocumentXSLT...
Source/core/xml/DocumentXSLT.cpp:65: RefPtr<ProcessingInstruction>
m_processingInstruction;
On 2014/11/17 14:18:31, haraken wrote:
> 
> This should be RefPtrWillBePersistent. Let's confirm that this CL builds with
> enable_oilpan=1.

Done.

https://codereview.chromium.org/730003002/diff/1/Source/core/xml/DocumentXSLT...
Source/core/xml/DocumentXSLT.cpp:84: v8::Local<v8::Context> context =
toV8Context(document.frame(), DOMWrapperWorld::mainWorld());
On 2014/11/17 14:18:31, haraken wrote:
> 
> Is it guaranteed that this is for the main world?

According to offline discussion, for the main world.

https://codereview.chromium.org/730003002/diff/1/Source/core/xml/DocumentXSLT...
Source/core/xml/DocumentXSLT.cpp:85: ScriptState* scriptState =
ScriptState::from(context);
On 2014/11/17 14:18:31, haraken wrote:
> 
> You can use ScriptState::forMainWorld(document.frame()). Then you won't need
to
> create a HandleScope.

Done.

https://codereview.chromium.org/730003002/diff/1/Source/core/xml/DocumentXSLT.h
File Source/core/xml/DocumentXSLT.h (right):

https://codereview.chromium.org/730003002/diff/1/Source/core/xml/DocumentXSLT...
Source/core/xml/DocumentXSLT.h:1: #ifndef DocumentXSLT_h
On 2014/11/17 14:18:32, haraken wrote:
> 
> Add a copyright.

Done.

https://codereview.chromium.org/730003002/diff/1/Source/core/xml/DocumentXSLT...
Source/core/xml/DocumentXSLT.h:35: static bool
hasTransformSourceDocument(Document&);
On 2014/11/17 14:18:32, haraken wrote:
> 
> We don't want to add these static methods to supplement classes. Instead, the
> caller sites should call:
> 
>   DocumentXSLT::from(*document).sheetLoaded()

I added the comment:
    // The following static methods doesn't use any instance of DocumentXSLT.   
                           
    // Just they are just using DocumetnXSLT namespace.

https://codereview.chromium.org/730003002/diff/1/Source/core/xml/XSLTProcesso...
File Source/core/xml/XSLTProcessor.cpp (right):

https://codereview.chromium.org/730003002/diff/1/Source/core/xml/XSLTProcesso...
Source/core/xml/XSLTProcessor.cpp:93: result->initContentSecurityPolicy(csp);
On 2014/11/17 14:18:32, haraken wrote:
> 
> Just help me understand: Why do we need to copy the ContentSecurityPolicy?

Because Document::initContentSecurityPolicy did:

void Document::initContentSecurityPolicy(PassRefPtr<ContentSecurityPolicy> csp)
{   
    setContentSecurityPolicy(csp ? csp : ContentSecurityPolicy::create());
    if (m_frame && m_frame->tree().parent() &&
m_frame->tree().parent()->isLocalFrame() &&
(shouldInheritSecurityOriginFromOwner(m_url) || isPluginDocument()))
       
contentSecurityPolicy()->copyStateFrom(toLocalFrame(m_frame->tree().parent())->document()->contentSecurityPolicy());
    if (transformSourceDocument()) // <--
       
contentSecurityPolicy()->copyStateFrom(transformSourceDocument()->contentSecurityPolicy());
// <--
    contentSecurityPolicy()->bindToExecutionContext(this);
}

Before applying this patch, i.e. result->initContentSecurityPolicy(), the
followings were done:

(1) ContentSecurityPolicy::create() (in Document.cpp),
(2) copy frame document's ContentSecurityPolicy (in Document.cpp),
(3) copy transformSourceDocument's ContentSecurityPolicy (in Document.cpp)

So, now (1) => (3) => (2)
(1) ContentSecurityPolicy::create() (in XSLTProcessor.cpp),
(3) copy transformSourceDocument's ContentSecurityPolicy (in XSLTProcessor.cpp),
(2) copy frame document's ContentSecurityPolicy (in Document.cpp)

https://codereview.chromium.org/730003002/diff/1/Source/core/xml/parser/XMLDo...
File Source/core/xml/parser/XMLDocumentParser.cpp (right):

https://codereview.chromium.org/730003002/diff/1/Source/core/xml/parser/XMLDo...
Source/core/xml/parser/XMLDocumentParser.cpp:106: if
(document->sawElementsInKnownNamespaces() ||
DocumentXSLT::hasTransformSourceDocument(*document))
On 2014/11/17 14:18:32, haraken wrote:
> 
> We want to write this as:
> 
>   DocumentXSLT::from(*document).hasTransformSourceDocument()

We don't need to create any DocumentXSLT instance here.

https://codereview.chromium.org/730003002/diff/1/Source/core/xml/parser/XMLDo...
Source/core/xml/parser/XMLDocumentParser.cpp:1169: if (m_sawXSLTransform &&
!DocumentXSLT::hasTransformSourceDocument(*document())) {
On 2014/11/17 14:18:32, haraken wrote:
> 
> Ditto.

Ditto.

https://codereview.chromium.org/730003002/diff/1/Source/core/xml/parser/XMLEr...
File Source/core/xml/parser/XMLErrors.cpp (right):

https://codereview.chromium.org/730003002/diff/1/Source/core/xml/parser/XMLEr...
Source/core/xml/parser/XMLErrors.cpp:155: if
(DocumentXSLT::hasTransformSourceDocument(*m_document)) {
On 2014/11/17 14:18:32, haraken wrote:
> 
> Ditto.

Ditto.

[haraken, 2014-11-19 09:01:36]

Mostly looks good.

https://codereview.chromium.org/730003002/diff/20001/Source/core/xml/Document...
File Source/core/xml/DocumentXSLT.cpp (right):

https://codereview.chromium.org/730003002/diff/20001/Source/core/xml/Document...
Source/core/xml/DocumentXSLT.cpp:24: class DOMContentLoadedListener : public
V8AbstractEventListener {

Add final

https://codereview.chromium.org/730003002/diff/20001/Source/core/xml/Document...
Source/core/xml/DocumentXSLT.cpp:33: ASSERT(event->type() ==
"DOMContentLoaded");

Don't you need:

  if (!RuntimeEnabledFeatures::xsltEnabled())
    return;

?

https://codereview.chromium.org/730003002/diff/20001/Source/core/xml/Document...
Source/core/xml/DocumentXSLT.cpp:36: Document& document = *toDocument(context);

Don't you need:

  if (document.parsing())
    return;

?

https://codereview.chromium.org/730003002/diff/20001/Source/core/xml/Document...
Source/core/xml/DocumentXSLT.cpp:45: if (!pi || pi !=
m_processingInstruction.get() || pi->isLoading())

.get() won't be needed.

https://codereview.chromium.org/730003002/diff/20001/Source/core/xml/Document...
Source/core/xml/DocumentXSLT.cpp:87: return listener;

listener.release()

https://codereview.chromium.org/730003002/diff/20001/Source/core/xml/Document...
Source/core/xml/DocumentXSLT.cpp:149: return
static_cast<DocumentXSLT*>(DocumentSupplement::from(document,
supplementName()));

What happens if there is a DocumentXSLT instance but the instance does not (yet)
have m_transformSourceDocument?

https://codereview.chromium.org/730003002/diff/20001/Source/core/xml/Document...
File Source/core/xml/DocumentXSLT.h (right):

https://codereview.chromium.org/730003002/diff/20001/Source/core/xml/Document...
Source/core/xml/DocumentXSLT.h:23: PassRefPtrWillBeRawPtr<Document>
transformSourceDocument()

Does this need to be a PassRefPtr? Probably Document* ?

https://codereview.chromium.org/730003002/diff/20001/Source/core/xml/Document...
Source/core/xml/DocumentXSLT.h:37: // The following static methods doesn't use
any instance of DocumentXSLT.

doesn't => don't

https://codereview.chromium.org/730003002/diff/20001/Source/core/xml/Document...
Source/core/xml/DocumentXSLT.h:38: // Just they are just using DocumetnXSLT
namespace.

Just they are => they are

[tasak, 2014-11-19 09:23:04]

https://codereview.chromium.org/730003002/diff/20001/Source/core/dom/Processi...
File Source/core/dom/ProcessingInstruction.cpp (right):

https://codereview.chromium.org/730003002/diff/20001/Source/core/dom/Processi...
Source/core/dom/ProcessingInstruction.cpp:269: ASSERT(m_listenerForXSLT);
Should be ASSERT(m_listenerForXSLT || !RuntimeEnabledFeatures::xsltEnabled());

https://codereview.chromium.org/730003002/diff/20001/Source/core/dom/Processi...
Source/core/dom/ProcessingInstruction.cpp:284: } else if (m_isXSL) {
Should be (m_isXSL && m_listenerForXSLT)

https://codereview.chromium.org/730003002/diff/20001/Source/core/xml/Document...
File Source/core/xml/DocumentXSLT.cpp (right):

https://codereview.chromium.org/730003002/diff/20001/Source/core/xml/Document...
Source/core/xml/DocumentXSLT.cpp:24: class DOMContentLoadedListener : public
V8AbstractEventListener {
On 2014/11/19 09:01:36, haraken wrote:
> 
> Add final

Done.

https://codereview.chromium.org/730003002/diff/20001/Source/core/xml/Document...
Source/core/xml/DocumentXSLT.cpp:33: ASSERT(event->type() ==
"DOMContentLoaded");
On 2014/11/19 09:01:36, haraken wrote:
> 
> Don't you need:
> 
>   if (!RuntimeEnabledFeatures::xsltEnabled())
>     return;
> 
> ?

If disabled, we don't add this listener.

https://codereview.chromium.org/730003002/diff/20001/Source/core/xml/Document...
Source/core/xml/DocumentXSLT.cpp:36: Document& document = *toDocument(context);
On 2014/11/19 09:01:35, haraken wrote:
> 
> Don't you need:
> 
>   if (document.parsing())
>     return;
> 
> ?

I think, in DOMContentLoaded, document has been already parsed.

https://codereview.chromium.org/730003002/diff/20001/Source/core/xml/Document...
Source/core/xml/DocumentXSLT.cpp:45: if (!pi || pi !=
m_processingInstruction.get() || pi->isLoading())
On 2014/11/19 09:01:35, haraken wrote:
> 
> .get() won't be needed.

Done.

https://codereview.chromium.org/730003002/diff/20001/Source/core/xml/Document...
Source/core/xml/DocumentXSLT.cpp:87: return listener;
On 2014/11/19 09:01:36, haraken wrote:
> 
> listener.release()

Done.

https://codereview.chromium.org/730003002/diff/20001/Source/core/xml/Document...
Source/core/xml/DocumentXSLT.cpp:149: return
static_cast<DocumentXSLT*>(DocumentSupplement::from(document,
supplementName()));
On 2014/11/19 09:01:35, haraken wrote:
> 
> What happens if there is a DocumentXSLT instance but the instance does not
(yet)
> have m_transformSourceDocument?

Only XSLTProcessor uses DocumentXSLT::from and use setTransformSourceDocument,
i.e.

        if (oldDocument) {
           
DocumentXSLT::from(*result).setTransformSourceDocument(oldDocument.get());

https://codereview.chromium.org/730003002/diff/20001/Source/core/xml/Document...
File Source/core/xml/DocumentXSLT.h (right):

https://codereview.chromium.org/730003002/diff/20001/Source/core/xml/Document...
Source/core/xml/DocumentXSLT.h:23: PassRefPtrWillBeRawPtr<Document>
transformSourceDocument()
On 2014/11/19 09:01:36, haraken wrote:
> 
> Does this need to be a PassRefPtr? Probably Document* ?

This code is the same as the original code:
PassRefPtrWillBeRawPtr<Document> transformSourceDocument() { return
m_transformSourceDocument; }

https://codereview.chromium.org/730003002/diff/20001/Source/core/xml/Document...
Source/core/xml/DocumentXSLT.h:37: // The following static methods doesn't use
any instance of DocumentXSLT.
On 2014/11/19 09:01:36, haraken wrote:
> 
> doesn't => don't

Done.

https://codereview.chromium.org/730003002/diff/20001/Source/core/xml/Document...
Source/core/xml/DocumentXSLT.h:38: // Just they are just using DocumetnXSLT
namespace.
On 2014/11/19 09:01:36, haraken wrote:
> 
> Just they are => they are

Done.
Also fixed Documetn => Document

[haraken, 2014-11-19 09:33:59]

LGTM

https://codereview.chromium.org/730003002/diff/20001/Source/core/xml/Document...
File Source/core/xml/DocumentXSLT.cpp (right):

https://codereview.chromium.org/730003002/diff/20001/Source/core/xml/Document...
Source/core/xml/DocumentXSLT.cpp:33: ASSERT(event->type() ==
"DOMContentLoaded");
On 2014/11/19 09:23:03, tasak wrote:
> On 2014/11/19 09:01:36, haraken wrote:
> > 
> > Don't you need:
> > 
> >   if (!RuntimeEnabledFeatures::xsltEnabled())
> >     return;
> > 
> > ?
> 
> If disabled, we don't add this listener.

Can we add an ASSERT?

https://codereview.chromium.org/730003002/diff/20001/Source/core/xml/Document...
Source/core/xml/DocumentXSLT.cpp:36: Document& document = *toDocument(context);
On 2014/11/19 09:23:03, tasak wrote:
> On 2014/11/19 09:01:35, haraken wrote:
> > 
> > Don't you need:
> > 
> >   if (document.parsing())
> >     return;
> > 
> > ?
> 
> I think, in DOMContentLoaded, document has been already parsed.

Can we add an ASSERT?

https://codereview.chromium.org/730003002/diff/20001/Source/core/xml/Document...
File Source/core/xml/DocumentXSLT.h (right):

https://codereview.chromium.org/730003002/diff/20001/Source/core/xml/Document...
Source/core/xml/DocumentXSLT.h:23: PassRefPtrWillBeRawPtr<Document>
transformSourceDocument()
On 2014/11/19 09:23:03, tasak wrote:
> On 2014/11/19 09:01:36, haraken wrote:
> > 
> > Does this need to be a PassRefPtr? Probably Document* ?
> 
> This code is the same as the original code:
> PassRefPtrWillBeRawPtr<Document> transformSourceDocument() { return
> m_transformSourceDocument; }

ok, but this looks not nice. Given that transformSourceDocument() is not passing
an ownership of the document, it should be Document*.

[tasak, 2014-11-19 11:10:25]

Patchset #4 (id:60001) has been deleted

[tasak, 2014-11-19 11:10:35]

Patchset #4 (id:80001) has been deleted

[tasak, 2014-11-19 11:12:35]

Thank you for review.

I will check when RuntimeEnabledFeatures::xsltEnabled() == false.

https://codereview.chromium.org/730003002/diff/20001/Source/core/xml/Document...
File Source/core/xml/DocumentXSLT.cpp (right):

https://codereview.chromium.org/730003002/diff/20001/Source/core/xml/Document...
Source/core/xml/DocumentXSLT.cpp:33: ASSERT(event->type() ==
"DOMContentLoaded");
On 2014/11/19 09:33:59, haraken wrote:
> On 2014/11/19 09:23:03, tasak wrote:
> > On 2014/11/19 09:01:36, haraken wrote:
> > > 
> > > Don't you need:
> > > 
> > >   if (!RuntimeEnabledFeatures::xsltEnabled())
> > >     return;
> > > 
> > > ?
> > 
> > If disabled, we don't add this listener.
> 
> Can we add an ASSERT?

Done.

https://codereview.chromium.org/730003002/diff/20001/Source/core/xml/Document...
Source/core/xml/DocumentXSLT.cpp:36: Document& document = *toDocument(context);
On 2014/11/19 09:33:59, haraken wrote:
> On 2014/11/19 09:23:03, tasak wrote:
> > On 2014/11/19 09:01:35, haraken wrote:
> > > 
> > > Don't you need:
> > > 
> > >   if (document.parsing())
> > >     return;
> > > 
> > > ?
> > 
> > I think, in DOMContentLoaded, document has been already parsed.
> 
> Can we add an ASSERT?

Done.

https://codereview.chromium.org/730003002/diff/20001/Source/core/xml/Document...
File Source/core/xml/DocumentXSLT.h (right):

https://codereview.chromium.org/730003002/diff/20001/Source/core/xml/Document...
Source/core/xml/DocumentXSLT.h:23: PassRefPtrWillBeRawPtr<Document>
transformSourceDocument()
On 2014/11/19 09:33:59, haraken wrote:
> On 2014/11/19 09:23:03, tasak wrote:
> > On 2014/11/19 09:01:36, haraken wrote:
> > > 
> > > Does this need to be a PassRefPtr? Probably Document* ?
> > 
> > This code is the same as the original code:
> > PassRefPtrWillBeRawPtr<Document> transformSourceDocument() { return
> > m_transformSourceDocument; }
> 
> ok, but this looks not nice. Given that transformSourceDocument() is not
passing
> an ownership of the document, it should be Document*.

Done.

[tasak, 2014-11-20 03:51:26]

The CQ bit was checked by tasak@google.com

[commit-bot: I haz the power, 2014-11-20 03:51:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/730003002/120001

[commit-bot: I haz the power, 2014-11-20 04:58:44]

Committed patchset #5 (id:120001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=185643