Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(3)

Issue 2684433003: Files required by a service now listed in manifest. (Closed)

Created:
2 years, 7 months ago by Jay Civelli
Modified:
2 years, 7 months ago
CC:
Aaron Boodman, abarth-chromium, chromium-reviews, darin (slow to review), darin-cc_chromium.org, jam, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, vmpstr+watch_chromium.org, yzshen+watch_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Files required by a service now listed in manifest. The files required in a service process can now be listed in the service's manifest. In the manifest the file path is given with a string ID and a platform. The file is then opened in the parent process and passed to the child process, its ID is specified with a new command-line switch that associates the manifest specified key and a GlobalDescriptor ID. There are used to populate the new FileDescriptorStore which is similar to GlobalDescriptor but uses string as keys (which makes it easier to prevent ID collisions between multiple services). The service code can then retrieve the FD from the FileDescriptorStore. The goal is to eventually switch entirely to only using the FileDescriptorStore, and getting rid of the GlobalDescriptor. BUG=690189 Review-Url: https://codereview.chromium.org/2684433003 Cr-Commit-Position: refs/heads/master@{#451019} Committed: https://chromium.googlesource.com/chromium/src/+/dad0cefaebae56eb5ed984b62ccebee165723a6a

Patch Set 1 #

Patch Set 2 : Working on Android. #

Patch Set 3 : More clean-up. #

Total comments: 1

Patch Set 4 : Synced #

Patch Set 5 : Fixed analysis. #

Total comments: 34

Patch Set 6 : Addressed @rockot's comments #

Patch Set 7 : Making behavior Linux & Android specific for now. #

Patch Set 8 : Clean-up + Windows' build fix #

Total comments: 2

Patch Set 9 : Synced + addressed comment from @rockot #

Total comments: 27

Patch Set 10 : Addressed @boliu and @jam's comments. #

Total comments: 10

Patch Set 11 : More comments addressed. #

Patch Set 12 : Fix android build #

Patch Set 13 : Fixed NaCl tests #

Patch Set 14 : More build fixes. #

Patch Set 15 : Synced #

Total comments: 4

Patch Set 16 : More comments addressed and bots fixed. #

Total comments: 3

Patch Set 17 : Removed unused method in apk_assets #

Total comments: 40

Patch Set 18 : Addressed dchen@'s comments. #

Total comments: 10

Patch Set 19 : Addressed dcheng@'s comments. #

Patch Set 20 : Synced #

