819239 - WebIDL API for HTMLDocument

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

We can sort out the collapsing the spec wants later, but for now I'd rather minimize the behavior changes and have the separate interface object.

Comment 1

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

One question: will this cause us to remove the nsIDOMDocument members from HTMLDocument.prototype?

Comment 2

[Boris Zbarsky [:bzbarsky]]

This bug won't do anything to the protos; it's just rearranging the C++.

There will probably be a second bug that will switch to the WebIDL HTMLDocument methods but change nothing about the set of methods on HTMLDocument.prototype.

Actually removing the nsIDOMDocument members from HTMLDocument.prototype means using a WebIDL prototype for HTMLDocument, which at the moment requires using a WebIDL instance object for HTMLDocument, which needs the name getter sorted out.  That'll be a third bug...

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Add the WebIDL API for HTMLDocument.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Add the WebIDL API for HTMLDocument.

Comment 5

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

Comment on attachment 690063 [details] [diff] [review]
Add the WebIDL API for HTMLDocument.

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

::: content/html/document/src/nsHTMLDocument.cpp
@@ +983,5 @@
> +  if (aDomain.IsEmpty()) {
> +    rv.Throw(NS_ERROR_DOM_BAD_DOCUMENT_DOMAIN);
> +    return;
> +  }
> +      

Whitespace

@@ +1067,5 @@
>    // outer most frameset element
>    nsRefPtr<nsContentList> nodeList =
>      NS_GetContentList(this, kNameSpaceID_XHTML, NS_LITERAL_STRING("frameset"));
>  
> +  return static_cast<nsGenericHTMLElement*>(nodeList->GetElementAt(0));

Some IsHTML() assertions would be nice here

@@ +1259,5 @@
>  {
>    if (!mScripts) {
>      mScripts = new nsContentList(this, kNameSpaceID_XHTML, nsGkAtoms::script, nsGkAtoms::script);
>    }
> +  return mScripts;  

Whitespace

@@ +1923,5 @@
> +void
> +nsHTMLDocument::Writeln(JSContext* cx, const Sequence<nsString>& aText,
> +                        ErrorResult& rv)
> +{
> +  return WriteCommon(cx, aText, true, rv);

No need for the returns

@@ +1962,4 @@
>    nsRefPtr<nsContentList> list = GetElementsByName(aElementName);
> +  if (!list) {
> +    rv.Throw(NS_ERROR_OUT_OF_MEMORY);
> +  }

You're not calling this, but you don't need to. We'll already have crashed if list is null here.

@@ +2048,5 @@
>      NS_GetFuncStringNodeList(this, MatchItems, DestroyTokens, CreateTokens,
> +                             aTypeNames);
> +  if (!elements) {
> +    rv.Throw(NS_ERROR_OUT_OF_MEMORY);
> +  }

This can't fail either.

@@ +2232,5 @@
> +  }
> +
> +  nsCOMPtr<nsISelection> sel;
> +  rv = window->GetSelection(getter_AddRefs(sel));
> +  return sel.forget();  

Whitespace

@@ +3741,2 @@
>    // return "" in that case anyway (bug 738385), so we just return NS_OK
>    // regardless.

No more NS_OK

::: content/html/document/src/nsHTMLDocument.h
@@ +215,5 @@
> +             mozilla::ErrorResult& rv);
> +  void Writeln(JSContext* cx, const mozilla::dom::Sequence<nsString>& aText,
> +               mozilla::ErrorResult& rv);
> +  // The XPCOM GetDesignMode() works OK for us, since it never throws.
> +  // XXXbz But really, designMode should be a WebIDL enum.

Case-insensitive enums? Maybe that would actually be a good idea...

Comment 6

[Boris Zbarsky [:bzbarsky]]

Fixed the whitespace nits.  

Hadn't realized NS_GetFuncString* is now infallible. Great!

I'd missed the case-insensitive thing.  Will take out the XXX comment for now.

Also added a GetLocation because otherwise we shadow the one on nsIDocument and my implementation of inherited unforgeables unwraps to most-derived interface.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: With the comment 5 comments addressed

Comment 8

[Peter Van der Beken [:peterv]]

Comment on attachment 691998 [details] [diff] [review]
With the comment 5 comments addressed

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

::: content/html/document/src/nsHTMLDocument.cpp
@@ +1792,5 @@
> +    rv = WriteCommon(cx, aText[0], aNewlineTerminate);
> +  } else {
> +    // XXXbz it would be nice if we could pass all the strings to the parser
> +    // without having to do all this copying and then ask it to start
> +    // parsing....

Want to file a bug?

Comment 9

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f870d9cd5af1

I'll file a bug on the parser thing.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Filed bug 824669.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f870d9cd5af1