Bug 38344 – Remove custom bindings for HTMLInputElement

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Adam Barth]

Remove custom bindings for HTMLInputElement

[Adam Barth]

Created attachment 54732 [details]
Patch

[WebKit Review Bot]

Attachment 54732 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:440:  Extra space before ( in function call  [whitespace/parens] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:456:  Extra space before ( in function call  [whitespace/parens] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:481:  Extra space before ( in function call  [whitespace/parens] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:591:  Extra space before )  [whitespace/parens] [2]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:597:  Extra space before )  [whitespace/parens] [2]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.h:164:  Extra space before ( in function call  [whitespace/parens] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.h:167:  Extra space before ( in function call  [whitespace/parens] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.h:173:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 8 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yaar Schnitman]

That's better.

[Eric Seidel (no email)]

Comment on attachment 54732 [details]
Patch

I think HTMLInputElement.h needs a comment as to why we're adding these.  But I think this looks OK.

Should we be be making selectionStart, etc. virtual and overriding them in HTMLIntputElement as private members?  Should we change the baseclass to take an ExceptionCode and only throw it for HTMLInputElement?

I think this is OK as is, but I worry that folks could easily use the wrong one.

[Darin Adler]

Comment on attachment 54732 [details]
Patch

Throwing an exception when previously we would silently do nothing could easily be a backward compatibility problem for the Objective-C bindings.

> +int HTMLInputElement::selectionStart(ExceptionCode& ec)
> +{
> +    if (!canHaveSelection()) {
> +        ec = TYPE_MISMATCH_ERR;
> +        return -1;
> +    }

I think this gives a different exception than before. Probably the new one is correct and the old was incorrect, but since it's a behavior change I'd like to see tests showing the behavior.

[Adam Barth]

We should be able to have the ObjC bindings call the non-ec version of the method.  I'm not sure why that didn't work.

[Eric Seidel (no email)]

Attachment 54732 [details] was posted by a committer and has review+, assigning to Adam Barth for commit.

[Adam Barth]

Committed r59502: <http://trac.webkit.org/changeset/59502>

[Adam Barth]

I landed the TestObj.idl changes in http://trac.webkit.org/changeset/59502.  I'm going to post a new patch that addresses Darin's feedback shortly.

[Adam Barth]

Created attachment 56139 [details]
work in progress

[Adam Barth]

Created attachment 56141 [details]
Patch

[Darin Adler]

Comment on attachment 56141 [details]
Patch

Good to avoid custom bindings.

Awkward to have these two versions of these basic functions side by side. The only other place I can think of where we have that is the Range class. Note the different way these functions are declared in that class compared to what we have in this patch. I think I like the Range style slightly better.

> +    // The methods that take ExceptionCodes check canHaveSelection().

Methods is not a C++ term, and I normally avoid using it to mean member function in WebKit code. I would say "The following functions" or "These functions" rather than "The methods".

[Eric Seidel (no email)]

I'm confused why this is open?