Bug 207372

Created attachment 390042 [details]
WIP patch for early feedback. Still some work to do.

(In reply to Lauro Moura from comment #1)
> Created attachment 390042 [details]
> WIP patch for early feedback. Still some work to do.

Still to be tested with WebKitGTK+.

For imported/w3c/webdriver/tests/back/user_prompts.py, the patch goes from 1 fail/18 errors to 2 fails,  5 errors, 11 passes. But there is a significant number of timeouts, causing a full webdriver test suite run go from ~28min to ~58min in my machine. Maybe some more work needed in the dialog lifetime handling.

Summary of test outcome changes with this patch (baseline == r255912, current = This patch on top of baseline):

        BASELINE-> CURRENT
        ERROR   -> PASS   : 352
        ERROR   -> FAIL   : 143
        XFAIL   -> XPASS  : 40
        FAIL    -> PASS   : 29
        XFAIL   -> TIMEOUT: 21
        ERROR   -> XFAIL  : 1
        PASS    -> FAIL   : 1

With the patch from bug207529, the test run time lowers to 13~14min. And the results compared to upstream:

Summary of changes from baseline:
        ERROR   -> PASS   : 627
        ERROR   -> FAIL   : 125
        XFAIL   -> XPASS  : 65
        FAIL    -> PASS   : 34
        TIMEOUT -> PASS   : 1

- 181 unexpected failures
- 106 expected to fail but passed.

Will submit an updated version (rebased on top of bug207529) with prompt input support tomorrow.

Why do you need new signals? it seems to me it's duplicating the existing script dialogs API. I don't see the point on supporting this only to make WebDriver tests pass. The goal of WebDriver is to test the browser, so if the browser doesn't support script dialogs, why would we want to test them?

To lower the time spent running WebDriver tests in WPE just skip the tests that are timing out due to unimplemented commands or features.

Created attachment 390481 [details]
Alternative patch, without signals

(In reply to Carlos Garcia Campos from comment #5)
> To lower the time spent running WebDriver tests in WPE just skip the tests
> that are timing out due to unimplemented commands or features.

This should be dealt by bug207529, bringing the time close to GTK's.

(In reply to Carlos Garcia Campos from comment #4)
> Why do you need new signals? it seems to me it's duplicating the existing
> script dialogs API. I don't see the point on supporting this only to make
> WebDriver tests pass. The goal of WebDriver is to test the browser, so if
> the browser doesn't support script dialogs, why would we want to test them?

There is the scenario of allowing WPE-based browsers to support WebDriver tests, no? With the current API this does not seem possible. GTK seems to handle this by making the WebView completely handle the prompt on its own (with the DialogImpl class), while WPE currently relays only the dialog creation to the embedder, with the "script-dialog" signal.

Is there another way to relay the dialog commands to the WebView user?

The alternative patch I just submitted implements the reactions to the WebDriver dialog commands diretly into the callbacks in ScriptDialogWPE.cpp, with the browser implementer just having to keep the dialog ref alive from the "script-dialog" signal. (Like the patch does for the MiniBrowser).

While not optimal - this approach does not guarantee the end user would actually have a sound dialog implementation - at least could show the ScriptDialogs react corretly to the WebDriver commands, like some kind of "reference behavior".

Created attachment 393010 [details]
Rebased and with prompt text fixes

(In reply to Lauro Moura from comment #7)
> (In reply to Carlos Garcia Campos from comment #5)
> > To lower the time spent running WebDriver tests in WPE just skip the tests
> > that are timing out due to unimplemented commands or features.
> 
> This should be dealt by bug207529, bringing the time close to GTK's.
> 
> (In reply to Carlos Garcia Campos from comment #4)
> > Why do you need new signals? it seems to me it's duplicating the existing
> > script dialogs API. I don't see the point on supporting this only to make
> > WebDriver tests pass. The goal of WebDriver is to test the browser, so if
> > the browser doesn't support script dialogs, why would we want to test them?
> 
> There is the scenario of allowing WPE-based browsers to support WebDriver
> tests, no? With the current API this does not seem possible. GTK seems to
> handle this by making the WebView completely handle the prompt on its own
> (with the DialogImpl class), while WPE currently relays only the dialog
> creation to the embedder, with the "script-dialog" signal.
> 
> Is there another way to relay the dialog commands to the WebView user?
> 
> The alternative patch I just submitted implements the reactions to the
> WebDriver dialog commands diretly into the callbacks in ScriptDialogWPE.cpp,
> with the browser implementer just having to keep the dialog ref alive from
> the "script-dialog" signal. (Like the patch does for the MiniBrowser).

Having to connect to WebKitWebView::script-dialog only to call
webkit_script_dialog_ref() looks clunky to me. Why would that
magically implement something that WebDriver might use? If I was
an user of that API, my thought would be “this is silly, instead
of me having to write an empty signal hanler, why wouldn't WPE
WebKit handle it all itself?”.
 
> While not optimal - this approach does not guarantee the end user would
> actually have a sound dialog implementation - at least could show the
> ScriptDialogs react corretly to the WebDriver commands, like some kind of
> "reference behavior".

If we want to provide some default “headless” implementation which is
enough for WebDriver, what I would do is provide a default handler for
the WebKitWebView::script-dialog signal which checks the web view with webkit_web_view_is_controlled_by_automation() and does the call to
webkit_script_dialog_ref() itself—or we have a dialog implementation.

This would still allow users of the API to provide their own dialogs
by connecting to the ::script-dialog signal and override the default
implementation.

I wonder what Carlos thinks about this possibility :)

Created attachment 393940 [details]
Updated patch after aperez comments.

Updated the patch by keeping the dialog alive if the webview is automated instead of relying on the script-dialog event on the browser side.

Comment on attachment 393940 [details]
Updated patch after aperez comments.

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

This approach looks neater to me, I have just one comment more,
and unless somebody (and with “somebody” I am thinking Carlos
García) has concerns we will be ready to land this :)

> Source/WebKit/UIProcess/API/wpe/WebKitScriptDialogWPE.cpp:36
> +    webkit_script_dialog_close(dialog);

Here we should call webkit_script_dialog_unref(dialog) to avoid
leaking it. That will be enough because the only ref will be the
one added in the webkitWebViewScriptDialog() function, so the
object will reach a refcount of zero and the webkit_script_dialog()
is called during destruction.

> Source/WebKit/UIProcess/API/wpe/WebKitScriptDialogWPE.cpp:44
> +    webkit_script_dialog_close(dialog);

Same here, replace with a call to webkit_script_dialog_unref().

Created attachment 393980 [details]
Updated patch after using unref instead of close.

Comment on attachment 393980 [details]
Updated patch after using unref instead of close.

Thanks a lot for taking the time to iterate over the patch.

For me this is now ready for r+, but I want to have Carlos'
rubber-stamp here, as he's our WebDriver resident expert :D

Comment on attachment 393980 [details]
Updated patch after using unref instead of close.

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

> Source/WebKit/UIProcess/API/wpe/WebKitScriptDialogWPE.cpp:26
> +// As WPE has currently no public API to allow the browser to respond to these commands,

This is not true, WebKitWebView::script-dialog is available in WPE and WebKitScriptDialog is exposed too.

> Source/WebKit/UIProcess/API/wpe/WebKitScriptDialogWPE.cpp:36
> +    auto dialog_type = webkit_script_dialog_get_dialog_type(dialog);
> +    if (dialog_type == WEBKIT_SCRIPT_DIALOG_CONFIRM || dialog_type == WEBKIT_SCRIPT_DIALOG_BEFORE_UNLOAD_CONFIRM)
> +        webkit_script_dialog_confirm_set_confirmed(dialog, TRUE);
> +    // W3C WebDriver tests expect an empty string instead of a null one when the prompt is accepted.
> +    if (dialog_type == WEBKIT_SCRIPT_DIALOG_PROMPT && dialog->text.isNull())
> +        webkit_script_dialog_prompt_set_text(dialog, "");
> +    webkit_script_dialog_unref(dialog);

This is assuming the app hasn't handled the WebKitWebView::script-dialog. In GTK we use the WebKitScriptDialog::nativeDialog pointer to check if the dialog is the default one or noty. We would need something similar for WPE, I think.

(In reply to Carlos Garcia Campos from comment #14)
> Comment on attachment 393980 [details]
> Updated patch after using unref instead of close.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=393980&action=review
> 
> > Source/WebKit/UIProcess/API/wpe/WebKitScriptDialogWPE.cpp:26
> > +// As WPE has currently no public API to allow the browser to respond to these commands,
> 
> This is not true, WebKitWebView::script-dialog is available in WPE and
> WebKitScriptDialog is exposed too.

By "these" I meant the commands from webdriver to accept/dismiss/set text in the dialog, which ends up calling these (currently) empty callbacks. What about:

"As WPE currently does not have a public API to forward these WebDriver commands to the browser, we mimic the expected behavior in these callbacks like the browser would do."

> 
> > Source/WebKit/UIProcess/API/wpe/WebKitScriptDialogWPE.cpp:36
> > +    auto dialog_type = webkit_script_dialog_get_dialog_type(dialog);
> > +    if (dialog_type == WEBKIT_SCRIPT_DIALOG_CONFIRM || dialog_type == WEBKIT_SCRIPT_DIALOG_BEFORE_UNLOAD_CONFIRM)
> > +        webkit_script_dialog_confirm_set_confirmed(dialog, TRUE);
> > +    // W3C WebDriver tests expect an empty string instead of a null one when the prompt is accepted.
> > +    if (dialog_type == WEBKIT_SCRIPT_DIALOG_PROMPT && dialog->text.isNull())
> > +        webkit_script_dialog_prompt_set_text(dialog, "");
> > +    webkit_script_dialog_unref(dialog);
> 
> This is assuming the app hasn't handled the WebKitWebView::script-dialog. In
> GTK we use the WebKitScriptDialog::nativeDialog pointer to check if the
> dialog is the default one or noty. We would need something similar for WPE,
> I think.

But assuming the app handled the dialog (in the sense that it kept it reffed waiting for user interaction), how would the WebDriver command be delivered to it to accept/dismiss the command?

Comment on attachment 393980 [details]
Updated patch after using unref instead of close.

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

>>> Source/WebKit/UIProcess/API/wpe/WebKitScriptDialogWPE.cpp:26
>>> +// As WPE has currently no public API to allow the browser to respond to these commands,
>> 
>> This is not true, WebKitWebView::script-dialog is available in WPE and WebKitScriptDialog is exposed too.
> 
> By "these" I meant the commands from webdriver to accept/dismiss/set text in the dialog, which ends up calling these (currently) empty callbacks. What about:
> 
> "As WPE currently does not have a public API to forward these WebDriver commands to the browser, we mimic the expected behavior in these callbacks like the browser would do."

These callbacks should do nothing when the user has handled the script dialog signal.

>>> Source/WebKit/UIProcess/API/wpe/WebKitScriptDialogWPE.cpp:36
>>> +    webkit_script_dialog_unref(dialog);
>> 
>> This is assuming the app hasn't handled the WebKitWebView::script-dialog. In GTK we use the WebKitScriptDialog::nativeDialog pointer to check if the dialog is the default one or noty. We would need something similar for WPE, I think.
> 
> But assuming the app handled the dialog (in the sense that it kept it reffed waiting for user interaction), how would the WebDriver command be delivered to it to accept/dismiss the command?

Handling the dialog means reffing the dialog *and* returning TRUE from the signal to stop propagation. Then the application is expected to call webkit_script_dialog_confirm_set_confirmed/webkit_script_dialog_prompt_set_text and then webkit_script_dialog_close. In the case of WebDriver, the web automation session client accepts/confirms the dialog, and you are indeed right we need to handle the case of the signal being handled by the app. There are FIXMEs in the code for that:

// FIXME: Add API to ask the user in case default implementation is not being used.

(In reply to Carlos Garcia Campos from comment #16)
> Comment on attachment 393980 [details]

snip.

> Handling the dialog means reffing the dialog *and* returning TRUE from the
> signal to stop propagation. Then the application is expected to call
> webkit_script_dialog_confirm_set_confirmed/
> webkit_script_dialog_prompt_set_text and then webkit_script_dialog_close. In
> the case of WebDriver, the web automation session client accepts/confirms
> the dialog, and you are indeed right we need to handle the case of the
> signal being handled by the app. There are FIXMEs in the code for that:
> 
> // FIXME: Add API to ask the user in case default implementation is not
> being used.

So, to check if I'm getting right:

* Automation session callbacks with the FIXME:
    * `webkitWebViewIsShowingScriptDialog`
    * `webkitWebViewSetCurrentScriptDialogUserInput`
    * `webkitWebViewAcceptCurrentScriptDialog`
    * `webkitWebViewDismissCurrentScriptDialog`

* These are the callbacks that must somehow check whether the user
  handled the dialog in the app or we are using the default
  implementation (currently empty in WPE, nativeDialog in GTK)

* In the case of the user handling the dialog, these callbacks must
  notify him that he should take the action requested by the WebDriver.

* In the case of the default implementation being used, it would use
  the ones from this current patch for WPE and the already existing
  nativeDialog for GTK (both implemented in
  `WebKitScriptDialogGTK/WPE.cpp`).

Created attachment 394573 [details]
WIP New changes from last comment on top of current patch

This is a WIP patch based on my last comment, on top of the current patch (both to be merged later).

Basically it adds the user-handled checking function (with a WPE dialog flag and checking nativeDialog for GTK) and some signals that the user would use when making his custom dialog react to the WebDriver commands.

Would this be the direction? If so, in the next iteration I'll merge with the current patch.

Created attachment 394575 [details]
Sample modifications to MiniBrowser mimicking a custom dialog

And this is what would it look like to the browser implementer when enabling WebDriver dialog support with the new signals for custom dialog handling.

Created attachment 397826 [details]
Updated patch

Updated the patch by adding a function to check whether the dialog was user handled and only falling back to the default behavior if we're using this default impl. Also fixed the WPE accept handler to use the default text the user provided not text.

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

Committed r261370: <https://trac.webkit.org/changeset/261370>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 397826 [details].