Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1485743002:
Push API: test that default notification is shown on Android when needed. (Closed)
|
Created:
5 years ago by Michael van Ouwerkerk Modified:
5 years ago CC:
chromium-reviews, Peter Beverloo, mvanouwerkerk+watch_chromium.org, mlamouri+watch-notifications_chromium.org, johnme+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@PushMessagingInstrumentationTest Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPush API: test that the default notification is shown on Android when needed.
* Adds an instrumentation test.
* Plumbs onMessageHandled through to Java.
* Decrements inflight messages after EnforceUserVisibleOnlyRequirements to prevent it (and its multi-threaded subroutines) from running multiple times concurrently.
BUG=558402
Committed: https://crrev.com/40206043977c5e7de9e769fdaa88e04ec21b17a7
Cr-Commit-Position: refs/heads/master@{#364771}
Patch Set 1 #Patch Set 2 : Reupload. #Patch Set 3 : Rebase. #Patch Set 4 : Tweak for build error. #
Total comments: 20
Patch Set 5 : Fix flakiness and address johnme's comments. #Patch Set 6 : Cleanups. #
Total comments: 8
Patch Set 7 : Address peter's comments. #
Total comments: 16
Patch Set 8 : Address peter's comments. #
Total comments: 24
Patch Set 9 : Address bauerb's comments. #
Total comments: 2
Patch Set 10 : Address bauerb's comments. #Messages
Total messages: 32 (9 generated)
mvanouwerkerk@chromium.org changed reviewers: + johnme@chromium.org
John, could you take a look please?
On 2015/11/30 17:39:09, Michael van Ouwerkerk wrote: > John, could you take a look please? Hi John, this might be the right way to go after all. Could you take a look please?
On 2015/12/04 17:10:06, Michael van Ouwerkerk wrote: > On 2015/11/30 17:39:09, Michael van Ouwerkerk wrote: > > John, could you take a look please? > > Hi John, this might be the right way to go after all. Could you take a look > please? ~ pinging John, pinging John, please click the Send button ~
https://codereview.chromium.org/1485743002/diff/60001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/push_messaging/PushMessagingTest.java (right): https://codereview.chromium.org/1485743002/diff/60001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/push_messaging/PushMessagingTest.java:41: private static final int TITLE_UPDATE_TIMEOUT_SECONDS = (int) scaleTimeout(2); You might want this in milliseconds? Otherwise if /data/local/tmp/chrome_timeout_scale is 1.49, scaleTimeout will still return 2 (since it returns the floor). Or change the scaleTimeout implementation to ceil instead of floor... Also, 2 seconds is low enough that flakiness seems more likely; perhaps leave it as 5? https://codereview.chromium.org/1485743002/diff/60001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/push_messaging/PushMessagingTest.java:81: setNotificationContentSettingForCurrentOrigin(ContentSetting.ALLOW); It's tempting to factor out a helper method: FakeGoogleCloudMessagingSubscriber trySubscribe() { FakeGoogleCloudMessagingSubscriber subscriber = new FakeGoogleCloudMessagingSubscriber(); GCMDriver.overrideSubscriberForTesting(subscriber); setNotificationContentSettingForCurrentOrigin(ContentSetting.ALLOW); runScriptAndWaitForTitle("subscribePush()", "subscribe ok"); assertEquals("1234567890", subscriber.getLastSubscribeSource()); return subscriber; } (maybe even assert that subtype starts with "wp:" + origin + "#", but ignore the GUID at the end if so) https://codereview.chromium.org/1485743002/diff/60001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/push_messaging/PushMessagingTest.java:95: sendPushAndWaitForTitle(appId, senderId, tab, "push 1 ok"); It's sad that we can't check that the default notification is not yet shown :| That would also avoid the race condition where the second push gets sent whilst still processing the first push. Please add a comment noting these issues. It would also be nice to reduce the chance of race conditions. Even something hacky might be an improvement, e.g.: for (int i = 0; i < 10; i++) runScriptAndWaitForTitle("document.title = " + i, tab, "" + i); https://codereview.chromium.org/1485743002/diff/60001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/push_messaging/PushMessagingTest.java:102: MoreAsserts.assertContainsRegex("user_visible_auto_notification", notificationEntry.tag); assertEquals should be fine here https://codereview.chromium.org/1485743002/diff/60001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/push_messaging/PushMessagingTest.java:126: private void runScriptAndWaitForTitle(String script, Tab tab, String expectedTitle) Probably clearer to move the optional argument to the end. Ditto for sendPushAndWaitForTitle https://codereview.chromium.org/1485743002/diff/60001/chrome/test/data/push_m... File chrome/test/data/push_messaging/push_messaging_test_android.html (right): https://codereview.chromium.org/1485743002/diff/60001/chrome/test/data/push_m... chrome/test/data/push_messaging/push_messaging_test_android.html:16: document.title = message; Please add some error handling for the case where the title wouldn't change, e.g.: if (message === document.title) document.title = new Error("Message must be different").stack; else document.title = message; (you may want to make a helper function for reporting errors, though the downside of that is that the helper function would make the stacktraces more confusing) https://codereview.chromium.org/1485743002/diff/60001/chrome/test/data/push_m... chrome/test/data/push_messaging/push_messaging_test_android.html:27: }) Nit: this indentation feels a little odd. How about: GetActivatedServiceWorker( 'push_messaging_test_worker_android.js', location.pathname ).then(registration => registration.pushManager.subscribe({ userVisibleOnly: true }) ).then(subscription => { setupMessageHandler(); sendToTest('subscribe ok'); }).catch(...); https://codereview.chromium.org/1485743002/diff/60001/chrome/test/data/push_m... chrome/test/data/push_messaging/push_messaging_test_android.html:33: function setNotifyOnPush(notify) { Can you add a "Called by test" comment, as otherwise this looks unused? https://codereview.chromium.org/1485743002/diff/60001/chrome/test/data/push_m... File chrome/test/data/push_messaging/push_messaging_test_worker_android.js (right): https://codereview.chromium.org/1485743002/diff/60001/chrome/test/data/push_m... chrome/test/data/push_messaging/push_messaging_test_worker_android.js:42: sendToTest('push ' + pushCounter + ' ok'); Nit: when notifyOnPush is true, consider waiting for showNotification to resolve before calling sendToTest? https://codereview.chromium.org/1485743002/diff/60001/chrome/test/data/push_m... chrome/test/data/push_messaging/push_messaging_test_worker_android.js:45: function sendToTest(message) { Nit: maybe put this above onmessage?
Thanks John! Please take another look. I've added a callback from the PushMessagingService to fix racing two push messages, and tweaked the inflight message tracking in PushMessagingServiceImpl to decrement _after_ enforcing useVisibleOnly, so that that part never runs concurrently. https://codereview.chromium.org/1485743002/diff/60001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/push_messaging/PushMessagingTest.java (right): https://codereview.chromium.org/1485743002/diff/60001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/push_messaging/PushMessagingTest.java:41: private static final int TITLE_UPDATE_TIMEOUT_SECONDS = (int) scaleTimeout(2); On 2015/12/08 17:12:31, johnme wrote: > You might want this in milliseconds? Otherwise if > /data/local/tmp/chrome_timeout_scale is 1.49, scaleTimeout will still return 2 > (since it returns the floor). Or change the scaleTimeout implementation to ceil > instead of floor... > > Also, 2 seconds is low enough that flakiness seems more likely; perhaps leave it > as 5? Sure I can keep it at 5. TabTitleObserver#waitForTitle requires an int to specify the number of seconds though. I think that the (unlikely?) rounding problem is mitigated by the larger timeout. https://codereview.chromium.org/1485743002/diff/60001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/push_messaging/PushMessagingTest.java:81: setNotificationContentSettingForCurrentOrigin(ContentSetting.ALLOW); On 2015/12/08 17:12:31, johnme wrote: > It's tempting to factor out a helper method: > > FakeGoogleCloudMessagingSubscriber trySubscribe() { > FakeGoogleCloudMessagingSubscriber subscriber = new > FakeGoogleCloudMessagingSubscriber(); > GCMDriver.overrideSubscriberForTesting(subscriber); > setNotificationContentSettingForCurrentOrigin(ContentSetting.ALLOW); > runScriptAndWaitForTitle("subscribePush()", "subscribe ok"); > assertEquals("1234567890", subscriber.getLastSubscribeSource()); > return subscriber; > } > > (maybe even assert that subtype starts with "wp:" + origin + "#", but ignore the > GUID at the end if so) Note that we would also have to load the test page before subscribing. If we add more tests here, this might be a nice bit of deduplication. As it is, I think we should try to avoid having quite so many browser and instrumentation tests, and focus on unit tests where feasible. https://codereview.chromium.org/1485743002/diff/60001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/push_messaging/PushMessagingTest.java:95: sendPushAndWaitForTitle(appId, senderId, tab, "push 1 ok"); On 2015/12/08 17:12:30, johnme wrote: > It's sad that we can't check that the default notification is not yet shown :| > That would also avoid the race condition where the second push gets sent whilst > still processing the first push. Please add a comment noting these issues. > > It would also be nice to reduce the chance of race conditions. Even something > hacky might be an improvement, e.g.: > for (int i = 0; i < 10; i++) > runScriptAndWaitForTitle("document.title = " + i, tab, "" + i); I've added a callback mechanism that waits until the push was fully handled. That should make things quite complete now. https://codereview.chromium.org/1485743002/diff/60001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/push_messaging/PushMessagingTest.java:102: MoreAsserts.assertContainsRegex("user_visible_auto_notification", notificationEntry.tag); On 2015/12/08 17:12:31, johnme wrote: > assertEquals should be fine here Oh... no there is a lot of additional stuff in that tag on the Java level :-) See NotificationUIManager#makePlatformTag https://codereview.chromium.org/1485743002/diff/60001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/push_messaging/PushMessagingTest.java:126: private void runScriptAndWaitForTitle(String script, Tab tab, String expectedTitle) On 2015/12/08 17:12:31, johnme wrote: > Probably clearer to move the optional argument to the end. Ditto for > sendPushAndWaitForTitle Done. https://codereview.chromium.org/1485743002/diff/60001/chrome/test/data/push_m... File chrome/test/data/push_messaging/push_messaging_test_android.html (right): https://codereview.chromium.org/1485743002/diff/60001/chrome/test/data/push_m... chrome/test/data/push_messaging/push_messaging_test_android.html:16: document.title = message; On 2015/12/08 17:12:31, johnme wrote: > Please add some error handling for the case where the title wouldn't change, > e.g.: > > if (message === document.title) > document.title = new Error("Message must be different").stack; > else > document.title = message; > > (you may want to make a helper function for reporting errors, though the > downside of that is that the helper function would make the stacktraces more > confusing) Done. https://codereview.chromium.org/1485743002/diff/60001/chrome/test/data/push_m... chrome/test/data/push_messaging/push_messaging_test_android.html:27: }) On 2015/12/08 17:12:31, johnme wrote: > Nit: this indentation feels a little odd. How about: > > GetActivatedServiceWorker( > 'push_messaging_test_worker_android.js', location.pathname > ).then(registration => > registration.pushManager.subscribe({ userVisibleOnly: true }) > ).then(subscription => { > setupMessageHandler(); > sendToTest('subscribe ok'); > }).catch(...); I've done something like that. https://codereview.chromium.org/1485743002/diff/60001/chrome/test/data/push_m... chrome/test/data/push_messaging/push_messaging_test_android.html:33: function setNotifyOnPush(notify) { On 2015/12/08 17:12:31, johnme wrote: > Can you add a "Called by test" comment, as otherwise this looks unused? Done. https://codereview.chromium.org/1485743002/diff/60001/chrome/test/data/push_m... File chrome/test/data/push_messaging/push_messaging_test_worker_android.js (right): https://codereview.chromium.org/1485743002/diff/60001/chrome/test/data/push_m... chrome/test/data/push_messaging/push_messaging_test_worker_android.js:42: sendToTest('push ' + pushCounter + ' ok'); On 2015/12/08 17:12:31, johnme wrote: > Nit: when notifyOnPush is true, consider waiting for showNotification to resolve > before calling sendToTest? Obsolete as this no longer relies on setting the title. https://codereview.chromium.org/1485743002/diff/60001/chrome/test/data/push_m... chrome/test/data/push_messaging/push_messaging_test_worker_android.js:45: function sendToTest(message) { On 2015/12/08 17:12:31, johnme wrote: > Nit: maybe put this above onmessage? Done.
mvanouwerkerk@chromium.org changed reviewers: + peter@chromium.org
Peter, could you also take a look please?
Description was changed from ========== Push API: test that default notification is shown when needed. BUG=558402 ========== to ========== Push API: test that the default notification is shown when needed. * Adds an instrumentation test. * Plumbs onMessageHandled through to Java. * Decrements inflight messages after EnforceUserVisibleOnlyRequirements to prevent it (and its multi-threaded subroutines) from running multiple times concurrently. BUG=558402 ==========
Description was changed from ========== Push API: test that the default notification is shown when needed. * Adds an instrumentation test. * Plumbs onMessageHandled through to Java. * Decrements inflight messages after EnforceUserVisibleOnlyRequirements to prevent it (and its multi-threaded subroutines) from running multiple times concurrently. BUG=558402 ========== to ========== Push API: test that the default notification is shown on Android when needed. * Adds an instrumentation test. * Plumbs onMessageHandled through to Java. * Decrements inflight messages after EnforceUserVisibleOnlyRequirements to prevent it (and its multi-threaded subroutines) from running multiple times concurrently. BUG=558402 ==========
I think the addition of this test is very valuable, thanks Michael! :-) https://codereview.chromium.org/1485743002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/push_messaging/PushMessagingService.java (right): https://codereview.chromium.org/1485743002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/push_messaging/PushMessagingService.java:15: * This class is the Java counterpart to the C++ PushMessagingServiceImpl class. I don't think we should have this class. It only exists for tests to have the ability to create a PushMessagingService::Delegate and listen to the generic onMessageHandled() callback, but it pretends to be a production class --and exists in production too. Instead, have you considered to make a testing-only delegate instead? In a nutshell: - Define a PushMessagingServiceImpl::Delegate class similar to the one in this file. No implementation by default; owned by the impl. - Create a PushMessagingTestUtil Java class w/ native counterpart (e.g. ValidationTestUtil.java / validation_test_util.cc) - PushMessagingTestUtil::CreateMessageDelegate(delegate) -> sets the native delegate, stores the Java instance in the delegate I think this would work - no impact on production code (aside of the null-check), no ifdefs, clear ownership and it's clear that it's intended for testing. The browser tests would also benefit from this system. https://codereview.chromium.org/1485743002/diff/100001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/push_messaging/PushMessagingTest.java (right): https://codereview.chromium.org/1485743002/diff/100001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/push_messaging/PushMessagingTest.java:136: assertEquals("This site has been updated in the background.", Let's only have the assert on line 138? While I think it's mostly a thing of the past, relying on the exact text might cause localization issues while running tests. This check is redundant with 138 anyway, assuming we don't do something silly like using that tag in the test :) https://codereview.chromium.org/1485743002/diff/100001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/push_messaging/PushMessagingTest.java:144: waitForNotificationManagerMutation(); This subtly relies on the fact that we close the default notification before showing a new one. Is it possible that there might be a race here? It would be hard to debug (observed failure would be a timeout), so consider documenting it in the comment on lines 140-141. https://codereview.chromium.org/1485743002/diff/100001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/push_messaging/PushMessagingTest.java:160: * {@code expectedTitle}. Out of interest - you're always very diligent in using Java annotations, also in block comments like this, but do we surface this somewhere? (Are there docs I don't know about?)
Thanks Peter! Please take another look. https://codereview.chromium.org/1485743002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/push_messaging/PushMessagingService.java (right): https://codereview.chromium.org/1485743002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/push_messaging/PushMessagingService.java:15: * This class is the Java counterpart to the C++ PushMessagingServiceImpl class. On 2015/12/10 01:32:56, Peter Beverloo wrote: > I don't think we should have this class. It only exists for tests to have the > ability to create a PushMessagingService::Delegate and listen to the generic > onMessageHandled() callback, but it pretends to be a production class --and > exists in production too. > > Instead, have you considered to make a testing-only delegate instead? In a > nutshell: > > - Define a PushMessagingServiceImpl::Delegate class similar to the one in this > file. No implementation by default; owned by the impl. > - Create a PushMessagingTestUtil Java class w/ native counterpart (e.g. > ValidationTestUtil.java / validation_test_util.cc) > - PushMessagingTestUtil::CreateMessageDelegate(delegate) -> sets the native > delegate, stores the Java instance in the delegate > > I think this would work - no impact on production code (aside of the > null-check), no ifdefs, clear ownership and it's clear that it's intended for > testing. The browser tests would also benefit from this system. As discussed offline, something needs to be in the production code to make this work in instrumentation tests. Also as discussed, I've minimized it. https://codereview.chromium.org/1485743002/diff/100001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/push_messaging/PushMessagingTest.java (right): https://codereview.chromium.org/1485743002/diff/100001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/push_messaging/PushMessagingTest.java:136: assertEquals("This site has been updated in the background.", On 2015/12/10 01:32:56, Peter Beverloo wrote: > Let's only have the assert on line 138? While I think it's mostly a thing of the > past, relying on the exact text might cause localization issues while running > tests. This check is redundant with 138 anyway, assuming we don't do something > silly like using that tag in the test :) Done. https://codereview.chromium.org/1485743002/diff/100001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/push_messaging/PushMessagingTest.java:144: waitForNotificationManagerMutation(); On 2015/12/10 01:32:56, Peter Beverloo wrote: > This subtly relies on the fact that we close the default notification before > showing a new one. Is it possible that there might be a race here? It would be > hard to debug (observed failure would be a timeout), so consider documenting it > in the comment on lines 140-141. No, this subtly does not have that problem, afaict. The test must wait for two mutations, and after the second there must be one notification. Whether the default notification is closed before or after the real one is shown does not matter for such logic. If I had reversed these lines, there would have been a potential race where after one mutation there are either one or two notifications. https://codereview.chromium.org/1485743002/diff/100001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/push_messaging/PushMessagingTest.java:160: * {@code expectedTitle}. On 2015/12/10 01:32:56, Peter Beverloo wrote: > Out of interest - you're always very diligent in using Java annotations, also in > block comments like this, but do we surface this somewhere? (Are there docs I > don't know about?) I'm actually not strict about this really, considering the available options in Javadoc. I do e.g. tend to prefer forms like {@code myParameter} over |myParameter| which is a common form in our C++ code. The latter form seems odd to use in Java as there are clear guidelines for how to do this, with @code being one of the options. The Chromium style guide indirectly refers to [1] and internally we have [2]. Javadoc viewers exist, Android extracts it for their published documentation, and we have internal specialized tools, but I don't know that the Chromium project is organized enough to use them. Nothing like the neat Markdown viewer [3]. [1] http://www.oracle.com/technetwork/java/javase/documentation/index-137868.html [2] go/java-practices/javadoc [3] https://chromium.googlesource.com/chromium/src/+/master/docs/android_logging.md
lgtm https://codereview.chromium.org/1485743002/diff/120001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/push_messaging/PushMessagingTest.java (right): https://codereview.chromium.org/1485743002/diff/120001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/push_messaging/PushMessagingTest.java:55: ThreadUtils.runOnUiThreadBlocking(new Runnable() { Why is it significant to run this on the UI thread? We know when setUp() and tearDown() execute relative to the test. https://codereview.chromium.org/1485743002/diff/120001/chrome/browser/android... File chrome/browser/android/chrome_jni_registrar.cc (right): https://codereview.chromium.org/1485743002/diff/120001/chrome/browser/android... chrome/browser/android/chrome_jni_registrar.cc:317: PushMessagingServiceObserverAndroid::RegisterJni}, nit: RegisterJNI or Register? (for consistency) https://codereview.chromium.org/1485743002/diff/120001/chrome/browser/push_me... File chrome/browser/push_messaging/push_messaging_service_impl.cc (right): https://codereview.chromium.org/1485743002/diff/120001/chrome/browser/push_me... chrome/browser/push_messaging/push_messaging_service_impl.cc:248: DCHECK(in_flight_message_deliveries_.count(app_id) >= 1); DCHECK_GE(in_flight_message_deliveries_.count(app_id), 1); (gives better error messages) https://codereview.chromium.org/1485743002/diff/120001/chrome/browser/push_me... File chrome/browser/push_messaging/push_messaging_service_impl.h (right): https://codereview.chromium.org/1485743002/diff/120001/chrome/browser/push_me... chrome/browser/push_messaging/push_messaging_service_impl.h:136: void OnMessageHandled(const std::string& app_id, nit: DidHandleMessage for consistency with the other methods in this class? https://codereview.chromium.org/1485743002/diff/120001/chrome/browser/push_me... File chrome/browser/push_messaging/push_messaging_service_observer.cc (right): https://codereview.chromium.org/1485743002/diff/120001/chrome/browser/push_me... chrome/browser/push_messaging/push_messaging_service_observer.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. With an #ifdef you'd be able to avoid having this entire file. (With the two if-defs in this file :).) Why did you choose for this solution? Otherwise, please initialize push_messaging_service_observer_ in the initialization list of the PushMessagingServiceImpl rather than in userland constructor code. https://codereview.chromium.org/1485743002/diff/120001/chrome/browser/push_me... File chrome/browser/push_messaging/push_messaging_service_observer_android.h (right): https://codereview.chromium.org/1485743002/diff/120001/chrome/browser/push_me... chrome/browser/push_messaging/push_messaging_service_observer_android.h:14: class PushMessagingServiceObserverAndroid // Observer for the PushMessagingService to be used on Android for forwarding // message delivered events to listeners in Java. https://codereview.chromium.org/1485743002/diff/120001/chrome/browser/push_me... chrome/browser/push_messaging/push_messaging_service_observer_android.h:18: void OnMessageHandled() override; nit: // PushMessagingServiceObserver implementation.
Thanks Peter. https://codereview.chromium.org/1485743002/diff/120001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/push_messaging/PushMessagingTest.java (right): https://codereview.chromium.org/1485743002/diff/120001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/push_messaging/PushMessagingTest.java:55: ThreadUtils.runOnUiThreadBlocking(new Runnable() { On 2015/12/10 18:47:51, Peter Beverloo wrote: > Why is it significant to run this on the UI thread? We know when setUp() and > tearDown() execute relative to the test. It modifies state in UI thread-only code. While we can reason about why and when it is safe to violate that rule and call it from the test thread, this seems like an inadvisable habit to get into. It would be nice to have lambdas, the Runnable verbosity is distracting: ThreadUtils.runOnUiThreadBlocking((Void v) -> PushMessagingServiceObserver.setListenerForTesting(this)); https://codereview.chromium.org/1485743002/diff/120001/chrome/browser/android... File chrome/browser/android/chrome_jni_registrar.cc (right): https://codereview.chromium.org/1485743002/diff/120001/chrome/browser/android... chrome/browser/android/chrome_jni_registrar.cc:317: PushMessagingServiceObserverAndroid::RegisterJni}, On 2015/12/10 18:47:51, Peter Beverloo wrote: > nit: RegisterJNI or Register? (for consistency) This is the more consistent form with regards to casing. "Jni" is already used 12 times in this file, "JNI" 5 times. https://codereview.chromium.org/1485743002/diff/120001/chrome/browser/push_me... File chrome/browser/push_messaging/push_messaging_service_impl.cc (right): https://codereview.chromium.org/1485743002/diff/120001/chrome/browser/push_me... chrome/browser/push_messaging/push_messaging_service_impl.cc:248: DCHECK(in_flight_message_deliveries_.count(app_id) >= 1); On 2015/12/10 18:47:51, Peter Beverloo wrote: > DCHECK_GE(in_flight_message_deliveries_.count(app_id), 1); > > (gives better error messages) Done. https://codereview.chromium.org/1485743002/diff/120001/chrome/browser/push_me... File chrome/browser/push_messaging/push_messaging_service_impl.h (right): https://codereview.chromium.org/1485743002/diff/120001/chrome/browser/push_me... chrome/browser/push_messaging/push_messaging_service_impl.h:136: void OnMessageHandled(const std::string& app_id, On 2015/12/10 18:47:51, Peter Beverloo wrote: > nit: DidHandleMessage for consistency with the other methods in this class? Done. https://codereview.chromium.org/1485743002/diff/120001/chrome/browser/push_me... File chrome/browser/push_messaging/push_messaging_service_observer.cc (right): https://codereview.chromium.org/1485743002/diff/120001/chrome/browser/push_me... chrome/browser/push_messaging/push_messaging_service_observer.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2015/12/10 18:47:51, Peter Beverloo wrote: > With an #ifdef you'd be able to avoid having this entire file. (With the two > if-defs in this file :).) Why did you choose for this solution? > > Otherwise, please initialize push_messaging_service_observer_ in the > initialization list of the PushMessagingServiceImpl rather than in userland > constructor code. I'm not sure how you would avoid defining this file without adding ifdefs elsewhere, as the Create method needs a definition. Initialization list done though. https://codereview.chromium.org/1485743002/diff/120001/chrome/browser/push_me... File chrome/browser/push_messaging/push_messaging_service_observer_android.h (right): https://codereview.chromium.org/1485743002/diff/120001/chrome/browser/push_me... chrome/browser/push_messaging/push_messaging_service_observer_android.h:14: class PushMessagingServiceObserverAndroid On 2015/12/10 18:47:52, Peter Beverloo wrote: > // Observer for the PushMessagingService to be used on Android for forwarding > // message delivered events to listeners in Java. Done. https://codereview.chromium.org/1485743002/diff/120001/chrome/browser/push_me... chrome/browser/push_messaging/push_messaging_service_observer_android.h:18: void OnMessageHandled() override; On 2015/12/10 18:47:52, Peter Beverloo wrote: > nit: // PushMessagingServiceObserver implementation. Done.
mvanouwerkerk@chromium.org changed reviewers: + bauerb@chromium.org
Bernhard, could you please review chrome/browser/android/chrome_jni_registrar.cc as owner?
https://codereview.chromium.org/1485743002/diff/120001/chrome/browser/push_me... File chrome/browser/push_messaging/push_messaging_service_observer.cc (right): https://codereview.chromium.org/1485743002/diff/120001/chrome/browser/push_me... chrome/browser/push_messaging/push_messaging_service_observer.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2015/12/11 11:23:54, Michael van Ouwerkerk wrote: > On 2015/12/10 18:47:51, Peter Beverloo wrote: > > With an #ifdef you'd be able to avoid having this entire file. (With the two > > if-defs in this file :).) Why did you choose for this solution? > > > > Otherwise, please initialize push_messaging_service_observer_ in the > > initialization list of the PushMessagingServiceImpl rather than in userland > > constructor code. > > I'm not sure how you would avoid defining this file without adding ifdefs > elsewhere, as the Create method needs a definition. Initialization list done > though. My question is why we need the Create() method at all. Why not have PushMessagingServiceObserver be a pure virtual interface and instantiate PushMessagingServiceObserverAndroid directly?
https://codereview.chromium.org/1485743002/diff/120001/chrome/browser/push_me... File chrome/browser/push_messaging/push_messaging_service_observer.cc (right): https://codereview.chromium.org/1485743002/diff/120001/chrome/browser/push_me... chrome/browser/push_messaging/push_messaging_service_observer.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2015/12/11 11:30:03, Peter Beverloo wrote: > On 2015/12/11 11:23:54, Michael van Ouwerkerk wrote: > > On 2015/12/10 18:47:51, Peter Beverloo wrote: > > > With an #ifdef you'd be able to avoid having this entire file. (With the two > > > if-defs in this file :).) Why did you choose for this solution? > > > > > > Otherwise, please initialize push_messaging_service_observer_ in the > > > initialization list of the PushMessagingServiceImpl rather than in userland > > > constructor code. > > > > I'm not sure how you would avoid defining this file without adding ifdefs > > elsewhere, as the Create method needs a definition. Initialization list done > > though. > > My question is why we need the Create() method at all. Why not have > PushMessagingServiceObserver be a pure virtual interface and instantiate > PushMessagingServiceObserverAndroid directly? That would move the ifdef to the callsite. One of the reasons for having this separate class at all is to reduce ifdefs in the PushMessagingServiceImpl.
https://codereview.chromium.org/1485743002/diff/140001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/push_messaging/PushMessagingTest.java (right): https://codereview.chromium.org/1485743002/diff/140001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/push_messaging/PushMessagingTest.java:180: private void waitForTitle(Tab tab, String expectedTitle) throws Exception { Do we really have to throw a general exception here? `throws TimeoutException, InterruptedException` is more characters, but a narrower set of exceptions. https://codereview.chromium.org/1485743002/diff/140001/chrome/browser/push_me... File chrome/browser/push_messaging/push_messaging_service_impl.cc (right): https://codereview.chromium.org/1485743002/diff/140001/chrome/browser/push_me... chrome/browser/push_messaging/push_messaging_service_impl.cc:251: weak_factory_.GetWeakPtr(), app_id, message_handled_closure); You may want to use ScopedClosureRunner (from base/callback_helpers.h) here. https://codereview.chromium.org/1485743002/diff/140001/chrome/browser/push_me... chrome/browser/push_messaging/push_messaging_service_impl.cc:295: DCHECK(in_flight_message_deliveries_.find(app_id) != Nit: You could extract the iterator to a local variable so you can reuse it below. https://codereview.chromium.org/1485743002/diff/140001/chrome/browser/push_me... File chrome/browser/push_messaging/push_messaging_service_observer.h (right): https://codereview.chromium.org/1485743002/diff/140001/chrome/browser/push_me... chrome/browser/push_messaging/push_messaging_service_observer.h:10: // Note: this class is to be used in tests only. Name it ...ForTesting or ...TestingObserver then? https://codereview.chromium.org/1485743002/diff/140001/chrome/browser/push_me... chrome/browser/push_messaging/push_messaging_service_observer.h:13: // Creates a new PushMessagingServiceObserver. Caller takes ownership. Return a scoped_ptr then? https://codereview.chromium.org/1485743002/diff/140001/chrome/browser/push_me... File chrome/browser/push_messaging/push_messaging_service_observer_android.h (right): https://codereview.chromium.org/1485743002/diff/140001/chrome/browser/push_me... chrome/browser/push_messaging/push_messaging_service_observer_android.h:25: friend class PushMessagingServiceObserver; You could add a protected empty constructor to the base class, then you don't need to friend it here. https://codereview.chromium.org/1485743002/diff/140001/chrome/test/data/push_... File chrome/test/data/push_messaging/push_messaging_test_android.html (right): https://codereview.chromium.org/1485743002/diff/140001/chrome/test/data/push_... chrome/test/data/push_messaging/push_messaging_test_android.html:51: .then(registration => { Nit: I think this should be indented two more spaces? https://codereview.chromium.org/1485743002/diff/140001/chrome/test/data/push_... File chrome/test/data/push_messaging/push_messaging_test_worker_android.js (right): https://codereview.chromium.org/1485743002/diff/140001/chrome/test/data/push_... chrome/test/data/push_messaging/push_messaging_test_worker_android.js:23: onmessage = event => { Nit: Use window.onmessage to make the scope of the variable clear?
lgtm, thanks! And +1 to bernhard's nits :) https://codereview.chromium.org/1485743002/diff/140001/chrome/browser/push_me... File chrome/browser/push_messaging/push_messaging_service_impl.cc (right): https://codereview.chromium.org/1485743002/diff/140001/chrome/browser/push_me... chrome/browser/push_messaging/push_messaging_service_impl.cc:306: push_messaging_service_observer_->OnMessageHandled(); Consider setting the message_handled_closure in OnMessage instead of adding this call here, to avoid some duplication.
lgtm, thanks! And +1 to bernhard's nits :)
Thanks Bernhard. Please take another look. https://codereview.chromium.org/1485743002/diff/140001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/push_messaging/PushMessagingTest.java (right): https://codereview.chromium.org/1485743002/diff/140001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/push_messaging/PushMessagingTest.java:180: private void waitForTitle(Tab tab, String expectedTitle) throws Exception { On 2015/12/11 12:28:41, Bernhard Bauer wrote: > Do we really have to throw a general exception here? `throws TimeoutException, > InterruptedException` is more characters, but a narrower set of exceptions. The TimeoutException is caught, so this only throws InterruptedExeption. Done. https://codereview.chromium.org/1485743002/diff/140001/chrome/browser/push_me... File chrome/browser/push_messaging/push_messaging_service_impl.cc (right): https://codereview.chromium.org/1485743002/diff/140001/chrome/browser/push_me... chrome/browser/push_messaging/push_messaging_service_impl.cc:251: weak_factory_.GetWeakPtr(), app_id, message_handled_closure); On 2015/12/11 12:28:41, Bernhard Bauer wrote: > You may want to use ScopedClosureRunner (from base/callback_helpers.h) here. Wow, that's cool. Maybe a little too cool for me. That opens the door much wider for code that does all kinds of interesting stuff when a function returns, in a non-obvious way. I'd suggest that when someone feels the need to use a ScopedClosureRunner, there might be a bigger need for cleaning up the function that would potentially use it. Reading through some examples in our current codebase that use ScopedClosureRunner, I found stuff like a param of type "scoped_ptr<base::ScopedClosureRunner>*" which is not encouraging either. https://codereview.chromium.org/1485743002/diff/140001/chrome/browser/push_me... chrome/browser/push_messaging/push_messaging_service_impl.cc:295: DCHECK(in_flight_message_deliveries_.find(app_id) != On 2015/12/11 12:28:41, Bernhard Bauer wrote: > Nit: You could extract the iterator to a local variable so you can reuse it > below. Done. https://codereview.chromium.org/1485743002/diff/140001/chrome/browser/push_me... chrome/browser/push_messaging/push_messaging_service_impl.cc:306: push_messaging_service_observer_->OnMessageHandled(); On 2015/12/11 14:42:56, johnme wrote: > Consider setting the message_handled_closure in OnMessage instead of adding this > call here, to avoid some duplication. Acknowledged. https://codereview.chromium.org/1485743002/diff/140001/chrome/browser/push_me... File chrome/browser/push_messaging/push_messaging_service_observer.h (right): https://codereview.chromium.org/1485743002/diff/140001/chrome/browser/push_me... chrome/browser/push_messaging/push_messaging_service_observer.h:10: // Note: this class is to be used in tests only. On 2015/12/11 12:28:42, Bernhard Bauer wrote: > Name it ...ForTesting or ...TestingObserver then? I've deleted this bit and expanded the comment in PushMessagingServiceObserver.java instead, where there is more context about how to clean this up in future. https://codereview.chromium.org/1485743002/diff/140001/chrome/browser/push_me... chrome/browser/push_messaging/push_messaging_service_observer.h:13: // Creates a new PushMessagingServiceObserver. Caller takes ownership. On 2015/12/11 12:28:42, Bernhard Bauer wrote: > Return a scoped_ptr then? Done. https://codereview.chromium.org/1485743002/diff/140001/chrome/browser/push_me... File chrome/browser/push_messaging/push_messaging_service_observer_android.h (right): https://codereview.chromium.org/1485743002/diff/140001/chrome/browser/push_me... chrome/browser/push_messaging/push_messaging_service_observer_android.h:25: friend class PushMessagingServiceObserver; On 2015/12/11 12:28:42, Bernhard Bauer wrote: > You could add a protected empty constructor to the base class, then you don't > need to friend it here. I'm not sure what you meant by this actually. How does that allow PushMessagingServiceObserver::Create to call PushMessagingServiceObserverAndroid? https://codereview.chromium.org/1485743002/diff/140001/chrome/test/data/push_... File chrome/test/data/push_messaging/push_messaging_test_android.html (right): https://codereview.chromium.org/1485743002/diff/140001/chrome/test/data/push_... chrome/test/data/push_messaging/push_messaging_test_android.html:51: .then(registration => { On 2015/12/11 12:28:42, Bernhard Bauer wrote: > Nit: I think this should be indented two more spaces? Done. https://codereview.chromium.org/1485743002/diff/140001/chrome/test/data/push_... File chrome/test/data/push_messaging/push_messaging_test_worker_android.js (right): https://codereview.chromium.org/1485743002/diff/140001/chrome/test/data/push_... chrome/test/data/push_messaging/push_messaging_test_worker_android.js:23: onmessage = event => { On 2015/12/11 12:28:42, Bernhard Bauer wrote: > Nit: Use window.onmessage to make the scope of the variable clear? This is service worker code, there is no window object. I can use "self" though if that makes it easier to read. Done.
https://codereview.chromium.org/1485743002/diff/140001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/push_messaging/PushMessagingTest.java (right): https://codereview.chromium.org/1485743002/diff/140001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/push_messaging/PushMessagingTest.java:180: private void waitForTitle(Tab tab, String expectedTitle) throws Exception { On 2015/12/11 16:05:55, Michael van Ouwerkerk wrote: > On 2015/12/11 12:28:41, Bernhard Bauer wrote: > > Do we really have to throw a general exception here? `throws TimeoutException, > > InterruptedException` is more characters, but a narrower set of exceptions. > > The TimeoutException is caught, so this only throws InterruptedExeption. Done. Now you can probably also update the other methods in here that throw Exception. https://codereview.chromium.org/1485743002/diff/140001/chrome/browser/push_me... File chrome/browser/push_messaging/push_messaging_service_impl.cc (right): https://codereview.chromium.org/1485743002/diff/140001/chrome/browser/push_me... chrome/browser/push_messaging/push_messaging_service_impl.cc:251: weak_factory_.GetWeakPtr(), app_id, message_handled_closure); On 2015/12/11 16:05:55, Michael van Ouwerkerk wrote: > On 2015/12/11 12:28:41, Bernhard Bauer wrote: > > You may want to use ScopedClosureRunner (from base/callback_helpers.h) here. > > Wow, that's cool. Maybe a little too cool for me. That opens the door much wider > for code that does all kinds of interesting stuff when a function returns, in a > non-obvious way. I'd suggest that when someone feels the need to use a > ScopedClosureRunner, there might be a bigger need for cleaning up the function > that would potentially use it. I'm not quite sure I get that argument -- wouldn't that imply that this function requires cleanup? If so, go right ahead 😃 I also don't think there's that much non-obvious about it; this is not much different from other objects that do things in their constructor, like freeing a pointer or closing a file handle. In fact, I would argue this makes the intent more obvious, because it's clear that the callback should run in all code paths -- similar to how a scoped_ptr signifies ownership on a conceptual level. In contrast, if you have Run() calls strewn all over the function, that is much less clear, *and* there is the chance that you might miss a code path. > Reading through some examples in our current codebase that use > ScopedClosureRunner, I found stuff like a param of type > "scoped_ptr<base::ScopedClosureRunner>*" which is not encouraging either. Again, I don't get this argument -- other people are using this in more complicated ways, so we shouldn't use it here? https://codereview.chromium.org/1485743002/diff/140001/chrome/test/data/push_... File chrome/test/data/push_messaging/push_messaging_test_worker_android.js (right): https://codereview.chromium.org/1485743002/diff/140001/chrome/test/data/push_... chrome/test/data/push_messaging/push_messaging_test_worker_android.js:23: onmessage = event => { On 2015/12/11 16:05:56, Michael van Ouwerkerk wrote: > On 2015/12/11 12:28:42, Bernhard Bauer wrote: > > Nit: Use window.onmessage to make the scope of the variable clear? > > This is service worker code, there is no window object. I can use "self" though > if that makes it easier to read. Done. Oh, right. See, all the more reason to make the scope explicit 😃 https://codereview.chromium.org/1485743002/diff/160001/chrome/browser/push_me... File chrome/browser/push_messaging/push_messaging_service_observer.cc (right): https://codereview.chromium.org/1485743002/diff/160001/chrome/browser/push_me... chrome/browser/push_messaging/push_messaging_service_observer.cc:18: observer.reset(new PushMessagingServiceObserverAndroid()); You could also `return make_scoped_ptr(new ...)`, but up to you.
Thanks Bernhard. Please take another look. https://codereview.chromium.org/1485743002/diff/140001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/push_messaging/PushMessagingTest.java (right): https://codereview.chromium.org/1485743002/diff/140001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/push_messaging/PushMessagingTest.java:180: private void waitForTitle(Tab tab, String expectedTitle) throws Exception { On 2015/12/11 16:35:57, Bernhard Bauer wrote: > On 2015/12/11 16:05:55, Michael van Ouwerkerk wrote: > > On 2015/12/11 12:28:41, Bernhard Bauer wrote: > > > Do we really have to throw a general exception here? `throws > TimeoutException, > > > InterruptedException` is more characters, but a narrower set of exceptions. > > > > The TimeoutException is caught, so this only throws InterruptedExeption. Done. > > Now you can probably also update the other methods in here that throw Exception. There are lots of other methods, and this chains into other files. As discussed offline, I've added a TODO for this. That said, setUp and tearDown cannot be changed as that's what they do in ActivityInstrumentationTestCase2. https://codereview.chromium.org/1485743002/diff/140001/chrome/browser/push_me... File chrome/browser/push_messaging/push_messaging_service_impl.cc (right): https://codereview.chromium.org/1485743002/diff/140001/chrome/browser/push_me... chrome/browser/push_messaging/push_messaging_service_impl.cc:251: weak_factory_.GetWeakPtr(), app_id, message_handled_closure); On 2015/12/11 16:35:57, Bernhard Bauer wrote: > On 2015/12/11 16:05:55, Michael van Ouwerkerk wrote: > > On 2015/12/11 12:28:41, Bernhard Bauer wrote: > > > You may want to use ScopedClosureRunner (from base/callback_helpers.h) here. > > > > Wow, that's cool. Maybe a little too cool for me. That opens the door much > wider > > for code that does all kinds of interesting stuff when a function returns, in > a > > non-obvious way. I'd suggest that when someone feels the need to use a > > ScopedClosureRunner, there might be a bigger need for cleaning up the function > > that would potentially use it. > > I'm not quite sure I get that argument -- wouldn't that imply that this function > requires cleanup? If so, go right ahead 😃 > > I also don't think there's that much non-obvious about it; this is not much > different from other objects that do things in their constructor, like freeing a > pointer or closing a file handle. In fact, I would argue this makes the intent > more obvious, because it's clear that the callback should run in all code paths > -- similar to how a scoped_ptr signifies ownership on a conceptual level. In > contrast, if you have Run() calls strewn all over the function, that is much > less clear, *and* there is the chance that you might miss a code path. > > > Reading through some examples in our current codebase that use > > ScopedClosureRunner, I found stuff like a param of type > > "scoped_ptr<base::ScopedClosureRunner>*" which is not encouraging either. > > Again, I don't get this argument -- other people are using this in more > complicated ways, so we shouldn't use it here? I'm not a big fan of this one, but you seem to feel strongly about it. It's really scope creep for the CL though as it is about refactoring existing code, so as discussed I've added a TODO for this. https://codereview.chromium.org/1485743002/diff/140001/chrome/test/data/push_... File chrome/test/data/push_messaging/push_messaging_test_worker_android.js (right): https://codereview.chromium.org/1485743002/diff/140001/chrome/test/data/push_... chrome/test/data/push_messaging/push_messaging_test_worker_android.js:23: onmessage = event => { On 2015/12/11 16:35:57, Bernhard Bauer wrote: > On 2015/12/11 16:05:56, Michael van Ouwerkerk wrote: > > On 2015/12/11 12:28:42, Bernhard Bauer wrote: > > > Nit: Use window.onmessage to make the scope of the variable clear? > > > > This is service worker code, there is no window object. I can use "self" > though > > if that makes it easier to read. Done. > > Oh, right. See, all the more reason to make the scope explicit 😃 Acknowledged. https://codereview.chromium.org/1485743002/diff/160001/chrome/browser/push_me... File chrome/browser/push_messaging/push_messaging_service_observer.cc (right): https://codereview.chromium.org/1485743002/diff/160001/chrome/browser/push_me... chrome/browser/push_messaging/push_messaging_service_observer.cc:18: observer.reset(new PushMessagingServiceObserverAndroid()); On 2015/12/11 16:35:57, Bernhard Bauer wrote: > You could also `return make_scoped_ptr(new ...)`, but up to you. So a nullptr cannot be passed to make_scoped_ptr, but it can simply be returned and coerced into a scoped_ptr. Cool. Done.
The CQ bit was checked by bauerb@chromium.org
lgtm
The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org, johnme@chromium.org Link to the patchset: https://codereview.chromium.org/1485743002/#ps180001 (title: "Address bauerb's comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1485743002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1485743002/180001
Message was sent while issue was closed.
Description was changed from ========== Push API: test that the default notification is shown on Android when needed. * Adds an instrumentation test. * Plumbs onMessageHandled through to Java. * Decrements inflight messages after EnforceUserVisibleOnlyRequirements to prevent it (and its multi-threaded subroutines) from running multiple times concurrently. BUG=558402 ========== to ========== Push API: test that the default notification is shown on Android when needed. * Adds an instrumentation test. * Plumbs onMessageHandled through to Java. * Decrements inflight messages after EnforceUserVisibleOnlyRequirements to prevent it (and its multi-threaded subroutines) from running multiple times concurrently. BUG=558402 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Push API: test that the default notification is shown on Android when needed. * Adds an instrumentation test. * Plumbs onMessageHandled through to Java. * Decrements inflight messages after EnforceUserVisibleOnlyRequirements to prevent it (and its multi-threaded subroutines) from running multiple times concurrently. BUG=558402 ========== to ========== Push API: test that the default notification is shown on Android when needed. * Adds an instrumentation test. * Plumbs onMessageHandled through to Java. * Decrements inflight messages after EnforceUserVisibleOnlyRequirements to prevent it (and its multi-threaded subroutines) from running multiple times concurrently. BUG=558402 Committed: https://crrev.com/40206043977c5e7de9e769fdaa88e04ec21b17a7 Cr-Commit-Position: refs/heads/master@{#364771} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/40206043977c5e7de9e769fdaa88e04ec21b17a7 Cr-Commit-Position: refs/heads/master@{#364771} |