Patch Set 21 : Fix build. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+983 lines, -203 lines) Patch
M android_webview/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +1 line, -0 lines 0 comments Download
M android_webview/lib/main/aw_main_delegate.cc View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +10 lines, -10 lines 0 comments Download
M base/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 2 chunks +4 lines, -0 lines 0 comments Download
M base/android/apk_assets.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 2 chunks +6 lines, -5 lines 0 comments Download
M base/android/apk_assets.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 3 chunks +11 lines, -9 lines 0 comments Download
A base/file_descriptor_store.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +73 lines, -0 lines 0 comments Download
A base/file_descriptor_store.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +73 lines, -0 lines 0 comments Download
M base/posix/global_descriptors.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 3 chunks +10 lines, -0 lines 0 comments Download
M base/posix/global_descriptors.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +16 lines, -0 lines 0 comments Download
M content/app/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +1 line, -0 lines 0 comments Download
M content/app/DEPS View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +1 line, -0 lines 0 comments Download
M content/app/android/child_process_service_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 3 chunks +47 lines, -8 lines 0 comments Download
M content/app/content_main_runner.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 8 chunks +45 lines, -30 lines 0 comments Download
M content/browser/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +2 lines, -0 lines 0 comments Download
M content/browser/child_process_launcher.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +6 lines, -0 lines 0 comments Download
M content/browser/child_process_launcher.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +8 lines, -0 lines 0 comments Download
M content/browser/child_process_launcher_helper.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 2 chunks +5 lines, -0 lines 0 comments Download
M content/browser/child_process_launcher_helper_android.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 3 chunks +20 lines, -24 lines 0 comments Download
M content/browser/child_process_launcher_helper_linux.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 3 chunks +23 lines, -32 lines 0 comments Download
M content/browser/child_process_launcher_helper_mac.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 3 chunks +22 lines, -2 lines 0 comments Download
M content/browser/child_process_launcher_helper_posix.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 2 chunks +20 lines, -2 lines 0 comments Download
M content/browser/child_process_launcher_helper_posix.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 2 chunks +105 lines, -3 lines 0 comments Download
M content/browser/child_process_launcher_helper_win.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 2 chunks +9 lines, -0 lines 0 comments Download
M content/browser/service_manager/service_manager_context.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 3 chunks +9 lines, -0 lines 0 comments Download
M content/gpu/gpu_main.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 2 chunks +1 line, -1 line 0 comments Download
M content/ppapi_plugin/ppapi_plugin_main.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 2 chunks +1 line, -1 line 0 comments Download
M content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 2 chunks +16 lines, -6 lines 0 comments Download
M content/public/app/mojo/content_renderer_manifest.json View 1 2 3 4 5 1 chunk +24 lines, -0 lines 0 comments Download
M content/public/app/mojo/content_utility_manifest.json View 1 2 3 4 5 1 chunk +24 lines, -0 lines 0 comments Download
M content/public/common/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 2 chunks +9 lines, -0 lines 0 comments Download
A content/public/common/content_descriptor_keys.h View 1 1 chunk +24 lines, -0 lines 0 comments Download
A content/public/common/content_descriptor_keys.cc View 1 2 3 4 5 1 chunk +17 lines, -0 lines 0 comments Download
M content/public/common/content_descriptors.h View 1 1 chunk +4 lines, -10 lines 0 comments Download
M content/public/common/content_switches.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 2 chunks +1 line, -2 lines 0 comments Download
M content/public/common/content_switches.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 2 chunks +5 lines, -8 lines 0 comments Download
M content/utility/utility_main.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +4 lines, -1 line 0 comments Download
M services/catalog/BUILD.gn View 1 2 3 4 5 2 chunks +9 lines, -2 lines 0 comments Download
A services/catalog/data/required_files View 1 1 chunk +17 lines, -0 lines 0 comments Download
M services/catalog/entry.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 3 chunks +6 lines, -3 lines 0 comments Download
M services/catalog/entry.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 4 chunks +14 lines, -32 lines 0 comments Download
M services/catalog/entry_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +35 lines, -11 lines 0 comments Download
M services/catalog/public/cpp/BUILD.gn View 1 2 3 4 5 2 chunks +3 lines, -0 lines 0 comments Download
A services/catalog/public/cpp/manifest_parsing_util.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +29 lines, -0 lines 0 comments Download
A services/catalog/public/cpp/manifest_parsing_util.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +107 lines, -0 lines 0 comments Download
M services/catalog/store.h View 1 chunk +10 lines, -0 lines 0 comments Download
M services/catalog/store.cc View 1 chunk +14 lines, -1 line 0 comments Download
M services/service_manager/background/BUILD.gn View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download
M services/service_manager/public/cpp/BUILD.gn View 1 2 3 1 chunk +2 lines, -0 lines 0 comments Download
A services/service_manager/public/cpp/lib/shared_file_util.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +49 lines, -0 lines 0 comments Download
A services/service_manager/public/cpp/shared_file_util.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +30 lines, -0 lines 0 comments Download

Messages

