Added addXSLStyleSheet and removeXSLStyleSheet to StyleEngine.

Added addXSLStyleSheet and removeXSLStyleSheet to StyleEngine.

To apply XSLStyleSheet, we only need the first one. We don't need
to have all XSL stylesheets if the XML document has more than two
XSL stylesheets. So modified not to add XSLStyleSheet to
StyleSheetCandidateNodes. Instead, modified to make StyleEngine have
the first XSLStyleSheet.

BUG=367689
TEST=No new test, because this patch is just for refactoring.

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

[tasak, 2014-05-01 08:26:57]

Would you review this CL?

[esprehn, 2014-05-22 18:32:19]

https://codereview.chromium.org/257363002/diff/30001/Source/core/dom/StyleEng...
File Source/core/dom/StyleEngine.cpp (right):

https://codereview.chromium.org/257363002/diff/30001/Source/core/dom/StyleEng...
Source/core/dom/StyleEngine.cpp:318: void StyleEngine::addXSLStyleSheet(Node*
node, bool createdByParser)
This should just take a ProcessingInstruction*

https://codereview.chromium.org/257363002/diff/30001/Source/core/dom/StyleEng...
Source/core/dom/StyleEngine.cpp:339: void StyleEngine::removeXSLStyleSheet(Node*
node)
ditto

https://codereview.chromium.org/257363002/diff/30001/Source/core/dom/StyleEng...
Source/core/dom/StyleEngine.cpp:526: return m_xslStyleSheet &&
RuntimeEnabledFeatures::xsltEnabled() && !m_document.transformSourceDocument();
Early return on the runtime enabled feature.

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

https://codereview.chromium.org/257363002/diff/30001/Source/core/dom/StyleEng...
File Source/core/dom/StyleEngine.h (right):

https://codereview.chromium.org/257363002/diff/30001/Source/core/dom/StyleEng...
Source/core/dom/StyleEngine.h:257: ProcessingInstruction* m_xslStyleSheet;
This looks really scary, can this be a RefPtr instead?

[tasak, 2014-05-27 06:17:29]

Thank you for reviewing.

https://codereview.chromium.org/257363002/diff/30001/Source/core/dom/StyleEng...
File Source/core/dom/StyleEngine.cpp (right):

https://codereview.chromium.org/257363002/diff/30001/Source/core/dom/StyleEng...
Source/core/dom/StyleEngine.cpp:318: void StyleEngine::addXSLStyleSheet(Node*
node, bool createdByParser)
On 2014/05/22 18:32:20, esprehn wrote:
> This should just take a ProcessingInstruction*

Done.

https://codereview.chromium.org/257363002/diff/30001/Source/core/dom/StyleEng...
Source/core/dom/StyleEngine.cpp:339: void StyleEngine::removeXSLStyleSheet(Node*
node)
On 2014/05/22 18:32:20, esprehn wrote:
> ditto

Done.

https://codereview.chromium.org/257363002/diff/30001/Source/core/dom/StyleEng...
Source/core/dom/StyleEngine.cpp:526: return m_xslStyleSheet &&
RuntimeEnabledFeatures::xsltEnabled() && !m_document.transformSourceDocument();
On 2014/05/22 18:32:20, esprehn wrote:
> Early return on the runtime enabled feature.
> 
> if (RuntimeEnabledFeatures::xsltEnabled())
>   return false;

Done.

https://codereview.chromium.org/257363002/diff/30001/Source/core/dom/StyleEng...
File Source/core/dom/StyleEngine.h (right):

https://codereview.chromium.org/257363002/diff/30001/Source/core/dom/StyleEng...
Source/core/dom/StyleEngine.h:257: ProcessingInstruction* m_xslStyleSheet;
On 2014/05/22 18:32:20, esprehn wrote:
> This looks really scary, can this be a RefPtr instead?

Done.

[esprehn, 2014-06-06 01:32:55]

lgtm, that destructor code makes no sense at all though, you're never
inDocument() when your destructor runs. This is okay since it was already there
I suppose.

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

