Bug 184357 – [WPE] Switch testing process to using WPEBackend-fdo

• Component: New Bugs
• Status: RESOLVED FIXED

[Zan Dobersek]

[WPE] Switch testing process to using WPEBackend-fdo

[Zan Dobersek]

Created attachment 337357 [details]
Patch

[EWS Watchlist]

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


ERROR: Tools/wpe/HeadlessViewBackend/HeadlessViewBackend.cpp:223:  The parameter name "!" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 10 files


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

[Zan Dobersek]

Created attachment 337359 [details]
Patch

[EWS Watchlist]

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


ERROR: Tools/wpe/HeadlessViewBackend/HeadlessViewBackend.cpp:223:  The parameter name "!" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 11 files


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

[Carlos Garcia Campos]

Comment on attachment 337359 [details]
Patch

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

> Tools/Scripts/webkitpy/port/wpe.py:-71
> -        return HeadlessDriver

Should we remove the headless driver then? or is it used somewhere else?

> Tools/Scripts/webkitpy/port/wpe.py:69
> +        return WaylandDriver

I find confusing that wayland driver is used unconditionally but the headless backend is now used unconditionally too.

[Carlos Garcia Campos]

Comment on attachment 337359 [details]
Patch

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

> Tools/TestWebKitAPI/glib/WebKitGLib/TestMain.h:-146
> -        const char* useHeadlessViewBackend = g_getenv("WPE_USE_HEADLESS_VIEW_BACKEND");

I guess we should also stop setting this variable in the scripts to run api and layout tests.

[Carlos Alberto Lopez Perez]

Comment on attachment 337359 [details]
Patch

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

Setting r- because wpebackend-fdo still doesn't handle input and its not possible to use a keyboard or a mouse with dyz, which is needed for testing.

>> Tools/Scripts/webkitpy/port/wpe.py:69
>> +        return WaylandDriver
> 
> I find confusing that wayland driver is used unconditionally but the headless backend is now used unconditionally too.

I also find this confusing.
I think the HeadlessDriver should inherit from WaylandDriver but keep its name as headless.
And ideally the WaylandDriver would work also (if specified via command-line argument), but the screen output will appear on the screen

[Zan Dobersek]

Created attachment 337685 [details]
Patch

[EWS Watchlist]

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


ERROR: Tools/wpe/HeadlessViewBackend/HeadlessViewBackend.cpp:223:  The parameter name "!" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 12 files


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

[Carlos Alberto Lopez Perez]

Comment on attachment 337685 [details]
Patch

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

