816410 - Convert XMLSerializer and DOMParser to WebIDL bindings

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Just remember that addons may use XMLSerializer and DOMParser, so we probably will have to
keep the old interfaces there too.

Comment 2

[Masatoshi Kimura [:emk]]

Attachment: Part 1: Convert XMLSerializer and DOMParser to WebIDL bindings

Comment 3

[Masatoshi Kimura [:emk]]

https://tbpl.mozilla.org/?tree=Try&rev=0271eabcaf0a

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 687352 [details] [diff] [review]
Part 1: Convert XMLSerializer and DOMParser to WebIDL bindings

Doesn't this disappear the parserFromBuffer/parseFromStream methods from DOMParser? I'd prefer that we just leave them, [ChromeOnly]....  I guess the separate constructor for Constructor() is an attempt to make sure only "new DOMParser" uses the new bindings, but the _prototype_ will be the same in either case and not have the methods on it, no?  Or am I missing something?

Similar for serializeToStream on XMLSerializer.

Apart from that, this looks great!

Comment 5

[Masatoshi Kimura [:emk]]

Attachment: Part 2: Add Mozilla-specifinc stuff

Comment 6

[Masatoshi Kimura [:emk]]

Attachment: Part 3: Remove legacy constructor function

It is no longer callable because it was superseded by the WebIDL constructor, AIUI

Comment 7

[Masatoshi Kimura [:emk]]

Attachment: Part 2: Add Mozilla-specifinc stuff

