Bug 165232 – [GTK][Wayland] Allow running the layout tests under a native Wayland environment.

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Carlos Alberto Lopez Perez]

In bug 156716 we implemented support for running the layout tests under a virtualized wayland environment (weston running on top of Xvfb).

We now want to allow running the tests in a native wayland environment (the user's wayland session). 

This feature is going to be used on the wayland test bot that we are going to deploy ASAP. We think running the tests on wayland with a hardware-backed OpenGL implementation makes more sense than use mesa's llvmpipe software rasterizer.

For Xorg, we do this by exporting the environment variable USE_NATIVE_XDISPLAY=1

I propose to do something similar here.

So:

* if you pass the flag --wayland to run-webkit-tests a wayland (virtualized) environment will be used
* if you pass the flag --wayland and also export USE_NATIVE_WAYLAND=1 then your native wayland environment will be used

I think this is consistent with the current behaviour for X11:

* if you don't pass any special flag, the default X11 (virtualized xvfb) will be used
* if you don't pass any special flag and you export USE_NATIVE_XDISPLAY=1 env var, then your native X11 session will be used.

[Carlos Alberto Lopez Perez]

Created attachment 295803 [details]
Patch

[Michael Catanzaro]

Comment on attachment 295803 [details]
Patch

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

> Tools/Scripts/webkitpy/port/gtk.py:58
> +        self._nativexorg = os.environ.get("USE_NATIVE_XDISPLAY") != None
> +        self._nativewayland = os.environ.get("USE_NATIVE_WAYLAND") != None

I think it would be much nicer to add a new flag than to use a new environment variable. We don't need to repeat the mistakes of the past just for the benefit of consistency with the old way of doing things. Passing a flag to the script is more transparent and all-around easier to use.

> Tools/Scripts/webkitpy/port/waylanddriver.py:59
> +        # Currently on WebKit2, there is no API for setting the application
> +        # cache directory. Each worker should have it's own and it should be
> +        # cleaned afterwards, so we set it to inside the temporary folder by
> +        # prepending XDG_CACHE_HOME with DUMPRENDERTREE_TEMP.
> +        driver_environment['XDG_CACHE_HOME'] = self._port.host.filesystem.join(str(self._driver_tempdir), 'appcache')

This comment is not correct, you can set the base-cache-dir property of WebKitWebsiteDataManager. But surely overriding $XDG_CACHE_HOME is the right thing to do here regardless?

[Carlos Alberto Lopez Perez]

(In reply to comment #2)
> Comment on attachment 295803 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=295803&action=review
> 
> > Tools/Scripts/webkitpy/port/gtk.py:58
> > +        self._nativexorg = os.environ.get("USE_NATIVE_XDISPLAY") != None
> > +        self._nativewayland = os.environ.get("USE_NATIVE_WAYLAND") != None
> 
> I think it would be much nicer to add a new flag than to use a new
> environment variable. We don't need to repeat the mistakes of the past just
> for the benefit of consistency with the old way of doing things. Passing a
> flag to the script is more transparent and all-around easier to use.
> 

You have convinced me.

I'm going to upload a new patch for review that removes all uses of tricky environment variables and instead introduces only one switch for selecting the display server.

--display-server=xvfb/xorg/wayland/weston (defaulting to xvfb)

This will work for the test scripts: run-gtk-tests, run-perf-tests and run-webkit-tests

I will also reconfigure (later, on another patch) our bots to pass the required flag when needed instead of using environment variables to trigger one driver or the other.

[Carlos Alberto Lopez Perez]

Created attachment 298368 [details]
Patch

[Michael Catanzaro]

Comment on attachment 298368 [details]
Patch

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

> Tools/Scripts/webkitpy/port/waylanddriver.py:55
> +        driver_environment['XDG_CACHE_HOME'] = self._port.host.filesystem.join(str(self._driver_tempdir), 'appcache')

Hm wait, I don't think appcache is a good name for this. That would be the subdirectory of our cache dir created by WebKit itself for web app persistence. http://www.w3schools.com/HTML/html5_app_cache.asp right?

[Carlos Alberto Lopez Perez]

(In reply to comment #5)
> Comment on attachment 298368 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=298368&action=review
> 
> > Tools/Scripts/webkitpy/port/waylanddriver.py:55
> > +        driver_environment['XDG_CACHE_HOME'] = self._port.host.filesystem.join(str(self._driver_tempdir), 'appcache')
> 
> Hm wait, I don't think appcache is a good name for this. That would be the
> subdirectory of our cache dir created by WebKit itself for web app
> persistence. http://www.w3schools.com/HTML/html5_app_cache.asp right?

I have copied this from all the other drivers. I'm unsure if this is still needed or not. But it feels safer in this patch to just replicate what the other drivers do regarding this.

If this variable is not needed anymore or needs a new name it should be fixed for all the drivers in another bug.

$ git grep XDG_CACHE_HOME Tools/Scripts/
Tools/Scripts/webkitpy/port/waylanddriver.py:        driver_environment['XDG_CACHE_HOME'] = self._port.host.filesystem.join(str(self._driver_tempdir), 'appcache')
Tools/Scripts/webkitpy/port/westondriver.py:        driver_environment['XDG_CACHE_HOME'] = self._ensure_driver_tmpdir_subdirectory('appcache')
Tools/Scripts/webkitpy/port/xorgdriver.py:        # prepending XDG_CACHE_HOME with DUMPRENDERTREE_TEMP.
Tools/Scripts/webkitpy/port/xorgdriver.py:        server_environment['XDG_CACHE_HOME'] = self._port.host.filesystem.join(str(self._driver_tempdir), 'appcache')
Tools/Scripts/webkitpy/port/xvfbdriver.py:        # prepending XDG_CACHE_HOME with DUMPRENDERTREE_TEMP.
Tools/Scripts/webkitpy/port/xvfbdriver.py:        environment['XDG_CACHE_HOME'] = self._port.host.filesystem.join(str(self._driver_tempdir), 'appcache')

[WebKit Commit Bot]

Comment on attachment 298368 [details]
Patch

Clearing flags on attachment: 298368

Committed r210523: <http://trac.webkit.org/changeset/210523>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Michael Catanzaro]

(In reply to comment #6)
> If this variable is not needed anymore or needs a new name it should be
> fixed for all the drivers in another bug.

Yeah, I think we should rename it from "appcache" to just "cache".

[Carlos Alberto Lopez Perez]

Committed r210547: <http://trac.webkit.org/changeset/210547>