https://codereview.chromium.org/257363002/diff/70001/Source/core/dom/Processi...
Source/core/dom/ProcessingInstruction.cpp:62: if (inDocument()) {
This does doesn't make any sense, you can't be inDocument() if your destructor
is running. I don't understand the old code either.

https://codereview.chromium.org/257363002/diff/70001/Source/core/dom/Processi...
File Source/core/dom/ProcessingInstruction.h (right):

https://codereview.chromium.org/257363002/diff/70001/Source/core/dom/Processi...
Source/core/dom/ProcessingInstruction.h:89: inline bool isXSLStyleSheet(const
Node* node)
This should take a reference.

[tasak, 2014-06-09 04:04:57]

Thank you for reviewing.

I added FIXME comment to ~ProcessingInstruction.

I think, we need to fix somewhere (I have not finished investigating yet) to
remove inDocument() code from ~ProcessingInstruction.

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

https://codereview.chromium.org/257363002/diff/70001/Source/core/dom/Processi...
Source/core/dom/ProcessingInstruction.cpp:62: if (inDocument()) {
On 2014/06/06 01:32:55, esprehn wrote:
> This does doesn't make any sense, you can't be inDocument() if your destructor
> is running. I don't understand the old code either.

I would like to attach crash log when I added ASSERT(!inDocument()); here:

STDOUT: #CRASHED - renderer (pid 19995)
STDERR: ASSERTION FAILED: !inDocument()
STDERR: ../../third_party/WebKit/Source/core/dom/ProcessingInstruction.cpp(62) :
virtual WebCore::ProcessingInstruction::~ProcessingInstruction()

STDERR: #3 0x000002143103
WebCore::ProcessingInstruction::~ProcessingInstruction()
STDERR: #4 0x000002143279
WebCore::ProcessingInstruction::~ProcessingInstruction()
STDERR: #5 0x000002063835 WebCore::removeDetachedChildrenInContainer<>()
STDERR: #6 0x00000205d7bb WebCore::ContainerNode::removeDetachedChildren()
STDERR: #7 0x00000207665c WebCore::Document::dispose()
STDERR: #8 0x00000207685c WebCore::Document::dispose()
STDERR: #9 0x000002129af4 WebCore::TreeScope::removedLastRefToScope()
STDERR: #10 0x000002125da7 WebCore::Node::removedLastRef()
STDERR: #11 0x000001f90647 WebCore::TreeShared<>::deref()
STDERR: #12 0x000001f9041d WTF::derefIfNotNull<>()
STDERR: #13 0x000001f94238 WTF::RefPtr<>::~RefPtr()
STDERR: #14 0x0000029fa7ae WebCore::XMLHttpRequest::~XMLHttpRequest()
STDERR: #15 0x0000029fa979 WebCore::XMLHttpRequest::~XMLHttpRequest()
STDERR: #16 0x0000029029d3 WTF::RefCounted<>::deref()
STDERR: #17 0x00000310fb63 WebCore::V8XMLHttpRequest::derefObject()
STDERR: #18 0x000002fa3db8 WebCore::releaseObject()
STDERR: #19 0x000002fb4a89 WebCore::ScriptWrappable::setWeakCallback()
STDERR: #20 0x000001b7b172
v8::internal::GlobalHandles::Node::PostGarbageCollectionProcessing()
STDERR: #21 0x000001b7abfc
v8::internal::GlobalHandles::PostGarbageCollectionProcessing()
STDERR: #22 0x000001ba32df v8::internal::Heap::PerformGarbageCollection()
STDERR: #23 0x000001ba274e v8::internal::Heap::CollectGarbage()
STDERR: #24 0x000001ba224e v8::internal::Heap::CollectAllGarbage()

I think, probably ProcessingInstruction and XSLT are not maintained correctly.
So currently, we will see inDocument() in ~ProcessingInstruction.

I added FIXME.

https://codereview.chromium.org/257363002/diff/70001/Source/core/dom/Processi...
File Source/core/dom/ProcessingInstruction.h (right):

https://codereview.chromium.org/257363002/diff/70001/Source/core/dom/Processi...
Source/core/dom/ProcessingInstruction.h:89: inline bool isXSLStyleSheet(const
Node* node)
On 2014/06/06 01:32:55, esprehn wrote:
> This should take a reference.

Done.

[tasak, 2014-06-09 04:05:00]

The CQ bit was checked by tasak@google.com

[commit-bot: I haz the power, 2014-06-09 04:06:00]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tasak@google.com/257363002/100006

[commit-bot: I haz the power, 2014-06-09 05:11:49]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/11216)

[commit-bot: I haz the power, 2014-06-09 05:47:28]

Change committed as 175775