Bug 88239 – Move setAutofilled from LayoutTestController to window.internals()

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: Tools / Tests

[Jesus Sanchez-Palencia]

Implement setAutofilled in window.internals, avoiding duplicated code in WebKitTestRunner and DumpRenderTree. This will also unskip tests for WKTR.

[Jesus Sanchez-Palencia]

Created attachment 145628 [details]
WIP patch for ews testing

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[James Robinson]

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.

[Build Bot]

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

[Alexey Proskuryakov]

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.

[Jesus Sanchez-Palencia]

(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!

[Jesus Sanchez-Palencia]

(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?

[Jesus Sanchez-Palencia]

Created attachment 145790 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 145790 [details]
Patch

LGTM.  I look forward to more awesome patches like this!

[WebKit Review Bot]

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

[James Robinson]

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

[Jesus Sanchez-Palencia]

(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.

[Daniel Bates]

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