825196 - Convert HTMLInputElement to WebIDL

Status: RESOLVED FIXED

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

Comment 1

[Boris Zbarsky [:bzbarsky]]

So for this one, other than exposing stufff like nsIDOMNSEditableElement via [ChromeOnly] props, we need to figure out a WebIDL reflection for Date, right?  Thoughts on what it should be?

One option would be to start with a dom::Date that just stores the timestamp and add functionality to it as needed...

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Sounds good to me!

Comment 3

[Andrea Marchesini [:baku]]

Attachment: part 1 - renaming

Comment 4

[Andrea Marchesini [:baku]]

Attachment: part 2 - webidl - WIP

Comment 5

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

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

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

::: content/smil/nsSMILAnimationFunction.h
@@ +378,5 @@
>    }
>    inline void SetKeyPointsErrorFlag(bool aNewValue) {
>      SetErrorFlag(BF_KEY_POINTS, aNewValue);
>    }
> +  // Helper method -- based on SET_BOOLBIT in HTMLInputElement.cpp

Heh, I killed SET_BOOLBIT long ago; remove the reference, please?

::: layout/forms/nsRangeFrame.cpp
@@ +308,5 @@
>  nsRangeFrame::GetValueAsFractionOfRange()
>  {
>    MOZ_ASSERT(mContent->IsHTML(nsGkAtoms::input), "bad cast");
> +  mozilla::dom::HTMLInputElement* input =
> +    static_cast<mozilla::dom::HTMLInputElement*>(mContent);

"using namespace mozilla;" and use "dom::HTMLInputElement", please.

Comment 6

[Andrea Marchesini [:baku]]

Attachment: part 1 - renaming

Comment 7

[Andrea Marchesini [:baku]]

Attachment: part 2 - webidl - WIP

Comment 8

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

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

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

This is from a cursory look.

::: content/html/content/src/HTMLInputElement.cpp
@@ +1267,5 @@
> +  return rv.ErrorCode();
> +}
> +
> +already_AddRefed<nsGenericHTMLElement>
> +HTMLInputElement::GetList() const

I think this can return a raw pointer

@@ +2047,5 @@
> +    mFileList = new nsDOMFileList(static_cast<nsIContent*>(this));
> +    UpdateFileList();
> +  }
> +
> +  return mFileList.forget();

You're clearing mFileList here. Just return mFileList as a raw pointer.

@@ +3920,5 @@
>        mControllers->AppendController(controller);
>      }
>    }
>  
> +  return mControllers.forget();

Same issue here

@@ +3984,5 @@
> +                                    const nsAString& aDirection)
> +{
> +  ErrorResult rv;
> +  const Optional<nsAString> direction;
> +  const_cast<Optional<nsAString>&>(direction) = &aDirection;

Just make it non-const and drop the const_cast.

Comment 9

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

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

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

::: content/html/content/src/HTMLInputElement.h
@@ +505,5 @@
> +  }
> +
> +  void SetSize(uint32_t aValue, ErrorResult& aRv)
> +  {
> +    SetHTMLIntAttr(nsGkAtoms::size, aValue, aRv);

NS_IMPL_UINT_ATTR_NON_ZERO_DEFAULT_VALUE does something rather different than SetHTMLIntAttr.

Comment 10

[Boris Zbarsky [:bzbarsky]]

> NS_IMPL_UINT_ATTR_NON_ZERO_DEFAULT_VALUE does something rather different 

And if we don't have tests for that, we should add them.

Comment 11

[Andrea Marchesini [:baku]]

Attachment: part 2 - webidl

It took forever to fix all the mochitests...

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

Comment 12

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

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

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

(Just a first pass.)

::: content/html/content/src/HTMLInputElement.cpp
@@ +1230,2 @@
>        }
>        const PRUnichar *name = PromiseFlatString(aValue).get();

I'm surprised this works. (Not new, though.)

