Convert HTMLInputElement to WebIDL

RESOLVED FIXED in mozilla22

Status

()

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: mounir, Assigned: baku)

Tracking

(Blocks: 3 bugs)

Trunk
mozilla22
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 10 obsolete attachments)

Comment hidden (empty)
(Reporter)

Updated

6 years ago
Blocks: 825197
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...
(Reporter)

Updated

6 years ago
Blocks: 826302
(Reporter)

Updated

6 years ago
Blocks: 826738
(Assignee)

Updated

6 years ago
Assignee: nobody → amarchesini
(Assignee)

Comment 3

6 years ago
Created attachment 727611 [details] [diff] [review]
part 1 - renaming
Attachment #727611 - Flags: review?(Ms2ger)
(Assignee)

Comment 4

6 years ago
Created attachment 727612 [details] [diff] [review]
part 2 - webidl - WIP
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.
Attachment #727611 - Flags: review?(Ms2ger) → review+
(Assignee)

Comment 6

6 years ago
Created attachment 728196 [details] [diff] [review]
part 1 - renaming
Attachment #727611 - Attachment is obsolete: true
Attachment #728196 - Flags: review+
(Assignee)

Comment 7

6 years ago
Created attachment 728209 [details] [diff] [review]
part 2 - webidl - WIP
Attachment #727612 - Attachment is obsolete: true
Attachment #728209 - Flags: feedback?(Ms2ger)
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 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.
> 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.
(Assignee)

Comment 11

6 years ago
Created attachment 728970 [details] [diff] [review]
part 2 - webidl

It took forever to fix all the mochitests...

https://tbpl.mozilla.org/?tree=Try&rev=2f5414a5f2cd
Attachment #728209 - Attachment is obsolete: true
Attachment #728209 - Flags: feedback?(Ms2ger)
Attachment #728970 - Flags: review?(Ms2ger)
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.
Attachment #728970 - Flags: review?(Ms2ger) → review-
> 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.
(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.
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.  ;)
And it looks like we'll need a change to content/html/content/test/reflect.js, too.
(Reporter)

Updated

6 years ago
Blocks: 757664
(Assignee)

Comment 17

6 years ago
Created attachment 729671 [details] [diff] [review]
part 2 - webidl

https://tbpl.mozilla.org/?tree=Try&rev=ad030a9fd0ca
Attachment #728970 - Attachment is obsolete: true
Attachment #729671 - Flags: review?(Ms2ger)
(Assignee)

Comment 18

6 years ago
> > 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 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]?
(Assignee)

Comment 20

6 years ago
Created attachment 729771 [details] [diff] [review]
part 2 - webidl
Attachment #729671 - Attachment is obsolete: true
Attachment #729671 - Flags: review?(Ms2ger)
Attachment #729771 - Flags: review?(Ms2ger)
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.
Attachment #729771 - Flags: review?(Ms2ger) → review-
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.
And for the plain string reflector getters, might be better to use a DOMString signature, and [Pure] as needed.
(Assignee)

Comment 24

6 years ago
> 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.
(Assignee)

Comment 25

6 years ago
Created attachment 730271 [details] [diff] [review]
part 2 - webidl
Attachment #729771 - Attachment is obsolete: true
Attachment #730271 - Flags: review?(Ms2ger)
(Assignee)

Comment 26

6 years ago
Created attachment 730272 [details] [diff] [review]
interdiff
Attachment #730272 - Flags: review?(Ms2ger)
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].
Attachment #730271 - Flags: review?(Ms2ger) → review+
> which get 2 + len("readonly ") spaces.

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

Comment 29

6 years ago
Created attachment 730375 [details] [diff] [review]
part 2 - webidl

For DOMString we have Bug 835800
Attachment #730271 - Attachment is obsolete: true
Attachment #730272 - Attachment is obsolete: true
Attachment #730272 - Flags: review?(Ms2ger)
Attachment #730375 - Flags: review+
> 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.  :(
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
This doesn't apply cleanly to inbound at all. Please rebase.
Keywords: checkin-needed
Yeah, mounir was all over nsHTMLInputElement just now...
(Assignee)

Comment 34

6 years ago
Created attachment 730741 [details] [diff] [review]
part 1 - renaming
Attachment #728196 - Attachment is obsolete: true
(Assignee)

Comment 35

6 years ago
Created attachment 730742 [details] [diff] [review]
part 2 - webidl
Attachment #730375 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3ffdb788846d
https://hg.mozilla.org/mozilla-central/rev/23850987c825
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
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).
Keywords: dev-doc-complete → dev-doc-needed
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 :(
Keywords: dev-doc-needed
No problem, I much appreciate you work!
Depends on: 856611
You need to log in before you can comment on or make changes to this bug.