> Tools/Scripts/webkitdirs.pm:1039
>      if (isWPE()) {
> -        return "$configurationProductDir/lib/libWPEWebKit.so";
> +        return "$configurationProductDir/lib/libWPEWebKit-0.1.so";

There is an issue with this change: it breaks run-minibrowser script: Can't find built framework at "/home/clopez/webkit/wpe/WebKitBuild/Release/lib/libWPEWebKit-0.1.so".

I think leaving it here pointing to the generic .so symlink is the right thing to do IMHO, because that way it won't break when the soname changes. In the run-minibrowser script we only want to check that the framework has been built, checking the generic .so is enough for that.


But then, If I revert that and try to run dyz I still get this error:

lib/wpewebkit_glibapi.lua:3: libWPEWebKit-0.1.so: cannot open shared object file: No such file or directory

This seems to be caused by dyz commit https://github.com/Igalia/dyz/commit/e13c39fb6c225f7c464c772df071d04018881c4e

The installable version of the library is versioned 1.0

$ ls -l WebKitBuild/Release/lib/libWPEWebKit.so.*
lrwxrwxrwx 1 clopez clopez       21 Apr 11 13:45 WebKitBuild/Release/lib/libWPEWebKit.so.1 -> libWPEWebKit.so.1.0.0
-rwxr-xr-x 1 clopez clopez 86654672 Apr 11 13:45 WebKitBuild/Release/lib/libWPEWebKit.so.1.0.0


Why it has been changed to 0.1 in dyz without changing also it on wpe as well?

[Zan Dobersek]

Comment on attachment 337685 [details]
Patch

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

>> Tools/Scripts/webkitdirs.pm:1039
>> +        return "$configurationProductDir/lib/libWPEWebKit-0.1.so";
> 
> There is an issue with this change: it breaks run-minibrowser script: Can't find built framework at "/home/clopez/webkit/wpe/WebKitBuild/Release/lib/libWPEWebKit-0.1.so".
> 
> I think leaving it here pointing to the generic .so symlink is the right thing to do IMHO, because that way it won't break when the soname changes. In the run-minibrowser script we only want to check that the framework has been built, checking the generic .so is enough for that.
> 
> 
> But then, If I revert that and try to run dyz I still get this error:
> 
> lib/wpewebkit_glibapi.lua:3: libWPEWebKit-0.1.so: cannot open shared object file: No such file or directory
> 
> This seems to be caused by dyz commit https://github.com/Igalia/dyz/commit/e13c39fb6c225f7c464c772df071d04018881c4e
> 
> The installable version of the library is versioned 1.0
> 
> $ ls -l WebKitBuild/Release/lib/libWPEWebKit.so.*
> lrwxrwxrwx 1 clopez clopez       21 Apr 11 13:45 WebKitBuild/Release/lib/libWPEWebKit.so.1 -> libWPEWebKit.so.1.0.0
> -rwxr-xr-x 1 clopez clopez 86654672 Apr 11 13:45 WebKitBuild/Release/lib/libWPEWebKit.so.1.0.0
> 
> 
> Why it has been changed to 0.1 in dyz without changing also it on wpe as well?

This already landed:
https://bugs.webkit.org/show_bug.cgi?id=180608

[Carlos Alberto Lopez Perez]

(In reply to Zan Dobersek from comment #11)
> Comment on attachment 337685 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=337685&action=review
> 
> >> Tools/Scripts/webkitdirs.pm:1039
> >> +        return "$configurationProductDir/lib/libWPEWebKit-0.1.so";
> > 
> > There is an issue with this change: it breaks run-minibrowser script: Can't find built framework at "/home/clopez/webkit/wpe/WebKitBuild/Release/lib/libWPEWebKit-0.1.so".
> > 
> > I think leaving it here pointing to the generic .so symlink is the right thing to do IMHO, because that way it won't break when the soname changes. In the run-minibrowser script we only want to check that the framework has been built, checking the generic .so is enough for that.
> > 
> > 
> > But then, If I revert that and try to run dyz I still get this error:
> > 
> > lib/wpewebkit_glibapi.lua:3: libWPEWebKit-0.1.so: cannot open shared object file: No such file or directory
> > 
> > This seems to be caused by dyz commit https://github.com/Igalia/dyz/commit/e13c39fb6c225f7c464c772df071d04018881c4e
> > 
> > The installable version of the library is versioned 1.0
> > 
> > $ ls -l WebKitBuild/Release/lib/libWPEWebKit.so.*
> > lrwxrwxrwx 1 clopez clopez       21 Apr 11 13:45 WebKitBuild/Release/lib/libWPEWebKit.so.1 -> libWPEWebKit.so.1.0.0
> > -rwxr-xr-x 1 clopez clopez 86654672 Apr 11 13:45 WebKitBuild/Release/lib/libWPEWebKit.so.1.0.0
> > 
> > 
> > Why it has been changed to 0.1 in dyz without changing also it on wpe as well?
> 
> This already landed:
> https://bugs.webkit.org/show_bug.cgi?id=180608

aggh... i missed that .. having different repos pointing to each one is a mess when updating, my fault...

But then, that also means that the wpe run-minibrowser script has been broken since then :(

[Carlos Alberto Lopez Perez]

Comment on attachment 337685 [details]
Patch

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

both dyz and layout-tests work fine for me now and the patch is fine, so r+

> Tools/Scripts/webkitpy/port/wpe.py:69
> -        if self._display_server == "wayland":
> -            return WaylandDriver
> -        return HeadlessDriver
> +        return WaylandDriver

I think we need to implement some function like gtk.py:check_sys_deps() so it aborts early if no wayland env is detected, but we can do that in a another patch
I also keep thinking the name is not the best, we are reusing the wayland driver for a headless driver that runs inside a wayland env, but ok for now

[Michael Catanzaro]

Comment on attachment 337685 [details]
Patch

I believe this breaks all the WebKitWebView constructors, which guarantee that a default backend is used when passed NULL for the WebKitWebViewBackend parameter. Correct?

So we need to change all five of the WebKitWebView constructors. For example, we would change:

 * webkit_web_view_new_with_related_view: (constructor)
 * @backend: (nullable) (transfer full): a #WebKitWebViewBackend, or %NULL to use the default
 * @web_view: the related #WebKitWebView

to:

 * webkit_web_view_new_with_related_view: (constructor)
 * @backend: (transfer full): a #WebKitWebViewBackend
 * @web_view: the related #WebKitWebView

dropping the (nullable), and then add a corresponding g_return_val_if_fail:

    g_return_val_if_fail(WEBKIT_IS_WEB_VIEW_BACKEND(backend, nullptr);
    g_return_val_if_fail(WEBKIT_IS_WEB_VIEW(webView), nullptr);

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #14)
> Comment on attachment 337685 [details]
> Patch
> 
> I believe this breaks all the WebKitWebView constructors, which guarantee
> that a default backend is used when passed NULL for the WebKitWebViewBackend
> parameter. Correct?

OK, this particular change does not itself break anything, but the implication of this change is that we will change WPEBackend-fdo to install the WPEBackend-default symlink. (Otherwise, we should not be changing the test infrastructure to test it.) And changing the default backend will break these APIs.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #14)
> Comment on attachment 337685 [details]
> Patch
> 
> I believe this breaks all the WebKitWebView constructors, which guarantee
> that a default backend is used when passed NULL for the WebKitWebViewBackend
> parameter. Correct?

You fixed this in bug #184513, thanks. Feel free to land this with Carlos Lopez's name as reviewer.

[Zan Dobersek]

Created attachment 337775 [details]
Patch for landing

[EWS Watchlist]

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


ERROR: Tools/wpe/HeadlessViewBackend/HeadlessViewBackend.cpp:223:  The parameter name "!" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 12 files


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

[Zan Dobersek]

Comment on attachment 337775 [details]
Patch for landing

Clearing flags on attachment: 337775

Committed r230562: <https://trac.webkit.org/changeset/230562>

[Zan Dobersek]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/39374557>