531259 - The SVG <script> element should respect the aFromParser flag on NS_NewElement()

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Henri Sivonen (:hsivonen)]

The HTML <script> element sets mDoneAddingChildren to PR_FALSE upon creation with NS_NewElement() when aFromParser==PR_TRUE.

The SVG <script> element doesn't do this and instead requires the parses to call WillCallDoneAddingChildren on newly-created element nodes.

The SVG <script> element should observe the aFromParser flag the same way the HTML <script> element does.

Comment 1

[Robert Longson [:longsonr]]

Attachment: patch

Comment 2

[Robert Longson [:longsonr]]

Attachment: this time without the extra semicolon

Comment 3

[Robert Longson [:longsonr]]

(In reply to comment #2)
> Created an attachment (id=419545) [details]

Try server was much happier this time.

Comment 4

[Henri Sivonen (:hsivonen)]

Comment on attachment 419545 [details] [diff] [review]
this time without the extra semicolon

(I'm officially not qualified to be a reviewer.)

I think it would make sense to remove WillCallDoneAddingChildren() altogether.

This should be simple in the case of http://mxr-test.konigsberg.mozilla.org/mozilla-central/source/content/xml/document/src/nsXMLContentSink.cpp#528

Then the remaining call site would be http://mxr-test.konigsberg.mozilla.org/mozilla-central/source/content/xslt/src/xslt/txMozillaXMLOutput.cpp#555 , but it's not clear to me why http://mxr-test.konigsberg.mozilla.org/mozilla-central/source/content/xslt/src/xslt/txMozillaXMLOutput.cpp#542 passes PR_FALSE as the last argument.

Comment 5

[Henri Sivonen (:hsivonen)]

Comment on attachment 419545 [details] [diff] [review]
this time without the extra semicolon

Oops. I didn't mean to mark r+ due to the remaining concerns.

Comment 6

[Robert Longson [:longsonr]]

(In reply to comment #4)
> (From update of attachment 419545 [details] [diff] [review])
> (I'm officially not qualified to be a reviewer.)
> 

I know :-) but I need to know I'm doing what you wanted. I'll get an additional review from someone and probably an sr too.

> I think it would make sense to remove WillCallDoneAddingChildren() altogether.

OK. I'll post an updated patch soon.

> 
> Then the remaining call site would be
> http://mxr-test.konigsberg.mozilla.org/mozilla-central/source/content/xslt/src/xslt/txMozillaXMLOutput.cpp#555

What about 
http://mxr-test.konigsberg.mozilla.org/mozilla-central/source/content/xslt/src/xslt/txMozillaXMLOutput.cpp#751

Is that just an error because html script elements don't need this call?

> , but it's not clear to me why
> http://mxr-test.konigsberg.mozilla.org/mozilla-central/source/content/xslt/src/xslt/txMozillaXMLOutput.cpp#542
> passes PR_FALSE as the last argument.

hmm. The only place it's true is nsXMLContentSink.cpp which seems to be the intent as it indicates that it's coming from the parser.

Comment 7

[Robert Longson [:longsonr]]

Attachment: address review comments

Comment 8

[Henri Sivonen (:hsivonen)]

Comment on attachment 420315 [details] [diff] [review]
address review comments

(In reply to comment #6)
> What about 
> http://mxr-test.konigsberg.mozilla.org/mozilla-central/source/content/xslt/src/xslt/txMozillaXMLOutput.cpp#751

I believe it's a bug that XSLT-created elements don't get aFromParser=PR_TRUE, but's that's probably best treated as a separate bug since this patch makes SVG script no worse off than XHTML script in XSLT output.

r=hsivonen.

Comment 9

[Johnny Stenback (:jst)]

Comment on attachment 420315 [details] [diff] [review]
address review comments

+#define NS_IMPL_NS_NEW_SVG_ELEMENT_CHECK_PARSER(_elementName) \
+nsresult                                                      \
+NS_NewSVG##_elementName##Element(nsIContent **aResult,        \
+                                 nsINodeInfo *aNodeInfo,      \
+                                 PRBool aFromParser)          \
+{                                                             \
+  nsSVG##_elementName##Element *it =                          \
+    new nsSVG##_elementName##Element(aNodeInfo, aFromParser); \
+  if (!it)                                                    \
+    return NS_ERROR_OUT_OF_MEMORY;                            \
+                                                              \
+  NS_ADDREF(it);                                              \
+                                                              \
+  nsresult rv = it->Init();                                   \
+                                                              \
+  if (NS_FAILED(rv)) {                                        \
+    NS_RELEASE(it);                                           \
+    return rv;                                                \
+  }                                                           \
+                                                              \
+  *aResult = it;                                              \
+                                                              \
+  return rv;                                                  \
+}

You don't at all have to, but you could make that more readable (IMO at least) and less error prone if you used nsRefPtr, i.e. something like this:

+  nsRefPtr<nsSVG##_elementName##Element> it =                 \
+    new nsSVG##_elementName##Element(aNodeInfo, aFromParser); \
+  if (!it)                                                    \
+    return NS_ERROR_OUT_OF_MEMORY;                            \
+                                                              \
+  nsresult rv = it->Init();                                   \
+                                                              \
+  if (NS_FAILED(rv)) {                                        \
+    return rv;                                                \
+  }                                                           \
+                                                              \
+  *aResult = it.forget().get();                               \
+                                                              \
+  return rv;                                                  \

r+sr=jst either way.

Comment 10

[Robert Longson [:longsonr]]

Attachment: update to tip and address jst's comment

Comment 11

[Robert Longson [:longsonr]]

pushed http://hg.mozilla.org/mozilla-central/rev/f66f5b9a7aaf