Total messages: 86 (58 generated)
Jay Civelli
https://codereview.chromium.org/2684433003/diff/40001/services/catalog/entry.cc File services/catalog/entry.cc (left): https://codereview.chromium.org/2684433003/diff/40001/services/catalog/entry.cc#oldcode112 services/catalog/entry.cc:112: std::unique_ptr<base::DictionaryValue> Entry::Serialize() const { Removed as unused.
2 years, 7 months ago (2017-02-08 22:52:27 UTC) #7
Ken Rockot(use gerrit already)
https://codereview.chromium.org/2684433003/diff/80001/base/file_descriptor_store.cc File base/file_descriptor_store.cc (right): https://codereview.chromium.org/2684433003/diff/80001/base/file_descriptor_store.cc#newcode25 base/file_descriptor_store.cc:25: typedef Singleton<base::FileDescriptorStore, Not sure Singleton is necessary, though I ...
2 years, 7 months ago (2017-02-09 17:17:28 UTC) #15
Jay Civelli
I moved the actual mapping to the Posix specific ChildProcessLauncherHelper for now as more work ...
2 years, 7 months ago (2017-02-09 22:08:27 UTC) #18
Ken Rockot(use gerrit already)
lgtm https://codereview.chromium.org/2684433003/diff/140001/content/browser/child_process_launcher_helper_posix.cc File content/browser/child_process_launcher_helper_posix.cc (right): https://codereview.chromium.org/2684433003/diff/140001/content/browser/child_process_launcher_helper_posix.cc#newcode123 content/browser/child_process_launcher_helper_posix.cc:123: std::map<std::string, std::string>& service_name_resolver = nit: move this lazy ...
2 years, 7 months ago (2017-02-09 22:46:13 UTC) #21
Jay Civelli
Adding OWNERS reviewers: @boliu, could you look at android_webview/? @jam, could you look at content/? ...
2 years, 7 months ago (2017-02-09 23:27:00 UTC) #25
boliu
looked at android part of content, since I supposedly own that as well Properly format ...
2 years, 7 months ago (2017-02-09 23:59:55 UTC) #28
jam
I defer to boliu for android files inside content what about windows? https://codereview.chromium.org/2684433003/diff/160001/content/app/content_main_runner.cc File content/app/content_main_runner.cc ...
2 years, 7 months ago (2017-02-10 00:44:52 UTC) #30
Jay Civelli
https://codereview.chromium.org/2684433003/diff/160001/content/app/android/child_process_service_impl.cc File content/app/android/child_process_service_impl.cc (right): https://codereview.chromium.org/2684433003/diff/160001/content/app/android/child_process_service_impl.cc#newcode131 content/app/android/child_process_service_impl.cc:131: if (shared_param.obj() == nullptr) On 2017/02/09 23:59:54, boliu wrote: ...
2 years, 7 months ago (2017-02-10 06:32:34 UTC) #32
dcheng
https://codereview.chromium.org/2684433003/diff/160001/base/android/apk_assets.h File base/android/apk_assets.h (right): https://codereview.chromium.org/2684433003/diff/160001/base/android/apk_assets.h#newcode39 base/android/apk_assets.h:39: const std::string& file_path); This should be a FilePath (unless ...
2 years, 7 months ago (2017-02-10 08:44:15 UTC) #36
boliu
android_webview and android under content lg2m % comments, but I'll wait until base reviews gets ...
2 years, 7 months ago (2017-02-10 21:33:03 UTC) #37
boliu
also, all android tests crashed.. so maybe this doesn't even work yet, but that's cq's ...
2 years, 7 months ago (2017-02-10 21:34:25 UTC) #38
nyquist
https://codereview.chromium.org/2684433003/diff/180001/base/android/apk_assets.cc File base/android/apk_assets.cc (right): https://codereview.chromium.org/2684433003/diff/180001/base/android/apk_assets.cc#newcode42 base/android/apk_assets.cc:42: return false; Should the function for global descriptors follow ...
2 years, 7 months ago (2017-02-10 23:37:50 UTC) #39
Jay Civelli
https://codereview.chromium.org/2684433003/diff/160001/base/android/apk_assets.h File base/android/apk_assets.h (right): https://codereview.chromium.org/2684433003/diff/160001/base/android/apk_assets.h#newcode39 base/android/apk_assets.h:39: const std::string& file_path); On 2017/02/10 08:44:15, dcheng wrote: > ...
2 years, 7 months ago (2017-02-13 18:48:26 UTC) #49
boliu
https://codereview.chromium.org/2684433003/diff/280001/content/public/android/java/src/org/chromium/content/common/ContentSwitches.java File content/public/android/java/src/org/chromium/content/common/ContentSwitches.java (right): https://codereview.chromium.org/2684433003/diff/280001/content/public/android/java/src/org/chromium/content/common/ContentSwitches.java#newcode82 content/public/android/java/src/org/chromium/content/common/ContentSwitches.java:82: public static final String SHARED_FILES = "shared-files"; this is ...
2 years, 7 months ago (2017-02-13 19:40:24 UTC) #55
jam
lgtm https://codereview.chromium.org/2684433003/diff/280001/content/app/content_main_runner.cc File content/app/content_main_runner.cc (right): https://codereview.chromium.org/2684433003/diff/280001/content/app/content_main_runner.cc#newcode292 content/app/content_main_runner.cc:292: void PopulateFDsFromCommandLine() { nit: move to existing posix ...
2 years, 7 months ago (2017-02-13 23:10:59 UTC) #58
Jay Civelli
https://codereview.chromium.org/2684433003/diff/280001/content/app/content_main_runner.cc File content/app/content_main_runner.cc (right): https://codereview.chromium.org/2684433003/diff/280001/content/app/content_main_runner.cc#newcode292 content/app/content_main_runner.cc:292: void PopulateFDsFromCommandLine() { On 2017/02/13 23:10:59, jam wrote: > ...
2 years, 7 months ago (2017-02-13 23:51:58 UTC) #60
nyquist
https://codereview.chromium.org/2684433003/diff/300001/base/android/apk_assets.h File base/android/apk_assets.h (right): https://codereview.chromium.org/2684433003/diff/300001/base/android/apk_assets.h#newcode34 base/android/apk_assets.h:34: const std::string& file_path); Well now it feels like this ...
2 years, 7 months ago (2017-02-14 05:46:07 UTC) #64
Jay Civelli
https://codereview.chromium.org/2684433003/diff/300001/base/android/apk_assets.h File base/android/apk_assets.h (right): https://codereview.chromium.org/2684433003/diff/300001/base/android/apk_assets.h#newcode34 base/android/apk_assets.h:34: const std::string& file_path); On 2017/02/14 05:46:06, nyquist wrote: > ...
2 years, 7 months ago (2017-02-14 17:46:18 UTC) #65
dcheng
Looks good over all, my comments are mostly nits (in particular, I think we can ...
2 years, 7 months ago (2017-02-15 08:05:28 UTC) #66
Jay Civelli
https://codereview.chromium.org/2684433003/diff/320001/base/file_descriptor_store.h File base/file_descriptor_store.h (right): https://codereview.chromium.org/2684433003/diff/320001/base/file_descriptor_store.h#newcode44 base/file_descriptor_store.h:44: // Gets a descriptor given a key and also ...
2 years, 7 months ago (2017-02-15 19:53:48 UTC) #67
dcheng
lgtm https://codereview.chromium.org/2684433003/diff/340001/content/app/content_main_runner.cc File content/app/content_main_runner.cc (right): https://codereview.chromium.org/2684433003/diff/340001/content/app/content_main_runner.cc#newcode274 content/app/content_main_runner.cc:274: for (const auto& descriptor : shared_file_descriptors.value()) { Shorter ...
2 years, 7 months ago (2017-02-15 22:01:03 UTC) #68
nyquist
base/android lgtm https://codereview.chromium.org/2684433003/diff/300001/base/android/apk_assets.h File base/android/apk_assets.h (right): https://codereview.chromium.org/2684433003/diff/300001/base/android/apk_assets.h#newcode34 base/android/apk_assets.h:34: const std::string& file_path); On 2017/02/14 17:46:18, Jay ...
2 years, 7 months ago (2017-02-15 22:01:40 UTC) #69
boliu
android_webview and android under content lgtm
2 years, 7 months ago (2017-02-15 22:23:32 UTC) #70
Jay Civelli
https://codereview.chromium.org/2684433003/diff/340001/content/app/content_main_runner.cc File content/app/content_main_runner.cc (right): https://codereview.chromium.org/2684433003/diff/340001/content/app/content_main_runner.cc#newcode274 content/app/content_main_runner.cc:274: for (const auto& descriptor : shared_file_descriptors.value()) { On 2017/02/15 ...
2 years, 7 months ago (2017-02-15 23:08:06 UTC) #71
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2684433003/380001
2 years, 7 months ago (2017-02-15 23:09:41 UTC) #74
commit-bot: I haz the power
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_android_rel_ng/builds/233388)
2 years, 7 months ago (2017-02-15 23:38:08 UTC) #76
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2684433003/400001
2 years, 7 months ago (2017-02-16 16:45:30 UTC) #83
commit-bot: I haz the power
2 years, 7 months ago (2017-02-16 18:39:44 UTC) #86
Message was sent while issue was closed.
Committed patchset #21 (id:400001) as
https://chromium.googlesource.com/chromium/src/+/dad0cefaebae56eb5ed984b62cce...

Powered by Google App Engine
This is Rietveld 408576698