839105 - Convert HTMLFieldSetElement to WebIDL

Status: RESOLVED FIXED

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

Comment 1

[Andrea Marchesini [:baku]]

https://tbpl.mozilla.org/?tree=Try&rev=bec9847c6097

Comment 2

[Andrea Marchesini [:baku]]

Attachment: part 1 - renaming

Comment 3

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

Comment on attachment 711761 [details] [diff] [review]
part 1 - renaming

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

::: content/base/public/nsINode.h
@@ +278,5 @@
>    // - nsGenericHTMLElement:  mForm, mFieldSet
>    // - nsGenericHTMLFrameElement: mFrameLoader (bug 672539), mTitleChangedListener
>    // - HTMLBodyElement:       mContentStyleRule
>    // - HTMLDataListElement:   mOptions
> +  // - HTMLFieldSetElement:   mElements, mDependentElements, mFirstLegend

Want to have a look which of these already lost their ns prefix?

::: content/html/content/src/nsHTMLFieldSetElement.h
@@ +3,5 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +#ifndef mozilla_dom_HTMLOptGroupElement_h
> +#define mozilla_dom_HTMLOptGroupElement_h

mozilla_dom_HTMLOptGroupElement_h?

Comment 4

[Andrea Marchesini [:baku]]

Attachment: part 1 - renaming

Comment 5

[Andrea Marchesini [:baku]]

Attachment: part 2 - webidl

Comment 6

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

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

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

I'd like to see an updated version

::: content/html/content/src/HTMLFieldSetElement.cpp
@@ +130,5 @@
> +  }
> +
> +  nsRefPtr<nsHTMLFormElement> ret = static_cast<nsHTMLFormElement*>(form.get());
> +  return ret.forget();
> +}

"using nsGenericHTMLFormElement::GetForm;" in the header

@@ +167,5 @@
> +  nsCOMPtr<nsIDOMHTMLCollection> elements;
> +  GetElements(getter_AddRefs(elements));
> +  nsCOMPtr<nsIHTMLCollection> ret = do_QueryInterface(elements);
> +  return ret.forget();
> +}

Please make this

nsIHTMLCollection*
HTMLFieldSetElement::Elements()
{
  if (!mElements) {
    mElements = ...;
  }
  return mElements;
}

and

NS_IMETHODIMP
HTMLFieldSetElement::GetElements(nsIDOMHTMLCollection** aElements)
{
  NS_ADDREF(*aElements = Elements());
  return NS_OK;
}

@@ +270,5 @@
> +  }
> +
> +  nsRefPtr<ValidityState> ret = static_cast<ValidityState*>(state.get());
> +  return ret.forget();
> +}

You don't need this at all.

::: content/html/content/src/HTMLFieldSetElement.h
@@ +106,5 @@
> +  {
> +    bool ret;
> +    aError = CheckValidity(&ret);
> +    return ret;
> +  }

"using nsIConstraintValidation::CheckValidity;"

Comment 7

[Andrea Marchesini [:baku]]

> > +  nsRefPtr<ValidityState> ret = static_cast<ValidityState*>(state.get());
> > +  return ret.forget();
> > +}
> 
> You don't need this at all.


I need a nsRefPtr<ValidityState> from a nsIDOMValidityState. I think I need this.

Comment 8

[Boris Zbarsky [:bzbarsky]]

> I need a nsRefPtr<ValidityState> from a nsIDOMValidityState. I think I need this.

You can get away with:

  return static_cast<ValidityState*>(state.forget().get());

That said, I think the point is that you don't need the function, period: nsIConstraintValidation has a Validity() function on it already that seems to do the right thing, so you can just inherit that, right?

Comment 9

[Andrea Marchesini [:baku]]

Attachment: part 2 - webidl (b)

Comment 10

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

Comment on attachment 711783 [details] [diff] [review]
part 2 - webidl (b)

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

::: content/html/content/src/HTMLFieldSetElement.cpp
@@ +153,5 @@
>  HTMLFieldSetElement::GetElements(nsIDOMHTMLCollection** aElements)
>  {
> +  nsCOMPtr<nsIHTMLCollection> elements = Elements();
> +  nsCOMPtr<nsIDOMHTMLCollection> e = do_QueryInterface(elements);
> +  NS_ADDREF(*aElements = e);

nsIHTMLCollection inherits from nsIDOMHTMLCollection. Please use the code I gave you.

@@ +166,5 @@
>                                    true);
>    }
>  
> +  nsCOMPtr<nsIHTMLCollection> ret = do_QueryInterface((nsIDOMHTMLCollection*)mElements);
> +  return ret.forget();

nsContentList inherits from nsIHTMLCollection. Please just use "return mElements;"

::: content/html/content/src/HTMLFieldSetElement.h
@@ +96,5 @@
> +  }
> +
> +  // XPCOM GetType is OK for us
> +
> +  already_AddRefed<nsIHTMLCollection> Elements();

Should return a raw, non-addrefed pointer.

@@ +109,5 @@
> +  {
> +    bool ret;
> +    aError = CheckValidity(&ret);
> +    return ret;
> +  }

Remove this.

Comment 11

[Andrea Marchesini [:baku]]

Attachment: part 2 - webidl (c)

Comment 12

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

Comment on attachment 711794 [details] [diff] [review]
part 2 - webidl (c)

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

::: content/html/content/src/HTMLFieldSetElement.cpp
@@ +120,5 @@
>    return nsGenericHTMLFormElement::GetForm(aForm);
>  }
>  
> +already_AddRefed<nsHTMLFormElement>
> +HTMLFieldSetElement::GetForm()

Remove this

::: content/html/content/src/HTMLFieldSetElement.h
@@ +106,5 @@
> +  // XPCOM GetValidationMessage is OK for us
> +
> +  // XPCOM CheckValidity is OK for us
> +
> +  bool CheckValidity(ErrorResult& aError)

Remove this

Comment 13

[Andrea Marchesini [:baku]]

Attachment: part 2 - webidl (d)

Comment 14

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

Comment on attachment 711808 [details] [diff] [review]
part 2 - webidl (d)

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

Thanks!

::: dom/bindings/Bindings.conf
@@ +402,5 @@
>      ]
>  },
>  
> +'HTMLFieldSetElement': {
> +  'hasInstanceInterface': 'nsIDOMHTMLFieldSetElement',

Nit: four spaces

Comment 15

[Andrea Marchesini [:baku]]

Attachment: part 2 - webidl

Comment 16

[Boris Zbarsky [:bzbarsky]]

> +  // XPCOM Validity is OK for us

It's actually a WebIDL signature, inherited from the nsIConstraintNavigation superclass, right?

Comment 17

[Andrea Marchesini [:baku]]

Attachment: part 2 - webidl

Right. Comment removed.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/afeaf8f42c09
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2ece1151f00

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/afeaf8f42c09
https://hg.mozilla.org/mozilla-central/rev/f2ece1151f00