Closed Bug 816410 Opened 12 years ago Closed 11 years ago

Convert XMLSerializer and DOMParser to WebIDL bindings

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: emk, Assigned: emk)

References

(Blocks 2 open bugs, )

Details

(Keywords: dev-doc-complete)

Attachments

(2 files, 6 obsolete files)

      No description provided.
Just remember that addons may use XMLSerializer and DOMParser, so we probably will have to
keep the old interfaces there too.
Depends on: 816487
Attachment #687352 - Flags: review?(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!
Attachment #687376 - Flags: review?(bzbarsky)
It is no longer callable because it was superseded by the WebIDL constructor, AIUI
Attachment #687377 - Flags: review?(bzbarsky)
Wrong patch attached :(
Attachment #687376 - Attachment is obsolete: true
Attachment #687376 - Flags: review?(bzbarsky)
Attachment #687378 - Flags: review?(bzbarsky)
Attachment #687352 - Attachment description: patch → Part 1: Convert XMLSerializer and DOMParser to WebIDL bindings
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()?
Using AsDOMNode()
Attachment #687352 - Attachment is obsolete: true
Attachment #687352 - Flags: review?(bzbarsky)
Attachment #687392 - Flags: review?(bzbarsky)
ditto
Attachment #687378 - Attachment is obsolete: true
Attachment #687378 - Flags: review?(bzbarsky)
Attachment #687393 - Flags: review?(bzbarsky)
Comment on attachment 687377 [details] [diff] [review]
Part 3: Remove legacy constructor function

r=me
Attachment #687377 - Flags: review?(bzbarsky) → review+
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?
Attachment #687392 - Flags: review?(bzbarsky) → review+
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.
Attachment #687393 - Flags: review?(bzbarsky) → review-
(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.
Attachment #687392 - Attachment is obsolete: true
Attachment #687393 - Attachment is obsolete: true
Attachment #687578 - Flags: review?(bzbarsky)
I cleaned-up a bit more while I'm here (make constructor functions static).
Attachment #687377 - Attachment is obsolete: true
Attachment #687580 - Flags: review?(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....
Attachment #687578 - Flags: review?(bzbarsky) → review+
Comment on attachment 687580 [details] [diff] [review]
Part 2: Remove legacy constructor functions

r=me
Attachment #687580 - Flags: review?(bzbarsky) → review+
Blocks: 817469
Assignee: nobody → VYV03354
Depends on: 817844
https://hg.mozilla.org/mozilla-central/rev/436999015159
https://hg.mozilla.org/mozilla-central/rev/7a751769149c
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Blocks: 814254
Keywords: dev-doc-needed
Depends on: 819588
Depends on: 820841
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
Depends on: 825239
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.