Bug 192622 – [GTK][WPE] Fix forwarding webkit socket to flatpak sandbox

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Patrick Griffis]

Fixes sandbox working inside Flatpak.

[Patrick Griffis]

Created attachment 357120 [details]
[GTK][WPE] Fix forwarding webkit socket to flatpak sandbox

[Carlos Garcia Campos]

Comment on attachment 357120 [details]
[GTK][WPE] Fix forwarding webkit socket to flatpak sandbox

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

> Source/WebKit/ChangeLog:7
> +

Pleas explain here what the patch does and it fixes.

> Source/WebKit/UIProcess/Launcher/glib/FlatpakLauncher.cpp:32
>  

Remove this empty line.

> Source/WebKit/UIProcess/Launcher/glib/FlatpakLauncher.cpp:37
> +GRefPtr<GSubprocess> flatpakSpawn(GSubprocessLauncher* launcher, const WebKit::ProcessLauncher::LaunchOptions& launchOptions, char** argv, GError **error, int webkitSocket)

I would add the parameter before the error one, which is typically the last one. I would use childProcessSocket or something like that instead of webkitsocket.

[Patrick Griffis]

Created attachment 357126 [details]
Patch

[EWS Watchlist]

Comment on attachment 357126 [details]
Patch

Attachment 357126 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/10368946

New failing tests:
webanimations/leak-document-with-web-animation.html

[EWS Watchlist]

Created attachment 357140 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Patrick Griffis]

To state the obvious, failing test isn't related to changes.

[Michael Catanzaro]

Comment on attachment 357126 [details]
Patch

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

Thanks for fixing Carlos's suggestions. LGTM, only one nit.

> Source/WebKit/UIProcess/Launcher/glib/FlatpakLauncher.h:39
> +GRefPtr<GSubprocess> flatpakSpawn(GSubprocessLauncher*, const WebKit::ProcessLauncher::LaunchOptions&, char** argv, int, GError**);

You should name the int parameter here. We use names in header files when the name would add meaning. E.g. we don't name the GSubprocessLauncher launcher or the LaunchOptions launchOptions, since that doesn't help readability. But the int should be named childProcessSocket, because that does improve readability.

[Patrick Griffis]

Created attachment 357236 [details]
Patch

[Michael Catanzaro]

Comment on attachment 357236 [details]
Patch

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

> Source/WebKit/UIProcess/Launcher/glib/FlatpakLauncher.cpp:36
> +GRefPtr<GSubprocess> flatpakSpawn(GSubprocessLauncher* launcher, const WebKit::ProcessLauncher::LaunchOptions& launchOptions, char** argv, int childProcessSocket, GError **error)

Oops, sorry I missed this in the last patch, but it should be GError** error.

[Patrick Griffis]

(In reply to Michael Catanzaro from comment #9)
> Oops, sorry I missed this in the last patch, but it should be GError** error.

Style script: ERROR: Source/WebKit/UIProcess/Launcher/glib/FlatpakLauncher.h:39:  The parameter name "error" adds no information, so it should be removed.  [readability/parameter_name] [5]

[EWS]

Comment on attachment 357236 [details]
Patch

Rejecting attachment 357236 [details] from commit-queue.

[e-mail] does not have committer permissions according to https://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[Patrick Griffis]

(In reply to Michael Catanzaro from comment #9)
> Oops, sorry I missed this in the last patch, but it should be GError** error.

Oh nevermind, misunderstood and thought you were talking about the header.

[Patrick Griffis]

Created attachment 357311 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 357311 [details]
Patch

Clearing flags on attachment: 357311

Committed r239204: <https://trac.webkit.org/changeset/239204>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.