Bug 48177 - Expose sendChangeEvent parameter in WebInputElement::setValue() API
Summary: Expose sendChangeEvent parameter in WebInputElement::setValue() API
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Ilya Sherman
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-10-23 01:38 PDT by Ilya Sherman
Modified: 2010-11-02 13:30 PDT (History)
9 users (show)

See Also:


Attachments
Patch (2.21 KB, patch)
2010-10-23 01:52 PDT, Ilya Sherman
no flags Details | Formatted Diff | Diff
Patch (2.22 KB, patch)
2010-10-23 01:59 PDT, Ilya Sherman
no flags Details | Formatted Diff | Diff
Addresses Darin's review comments (2.16 KB, patch)
2010-11-01 13:27 PDT, Ilya Sherman
no flags Details | Formatted Diff | Diff
Patch (2.17 KB, patch)
2010-11-01 14:50 PDT, Ilya Sherman
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ilya Sherman 2010-10-23 01:38:12 PDT
The Chromium API is missing the second parameter for WebInputElement::setValue(), which is a boolean value controlling whether or not to send an onChange() JS event.  We want to be able to send the onChange() event for form autofill; but this is not possible with the current API.
Comment 1 Ilya Sherman 2010-10-23 01:52:35 PDT
Created attachment 71630 [details]
Patch
Comment 2 Ilya Sherman 2010-10-23 01:56:12 PDT
My thinking is to have both versions of setValue() temporarily.  Once Chromium code is updated (which I will do once this patch lands), I'll post a new WebKit patch to remove the single-parameter version of setValue().
Comment 3 WebKit Review Bot 2010-10-23 01:56:22 PDT
Attachment 71630 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/chromium/ChangeLog:6:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Ilya Sherman 2010-10-23 01:59:10 PDT
Created attachment 71631 [details]
Patch
Comment 5 Kent Tamura 2010-10-28 18:52:17 PDT
Comment on attachment 71631 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=71631&action=review

> WebKit/chromium/public/WebInputElement.h:69
> +        WEBKIT_API void setValue(const WebString&); // TODO(isherman): Remove once chromium code is updated.

We use some default arguments in WebKit API.

WebKit/chromium/public/WebCursorInfo.h:    explicit WebCursorInfo(Type type = TypePointer)
WebKit/chromium/public/WebFrame.h:                          bool replace = false) = 0;
WebKit/chromium/public/WebFrame.h:                                bool replace = false) = 0; 
WebKit/chromium/public/WebFrame.h:    virtual int printBegin(const WebSize& pageSize, int printerDPI = 72,
WebKit/chromium/public/WebFrame.h:                           bool* useBrowserOverlays = 0) = 0;
WebKit/chromium/public/WebInputEvent.h:    WebKeyboardEvent(unsigned sizeParam = sizeof(WebKeyboardEvent))
WebKit/chromium/public/WebInputEvent.h:    WebMouseEvent(unsigned sizeParam = sizeof(WebMouseEvent))
WebKit/chromium/public/WebInputEvent.h:    WebMouseWheelEvent(unsigned sizeParam = sizeof(WebMouseWheelEvent))
WebKit/chromium/public/WebInputEvent.h:    WebTouchEvent(unsigned sizeParam = sizeof(WebTouchEvent))
WebKit/chromium/public/WebVector.h:    explicit WebVector(size_t size = 0)

It might be ok to use default argument though Chromium style doesn't allow it.  I'm not 100% sure.
Comment 6 Ilya Sherman 2010-10-28 23:03:48 PDT
(In reply to comment #5)
> (From update of attachment 71631 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=71631&action=review
> 
> > WebKit/chromium/public/WebInputElement.h:69
> > +        WEBKIT_API void setValue(const WebString&); // TODO(isherman): Remove once chromium code is updated.
> 
> We use some default arguments in WebKit API.
> It might be ok to use default argument though Chromium style doesn't allow it.  I'm not 100% sure.

I'm not really opposed to using a default argument here -- I was just trying to stay consistent with Chromium style =)

What say ye, wise guardians of the Chromium WebKit API?
Comment 7 Darin Fisher (:fishd, Google) 2010-11-01 13:09:28 PDT
Comment on attachment 71631 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=71631&action=review

> WebKit/chromium/public/WebInputElement.h:70
> +        WEBKIT_API void setValue(const WebString&, bool);

yes, please use a default argument.  also, please give this "bool" parameter a name.
Comment 8 Ilya Sherman 2010-11-01 13:27:06 PDT
Created attachment 72547 [details]
Addresses Darin's review comments

Done and done =)
Comment 9 Kent Tamura 2010-11-01 14:14:22 PDT
Comment on attachment 72547 [details]
Addresses Darin's review comments

View in context: https://bugs.webkit.org/attachment.cgi?id=72547&action=review

> WebKit/chromium/ChangeLog:5
> +        Expose |sendChangeEvent| parameter in WebInputElement::setValue() API,

nit: we don't enclose a parameter name with || in WebKit style.
Comment 10 Ilya Sherman 2010-11-01 14:50:33 PDT
Created attachment 72567 [details]
Patch
Comment 11 WebKit Commit Bot 2010-11-02 13:11:50 PDT
The commit-queue encountered the following flaky tests while processing attachment 72567 [details]:

http/tests/appcache/fail-on-update-2.html
animations/suspend-resume-animation.html

Please file bugs against the tests.  These tests were authored by ap@webkit.org and cmarrin@apple.com.  The commit-queue is continuing to process your patch.
Comment 12 WebKit Commit Bot 2010-11-02 13:30:48 PDT
Comment on attachment 72567 [details]
Patch

Clearing flags on attachment: 72567

Committed r71158: <http://trac.webkit.org/changeset/71158>
Comment 13 WebKit Commit Bot 2010-11-02 13:30:55 PDT
All reviewed patches have been landed.  Closing bug.