795610 - Kill NS_FORWARD_NSIDOMHTMLELEMENT_BASIC

Status: RESOLVED FIXED

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

Description

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

Attachment: Part a: Outparamdel GetBoolAttr/GetIntAttr

This setup doesn't really work for the WebIDL bindings. Instead, we should have everyone forward the XPIDL methods to nsGenericHTMLElement, which calls the WebIDL methods, and have subclasses override the WebIDL methods where necessary.

Patches are more or less loosely based on Peter's queue.

Comment 1

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

Attachment: Part b: tabIndex

I went for a virtual TabIndexDefault() instead of overriding TabIndex() to avoid duplicating code too much.

Comment 2

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

Attachment: Part c: draggable

Comment 3

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

Attachment: Part d: click()

Comment 4

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

Attachment: Part e: focus()

Comment 5

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

Attachment: Part f: innerHTML

Comment 6

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

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

Comment 7

[Mounir Lamouri (:mounir)]

Comment on attachment 666217 [details] [diff] [review]
Part c: draggable

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

::: content/html/content/src/nsHTMLAnchorElement.cpp
@@ +183,4 @@
>    }
>  
>    // no href, so just use the same behavior as other elements
> +  return nsGenericHTMLElement::Draggable();

FWIW, I would have done:

if (!HasAttr(kNameSpaceID_None, nsGkAtoms::href)) {
  return nsGenericHTMLElement::Draggable();
}

return !AttrValueIs(kNameSpaceID_None, nsGkAtoms::draggable, nsGkAtoms::_false, eIgnoreCase);

Comment 8

[Mounir Lamouri (:mounir)]

Comment on attachment 666218 [details] [diff] [review]
Part d: click()

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

r=me with s/MozClick/DOMClick/g

::: dom/interfaces/html/nsIDOMHTMLElement.idl
@@ +48,5 @@
>     *
>     * See <http://www.whatwg.org/html5/#the-hidden-attribute>.
>     */
>             attribute boolean          hidden;
> +  [binaryname(MozClick)]

I would prefer naming this DOMClick(). MozClick() gives the impression that it's a mozilla extensions while DOMClick() makes it clear it's the DOM API click() method IMO.

Comment 9

[Mounir Lamouri (:mounir)]

Comment on attachment 666219 [details] [diff] [review]
Part e: focus()

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

r=me with s/MozFocus/DOMFocus/g and other comments applied.

::: content/html/content/src/nsGenericHTMLElement.cpp
@@ +202,5 @@
>      if (!fm->GetFocusedContent() ||
>          fm->GetFocusedContent()->OwnerDoc() != document) {
> +      mozilla::ErrorResult rv;
> +      mElement->Focus(rv);
> +      return rv.ErrorCode();

Can't you do:
return MozFocus();

::: content/html/content/src/nsHTMLLegendElement.cpp
@@ +172,5 @@
>                                        bool aIsTrustedEvent)
>  {
>    // just use the same behaviour as the focus method
> +  mozilla::ErrorResult rv;
> +  Focus(rv);

I would prefer:
Focus(mozilla::ErrorResult());

Comment 10

[Mounir Lamouri (:mounir)]

Comment on attachment 666220 [details] [diff] [review]
Part f: innerHTML

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

r=me with s/MozInnerHTML/DOMInnerHTML/g

::: content/html/content/src/nsGenericHTMLElement.cpp
@@ +1345,5 @@
>    }
>    mb.RemovalDone();
>  
>    nsAutoScriptLoaderDisabler sld(doc);
>    

As long as you are here, could you remove those two whitespaces?

::: content/html/content/src/nsHTMLStyleElement.cpp
@@ +270,3 @@
>    
>    SetEnableUpdates(true);
>    

As long as you are here, could you remove the two whitespaces on the lines before and after SetEnableUpdates(true);

Comment 11

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

Attachment: Part g: Custom quickstubs for speed

Just to make absolutely sure that we're not getting slower here.

Comment 12

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

https://hg.mozilla.org/mozilla-central/rev/78978886622b
https://hg.mozilla.org/mozilla-central/rev/80ea49628d76
https://hg.mozilla.org/mozilla-central/rev/f5012893e985
https://hg.mozilla.org/mozilla-central/rev/26167a82736d
https://hg.mozilla.org/mozilla-central/rev/9f999446be73
https://hg.mozilla.org/mozilla-central/rev/9d42fde0e996