Closed Bug 839105 Opened 7 years ago Closed 7 years ago

Convert HTMLFieldSetElement to WebIDL

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla21

People

(Reporter: baku, Assigned: baku)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 6 obsolete files)

No description provided.
Attached patch part 1 - renaming (obsolete) — Splinter Review
Attachment #711761 - Flags: review?(Ms2ger)
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?
Attachment #711761 - Flags: review?(Ms2ger) → review+
Attachment #711761 - Attachment is obsolete: true
Attachment #711764 - Flags: review+
Attached patch part 2 - webidl (obsolete) — Splinter Review
Attachment #711765 - Flags: review?(Ms2ger)
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;"
Attachment #711765 - Flags: review?(Ms2ger) → review-
> > +  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.
> 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?
Attached patch part 2 - webidl (b) (obsolete) — Splinter Review
Attachment #711765 - Attachment is obsolete: true
Attachment #711783 - Flags: review?(Ms2ger)
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.
Attachment #711783 - Flags: review?(Ms2ger) → review-
Attached patch part 2 - webidl (c) (obsolete) — Splinter Review
Attachment #711783 - Attachment is obsolete: true
Attachment #711794 - Flags: review?(Ms2ger)
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
Attachment #711794 - Flags: review?(Ms2ger) → review-
Attached patch part 2 - webidl (d) (obsolete) — Splinter Review
Attachment #711794 - Attachment is obsolete: true
Attachment #711808 - Flags: review?(Ms2ger)
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
Attachment #711808 - Flags: review?(Ms2ger) → review+
Attached patch part 2 - webidl (obsolete) — Splinter Review
Attachment #711808 - Attachment is obsolete: true
Attachment #711814 - Flags: review+
Keywords: checkin-needed
> +  // XPCOM Validity is OK for us

It's actually a WebIDL signature, inherited from the nsIConstraintNavigation superclass, right?
Attached patch part 2 - webidlSplinter Review
Right. Comment removed.
Attachment #711814 - Attachment is obsolete: true
Attachment #711829 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/afeaf8f42c09
https://hg.mozilla.org/mozilla-central/rev/f2ece1151f00
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Depends on: 841466
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.