Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2787963003:
WebView: Refactor PlatformServiceBridge.getInstance (Closed)
|
Created:
3 years, 8 months ago by paulmiller Modified:
3 years, 8 months ago CC:
chromium-reviews, kalyank, sadrul, android-webview-reviews_chromium.org, agrieve+watch_chromium.org, timvolodine Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionWebView: Refactor PlatformServiceBridge.getInstance
Split getInstance into a creator (getOrCreateInstance), a getter
(getInstance), and setter for testing (injectInstance). This way, only
the first user of PlatformServiceBridge need supply a Context.
injectInstance cuts down on some redundant test code.
Also remove the lock around creation, since that happens in
WebViewChromiumFactoryProvider.startChromiumLocked, before there are any
other users.
BUG=681805
BUG=706495
Review-Url: https://codereview.chromium.org/2787963003
Cr-Commit-Position: refs/heads/master@{#462134}
Committed: https://chromium.googlesource.com/chromium/src/+/2009b0e3ce935a30f27334687773f7f3abc809cb
Patch Set 1 #Patch Set 2 : make injectInstance less picky #
Total comments: 13
Patch Set 3 : once more with feeling and getOrCreateInstance #
Total comments: 5
Patch Set 4 : hoist getOrCreateInstance call and simplify tests #
Total comments: 6
Patch Set 5 : volatile sInstance and comments #
Messages
Total messages: 30 (12 generated)
Description was changed from ========== WebView: Refactor PlatformServiceBridge.getInstance Split getInstance into a creator (createInstance), a getter (getInstance), and setter for testing (injectInstance). This way, only the first user of PlatformServiceBridge need supply a Context. injectInstance cuts down on some redundant test code. Also remove the lock around creation, since that happens in WebViewChromiumFactoryProvider.startChromiumLocked, before there are any other users. BUG=681805 BUG=706495 ========== to ========== WebView: Refactor PlatformServiceBridge.getInstance Split getInstance into a creator (createInstance), a getter (getInstance), and setter for testing (injectInstance). This way, only the first user of PlatformServiceBridge need supply a Context. injectInstance cuts down on some redundant test code. Also remove the lock around creation, since that happens in WebViewChromiumFactoryProvider.startChromiumLocked, before there are any other users. BUG=681805 BUG=706495 ==========
paulmiller@chromium.org changed reviewers: + gsennton@chromium.org, michaelbai@chromium.org, timvolodine@chromium.org
On 2017/03/30 23:12:36, paulmiller wrote: > mailto:paulmiller@chromium.org changed reviewers: > + mailto:gsennton@chromium.org, mailto:michaelbai@chromium.org, mailto:timvolodine@chromium.org PTAL. Added Michael for the getInstance change, Gustav for the minidump bit, and Tim for the lock removal and because I'll be updating his downstream getInstance call next.
paulmiller@chromium.org changed reviewers: - michaelbai@chromium.org
On 2017/03/30 23:16:03, paulmiller wrote: > On 2017/03/30 23:12:36, paulmiller wrote: > > mailto:paulmiller@chromium.org changed reviewers: > > + mailto:gsennton@chromium.org, mailto:michaelbai@chromium.org, > mailto:timvolodine@chromium.org > > PTAL. Added Michael for the getInstance change, Gustav for the minidump bit, and > Tim for the lock removal and because I'll be updating his downstream getInstance > call next. Michael is out. Just Gustav and Tim, then.
paulmiller@chromium.org changed reviewers: + tobiasjs@chromium.org - gsennton@chromium.org, timvolodine@chromium.org
On 2017/03/31 21:33:42, paulmiller wrote: > On 2017/03/30 23:16:03, paulmiller wrote: > > On 2017/03/30 23:12:36, paulmiller wrote: > > > mailto:paulmiller@chromium.org changed reviewers: > > > + mailto:gsennton@chromium.org, mailto:michaelbai@chromium.org, > > mailto:timvolodine@chromium.org > > > > PTAL. Added Michael for the getInstance change, Gustav for the minidump bit, > and > > Tim for the lock removal and because I'll be updating his downstream > getInstance > > call next. > > Michael is out. Just Gustav and Tim, then. Nerp, that doesn't work, because then there're no owners. Gustav & Tim to CC, Toby to review.
gsennton@chromium.org changed reviewers: + gsennton@chromium.org
https://codereview.chromium.org/2787963003/diff/20001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/crash/AwMinidumpUploaderDelegate.java (right): https://codereview.chromium.org/2787963003/diff/20001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/crash/AwMinidumpUploaderDelegate.java:79: PlatformServiceBridge.getInstance().queryMetricsSetting(new ValueCallback<Boolean>() { When is the PlatformServiceBridge created in this case? If I'm not completely mistaken this runs in a separate process which hasn't run any code from WebViewChromiumFactoryProvider.
https://codereview.chromium.org/2787963003/diff/20001/android_webview/glue/ja... File android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java (right): https://codereview.chromium.org/2787963003/diff/20001/android_webview/glue/ja... android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java:423: PlatformServiceBridge.createInstance(context).queryMetricsSetting( Keep this getInstance() and add a createInstance() earlier in the method? I had to check that handleMinidumps didn't rely on the instance having been created, and I'm not certain that there aren't other methods that start operations on other threads above. https://codereview.chromium.org/2787963003/diff/20001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/PlatformServiceBridge.java (right): https://codereview.chromium.org/2787963003/diff/20001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/PlatformServiceBridge.java:30: public static PlatformServiceBridge createInstance(Context appContext) { Given that this only ever returns sInstance (or throws), maybe it shouldn't return a value? https://codereview.chromium.org/2787963003/diff/20001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/crash/MinidumpUploaderTest.java (right): https://codereview.chromium.org/2787963003/diff/20001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/crash/MinidumpUploaderTest.java:64: private static class BooleanTestPlatformServiceBridge extends TestPlatformServiceBridge { Can't this just be replaced with a PermissionTestPlatformServiceBridge created with MockCrashReportingPermissionManager() {{ mIsUserPermitted = enabled; }};? And then you could fold it all back into TestPlatformServiceBridge. Although, to be honest, this seems backwards. The PSB should be the source of truth regarding whether usage and crash reporting is permitted by the user. Otherwise: the name should mention what boolean this is controlling.
timvolodine@chromium.org changed reviewers: + timvolodine@chromium.org
https://codereview.chromium.org/2787963003/diff/20001/android_webview/glue/ja... File android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java (right): https://codereview.chromium.org/2787963003/diff/20001/android_webview/glue/ja... android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java:422: // Actions conditioned on whether the Android Checkbox is toggled on perhaps also provide a comment that the "createInstance" call creates a singleton instance used elsewhere and that it's crucial to do this here at startup i.e. the location of this code is important.
https://codereview.chromium.org/2787963003/diff/20001/android_webview/glue/ja... File android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java (right): https://codereview.chromium.org/2787963003/diff/20001/android_webview/glue/ja... android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java:422: // Actions conditioned on whether the Android Checkbox is toggled on On 2017/04/03 12:18:52, timvolodine wrote: > perhaps also provide a comment that the "createInstance" call creates a > singleton instance used elsewhere and that it's crucial to do this here at > startup i.e. the location of this code is important. Done. https://codereview.chromium.org/2787963003/diff/20001/android_webview/glue/ja... android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java:423: PlatformServiceBridge.createInstance(context).queryMetricsSetting( On 2017/04/03 10:40:06, Tobias Sargeant wrote: > Keep this getInstance() and add a createInstance() earlier in the method? I had > to check that handleMinidumps didn't rely on the instance having been created, > and I'm not certain that there aren't other methods that start operations on > other threads above. IMHO, if we preemptively create our singletons as early as possible, that removes the create-on-first-use benefit of singletons. The only case we need to worry about is calling "getInstance(null)" (because that's the version that can't create if it's the first use) and there's only 1 of those: https://cs.chromium.org/search/?q=PlatformServiceBridge%5C.getInstance%5C(nul... https://codereview.chromium.org/2787963003/diff/20001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/PlatformServiceBridge.java (right): https://codereview.chromium.org/2787963003/diff/20001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/PlatformServiceBridge.java:30: public static PlatformServiceBridge createInstance(Context appContext) { On 2017/04/03 10:40:06, Tobias Sargeant wrote: > Given that this only ever returns sInstance (or throws), maybe it shouldn't > return a value? Why is that? If people are going to be calling getInstance immediately after createInstance, it seems more convenient to combine them. https://codereview.chromium.org/2787963003/diff/20001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/crash/AwMinidumpUploaderDelegate.java (right): https://codereview.chromium.org/2787963003/diff/20001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/crash/AwMinidumpUploaderDelegate.java:79: PlatformServiceBridge.getInstance().queryMetricsSetting(new ValueCallback<Boolean>() { On 2017/04/03 09:56:20, gsennton wrote: > When is the PlatformServiceBridge created in this case? If I'm not completely > mistaken this runs in a separate process which hasn't run any code from > WebViewChromiumFactoryProvider. Aye. I thought it would work because the tests passed, but testing in an actual app shows you're right. More experimentation shows that calling createInstance() exactly once is actually kind of tricky. E.g. this doc: https://developer.android.com/guide/components/activities/activity-lifecycle.... suggests you'll only get an onCreate call in a fresh process, so I figured it would be safe to call createInstance() in AwMinidumpUploadJobService.onCreate() or CrashReceiverService.onCreate(). But that didn't work, so I guess Services are different? The documentation isn't great. I think the solution is to make createInstance() more singleton-y: it shouldn't punish you for calling it multiple times, so you can just call it whenever it's convenient and not have to think so hard about whether it could have already been created. I'm renaming createInstance() to getOrCreateInstance() to reflect that. https://codereview.chromium.org/2787963003/diff/20001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/crash/MinidumpUploaderTest.java (right): https://codereview.chromium.org/2787963003/diff/20001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/crash/MinidumpUploaderTest.java:64: private static class BooleanTestPlatformServiceBridge extends TestPlatformServiceBridge { On 2017/04/03 10:40:06, Tobias Sargeant wrote: > Can't this just be replaced with a PermissionTestPlatformServiceBridge created > with > > MockCrashReportingPermissionManager() {{ mIsUserPermitted = enabled; }};? > > And then you could fold it all back into TestPlatformServiceBridge. > Done. > Although, to be honest, this seems backwards. The PSB should be the source of > truth regarding whether usage and crash reporting is permitted by the user. > > Otherwise: the name should mention what boolean this is controlling. I don't follow. Do you mean, injecting a mock PlatformServiceBridge is backwards?
Description was changed from ========== WebView: Refactor PlatformServiceBridge.getInstance Split getInstance into a creator (createInstance), a getter (getInstance), and setter for testing (injectInstance). This way, only the first user of PlatformServiceBridge need supply a Context. injectInstance cuts down on some redundant test code. Also remove the lock around creation, since that happens in WebViewChromiumFactoryProvider.startChromiumLocked, before there are any other users. BUG=681805 BUG=706495 ========== to ========== WebView: Refactor PlatformServiceBridge.getInstance Split getInstance into a creator (getOrCreateInstance), a getter (getInstance), and setter for testing (injectInstance). This way, only the first user of PlatformServiceBridge need supply a Context. injectInstance cuts down on some redundant test code. Also remove the lock around creation, since that happens in WebViewChromiumFactoryProvider.startChromiumLocked, before there are any other users. BUG=681805 BUG=706495 ==========
https://codereview.chromium.org/2787963003/diff/20001/android_webview/glue/ja... File android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java (right): https://codereview.chromium.org/2787963003/diff/20001/android_webview/glue/ja... android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java:423: PlatformServiceBridge.createInstance(context).queryMetricsSetting( On 2017/04/03 23:34:16, paulmiller wrote: > On 2017/04/03 10:40:06, Tobias Sargeant wrote: > > Keep this getInstance() and add a createInstance() earlier in the method? I > had > > to check that handleMinidumps didn't rely on the instance having been created, > > and I'm not certain that there aren't other methods that start operations on > > other threads above. > > IMHO, if we preemptively create our singletons as early as possible, that > removes the create-on-first-use benefit of singletons. The only case we need to > worry about is calling "getInstance(null)" (because that's the version that > can't create if it's the first use) and there's only 1 of those: > > https://cs.chromium.org/search/?q=PlatformServiceBridge%5C.getInstance%5C(nul... Is create-on-first-use really that much of a benefit, though? Especially if there are cases where we can't create? It becomes kind of hard to reason about whether those cases execute before or after creation of the singleton instance. In the example here it would be easy for the AsyncTask that handleMinidumps() creates to be changed to call getInstance (because it doesn't have a context to call getOrCreateInstance with) and have that be unintentionally racy. If we can ensure that that can't happen now by creating the instance earlier, shouldn't we? https://codereview.chromium.org/2787963003/diff/20001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/PlatformServiceBridge.java (right): https://codereview.chromium.org/2787963003/diff/20001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/PlatformServiceBridge.java:30: public static PlatformServiceBridge createInstance(Context appContext) { On 2017/04/03 23:34:16, paulmiller wrote: > On 2017/04/03 10:40:06, Tobias Sargeant wrote: > > Given that this only ever returns sInstance (or throws), maybe it shouldn't > > return a value? > > Why is that? If people are going to be calling getInstance immediately after > createInstance, it seems more convenient to combine them. My personal preference if there's a createInstance() and a getInstance() is not to favour convenience if that means that sometimes you'll call createInstance without using its return value. getOrCreateInstance() makes that question irrelevant however. https://codereview.chromium.org/2787963003/diff/20001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/crash/MinidumpUploaderTest.java (right): https://codereview.chromium.org/2787963003/diff/20001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/crash/MinidumpUploaderTest.java:64: private static class BooleanTestPlatformServiceBridge extends TestPlatformServiceBridge { On 2017/04/03 23:34:16, paulmiller wrote: > On 2017/04/03 10:40:06, Tobias Sargeant wrote: > > Can't this just be replaced with a PermissionTestPlatformServiceBridge created > > with > > > > MockCrashReportingPermissionManager() {{ mIsUserPermitted = enabled; }};? > > > > And then you could fold it all back into TestPlatformServiceBridge. > > > > Done. > > > Although, to be honest, this seems backwards. The PSB should be the source of > > truth regarding whether usage and crash reporting is permitted by the user. > > > > Otherwise: the name should mention what boolean this is controlling. > > I don't follow. Do you mean, injecting a mock PlatformServiceBridge is > backwards? I mean the fact that the PermissionTestPlatformServiceBridge is asking the (mocked) permission manager for the value of isUsageAndCrashReportingPermittedByUser(). It's the other way around in non-test code.
https://codereview.chromium.org/2787963003/diff/40001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwMetricsLogUploader.java (right): https://codereview.chromium.org/2787963003/diff/40001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwMetricsLogUploader.java:17: PlatformServiceBridge.getInstance().logMetrics(data); Apologies if you've only answered this somewhere else - I assume we are guaranteed to have called getOrCreateInstance from WebViewChromiumFactoryProvider before we call getInstance here? https://codereview.chromium.org/2787963003/diff/40001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/crash/MinidumpUploaderTest.java (right): https://codereview.chromium.org/2787963003/diff/40001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/crash/MinidumpUploaderTest.java:57: private static class TestPlatformServiceBridge extends PlatformServiceBridge { Do we need this class if we have a way of injecting a PermissionManager into PlatformServiceBridge? (or vice versa) https://codereview.chromium.org/2787963003/diff/40001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/crash/MinidumpUploaderTest.java:76: private class TestMinidumpUploaderImpl extends MinidumpUploaderImpl { Shouldn't this class still be static?
https://codereview.chromium.org/2787963003/diff/40001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwMetricsLogUploader.java (right): https://codereview.chromium.org/2787963003/diff/40001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwMetricsLogUploader.java:17: PlatformServiceBridge.getInstance().logMetrics(data); On 2017/04/04 13:47:54, gsennton wrote: > Apologies if you've only answered this somewhere else - I assume we are > guaranteed to have called getOrCreateInstance from > WebViewChromiumFactoryProvider before we call getInstance here? Yes. But if it confused you then I should probably add a comment. https://codereview.chromium.org/2787963003/diff/40001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/crash/MinidumpUploaderTest.java (right): https://codereview.chromium.org/2787963003/diff/40001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/crash/MinidumpUploaderTest.java:76: private class TestMinidumpUploaderImpl extends MinidumpUploaderImpl { On 2017/04/04 13:47:54, gsennton wrote: > Shouldn't this class still be static? I made it non-static so I could do getInstrumentation().getTargetContext(). And then createCallableListMinidumpUploader() also has to be non-static because it creates one of these.
> I mean the fact that the PermissionTestPlatformServiceBridge is asking the > (mocked) permission manager for the value of > isUsageAndCrashReportingPermittedByUser(). It's the other way around in non-test > code. Oh. Yeah, the test worked that way to begin with. I can switch it to the boolean version, so it doesn't depend on the permission manager.
PTAL at #4
LGTM. https://codereview.chromium.org/2787963003/diff/60001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/PlatformServiceBridge.java (right): https://codereview.chromium.org/2787963003/diff/60001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/PlatformServiceBridge.java:24: private static PlatformServiceBridge sInstance; Seeing as this is set once, but can be read from multiple threads, mark as volatile.
Wow, sorry for the mess in MinidumpUploaderTest, I really need to clean that up :(. Thanks Paul! https://codereview.chromium.org/2787963003/diff/60001/android_webview/glue/ja... File android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java (right): https://codereview.chromium.org/2787963003/diff/60001/android_webview/glue/ja... android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java:425: // Future calls to PlatformServiceBridge.getInstance() rely on it having been created here. This comment doesn't fit here anymore? https://codereview.chromium.org/2787963003/diff/60001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/PlatformServiceBridge.java (right): https://codereview.chromium.org/2787963003/diff/60001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/PlatformServiceBridge.java:24: private static PlatformServiceBridge sInstance; On 2017/04/05 13:00:21, Tobias Sargeant wrote: > Seeing as this is set once, but can be read from multiple threads, mark as > volatile. Huh, is this not always called on the UI thread? (if it is we should have a ThreadUtils assertion in getInstance()).
lgtm % the comments from the previous reply.
https://codereview.chromium.org/2787963003/diff/60001/android_webview/glue/ja... File android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java (right): https://codereview.chromium.org/2787963003/diff/60001/android_webview/glue/ja... android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java:425: // Future calls to PlatformServiceBridge.getInstance() rely on it having been created here. On 2017/04/05 15:21:13, gsennton wrote: > This comment doesn't fit here anymore? Done. https://codereview.chromium.org/2787963003/diff/60001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/PlatformServiceBridge.java (right): https://codereview.chromium.org/2787963003/diff/60001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/PlatformServiceBridge.java:24: private static PlatformServiceBridge sInstance; On 2017/04/05 13:00:21, Tobias Sargeant wrote: > Seeing as this is set once, but can be read from multiple threads, mark as > volatile. Done. https://codereview.chromium.org/2787963003/diff/60001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/PlatformServiceBridge.java:24: private static PlatformServiceBridge sInstance; On 2017/04/05 15:21:13, gsennton wrote: > On 2017/04/05 13:00:21, Tobias Sargeant wrote: > > Seeing as this is set once, but can be read from multiple threads, mark as > > volatile. > > Huh, is this not always called on the UI thread? (if it is we should have a > ThreadUtils assertion in getInstance()). Safe Browsing uses getInstance on a different thread. I'll add a comment to clarify.
paulmiller@chromium.org changed reviewers: - timvolodine@chromium.org
The CQ bit was checked by paulmiller@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gsennton@chromium.org, tobiasjs@chromium.org Link to the patchset: https://codereview.chromium.org/2787963003/#ps80001 (title: "volatile sInstance and comments")
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": 80001, "attempt_start_ts": 1491412141559080,
"parent_rev": "bb4de8dfcf55b06af46e21d9a2acb5e0837fd9bc", "commit_rev":
"2009b0e3ce935a30f27334687773f7f3abc809cb"}
Message was sent while issue was closed.
Description was changed from ========== WebView: Refactor PlatformServiceBridge.getInstance Split getInstance into a creator (getOrCreateInstance), a getter (getInstance), and setter for testing (injectInstance). This way, only the first user of PlatformServiceBridge need supply a Context. injectInstance cuts down on some redundant test code. Also remove the lock around creation, since that happens in WebViewChromiumFactoryProvider.startChromiumLocked, before there are any other users. BUG=681805 BUG=706495 ========== to ========== WebView: Refactor PlatformServiceBridge.getInstance Split getInstance into a creator (getOrCreateInstance), a getter (getInstance), and setter for testing (injectInstance). This way, only the first user of PlatformServiceBridge need supply a Context. injectInstance cuts down on some redundant test code. Also remove the lock around creation, since that happens in WebViewChromiumFactoryProvider.startChromiumLocked, before there are any other users. BUG=681805 BUG=706495 Review-Url: https://codereview.chromium.org/2787963003 Cr-Commit-Position: refs/heads/master@{#462134} Committed: https://chromium.googlesource.com/chromium/src/+/2009b0e3ce935a30f27334687773... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/2009b0e3ce935a30f27334687773... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
