Closed Bug 819239 Opened 7 years ago Closed 7 years ago

WebIDL API for HTMLDocument

Categories

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

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

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.
One question: will this cause us to remove the nsIDOMDocument members from HTMLDocument.prototype?
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...
Blocks: 819624
Whiteboard: [need review]
Attachment #690059 - Flags: review?(peterv)
Attachment #690063 - Flags: review?(peterv)
Attachment #690059 - Attachment is obsolete: true
Attachment #690059 - Flags: review?(peterv)
Depends on: 821438
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...
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.
Attachment #690063 - Attachment is obsolete: true
Attachment #690063 - Flags: review?(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?
Attachment #691998 - Flags: review?(peterv) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/f870d9cd5af1

I'll file a bug on the parser thing.
Flags: in-testsuite-
Whiteboard: [need review]
Target Milestone: --- → mozilla20
https://hg.mozilla.org/mozilla-central/rev/f870d9cd5af1
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Depends on: CVE-2013-1704
Depends on: CVE-2016-1961
Component: DOM → DOM: Core & HTML
Blocks: 755679
You need to log in before you can comment on or make changes to this bug.