Closed Bug 531259 Opened 10 years ago Closed 10 years ago

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

Categories

(Core :: SVG, defect, trivial)

defect
Not set
trivial

Tracking

()

RESOLVED FIXED

People

(Reporter: hsivonen, Assigned: longsonr)

References

Details

Attachments

(1 file, 3 obsolete files)

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.
See Also: → 528442
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → longsonr
Attachment #419453 - Flags: review?(hsivonen)
Attachment #419453 - Attachment is obsolete: true
Attachment #419545 - Flags: review?(hsivonen)
Attachment #419453 - Flags: review?(hsivonen)
(In reply to comment #2)
> Created an attachment (id=419545) [details]

Try server was much happier this time.
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.
Attachment #419545 - Flags: review?(hsivonen) → review+
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.
Attachment #419545 - Flags: review+ → review?(hsivonen)
(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.
Attached patch address review comments (obsolete) — Splinter Review
Attachment #419545 - Attachment is obsolete: true
Attachment #420315 - Flags: review?(hsivonen)
Attachment #419545 - Flags: review?(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.
Attachment #420315 - Flags: review?(hsivonen) → review+
Attachment #420315 - Flags: superreview?(jst)
Attachment #420315 - Flags: review?(jst)
Attachment #420315 - Flags: superreview?(jst)
Attachment #420315 - Flags: superreview+
Attachment #420315 - Flags: review?(jst)
Attachment #420315 - Flags: review+
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.
Attachment #420315 - Attachment is obsolete: true
pushed http://hg.mozilla.org/mozilla-central/rev/f66f5b9a7aaf
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.