841442 - Move HTMLFormElement to WebIDL

Status: RESOLVED FIXED

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

Comment 2

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

Form, not Frame

Comment 3

[Andrea Marchesini [:baku]]

Attachment: part 1 - renaming

Comment 4

[Andrea Marchesini [:baku]]

Attachment: part 2 - webidl

Just a feedback because OverrideBuiltins is not implemented yet.

Comment 5

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

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

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

::: content/base/src/nsFormData.h
@@ +22,1 @@
>  class GlobalObject;

G then H

::: content/html/content/src/nsHTMLFormElement.cpp
@@ +746,5 @@
>    return SubmitSubmission(submission); 
>  }
>  
>  nsresult
> +HTMLFormElement::BuildSubmission(nsFormSubmission** aFormSubmission, 

Nit: trailing whitespace

@@ +920,5 @@
>  
>  nsresult
> +HTMLFormElement::NotifySubmitObservers(nsIURI* aActionURL,
> +                                       bool* aCancelSubmit,
> +                                       bool    aEarlyNotify)

Nit: weird spacing

@@ +1010,5 @@
>  
>  // nsIForm
>  
>  NS_IMETHODIMP_(uint32_t)
> +HTMLFormElement::GetElementCount() const 

Nit: trailing ws

::: content/html/content/src/nsHTMLInputElement.cpp
@@ +2238,5 @@
>      // TODO: removing this code and have the submit event sent by the form,
>      // bug 592124.
>      // If there's only one text control, just submit the form
>      // Hold strong ref across the event
> +    nsRefPtr<mozilla::dom::HTMLFormElement> form = mForm;

Add a using namespace... if there isn't one yet, and drop the mozilla::dom here.

@@ +2978,5 @@
>                                // make sure formnovalidate is used only if it's a submit control.
>                                HasAttr(kNameSpaceID_None, nsGkAtoms::formnovalidate) ||
>                                mForm->CheckValidFormSubmission())) {
>                // Hold a strong ref while dispatching
> +              nsRefPtr<mozilla::dom::HTMLFormElement> form(mForm);

And here

::: content/html/content/src/nsIConstraintValidation.cpp
@@ +139,5 @@
>      nsCOMPtr<nsIFormControl> formCtrl = do_QueryInterface(this);
>      NS_ASSERTION(formCtrl, "This interface should be used by form elements!");
>  
> +    mozilla::dom::HTMLFormElement* form =
> +      static_cast<mozilla::dom::HTMLFormElement*>(formCtrl->GetFormElement());

Same

@@ +167,5 @@
>      nsCOMPtr<nsIFormControl> formCtrl = do_QueryInterface(this);
>      NS_ASSERTION(formCtrl, "This interface should be used by form elements!");
>  
> +    mozilla::dom::HTMLFormElement* form =
> +      static_cast<mozilla::dom::HTMLFormElement*>(formCtrl->GetFormElement());

Same

Comment 6

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

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

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

::: content/base/src/nsFormData.cpp
@@ +107,5 @@
>  }
>  
>  /* static */ already_AddRefed<nsFormData>
>  nsFormData::Constructor(const GlobalObject& aGlobal,
> +                        const Optional<NonNull<HTMLFormElement> >& aFormElement,

Make this const Optional<HTMLFormElement&>& aFormElement

::: content/html/content/src/HTMLFormElement.cpp
@@ +319,5 @@
>  
> +already_AddRefed<nsIDOMHTMLCollection>
> +HTMLFormElement::Elements()
> +{
> +  return mControls.forget();

You don't want to do this. Return a raw pointer and don't forget().

@@ +330,1 @@
>    NS_ADDREF(*aElements);

Leak, and a crash if you call it twice.

@@ +1604,5 @@
> +int32_t
> +HTMLFormElement::GetLength(ErrorResult& aRv)
> +{
> +  uint32_t length;
> +  aRv = mControls->GetLength(&length);

Just call mControls->Length()

::: content/html/content/src/HTMLFormElement.h
@@ +248,5 @@
> +
> +  // XPCOM GetAcceptCharset() is OK
> +  void SetAcceptCharset(const nsAString& aValue, ErrorResult& aRv)
> +  {
> +    aRv =SetAttr(kNameSpaceID_None, nsGkAtoms::acceptcharset, nullptr, aValue, true);

SetHTMLAttr

@@ +254,5 @@
> +
> +  // XPCOM GetAction() is OK
> +  void SetAction(const nsAString& aValue, ErrorResult& aRv)
> +  {
> +    aRv = SetAttrHelper(nsGkAtoms::action, aValue);

And the same for the rest

Comment 7

[Boris Zbarsky [:bzbarsky]]

>+  // XPCOM GetAction() is OK

For what it's worth, you may want to write non-XPCOM getters using DOMString anyway, and may want to mark some of this stuff as [Pure]...

You really do need to implement the named and indexed getters, too.

Comment 8

[Andrea Marchesini [:baku]]

Attachment: part 2 - webidl

Comment 9

[Boris Zbarsky [:bzbarsky]]

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

>+    MOZ_ASSERT(aFormElement.Value().get());

It's NonNull.  You can assert that, I guess, but it really is.  ;)

>+    aRv = aFormElement.Value().get()->WalkFormElements(formData);

  aRv = aFormElement.Value().WalkFormElements(formData);

>+#include "mozilla/dom/BindingUtils.h"

This is generally a bad idea to include in headers, since it pulls in all sorts of xpconnect gunk.

Can we move NonNull into BindingDeclarations.h instead?

>+  void GetAction(DOMString& aValue)
>+  {
>+    GetHTMLAttr(nsGkAtoms::action, aValue);

This is wrong, as far as I can tell; if our tests didn't catch it they're pretty lacking.  Please keep using the XPCOM getter here for now.

>+  [Pure]
>+  readonly attribute HTMLFormControlsCollection elements;

Is there a reason this can't be [Constant]?

r=me with those changes.

Comment 10

[Andrea Marchesini [:baku]]

Attachment: part 2 - webidl

Comment 12

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

Peter, will this need special things for the named getter? A js::ExpandoAndGeneration member?

Comment 13

[Peter Van der Beken [:peterv]]

A |js::ExpandoAndGeneration mExpandoAndGeneration| member, code to trace mExpandoAndGeneration.expando for the CC (NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS_INHERITED with a set of NS_IMPL_CYCLE_COLLECTION_TRACE* macros, ...) and to reset it to UndefinedValue() from Unlink, and code to ++mExpandoAndGeneration.generation every time a name is added or removed from the set of exposed names (probably in AddElement/RemoveElement?). See the changes to content/base/src/nsDocument.cpp|.h in http://hg.mozilla.org/integration/mozilla-inbound/rev/9fc3d4e62179.

Comment 14

[Andrea Marchesini [:baku]]

Attachment: part 2 - webidl

peterv, can you review the mExpandoAndGeneration use? Thanks.

Comment 15

[Andrea Marchesini [:baku]]

Attachment: part 1 - renaming

Comment 16

[Boris Zbarsky [:bzbarsky]]

For what it's worth, I can probably review the mExpandoAndGeneration bits, in the interests of getting this in sooner.  ;)

In RemoveElementFromTableInternal, you should only increase the generation when you actually remove the entry from aTable.

For the past names, if you wanted to you could just check the table entry count before/after to see whether anything got removed instead of doing the closure thing.

There is no need to always rev the generation in RemoveElementFromTable if we didn't change the past name map.

In AddElementToTableInternal, you only need to increment the generation when there was no entry already.

Apart from that, those bits look good.  I haven't read the rest in detail yet.

Comment 17

[Andrea Marchesini [:baku]]

Attachment: part 2 - webidl

A new version with your comments applied.

Comment 18

[Peter Van der Beken [:peterv]]

Why are we making HTMLFormControlsCollection an external type? Can't we make it an empty interface for now with nsFormControlList as its native type?

Comment 19

[Boris Zbarsky [:bzbarsky]]

I think we should just typedef HTMLFormControlsCollection to HTMLCollection for the moment.  The only difference in the spec, really, is the legacycaller bit, which I assume we have no plans to implement anyway...

Comment 20

[Boris Zbarsky [:bzbarsky]]

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

>+++ b/content/base/src/nsFormData.cpp
>+    aRv = aFormElement.Value().get()->WalkFormElements(formData);

  aRv = aFormElement.Value().WalkFormElements(formData);

>+++ b/content/html/content/src/HTMLFormElement.cpp
>+  ++tmp->mExpandoAndGeneration.generation;
>+  tmp->mExpandoAndGeneration.expando = JS::UndefinedValue();

Maybe a followup here to add an Unlink() method on the ExpandoAndGeneration struct that does just that, so we don't have to copy/paste it all over?

>+nsIDOMHTMLCollection*
>+HTMLFormElement::Elements()

Maybe document that it's only out-of-line because the class definition is not available in the header?

> RemoveElementFromTableInternal(

Would it make sense to make this class method so we can avoid having mExpandoAndGeneration public?

>+class RemovePastNamesData

I still say this would be better done by comparing the entry counts before and after the Enumerate() call and incrementing if the count changed.

>@@ -2462,16 +2512,17 @@ AddElementToTableInternal(

Again, maybe this should become a class method.

>+++ b/dom/bindings/BindingDeclarations.h
>+class NonNull

Please put this after the Optional<> specializations, not before them?  Or put it before the Optional general template?  I just don't want it between the template and its specializations.

>+++ b/dom/bindings/Bindings.conf
>+addExternalIface('HTMLFormControlsCollection', nativeType='nsIDOMHTMLCollection')

This should die if we do the typedef thing I suggested.

>+++ b/dom/webidl/HTMLFormElement.webidl
>+ * � Copyright 2004-2011 Apple Computer, Inc., Mozilla Foundation, and

Please use UTF-8, not ISO-8859-1.

>+interface HTMLFormControlsCollection;

Like I said, just make this a typedef for HTMLCollection for now.

>+interface HTMLFormElement : HTMLElement {
>+  [Constant, Pure]
>+  readonly attribute HTMLFormControlsCollection elements;

Constant implies Pure; just drop the Pure.

>+  // TODO getter Element (unsigned long index);
>+  // TODO getter object (DOMString name);

These can't be todo.  They need to actually happen for this to land.  Interdiff implementing them is fine, of course.  r- pending the existence of this code.

Please remove the nsIDOMHTMLFormElement bits from dom_quickstubs.qsconf.

There needs to be a followup bug in which we remove classinfo for HTMLFormElement and the hasXPConnectImpls annotations from HTMLElement, Element, and Node, as well as the quickstub bits for those three interfaces and the corresponding customIncludes in dom_quickstubs.qsconf.

And a separate followup to put SetIsDOMBinding() in the nsINode constructor and remove it from all the subclasses, once that previous thing is done.

Comment 21

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

And don't forget to update the special case in reflect.js.

Comment 22

[Andrea Marchesini [:baku]]

Attachment: part 2 - webidl

Comment 23

[Andrea Marchesini [:baku]]

Attachment: interdiff

A part your comments, this interdiff adds |operator->() const| to NonNull template in order to do:

aRv = aFormElement.Value()->WalkFormElements(formData);

Comment 24

[Boris Zbarsky [:bzbarsky]]

No, please take out that operator->.  Does aFormElement.Value().WalkFormElements(formData) not work?

Comment 25

[Andrea Marchesini [:baku]]

(In reply to Boris Zbarsky (:bz) from comment #24)
> No, please take out that operator->.  Does
> aFormElement.Value().WalkFormElements(formData) not work?

No, it doesn't:

error: ‘const class mozilla::dom::NonNull<mozilla::dom::HTMLFormElement>’ has no member named ‘WalkFormElements’

Comment 26

[Boris Zbarsky [:bzbarsky]]

OK.  Let's stick with the .get() you used to have, then, for now, and I'll make it all nice in bug 883827.

Comment 27

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 763152 [details] [diff] [review]
interdiff

> HTMLFormElement::RemoveElementFromTable(nsGenericHTMLFormElement* aElement,
>+    mExpandoAndGeneration.generation += count - mPastNameLookupTable.Count();

How about:

  if (count != mPastNameLookupTable.Count()) {
    ++mExpandoAndGeneration.generation;
  }

and maybe rename "count" to "oldCount"?

>+HTMLFormElement::NamedGetter(const nsAString& aName, bool &aFound)

As discussed on irc, the casts the Element* here are suspect, and the one casting the return value of DoResolveName is bogus.  Also as discussed please make this just return nsISupports in the webidl for now.

Also, is there a reason to not call this method from FindNamedItem instead of duplicating the code?  If not (and I suspect not), please do so.

>+HTMLFormElement::GetSupportedNames(nsTArray<nsString >& aRetval)

This matches neither our old behavior nor the spec.  Furthermore, I think the spec here is bogus-ish: see https://www.w3.org/Bugs/Public/show_bug.cgi?id=22320

Please just make this return without putting anything in aRetval, ok?

r- because I want to see the updated NamedGetter code, but this is very close!

Comment 28

[Andrea Marchesini [:baku]]

Attachment: part 2 - webidl

Comment 29

[Andrea Marchesini [:baku]]

Attachment: interdiff

Comment 30

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 763556 [details] [diff] [review]
interdiff

This still haws the buggy GetSupportedNames, no?  Please take that call to mControls->GetSupportedNames out and replace it with a comment pointing to the W3C bug.

r=me with that.

Comment 31

[Andrea Marchesini [:baku]]

Attachment: part 2 - webidl

Comment 32

[Boris Zbarsky [:bzbarsky]]

Just as a note, the renaming patch here has NOT been rebased on top of what landed in bug 879319.  So it does not actually apply on inbound.

Comment 33

[Andrea Marchesini [:baku]]

Attachment: part 1 - renaming

Comment 34

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/192cecc0111e
https://hg.mozilla.org/integration/mozilla-inbound/rev/259e68f8843d

Yay!

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/192cecc0111e
https://hg.mozilla.org/mozilla-central/rev/259e68f8843d