Kill NS_FORWARD_NSIDOMHTMLELEMENT_BASIC

RESOLVED FIXED in mozilla18

Status

()

Core
DOM
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Ms2ger, Assigned: Ms2ger)

Tracking

Trunk
mozilla18
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 attachments)

(Assignee)

Description

6 years ago
Created attachment 666215 [details] [diff] [review]
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.
Attachment #666215 - Flags: review?(mounir)
(Assignee)

Comment 1

6 years ago
Created attachment 666216 [details] [diff] [review]
Part b: tabIndex

I went for a virtual TabIndexDefault() instead of overriding TabIndex() to avoid duplicating code too much.
Attachment #666216 - Flags: review?(mounir)
(Assignee)

Comment 2

6 years ago
Created attachment 666217 [details] [diff] [review]
Part c: draggable
Attachment #666217 - Flags: review?(mounir)
(Assignee)

Comment 3

6 years ago
Created attachment 666218 [details] [diff] [review]
Part d: click()
Attachment #666218 - Flags: review?(mounir)
(Assignee)

Comment 4

6 years ago
Created attachment 666219 [details] [diff] [review]
Part e: focus()
Attachment #666219 - Flags: review?(mounir)
(Assignee)

Comment 5

6 years ago
Created attachment 666220 [details] [diff] [review]
Part f: innerHTML
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+
(Assignee)

Comment 11

6 years ago
Created attachment 668575 [details] [diff] [review]
Part g: Custom quickstubs for speed

Just to make absolutely sure that we're not getting slower here.
Attachment #668575 - Flags: review?(mounir)
Attachment #668575 - Flags: review?(mounir) → review+
You need to log in before you can comment on or make changes to this bug.