839447 - Convert HTMLOptionElement to WebIDL

Status: RESOLVED FIXED

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

Comment 1

[Andrea Marchesini [:baku]]

Attachment: part 1 - renaming

Comment 2

[Andrea Marchesini [:baku]]

Attachment: part 2 - webidl

Comment 3

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

Comment on attachment 711826 [details] [diff] [review]
part 2 - webidl

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

Looks good, except for the constructor issue; I'd like Peter to check that.

::: content/html/content/src/HTMLOptionElement.h
@@ +9,5 @@
>  
>  #include "nsGenericHTMLElement.h"
>  #include "nsIDOMHTMLOptionElement.h"
>  #include "nsIJSNativeInitializer.h"
> +#include <nsHTMLFormElement.h>

"" instead of <>

@@ +90,5 @@
> +  {
> +    SetHTMLBoolAttr(nsGkAtoms::disabled, aValue, aRv);
> +  }
> +
> +  already_AddRefed<nsHTMLFormElement> GetForm()

You should be able to just remove this too, if you add "using nsGenericHTMLFormElement::GetForm;"

@@ +130,5 @@
> +  }
> +
> +  int32_t GetIndex(ErrorResult& aRv)
> +  {
> +    int32_t id;

Nit: please initialize this to zero, for safety

::: content/html/content/src/nsHTMLSelectElement.h
@@ +669,1 @@
>      static_cast<nsGenericHTMLElement&>(*aElement.GetAsHTMLOptGroupElement());

Looks like you're getting a merge conflict with yourself in bug 839056?

::: dom/webidl/HTMLOptionElement.webidl
@@ +14,5 @@
> +[NamedConstructor=Option(),
> + NamedConstructor=Option(DOMString text),
> + NamedConstructor=Option(DOMString text, DOMString value),
> + NamedConstructor=Option(DOMString text, DOMString value, boolean defaultSelected),
> + NamedConstructor=Option(DOMString text, DOMString value, boolean defaultSelected, boolean selected)]

Uh, you don't seem to have added code for this... Peter?

Comment 4

[Peter Van der Beken [:peterv]]

Comment on attachment 711826 [details] [diff] [review]
part 2 - webidl

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

::: content/html/content/src/HTMLOptionElement.h
@@ +90,5 @@
> +  {
> +    SetHTMLBoolAttr(nsGkAtoms::disabled, aValue, aRv);
> +  }
> +
> +  already_AddRefed<nsHTMLFormElement> GetForm()

HTMLOptionElement is not a nsGenericHTMLFormElement, so we can't do |using ...|.

I wish we'd implement GetForm (marked as resultNotAddRefed) as

nsHTMLFormElement*
HTMLOptionElement::GetForm()
{
  nsHTMLSelectElement* selectControl = GetSelect();

  return selectControl ? selectControl->GetForm() : nullptr;
}

and make the XPCOM one forward to this.

::: dom/bindings/Bindings.conf
@@ +441,5 @@
>  },
>  
> +'HTMLOptionElement': {
> +    'hasInstanceInterface': 'nsIDOMHTMLOptionElement'
> +},

Obsolete.

::: dom/webidl/HTMLOptionElement.webidl
@@ +14,5 @@
> +[NamedConstructor=Option(),
> + NamedConstructor=Option(DOMString text),
> + NamedConstructor=Option(DOMString text, DOMString value),
> + NamedConstructor=Option(DOMString text, DOMString value, boolean defaultSelected),
> + NamedConstructor=Option(DOMString text, DOMString value, boolean defaultSelected, boolean selected)]

This is fine. We'll get NamedConstructor support soon, but in the meantime the old XPCOM constructors will keep working.

Comment 5

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

(In reply to Peter Van der Beken [:peterv] from comment #4)
> ::: dom/webidl/HTMLOptionElement.webidl
> @@ +14,5 @@
> > +[NamedConstructor=Option(),
> > + NamedConstructor=Option(DOMString text),
> > + NamedConstructor=Option(DOMString text, DOMString value),
> > + NamedConstructor=Option(DOMString text, DOMString value, boolean defaultSelected),
> > + NamedConstructor=Option(DOMString text, DOMString value, boolean defaultSelected, boolean selected)]
> 
> This is fine. We'll get NamedConstructor support soon, but in the meantime
> the old XPCOM constructors will keep working.

Let's comment them out for now, then, to avoid confusion.

Comment 6

[Andrea Marchesini [:baku]]

Attachment: part 2 - webidl

Comment 7

[Andrea Marchesini [:baku]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ea6e27e21be8
https://hg.mozilla.org/integration/mozilla-inbound/rev/68cb1f5270a8

Comment 8

[Gregory Szorc [:gps]]

https://hg.mozilla.org/mozilla-central/rev/ea6e27e21be8
https://hg.mozilla.org/mozilla-central/rev/68cb1f5270a8

Comment 9

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

Attachment: Part 3: Add a missing addref

Comment 10

[Mounir Lamouri (:mounir)]

Comment on attachment 715430 [details] [diff] [review]
Part 3: Add a missing addref

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

::: content/html/content/src/HTMLOptionElement.cpp
@@ +100,5 @@
>  NS_IMETHODIMP
>  HTMLOptionElement::GetForm(nsIDOMHTMLFormElement** aForm)
>  {
>    NS_ENSURE_ARG_POINTER(aForm);
> +  NS_ADDREF(*aForm = GetForm());

Please, remove NS_ENSURE_ARG_POINTER() as long as you are here.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Part 3 landed directly on m-c (pushed on top of orange, no less...)
https://hg.mozilla.org/mozilla-central/rev/b8b9100f6c41

Comment 12

[Boris Zbarsky [:bzbarsky]]

> +  NS_ADDREF(*aForm = GetForm());

Uh... NS_IF_ADDREF, no?

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cc37417e2c28

Comment 14

[Ryan VanderMeulen [:RyanVM]]

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