Wrong patch attached :(

Comment 8

[Masatoshi Kimura [:emk]]

https://tbpl.mozilla.org/?tree=Try&rev=3256589d2204

Comment 9

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 687378 [details] [diff] [review]
Part 2: Add Mozilla-specifinc stuff

Review of attachment 687378 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/nsDOMSerializer.cpp
@@ +125,5 @@
> +nsDOMSerializer::SerializeToStream(nsINode& aRoot, nsIOutputStream* aStream,
> +                                   const nsAString& aCharset, ErrorResult& rv)
> +{
> +  nsCOMPtr<nsIDOMNode> domRoot(do_QueryInterface(&aRoot));
> +  rv = nsDOMSerializer::SerializeToStream(domRoot, aStream,

aRoot.AsDOMNode()?

Comment 10

[Masatoshi Kimura [:emk]]

Attachment: Part 1: Convert XMLSerializer and DOMParser to WebIDL bindings

Using AsDOMNode()

Comment 11

[Masatoshi Kimura [:emk]]

Attachment: Part 2: Add Mozilla-specifinc stuff

ditto

Comment 12

[Masatoshi Kimura [:emk]]

https://tbpl.mozilla.org/?tree=Try&rev=51b4565ad8ad

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 687377 [details] [diff] [review]
Part 3: Remove legacy constructor function

r=me

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 687392 [details] [diff] [review]
Part 1: Convert XMLSerializer and DOMParser to WebIDL bindings

r=me if you also mark parseFromString as [Creator].  It always returns a new document, right?

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 687393 [details] [diff] [review]
Part 2: Add Mozilla-specifinc stuff

Note that you might want to fold this into a single changeset as part 1, so there isn't a changeset in which things don't work quite right...

Thinking about this more, by the way, I'm a bit surprised that part 1 on its own passed tests; any use of |new DOMParser()| in a chrome window followed by parseFromStream/parseFromBuffer should have failed with it.  Do we not have test coverage for that?  Apparently not.  :(  In any case, I would bet money that extensions use them.  Would you be willing to add some tests here?  If not, at least file a followup on that?

>Bug 816410 - Part 2: Add Mozilla-specifinc stuff

s/specifinc/specific/

>+already_AddRefed<nsIDocument>
>+nsDOMParser::ParseFromBuffer(const nsAString& aBuf, SupportedType aType,

This is odd.  The xpidl version of this takes 3 arguments, doesn't it?  And the first one is an array in JS.  I would have thought the right WebIDL signature here would be something like:

  // Throws if the passed-in length is greater than the actual sequence length
  [Throws, ChromeOnly, Creator]
  parseFromBuffer(sequence<octet> buf, unsigned long length, SupportedType type);

and check the lengths before calling the existing ParseFromBuffer (because right now XPConnect handles that check for us).  Again, I think our complete lack of test coverage here is not helping...

With this patch, can we SeIsDOMBinding() in the no-argument nsDOMParser constructor (which I presume is only called from createInstance at this point)?

>+++ b/dom/webidl/DOMParser.webidl
>+// the latter is Mozilla-specific

Yeah, I wish we had a way to flag that in the IDL.  :(  Ah, well.

>+  [Throws, ChromeOnly]

Also [Creator].  Same for parseeFromStream.

r- because I'd like to see a fix for the parseFromBuffer bit.

Comment 16

[Masatoshi Kimura [:emk]]

Attachment: Part 1: Convert XMLSerializer and DOMParser to WebIDL bindings

(In reply to Boris Zbarsky (:bz) from comment #15)
> Comment on attachment 687393 [details] [diff] [review]
> Part 2: Add Mozilla-specifinc stuff
> 
> Note that you might want to fold this into a single changeset as part 1, so
> there isn't a changeset in which things don't work quite right...

Folded two patches.

> Thinking about this more, by the way, I'm a bit surprised that part 1 on its
> own passed tests; any use of |new DOMParser()| in a chrome window followed
> by parseFromStream/parseFromBuffer should have failed with it.  Do we not
> have test coverage for that?  Apparently not.  :(  In any case, I would bet
> money that extensions use them.  Would you be willing to add some tests
> here?  If not, at least file a followup on that?

Added a mochitest. It found some bugs on the patch :)

> >Bug 816410 - Part 2: Add Mozilla-specifinc stuff
> 
> s/specifinc/specific/

This comment is no longer present because the second patch has been folded.

> >+already_AddRefed<nsIDocument>
> >+nsDOMParser::ParseFromBuffer(const nsAString& aBuf, SupportedType aType,
> 
> This is odd.  The xpidl version of this takes 3 arguments, doesn't it?  And
> the first one is an array in JS.  I would have thought the right WebIDL
> signature here would be something like:
> 
>   // Throws if the passed-in length is greater than the actual sequence
> length
>   [Throws, ChromeOnly, Creator]
>   parseFromBuffer(sequence<octet> buf, unsigned long length, SupportedType
> type);
> 
> and check the lengths before calling the existing ParseFromBuffer (because
> right now XPConnect handles that check for us).  Again, I think our complete
> lack of test coverage here is not helping...

I (mis)thought size_is annotation would do some magic. Anyway, DOMString was wrong. Fixed and added a testcase.

> With this patch, can we SeIsDOMBinding() in the no-argument nsDOMParser
> constructor (which I presume is only called from createInstance at this
> point)?

Done.

> >+++ b/dom/webidl/DOMParser.webidl
> >+// the latter is Mozilla-specific
> 
> Yeah, I wish we had a way to flag that in the IDL.  :(  Ah, well.

I had to make a test case todo :(

> >+  [Throws, ChromeOnly]
> 
> Also [Creator].  Same for parseeFromStream.

Done. 
By the way, I refered DOMImplementation.webidl when writing the patch.
Do createDocumentType, createDocument, createHTMLDocument require [Creator] as well? If so, I'll file a followup bug for them.

Comment 17

[Masatoshi Kimura [:emk]]

Attachment: Part 2: Remove legacy constructor functions

I cleaned-up a bit more while I'm here (make constructor functions static).

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 687578 [details] [diff] [review]
Part 1: Convert XMLSerializer and DOMParser to WebIDL bindings

> Do createDocumentType, createDocument, createHTMLDocument require [Creator] as
> well?

Few things _require_ [Creator].  But yes, it would be good to flag them with it.

r=me.  Thank you for writing the tests!

One followup we should get filed: the canonical implementation of ParseFromString should be the WebIDL one, because the actual implementation converts the PRUnichar* to a nsAString anyway, and as things are written right now passing a string with null to the WebIDL method would truncate at the null for no good reason....

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 687580 [details] [diff] [review]
Part 2: Remove legacy constructor functions

r=me

Comment 20

[Masatoshi Kimura [:emk]]

https://tbpl.mozilla.org/?tree=Try&rev=77bda6856d25

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/436999015159
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a751769149c

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/436999015159
https://hg.mozilla.org/mozilla-central/rev/7a751769149c

Comment 23

[Masatoshi Kimura [:emk]]

Updated:
https://developer.mozilla.org/en-US/docs/Firefox_20_for_developers
https://developer.mozilla.org/en-US/docs/XMLSerializer

https://developer.mozilla.org/en-US/docs/DOM/DOMParser
didn't need an update because it explained only about parseFromString from the start.

https://developer.mozilla.org/en-US/docs/nsIDOMParser
https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsIDOMSerializer
don't need an update because they are documents for add-on developers.

Comment 24

[:Gavin Sharp [email: [e-mail]]]

I also updated Firefox 20 for developers to mention the removal of the nsIDOMParserJS interface, given bug 825239:
https://developer.mozilla.org/en-US/docs/Firefox_20_for_developers$compare?to=344245&from=343893