[webkit-reviews] review denied: [Bug 41348] Remove global variables from XSLTProcessorLibxslt.cpp : [Attachment 60362] New proposed patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Andreas Wictor
<andreas.wictor at xcerion.com>'s request for review:
Bug 41348: Remove global variables from XSLTProcessorLibxslt.cpp
https://bugs.webkit.org/show_bug.cgi?id=41348

Attachment 60362: New proposed patch
https://bugs.webkit.org/attachment.cgi?id=60362&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
+static HashSet<XSLTProcessor*> globalXSLTProcessors;
+static HashSet<XSLStyleSheet*> globalXSLStyleSheets;

I don't think this will even build on Mac - there is a step in build script
that verifies that there are no global destructors. Our usual pattern is to
have a static function with a DEFINE_STATIC_GLOBAL macro inside.

+	 xsltTransformContextPtr context =
reinterpret_cast<xsltTransformContextPtr>(ctxt);
+	 XSLTProcessor* processor =
reinterpret_cast<XSLTProcessor*>(context->_private);

Didn't static_cast work?

+		 fprintf(stderr, "Expected a registred XSLTProcessor pointer in
the libxslt "
+			 "_private field but found something else: %p\n",
context->_private);

This is not a message that can help the author of an application using WebKit
much. We need to give them pointers they can use to work around the problem.

Something like "WebKit XSLT document loader was called with unknown
transformation context." might be more helpful.

Looking at just this code, it seems that it can be simplified a little by
storing transformation contexts instead of processors/stylesheets. But it's
difficult to get to the context from where _private is set. Maybe it can be
structured differently, although I don't have a concrete solution.

+    Node* m_sourceNode;

Ideally, this would have a comment explaining why this can't become a dangling
pointer.

r- because of HashSet destructor issue.