Bug 88239

Created attachment 145628 [details] WIP patch for ews testing

Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

Comment on attachment 145628 [details] WIP patch for ews testing View in context: https://bugs.webkit.org/attachment.cgi?id=145628&action=review Moving the test hook to window.internals seems like a good thing, but Chromium at least will need to continue exposing this functionality via public API. I expect this to be true for other ports as well. > Source/WebKit/chromium/public/WebInputElement.h:92 > + // FIXME: This can be removed if there are no Chromium-internal calls to it. > + // See http://webkit.org/b/88239. > WEBKIT_EXPORT void setAutofilled(bool); This is incorrect - we call setAutofilled() via the public API and we will continue doing so. It's a generally useful thing to know about for any embedder who implements autofill to set. Also, some spurious newlines here. Please remove before landing.

Comment on attachment 145628 [details] WIP patch for ews testing Attachment 145628 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12906089

Comment on attachment 145628 [details] WIP patch for ews testing View in context: https://bugs.webkit.org/attachment.cgi?id=145628&action=review > Source/WebKit/mac/DOM/WebDOMOperationsPrivate.h:44 > +// FIXME: This can be removed if there are no Apple-internal calls to it. > +// See http://webkit.org/b/88239. > - (void)_setAutofilled:(BOOL)autofilled; This is in use, and can not be removed. I'd like the API layer to continue being tested, too.

(In reply to comment #3) Hi! > (From update of attachment 145628 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=145628&action=review > > Moving the test hook to window.internals seems like a good thing, but Chromium at least will need to continue exposing this functionality via public API. I expect this to be true for other ports as well. I'm not removing any private nor public APIs from any port, so I guess we are fine about this. I will remove the FIXMEs now that I know that these functions are actually used. > > > Source/WebKit/chromium/public/WebInputElement.h:92 > > + // FIXME: This can be removed if there are no Chromium-internal calls to it. > > + // See http://webkit.org/b/88239. > > WEBKIT_EXPORT void setAutofilled(bool); > > This is incorrect - we call setAutofilled() via the public API and we will continue doing so. It's a generally useful thing to know about for any embedder who implements autofill to set. > > Also, some spurious newlines here. Please remove before landing. Will do, thanks!

(In reply to comment #5) Hi! > (From update of attachment 145628 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=145628&action=review > > > Source/WebKit/mac/DOM/WebDOMOperationsPrivate.h:44 > > +// FIXME: This can be removed if there are no Apple-internal calls to it. > > +// See http://webkit.org/b/88239. > > - (void)_setAutofilled:(BOOL)autofilled; > > This is in use, and can not be removed. I'd like the API layer to continue being tested, too. I will remove the FIXMEs here as well, thanks. I don't really get why using DRT to test public API seems reasonable, but of course no API layer should be left untested. I can see that most of these "move from LTC to window.internals" will be blocked by this issue, so the way here will be writing API tests for Mac and Win?

Created attachment 145790 [details] Patch

Comment on attachment 145790 [details] Patch LGTM. I look forward to more awesome patches like this!

Comment on attachment 145790 [details] Patch Rejecting attachment 145790 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: g file LayoutTests/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patching file LayoutTests/fast/forms/input-autofilled.html patching file LayoutTests/fast/forms/reset-autofilled.html patching file LayoutTests/platform/wk2/Skipped Hunk #1 succeeded at 1001 (offset 2 lines). patching file ChangeLog Hunk #1 succeeded at 1 with fuzz 3. Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Eric Seidel']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue/ Full output: http://queues.webkit.org/results/13003205

I thought the consensus was not to move this one since it's testing important WebKit API on multiple ports.

(In reply to comment #9) > (From update of attachment 145790 [details]) > LGTM. I look forward to more awesome patches like this! :D Thanks for the review and feedback! (In reply to comment #11) > I thought the consensus was not to move this one since it's testing important WebKit API on multiple ports. I'm not that sure we've ever reached a consensus here, but I'm looking forward to it. Patch-wise, I will fix it on Monday because I was/am out of the office this week.

*** This bug has been marked as a duplicate of bug 110521 ***