Bug 184357

Created attachment 337357 [details] Patch

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.

Created attachment 337359 [details] Patch

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.

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.

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.

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

Created attachment 337685 [details] Patch

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.

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?

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

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

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

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);

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

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

Created attachment 337775 [details] Patch for landing

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.

Comment on attachment 337775 [details] Patch for landing Clearing flags on attachment: 337775 Committed r230562: <https://trac.webkit.org/changeset/230562>

All reviewed patches have been landed. Closing bug.

<rdar://problem/39374557>