Last Comment Bug 816410 - Convert XMLSerializer and DOMParser to WebIDL bindings
: Convert XMLSerializer and DOMParser to WebIDL bindings
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla20
Assigned To: Masatoshi Kimura [:emk] (use Splinter to ask me for review, see bugzil.la/1321953)
:
: Andrew Overholt [:overholt]
Mentors:
http://domparsing.spec.whatwg.org/
Depends on: 816487 817844 819588 820841 825239
Blocks: ParisBindings domparsing 814254 817469
  Show dependency treegraph
 
Reported: 2012-11-28 21:32 PST by Masatoshi Kimura [:emk] (use Splinter to ask me for review, see bugzil.la/1321953)
Modified: 2013-02-08 15:01 PST (History)
6 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part 1: Convert XMLSerializer and DOMParser to WebIDL bindings (12.22 KB, patch)
2012-11-30 20:32 PST, Masatoshi Kimura [:emk] (use Splinter to ask me for review, see bugzil.la/1321953)
no flags Details | Diff | Splinter Review
Part 2: Add Mozilla-specifinc stuff (8.09 KB, patch)
2012-12-01 03:55 PST, Masatoshi Kimura [:emk] (use Splinter to ask me for review, see bugzil.la/1321953)
no flags Details | Diff | Splinter Review
Part 3: Remove legacy constructor function (8.09 KB, patch)
2012-12-01 03:57 PST, Masatoshi Kimura [:emk] (use Splinter to ask me for review, see bugzil.la/1321953)
bzbarsky: review+
Details | Diff | Splinter Review
Part 2: Add Mozilla-specifinc stuff (11.74 KB, patch)
2012-12-01 03:58 PST, Masatoshi Kimura [:emk] (use Splinter to ask me for review, see bugzil.la/1321953)
no flags Details | Diff | Splinter Review
Part 1: Convert XMLSerializer and DOMParser to WebIDL bindings (12.17 KB, patch)
2012-12-01 05:33 PST, Masatoshi Kimura [:emk] (use Splinter to ask me for review, see bugzil.la/1321953)
bzbarsky: review+
Details | Diff | Splinter Review
Part 2: Add Mozilla-specifinc stuff (11.69 KB, patch)
2012-12-01 05:34 PST, Masatoshi Kimura [:emk] (use Splinter to ask me for review, see bugzil.la/1321953)
bzbarsky: review-
Details | Diff | Splinter Review
Part 1: Convert XMLSerializer and DOMParser to WebIDL bindings (31.92 KB, patch)
2012-12-02 15:54 PST, Masatoshi Kimura [:emk] (use Splinter to ask me for review, see bugzil.la/1321953)
bzbarsky: review+
Details | Diff | Splinter Review
Part 2: Remove legacy constructor functions (16.89 KB, patch)
2012-12-02 15:55 PST, Masatoshi Kimura [:emk] (use Splinter to ask me for review, see bugzil.la/1321953)
bzbarsky: review+
Details | Diff | Splinter Review

Description Masatoshi Kimura [:emk] (use Splinter to ask me for review, see bugzil.la/1321953) 2012-11-28 21:32:21 PST

    
Comment 1 Olli Pettay [:smaug] 2012-11-29 04:42:48 PST
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] (use Splinter to ask me for review, see bugzil.la/1321953) 2012-11-30 20:32:12 PST
Created attachment 687352 [details] [diff] [review]
Part 1: Convert XMLSerializer and DOMParser to WebIDL bindings
Comment 3 Masatoshi Kimura [:emk] (use Splinter to ask me for review, see bugzil.la/1321953) 2012-11-30 20:32:29 PST
https://tbpl.mozilla.org/?tree=Try&rev=0271eabcaf0a
Comment 4 Boris Zbarsky [:bz] (still a bit busy) 2012-11-30 20:53:57 PST
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] (use Splinter to ask me for review, see bugzil.la/1321953) 2012-12-01 03:55:50 PST
Created attachment 687376 [details] [diff] [review]
Part 2: Add Mozilla-specifinc stuff
Comment 6 Masatoshi Kimura [:emk] (use Splinter to ask me for review, see bugzil.la/1321953) 2012-12-01 03:57:44 PST
Created attachment 687377 [details] [diff] [review]
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] (use Splinter to ask me for review, see bugzil.la/1321953) 2012-12-01 03:58:38 PST
Created attachment 687378 [details] [diff] [review]
Part 2: Add Mozilla-specifinc stuff

Wrong patch attached :(
Comment 8 Masatoshi Kimura [:emk] (use Splinter to ask me for review, see bugzil.la/1321953) 2012-12-01 04:01:57 PST
https://tbpl.mozilla.org/?tree=Try&rev=3256589d2204
Comment 9 :Ms2ger (⌚ UTC+1/+2) 2012-12-01 04:35:32 PST
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] (use Splinter to ask me for review, see bugzil.la/1321953) 2012-12-01 05:33:49 PST
Created attachment 687392 [details] [diff] [review]
Part 1: Convert XMLSerializer and DOMParser to WebIDL bindings

Using AsDOMNode()
Comment 11 Masatoshi Kimura [:emk] (use Splinter to ask me for review, see bugzil.la/1321953) 2012-12-01 05:34:28 PST
Created attachment 687393 [details] [diff] [review]
Part 2: Add Mozilla-specifinc stuff

ditto
Comment 12 Masatoshi Kimura [:emk] (use Splinter to ask me for review, see bugzil.la/1321953) 2012-12-01 07:17:16 PST
https://tbpl.mozilla.org/?tree=Try&rev=51b4565ad8ad
Comment 13 Boris Zbarsky [:bz] (still a bit busy) 2012-12-01 18:34:38 PST
Comment on attachment 687377 [details] [diff] [review]
Part 3: Remove legacy constructor function

r=me
Comment 14 Boris Zbarsky [:bz] (still a bit busy) 2012-12-01 18:38:40 PST
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 [:bz] (still a bit busy) 2012-12-01 19:02:46 PST
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] (use Splinter to ask me for review, see bugzil.la/1321953) 2012-12-02 15:54:02 PST
Created attachment 687578 [details] [diff] [review]
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] (use Splinter to ask me for review, see bugzil.la/1321953) 2012-12-02 15:55:43 PST
Created attachment 687580 [details] [diff] [review]
Part 2: Remove legacy constructor functions

I cleaned-up a bit more while I'm here (make constructor functions static).
Comment 18 Boris Zbarsky [:bz] (still a bit busy) 2012-12-02 17:59:59 PST
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 [:bz] (still a bit busy) 2012-12-02 18:01:50 PST
Comment on attachment 687580 [details] [diff] [review]
Part 2: Remove legacy constructor functions

r=me
Comment 20 Masatoshi Kimura [:emk] (use Splinter to ask me for review, see bugzil.la/1321953) 2012-12-02 19:06:02 PST
https://tbpl.mozilla.org/?tree=Try&rev=77bda6856d25
Comment 23 Masatoshi Kimura [:emk] (use Splinter to ask me for review, see bugzil.la/1321953) 2013-01-01 05:59:00 PST
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: gavin@gavinsharp.com] 2013-01-03 12:19:56 PST
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

Note You need to log in before you can comment on or make changes to this bug.