Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2049843004:
Upstream: Renderers are running in WebAPKs. (Closed)
DescriptionUpstream: Renderers are running in WebAPKs.
This Cl includes the following things:
1. Introduce WebApkSandboxedProcessService which works the same as
Chrome's ChildProcessService, but loads Chrome's code via
Chrome's ClassLoader.
2. Move most of the functionality from ChildProcessService to
ChildProcessServiceImpl, so we could let WebApkSandboxedProcessService
load the ChildProcessServiceImpl class and create an Impl object which
loads Chrome's native libraries and creates renderer process. The
ChildProcessService can't be loaded directly through the same way, it
might because this class extends the Android Service class and
complicates things.
3. Rename child_process_service.h(.cc) to child_process_service_impl.h(.cc)
and updates the JNI register.
4. Pass in the class name of WebApkSandboxedProcessService via
ChildProcessCreationParams in WebApkActivity. So ChildProcessLauncher
knows the class name of the service to connect.
BUG=609122
Committed: https://crrev.com/3f9d474841d96c1cf75186190d20379b074eb8fb
Cr-Commit-Position: refs/heads/master@{#402026}
Patch Set 1 : #
Total comments: 40
Patch Set 2 : pkotwicz@'s comments. #
Total comments: 15
Patch Set 3 : Nits. #
Total comments: 26
Patch Set 4 : Clean up. #
Total comments: 3
Patch Set 5 : Rebase. #Patch Set 6 : Update comments. #
Total comments: 18
Patch Set 7 : Maria's comments and rebase. #
Total comments: 4
Patch Set 8 : Add a check. #
Total comments: 6
Patch Set 9 : Maria's comments. #
Total comments: 4
Patch Set 10 : Add SANDBOXED_SERVICES_NAME in WebAPK's AndroidManifest.xml. #Patch Set 11 #
Total comments: 6
Patch Set 12 : Always check command line flags in getNum(Class)ofService(). #
Total comments: 4
Messages
Total messages: 68 (29 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Description was changed from ========== Renderers are running in WebAPKs. Introduce WebapkSandboxedProcessService. BUG= ========== to ========== Renderers are running in WebAPKs. This Cl includes the follwoings: 1. Introduce WebApkSandboxedProcessService which works the same as Chrome's ChildProcessService, but load Chrome's code via BaseDexClassLoader. 2. Move most of the functionalities from ChildProcessService to ChildProcessServiceImpl, so we could let WebApkSandboxedProcessService load the ChildProcessServiceImpl class and create an Impl object which loads Chrome's native libraries and creates renderer process. The ChildProcessService can't be loaded through the same way, beacuse this class extends the Android Service class, so we simplify it by introducing the ChildProcessServiceImpl. 3. Rename child_process_service.h(.cc) to child_process_service_impl.h(.cc) and updates the JNI register. BUG=609122 ==========
Description was changed from ========== Renderers are running in WebAPKs. This Cl includes the follwoings: 1. Introduce WebApkSandboxedProcessService which works the same as Chrome's ChildProcessService, but load Chrome's code via BaseDexClassLoader. 2. Move most of the functionalities from ChildProcessService to ChildProcessServiceImpl, so we could let WebApkSandboxedProcessService load the ChildProcessServiceImpl class and create an Impl object which loads Chrome's native libraries and creates renderer process. The ChildProcessService can't be loaded through the same way, beacuse this class extends the Android Service class, so we simplify it by introducing the ChildProcessServiceImpl. 3. Rename child_process_service.h(.cc) to child_process_service_impl.h(.cc) and updates the JNI register. BUG=609122 ========== to ========== Renderers are running in WebAPKs. This Cl includes the follwoing things: 1. Introduce WebApkSandboxedProcessService which works the same as Chrome's ChildProcessService, but load Chrome's code via BaseDexClassLoader. 2. Move most of the functionalities from ChildProcessService to ChildProcessServiceImpl, so we could let WebApkSandboxedProcessService load the ChildProcessServiceImpl class and create an Impl object which loads Chrome's native libraries and creates renderer process. The ChildProcessService can't be loaded through the same way, beacuse this class extends the Android Service class, so we simplify it by introducing the ChildProcessServiceImpl. 3. Rename child_process_service.h(.cc) to child_process_service_impl.h(.cc) and updates the JNI register. BUG=609122 ==========
Description was changed from ========== Renderers are running in WebAPKs. This Cl includes the follwoing things: 1. Introduce WebApkSandboxedProcessService which works the same as Chrome's ChildProcessService, but load Chrome's code via BaseDexClassLoader. 2. Move most of the functionalities from ChildProcessService to ChildProcessServiceImpl, so we could let WebApkSandboxedProcessService load the ChildProcessServiceImpl class and create an Impl object which loads Chrome's native libraries and creates renderer process. The ChildProcessService can't be loaded through the same way, beacuse this class extends the Android Service class, so we simplify it by introducing the ChildProcessServiceImpl. 3. Rename child_process_service.h(.cc) to child_process_service_impl.h(.cc) and updates the JNI register. BUG=609122 ========== to ========== Renderers are running in WebAPKs. This Cl includes the following things: 1. Introduce WebApkSandboxedProcessService which works the same as Chrome's ChildProcessService, but load Chrome's code via BaseDexClassLoader. 2. Move most of the functionalities from ChildProcessService to ChildProcessServiceImpl, so we could let WebApkSandboxedProcessService load the ChildProcessServiceImpl class and create an Impl object which loads Chrome's native libraries and creates renderer process. The ChildProcessService can't be loaded through the same way, it might because this class extends the Android Service class and complicates things. 3. Rename child_process_service.h(.cc) to child_process_service_impl.h(.cc) and updates the JNI register. BUG=609122 ==========
Description was changed from ========== Renderers are running in WebAPKs. This Cl includes the following things: 1. Introduce WebApkSandboxedProcessService which works the same as Chrome's ChildProcessService, but load Chrome's code via BaseDexClassLoader. 2. Move most of the functionalities from ChildProcessService to ChildProcessServiceImpl, so we could let WebApkSandboxedProcessService load the ChildProcessServiceImpl class and create an Impl object which loads Chrome's native libraries and creates renderer process. The ChildProcessService can't be loaded through the same way, it might because this class extends the Android Service class and complicates things. 3. Rename child_process_service.h(.cc) to child_process_service_impl.h(.cc) and updates the JNI register. BUG=609122 ========== to ========== Renderers are running in WebAPKs. This Cl includes the following things: 1. Introduce WebApkSandboxedProcessService which works the same as Chrome's ChildProcessService, but load Chrome's code via BaseDexClassLoader. 2. Move most of the functionalities from ChildProcessService to ChildProcessServiceImpl, so we could let WebApkSandboxedProcessService load the ChildProcessServiceImpl class and create an Impl object which loads Chrome's native libraries and creates renderer process. The ChildProcessService can't be loaded directly through the same way, it might because this class extends the Android Service class and complicates things. 3. Rename child_process_service.h(.cc) to child_process_service_impl.h(.cc) and updates the JNI register. BUG=609122 ==========
Description was changed from ========== Renderers are running in WebAPKs. This Cl includes the following things: 1. Introduce WebApkSandboxedProcessService which works the same as Chrome's ChildProcessService, but load Chrome's code via BaseDexClassLoader. 2. Move most of the functionalities from ChildProcessService to ChildProcessServiceImpl, so we could let WebApkSandboxedProcessService load the ChildProcessServiceImpl class and create an Impl object which loads Chrome's native libraries and creates renderer process. The ChildProcessService can't be loaded directly through the same way, it might because this class extends the Android Service class and complicates things. 3. Rename child_process_service.h(.cc) to child_process_service_impl.h(.cc) and updates the JNI register. BUG=609122 ========== to ========== Renderers are running in WebAPKs. This Cl includes the following things: 1. Introduce WebApkSandboxedProcessService which works the same as Chrome's ChildProcessService, but load Chrome's code via BaseDexClassLoader. 2. Move most of the functionalities from ChildProcessService to ChildProcessServiceImpl, so we could let WebApkSandboxedProcessService load the ChildProcessServiceImpl class and create an Impl object which loads Chrome's native libraries and creates renderer process. The ChildProcessService can't be loaded directly through the same way, it might because this class extends the Android Service class and complicates things. 3. Rename child_process_service.h(.cc) to child_process_service_impl.h(.cc) and updates the JNI register. BUG=609122 ==========
Description was changed from ========== Renderers are running in WebAPKs. This Cl includes the following things: 1. Introduce WebApkSandboxedProcessService which works the same as Chrome's ChildProcessService, but load Chrome's code via BaseDexClassLoader. 2. Move most of the functionalities from ChildProcessService to ChildProcessServiceImpl, so we could let WebApkSandboxedProcessService load the ChildProcessServiceImpl class and create an Impl object which loads Chrome's native libraries and creates renderer process. The ChildProcessService can't be loaded directly through the same way, it might because this class extends the Android Service class and complicates things. 3. Rename child_process_service.h(.cc) to child_process_service_impl.h(.cc) and updates the JNI register. BUG=609122 ========== to ========== Upstream: Renderers are running in WebAPKs. This Cl includes the following things: 1. Introduce WebApkSandboxedProcessService which works the same as Chrome's ChildProcessService, but load Chrome's code via BaseDexClassLoader. 2. Move most of the functionalities from ChildProcessService to ChildProcessServiceImpl, so we could let WebApkSandboxedProcessService load the ChildProcessServiceImpl class and create an Impl object which loads Chrome's native libraries and creates renderer process. The ChildProcessService can't be loaded directly through the same way, it might because this class extends the Android Service class and complicates things. 3. Rename child_process_service.h(.cc) to child_process_service_impl.h(.cc) and updates the JNI register. BUG=609122 ==========
hanxi@chromium.org changed reviewers: + pkotwicz@chromium.org
Hi Peter, could you please take a look? Thanks!
Description was changed from ========== Upstream: Renderers are running in WebAPKs. This Cl includes the following things: 1. Introduce WebApkSandboxedProcessService which works the same as Chrome's ChildProcessService, but load Chrome's code via BaseDexClassLoader. 2. Move most of the functionalities from ChildProcessService to ChildProcessServiceImpl, so we could let WebApkSandboxedProcessService load the ChildProcessServiceImpl class and create an Impl object which loads Chrome's native libraries and creates renderer process. The ChildProcessService can't be loaded directly through the same way, it might because this class extends the Android Service class and complicates things. 3. Rename child_process_service.h(.cc) to child_process_service_impl.h(.cc) and updates the JNI register. BUG=609122 ========== to ========== Upstream: Renderers are running in WebAPKs. This Cl includes the following things: 1. Introduce WebApkSandboxedProcessService which works the same as Chrome's ChildProcessService, but load Chrome's code via BaseDexClassLoader. 2. Move most of the functionalities from ChildProcessService to ChildProcessServiceImpl, so we could let WebApkSandboxedProcessService load the ChildProcessServiceImpl class and create an Impl object which loads Chrome's native libraries and creates renderer process. The ChildProcessService can't be loaded directly through the same way, it might because this class extends the Android Service class and complicates things. 3. Rename child_process_service.h(.cc) to child_process_service_impl.h(.cc) and updates the JNI register. 4. Pass in the class name of WebApkSandboxedProcessService via ChildProcessCreationParams in WebApkActivity. So ChildProcessLauncher knows the class name of the service to connect. BUG=609122 ==========
Thank you for figuring out how to use the ChildProcessService via reflection for WebAPKs If I kill both the Chrome and the WebAPK processes and launch the WebAPK from the app list, the WebAPK is launched in Chrome and not as a fullscreen webapp. If I close the WebAPK (but do not kill the WebAPK process) the WebAPK is launched fine afterwards https://codereview.chromium.org/2049843004/diff/60001/chrome/android/webapk/l... File chrome/android/webapk/libs/common/src/org/chromium/webapk/lib/common/WebApkSandboxedProcessService.java (right): https://codereview.chromium.org/2049843004/diff/60001/chrome/android/webapk/l... chrome/android/webapk/libs/common/src/org/chromium/webapk/lib/common/WebApkSandboxedProcessService.java:55: ClassLoader.getSystemClassLoader()); can you instead return remoteContext.getClassLoader() ? Can you remove the ClassLoader parameter from ChildProcessServiceImpl#create()? https://codereview.chromium.org/2049843004/diff/60001/chrome/android/webapk/l... chrome/android/webapk/libs/common/src/org/chromium/webapk/lib/common/WebApkSandboxedProcessService.java:64: getApplicationContext()); Can the |classLoader| variable be of type ClassLoader instead of being of type BaseDexClassLoader? https://codereview.chromium.org/2049843004/diff/60001/chrome/android/webapk/l... chrome/android/webapk/libs/common/src/org/chromium/webapk/lib/common/WebApkSandboxedProcessService.java:66: classLoader.loadClass(CHILD_PROCESS_SERVICE_IMPL); Nit: Can you call mChildProcessServiceImplClass.newInstance() ? https://codereview.chromium.org/2049843004/diff/60001/chrome/android/webapk/l... chrome/android/webapk/libs/common/src/org/chromium/webapk/lib/common/WebApkSandboxedProcessService.java:69: mChildProcessServiceImplInstance = childProcessServiceImplConstructor.newInstance(); Nit: mChildProcessServiceImplClass.newInstance() https://codereview.chromium.org/2049843004/diff/60001/chrome/android/webapk/l... chrome/android/webapk/libs/common/src/org/chromium/webapk/lib/common/WebApkSandboxedProcessService.java:92: } catch (NoSuchMethodException | IllegalAccessException | IllegalArgumentException Nit: Can you replace this with catch (Exception e) { ... } ? https://codereview.chromium.org/2049843004/diff/60001/chrome/android/webapk/l... chrome/android/webapk/libs/common/src/org/chromium/webapk/lib/common/WebApkSandboxedProcessService.java:107: | InvocationTargetException e) { Nit: Can you replace this with catch (Exception e) { ... } ? https://codereview.chromium.org/2049843004/diff/60001/chrome/android/webapk/s... File chrome/android/webapk/shell_apk/AndroidManifest.xml (right): https://codereview.chromium.org/2049843004/diff/60001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/AndroidManifest.xml:48: android:exported="true"/> - Looking at chrome/android/java/AndroidManifest.xml the sandboxed services specify an android:permission do we need to this here too? - I think that you need tools:ignore="ExportedService" otherwise proguard complains https://codereview.chromium.org/2049843004/diff/60001/content/public/android/... File content/public/android/java/src/org/chromium/content/app/ChildProcessService.java (right): https://codereview.chromium.org/2049843004/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/app/ChildProcessService.java:26: @JNINamespace("content") Is the JNINamespace annotation needed? https://codereview.chromium.org/2049843004/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/app/ChildProcessService.java:27: @SuppressWarnings("SynchronizeOnNonFinalField") Is this annotation needed? https://codereview.chromium.org/2049843004/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/app/ChildProcessService.java:29: private static final String TAG = "ChildProcessService"; Nit: Remove TAG because it is unused https://codereview.chromium.org/2049843004/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/app/ChildProcessService.java:61: */ It feels weird to have initializeParams() and getServiceInfo() in this class. I think that adding a getter for ChildProcessServiceImpl e.g. ChildProcessService#getChildProcessServiceImpl() and making DownloadProcessService use the getter would be better https://codereview.chromium.org/2049843004/diff/60001/content/public/android/... File content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java (left): https://codereview.chromium.org/2049843004/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java:91: // Binder object used by clients for this service. Why this change? I tried reverting this change and things still seem to work https://codereview.chromium.org/2049843004/diff/60001/content/public/android/... File content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java (right): https://codereview.chromium.org/2049843004/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java:109: 📚 Please document this method. https://codereview.chromium.org/2049843004/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java:112: //Debug.waitForDebugger(); 😁 https://codereview.chromium.org/2049843004/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java:120: mClassLoader = classLoader; Nit: initialize |mClassLoader| at the top of the method on line 112 https://codereview.chromium.org/2049843004/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java:121: final ChildProcessServiceImpl instance = this; Remove line 121. You can use ChildProcessServiceImpl.this instead of |instance| below. https://codereview.chromium.org/2049843004/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java:211: Log.d(TAG, "mMainThread is started: %s", mMainThread.getName()); Log.i() to match the remainder of the file? https://codereview.chromium.org/2049843004/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java:245: // scratch every time. Nit: Remove the comment w.r.t to stopSelf() https://codereview.chromium.org/2049843004/diff/60001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ChildProcessCreationParams.java (right): https://codereview.chromium.org/2049843004/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ChildProcessCreationParams.java:66: return mLibraryProcessType; I can't seem to find where this function is called
Hi Peter, could you please take another look? Thanks! https://codereview.chromium.org/2049843004/diff/60001/chrome/android/webapk/l... File chrome/android/webapk/libs/common/src/org/chromium/webapk/lib/common/WebApkSandboxedProcessService.java (right): https://codereview.chromium.org/2049843004/diff/60001/chrome/android/webapk/l... chrome/android/webapk/libs/common/src/org/chromium/webapk/lib/common/WebApkSandboxedProcessService.java:55: ClassLoader.getSystemClassLoader()); On 2016/06/10 21:29:47, pkotwicz wrote: > can you instead return remoteContext.getClassLoader() ? > Can you remove the ClassLoader parameter from ChildProcessServiceImpl#create()? It works! Updated the ChildProcessServiceImpl#create() as well. https://codereview.chromium.org/2049843004/diff/60001/chrome/android/webapk/l... chrome/android/webapk/libs/common/src/org/chromium/webapk/lib/common/WebApkSandboxedProcessService.java:64: getApplicationContext()); On 2016/06/10 21:29:47, pkotwicz wrote: > Can the |classLoader| variable be of type ClassLoader instead of being of type > BaseDexClassLoader? Done. https://codereview.chromium.org/2049843004/diff/60001/chrome/android/webapk/l... chrome/android/webapk/libs/common/src/org/chromium/webapk/lib/common/WebApkSandboxedProcessService.java:66: classLoader.loadClass(CHILD_PROCESS_SERVICE_IMPL); On 2016/06/10 21:29:47, pkotwicz wrote: > Nit: Can you call mChildProcessServiceImplClass.newInstance() ? Thanks for catching this. I forgot to remove the constructor after removing the parameter in the constructor function. The newInstance() call is enough. https://codereview.chromium.org/2049843004/diff/60001/chrome/android/webapk/l... chrome/android/webapk/libs/common/src/org/chromium/webapk/lib/common/WebApkSandboxedProcessService.java:69: mChildProcessServiceImplInstance = childProcessServiceImplConstructor.newInstance(); On 2016/06/10 21:29:47, pkotwicz wrote: > Nit: mChildProcessServiceImplClass.newInstance() Done. https://codereview.chromium.org/2049843004/diff/60001/chrome/android/webapk/l... chrome/android/webapk/libs/common/src/org/chromium/webapk/lib/common/WebApkSandboxedProcessService.java:92: } catch (NoSuchMethodException | IllegalAccessException | IllegalArgumentException On 2016/06/10 21:29:47, pkotwicz wrote: > Nit: Can you replace this with catch (Exception e) { ... } ? Done. https://codereview.chromium.org/2049843004/diff/60001/chrome/android/webapk/l... chrome/android/webapk/libs/common/src/org/chromium/webapk/lib/common/WebApkSandboxedProcessService.java:107: | InvocationTargetException e) { On 2016/06/10 21:29:47, pkotwicz wrote: > Nit: Can you replace this with catch (Exception e) { ... } ? Done. https://codereview.chromium.org/2049843004/diff/60001/chrome/android/webapk/s... File chrome/android/webapk/shell_apk/AndroidManifest.xml (right): https://codereview.chromium.org/2049843004/diff/60001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/AndroidManifest.xml:48: android:exported="true"/> On 2016/06/10 21:29:47, pkotwicz wrote: > - Looking at chrome/android/java/AndroidManifest.xml the sandboxed services > specify an android:permission do we need to this here too? > - I think that you need tools:ignore="ExportedService" otherwise proguard > complains Add tools:ignore="ExportedService". Hmmm, if we specify a permission here, does that mean Chrome needs to declare all these permissions if Chrome wants to connect to WebAPKs' child process? https://codereview.chromium.org/2049843004/diff/60001/content/public/android/... File content/public/android/java/src/org/chromium/content/app/ChildProcessService.java (right): https://codereview.chromium.org/2049843004/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/app/ChildProcessService.java:26: @JNINamespace("content") On 2016/06/10 21:29:47, pkotwicz wrote: > Is the JNINamespace annotation needed? We have to keep the namespace, since all of its subclass has, and Chrome crashes after removing the namespace. I don't think it is worthy to do some other changes just for removing a namespace here. https://codereview.chromium.org/2049843004/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/app/ChildProcessService.java:27: @SuppressWarnings("SynchronizeOnNonFinalField") On 2016/06/10 21:29:47, pkotwicz wrote: > Is this annotation needed? Removed. https://codereview.chromium.org/2049843004/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/app/ChildProcessService.java:29: private static final String TAG = "ChildProcessService"; On 2016/06/10 21:29:47, pkotwicz wrote: > Nit: Remove TAG because it is unused Done. https://codereview.chromium.org/2049843004/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/app/ChildProcessService.java:61: */ On 2016/06/10 21:29:47, pkotwicz wrote: > It feels weird to have initializeParams() and getServiceInfo() in this class. > > I think that adding a getter for ChildProcessServiceImpl e.g. > ChildProcessService#getChildProcessServiceImpl() and making > DownloadProcessService use the getter would be better Done. https://codereview.chromium.org/2049843004/diff/60001/content/public/android/... File content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java (left): https://codereview.chromium.org/2049843004/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java:91: // Binder object used by clients for this service. On 2016/06/10 21:29:48, pkotwicz wrote: > Why this change? I tried reverting this change and things still seem to work Good catch. I made this changes before introducing ChildProcessServiceImpl which doesn't extend a Service. Reverted. https://codereview.chromium.org/2049843004/diff/60001/content/public/android/... File content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java (right): https://codereview.chromium.org/2049843004/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java:109: On 2016/06/10 21:29:48, pkotwicz wrote: > 📚 Please document this method. Done. https://codereview.chromium.org/2049843004/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java:112: //Debug.waitForDebugger(); On 2016/06/10 21:29:47, pkotwicz wrote: > 😁 Missed this, removed. https://codereview.chromium.org/2049843004/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java:120: mClassLoader = classLoader; On 2016/06/10 21:29:48, pkotwicz wrote: > Nit: initialize |mClassLoader| at the top of the method on line 112 Done. https://codereview.chromium.org/2049843004/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java:121: final ChildProcessServiceImpl instance = this; On 2016/06/10 21:29:47, pkotwicz wrote: > Remove line 121. You can use ChildProcessServiceImpl.this instead of |instance| > below. Good to know, thanks! https://codereview.chromium.org/2049843004/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java:211: Log.d(TAG, "mMainThread is started: %s", mMainThread.getName()); On 2016/06/10 21:29:47, pkotwicz wrote: > Log.i() to match the remainder of the file? Sorry, it was debugging log, removed. https://codereview.chromium.org/2049843004/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java:245: // scratch every time. On 2016/06/10 21:29:47, pkotwicz wrote: > Nit: Remove the comment w.r.t to stopSelf() Done. https://codereview.chromium.org/2049843004/diff/60001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ChildProcessCreationParams.java (right): https://codereview.chromium.org/2049843004/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ChildProcessCreationParams.java:66: return mLibraryProcessType; On 2016/06/10 21:29:48, pkotwicz wrote: > I can't seem to find where this function is called Removed.
Patchset #3 (id:100001) has been deleted
Description was changed from ========== Upstream: Renderers are running in WebAPKs. This Cl includes the following things: 1. Introduce WebApkSandboxedProcessService which works the same as Chrome's ChildProcessService, but load Chrome's code via BaseDexClassLoader. 2. Move most of the functionalities from ChildProcessService to ChildProcessServiceImpl, so we could let WebApkSandboxedProcessService load the ChildProcessServiceImpl class and create an Impl object which loads Chrome's native libraries and creates renderer process. The ChildProcessService can't be loaded directly through the same way, it might because this class extends the Android Service class and complicates things. 3. Rename child_process_service.h(.cc) to child_process_service_impl.h(.cc) and updates the JNI register. 4. Pass in the class name of WebApkSandboxedProcessService via ChildProcessCreationParams in WebApkActivity. So ChildProcessLauncher knows the class name of the service to connect. BUG=609122 ========== to ========== Upstream: Renderers are running in WebAPKs. This Cl includes the following things: 1. Introduce WebApkSandboxedProcessService which works the same as Chrome's ChildProcessService, but loads Chrome's code via BaseDexClassLoader. 2. Move most of the functionalities from ChildProcessService to ChildProcessServiceImpl, so we could let WebApkSandboxedProcessService load the ChildProcessServiceImpl class and create an Impl object which loads Chrome's native libraries and creates renderer process. The ChildProcessService can't be loaded directly through the same way, it might because this class extends the Android Service class and complicates things. 3. Rename child_process_service.h(.cc) to child_process_service_impl.h(.cc) and updates the JNI register. 4. Pass in the class name of WebApkSandboxedProcessService via ChildProcessCreationParams in WebApkActivity. So ChildProcessLauncher knows the class name of the service to connect. BUG=609122 ==========
Description was changed from ========== Upstream: Renderers are running in WebAPKs. This Cl includes the following things: 1. Introduce WebApkSandboxedProcessService which works the same as Chrome's ChildProcessService, but loads Chrome's code via BaseDexClassLoader. 2. Move most of the functionalities from ChildProcessService to ChildProcessServiceImpl, so we could let WebApkSandboxedProcessService load the ChildProcessServiceImpl class and create an Impl object which loads Chrome's native libraries and creates renderer process. The ChildProcessService can't be loaded directly through the same way, it might because this class extends the Android Service class and complicates things. 3. Rename child_process_service.h(.cc) to child_process_service_impl.h(.cc) and updates the JNI register. 4. Pass in the class name of WebApkSandboxedProcessService via ChildProcessCreationParams in WebApkActivity. So ChildProcessLauncher knows the class name of the service to connect. BUG=609122 ========== to ========== Upstream: Renderers are running in WebAPKs. This Cl includes the following things: 1. Introduce WebApkSandboxedProcessService which works the same as Chrome's ChildProcessService, but loads Chrome's code via BaseDexClassLoader. 2. Move most of the functionality from ChildProcessService to ChildProcessServiceImpl, so we could let WebApkSandboxedProcessService load the ChildProcessServiceImpl class and create an Impl object which loads Chrome's native libraries and creates renderer process. The ChildProcessService can't be loaded directly through the same way, it might because this class extends the Android Service class and complicates things. 3. Rename child_process_service.h(.cc) to child_process_service_impl.h(.cc) and updates the JNI register. 4. Pass in the class name of WebApkSandboxedProcessService via ChildProcessCreationParams in WebApkActivity. So ChildProcessLauncher knows the class name of the service to connect. BUG=609122 ==========
Thank you for addressing all of my comments. Generally looks good https://codereview.chromium.org/2049843004/diff/60001/chrome/android/webapk/s... File chrome/android/webapk/shell_apk/AndroidManifest.xml (right): https://codereview.chromium.org/2049843004/diff/60001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/AndroidManifest.xml:48: android:exported="true"/> I think that Chrome would have to declare the permission if it wants to connect with the WebAPK. Chrome's sandboxed renderers use a permission with "signature" protectionLevel. On second thought, I do not know whether we want to use that for the WebAPK's sandboxed renderers. Should Chrome and WebAPKs have the same signature? https://codereview.chromium.org/2049843004/diff/80001/chrome/android/webapk/l... File chrome/android/webapk/libs/common/src/org/chromium/webapk/lib/common/WebApkSandboxedProcessService.java (right): https://codereview.chromium.org/2049843004/diff/80001/chrome/android/webapk/l... chrome/android/webapk/libs/common/src/org/chromium/webapk/lib/common/WebApkSandboxedProcessService.java:53: remoteContext.getClassLoader()); Does removing line 52 & 53 and replacing them with the following work? return remoteContext.getClassLoader(); https://codereview.chromium.org/2049843004/diff/80001/content/public/android/... File content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java (right): https://codereview.chromium.org/2049843004/diff/80001/content/public/android/... content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java:39: * This class provides all the implementations for {@link ChildProcessService} which owns an How about: "This class implements all of the functionality for {@link ChildProcessService} which ..." https://codereview.chromium.org/2049843004/diff/80001/content/public/android/... content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java:86: Can you please add a comment to |mClassLoader| and maybe rename the variable to |mHostClassLoader|? It is not clear which context |mClassLoader| is for. https://codereview.chromium.org/2049843004/diff/80001/content/public/android/... content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java:89: // Return a Linker instance. If testing, the Linker needs special setup. Nit: Can you move this function so that gerrit does not think that this is a new function? https://codereview.chromium.org/2049843004/diff/80001/content/public/android/... content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java:102: /* package */ static Context getContext() { This function seems unused. I posted a CL to remove it in https://codereview.chromium.org/2061743003/ We'll find out if the existence of the function is a hack https://codereview.chromium.org/2049843004/diff/80001/content/public/android/... content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java:118: Can you please add a comment about which context to use. To my surprise deleting line 119 does not cause a crash when a renderer is launched. I am pretty sure that the call is necessary, however I do not know why https://codereview.chromium.org/2049843004/diff/80001/content/public/android/... content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java:172: LibraryLoader.get(mLibraryProcessType).loadNow(context); Should this be |hostBrowserContext|? (I am unsure if this is the code that you told me about before leaving for the day)
Description was changed from ========== Upstream: Renderers are running in WebAPKs. This Cl includes the following things: 1. Introduce WebApkSandboxedProcessService which works the same as Chrome's ChildProcessService, but loads Chrome's code via BaseDexClassLoader. 2. Move most of the functionality from ChildProcessService to ChildProcessServiceImpl, so we could let WebApkSandboxedProcessService load the ChildProcessServiceImpl class and create an Impl object which loads Chrome's native libraries and creates renderer process. The ChildProcessService can't be loaded directly through the same way, it might because this class extends the Android Service class and complicates things. 3. Rename child_process_service.h(.cc) to child_process_service_impl.h(.cc) and updates the JNI register. 4. Pass in the class name of WebApkSandboxedProcessService via ChildProcessCreationParams in WebApkActivity. So ChildProcessLauncher knows the class name of the service to connect. BUG=609122 ========== to ========== Upstream: Renderers are running in WebAPKs. This Cl includes the following things: 1. Introduce WebApkSandboxedProcessService which works the same as Chrome's ChildProcessService, but loads Chrome's code via Chrome's ClassLoader. 2. Move most of the functionality from ChildProcessService to ChildProcessServiceImpl, so we could let WebApkSandboxedProcessService load the ChildProcessServiceImpl class and create an Impl object which loads Chrome's native libraries and creates renderer process. The ChildProcessService can't be loaded directly through the same way, it might because this class extends the Android Service class and complicates things. 3. Rename child_process_service.h(.cc) to child_process_service_impl.h(.cc) and updates the JNI register. 4. Pass in the class name of WebApkSandboxedProcessService via ChildProcessCreationParams in WebApkActivity. So ChildProcessLauncher knows the class name of the service to connect. BUG=609122 ==========
Patchset #3 (id:120001) has been deleted
Hi Peter, PTAL, thanks! https://codereview.chromium.org/2049843004/diff/60001/chrome/android/webapk/s... File chrome/android/webapk/shell_apk/AndroidManifest.xml (right): https://codereview.chromium.org/2049843004/diff/60001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/AndroidManifest.xml:48: android:exported="true"/> On 2016/06/14 01:19:48, pkotwicz wrote: > I think that Chrome would have to declare the permission if it wants to connect > with the WebAPK. Chrome's sandboxed renderers use a permission with "signature" > protectionLevel. On second thought, I do not know whether we want to use that > for the WebAPK's sandboxed renderers. Should Chrome and WebAPKs have the same > signature? I think WebAPK will be signed by using Chromium keys, same as dev/beta/canary, but Chrome is signed with Google's key. On the other side, if each WebAPK declares its own permission for the child process service, while we have to make Chrome declare all these permissions if Chrome wants to connect to WebAPKs, which is impossible. https://codereview.chromium.org/2049843004/diff/80001/chrome/android/webapk/l... File chrome/android/webapk/libs/common/src/org/chromium/webapk/lib/common/WebApkSandboxedProcessService.java (right): https://codereview.chromium.org/2049843004/diff/80001/chrome/android/webapk/l... chrome/android/webapk/libs/common/src/org/chromium/webapk/lib/common/WebApkSandboxedProcessService.java:53: remoteContext.getClassLoader()); On 2016/06/14 01:19:48, pkotwicz wrote: > Does removing line 52 & 53 and replacing them with the following work? > > return remoteContext.getClassLoader(); I had the same thoughts before and it works! https://codereview.chromium.org/2049843004/diff/80001/content/public/android/... File content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java (right): https://codereview.chromium.org/2049843004/diff/80001/content/public/android/... content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java:39: * This class provides all the implementations for {@link ChildProcessService} which owns an On 2016/06/14 01:19:48, pkotwicz wrote: > How about: "This class implements all of the functionality for {@link > ChildProcessService} which ..." Done. https://codereview.chromium.org/2049843004/diff/80001/content/public/android/... content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java:86: On 2016/06/14 01:19:48, pkotwicz wrote: > Can you please add a comment to |mClassLoader| and maybe rename the variable to > |mHostClassLoader|? > > It is not clear which context |mClassLoader| is for. Done. https://codereview.chromium.org/2049843004/diff/80001/content/public/android/... content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java:89: // Return a Linker instance. If testing, the Linker needs special setup. On 2016/06/14 01:19:48, pkotwicz wrote: > Nit: Can you move this function so that gerrit does not think that this is a new > function? Done. https://codereview.chromium.org/2049843004/diff/80001/content/public/android/... content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java:102: /* package */ static Context getContext() { On 2016/06/14 01:19:48, pkotwicz wrote: > This function seems unused. I posted a CL to remove it in > https://codereview.chromium.org/2061743003/ We'll find out if the existence of > the function is a hack Thanks for creating the clean up CL! https://codereview.chromium.org/2049843004/diff/80001/content/public/android/... content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java:118: On 2016/06/14 01:19:48, pkotwicz wrote: > Can you please add a comment about which context to use. To my surprise deleting > line 119 does not cause a crash when a renderer is launched. I am pretty sure > that the call is necessary, however I do not know why Done. https://codereview.chromium.org/2049843004/diff/80001/content/public/android/... content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java:172: LibraryLoader.get(mLibraryProcessType).loadNow(context); On 2016/06/14 01:19:48, pkotwicz wrote: > Should this be |hostBrowserContext|? (I am unsure if this is the code that you > told me about before leaving for the day) It looks like we should use |hostBrowserContext| too, but I don't know why we didn't update here before. Updated. It's not the code that I told you. Actually, the changes that I mentioned already in, I might just looked at a wrong branch. Sorry for the misleading.
Some final comments, then L-G-T-M https://codereview.chromium.org/2049843004/diff/80001/content/public/android/... File content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java (right): https://codereview.chromium.org/2049843004/diff/80001/content/public/android/... content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java:102: /* package */ static Context getContext() { It looks like dtrainor@ is at Blimpcon. We'll probably not get a response till Thursday https://codereview.chromium.org/2049843004/diff/140001/chrome/android/webapk/... File chrome/android/webapk/libs/common/BUILD.gn (right): https://codereview.chromium.org/2049843004/diff/140001/chrome/android/webapk/... chrome/android/webapk/libs/common/BUILD.gn:10: "src/org/chromium/webapk/lib/common/WebApkSandboxedProcessService.java", I am unhappy that WebApkSandboxedProcessService is in webapk/lib/common instead of webapk/shell_apk but I understand why you are doing it. I unfortunately cannot think of a better way. https://codereview.chromium.org/2049843004/diff/140001/chrome/android/webapk/... File chrome/android/webapk/libs/common/src/org/chromium/webapk/lib/common/WebApkSandboxedProcessService.java (right): https://codereview.chromium.org/2049843004/diff/140001/chrome/android/webapk/... chrome/android/webapk/libs/common/src/org/chromium/webapk/lib/common/WebApkSandboxedProcessService.java:18: * creates renderer processes. Nit: Can you please update the comment? This class no longer uses BaseDexClassLoader https://codereview.chromium.org/2049843004/diff/140001/chrome/android/webapk/... chrome/android/webapk/libs/common/src/org/chromium/webapk/lib/common/WebApkSandboxedProcessService.java:21: private static final String CHILD_PROCESS_SERVICE_IMPL = Nit: Rename to CHILD_PROCESS_SERVICE_IMPL_CLASS_NAME https://codereview.chromium.org/2049843004/diff/140001/chrome/android/webapk/... chrome/android/webapk/libs/common/src/org/chromium/webapk/lib/common/WebApkSandboxedProcessService.java:55: try { Can you just do: Context hostBrowserContext = WebApkUtils.getHostBrowserContext(getApplicationContext()); ClassLoader classLoader = hostBrowserContext.getClassLoader(); and remove the getClassLoaderInstance() function. I think that Context#getClassLoader() is fast. Can you double check? You are already calling WebApkUtils#getHostBrowserContext() on line 65 https://codereview.chromium.org/2049843004/diff/140001/content/public/android... File content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java (right): https://codereview.chromium.org/2049843004/diff/140001/content/public/android... content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java:102: // The class loader of the Host Browser. How about: "The ClassLoader for the host browser context." https://codereview.chromium.org/2049843004/diff/140001/content/public/android... content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java:112: * @param hostBrowserContext The context of the Host Browser (i.e. Chrome). Nit: host browser (no capitals) https://codereview.chromium.org/2049843004/diff/140001/content/public/android... content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java:122: // Initialize the context for the application who owns this ChildProcessServiceImpl object. Nit: who -> that "who" is for people only. According to http://www.grammarbook.com/grammar/whoVwhVt.asp "which" and "that" are somewhat interchangeable. https://codereview.chromium.org/2049843004/diff/140001/content/public/android... content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java:123: // The application could a WebAPK. Nit: Remove line 123. I do not think that it makes the code clearer. https://codereview.chromium.org/2049843004/diff/140001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ChildProcessCreationParams.java (right): https://codereview.chromium.org/2049843004/diff/140001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ChildProcessCreationParams.java:22: private final Class<? extends Service> mServiceName; Nit: rename mServiceName to mServiceClass https://codereview.chromium.org/2049843004/diff/140001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ChildProcessCreationParams.java:37: int libraryProcessType) { Java and C++11 support delegated constructors. You can do: this(packageName, extraBindFlags, libraryProcessType, null) to call the 4 argument constructor from the 3 argument constuctor. https://codereview.chromium.org/2049843004/diff/140001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ChildProcessCreationParams.java:65: public Class<? extends Service> getServiceName() { Nit: Rename to getServiceClass() https://codereview.chromium.org/2049843004/diff/140001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java (right): https://codereview.chromium.org/2049843004/diff/140001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:75: Class<? extends Service> serviceName) { Does {@link serviceName} need to be passed into the ChildConnectionAllocator constructor given that ChildProcessCreationParams are passed into ChildConnectionAllocator#allocate()? The child class can be determined based on ChildConnectionAllocator#mInSandbox and ChildProcessCreationParams in ChildConnectionAllocator#allocate()
Patchset #4 (id:160001) has been deleted
Patchset #4 (id:180001) has been deleted
Hi Peter, PTAL, thanks! https://codereview.chromium.org/2049843004/diff/140001/chrome/android/webapk/... File chrome/android/webapk/libs/common/BUILD.gn (right): https://codereview.chromium.org/2049843004/diff/140001/chrome/android/webapk/... chrome/android/webapk/libs/common/BUILD.gn:10: "src/org/chromium/webapk/lib/common/WebApkSandboxedProcessService.java", On 2016/06/15 14:13:50, pkotwicz wrote: > I am unhappy that WebApkSandboxedProcessService is in webapk/lib/common instead > of webapk/shell_apk but I understand why you are doing it. I unfortunately > cannot think of a better way. Totally agree. Putting in webapk/shell_apk was what I did first, and then I realized the dependencies needed to introduce:( https://codereview.chromium.org/2049843004/diff/140001/chrome/android/webapk/... File chrome/android/webapk/libs/common/src/org/chromium/webapk/lib/common/WebApkSandboxedProcessService.java (right): https://codereview.chromium.org/2049843004/diff/140001/chrome/android/webapk/... chrome/android/webapk/libs/common/src/org/chromium/webapk/lib/common/WebApkSandboxedProcessService.java:18: * creates renderer processes. On 2016/06/15 14:13:50, pkotwicz wrote: > Nit: Can you please update the comment? This class no longer uses > BaseDexClassLoader Done. https://codereview.chromium.org/2049843004/diff/140001/chrome/android/webapk/... chrome/android/webapk/libs/common/src/org/chromium/webapk/lib/common/WebApkSandboxedProcessService.java:21: private static final String CHILD_PROCESS_SERVICE_IMPL = On 2016/06/15 14:13:50, pkotwicz wrote: > Nit: Rename to CHILD_PROCESS_SERVICE_IMPL_CLASS_NAME Done. https://codereview.chromium.org/2049843004/diff/140001/chrome/android/webapk/... chrome/android/webapk/libs/common/src/org/chromium/webapk/lib/common/WebApkSandboxedProcessService.java:55: try { On 2016/06/15 14:13:50, pkotwicz wrote: > Can you just do: > Context hostBrowserContext = > WebApkUtils.getHostBrowserContext(getApplicationContext()); > ClassLoader classLoader = hostBrowserContext.getClassLoader(); > > and remove the getClassLoaderInstance() function. I think that > Context#getClassLoader() is fast. Can you double check? You are already calling > WebApkUtils#getHostBrowserContext() on line 65 You are right, removed since we have to call the WebApkUtils#getHostBrowserContext() anyway. https://codereview.chromium.org/2049843004/diff/140001/content/public/android... File content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java (right): https://codereview.chromium.org/2049843004/diff/140001/content/public/android... content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java:102: // The class loader of the Host Browser. On 2016/06/15 14:13:50, pkotwicz wrote: > How about: "The ClassLoader for the host browser context." Done. https://codereview.chromium.org/2049843004/diff/140001/content/public/android... content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java:112: * @param hostBrowserContext The context of the Host Browser (i.e. Chrome). On 2016/06/15 14:13:50, pkotwicz wrote: > Nit: host browser (no capitals) Done. https://codereview.chromium.org/2049843004/diff/140001/content/public/android... content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java:122: // Initialize the context for the application who owns this ChildProcessServiceImpl object. On 2016/06/15 14:13:51, pkotwicz wrote: > Nit: who -> that > > "who" is for people only. According to > http://www.grammarbook.com/grammar/whoVwhVt.asp "which" and "that" are somewhat > interchangeable. Done. https://codereview.chromium.org/2049843004/diff/140001/content/public/android... content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java:123: // The application could a WebAPK. On 2016/06/15 14:13:50, pkotwicz wrote: > Nit: Remove line 123. I do not think that it makes the code clearer. Done. https://codereview.chromium.org/2049843004/diff/140001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ChildProcessCreationParams.java (right): https://codereview.chromium.org/2049843004/diff/140001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ChildProcessCreationParams.java:22: private final Class<? extends Service> mServiceName; On 2016/06/15 14:13:51, pkotwicz wrote: > Nit: rename mServiceName to mServiceClass Done. https://codereview.chromium.org/2049843004/diff/140001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ChildProcessCreationParams.java:37: int libraryProcessType) { On 2016/06/15 14:13:51, pkotwicz wrote: > Java and C++11 support delegated constructors. You can do: > this(packageName, extraBindFlags, libraryProcessType, null) > to call the 4 argument constructor from the 3 argument constuctor. Thanks for the suggestion! https://codereview.chromium.org/2049843004/diff/140001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ChildProcessCreationParams.java:65: public Class<? extends Service> getServiceName() { On 2016/06/15 14:13:51, pkotwicz wrote: > Nit: Rename to getServiceClass() Done. https://codereview.chromium.org/2049843004/diff/140001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java (right): https://codereview.chromium.org/2049843004/diff/140001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:75: Class<? extends Service> serviceName) { On 2016/06/15 14:13:51, pkotwicz wrote: > Does {@link serviceName} need to be passed into the ChildConnectionAllocator > constructor given that ChildProcessCreationParams are passed into > ChildConnectionAllocator#allocate()? > > The child class can be determined based on ChildConnectionAllocator#mInSandbox > and ChildProcessCreationParams in ChildConnectionAllocator#allocate() I understand why you suggest this, but I am feeling bad to keep a wrong Service class in the Allocator. How about remove the |mServiceClass| in the |ChildProcessAllocator| and always use a new added getServiceClass() function?
LGTM! https://codereview.chromium.org/2049843004/diff/140001/chrome/android/webapk/... File chrome/android/webapk/libs/common/src/org/chromium/webapk/lib/common/WebApkSandboxedProcessService.java (right): https://codereview.chromium.org/2049843004/diff/140001/chrome/android/webapk/... chrome/android/webapk/libs/common/src/org/chromium/webapk/lib/common/WebApkSandboxedProcessService.java:55: try { Thank you for making this change! https://codereview.chromium.org/2049843004/diff/140001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java (right): https://codereview.chromium.org/2049843004/diff/140001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:75: Class<? extends Service> serviceName) { I agree, it is good to remove ChildConnectionAllocator#mChildClass I was thinking of determining which service class to use inline in ChildConnectionAllocator#allocate() but adding the ChildConnectionAllocator#getServiceClass() function is OK too. https://codereview.chromium.org/2049843004/diff/200001/chrome/android/webapk/... File chrome/android/webapk/libs/common/src/org/chromium/webapk/lib/common/WebApkSandboxedProcessService.java (right): https://codereview.chromium.org/2049843004/diff/200001/chrome/android/webapk/... chrome/android/webapk/libs/common/src/org/chromium/webapk/lib/common/WebApkSandboxedProcessService.java:16: * Child process service hosted by WebAPKs. This class uses Chrome's ClassLoader to create an Nit: an -> a https://codereview.chromium.org/2049843004/diff/200001/chrome/android/webapk/... chrome/android/webapk/libs/common/src/org/chromium/webapk/lib/common/WebApkSandboxedProcessService.java:18: * and creates renderer processes. Nit: Replace "creates renderer processes" with "creates the renderer". I think that: - WebApkSandboxedProcessService is in a separate process and that ChildProcessServiceImpl does not create any processes. - there is one renderer per ChildProcessServiceImpl
hanxi@chromium.org changed reviewers: + mariakhomenko@chromium.org
Hi Maria, could you please take a look? Thanks! https://codereview.chromium.org/2049843004/diff/200001/chrome/android/webapk/... File chrome/android/webapk/libs/common/src/org/chromium/webapk/lib/common/WebApkSandboxedProcessService.java (right): https://codereview.chromium.org/2049843004/diff/200001/chrome/android/webapk/... chrome/android/webapk/libs/common/src/org/chromium/webapk/lib/common/WebApkSandboxedProcessService.java:18: * and creates renderer processes. On 2016/06/15 17:51:01, pkotwicz wrote: > Nit: Replace "creates renderer processes" with "creates the renderer". > > I think that: > - WebApkSandboxedProcessService is in a separate process and that > ChildProcessServiceImpl does not create any processes. > - there is one renderer per ChildProcessServiceImpl Yes, this is more accurate. Thanks.
hanxi@chromium.org changed reviewers: + michaelbai@chromium.org
+michaelbai@ Hi Maria and Tao, since both of you review Peter's and my CLs for child process creation, could you please take a look? Thanks!
https://codereview.chromium.org/2049843004/diff/240001/chrome/android/webapk/... File chrome/android/webapk/libs/common/src/org/chromium/webapk/lib/common/WebApkSandboxedProcessService.java (right): https://codereview.chromium.org/2049843004/diff/240001/chrome/android/webapk/... chrome/android/webapk/libs/common/src/org/chromium/webapk/lib/common/WebApkSandboxedProcessService.java:26: private Object mChildProcessServiceImplInstance; final on both objects above? https://codereview.chromium.org/2049843004/diff/240001/chrome/android/webapk/... chrome/android/webapk/libs/common/src/org/chromium/webapk/lib/common/WebApkSandboxedProcessService.java:37: classLoader.loadClass(CHILD_PROCESS_SERVICE_IMPL_CLASS_NAME); Do we need all this reflection here because we want to use a different class loader? https://codereview.chromium.org/2049843004/diff/240001/chrome/android/webapk/... chrome/android/webapk/libs/common/src/org/chromium/webapk/lib/common/WebApkSandboxedProcessService.java:46: e.printStackTrace(); you can pass e as the third parameter to log and it will then print the stacktrace. I would suggest logging at a lower verbocity level for something like this where failures are not expected. https://codereview.chromium.org/2049843004/diff/240001/content/public/android... File content/public/android/java/src/org/chromium/content/app/ChildProcessService.java (right): https://codereview.chromium.org/2049843004/diff/240001/content/public/android... content/public/android/java/src/org/chromium/content/app/ChildProcessService.java:28: new ChildProcessServiceImpl(); nit: this looks like it could fit on one line? https://codereview.chromium.org/2049843004/diff/240001/content/public/android... File content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java (right): https://codereview.chromium.org/2049843004/diff/240001/content/public/android... content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java:103: private ClassLoader mHostClassLoader; final? https://codereview.chromium.org/2049843004/diff/240001/content/public/android... content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java:110: * Loads Chrome's native libraries and initializes a ChildProcessServiceImapl. nit: Iampl -> Impl https://codereview.chromium.org/2049843004/diff/240001/content/public/android... File content/public/android/java/src/org/chromium/content/app/DownloadProcessService.java (right): https://codereview.chromium.org/2049843004/diff/240001/content/public/android... content/public/android/java/src/org/chromium/content/app/DownloadProcessService.java:43: ChildProcessServiceImpl impl = getChildProcessServiceImpl(); I think that instead of surfacing the impl and letting code here call it, you should expose the calls on ChildProcessService and proxy inside the service. This leaks implementation details. https://codereview.chromium.org/2049843004/diff/240001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ChildProcessCreationParams.java (right): https://codereview.chromium.org/2049843004/diff/240001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ChildProcessCreationParams.java:42: int libraryProcessType, Class<? extends Service> servicClass) { servic -> service https://codereview.chromium.org/2049843004/diff/240001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java (right): https://codereview.chromium.org/2049843004/diff/240001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:103: ? creationParams.getServiceClass() : null); I think that's an odd division of logic. Why not just pass creation params into getServiceClass() call?
Patchset #7 (id:260001) has been deleted
Patchset #7 (id:280001) has been deleted
Hi Maria, could you please take another look? Thanks! https://codereview.chromium.org/2049843004/diff/240001/chrome/android/webapk/... File chrome/android/webapk/libs/common/src/org/chromium/webapk/lib/common/WebApkSandboxedProcessService.java (right): https://codereview.chromium.org/2049843004/diff/240001/chrome/android/webapk/... chrome/android/webapk/libs/common/src/org/chromium/webapk/lib/common/WebApkSandboxedProcessService.java:26: private Object mChildProcessServiceImplInstance; On 2016/06/17 17:09:24, Maria wrote: > final on both objects above? These two objects are initialized in onCreate(), not in the constructor. The WebApkSandboxedProcessService extends Service, therefore all the logic are implemented onCreate(). https://codereview.chromium.org/2049843004/diff/240001/chrome/android/webapk/... chrome/android/webapk/libs/common/src/org/chromium/webapk/lib/common/WebApkSandboxedProcessService.java:37: classLoader.loadClass(CHILD_PROCESS_SERVICE_IMPL_CLASS_NAME); On 2016/06/17 17:09:24, Maria wrote: > Do we need all this reflection here because we want to use a different class > loader? We only use Chrome's class loader to load ChildProcessServiceImpl class, not to replace the WebAPK's class loader. So I think these reflection are still necessary. Does it answer your question? https://codereview.chromium.org/2049843004/diff/240001/chrome/android/webapk/... chrome/android/webapk/libs/common/src/org/chromium/webapk/lib/common/WebApkSandboxedProcessService.java:46: e.printStackTrace(); On 2016/06/17 17:09:24, Maria wrote: > you can pass e as the third parameter to log and it will then print the > stacktrace. > > I would suggest logging at a lower verbocity level for something like this where > failures are not expected. Good catch, thanks! https://codereview.chromium.org/2049843004/diff/240001/content/public/android... File content/public/android/java/src/org/chromium/content/app/ChildProcessService.java (right): https://codereview.chromium.org/2049843004/diff/240001/content/public/android... content/public/android/java/src/org/chromium/content/app/ChildProcessService.java:28: new ChildProcessServiceImpl(); On 2016/06/17 17:09:24, Maria wrote: > nit: this looks like it could fit on one line? Done. https://codereview.chromium.org/2049843004/diff/240001/content/public/android... File content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java (right): https://codereview.chromium.org/2049843004/diff/240001/content/public/android... content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java:103: private ClassLoader mHostClassLoader; On 2016/06/17 17:09:24, Maria wrote: > final? It is still because |mHostClassLoader| isn't initialized in the constructor. We have to keep the function ChildProcessServiceImpl#create(), because in ChildProcessService, we mark the |mChildProcessServiceImpl| as final, so it has to be initiated it inline, not in OnCreate(). https://codereview.chromium.org/2049843004/diff/240001/content/public/android... content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java:110: * Loads Chrome's native libraries and initializes a ChildProcessServiceImapl. On 2016/06/17 17:09:24, Maria wrote: > nit: Iampl -> Impl Sorry. https://codereview.chromium.org/2049843004/diff/240001/content/public/android... File content/public/android/java/src/org/chromium/content/app/DownloadProcessService.java (right): https://codereview.chromium.org/2049843004/diff/240001/content/public/android... content/public/android/java/src/org/chromium/content/app/DownloadProcessService.java:43: ChildProcessServiceImpl impl = getChildProcessServiceImpl(); On 2016/06/17 17:09:24, Maria wrote: > I think that instead of surfacing the impl and letting code here call it, you > should expose the calls on ChildProcessService and proxy inside the service. > This leaks implementation details. That is what I did before. Sorry Peter, revert it back. https://codereview.chromium.org/2049843004/diff/240001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ChildProcessCreationParams.java (right): https://codereview.chromium.org/2049843004/diff/240001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ChildProcessCreationParams.java:42: int libraryProcessType, Class<? extends Service> servicClass) { On 2016/06/17 17:09:24, Maria wrote: > servic -> service Done. https://codereview.chromium.org/2049843004/diff/240001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java (right): https://codereview.chromium.org/2049843004/diff/240001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:103: ? creationParams.getServiceClass() : null); On 2016/06/17 17:09:24, Maria wrote: > I think that's an odd division of logic. Why not just pass creation params into > getServiceClass() call? It sounds good to me, since we can just pass null if we want to have a default service class, and only construct a CreationParams when a WebAPK's service class is needed. Updated.
I didn't look through the patch yet, could you help me to understand what prevent you from using SandboxedProcessService instead of inventing WebApkSandboxedProcessService? https://codereview.chromium.org/2049843004/diff/300001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/init/ChromeBrowserInitializer.java (right): https://codereview.chromium.org/2049843004/diff/300001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/init/ChromeBrowserInitializer.java:320: if (creationParams != null) { Maybe also check ChildProcessCreationParams.get() != null ? https://codereview.chromium.org/2049843004/diff/300001/chrome/android/webapk/... File chrome/android/webapk/libs/common/src/org/chromium/webapk/lib/common/WebApkSandboxedProcessService.java (right): https://codereview.chromium.org/2049843004/diff/300001/chrome/android/webapk/... chrome/android/webapk/libs/common/src/org/chromium/webapk/lib/common/WebApkSandboxedProcessService.java:32: try { Why don't you create ChildProcessServiceImpl directly?
We want to keep WebAPK (gn target is shell_apk) small, so it doesn't include any Chrome's code. Since WebAPK holds Chrome's renderer process, it needs to load Chrome's native libraries etc. to create a ChildProcessService. In order to load Chrome's ChildProcessService in WebAPK, we have to use reflection to copy elements from Chrome's ClassLoader to WebAPK's ClassLoader(see https://codereview.chromium.org/1710083004/diff/120001/web_apks/minting_examp...). However, Android team doesn't suggest using any reflection like that. Additionally, that approach also introduces some ugly dependency in LibraryLoader. So we figure out another way to do it alternatively, as described in this CL. https://codereview.chromium.org/2049843004/diff/300001/chrome/android/webapk/... File chrome/android/webapk/libs/common/src/org/chromium/webapk/lib/common/WebApkSandboxedProcessService.java (right): https://codereview.chromium.org/2049843004/diff/300001/chrome/android/webapk/... chrome/android/webapk/libs/common/src/org/chromium/webapk/lib/common/WebApkSandboxedProcessService.java:32: try { On 2016/06/20 17:58:46, michaelbai wrote: > Why don't you create ChildProcessServiceImpl directly? We want to keep WebAPK is small, so it doesn't depend on any Chrome's code. Since ChildProcessServiceImpl is in Chrome' code base, even though WebAPK (gn target is shell_apk) uses Chrome's ClassLoader, the shell_apk doesn't have knowledge of the ChildProcessServiceImpl, so it has to use reflection to call any function in ChildProcessServiceImpl.
lgtm https://codereview.chromium.org/2049843004/diff/320001/chrome/android/webapk/... File chrome/android/webapk/libs/common/src/org/chromium/webapk/lib/common/WebApkSandboxedProcessService.java (right): https://codereview.chromium.org/2049843004/diff/320001/chrome/android/webapk/... chrome/android/webapk/libs/common/src/org/chromium/webapk/lib/common/WebApkSandboxedProcessService.java:22: "org.chromium.content.app.ChildProcessServiceImpl"; I think you may want to add a test that this is updated if above is renamed or at least a comment in the file to say so. https://codereview.chromium.org/2049843004/diff/320001/chrome/android/webapk/... chrome/android/webapk/libs/common/src/org/chromium/webapk/lib/common/WebApkSandboxedProcessService.java:45: Log.d(TAG, "Unable to create a ChildProcessServiceImpl for the WebAPK.", e); I would log with lower verbosity in all these methods so that it surfaces in logs https://codereview.chromium.org/2049843004/diff/320001/content/public/android... File content/public/android/java/src/org/chromium/content/app/ChildProcessService.java (right): https://codereview.chromium.org/2049843004/diff/320001/content/public/android... content/public/android/java/src/org/chromium/content/app/ChildProcessService.java:38: @SuppressFBWarnings("DM_EXIT") this warning belongs on the IMPL now
michaelbai@chromium.org changed reviewers: + torne@chromium.org
torne@ would you like take a look? it might relate to WebView Zygote, multiple process, or you may have other idea for how to implement this?
https://codereview.chromium.org/2049843004/diff/300001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/init/ChromeBrowserInitializer.java (right): https://codereview.chromium.org/2049843004/diff/300001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/init/ChromeBrowserInitializer.java:320: if (creationParams != null) { On 2016/06/20 17:58:46, michaelbai wrote: > Maybe also check ChildProcessCreationParams.get() != null ? Done. https://codereview.chromium.org/2049843004/diff/320001/chrome/android/webapk/... File chrome/android/webapk/libs/common/src/org/chromium/webapk/lib/common/WebApkSandboxedProcessService.java (right): https://codereview.chromium.org/2049843004/diff/320001/chrome/android/webapk/... chrome/android/webapk/libs/common/src/org/chromium/webapk/lib/common/WebApkSandboxedProcessService.java:22: "org.chromium.content.app.ChildProcessServiceImpl"; On 2016/06/20 18:49:22, Maria wrote: > I think you may want to add a test that this is updated if above is renamed or > at least a comment in the file to say so. Good point. I added a note here. Beside, I add a WebAPK's instrument test in another CL (https://codereview.chromium.org/2076583002/). It is not directly related, but will fail if the class is renamed. https://codereview.chromium.org/2049843004/diff/320001/chrome/android/webapk/... chrome/android/webapk/libs/common/src/org/chromium/webapk/lib/common/WebApkSandboxedProcessService.java:45: Log.d(TAG, "Unable to create a ChildProcessServiceImpl for the WebAPK.", e); On 2016/06/20 18:49:22, Maria wrote: > I would log with lower verbosity in all these methods so that it surfaces in > logs Got it this time:) https://codereview.chromium.org/2049843004/diff/320001/content/public/android... File content/public/android/java/src/org/chromium/content/app/ChildProcessService.java (right): https://codereview.chromium.org/2049843004/diff/320001/content/public/android... content/public/android/java/src/org/chromium/content/app/ChildProcessService.java:38: @SuppressFBWarnings("DM_EXIT") On 2016/06/20 18:49:22, Maria wrote: > this warning belongs on the IMPL now Removed.
On 2016/06/20 19:06:57, michaelbai wrote: > torne@ would you like take a look? it might relate to WebView Zygote, multiple > process, or you may have other idea for how to implement this? I haven't looked at the CL in detail, but this is probably going to have implications for webview multiprocess/zygote stuff. We added the externalService functionality to Android N so that WebView apps could bind to renderers declared in the webview/chrome manifest and run them *associated with the calling app's UID* instead of webview/chrome's. The webapk stuff here is doing the exact opposite - having Chrome bind to renderers declared in the webapk but leaving the renderer associated with the webapk's UID. So if we introduce a special way to launch chrome/webview renderers (from a zygote) then it's going to have to work differently in this case, and right now we haven't included the required alternate mode of operation in the design I don't think. We should probably schedule some time to chat about this? Who would be good to involve on the webapk side?
On 2016/06/20 19:06:57, michaelbai wrote: > torne@ would you like take a look? it might relate to WebView Zygote, multiple > process, or you may have other idea for how to implement this? I haven't looked at the CL in detail, but this is probably going to have implications for webview multiprocess/zygote stuff. We added the externalService functionality to Android N so that WebView apps could bind to renderers declared in the webview/chrome manifest and run them *associated with the calling app's UID* instead of webview/chrome's. The webapk stuff here is doing the exact opposite - having Chrome bind to renderers declared in the webapk but leaving the renderer associated with the webapk's UID. So if we introduce a special way to launch chrome/webview renderers (from a zygote) then it's going to have to work differently in this case, and right now we haven't included the required alternate mode of operation in the design I don't think. We should probably schedule some time to chat about this? Who would be good to involve on the webapk side?
Richard, please feel free to add a meeting on my calendar. WebApk side will be pkotwicz@ and me. On Jun 21, 2016 6:51 AM, <torne@chromium.org> wrote: > On 2016/06/20 19:06:57, michaelbai wrote: > > torne@ would you like take a look? it might relate to WebView Zygote, > multiple > > process, or you may have other idea for how to implement this? > > I haven't looked at the CL in detail, but this is probably going to have > implications for webview multiprocess/zygote stuff. We added the > externalService > functionality to Android N so that WebView apps could bind to renderers > declared > in the webview/chrome manifest and run them *associated with the calling > app's > UID* instead of webview/chrome's. The webapk stuff here is doing the exact > opposite - having Chrome bind to renderers declared in the webapk but > leaving > the renderer associated with the webapk's UID. > > So if we introduce a special way to launch chrome/webview renderers (from a > zygote) then it's going to have to work differently in this case, and > right now > we haven't included the required alternate mode of operation in the design > I > don't think. We should probably schedule some time to chat about this? Who > would > be good to involve on the webapk side? > > https://codereview.chromium.org/2049843004/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/06/21 12:42:19, chromium-reviews wrote: > Richard, please feel free to add a meeting on my calendar. WebApk side will > be pkotwicz@ and me. Sent you an invite, thanks.
sievers@chromium.org changed reviewers: + sievers@chromium.org
https://codereview.chromium.org/2049843004/diff/340001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2049843004/diff/340001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:135: WebApkSandboxedProcessService.class); Would it also be possible to override through the manifest somehow that this uses a different service class?
https://codereview.chromium.org/2049843004/diff/340001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2049843004/diff/340001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:135: WebApkSandboxedProcessService.class); On 2016/06/23 17:18:50, sievers wrote: > Would it also be possible to override through the manifest somehow that this > uses a different service class? Do you ask is it possible that we may change the service name in the future? It might be possible, even though the chance is low.
https://codereview.chromium.org/2049843004/diff/340001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2049843004/diff/340001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:135: WebApkSandboxedProcessService.class); On 2016/06/23 17:34:56, Xi Han wrote: > On 2016/06/23 17:18:50, sievers wrote: > > Would it also be possible to override through the manifest somehow that this > > uses a different service class? > > Do you ask is it possible that we may change the service name in the future? It > might be possible, even though the chance is low. And also because maybe it's a nicer place than setting it globally here? Given that you already have to define each service in the manifest anyhow depending on how many you have (see ChildProcessLauncher.getNumberOfServices()). I'm also coming a bit from the point of view what really needs to be a content-public API here (see my comment in other patch). Generally all the multi-process stuff (or at least all the launching logic) is more of an implementation detail (at least on other platforms). Now on Android we have named services that an application already needs to define in the manifest. But maybe we could just handle all the configuration there. (Well, I guess for WebView we cannot though.)
https://codereview.chromium.org/2049843004/diff/340001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2049843004/diff/340001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:135: WebApkSandboxedProcessService.class); On 2016/06/23 17:45:33, sievers wrote: > On 2016/06/23 17:34:56, Xi Han wrote: > > On 2016/06/23 17:18:50, sievers wrote: > > > Would it also be possible to override through the manifest somehow that this > > > uses a different service class? > > > > Do you ask is it possible that we may change the service name in the future? > It > > might be possible, even though the chance is low. > > And also because maybe it's a nicer place than setting it globally here? > Given that you already have to define each service in the manifest anyhow > depending on how many you have (see ChildProcessLauncher.getNumberOfServices()). > > I'm also coming a bit from the point of view what really needs to be a > content-public API here (see my comment in other patch). > > Generally all the multi-process stuff (or at least all the launching logic) is > more of an implementation detail (at least on other platforms). > Now on Android we have named services that an application already needs to > define in the manifest. But maybe we could just handle all the configuration > there. (Well, I guess for WebView we cannot though.) Sounds like maybe we need to introduce something like: <meta-data android:name="org.chromium.content.browser.CLASS_SANDBOXED_SERVICES" android:value="{{ SandboxedProcessServiceClass }}"/> <meta-data android:name="org.chromium.content.browser.CLASS_PRIVILEGED_SERVICES" android:value="{{ PriviledgedProcessServiceClass }}"/> Only one concern is that we always have to read metadata when the class is needed.
On 2016/06/23 18:00:26, Xi Han wrote: > https://codereview.chromium.org/2049843004/diff/340001/chrome/android/java/sr... > File > chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java > (right): > > https://codereview.chromium.org/2049843004/diff/340001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:135: > WebApkSandboxedProcessService.class); > On 2016/06/23 17:45:33, sievers wrote: > > On 2016/06/23 17:34:56, Xi Han wrote: > > > On 2016/06/23 17:18:50, sievers wrote: > > > > Would it also be possible to override through the manifest somehow that > this > > > > uses a different service class? > > > > > > Do you ask is it possible that we may change the service name in the future? > > It > > > might be possible, even though the chance is low. > > > > And also because maybe it's a nicer place than setting it globally here? > > Given that you already have to define each service in the manifest anyhow > > depending on how many you have (see > ChildProcessLauncher.getNumberOfServices()). > > > > I'm also coming a bit from the point of view what really needs to be a > > content-public API here (see my comment in other patch). > > > > Generally all the multi-process stuff (or at least all the launching logic) is > > more of an implementation detail (at least on other platforms). > > Now on Android we have named services that an application already needs to > > define in the manifest. But maybe we could just handle all the configuration > > there. (Well, I guess for WebView we cannot though.) > > > Sounds like maybe we need to introduce something like: > <meta-data android:name="org.chromium.content.browser.CLASS_SANDBOXED_SERVICES" > android:value="{{ SandboxedProcessServiceClass }}"/> > <meta-data android:name="org.chromium.content.browser.CLASS_PRIVILEGED_SERVICES" > android:value="{{ PriviledgedProcessServiceClass }}"/> I like that idea. > Only one concern is that we always have to read metadata when the class is > needed. Could we just do it once in initConnectionAllocatorsIfNecessary() when we also read NUM_SANDBOXED_SERVICES from the manifest?
On 2016/06/23 20:24:04, sievers wrote: > On 2016/06/23 18:00:26, Xi Han wrote: > > > https://codereview.chromium.org/2049843004/diff/340001/chrome/android/java/sr... > > File > > > chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java > > (right): > > > > > https://codereview.chromium.org/2049843004/diff/340001/chrome/android/java/sr... > > > chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:135: > > WebApkSandboxedProcessService.class); > > On 2016/06/23 17:45:33, sievers wrote: > > > On 2016/06/23 17:34:56, Xi Han wrote: > > > > On 2016/06/23 17:18:50, sievers wrote: > > > > > Would it also be possible to override through the manifest somehow that > > this > > > > > uses a different service class? > > > > > > > > Do you ask is it possible that we may change the service name in the > future? > > > It > > > > might be possible, even though the chance is low. > > > > > > And also because maybe it's a nicer place than setting it globally here? > > > Given that you already have to define each service in the manifest anyhow > > > depending on how many you have (see > > ChildProcessLauncher.getNumberOfServices()). > > > > > > I'm also coming a bit from the point of view what really needs to be a > > > content-public API here (see my comment in other patch). > > > > > > Generally all the multi-process stuff (or at least all the launching logic) > is > > > more of an implementation detail (at least on other platforms). > > > Now on Android we have named services that an application already needs to > > > define in the manifest. But maybe we could just handle all the configuration > > > there. (Well, I guess for WebView we cannot though.) > > > > > > Sounds like maybe we need to introduce something like: > > <meta-data > android:name="org.chromium.content.browser.CLASS_SANDBOXED_SERVICES" > > android:value="{{ SandboxedProcessServiceClass }}"/> > > <meta-data > android:name="org.chromium.content.browser.CLASS_PRIVILEGED_SERVICES" > > android:value="{{ PriviledgedProcessServiceClass }}"/> > > I like that idea. > > > Only one concern is that we always have to read metadata when the class is > > needed. > > Could we just do it once in initConnectionAllocatorsIfNecessary() when we also > read NUM_SANDBOXED_SERVICES from the manifest? It is doable, and we need to add a commanline flag for the testing apks which don't have these fields declared.
On 2016/06/23 20:42:03, Xi Han wrote: > On 2016/06/23 20:24:04, sievers wrote: > > On 2016/06/23 18:00:26, Xi Han wrote: > > > > > > https://codereview.chromium.org/2049843004/diff/340001/chrome/android/java/sr... > > > File > > > > > > chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java > > > (right): > > > > > > > > > https://codereview.chromium.org/2049843004/diff/340001/chrome/android/java/sr... > > > > > > chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:135: > > > WebApkSandboxedProcessService.class); > > > On 2016/06/23 17:45:33, sievers wrote: > > > > On 2016/06/23 17:34:56, Xi Han wrote: > > > > > On 2016/06/23 17:18:50, sievers wrote: > > > > > > Would it also be possible to override through the manifest somehow > that > > > this > > > > > > uses a different service class? > > > > > > > > > > Do you ask is it possible that we may change the service name in the > > future? > > > > It > > > > > might be possible, even though the chance is low. > > > > > > > > And also because maybe it's a nicer place than setting it globally here? > > > > Given that you already have to define each service in the manifest anyhow > > > > depending on how many you have (see > > > ChildProcessLauncher.getNumberOfServices()). > > > > > > > > I'm also coming a bit from the point of view what really needs to be a > > > > content-public API here (see my comment in other patch). > > > > > > > > Generally all the multi-process stuff (or at least all the launching > logic) > > is > > > > more of an implementation detail (at least on other platforms). > > > > Now on Android we have named services that an application already needs to > > > > define in the manifest. But maybe we could just handle all the > configuration > > > > there. (Well, I guess for WebView we cannot though.) > > > > > > > > > Sounds like maybe we need to introduce something like: > > > <meta-data > > android:name="org.chromium.content.browser.CLASS_SANDBOXED_SERVICES" > > > android:value="{{ SandboxedProcessServiceClass }}"/> > > > <meta-data > > android:name="org.chromium.content.browser.CLASS_PRIVILEGED_SERVICES" > > > android:value="{{ PriviledgedProcessServiceClass }}"/> > > > > I like that idea. > > > > > Only one concern is that we always have to read metadata when the class is > > > needed. > > > > Could we just do it once in initConnectionAllocatorsIfNecessary() when we also > > read NUM_SANDBOXED_SERVICES from the manifest? > > > It is doable, and we need to add a commanline flag for the testing apks which > don't have these fields declared. If they are WebApk specific activities in chrome/, shouldn't they be declaring those? If those are tests inside content/, then can we just add an override '*ForTesting()' in ChildProcessLauncher or so?
On 2016/06/23 21:38:57, sievers wrote: > On 2016/06/23 20:42:03, Xi Han wrote: > > On 2016/06/23 20:24:04, sievers wrote: > > > On 2016/06/23 18:00:26, Xi Han wrote: > > > > > > > > > > https://codereview.chromium.org/2049843004/diff/340001/chrome/android/java/sr... > > > > File > > > > > > > > > > chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java > > > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2049843004/diff/340001/chrome/android/java/sr... > > > > > > > > > > chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:135: > > > > WebApkSandboxedProcessService.class); > > > > On 2016/06/23 17:45:33, sievers wrote: > > > > > On 2016/06/23 17:34:56, Xi Han wrote: > > > > > > On 2016/06/23 17:18:50, sievers wrote: > > > > > > > Would it also be possible to override through the manifest somehow > > that > > > > this > > > > > > > uses a different service class? > > > > > > > > > > > > Do you ask is it possible that we may change the service name in the > > > future? > > > > > It > > > > > > might be possible, even though the chance is low. > > > > > > > > > > And also because maybe it's a nicer place than setting it globally here? > > > > > Given that you already have to define each service in the manifest > anyhow > > > > > depending on how many you have (see > > > > ChildProcessLauncher.getNumberOfServices()). > > > > > > > > > > I'm also coming a bit from the point of view what really needs to be a > > > > > content-public API here (see my comment in other patch). > > > > > > > > > > Generally all the multi-process stuff (or at least all the launching > > logic) > > > is > > > > > more of an implementation detail (at least on other platforms). > > > > > Now on Android we have named services that an application already needs > to > > > > > define in the manifest. But maybe we could just handle all the > > configuration > > > > > there. (Well, I guess for WebView we cannot though.) > > > > > > > > > > > > Sounds like maybe we need to introduce something like: > > > > <meta-data > > > android:name="org.chromium.content.browser.CLASS_SANDBOXED_SERVICES" > > > > android:value="{{ SandboxedProcessServiceClass }}"/> > > > > <meta-data > > > android:name="org.chromium.content.browser.CLASS_PRIVILEGED_SERVICES" > > > > android:value="{{ PriviledgedProcessServiceClass }}"/> > > > > > > I like that idea. > > > > > > > Only one concern is that we always have to read metadata when the class is > > > > needed. > > > > > > Could we just do it once in initConnectionAllocatorsIfNecessary() when we > also > > > read NUM_SANDBOXED_SERVICES from the manifest? > > > > > > It is doable, and we need to add a commanline flag for the testing apks which > > don't have these fields declared. > > If they are WebApk specific activities in chrome/, shouldn't they be declaring > those? > > If those are tests inside content/, then can we just add an override > '*ForTesting()' in ChildProcessLauncher or so? I mean a flag like SWITCH_NUM_SANDBOXED_SERVICES_FOR_TESTING to bypass some tests that use fake context and can't get the real service name in AndroidManifest.xml:)
On 2016/06/23 23:07:34, Xi Han wrote: > On 2016/06/23 21:38:57, sievers wrote: > > On 2016/06/23 20:42:03, Xi Han wrote: > > > On 2016/06/23 20:24:04, sievers wrote: > > > > On 2016/06/23 18:00:26, Xi Han wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/2049843004/diff/340001/chrome/android/java/sr... > > > > > File > > > > > > > > > > > > > > > chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java > > > > > (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2049843004/diff/340001/chrome/android/java/sr... > > > > > > > > > > > > > > > chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:135: > > > > > WebApkSandboxedProcessService.class); > > > > > On 2016/06/23 17:45:33, sievers wrote: > > > > > > On 2016/06/23 17:34:56, Xi Han wrote: > > > > > > > On 2016/06/23 17:18:50, sievers wrote: > > > > > > > > Would it also be possible to override through the manifest somehow > > > that > > > > > this > > > > > > > > uses a different service class? > > > > > > > > > > > > > > Do you ask is it possible that we may change the service name in the > > > > future? > > > > > > It > > > > > > > might be possible, even though the chance is low. > > > > > > > > > > > > And also because maybe it's a nicer place than setting it globally > here? > > > > > > Given that you already have to define each service in the manifest > > anyhow > > > > > > depending on how many you have (see > > > > > ChildProcessLauncher.getNumberOfServices()). > > > > > > > > > > > > I'm also coming a bit from the point of view what really needs to be a > > > > > > content-public API here (see my comment in other patch). > > > > > > > > > > > > Generally all the multi-process stuff (or at least all the launching > > > logic) > > > > is > > > > > > more of an implementation detail (at least on other platforms). > > > > > > Now on Android we have named services that an application already > needs > > to > > > > > > define in the manifest. But maybe we could just handle all the > > > configuration > > > > > > there. (Well, I guess for WebView we cannot though.) > > > > > > > > > > > > > > > Sounds like maybe we need to introduce something like: > > > > > <meta-data > > > > android:name="org.chromium.content.browser.CLASS_SANDBOXED_SERVICES" > > > > > android:value="{{ SandboxedProcessServiceClass }}"/> > > > > > <meta-data > > > > android:name="org.chromium.content.browser.CLASS_PRIVILEGED_SERVICES" > > > > > android:value="{{ PriviledgedProcessServiceClass }}"/> > > > > > > > > I like that idea. > > > > > > > > > Only one concern is that we always have to read metadata when the class > is > > > > > needed. > > > > > > > > Could we just do it once in initConnectionAllocatorsIfNecessary() when we > > also > > > > read NUM_SANDBOXED_SERVICES from the manifest? > > > > > > > > > It is doable, and we need to add a commanline flag for the testing apks > which > > > don't have these fields declared. > > > > If they are WebApk specific activities in chrome/, shouldn't they be declaring > > those? > > > > If those are tests inside content/, then can we just add an override > > '*ForTesting()' in ChildProcessLauncher or so? > > > I mean a flag like SWITCH_NUM_SANDBOXED_SERVICES_FOR_TESTING to bypass some > tests that use fake context and can't get the real service name in > AndroidManifest.xml:) Yea that's fine, or even just a static method on ChildProcessLauncher to override it for testing. But why not set it in every manifest? I assume most of these use content shell and that would want to set it correctly anyhow.
On 2016/06/23 23:39:28, sievers wrote: > On 2016/06/23 23:07:34, Xi Han wrote: > > On 2016/06/23 21:38:57, sievers wrote: > > > On 2016/06/23 20:42:03, Xi Han wrote: > > > > On 2016/06/23 20:24:04, sievers wrote: > > > > > On 2016/06/23 18:00:26, Xi Han wrote: > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2049843004/diff/340001/chrome/android/java/sr... > > > > > > File > > > > > > > > > > > > > > > > > > > > > chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java > > > > > > (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2049843004/diff/340001/chrome/android/java/sr... > > > > > > > > > > > > > > > > > > > > > chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:135: > > > > > > WebApkSandboxedProcessService.class); > > > > > > On 2016/06/23 17:45:33, sievers wrote: > > > > > > > On 2016/06/23 17:34:56, Xi Han wrote: > > > > > > > > On 2016/06/23 17:18:50, sievers wrote: > > > > > > > > > Would it also be possible to override through the manifest > somehow > > > > that > > > > > > this > > > > > > > > > uses a different service class? > > > > > > > > > > > > > > > > Do you ask is it possible that we may change the service name in > the > > > > > future? > > > > > > > It > > > > > > > > might be possible, even though the chance is low. > > > > > > > > > > > > > > And also because maybe it's a nicer place than setting it globally > > here? > > > > > > > Given that you already have to define each service in the manifest > > > anyhow > > > > > > > depending on how many you have (see > > > > > > ChildProcessLauncher.getNumberOfServices()). > > > > > > > > > > > > > > I'm also coming a bit from the point of view what really needs to be > a > > > > > > > content-public API here (see my comment in other patch). > > > > > > > > > > > > > > Generally all the multi-process stuff (or at least all the launching > > > > logic) > > > > > is > > > > > > > more of an implementation detail (at least on other platforms). > > > > > > > Now on Android we have named services that an application already > > needs > > > to > > > > > > > define in the manifest. But maybe we could just handle all the > > > > configuration > > > > > > > there. (Well, I guess for WebView we cannot though.) > > > > > > > > > > > > > > > > > > Sounds like maybe we need to introduce something like: > > > > > > <meta-data > > > > > android:name="org.chromium.content.browser.CLASS_SANDBOXED_SERVICES" > > > > > > android:value="{{ SandboxedProcessServiceClass }}"/> > > > > > > <meta-data > > > > > android:name="org.chromium.content.browser.CLASS_PRIVILEGED_SERVICES" > > > > > > android:value="{{ PriviledgedProcessServiceClass }}"/> > > > > > > > > > > I like that idea. > > > > > > > > > > > Only one concern is that we always have to read metadata when the > class > > is > > > > > > needed. > > > > > > > > > > Could we just do it once in initConnectionAllocatorsIfNecessary() when > we > > > also > > > > > read NUM_SANDBOXED_SERVICES from the manifest? > > > > > > > > > > > > It is doable, and we need to add a commanline flag for the testing apks > > which > > > > don't have these fields declared. > > > > > > If they are WebApk specific activities in chrome/, shouldn't they be > declaring > > > those? > > > > > > If those are tests inside content/, then can we just add an override > > > '*ForTesting()' in ChildProcessLauncher or so? > > > > > > I mean a flag like SWITCH_NUM_SANDBOXED_SERVICES_FOR_TESTING to bypass some > > tests that use fake context and can't get the real service name in > > AndroidManifest.xml:) > > Yea that's fine, or even just a static method on ChildProcessLauncher to > override it for testing. > > But why not set it in every manifest? I assume most of these use content shell > and that would want to set it correctly anyhow. And I thought the only ones that would want to use something different than the default ("SandboxedProcessService") would be the tests you are adding in the other patch, and which are specifically testing that you can override the name :)
I introduce the SANDBOXED_SERVICES_NAME in AndroidManifest.xml, and the getClassOfService() returns the service class if exists in the meta-data, or returns the default SandboxedProcessService class. Didn't introduce the metadata for privileged process, since no one has declared a different one yet. PTAL, thanks!
https://codereview.chromium.org/2049843004/diff/380001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java (right): https://codereview.chromium.org/2049843004/diff/380001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:295: return SandboxedProcessService.class; Should this throw an exception since it's a malformatted entry? https://codereview.chromium.org/2049843004/diff/380001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:302: return SandboxedProcessService.class; And maybe this also? https://codereview.chromium.org/2049843004/diff/380001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:322: getClassOfService(context, true, packageName))); Doesn't it ignore the class name you set in ChildProcessCreationParams for web apks here?
Patchset #12 (id:400001) has been deleted
Hi Daniel, I updated the logic in getNum(Class)OfService to prevent tests failures when passing an arbitrary package name in CL (https://codereview.chromium.org/2076583002/).PTAL, thanks! https://codereview.chromium.org/2049843004/diff/380001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java (right): https://codereview.chromium.org/2049843004/diff/380001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:295: return SandboxedProcessService.class; On 2016/06/24 19:54:32, sievers wrote: > Should this throw an exception since it's a malformatted entry? Done. https://codereview.chromium.org/2049843004/diff/380001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:302: return SandboxedProcessService.class; On 2016/06/24 19:54:32, sievers wrote: > And maybe this also? This causes ChildProcessLauncherTest which with a fake external apk pakcage name failed. Update the logic here and in getNumOfService(): always check whether have a command line switch for these two fields. https://codereview.chromium.org/2049843004/diff/380001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:322: getClassOfService(context, true, packageName))); On 2016/06/24 19:54:32, sievers wrote: > Doesn't it ignore the class name you set in ChildProcessCreationParams for web > apks here? Forgot to clean up the WebApkActivity.
lgtm https://codereview.chromium.org/2049843004/diff/420001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/init/ChromeBrowserInitializer.java (right): https://codereview.chromium.org/2049843004/diff/420001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/init/ChromeBrowserInitializer.java:321: ChildProcessCreationParams.set(creationParams); Is this still needed? https://codereview.chromium.org/2049843004/diff/420001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java (right): https://codereview.chromium.org/2049843004/diff/420001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:311: return SandboxedProcessService.class; This should probably return PrivilegedProcessService.class if |!inSandbox|. (Launching the GPU process as sandboxed will cause subtle problems on SELinux enabled devices where video playback will break.) And I would maybe also throw an exception if |appInfo.metaData| is valid but serviceName somehow still ends up being null. (I guess if you put something that's not a string in the manifest?)
https://codereview.chromium.org/2049843004/diff/420001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/init/ChromeBrowserInitializer.java (right): https://codereview.chromium.org/2049843004/diff/420001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/init/ChromeBrowserInitializer.java:321: ChildProcessCreationParams.set(creationParams); On 2016/06/24 21:33:40, sievers wrote: > Is this still needed? Since I am not sure whether we can remove ChildProcessCreationParams.set(creationParams) entirely, so the if check is still needed. It prevent the creation parameter being reset, since we still need the package name within it. https://codereview.chromium.org/2049843004/diff/420001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java (right): https://codereview.chromium.org/2049843004/diff/420001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:311: return SandboxedProcessService.class; On 2016/06/24 21:33:40, sievers wrote: > This should probably return PrivilegedProcessService.class if |!inSandbox|. > (Launching the GPU process as sandboxed will cause subtle problems on SELinux > enabled devices where video playback will break.) The first check is |!inSandbox|, and already returns PrivilegedProcessService.class for that case. Do I miss your point? > And I would maybe also throw an exception if |appInfo.metaData| is valid but > serviceName somehow still ends up being null. (I guess if you put something > that's not a string in the manifest?) The manifest could have other kinds of metadata, so even the |appInfo.metaData| is valid, it could still miss the serviceName. So we can't tell whether missing the service name or the service name isn't a string, when calling |appInfo.metaData.getString(SANDBOXED_SERVICES_ NAME_KEY)|. So I split the error cases as followings: 1) null service name in metadata: use the default SandboxedProcessService. 2) have service name, but not valid: throw "Illegal meta data value: the child service class doesn't exist" error.
The CQ bit was checked by hanxi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkotwicz@chromium.org, mariakhomenko@chromium.org Link to the patchset: https://codereview.chromium.org/2049843004/#ps420001 (title: "Always check command line flags in getNum(Class)ofService().")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Upstream: Renderers are running in WebAPKs. This Cl includes the following things: 1. Introduce WebApkSandboxedProcessService which works the same as Chrome's ChildProcessService, but loads Chrome's code via Chrome's ClassLoader. 2. Move most of the functionality from ChildProcessService to ChildProcessServiceImpl, so we could let WebApkSandboxedProcessService load the ChildProcessServiceImpl class and create an Impl object which loads Chrome's native libraries and creates renderer process. The ChildProcessService can't be loaded directly through the same way, it might because this class extends the Android Service class and complicates things. 3. Rename child_process_service.h(.cc) to child_process_service_impl.h(.cc) and updates the JNI register. 4. Pass in the class name of WebApkSandboxedProcessService via ChildProcessCreationParams in WebApkActivity. So ChildProcessLauncher knows the class name of the service to connect. BUG=609122 ========== to ========== Upstream: Renderers are running in WebAPKs. This Cl includes the following things: 1. Introduce WebApkSandboxedProcessService which works the same as Chrome's ChildProcessService, but loads Chrome's code via Chrome's ClassLoader. 2. Move most of the functionality from ChildProcessService to ChildProcessServiceImpl, so we could let WebApkSandboxedProcessService load the ChildProcessServiceImpl class and create an Impl object which loads Chrome's native libraries and creates renderer process. The ChildProcessService can't be loaded directly through the same way, it might because this class extends the Android Service class and complicates things. 3. Rename child_process_service.h(.cc) to child_process_service_impl.h(.cc) and updates the JNI register. 4. Pass in the class name of WebApkSandboxedProcessService via ChildProcessCreationParams in WebApkActivity. So ChildProcessLauncher knows the class name of the service to connect. BUG=609122 ==========
Message was sent while issue was closed.
Committed patchset #12 (id:420001)
Message was sent while issue was closed.
Description was changed from ========== Upstream: Renderers are running in WebAPKs. This Cl includes the following things: 1. Introduce WebApkSandboxedProcessService which works the same as Chrome's ChildProcessService, but loads Chrome's code via Chrome's ClassLoader. 2. Move most of the functionality from ChildProcessService to ChildProcessServiceImpl, so we could let WebApkSandboxedProcessService load the ChildProcessServiceImpl class and create an Impl object which loads Chrome's native libraries and creates renderer process. The ChildProcessService can't be loaded directly through the same way, it might because this class extends the Android Service class and complicates things. 3. Rename child_process_service.h(.cc) to child_process_service_impl.h(.cc) and updates the JNI register. 4. Pass in the class name of WebApkSandboxedProcessService via ChildProcessCreationParams in WebApkActivity. So ChildProcessLauncher knows the class name of the service to connect. BUG=609122 ========== to ========== Upstream: Renderers are running in WebAPKs. This Cl includes the following things: 1. Introduce WebApkSandboxedProcessService which works the same as Chrome's ChildProcessService, but loads Chrome's code via Chrome's ClassLoader. 2. Move most of the functionality from ChildProcessService to ChildProcessServiceImpl, so we could let WebApkSandboxedProcessService load the ChildProcessServiceImpl class and create an Impl object which loads Chrome's native libraries and creates renderer process. The ChildProcessService can't be loaded directly through the same way, it might because this class extends the Android Service class and complicates things. 3. Rename child_process_service.h(.cc) to child_process_service_impl.h(.cc) and updates the JNI register. 4. Pass in the class name of WebApkSandboxedProcessService via ChildProcessCreationParams in WebApkActivity. So ChildProcessLauncher knows the class name of the service to connect. BUG=609122 Committed: https://crrev.com/3f9d474841d96c1cf75186190d20379b074eb8fb Cr-Commit-Position: refs/heads/master@{#402026} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/3f9d474841d96c1cf75186190d20379b074eb8fb Cr-Commit-Position: refs/heads/master@{#402026} |
