Closed Bug 795610 Opened 7 years ago Closed 7 years ago

Kill NS_FORWARD_NSIDOMHTMLELEMENT_BASIC

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: Ms2ger, Assigned: Ms2ger)

References

Details

Attachments

(7 files)

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.
Attachment #666215 - Flags: review?(mounir)
Attached patch Part b: tabIndexSplinter Review
I went for a virtual TabIndexDefault() instead of overriding TabIndex() to avoid duplicating code too much.
Attachment #666216 - Flags: review?(mounir)
Attachment #666217 - Flags: review?(mounir)
Attached patch Part d: click()Splinter Review
Attachment #666218 - Flags: review?(mounir)
Attached patch Part e: focus()Splinter Review
Attachment #666219 - Flags: review?(mounir)
Attachment #666220 - Flags: review?(mounir)
Attachment #666215 - Flags: review?(mounir) → review+
Attachment #666216 - Flags: review?(mounir) → review+
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);
Attachment #666217 - Flags: review?(mounir) → review+
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.
Attachment #666218 - Flags: review?(mounir) → review+
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());
Attachment #666219 - Flags: review?(mounir) → review+
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);
Attachment #666220 - Flags: review?(mounir) → review+
Just to make absolutely sure that we're not getting slower here.
Attachment #668575 - Flags: review?(mounir)
Attachment #668575 - Flags: review?(mounir) → review+
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.