|
|
Created:
4 years, 1 month ago by Miguel Garcia Modified:
4 years, 1 month ago CC:
chromium-reviews, Peter Beverloo, mlamouri+watch-notifications_chromium.org, awdf+watch_chromium.org, mac-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCreate a stub NSUserNotificationCenter and make tests use it.
Also fix a bug in the bridge found by the tests.
BUG=571056
Committed: https://crrev.com/f052abfe2134e4af31100d49d3858ed8727f8c52
Cr-Commit-Position: refs/heads/master@{#430359}
Patch Set 1 #Patch Set 2 : remove parametrized arrays #Patch Set 3 : - #
Total comments: 10
Patch Set 4 : Switch to subclassing instead of Swizzling #
Total comments: 18
Patch Set 5 : Peter's review #
Total comments: 6
Patch Set 6 : Robert's review #
Total comments: 10
Patch Set 7 : review #
Total comments: 2
Patch Set 8 : review #
Dependent Patchsets: Messages
Total messages: 49 (35 generated)
The CQ bit was checked by miguelg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Create stub NSUserNotificationCenter and make tests use it. Also fix a bug in the bridge found by the tests. BUG=571056 ========== to ========== Create a stub NSUserNotificationCenter and make tests use it. Also fix a bug in the bridge found by the tests. BUG=571056 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by miguelg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by miguelg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:1) has been deleted
The CQ bit was checked by miguelg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
miguelg@chromium.org changed reviewers: + peter@chromium.org, rsesek@chromium.org
This CL is just an idea for your consideration, although if we decide to not go ahead I will have to submit the bug fix in a separate CL. The idea here is to create a class that stubs the NSUserNotificationCenter completely and provides an implementation suitable for tests. I have tried subclassing but none of my attempts have worked since NSUserNotificationCenter is a system class. I therefore went with Swizzling which required me to add a bunch of hacks explained in the .mm file. I still think it's a good idea to do this since the test itself is now super clean. It just injects a stub notification center and uses the public api to verify what happens to the notifications, as opposed to testing whether a certain method is called as it was doing before. This also allows me to reuse the same stub implementation across different tests cases (and eventually more tests) so I ended up adding a few more (and found a bug in the process!) Because I had to swizzle several methods I shied away from ScopedObjCClassSwizzlersince that only works for one method at a time but I am open to suggestions here.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2458703003/diff/60001/chrome/browser/notifica... File chrome/browser/notifications/stub_notification_center_mac.h (right): https://codereview.chromium.org/2458703003/diff/60001/chrome/browser/notifica... chrome/browser/notifications/stub_notification_center_mac.h:9: @class NSUserNotification; No forwards needed given that you've got AppKit.h here. https://codereview.chromium.org/2458703003/diff/60001/chrome/browser/notifica... File chrome/browser/notifications/stub_notification_center_mac.mm (right): https://codereview.chromium.org/2458703003/diff/60001/chrome/browser/notifica... chrome/browser/notifications/stub_notification_center_mac.mm:4: #include "chrome/browser/notifications/stub_notification_center_mac.h" nit: blank line before https://codereview.chromium.org/2458703003/diff/60001/chrome/browser/notifica... chrome/browser/notifications/stub_notification_center_mac.mm:11: // Since it's hard/impossible to subclass NSUserNotificationCenter this class I just realized we could do something devilish to make subclassing possible. I tested this and it appears to work: http://pastebin.com/YBTDegcz https://codereview.chromium.org/2458703003/diff/60001/chrome/browser/notifica... chrome/browser/notifications/stub_notification_center_mac.mm:37: @dynamic notifications; Not needed if you're providing definitions below. https://codereview.chromium.org/2458703003/diff/60001/chrome/browser/notifica... chrome/browser/notifications/stub_notification_center_mac.mm:98: - (void)initialize { All of this swizzling should get cleaned up at the end of the test.
The CQ bit was checked by miguelg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2458703003/diff/60001/chrome/browser/notifica... File chrome/browser/notifications/stub_notification_center_mac.h (right): https://codereview.chromium.org/2458703003/diff/60001/chrome/browser/notifica... chrome/browser/notifications/stub_notification_center_mac.h:9: @class NSUserNotification; On 2016/10/31 18:57:30, Robert Sesek wrote: > No forwards needed given that you've got AppKit.h here. Done. https://codereview.chromium.org/2458703003/diff/60001/chrome/browser/notifica... File chrome/browser/notifications/stub_notification_center_mac.mm (right): https://codereview.chromium.org/2458703003/diff/60001/chrome/browser/notifica... chrome/browser/notifications/stub_notification_center_mac.mm:4: #include "chrome/browser/notifications/stub_notification_center_mac.h" On 2016/10/31 18:57:30, Robert Sesek wrote: > nit: blank line before Done. https://codereview.chromium.org/2458703003/diff/60001/chrome/browser/notifica... chrome/browser/notifications/stub_notification_center_mac.mm:11: // Since it's hard/impossible to subclass NSUserNotificationCenter this class On 2016/10/31 18:57:30, Robert Sesek wrote: > I just realized we could do something devilish to make subclassing possible. I > tested this and it appears to work: > > http://pastebin.com/YBTDegcz I spent a good five minutes trying to think of excuses to not change it :) But this is soo much better! It will even let me test click events and so now that I can inject the delegate. https://codereview.chromium.org/2458703003/diff/60001/chrome/browser/notifica... chrome/browser/notifications/stub_notification_center_mac.mm:37: @dynamic notifications; On 2016/10/31 18:57:30, Robert Sesek wrote: > Not needed if you're providing definitions below. Done. https://codereview.chromium.org/2458703003/diff/60001/chrome/browser/notifica... chrome/browser/notifications/stub_notification_center_mac.mm:98: - (void)initialize { On 2016/10/31 18:57:30, Robert Sesek wrote: > All of this swizzling should get cleaned up at the end of the test. Acknowledged.
https://codereview.chromium.org/2458703003/diff/80001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm (right): https://codereview.chromium.org/2458703003/diff/80001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm:87: return notification_center_; nit: notification_center_.get() We've been moving away from implicit conversions for other types for a while. I imagine that it'll eventually go for Mac's ScopedTypeRef too. https://codereview.chromium.org/2458703003/diff/80001/chrome/browser/notifica... File chrome/browser/notifications/stub_notification_center_mac.h (right): https://codereview.chromium.org/2458703003/diff/80001/chrome/browser/notifica... chrome/browser/notifications/stub_notification_center_mac.h:4: #ifndef CHROME_BROWSER_NOTIFICATIONS_MOCK_NOTIFICATION_CENTER_MAC_H_ nit: blank line above here... https://codereview.chromium.org/2458703003/diff/80001/chrome/browser/notifica... chrome/browser/notifications/stub_notification_center_mac.h:9: @class NSUserNotificationCenterDelegate; No need. (You import AppKit.h) https://codereview.chromium.org/2458703003/diff/80001/chrome/browser/notifica... chrome/browser/notifications/stub_notification_center_mac.h:11: @interface TestNotificationCenter : NSUserNotificationCenter You're got a bit of a naming conundrum going on here. The file is named StubNotificationCenterMac, the header guards say MockNotificationCenterMac, the defined interface says TestNotificationCenter. Please pick one. https://codereview.chromium.org/2458703003/diff/80001/chrome/browser/notifica... chrome/browser/notifications/stub_notification_center_mac.h:11: @interface TestNotificationCenter : NSUserNotificationCenter Whichever name you settle on, could you leave a brief one or two line description above the interface to explain exactly what it does, and how it's meant to be used? https://codereview.chromium.org/2458703003/diff/80001/chrome/browser/notifica... File chrome/browser/notifications/stub_notification_center_mac.mm (right): https://codereview.chromium.org/2458703003/diff/80001/chrome/browser/notifica... chrome/browser/notifications/stub_notification_center_mac.mm:16: alerts_ = [NSMutableArray arrayWithCapacity:0]; nit: [[NSMutableArray alloc] init]; https://codereview.chromium.org/2458703003/diff/80001/chrome/browser/notifica... chrome/browser/notifications/stub_notification_center_mac.mm:39: NSString* notification_id = [notification.userInfo Since this is a .mm file, I think you want camelCase? (I.e. notificationId) https://codereview.chromium.org/2458703003/diff/80001/chrome/browser/notifica... chrome/browser/notifications/stub_notification_center_mac.mm:44: DCHECK_NE(notification_id, nil); I think DCHECK(notification_id) would just work for these?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by miguelg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2458703003/diff/100001/chrome/browser/notific... File chrome/browser/notifications/stub_notification_center_mac.mm (right): https://codereview.chromium.org/2458703003/diff/100001/chrome/browser/notific... chrome/browser/notifications/stub_notification_center_mac.mm:11: NSMutableArray* alerts_; This should be in a scoped_nsobject. https://codereview.chromium.org/2458703003/diff/100001/chrome/browser/notific... chrome/browser/notifications/stub_notification_center_mac.mm:16: alerts_ = [NSMutableArray arrayWithCapacity:1]; And this should be an alloc/init. https://codereview.chromium.org/2458703003/diff/100001/chrome/browser/notific... chrome/browser/notifications/stub_notification_center_mac.mm:35: return alerts_; Because alerts_ is mutable, return [[alerts_ copy] autorelease] to prevent a caller from accidentally mutating it.
The CQ bit was checked by miguelg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2458703003/diff/80001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm (right): https://codereview.chromium.org/2458703003/diff/80001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm:87: return notification_center_; On 2016/11/01 11:25:39, Peter Beverloo wrote: > nit: notification_center_.get() > > We've been moving away from implicit conversions for other types for a while. I > imagine that it'll eventually go for Mac's ScopedTypeRef too. Done. https://codereview.chromium.org/2458703003/diff/80001/chrome/browser/notifica... File chrome/browser/notifications/stub_notification_center_mac.h (right): https://codereview.chromium.org/2458703003/diff/80001/chrome/browser/notifica... chrome/browser/notifications/stub_notification_center_mac.h:4: #ifndef CHROME_BROWSER_NOTIFICATIONS_MOCK_NOTIFICATION_CENTER_MAC_H_ On 2016/11/01 11:25:39, Peter Beverloo wrote: > nit: blank line above here... Done. https://codereview.chromium.org/2458703003/diff/80001/chrome/browser/notifica... chrome/browser/notifications/stub_notification_center_mac.h:9: @class NSUserNotificationCenterDelegate; On 2016/11/01 11:25:39, Peter Beverloo wrote: > No need. (You import AppKit.h) AppKit.h does not include the delegate https://github.com/thezerobit/gnustep-gui-sony/blob/master/Headers/AppKit/App... https://codereview.chromium.org/2458703003/diff/80001/chrome/browser/notifica... chrome/browser/notifications/stub_notification_center_mac.h:11: @interface TestNotificationCenter : NSUserNotificationCenter On 2016/11/01 11:25:39, Peter Beverloo wrote: > You're got a bit of a naming conundrum going on here. The file is named > StubNotificationCenterMac, the header guards say MockNotificationCenterMac, the > defined interface says TestNotificationCenter. > > Please pick one. Went with stub https://codereview.chromium.org/2458703003/diff/80001/chrome/browser/notifica... chrome/browser/notifications/stub_notification_center_mac.h:11: @interface TestNotificationCenter : NSUserNotificationCenter On 2016/11/01 11:25:39, Peter Beverloo wrote: > Whichever name you settle on, could you leave a brief one or two line > description above the interface to explain exactly what it does, and how it's > meant to be used? Done. https://codereview.chromium.org/2458703003/diff/80001/chrome/browser/notifica... File chrome/browser/notifications/stub_notification_center_mac.mm (right): https://codereview.chromium.org/2458703003/diff/80001/chrome/browser/notifica... chrome/browser/notifications/stub_notification_center_mac.mm:16: alerts_ = [NSMutableArray arrayWithCapacity:0]; On 2016/11/01 11:25:39, Peter Beverloo wrote: > nit: [[NSMutableArray alloc] init]; I rather use ref counting here, why would you want to deal with the lifecycle yourself? I changed it to capacity 1 since that's what most test use (it's just a hint anyway) https://codereview.chromium.org/2458703003/diff/80001/chrome/browser/notifica... chrome/browser/notifications/stub_notification_center_mac.mm:39: NSString* notification_id = [notification.userInfo On 2016/11/01 11:25:39, Peter Beverloo wrote: > Since this is a .mm file, I think you want camelCase? (I.e. notificationId) Done. https://codereview.chromium.org/2458703003/diff/80001/chrome/browser/notifica... chrome/browser/notifications/stub_notification_center_mac.mm:44: DCHECK_NE(notification_id, nil); On 2016/11/01 11:25:39, Peter Beverloo wrote: > I think DCHECK(notification_id) would just work for these? Done. https://codereview.chromium.org/2458703003/diff/100001/chrome/browser/notific... File chrome/browser/notifications/stub_notification_center_mac.mm (right): https://codereview.chromium.org/2458703003/diff/100001/chrome/browser/notific... chrome/browser/notifications/stub_notification_center_mac.mm:11: NSMutableArray* alerts_; On 2016/11/01 15:32:38, Robert Sesek wrote: > This should be in a scoped_nsobject. Done. https://codereview.chromium.org/2458703003/diff/100001/chrome/browser/notific... chrome/browser/notifications/stub_notification_center_mac.mm:16: alerts_ = [NSMutableArray arrayWithCapacity:1]; On 2016/11/01 15:32:38, Robert Sesek wrote: > And this should be an alloc/init. Done. https://codereview.chromium.org/2458703003/diff/100001/chrome/browser/notific... chrome/browser/notifications/stub_notification_center_mac.mm:35: return alerts_; On 2016/11/01 15:32:38, Robert Sesek wrote: > Because alerts_ is mutable, return [[alerts_ copy] autorelease] to prevent a > caller from accidentally mutating it. Done.
https://codereview.chromium.org/2458703003/diff/80001/chrome/browser/notifica... File chrome/browser/notifications/stub_notification_center_mac.mm (right): https://codereview.chromium.org/2458703003/diff/80001/chrome/browser/notifica... chrome/browser/notifications/stub_notification_center_mac.mm:16: alerts_ = [NSMutableArray arrayWithCapacity:0]; And Robert seems to agree with you so I changed it. Apparently getting an autorelease object is not advised unless you control the autorelease pool (which we don't for tests) On 2016/11/01 16:13:52, Miguel Garcia wrote: > On 2016/11/01 11:25:39, Peter Beverloo wrote: > > nit: [[NSMutableArray alloc] init]; > > I rather use ref counting here, why would you want to deal with the lifecycle > yourself? > I changed it to capacity 1 since that's what most test use (it's just a hint > anyway)
https://codereview.chromium.org/2458703003/diff/80001/chrome/browser/notifica... File chrome/browser/notifications/stub_notification_center_mac.mm (right): https://codereview.chromium.org/2458703003/diff/80001/chrome/browser/notifica... chrome/browser/notifications/stub_notification_center_mac.mm:16: alerts_ = [NSMutableArray arrayWithCapacity:0]; On 2016/11/01 16:15:49, Miguel Garcia wrote: > And Robert seems to agree with you so I changed it. Apparently getting an > autorelease object is not advised unless you control the autorelease pool (which > we don't for tests) > The issue is storing an autoreleased object (which you do not own) as an ivar. You can add ownership with either retain, or alloc/init. While they are functionally equivalent in this case, alloc/init is conventional. https://codereview.chromium.org/2458703003/diff/120001/chrome/browser/notific... File chrome/browser/notifications/stub_notification_center_mac.h (right): https://codereview.chromium.org/2458703003/diff/120001/chrome/browser/notific... chrome/browser/notifications/stub_notification_center_mac.h:10: @class NSUserNotificationCenterDelegate; Not needed with AppKit.h? https://codereview.chromium.org/2458703003/diff/120001/chrome/browser/notific... chrome/browser/notifications/stub_notification_center_mac.h:14: // notifications. Join with previous line. https://codereview.chromium.org/2458703003/diff/120001/chrome/browser/notific... chrome/browser/notifications/stub_notification_center_mac.h:17: // by the caller. Join with previous line. https://codereview.chromium.org/2458703003/diff/120001/chrome/browser/notific... File chrome/browser/notifications/stub_notification_center_mac.mm (right): https://codereview.chromium.org/2458703003/diff/120001/chrome/browser/notific... chrome/browser/notifications/stub_notification_center_mac.mm:63: - (void)setDelegate:(NSUserNotificationCenterDelegate*)delegate { Leave a comment why you nop this out?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by miguelg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2458703003/diff/120001/chrome/browser/notific... File chrome/browser/notifications/stub_notification_center_mac.h (right): https://codereview.chromium.org/2458703003/diff/120001/chrome/browser/notific... chrome/browser/notifications/stub_notification_center_mac.h:10: @class NSUserNotificationCenterDelegate; On 2016/11/01 17:09:01, Robert Sesek wrote: > Not needed with AppKit.h? Peter asked the same thing. I swear it does not compile if I remove this :) https://codereview.chromium.org/2458703003/diff/120001/chrome/browser/notific... chrome/browser/notifications/stub_notification_center_mac.h:14: // notifications. On 2016/11/01 17:09:01, Robert Sesek wrote: > Join with previous line. Done. https://codereview.chromium.org/2458703003/diff/120001/chrome/browser/notific... chrome/browser/notifications/stub_notification_center_mac.h:17: // by the caller. On 2016/11/01 17:09:01, Robert Sesek wrote: > Join with previous line. Done. https://codereview.chromium.org/2458703003/diff/120001/chrome/browser/notific... File chrome/browser/notifications/stub_notification_center_mac.mm (right): https://codereview.chromium.org/2458703003/diff/120001/chrome/browser/notific... chrome/browser/notifications/stub_notification_center_mac.mm:63: - (void)setDelegate:(NSUserNotificationCenterDelegate*)delegate { On 2016/11/01 17:09:01, Robert Sesek wrote: > Leave a comment why you nop this out? Done.
LGTM % import comment https://codereview.chromium.org/2458703003/diff/120001/chrome/browser/notific... File chrome/browser/notifications/stub_notification_center_mac.h (right): https://codereview.chromium.org/2458703003/diff/120001/chrome/browser/notific... chrome/browser/notifications/stub_notification_center_mac.h:10: @class NSUserNotificationCenterDelegate; On 2016/11/07 14:18:41, Miguel Garcia wrote: > On 2016/11/01 17:09:01, Robert Sesek wrote: > > Not needed with AppKit.h? > > Peter asked the same thing. I swear it does not compile if I remove this :) Oh, try Cocoa/Cocoa.h instead. NSUserNotificationCenter and friends are in Foundation rather than AppKit. Cocoa is the forwarding header for both. https://codereview.chromium.org/2458703003/diff/140001/chrome/browser/notific... File chrome/browser/notifications/stub_notification_center_mac.mm (right): https://codereview.chromium.org/2458703003/diff/140001/chrome/browser/notific... chrome/browser/notifications/stub_notification_center_mac.mm:5: #include "chrome/browser/notifications/stub_notification_center_mac.h" #import instead of #include here
https://codereview.chromium.org/2458703003/diff/120001/chrome/browser/notific... File chrome/browser/notifications/stub_notification_center_mac.h (right): https://codereview.chromium.org/2458703003/diff/120001/chrome/browser/notific... chrome/browser/notifications/stub_notification_center_mac.h:10: @class NSUserNotificationCenterDelegate; On 2016/11/07 16:16:50, Robert Sesek wrote: > On 2016/11/07 14:18:41, Miguel Garcia wrote: > > On 2016/11/01 17:09:01, Robert Sesek wrote: > > > Not needed with AppKit.h? > > > > Peter asked the same thing. I swear it does not compile if I remove this :) > > Oh, try Cocoa/Cocoa.h instead. NSUserNotificationCenter and friends are in > Foundation rather than AppKit. Cocoa is the forwarding header for both. Done, (NSUserNotificationCenterDelegate is a protocol so it has to be passed as id<NSUserNotificationCenterDelegate> ) https://codereview.chromium.org/2458703003/diff/140001/chrome/browser/notific... File chrome/browser/notifications/stub_notification_center_mac.mm (right): https://codereview.chromium.org/2458703003/diff/140001/chrome/browser/notific... chrome/browser/notifications/stub_notification_center_mac.mm:5: #include "chrome/browser/notifications/stub_notification_center_mac.h" On 2016/11/07 16:16:50, Robert Sesek wrote: > #import instead of #include here Done, it is not really clear to me what are the rules to decide this, can you explain them quickly or send me a pointer?
The CQ bit was checked by miguelg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org Link to the patchset: https://codereview.chromium.org/2458703003/#ps160001 (title: "review")
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 ========== Create a stub NSUserNotificationCenter and make tests use it. Also fix a bug in the bridge found by the tests. BUG=571056 ========== to ========== Create a stub NSUserNotificationCenter and make tests use it. Also fix a bug in the bridge found by the tests. BUG=571056 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Create a stub NSUserNotificationCenter and make tests use it. Also fix a bug in the bridge found by the tests. BUG=571056 ========== to ========== Create a stub NSUserNotificationCenter and make tests use it. Also fix a bug in the bridge found by the tests. BUG=571056 Committed: https://crrev.com/f052abfe2134e4af31100d49d3858ed8727f8c52 Cr-Commit-Position: refs/heads/master@{#430359} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/f052abfe2134e4af31100d49d3858ed8727f8c52 Cr-Commit-Position: refs/heads/master@{#430359} |