Closed Bug 807075 Opened 7 years ago Closed 7 years ago

New DOM binding APIs for HTMLElement

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla19

People

(Reporter: peterv, Assigned: peterv)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(1 file)

Attached patch v1Splinter Review
No description provided.
Attachment #676734 - Flags: review?(bzbarsky)
Comment on attachment 676734 [details] [diff] [review]
v1

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

::: content/html/content/src/nsHTMLObjectElement.cpp
@@ +310,5 @@
>    }
>  
> +  return IsEditableRoot() ||
> +         (Type() == eType_Document &&
> +          nsContentUtils::IsSubDocumentTabbable(const_cast<nsHTMLObjectElement*>(this)));

I don't think we should make TabIndex() const because of this part

::: content/html/content/src/nsHTMLUnknownElement.h
@@ +8,5 @@
> +#include "nsGenericHTMLElement.h"
> +#include "nsIDOMHTMLUnknownElement.h"
> +
> +class nsHTMLUnknownElement : public nsGenericHTMLElement
> +                           , public nsIDOMHTMLUnknownElement

You don't actually include this in nsHTMLUnknownElement.cpp
Comment on attachment 676734 [details] [diff] [review]
v1

>+++ b/content/html/content/src/nsGenericHTMLElement.cpp
>-NS_IMPL_BOOL_ATTR(nsGenericHTMLElement, Hidden, hidden)

I lightly wonder whether it's worth having macros for the new stuff too.  Maybe it's not.

>-nsGenericHTMLElement::GetClassName(nsAString& aClassName)

It's worth documenting in the header where you declare all the WebIDL stuff that we have a className getter/setter returning nsresult that always return null, so we can just use those.

>@@ -545,80 +473,18 @@ nsGenericHTMLElement::GetOffsetRect(nsRe
>+  return static_cast<nsGenericElement*>(offsetParent);

If we can't easily make offsetParent an nsGenericElement, can we at least make this a cast from offsetParent->AsElement() so it will assert if somehow it's not an element?  Once we merge Element and nsGenericElement, we can then get rid of the cast.

>+nsGenericHTMLElement::SetItemValue(JSContext* aCx, JS::Value aValue,

Hrm.  We should consider seeing if a union would work here, after we implement union return values.

>+    // XXX How do we signal failure here?

By throwing something on aError.  The codegen will then not throw anything of its own if there's an exception on the JSContext already.

>+++ b/content/html/content/src/nsHTMLSelectElement.cpp
>+    nsGenericHTMLElement::AppendChild(*aElement, aError);

aElement could be null there, as far as I can tell.  We need to handle that.

>+  while (ancestor != this) {

nsContentUtils::ContentIsDescendantOf?

>+  parent->InsertBefore(*aElement, aBefore, aError);

Again, aElement can be null.

> nsHTMLSelectElement::Add(nsIDOMHTMLElement* aElement,
>+  nsGenericHTMLElement* htmlElement =
>+    nsGenericHTMLElement::FromContentOrNull(element);

This is where I think we should null-check and then throw if null or whatever the right behavior is.

>+++ b/content/html/content/src/nsHTMLSelectElement.h
> nsHTMLOptionCollection::Add(const HTMLOptionOrOptGroupElement& aElement,
>+    nsCOMPtr<nsIContent> content =
>+      do_QueryInterface(aBefore.Value().GetAsHTMLElement());

This is needed because the codegen still maps HTMLElement to nsIDOMHTMLElement?

Please file a followup on making this better and mark it dependend on the bug for flipping on the HTMLElement binding?

>+++ b/content/html/content/src/nsHTMLUnknownElement.cpp

Like ms2ger said, you're not using the header here...

>+++ b/dom/webidl/HTMLElement.webidl
>+  // This should move to Element
>+           attribute DOMString className;

Please file a bug, reference it here?

>+  // These should move to Element
>+  attribute DOMString innerHTML;
>+  attribute DOMString outerHTML;
>+  void insertAdjacentHTML(DOMString position, DOMString text);

Likewise.

r=me with those nits.
Attachment #676734 - Flags: review?(bzbarsky) → review+
(In reply to Ed Morley [:edmorley UTC+0] from comment #4)
> Also browser-chrome assertions:
> https://tbpl.mozilla.org/php/getParsedLog.php?id=17032080&tree=Mozilla-
> Inbound

Actually these assertions may be unrelated (also occurring on another tree).
https://hg.mozilla.org/try/rev/ca4e345c7ad4
Target Milestone: --- → mozilla19
https://hg.mozilla.org/mozilla-central/rev/da5761fc30b1
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Depends on: 812621
Blocks: 865147
No longer blocks: 865147
Depends on: 865147
Blocks: 865147
No longer depends on: 865147
No longer blocks: 865147
Depends on: 865147
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.