Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2645413002:
Prepare to get the multiprocess setting from WebViewDelegate. (Closed)
DescriptionPrepare to get the multiprocess setting from WebViewDelegate.
In future OS versions, we should fetch the multiprocess-enabled setting
from WebViewDelegate instead of querying the setting directly. Add the
skeleton code to get this from the delegate under a build version
condition.
BUG=684489
Review-Url: https://codereview.chromium.org/2645413002
Cr-Commit-Position: refs/heads/master@{#445705}
Committed: https://chromium.googlesource.com/chromium/src/+/acc84fbd540feec6c0fa6b2165b0da7eeb8d0456
Patch Set 1 #
Total comments: 10
Patch Set 2 : Move version condition logic back to FactoryProvider #
Messages
Total messages: 15 (6 generated)
torne@chromium.org changed reviewers: + boliu@chromium.org, tobiasjs@chromium.org
Toby, can you take a look? Bo, does this approach fit with your plan for how to handle future features? (downstream CL also sent to you) https://codereview.chromium.org/2645413002/diff/1/android_webview/glue/java/s... File android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java (right): https://codereview.chromium.org/2645413002/diff/1/android_webview/glue/java/s... android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java:202: WebViewChromiumFactoryProvider(WebViewDelegate delegate) { This allows the subclass version of create() to create its own subclass of the ProxyDelegate instead of calling the constructor above.
On 2017/01/23 16:09:16, Torne wrote: > Toby, can you take a look? > > Bo, does this approach fit with your plan for how to handle future features? > (downstream CL also sent to you) > > https://codereview.chromium.org/2645413002/diff/1/android_webview/glue/java/s... > File > android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java > (right): > > https://codereview.chromium.org/2645413002/diff/1/android_webview/glue/java/s... > android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java:202: > WebViewChromiumFactoryProvider(WebViewDelegate delegate) { > This allows the subclass version of create() to create its own subclass of the > ProxyDelegate instead of calling the constructor above. LGTM.
I guess looking at the downstream CL, I'm a bit surprised there are so many FooForO things already. I guess when upstreaming, all the ForO classes, except ProviderFactory, can be merged into to the upstream classes directly, so should be ok. Small comments https://codereview.chromium.org/2645413002/diff/1/android_webview/glue/java/s... File android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java (right): https://codereview.chromium.org/2645413002/diff/1/android_webview/glue/java/s... android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java:202: WebViewChromiumFactoryProvider(WebViewDelegate delegate) { On 2017/01/23 16:09:16, Torne wrote: > This allows the subclass version of create() to create its own subclass of the > ProxyDelegate instead of calling the constructor above. protected? https://codereview.chromium.org/2645413002/diff/1/android_webview/glue/java/s... File android_webview/glue/java/src/com/android/webview/chromium/WebViewDelegateFactory.java (right): https://codereview.chromium.org/2645413002/diff/1/android_webview/glue/java/s... android_webview/glue/java/src/com/android/webview/chromium/WebViewDelegateFactory.java:119: static class ProxyDelegate implements WebViewDelegate { does protected work here too..? https://codereview.chromium.org/2645413002/diff/1/android_webview/glue/java/s... android_webview/glue/java/src/com/android/webview/chromium/WebViewDelegateFactory.java:210: // Multiprocess requires N or later. I suppose this class was never meant to contain implementation, only call forwarding, although clearly there are exceptions already..
Yeah, we'll get rid of all the ForO things after O is released other than the main factory provider, so I don't think it matters too much. https://codereview.chromium.org/2645413002/diff/1/android_webview/glue/java/s... File android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java (right): https://codereview.chromium.org/2645413002/diff/1/android_webview/glue/java/s... android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java:202: WebViewChromiumFactoryProvider(WebViewDelegate delegate) { On 2017/01/23 16:34:18, boliu wrote: > On 2017/01/23 16:09:16, Torne wrote: > > This allows the subclass version of create() to create its own subclass of the > > ProxyDelegate instead of calling the constructor above. > > protected? No point; the downstream version is in the same package and so it doesn't need to be protected for it to access it. https://codereview.chromium.org/2645413002/diff/1/android_webview/glue/java/s... File android_webview/glue/java/src/com/android/webview/chromium/WebViewDelegateFactory.java (right): https://codereview.chromium.org/2645413002/diff/1/android_webview/glue/java/s... android_webview/glue/java/src/com/android/webview/chromium/WebViewDelegateFactory.java:119: static class ProxyDelegate implements WebViewDelegate { On 2017/01/23 16:34:18, boliu wrote: > does protected work here too..? Likewise no point. https://codereview.chromium.org/2645413002/diff/1/android_webview/glue/java/s... android_webview/glue/java/src/com/android/webview/chromium/WebViewDelegateFactory.java:210: // Multiprocess requires N or later. On 2017/01/23 16:34:18, boliu wrote: > I suppose this class was never meant to contain implementation, only call > forwarding, although clearly there are exceptions already.. I was considering renaming it to Api22AndUpDelegate or something, but I want to keep this CL minimal for cherrypicking to M57. If you think that's a good idea I can do it in a followup.
lgtm to land, fwiw https://codereview.chromium.org/2645413002/diff/1/android_webview/glue/java/s... File android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java (right): https://codereview.chromium.org/2645413002/diff/1/android_webview/glue/java/s... android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java:202: WebViewChromiumFactoryProvider(WebViewDelegate delegate) { On 2017/01/23 17:27:30, Torne wrote: > On 2017/01/23 16:34:18, boliu wrote: > > On 2017/01/23 16:09:16, Torne wrote: > > > This allows the subclass version of create() to create its own subclass of > the > > > ProxyDelegate instead of calling the constructor above. > > > > protected? > > No point; the downstream version is in the same package and so it doesn't need > to be protected for it to access it. oh.. package is actually more strict than protected https://codereview.chromium.org/2645413002/diff/1/android_webview/glue/java/s... File android_webview/glue/java/src/com/android/webview/chromium/WebViewDelegateFactory.java (right): https://codereview.chromium.org/2645413002/diff/1/android_webview/glue/java/s... android_webview/glue/java/src/com/android/webview/chromium/WebViewDelegateFactory.java:210: // Multiprocess requires N or later. On 2017/01/23 17:27:30, Torne wrote: > On 2017/01/23 16:34:18, boliu wrote: > > I suppose this class was never meant to contain implementation, only call > > forwarding, although clearly there are exceptions already.. > > I was considering renaming it to Api22AndUpDelegate or something, but I want to > keep this CL minimal for cherrypicking to M57. If you think that's a good idea I > can do it in a followup. Err, no :p I meant at callsite, have a if <= N, do this, else, call delegate. The reason we have this thing at all is because the framework delegate didn't exist in api 21. Above 21, this should behave exactly like the framework delegate, ie some methods are only safe to call on certain api levels. That's my mental model anyway.. doesn't matter much either way, since there's already other implementation here.
https://codereview.chromium.org/2645413002/diff/1/android_webview/glue/java/s... File android_webview/glue/java/src/com/android/webview/chromium/WebViewDelegateFactory.java (right): https://codereview.chromium.org/2645413002/diff/1/android_webview/glue/java/s... android_webview/glue/java/src/com/android/webview/chromium/WebViewDelegateFactory.java:210: // Multiprocess requires N or later. On 2017/01/23 17:44:04, boliu wrote: > On 2017/01/23 17:27:30, Torne wrote: > > On 2017/01/23 16:34:18, boliu wrote: > > > I suppose this class was never meant to contain implementation, only call > > > forwarding, although clearly there are exceptions already.. > > > > I was considering renaming it to Api22AndUpDelegate or something, but I want > to > > keep this CL minimal for cherrypicking to M57. If you think that's a good idea > I > > can do it in a followup. > > Err, no :p > > I meant at callsite, have a if <= N, do this, else, call delegate. The reason we > have this thing at all is because the framework delegate didn't exist in api 21. > Above 21, this should behave exactly like the framework delegate, ie some > methods are only safe to call on certain api levels. > > That's my mental model anyway.. doesn't matter much either way, since there's > already other implementation here. I think that's a better idea actually; I'll do it that way instead, which means the initial version of this upstream just won't do anything..
Description was changed from ========== Prepare to get the multiprocess setting from WebViewDelegate. In future OS versions, we should fetch the multiprocess-enabled setting from WebViewDelegate instead of querying the setting directly. Move the current logic into ProxyDelegate so that it can be overridden. BUG= ========== to ========== Prepare to get the multiprocess setting from WebViewDelegate. In future OS versions, we should fetch the multiprocess-enabled setting from WebViewDelegate instead of querying the setting directly. Add the skeleton code to get this from the delegate under a build version condition. BUG=684489 ==========
updated version LGTM.
The CQ bit was checked by torne@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from boliu@chromium.org Link to the patchset: https://codereview.chromium.org/2645413002/#ps20001 (title: "Move version condition logic back to FactoryProvider")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1485258020212530,
"parent_rev": "6aace7622d23d02b6865b30a7b4a8e83d488a0d6", "commit_rev":
"acc84fbd540feec6c0fa6b2165b0da7eeb8d0456"}
Message was sent while issue was closed.
Description was changed from ========== Prepare to get the multiprocess setting from WebViewDelegate. In future OS versions, we should fetch the multiprocess-enabled setting from WebViewDelegate instead of querying the setting directly. Add the skeleton code to get this from the delegate under a build version condition. BUG=684489 ========== to ========== Prepare to get the multiprocess setting from WebViewDelegate. In future OS versions, we should fetch the multiprocess-enabled setting from WebViewDelegate instead of querying the setting directly. Add the skeleton code to get this from the delegate under a build version condition. BUG=684489 Review-Url: https://codereview.chromium.org/2645413002 Cr-Commit-Position: refs/heads/master@{#445705} Committed: https://chromium.googlesource.com/chromium/src/+/acc84fbd540feec6c0fa6b2165b0... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/acc84fbd540feec6c0fa6b2165b0... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