@@ +1407,3 @@
>  {
>    if (mType != NS_FORM_INPUT_DATE && mType != NS_FORM_INPUT_TIME) {
> +    return JSVAL_NULL;

Nit: JS::NullValue() throughout.

@@ +1429,5 @@
>        jsval fullYear[3];
>        fullYear[0].setInt32(year);
>        fullYear[1].setInt32(month - 1);
>        fullYear[2].setInt32(day);
> +      if(!JS::Call(aCx, date, "setUTCFullYear", 3, fullYear, &rval)) {

Fix the missing space before the (, while you're here.

@@ +1436,3 @@
>        }
>  
> +      return date ? OBJECT_TO_JSVAL(date) : JSVAL_NULL;

JS::ObjectOrNullValue

@@ +1453,3 @@
>        }
>  
> +      return date ? OBJECT_TO_JSVAL(date) : JSVAL_NULL;

Same

@@ +3988,5 @@
> +                                    const nsAString& aDirection)
> +{
> +  ErrorResult rv;
> +  const Optional<nsAString> direction;
> +  const_cast<Optional<nsAString>&>(direction) = &aDirection;

Drop the constness.

::: dom/webidl/HTMLInputElement.webidl
@@ +133,5 @@
> +  readonly attribute long                  textLength;
> +
> + /* TODO
> +  void mozGetFileNameArray([optional] out unsigned long aLength,
> +                           [array,size_is(aLength), retval] out wstring aFileNames);

We'll need to deal with this.

@@ +136,5 @@
> +  void mozGetFileNameArray([optional] out unsigned long aLength,
> +                           [array,size_is(aLength), retval] out wstring aFileNames);
> +  */
> +  [ChromeOnly]
> +  void mozSetFileNameArray(DOMStringList fileNames, unsigned long length);

Make this take a sequence<DOMString> and drop the length argument.

::: layout/generic/test/test_selection_expanding.html
@@ +82,4 @@
>    function getSelectionForEditor(aEditorElement)
>    {
> +    const nsIDOMNSEditableElement = SpecialPowers.Ci.nsIDOMNSEditableElement;
> +    return SpecialPowers.wrap(aEditorElement).QueryInterface(nsIDOMNSEditableElement).editor.selection;

You shouldn't need to QI, since you also implemented editor as a ChromeOnly attribute.

Comment 13

[Boris Zbarsky [:bzbarsky]]

> I'm surprised this works. (Not new, though.)

It'll work if aValue is already flat.... That said, I didn't think that was necessarily the case for strings coming from JS.

> We'll need to deal with this.

What dealing is needed, really?  Just return a sequence<DOMString>, and it should Just Work.

Comment 14

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

(In reply to Boris Zbarsky (:bz) from comment #13)
> > I'm surprised this works. (Not new, though.)
> 
> It'll work if aValue is already flat.... That said, I didn't think that was
> necessarily the case for strings coming from JS.

Sounds like we'd better fix it, then.

> > We'll need to deal with this.
> 
> What dealing is needed, really?  Just return a sequence<DOMString>, and it
> should Just Work.

That dealing, apparently. All the better if we can kill the useless outparam.

Comment 15

[Boris Zbarsky [:bzbarsky]]

So you write your IDL like so:

  sequence<DOMString> mozGetFileNameArray();

and your C++ singnature ends up like so:

  void MozGetFileNameArray(nsTArray<nsString>& aRetval);

and then it should all be easy going.  ;)

Comment 16

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

And it looks like we'll need a change to content/html/content/test/reflect.js, too.

Comment 17

[Andrea Marchesini [:baku]]

Attachment: part 2 - webidl

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

Comment 18

[Andrea Marchesini [:baku]]

> > It'll work if aValue is already flat.... That said, I didn't think that was
> > necessarily the case for strings coming from JS.
> 
> Sounds like we'd better fix it, then.

Sure. But in a following up bug.

Comment 19

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

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

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

Not going to get through it tonight, I'm afraid.

::: content/html/content/src/HTMLInputElement.cpp
@@ +1297,1 @@
>      return NS_OK;

You need to set *aValue to null here.

@@ +1741,5 @@
>      return NS_ERROR_OUT_OF_MEMORY;
>    }
>  
> +  for (uint32_t i = 0; i < *aLength; ++i) {
> +    ret[i] = NS_strdup(array[0].get());

array[i], right?

@@ +1791,5 @@
> +  }
> +
> +  Sequence<nsString> list;
> +  for (uint32_t i = 0; i < aLength; ++i) {
> +    list.AppendElement(nsDependentString(aFileNames[0]));

[i]?

Comment 20

[Andrea Marchesini [:baku]]

Attachment: part 2 - webidl

Comment 21

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

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

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

Looks so good I'd like to see it once more :)

::: content/html/content/src/HTMLInputElement.cpp
@@ +76,5 @@
>  #include "nsIObserverService.h"
>  #include "nsIPopupWindowManager.h"
>  #include "nsGlobalWindow.h"
> +#include "nsIDOMDOMStringList.h"
> +#include "nsDOMLists.h"

I think we can do without those.

@@ +1230,3 @@
>        }
>        const PRUnichar *name = PromiseFlatString(aValue).get();
> +      aRv = MozSetFileNameArray(&name, 1);

Please call the overload with that takes a Sequence instead.

@@ +1455,3 @@
>        }
>  
> +      return JS::ObjectOrNullValue(date);

Make it JS::ObjectValue, please.

@@ +1464,4 @@
>  }
>  
>  NS_IMETHODIMP
> +HTMLInputElement::GetValueAsDate(JSContext* aCx, jsval* aDate)

