Move HTMLFormElement to WebIDL

RESOLVED FIXED in mozilla24

Status

()

defect
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: Ms2ger, Assigned: baku)

Tracking

(Blocks 1 bug)

Trunk
mozilla24
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 11 obsolete attachments)

No description provided.
Depends on: 841447
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 838343
Form, not Frame
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Assignee: nobody → amarchesini
Posted patch part 1 - renaming (obsolete) — Splinter Review
Attachment #721687 - Flags: review?(Ms2ger)
Posted patch part 2 - webidl (obsolete) — Splinter Review
Just a feedback because OverrideBuiltins is not implemented yet.
Attachment #721688 - Flags: feedback?(Ms2ger)
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
Attachment #721687 - Flags: review?(Ms2ger) → review+
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
Attachment #721688 - Flags: feedback?(Ms2ger)
>+  // 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.
Posted patch part 2 - webidl (obsolete) — Splinter Review
Attachment #721688 - Attachment is obsolete: true
Attachment #726241 - Flags: review?(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.
Attachment #726241 - Flags: review?(bzbarsky) → review+
Posted patch part 2 - webidl (obsolete) — Splinter Review
Attachment #726241 - Attachment is obsolete: true
Attachment #726625 - Flags: review+
Duplicate of this bug: 485220
Peter, will this need special things for the named getter? A js::ExpandoAndGeneration member?
Flags: needinfo?(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.
Flags: needinfo?(peterv)
No longer depends on: 841447
Depends on: 870787
Posted patch part 2 - webidl (obsolete) — Splinter Review
peterv, can you review the mExpandoAndGeneration use? Thanks.
Attachment #726625 - Attachment is obsolete: true
Attachment #761978 - Flags: review?(peterv)
Posted patch part 1 - renaming (obsolete) — Splinter Review
Attachment #721687 - Attachment is obsolete: true
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.
Posted patch part 2 - webidl (obsolete) — Splinter Review
A new version with your comments applied.
Attachment #761978 - Attachment is obsolete: true
Attachment #761978 - Flags: review?(peterv)
Attachment #762598 - Flags: review?(bzbarsky)
Why are we making HTMLFormControlsCollection an external type? Can't we make it an empty interface for now with nsFormControlList as its native type?
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 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.
Attachment #762598 - Flags: review?(bzbarsky) → review-
And don't forget to update the special case in reflect.js.
Posted patch part 2 - webidl (obsolete) — Splinter Review
Attachment #762598 - Attachment is obsolete: true
Posted patch interdiff (obsolete) — Splinter Review
A part your comments, this interdiff adds |operator->() const| to NonNull template in order to do:

aRv = aFormElement.Value()->WalkFormElements(formData);
Attachment #763152 - Flags: review?(bzbarsky)
No, please take out that operator->.  Does aFormElement.Value().WalkFormElements(formData) not work?
(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’
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 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!
Attachment #763152 - Flags: review?(bzbarsky) → review-
Posted patch part 2 - webidl (obsolete) — Splinter Review
Attachment #763151 - Attachment is obsolete: true
Posted patch interdiff (obsolete) — Splinter Review
Attachment #763152 - Attachment is obsolete: true
Attachment #763556 - Flags: review?(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.
Attachment #763556 - Flags: review?(bzbarsky) → review+
Attachment #763554 - Attachment is obsolete: true
Attachment #763556 - Attachment is obsolete: true
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.
Attachment #761979 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/192cecc0111e
https://hg.mozilla.org/mozilla-central/rev/259e68f8843d
Status: REOPENED → RESOLVED
Closed: 7 years ago6 years ago
Resolution: --- → FIXED
Blocks: 898832
Duplicate of this bug: 307415
You need to log in before you can comment on or make changes to this bug.