Bug 192622 - [GTK][WPE] Fix forwarding webkit socket to flatpak sandbox
Summary: [GTK][WPE] Fix forwarding webkit socket to flatpak sandbox
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-12-12 06:08 PST by Patrick Griffis
Modified: 2018-12-14 06:38 PST (History)
6 users (show)

See Also:


Attachments
[GTK][WPE] Fix forwarding webkit socket to flatpak sandbox (4.05 KB, patch)
2018-12-12 06:09 PST, Patrick Griffis
no flags Details | Formatted Diff | Diff
Patch (3.88 KB, patch)
2018-12-12 07:39 PST, Patrick Griffis
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews206 for win-future (12.79 MB, application/zip)
2018-12-12 09:33 PST, EWS Watchlist
no flags Details
Patch (3.88 KB, patch)
2018-12-13 09:47 PST, Patrick Griffis
no flags Details | Formatted Diff | Diff
Patch (3.88 KB, patch)
2018-12-14 06:00 PST, Patrick Griffis
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Patrick Griffis 2018-12-12 06:08:36 PST
Fixes sandbox working inside Flatpak.
Comment 1 Patrick Griffis 2018-12-12 06:09:55 PST
Created attachment 357120 [details]
[GTK][WPE] Fix forwarding webkit socket to flatpak sandbox
Comment 2 Carlos Garcia Campos 2018-12-12 06:50:36 PST
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.
Comment 3 Patrick Griffis 2018-12-12 07:39:52 PST
Created attachment 357126 [details]
Patch
Comment 4 EWS Watchlist 2018-12-12 09:33:31 PST
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
Comment 5 EWS Watchlist 2018-12-12 09:33:45 PST
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
Comment 6 Patrick Griffis 2018-12-12 10:25:13 PST
To state the obvious, failing test isn't related to changes.
Comment 7 Michael Catanzaro 2018-12-12 11:16:37 PST
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.
Comment 8 Patrick Griffis 2018-12-13 09:47:12 PST
Created attachment 357236 [details]
Patch
Comment 9 Michael Catanzaro 2018-12-13 11:08:54 PST
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.
Comment 10 Patrick Griffis 2018-12-14 05:57:38 PST
(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]
Comment 11 EWS 2018-12-14 05:58:24 PST
Comment on attachment 357236 [details]
Patch

Rejecting attachment 357236 [details] from commit-queue.

pgriffis@igalia.com 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.
Comment 12 Patrick Griffis 2018-12-14 05:59:31 PST
(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.
Comment 13 Patrick Griffis 2018-12-14 06:00:19 PST
Created attachment 357311 [details]
Patch
Comment 14 WebKit Commit Bot 2018-12-14 06:38:56 PST
Comment on attachment 357311 [details]
Patch

Clearing flags on attachment: 357311

Committed r239204: <https://trac.webkit.org/changeset/239204>
Comment 15 WebKit Commit Bot 2018-12-14 06:38:58 PST
All reviewed patches have been landed.  Closing bug.