JS::Value

@@ +1504,5 @@
>    SetValue(timestamp.toNumber());
> +}
> +
> +NS_IMETHODIMP
> +HTMLInputElement::SetValueAsDate(JSContext* aCx, const jsval& aDate)

JS::Value

@@ +1784,5 @@
>    SetFiles(files, true);
> +}
> +
> +NS_IMETHODIMP
> +HTMLInputElement::MozSetFileNameArray(const PRUnichar **aFileNames, uint32_t aLength)

** to the left

@@ +3951,5 @@
> +int32_t
> +HTMLInputElement::GetTextLength(ErrorResult& aRv)
> +{
> +  nsAutoString val;
> +  aRv = GetValue(val);

Call as GetValue(val, aRv)?

::: content/html/content/src/HTMLInputElement.h
@@ +211,5 @@
>    virtual nsIDOMNode* AsDOMNode() { return this; }
>  
>    // nsIConstraintValidation
>    bool     IsTooLong();
> +  bool     IsValueMissing();

I don't understand why this change would need to be made.

@@ +351,5 @@
> +  {
> +    SetHTMLBoolAttr(nsGkAtoms::autofocus, aValue, aRv);
> +  }
> +
> +  void SetDefaultChecked(bool aValue, ErrorResult& aRv)

Move the existing DefaultChecked() here?

@@ +356,5 @@
> +  {
> +    SetHTMLBoolAttr(nsGkAtoms::checked, aValue, aRv);
> +  }
> +
> +  // XPCOM SetChecked() is OK

And the existing Checked()

@@ +368,5 @@
> +  {
> +    SetHTMLBoolAttr(nsGkAtoms::disabled, aValue, aRv);
> +  }
> +
> +  nsDOMFileList* GetFiles();

And where does GetForm() come from?

@@ +411,5 @@
> +  {
> +    aRv = nsGenericHTMLElement::SetUnsignedIntAttr(nsGkAtoms::height, aValue);
> +  }
> +
> +  void SetIndeterminate(bool aValue, ErrorResult& aRv)

Move the existing Indeterminate()

@@ +413,5 @@
> +  }
> +
> +  void SetIndeterminate(bool aValue, ErrorResult& aRv)
> +  {
> +    aRv = SetIndeterminateInternal(aValue, true);

Please fix SetIndeterminateInternal to return void, and drop the [SetterThrows].

@@ +540,5 @@
> +  {
> +    SetHTMLAttr(nsGkAtoms::value, aValue, aRv);
> +  }
> +
> +  // XPCOM GetValue() is OK

Please make GetValueInternal return void.

@@ +562,5 @@
> +  {
> +    aRv = nsGenericHTMLElement::SetUnsignedIntAttr(nsGkAtoms::width, aValue);
> +  }
> +
> +  void StepUp(const Optional< int32_t >& n, ErrorResult& aRv)

These should have "= 1" in the IDL. (I filed <https://www.w3.org/Bugs/Public/show_bug.cgi?id=21415>.)

@@ +587,5 @@
> +
> +  void GetSelectionDirection(nsAString& aValue, ErrorResult& aRv);
> +  void SetSelectionDirection(const nsAString& aValue, ErrorResult& aRv);
> +
> +  void SetSelectionRange(int32_t start, int32_t end,

aStart, aEnd

@@ +592,5 @@
> +                         const Optional< nsAString >& direction,
> +                         ErrorResult &aRv);
> +
> +  // XPCOM GetAlign() is OK
> +  void SetAlign(const nsAString& aValue, ErrorResult &aRv)

& to the left

@@ +607,5 @@
> +  nsIControllers* GetControllers(ErrorResult& aRv);
> +
> +  int32_t GetTextLength(ErrorResult& aRv);
> +
> +  void MozGetFileNameArray(nsTArray< nsString >& retval);

aFileNames

@@ +647,5 @@
>      // throw the INVALID_STATE_ERR exception.
>      VALUE_MODE_FILENAME
>    };
>  
> +  const nsCOMArray<nsIDOMFile>& GetFilesInternal() const

Why move this around?

::: dom/webidl/HTMLInputElement.webidl
@@ +137,5 @@
> +
> +  [ChromeOnly]
> +  void mozSetFileNameArray(sequence<DOMString> fileNames);
> +
> +    /**

Indentation is off.

@@ +157,5 @@
> +  // 'change' event for example will be dispatched when focusing out the
> +  // element.
> +  [ChromeOnly]
> +  void setUserInput(DOMString input);
> +};

Missing an "HTMLInputElement implements MozImageLoadingContent;" statement, I believe.

::: editor/libeditor/base/tests/test_bug502673.html
@@ +55,2 @@
>              return this;
> +          throw Special.Cr.NS_ERROR_NO_INTERFACE;

I think you accidentally half a word.

Comment 22

[Boris Zbarsky [:bzbarsky]]

At first glance that patch loses nsIPhonetic.

Also, just adding a comment in the WebIDL to update it if nsIDOMNSEditableElement changes is not that useful.  The comment should be in nsIDOMNSEditableElement too.

Comment 23

[Boris Zbarsky [:bzbarsky]]

And for the plain string reflector getters, might be better to use a DOMString signature, and [Pure] as needed.

Comment 24

[Andrea Marchesini [:baku]]

> Please make GetValueInternal return void.

Somewhere we have checks of the return value of it.

> > +  void StepUp(const Optional< int32_t >& n, ErrorResult& aRv)
> 
> These should have "= 1" in the IDL. (I filed
> <https://www.w3.org/Bugs/Public/show_bug.cgi?id=21415>.)

Should I change something... ?

> @@ +647,5 @@
> >      // throw the INVALID_STATE_ERR exception.
> >      VALUE_MODE_FILENAME
> >    };
> >  
> > +  const nsCOMArray<nsIDOMFile>& GetFilesInternal() const
> 
> Why move this around?

it didn't exist before.

Comment 25

[Andrea Marchesini [:baku]]

Attachment: part 2 - webidl

Comment 26

[Andrea Marchesini [:baku]]

Attachment: interdiff

Comment 27

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

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

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

File a followup for using DOMString more.

::: dom/webidl/HTMLInputElement.webidl
@@ +14,5 @@
> +
> +interface nsIControllers;
> +
> +interface HTMLInputElement : HTMLElement {
> +           [Pure, SetterThrows]

Indentation is weird in this file... I'd like everything to be indented 2 spaces, except possibly writable attributes, which get 2 + len("readonly ") spaces.

@@ +151,5 @@
> +};
> +
> +partial interface HTMLInputElement {
> +           // Mirrored chrome-only nsIDOMNSEditableElement methods.  Please make sure
> +           // to update this list if nsIDOMNSEditableElement changes.

Add this comment to nsIDOMNSEditableElement.idl.

@@ +167,5 @@
> +
> +[NoInterfaceObject]
> +interface MozPhonetic {
> +           [Pure]
> +  readonly attribute DOMString phonetic;

Should be [ChromeOnly].

Comment 28

[Boris Zbarsky [:bzbarsky]]

> which get 2 + len("readonly ") spaces.

That's the "standard" indentation pattern web specs use, sadly.

Comment 29

[Andrea Marchesini [:baku]]

Attachment: part 2 - webidl

For DOMString we have Bug 835800

Comment 30

[Andrea Marchesini [:baku]]

Just last test on try: https://tbpl.mozilla.org/?tree=Try&rev=cad4c45c90aa

Comment 31

[Boris Zbarsky [:bzbarsky]]

> For DOMString we have Bug 835800

Sort of.  I was going to fix that bug by checking the files which got converted _before_ it got filed... but now we'll have to re-audit all of them.  :(

Comment 32

[Ryan VanderMeulen [:RyanVM]]

This doesn't apply cleanly to inbound at all. Please rebase.

Comment 33

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

Yeah, mounir was all over nsHTMLInputElement just now...

Comment 34

[Andrea Marchesini [:baku]]

Attachment: part 1 - renaming

Comment 35

[Andrea Marchesini [:baku]]

Attachment: part 2 - webidl

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3ffdb788846d
https://hg.mozilla.org/integration/mozilla-inbound/rev/23850987c825

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3ffdb788846d
https://hg.mozilla.org/mozilla-central/rev/23850987c825

Comment 38

[Kohei Yoshino [:kohei]]

https://developer.mozilla.org/en-US/docs/Site_Compatibility_for_Firefox_22
https://developer.mozilla.org/en-US/docs/DOM/Input.mozGetFileNameArray
https://developer.mozilla.org/en-US/docs/DOM/Input.mozSetFileNameArray

Comment 39

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

I don't think the obsolescence banner is correct; these methods have never been available to web content (they threw if you called them from content).

Comment 40

[Kohei Yoshino [:kohei]]

Tested these methods on Firefox 19 and confirmed that it threw an Error: "The operation is insecure". I just removed the item from the compat doc and corrected the reference pages. I misread the situation :(

Comment 41

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

No problem, I much appreciate you work!