Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1972013003:
Add ContextMenuCoordinator. (Closed)
DescriptionContextMenuCoordinator replaces ContextMenuController with an
updated interface that accepts ContextMenuParams instead of the
intermediate ContextMenuHolder object.
BUG=614389
Committed: https://crrev.com/94187267a370bf4c0950e3e4e68e4f360510e166
Cr-Commit-Position: refs/heads/master@{#395652}
Patch Set 1 #Patch Set 2 : Remove old files. #
Total comments: 22
Patch Set 3 : Respond to CL comments. #
Total comments: 27
Patch Set 4 : Cleanup based on CL comments. #
Total comments: 29
Patch Set 5 : Respond to CL comments. #Patch Set 6 : Rename ContextMenuWrangler to coordinator. #
Total comments: 6
Patch Set 7 : Improve unittest context menu validation. #
Total comments: 21
Patch Set 8 : Split unittest suite up into multiple tests. #
Total comments: 26
Patch Set 9 : Cleanup #Patch Set 10 : Cleanup local variable names. #Patch Set 11 : Revert web_state_delegate_bridge_unittest. #
Total comments: 2
Patch Set 12 : Rebase. Update comments. #Patch Set 13 : Fix unittest when running on iPhone. #
Dependent Patchsets: Messages
Total messages: 51 (16 generated)
michaeldo@chromium.org changed reviewers: + eugenebut@chromium.org
https://codereview.chromium.org/1972013003/diff/20001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/context_menu/context_menu_controller.mm (right): https://codereview.chromium.org/1972013003/diff/20001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/context_menu/context_menu_controller.mm:17: @interface ContextMenuController () { Thank you for this cleanup. Could you please land it separately. https://codereview.chromium.org/1972013003/diff/20001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/context_menu/context_menu_wrangler.h (right): https://codereview.chromium.org/1972013003/diff/20001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/context_menu/context_menu_wrangler.h:16: // ContextMenuHolder with the title and action to associate to each menu Please update comments, it does not use take ContextMenuHolder https://codereview.chromium.org/1972013003/diff/20001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/context_menu/context_menu_wrangler.h:18: @interface ContextMenuWrangler : ContextMenuController Probably there is no need for subclassing. https://codereview.chromium.org/1972013003/diff/20001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/context_menu/context_menu_wrangler.h:20: // Initialize with details provided in |params|. s/Initialize/Initializes https://codereview.chromium.org/1972013003/diff/20001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/context_menu/context_menu_wrangler.h:23: - (void)appendItemWithTitle:(NSString*)title action:(ProceduralBlock)action; s/append/add ? which is a common add action in Objective-C libraries. https://codereview.chromium.org/1972013003/diff/20001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/context_menu/context_menu_wrangler.mm (right): https://codereview.chromium.org/1972013003/diff/20001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/context_menu/context_menu_wrangler.mm:22: @property(nonatomic, readonly) UIAlertController* alert; NIT: s/alert/alertController https://codereview.chromium.org/1972013003/diff/20001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/context_menu/context_menu_wrangler.mm:80: base::WeakNSObject<ContextMenuWrangler> weakSelf(self); This should be a part of alert creation, not presentation. Presenting the alert multiple times should not end up with multiple Cancel buttons. https://codereview.chromium.org/1972013003/diff/20001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/context_menu/context_menu_wrangler_unittest.mm (right): https://codereview.chromium.org/1972013003/diff/20001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/context_menu/context_menu_wrangler_unittest.mm:14: namespace { NIT: No need to put tests into anonymous namespace https://codereview.chromium.org/1972013003/diff/20001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/context_menu/context_menu_wrangler_unittest.mm:16: class ContextMenuWranglerTest : public PlatformTest { Needs comment https://codereview.chromium.org/1972013003/diff/20001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/context_menu/context_menu_wrangler_unittest.mm:29: TEST_F(ContextMenuWranglerTest, DisplayContextMenu) { Every test needs comments https://codereview.chromium.org/1972013003/diff/20001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/context_menu/context_menu_wrangler_unittest.mm:36: [_menuWrangler appendItemWithTitle:@"foo" action:NULL]; s/NULL/nil or nullptr
https://codereview.chromium.org/1972013003/diff/20001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/context_menu/context_menu_controller.mm (right): https://codereview.chromium.org/1972013003/diff/20001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/context_menu/context_menu_controller.mm:17: @interface ContextMenuController () { On 2016/05/12 22:27:48, Eugene But wrote: > Thank you for this cleanup. Could you please land it separately. yes, I will move to a separate CL and remove from here. https://codereview.chromium.org/1972013003/diff/20001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/context_menu/context_menu_wrangler.h (right): https://codereview.chromium.org/1972013003/diff/20001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/context_menu/context_menu_wrangler.h:16: // ContextMenuHolder with the title and action to associate to each menu On 2016/05/12 22:27:48, Eugene But wrote: > Please update comments, it does not use take ContextMenuHolder Done. https://codereview.chromium.org/1972013003/diff/20001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/context_menu/context_menu_wrangler.h:18: @interface ContextMenuWrangler : ContextMenuController On 2016/05/12 22:27:48, Eugene But wrote: > Probably there is no need for subclassing. Done. https://codereview.chromium.org/1972013003/diff/20001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/context_menu/context_menu_wrangler.h:20: // Initialize with details provided in |params|. On 2016/05/12 22:27:48, Eugene But wrote: > s/Initialize/Initializes Done. https://codereview.chromium.org/1972013003/diff/20001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/context_menu/context_menu_wrangler.h:23: - (void)appendItemWithTitle:(NSString*)title action:(ProceduralBlock)action; On 2016/05/12 22:27:48, Eugene But wrote: > s/append/add ? which is a common add action in Objective-C libraries. I think you're right. Add seems better. Append is primarily used with strings. https://codereview.chromium.org/1972013003/diff/20001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/context_menu/context_menu_wrangler.mm (right): https://codereview.chromium.org/1972013003/diff/20001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/context_menu/context_menu_wrangler.mm:22: @property(nonatomic, readonly) UIAlertController* alert; On 2016/05/12 22:27:48, Eugene But wrote: > NIT: s/alert/alertController Done. https://codereview.chromium.org/1972013003/diff/20001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/context_menu/context_menu_wrangler.mm:80: base::WeakNSObject<ContextMenuWrangler> weakSelf(self); On 2016/05/12 22:27:48, Eugene But wrote: > This should be a part of alert creation, not presentation. Presenting the alert > multiple times should not end up with multiple Cancel buttons. I thought it would not end up at the end of the list based on the comment in existing contextMenuController. I'll move to creation! https://codereview.chromium.org/1972013003/diff/20001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/context_menu/context_menu_wrangler_unittest.mm (right): https://codereview.chromium.org/1972013003/diff/20001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/context_menu/context_menu_wrangler_unittest.mm:14: namespace { On 2016/05/12 22:27:49, Eugene But wrote: > NIT: No need to put tests into anonymous namespace Done. https://codereview.chromium.org/1972013003/diff/20001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/context_menu/context_menu_wrangler_unittest.mm:16: class ContextMenuWranglerTest : public PlatformTest { On 2016/05/12 22:27:49, Eugene But wrote: > Needs comment Done. https://codereview.chromium.org/1972013003/diff/20001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/context_menu/context_menu_wrangler_unittest.mm:29: TEST_F(ContextMenuWranglerTest, DisplayContextMenu) { On 2016/05/12 22:27:49, Eugene But wrote: > Every test needs comments Done. https://codereview.chromium.org/1972013003/diff/20001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/context_menu/context_menu_wrangler_unittest.mm:36: [_menuWrangler appendItemWithTitle:@"foo" action:NULL]; On 2016/05/12 22:27:49, Eugene But wrote: > s/NULL/nil or nullptr Done.
Description was changed from ========== Add ContextMenuWrangler. ContextMenuWrangler replaces ContextMenuController with an updated interface that accepts ContextMenuParams instead of the intermediate ContextMenuHolder object. Remove system alert view/action sheet abstraction layer from ContextMenuController. BUG=none ========== to ========== Add ContextMenuWrangler. ContextMenuWrangler replaces ContextMenuController with an updated interface that accepts ContextMenuParams instead of the intermediate ContextMenuHolder object. BUG=none ==========
eugenebut@chromium.org changed reviewers: + marq@chromium.org
lgtm, with comments. I assume that Mark will also take a look. https://codereview.chromium.org/1972013003/diff/40001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/context_menu/context_menu_wrangler.h (right): https://codereview.chromium.org/1972013003/diff/40001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/context_menu/context_menu_wrangler.h:21: @property(nonatomic, readonly, getter=isVisible) BOOL visible; Is this method actually needed? https://codereview.chromium.org/1972013003/diff/40001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/context_menu/context_menu_wrangler.mm (right): https://codereview.chromium.org/1972013003/diff/40001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/context_menu/context_menu_wrangler.mm:27: // Displays a context menu. Drop this comment. Interface already has documentation. https://codereview.chromium.org/1972013003/diff/40001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/context_menu/context_menu_wrangler.mm:45: - (UIAlertController*)alertController { Move to the bottom. Before |alertDismissed|. https://codereview.chromium.org/1972013003/diff/40001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/context_menu/context_menu_wrangler.mm:56: UIAlertAction* cancel_action = s/cancel_action/cancelAction https://codereview.chromium.org/1972013003/diff/40001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/context_menu/context_menu_wrangler_unittest.mm (right): https://codereview.chromium.org/1972013003/diff/40001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/context_menu/context_menu_wrangler_unittest.mm:24: base::scoped_nsobject<ContextMenuWrangler> _menuWrangler; s/_menuWrangler/menu_wrangler_ https://codereview.chromium.org/1972013003/diff/40001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/context_menu/context_menu_wrangler_unittest.mm:25: base::scoped_nsobject<UIWindow> _window; s/_window/window_
eugenebut@chromium.org changed reviewers: + jyquinn@chromium.org
Sorry, missed the fact that Jackie is an owner of this. Ideally you want a review from owners in the folder you changing.
marq@chromium.org changed required reviewers: + marq@chromium.org
https://codereview.chromium.org/1972013003/diff/40001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/context_menu/context_menu_wrangler.h (right): https://codereview.chromium.org/1972013003/diff/40001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/context_menu/context_menu_wrangler.h:24: - (instancetype)initWithContextMenuParams:(const web::ContextMenuParams&)params; I'd like coordinators to typically accept a view controller as a parameter; in this case it would be used as the VC that the context menu is presented from. https://codereview.chromium.org/1972013003/diff/40001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/context_menu/context_menu_wrangler.h:28: - (void)present; The coordinator's job is to manage a discrete UI interaction; I prefer 'start' as the verb/method name used to kick things off. https://codereview.chromium.org/1972013003/diff/40001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/context_menu/context_menu_wrangler.h:31: - (void)dismissAnimated:(BOOL)animated What's the use case for this being called by external code? My preference would be for a 'stop' method that determines if any animation needs to happen internally. https://codereview.chromium.org/1972013003/diff/40001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/context_menu/context_menu_wrangler.mm (right): https://codereview.chromium.org/1972013003/diff/40001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/context_menu/context_menu_wrangler.mm:51: alert.popoverPresentationController.sourceView = _params.view; DCHECK at some stage that _params.view is a subview of the presenting view controller's view? https://codereview.chromium.org/1972013003/diff/40001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/context_menu/context_menu_wrangler.mm:69: - (void)addItemWithTitle:(NSString*)title action:(ProceduralBlock)actionBlock { It feels like everything that's added here should really be in the contextMenuParams. (modulo whatever decision we make about actions). https://codereview.chromium.org/1972013003/diff/40001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/context_menu/context_menu_wrangler.mm:72: [weakSelf alertDismissed]; Annoying that there's no callback or delegate to let us know when the sheet is done. https://codereview.chromium.org/1972013003/diff/40001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/context_menu/context_menu_wrangler.mm:93: topController = topController.presentedViewController; We do this a lot and I regard it as a fairly pernicious antipattern. Much better to have this object be handed the VC that will present it.
https://codereview.chromium.org/1972013003/diff/40001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/context_menu/context_menu_wrangler.h (right): https://codereview.chromium.org/1972013003/diff/40001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/context_menu/context_menu_wrangler.h:21: @property(nonatomic, readonly, getter=isVisible) BOOL visible; On 2016/05/13 01:23:55, Eugene But wrote: > Is this method actually needed? It is needed (for now). It is used downstream, I can PM you more details. https://codereview.chromium.org/1972013003/diff/40001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/context_menu/context_menu_wrangler.h:24: - (instancetype)initWithContextMenuParams:(const web::ContextMenuParams&)params; On 2016/05/13 14:51:29, marq wrote: > I'd like coordinators to typically accept a view controller as a parameter; in > this case it would be used as the VC that the context menu is presented from. Done. https://codereview.chromium.org/1972013003/diff/40001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/context_menu/context_menu_wrangler.h:28: - (void)present; On 2016/05/13 14:51:29, marq wrote: > The coordinator's job is to manage a discrete UI interaction; I prefer 'start' > as the verb/method name used to kick things off. Done. https://codereview.chromium.org/1972013003/diff/40001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/context_menu/context_menu_wrangler.h:31: - (void)dismissAnimated:(BOOL)animated On 2016/05/13 14:51:29, marq wrote: > What's the use case for this being called by external code? My preference would > be for a 'stop' method that determines if any animation needs to happen > internally. This appears to only be called from "dismissModals", so it would be no animation. I have updated it. https://codereview.chromium.org/1972013003/diff/40001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/context_menu/context_menu_wrangler.mm (right): https://codereview.chromium.org/1972013003/diff/40001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/context_menu/context_menu_wrangler.mm:27: // Displays a context menu. On 2016/05/13 01:23:55, Eugene But wrote: > Drop this comment. Interface already has documentation. Done. https://codereview.chromium.org/1972013003/diff/40001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/context_menu/context_menu_wrangler.mm:45: - (UIAlertController*)alertController { On 2016/05/13 01:23:55, Eugene But wrote: > Move to the bottom. Before |alertDismissed|. Done. https://codereview.chromium.org/1972013003/diff/40001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/context_menu/context_menu_wrangler.mm:51: alert.popoverPresentationController.sourceView = _params.view; On 2016/05/13 14:51:29, marq wrote: > DCHECK at some stage that _params.view is a subview of the presenting view > controller's view? I agree, good idea. Done. https://codereview.chromium.org/1972013003/diff/40001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/context_menu/context_menu_wrangler.mm:56: UIAlertAction* cancel_action = On 2016/05/13 01:23:55, Eugene But wrote: > s/cancel_action/cancelAction Done. https://codereview.chromium.org/1972013003/diff/40001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/context_menu/context_menu_wrangler.mm:69: - (void)addItemWithTitle:(NSString*)title action:(ProceduralBlock)actionBlock { On 2016/05/13 14:51:29, marq wrote: > It feels like everything that's added here should really be in the > contextMenuParams. > > (modulo whatever decision we make about actions). This would go against what chromium is doing, but I do understand what you mean. I think the use of this entry point will diminish if we can successfully merge the shared logic. This seems OK to me for now, and we can evaluate the remaining use cases at that point. https://codereview.chromium.org/1972013003/diff/40001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/context_menu/context_menu_wrangler.mm:72: [weakSelf alertDismissed]; On 2016/05/13 14:51:29, marq wrote: > Annoying that there's no callback or delegate to let us know when the sheet is > done. This is in the plans, once migrated to this new class, there is still work to be done to merge all the similar logic. There is no need right now for the delegate, so the block is the only current callback. https://codereview.chromium.org/1972013003/diff/40001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/context_menu/context_menu_wrangler.mm:93: topController = topController.presentedViewController; On 2016/05/13 14:51:29, marq wrote: > We do this a lot and I regard it as a fairly pernicious antipattern. Much better > to have this object be handed the VC that will present it. I agree, will update to pass in the VC. https://codereview.chromium.org/1972013003/diff/40001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/context_menu/context_menu_wrangler_unittest.mm (right): https://codereview.chromium.org/1972013003/diff/40001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/context_menu/context_menu_wrangler_unittest.mm:24: base::scoped_nsobject<ContextMenuWrangler> _menuWrangler; On 2016/05/13 01:23:56, Eugene But wrote: > s/_menuWrangler/menu_wrangler_ Done. https://codereview.chromium.org/1972013003/diff/40001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/context_menu/context_menu_wrangler_unittest.mm:25: base::scoped_nsobject<UIWindow> _window; On 2016/05/13 01:23:56, Eugene But wrote: > s/_window/window_ Done.
https://codereview.chromium.org/1972013003/diff/40001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/context_menu/context_menu_wrangler.mm (right): https://codereview.chromium.org/1972013003/diff/40001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/context_menu/context_menu_wrangler.mm:69: - (void)addItemWithTitle:(NSString*)title action:(ProceduralBlock)actionBlock { On 2016/05/17 18:03:03, michaeldo wrote: > On 2016/05/13 14:51:29, marq wrote: > > It feels like everything that's added here should really be in the > > contextMenuParams. > > > > (modulo whatever decision we make about actions). > > This would go against what chromium is doing, but I do understand what you mean. > I think the use of this entry point will diminish if we can successfully merge > the shared logic. This seems OK to me for now, and we can evaluate the remaining > use cases at that point. Web can not be responsible for deciding about the content of the content menu (f.e. web does not have a concept of Tabs). Because of that web::ContentMenuParams mirrors content::ContentMenuParams.
https://codereview.chromium.org/1972013003/diff/60001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/context_menu/context_menu_wrangler.h (right): https://codereview.chromium.org/1972013003/diff/60001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/context_menu/context_menu_wrangler.h:25: - (instancetype)initWithViewController:(UIViewController*)viewController initWithPresentingViewController: ? https://codereview.chromium.org/1972013003/diff/60001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/context_menu/context_menu_wrangler.h:32: - (void)start; Method should describe the action. -[ContextMenuWrangler start] is too generic and can mean anything.
https://codereview.chromium.org/1972013003/diff/60001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/context_menu/context_menu_wrangler.h (right): https://codereview.chromium.org/1972013003/diff/60001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/context_menu/context_menu_wrangler.h:25: - (instancetype)initWithViewController:(UIViewController*)viewController On 2016/05/17 19:40:30, Eugene But wrote: > initWithPresentingViewController: ? I like that name a bit better, but then the method name wraps strangely: - (instancetype) initWithPresentingViewController:(UIViewController*)viewController params:(const web::ContextMenuParams&)params; https://codereview.chromium.org/1972013003/diff/60001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/context_menu/context_menu_wrangler.h:32: - (void)start; On 2016/05/17 19:40:30, Eugene But wrote: > Method should describe the action. -[ContextMenuWrangler start] is too generic > and can mean anything. Mark, what do you think? This should be consistent for coordinators, but until this is named a coordinator (and likely inheriting from your ChromeCoordinator), should I rename this to something more descriptive like "present"?
https://codereview.chromium.org/1972013003/diff/60001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/context_menu/context_menu_wrangler.h (right): https://codereview.chromium.org/1972013003/diff/60001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/context_menu/context_menu_wrangler.h:32: - (void)start; On 2016/05/17 21:08:30, michaeldo wrote: > On 2016/05/17 19:40:30, Eugene But wrote: > > Method should describe the action. -[ContextMenuWrangler start] is too generic > > and can mean anything. > > Mark, what do you think? This should be consistent for coordinators, but until > this is named a coordinator (and likely inheriting from your ChromeCoordinator), > should I rename this to something more descriptive like "present"? Honestly I really have troubles finding a good name which is: 1.) Prefixed with "start" and/or has only word 2.) Follows "Coding Guidelines for Cocoa" 3.) Reflects the purpose of the methods Even if the class name is ContextMenuCoordinator, one word name like "start" still does not feel right. "Starting coordination" is very vague thing, while method name should be very specific. Questions to Mark: Why do you want start/stop? Is there a class in Standard library that inspired you for this naming scheme? (if there is no class than maybe we should not use start/stop naming scheme at all. Objective-C standard library is very mature and if some naming scheme does not exist there, it probably should not exist at all). How would you name this method?
https://codereview.chromium.org/1972013003/diff/60001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/context_menu/context_menu_wrangler.h (right): https://codereview.chromium.org/1972013003/diff/60001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/context_menu/context_menu_wrangler.h:25: - (instancetype)initWithViewController:(UIViewController*)viewController On 2016/05/17 21:08:30, michaeldo wrote: > On 2016/05/17 19:40:30, Eugene But wrote: > > initWithPresentingViewController: ? > > I like that name a bit better, but then the method name wraps strangely: The fact that this object happens to use |viewController| for presentation is an implementation detail. In general coordinators will accept some view controller that they run 'on top of', but how they do that shouldn't matter elsewhere. So I don't think this is necessary. > - (instancetype) > initWithPresentingViewController:(UIViewController*)viewController > params:(const web::ContextMenuParams&)params; Line length issues should be at the bottom of the list of concerns around naming; if line length limits change, we wouldn't go back and use more descriptive but more cumbersome names. You can also wrap like this: - (instancetype)initWithPresentingViewController: (UIViewController*)viewController params: (const web::ContextMenuParams&)params; (Although, for obvious reasons, you may not care to). https://codereview.chromium.org/1972013003/diff/60001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/context_menu/context_menu_wrangler.h:32: - (void)start; On 2016/05/17 22:49:40, Eugene But wrote: > On 2016/05/17 21:08:30, michaeldo wrote: > > On 2016/05/17 19:40:30, Eugene But wrote: > > > Method should describe the action. -[ContextMenuWrangler start] is too > generic > > > and can mean anything. > > > > Mark, what do you think? This should be consistent for coordinators, but until > > this is named a coordinator (and likely inheriting from your > ChromeCoordinator), > > should I rename this to something more descriptive like "present"? > Honestly I really have troubles finding a good name which is: > 1.) Prefixed with "start" and/or has only word > 2.) Follows "Coding Guidelines for Cocoa" > 3.) Reflects the purpose of the methods > > Even if the class name is ContextMenuCoordinator, one word name like "start" > still does not feel right. "Starting coordination" is very vague thing, while > method name should be very specific. Questions to Mark: > Why do you want start/stop? > Is there a class in Standard library that inspired you for this naming scheme? > (if there is no class than maybe we should not use start/stop naming scheme at > all. Objective-C standard library is very mature and if some naming scheme does > not exist there, it probably should not exist at all). > How would you name this method? The overall idea is that a coordinator manages a specific user interaction -- a context menu, a login screen, First Run, a Safe Mode launch, etc. But the specifics of how that UI is displayed is by design not part of the API. It's intended to be understood that when you -start a ContextMenuCoordinator, that what it will start doing is running the UI for a context menu. The presentation of the menu is part, but not all of that; to be accurate the method would be something like -startRunningTheMenu, but since this class's job is, in fact, to run the menu, it seems sufficient to me to just say 'start'. The best analogy I can think of to existing iOS libraries is to network sessions of various kinds -- you set them up, then you start them, and you may stop, pause, or cancel them along the way. GKVoiceChat has a -start method (not -startVoiceChat) and NSURLSessionTask has -cancel, -resume, and -suspend (not -cancelTask or -cancelLoading or whatever). https://codereview.chromium.org/1972013003/diff/60001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/context_menu/context_menu_wrangler.mm (right): https://codereview.chromium.org/1972013003/diff/60001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/context_menu/context_menu_wrangler.mm:15: base::WeakNSObject<UIAlertController> _alertController; Should this really be weak? https://codereview.chromium.org/1972013003/diff/60001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/context_menu/context_menu_wrangler.mm:30: @synthesize visible = _visible; Doesn't this property end up just being YES if _alertController is nonnull and NO otherwise? If so, do you need to track it in a separate variable? https://codereview.chromium.org/1972013003/diff/60001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/context_menu/context_menu_wrangler.mm:41: #pragma mark - Object lifecycle https://codereview.chromium.org/1972013003/diff/60001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/context_menu/context_menu_wrangler.mm:43: [_alertController dismissViewControllerAnimated:NO completion:nil]; I'd prefer to have this call [self stop] instead of duplicating code. https://codereview.chromium.org/1972013003/diff/60001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/context_menu/context_menu_wrangler.mm:47: - (void)addItemWithTitle:(NSString*)title action:(ProceduralBlock)actionBlock { #pragma mark - Public methods https://codereview.chromium.org/1972013003/diff/60001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/context_menu/context_menu_wrangler.mm:64: _alertController.reset(); This case destroys all of the config information created by calls to -addItemWithTitle. That should probably be documented somewhere. https://codereview.chromium.org/1972013003/diff/60001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/context_menu/context_menu_wrangler.mm:68: [_presentingViewController presentViewController:_alertController _alertController or self.alertController? https://codereview.chromium.org/1972013003/diff/60001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/context_menu/context_menu_wrangler.mm:80: - (UIAlertController*)alertController { #pragma mark - Property implementation https://codereview.chromium.org/1972013003/diff/60001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/context_menu/context_menu_wrangler.mm:105: - (void)alertDismissed { #pragma mark - Private methods https://codereview.chromium.org/1972013003/diff/60001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/context_menu/context_menu_wrangler.mm:106: [self setVisible:NO]; Please be consistent with how you assign to this property; I'd prefer dot notation everywhere. (self.visible = NO).
https://codereview.chromium.org/1972013003/diff/60001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/context_menu/context_menu_wrangler.mm (right): https://codereview.chromium.org/1972013003/diff/60001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/context_menu/context_menu_wrangler.mm:15: base::WeakNSObject<UIAlertController> _alertController; On 2016/05/18 10:38:10, marq wrote: > Should this really be weak? This should NOT be weak, since it may be deallocated after some buttons are added to it, but before it is presented. This could cause some "lost buttons. My mistake was that it is weak in the existing ContextMenuController class, which is where much of this logic is modeled after. https://codereview.chromium.org/1972013003/diff/60001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/context_menu/context_menu_wrangler.mm:30: @synthesize visible = _visible; On 2016/05/18 10:38:10, marq wrote: > Doesn't this property end up just being YES if _alertController is nonnull and > NO otherwise? If so, do you need to track it in a separate variable? It doesn't quite follow _alertController. This is set to yes when start is called, and set to NO once dismissed. The exact use case where we use this externally should be evaluated. Maybe at that location we can just check if a ContextMenuCoordinator exists or not. https://codereview.chromium.org/1972013003/diff/60001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/context_menu/context_menu_wrangler.mm:41: On 2016/05/18 10:38:10, marq wrote: > #pragma mark - Object lifecycle Done. https://codereview.chromium.org/1972013003/diff/60001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/context_menu/context_menu_wrangler.mm:43: [_alertController dismissViewControllerAnimated:NO completion:nil]; On 2016/05/18 10:38:10, marq wrote: > I'd prefer to have this call [self stop] instead of duplicating code. Done. https://codereview.chromium.org/1972013003/diff/60001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/context_menu/context_menu_wrangler.mm:47: - (void)addItemWithTitle:(NSString*)title action:(ProceduralBlock)actionBlock { On 2016/05/18 10:38:09, marq wrote: > #pragma mark - Public methods Done. https://codereview.chromium.org/1972013003/diff/60001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/context_menu/context_menu_wrangler.mm:64: _alertController.reset(); On 2016/05/18 10:38:10, marq wrote: > This case destroys all of the config information created by calls to > -addItemWithTitle. That should probably be documented somewhere. I agree, but maybe it isn't really necessary to destroy it? That was the prior logic, but I'd prefer to simply early return. https://codereview.chromium.org/1972013003/diff/60001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/context_menu/context_menu_wrangler.mm:68: [_presentingViewController presentViewController:_alertController On 2016/05/18 10:38:09, marq wrote: > _alertController or self.alertController? I changed this to self.alertController. I think it is better, although if it hasn't been created until it is presented, it will only have a cancel button. https://codereview.chromium.org/1972013003/diff/60001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/context_menu/context_menu_wrangler.mm:80: - (UIAlertController*)alertController { On 2016/05/18 10:38:10, marq wrote: > #pragma mark - Property implementation Done. https://codereview.chromium.org/1972013003/diff/60001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/context_menu/context_menu_wrangler.mm:105: - (void)alertDismissed { On 2016/05/18 10:38:10, marq wrote: > #pragma mark - Private methods Done. https://codereview.chromium.org/1972013003/diff/60001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/context_menu/context_menu_wrangler.mm:106: [self setVisible:NO]; On 2016/05/18 10:38:09, marq wrote: > Please be consistent with how you assign to this property; I'd prefer dot > notation everywhere. (self.visible = NO). agreed! Done.
https://codereview.chromium.org/1972013003/diff/60001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/context_menu/context_menu_wrangler.h (right): https://codereview.chromium.org/1972013003/diff/60001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/context_menu/context_menu_wrangler.h:25: - (instancetype)initWithViewController:(UIViewController*)viewController On 2016/05/18 10:38:09, marq wrote: > On 2016/05/17 21:08:30, michaeldo wrote: > > On 2016/05/17 19:40:30, Eugene But wrote: > > > initWithPresentingViewController: ? > > > > I like that name a bit better, but then the method name wraps strangely: > > The fact that this object happens to use |viewController| for presentation is an > implementation detail. In general coordinators will accept some view controller > that they run 'on top of', but how they do that shouldn't matter elsewhere. So I > don't think this is necessary. > > > - (instancetype) > > initWithPresentingViewController:(UIViewController*)viewController > > params:(const web::ContextMenuParams&)params; > > Line length issues should be at the bottom of the list of concerns around > naming; if line length limits change, we wouldn't go back and use more > descriptive but more cumbersome names. > > You can also wrap like this: > > - (instancetype)initWithPresentingViewController: > (UIViewController*)viewController > params: > (const web::ContextMenuParams&)params; > > (Although, for obvious reasons, you may not care to). If clients of this method don't know how |viewController| is used, then how do they know what should be passed? Root view controller, view controller which is not in the hierarchy, nil? https://codereview.chromium.org/1972013003/diff/60001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/context_menu/context_menu_wrangler.h:32: - (void)start; On 2016/05/18 10:38:09, marq wrote: > On 2016/05/17 22:49:40, Eugene But wrote: > > On 2016/05/17 21:08:30, michaeldo wrote: > > > On 2016/05/17 19:40:30, Eugene But wrote: > > > > Method should describe the action. -[ContextMenuWrangler start] is too > > generic > > > > and can mean anything. > > > > > > Mark, what do you think? This should be consistent for coordinators, but > until > > > this is named a coordinator (and likely inheriting from your > > ChromeCoordinator), > > > should I rename this to something more descriptive like "present"? > > Honestly I really have troubles finding a good name which is: > > 1.) Prefixed with "start" and/or has only word > > 2.) Follows "Coding Guidelines for Cocoa" > > 3.) Reflects the purpose of the methods > > > > Even if the class name is ContextMenuCoordinator, one word name like "start" > > still does not feel right. "Starting coordination" is very vague thing, while > > method name should be very specific. Questions to Mark: > > Why do you want start/stop? > > Is there a class in Standard library that inspired you for this naming scheme? > > (if there is no class than maybe we should not use start/stop naming scheme at > > all. Objective-C standard library is very mature and if some naming scheme > does > > not exist there, it probably should not exist at all). > > How would you name this method? > > The overall idea is that a coordinator manages a specific user interaction -- a > context menu, a login screen, First Run, a Safe Mode launch, etc. But the > specifics of how that UI is displayed is by design not part of the API. It's > intended to be understood that when you -start a ContextMenuCoordinator, that > what it will start doing is running the UI for a context menu. The presentation > of the menu is part, but not all of that; to be accurate the method would be > something like -startRunningTheMenu, but since this class's job is, in fact, to > run the menu, it seems sufficient to me to just say 'start'. > > The best analogy I can think of to existing iOS libraries is to network sessions > of various kinds -- you set them up, then you start them, and you may stop, > pause, or cancel them along the way. GKVoiceChat has a -start method (not > -startVoiceChat) and NSURLSessionTask has -cancel, -resume, and -suspend (not > -cancelTask or -cancelLoading or whatever). Thanks for detailed explanation, I understand your rationale now. |startRunningTheMenu| would indeed be an unusual name for Objecitve-C. [ContextMenuCoordinator start] is very different from [GKVoiceChat start]. The latter is well defined thing (it starts a voice chat), while people will obsoletely need to read class comments to understand what [ContextMenuCoordinator start] does. Anyways discussion the naming scheme is out of scope for this CL, so I'm done with asking the questions :) Overall the idea of coordinators sounds very good to me. Thanks again.
https://codereview.chromium.org/1972013003/diff/100001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/context_menu/context_menu_coordinator_unittest.mm (right): https://codereview.chromium.org/1972013003/diff/100001/ios/chrome/browser/ui/... ios/chrome/browser/ui/context_menu/context_menu_coordinator_unittest.mm:38: action:^{ Why one nil action and one ^{} action? Also, would it be possible to test functionality of the context menu (that item titles and actions show up correctly in the context menu)?
https://codereview.chromium.org/1972013003/diff/100001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/context_menu/context_menu_coordinator_unittest.mm (right): https://codereview.chromium.org/1972013003/diff/100001/ios/chrome/browser/ui/... ios/chrome/browser/ui/context_menu/context_menu_coordinator_unittest.mm:38: action:^{ On 2016/05/19 15:39:28, Jackie Quinn wrote: > Why one nil action and one ^{} action? Also, would it be possible to test > functionality of the context menu (that item titles and actions show up > correctly in the context menu)? The nil vs block (even though it's empty) are simply two different cases, so I thought it would be good to cover both here. If nil was only used, the test may not cover adding an actual block crashes for some reason. Maybe a stretch, but I think it's worth having both. Testing the actual functionality would be ideal, but I'm not sure it's practical as that info isn't exposed. However, there are other tests which exercise the context menu functionality in the app.
lgtm
On 2016/05/18 10:38:10, marq wrote: > https://codereview.chromium.org/1972013003/diff/60001/ios/chrome/browser/ui/c... > File ios/chrome/browser/ui/context_menu/context_menu_wrangler.h (right): > > https://codereview.chromium.org/1972013003/diff/60001/ios/chrome/browser/ui/c... > ios/chrome/browser/ui/context_menu/context_menu_wrangler.h:25: - > (instancetype)initWithViewController:(UIViewController*)viewController > On 2016/05/17 21:08:30, michaeldo wrote: > > On 2016/05/17 19:40:30, Eugene But wrote: > > > initWithPresentingViewController: ? > > > > I like that name a bit better, but then the method name wraps strangely: > > The fact that this object happens to use |viewController| for presentation is an > implementation detail. In general coordinators will accept some view controller > that they run 'on top of', but how they do that shouldn't matter elsewhere. So I > don't think this is necessary. > > > - (instancetype) > > initWithPresentingViewController:(UIViewController*)viewController > > params:(const web::ContextMenuParams&)params; > > Line length issues should be at the bottom of the list of concerns around > naming; if line length limits change, we wouldn't go back and use more > descriptive but more cumbersome names. > > You can also wrap like this: > > - (instancetype)initWithPresentingViewController: > (UIViewController*)viewController > params: > (const web::ContextMenuParams&)params; > > (Although, for obvious reasons, you may not care to). > > https://codereview.chromium.org/1972013003/diff/60001/ios/chrome/browser/ui/c... > ios/chrome/browser/ui/context_menu/context_menu_wrangler.h:32: - (void)start; > On 2016/05/17 22:49:40, Eugene But wrote: > > On 2016/05/17 21:08:30, michaeldo wrote: > > > On 2016/05/17 19:40:30, Eugene But wrote: > > > > Method should describe the action. -[ContextMenuWrangler start] is too > > generic > > > > and can mean anything. > > > > > > Mark, what do you think? This should be consistent for coordinators, but > until > > > this is named a coordinator (and likely inheriting from your > > ChromeCoordinator), > > > should I rename this to something more descriptive like "present"? > > Honestly I really have troubles finding a good name which is: > > 1.) Prefixed with "start" and/or has only word > > 2.) Follows "Coding Guidelines for Cocoa" > > 3.) Reflects the purpose of the methods > > > > Even if the class name is ContextMenuCoordinator, one word name like "start" > > still does not feel right. "Starting coordination" is very vague thing, while > > method name should be very specific. Questions to Mark: > > Why do you want start/stop? > > Is there a class in Standard library that inspired you for this naming scheme? > > (if there is no class than maybe we should not use start/stop naming scheme at > > all. Objective-C standard library is very mature and if some naming scheme > does > > not exist there, it probably should not exist at all). > > How would you name this method? > > The overall idea is that a coordinator manages a specific user interaction -- a > context menu, a login screen, First Run, a Safe Mode launch, etc. But the > specifics of how that UI is displayed is by design not part of the API. It's > intended to be understood that when you -start a ContextMenuCoordinator, that > what it will start doing is running the UI for a context menu. The presentation > of the menu is part, but not all of that; to be accurate the method would be > something like -startRunningTheMenu, but since this class's job is, in fact, to > run the menu, it seems sufficient to me to just say 'start'. > > The best analogy I can think of to existing iOS libraries is to network sessions > of various kinds -- you set them up, then you start them, and you may stop, > pause, or cancel them along the way. GKVoiceChat has a -start method (not > -startVoiceChat) and NSURLSessionTask has -cancel, -resume, and -suspend (not > -cancelTask or -cancelLoading or whatever). > > https://codereview.chromium.org/1972013003/diff/60001/ios/chrome/browser/ui/c... > File ios/chrome/browser/ui/context_menu/context_menu_wrangler.mm (right): > > https://codereview.chromium.org/1972013003/diff/60001/ios/chrome/browser/ui/c... > ios/chrome/browser/ui/context_menu/context_menu_wrangler.mm:15: > base::WeakNSObject<UIAlertController> _alertController; > Should this really be weak? > > https://codereview.chromium.org/1972013003/diff/60001/ios/chrome/browser/ui/c... > ios/chrome/browser/ui/context_menu/context_menu_wrangler.mm:30: @synthesize > visible = _visible; > Doesn't this property end up just being YES if _alertController is nonnull and > NO otherwise? If so, do you need to track it in a separate variable? > > https://codereview.chromium.org/1972013003/diff/60001/ios/chrome/browser/ui/c... > ios/chrome/browser/ui/context_menu/context_menu_wrangler.mm:41: > #pragma mark - Object lifecycle > > https://codereview.chromium.org/1972013003/diff/60001/ios/chrome/browser/ui/c... > ios/chrome/browser/ui/context_menu/context_menu_wrangler.mm:43: > [_alertController dismissViewControllerAnimated:NO completion:nil]; > I'd prefer to have this call [self stop] instead of duplicating code. > > https://codereview.chromium.org/1972013003/diff/60001/ios/chrome/browser/ui/c... > ios/chrome/browser/ui/context_menu/context_menu_wrangler.mm:47: - > (void)addItemWithTitle:(NSString*)title action:(ProceduralBlock)actionBlock { > #pragma mark - Public methods > > https://codereview.chromium.org/1972013003/diff/60001/ios/chrome/browser/ui/c... > ios/chrome/browser/ui/context_menu/context_menu_wrangler.mm:64: > _alertController.reset(); > This case destroys all of the config information created by calls to > -addItemWithTitle. That should probably be documented somewhere. > > https://codereview.chromium.org/1972013003/diff/60001/ios/chrome/browser/ui/c... > ios/chrome/browser/ui/context_menu/context_menu_wrangler.mm:68: > [_presentingViewController presentViewController:_alertController > _alertController or self.alertController? > > https://codereview.chromium.org/1972013003/diff/60001/ios/chrome/browser/ui/c... > ios/chrome/browser/ui/context_menu/context_menu_wrangler.mm:80: - > (UIAlertController*)alertController { > #pragma mark - Property implementation > > https://codereview.chromium.org/1972013003/diff/60001/ios/chrome/browser/ui/c... > ios/chrome/browser/ui/context_menu/context_menu_wrangler.mm:105: - > (void)alertDismissed { > #pragma mark - Private methods > > https://codereview.chromium.org/1972013003/diff/60001/ios/chrome/browser/ui/c... > ios/chrome/browser/ui/context_menu/context_menu_wrangler.mm:106: [self > setVisible:NO]; > Please be consistent with how you assign to this property; I'd prefer dot > notation everywhere. (self.visible = NO). Mark, PTAL. Thanks!
https://codereview.chromium.org/1972013003/diff/100001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/context_menu/context_menu_coordinator_unittest.mm (right): https://codereview.chromium.org/1972013003/diff/100001/ios/chrome/browser/ui/... ios/chrome/browser/ui/context_menu/context_menu_coordinator_unittest.mm:34: [[ContextMenuCoordinator alloc] initWithContextMenuParams:params]); initWithViewController: ? https://codereview.chromium.org/1972013003/diff/100001/ios/chrome/browser/ui/... ios/chrome/browser/ui/context_menu/context_menu_coordinator_unittest.mm:38: action:^{ On 2016/05/19 15:57:30, michaeldo wrote: > On 2016/05/19 15:39:28, Jackie Quinn wrote: > > Why one nil action and one ^{} action? Also, would it be possible to test > > functionality of the context menu (that item titles and actions show up > > correctly in the context menu)? > > The nil vs block (even though it's empty) are simply two different cases, so I > thought it would be good to cover both here. If nil was only used, the test may > not cover adding an actual block crashes for some reason. Maybe a stretch, but I > think it's worth having both. > > Testing the actual functionality would be ideal, but I'm not sure it's practical > as that info isn't exposed. However, there are other tests which exercise the > context menu functionality in the app. Now when your class takes view controller you can actually test that UIAlertController has the right actions (everything you need is actually exposed). Other tests (like integration test) indeed test these things, but there is no reason for not testing this class at the unit test level.
https://codereview.chromium.org/1972013003/diff/100001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/context_menu/context_menu_coordinator_unittest.mm (right): https://codereview.chromium.org/1972013003/diff/100001/ios/chrome/browser/ui/... ios/chrome/browser/ui/context_menu/context_menu_coordinator_unittest.mm:34: [[ContextMenuCoordinator alloc] initWithContextMenuParams:params]); On 2016/05/19 19:00:06, Eugene But wrote: > initWithViewController: ? This didn't get updated before. Fixed now with improved unittest validation though! https://codereview.chromium.org/1972013003/diff/100001/ios/chrome/browser/ui/... ios/chrome/browser/ui/context_menu/context_menu_coordinator_unittest.mm:38: action:^{ On 2016/05/19 19:00:06, Eugene But wrote: > On 2016/05/19 15:57:30, michaeldo wrote: > > On 2016/05/19 15:39:28, Jackie Quinn wrote: > > > Why one nil action and one ^{} action? Also, would it be possible to test > > > functionality of the context menu (that item titles and actions show up > > > correctly in the context menu)? > > > > The nil vs block (even though it's empty) are simply two different cases, so I > > thought it would be good to cover both here. If nil was only used, the test > may > > not cover adding an actual block crashes for some reason. Maybe a stretch, but > I > > think it's worth having both. > > > > Testing the actual functionality would be ideal, but I'm not sure it's > practical > > as that info isn't exposed. However, there are other tests which exercise the > > context menu functionality in the app. > Now when your class takes view controller you can actually test that > UIAlertController has the right actions (everything you need is actually > exposed). > Other tests (like integration test) indeed test these things, but there is no > reason for not testing this class at the unit test level. You're definitely right, I've improved the unittest to validate the displayed UIAlertActions.
On 2016/05/20 16:34:24, michaeldo wrote: > https://codereview.chromium.org/1972013003/diff/100001/ios/chrome/browser/ui/... > File ios/chrome/browser/ui/context_menu/context_menu_coordinator_unittest.mm > (right): > > https://codereview.chromium.org/1972013003/diff/100001/ios/chrome/browser/ui/... > ios/chrome/browser/ui/context_menu/context_menu_coordinator_unittest.mm:34: > [[ContextMenuCoordinator alloc] initWithContextMenuParams:params]); > On 2016/05/19 19:00:06, Eugene But wrote: > > initWithViewController: ? > > This didn't get updated before. Fixed now with improved unittest validation > though! > > https://codereview.chromium.org/1972013003/diff/100001/ios/chrome/browser/ui/... > ios/chrome/browser/ui/context_menu/context_menu_coordinator_unittest.mm:38: > action:^{ > On 2016/05/19 19:00:06, Eugene But wrote: > > On 2016/05/19 15:57:30, michaeldo wrote: > > > On 2016/05/19 15:39:28, Jackie Quinn wrote: > > > > Why one nil action and one ^{} action? Also, would it be possible to test > > > > functionality of the context menu (that item titles and actions show up > > > > correctly in the context menu)? > > > > > > The nil vs block (even though it's empty) are simply two different cases, so > I > > > thought it would be good to cover both here. If nil was only used, the test > > may > > > not cover adding an actual block crashes for some reason. Maybe a stretch, > but > > I > > > think it's worth having both. > > > > > > Testing the actual functionality would be ideal, but I'm not sure it's > > practical > > > as that info isn't exposed. However, there are other tests which exercise > the > > > context menu functionality in the app. > > Now when your class takes view controller you can actually test that > > UIAlertController has the right actions (everything you need is actually > > exposed). > > Other tests (like integration test) indeed test these things, but there is no > > reason for not testing this class at the unit test level. > > You're definitely right, I've improved the unittest to validate the displayed > UIAlertActions. PTAL at last patchset. Improved unittest!
https://codereview.chromium.org/1972013003/diff/120001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/context_menu/context_menu_coordinator_unittest.mm (right): https://codereview.chromium.org/1972013003/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/context_menu/context_menu_coordinator_unittest.mm:1: // Copyright 2012 The Chromium Authors. All rights reserved. 2016 https://codereview.chromium.org/1972013003/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/context_menu/context_menu_coordinator_unittest.mm:9: #include "base/mac/scoped_nsobject.h" s/include/import scoped_nsobject.h has Objective-C code inside: #import <Foundation/NSObject.h> @class NSAutoreleasePool; https://codereview.chromium.org/1972013003/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/context_menu/context_menu_coordinator_unittest.mm:21: view_controller_.reset([UIViewController new]); s/new/alloc init https://google.github.io/styleguide/objcguide.xml#Avoid_+new https://codereview.chromium.org/1972013003/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/context_menu/context_menu_coordinator_unittest.mm:31: // Tests the context menu reports as visible after presenting. This test does more than just visibility check. Could you please split this method into multiple tests: 1.) Visibility 2.) Dismissal 3.) Actions are correct 4.) Cancel button is present 5.) web::ContextMenuParams are not ignored https://codereview.chromium.org/1972013003/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/context_menu/context_menu_coordinator_unittest.mm:40: NSArray* menuTitles = @[ @"foo", @"bar" ]; s/menuTitles/menu_titles TEST_F expands to C++ method and all code inside should follow C++ style guide. Please fix the style for other local variables. https://codereview.chromium.org/1972013003/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/context_menu/context_menu_coordinator_unittest.mm:44: }]; NIT: Can your test verify that action block is correct? https://codereview.chromium.org/1972013003/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/context_menu/context_menu_coordinator_unittest.mm:52: [menu_coordinator_ presentedAlertController]; Test should test only public API. Please remove this from the test and remove exposing private method. See this: https://goto.google.com/test-only-public-api https://codereview.chromium.org/1972013003/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/context_menu/context_menu_coordinator_unittest.mm:53: EXPECT_EQ(alertController, [view_controller_ presentedViewController]); Expected should be an the left and actual on the right. Please flip params. https://codereview.chromium.org/1972013003/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/context_menu/context_menu_coordinator_unittest.mm:68: GTEST_FAIL(); I think you want to use ADD_FAILURE(), otherwise the whole suite will fail and that's not what we want. https://codereview.chromium.org/1972013003/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/context_menu/context_menu_coordinator_unittest.mm:73: EXPECT_EQ([remainingTitles count], (NSUInteger)0); Please flip params https://codereview.chromium.org/1972013003/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/context_menu/context_menu_coordinator_unittest.mm:73: EXPECT_EQ([remainingTitles count], (NSUInteger)0); Please no C-Style casting. Would this work? s/(NSUInteger)0/0U or 0LU
https://codereview.chromium.org/1972013003/diff/120001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/context_menu/context_menu_coordinator_unittest.mm (right): https://codereview.chromium.org/1972013003/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/context_menu/context_menu_coordinator_unittest.mm:1: // Copyright 2012 The Chromium Authors. All rights reserved. On 2016/05/20 17:49:35, Eugene But wrote: > 2016 Done. https://codereview.chromium.org/1972013003/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/context_menu/context_menu_coordinator_unittest.mm:9: #include "base/mac/scoped_nsobject.h" On 2016/05/20 17:49:35, Eugene But wrote: > s/include/import > > scoped_nsobject.h has Objective-C code inside: > > #import <Foundation/NSObject.h> > @class NSAutoreleasePool; Copied from ContextMenuCoordinator, fixed there too! https://codereview.chromium.org/1972013003/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/context_menu/context_menu_coordinator_unittest.mm:21: view_controller_.reset([UIViewController new]); On 2016/05/20 17:49:35, Eugene But wrote: > s/new/alloc init > > https://google.github.io/styleguide/objcguide.xml#Avoid_+new Done. https://codereview.chromium.org/1972013003/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/context_menu/context_menu_coordinator_unittest.mm:31: // Tests the context menu reports as visible after presenting. On 2016/05/20 17:49:35, Eugene But wrote: > This test does more than just visibility check. Could you please split this > method into multiple tests: > 1.) Visibility > 2.) Dismissal > 3.) Actions are correct > 4.) Cancel button is present > 5.) web::ContextMenuParams are not ignored Done. https://codereview.chromium.org/1972013003/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/context_menu/context_menu_coordinator_unittest.mm:40: NSArray* menuTitles = @[ @"foo", @"bar" ]; On 2016/05/20 17:49:35, Eugene But wrote: > s/menuTitles/menu_titles > > TEST_F expands to C++ method and all code inside should follow C++ style guide. > > Please fix the style for other local variables. Done. https://codereview.chromium.org/1972013003/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/context_menu/context_menu_coordinator_unittest.mm:44: }]; On 2016/05/20 17:49:35, Eugene But wrote: > NIT: Can your test verify that action block is correct? I can not test that the block is correct, it is not exposed on the UIAlertAction. https://codereview.chromium.org/1972013003/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/context_menu/context_menu_coordinator_unittest.mm:52: [menu_coordinator_ presentedAlertController]; On 2016/05/20 17:49:35, Eugene But wrote: > Test should test only public API. Please remove this from the test and remove > exposing private method. > > See this: https://goto.google.com/test-only-public-api Done. Please check the change I made. I am now getting the presentedViewController, checking class type, and casting instead of exposing testing API. https://codereview.chromium.org/1972013003/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/context_menu/context_menu_coordinator_unittest.mm:53: EXPECT_EQ(alertController, [view_controller_ presentedViewController]); On 2016/05/20 17:49:35, Eugene But wrote: > Expected should be an the left and actual on the right. Please flip params. Done. https://codereview.chromium.org/1972013003/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/context_menu/context_menu_coordinator_unittest.mm:68: GTEST_FAIL(); On 2016/05/20 17:49:35, Eugene But wrote: > I think you want to use ADD_FAILURE(), otherwise the whole suite will fail and > that's not what we want. Thanks for the tip, this isn't needed anymore once I split up the tests. https://codereview.chromium.org/1972013003/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/context_menu/context_menu_coordinator_unittest.mm:73: EXPECT_EQ([remainingTitles count], (NSUInteger)0); On 2016/05/20 17:49:35, Eugene But wrote: > Please no C-Style casting. Would this work? s/(NSUInteger)0/0U or 0LU Done.
lgtm https://codereview.chromium.org/1972013003/diff/140001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/context_menu/context_menu_coordinator.mm (right): https://codereview.chromium.org/1972013003/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/context_menu/context_menu_coordinator.mm:9: #import "base/strings/sys_string_conversions.h" s/import/include because @class NSString; is behind ifdefs https://codereview.chromium.org/1972013003/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/context_menu/context_menu_coordinator.mm:43: #pragma mark - Object Lifecycle Optional NIT: Add line-breaks after pragma marks https://codereview.chromium.org/1972013003/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/context_menu/context_menu_coordinator.mm:52: void (^actionHandler)(UIAlertAction*) = ^(UIAlertAction*) { Optional NIT: Drop |actionHandler| variable and inline the block. https://codereview.chromium.org/1972013003/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/context_menu/context_menu_coordinator.mm:78: [self setVisible:NO]; self.visible = NO; for consistency https://codereview.chromium.org/1972013003/diff/140001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/context_menu/context_menu_coordinator_unittest.mm (right): https://codereview.chromium.org/1972013003/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/context_menu/context_menu_coordinator_unittest.mm:78: isKindOfClass:UIAlertController.class]); [UIAlertController class] https://codereview.chromium.org/1972013003/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/context_menu/context_menu_coordinator_unittest.mm:79: UIAlertController* alert_controller = static_cast<UIAlertController*>( ObjCCastStrict https://codereview.chromium.org/1972013003/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/context_menu/context_menu_coordinator_unittest.mm:84: [alert_controller.actions Why not a for loop? https://codereview.chromium.org/1972013003/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/context_menu/context_menu_coordinator_unittest.mm:114: [[alert_controller.actions firstObject] style]); NIT: alert_controller.actions.firstObject for consistency https://codereview.chromium.org/1972013003/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/context_menu/context_menu_coordinator_unittest.mm:117: // Test that the ContextMenuParams are used to display context menu. s/Test/Tests https://codereview.chromium.org/1972013003/diff/140001/ios/web/web_state/web_... File ios/web/web_state/web_state_delegate_bridge_unittest.mm (right): https://codereview.chromium.org/1972013003/diff/140001/ios/web/web_state/web_... ios/web/web_state/web_state_delegate_bridge_unittest.mm:56: contextMenuParams.view.reset([UIView new]); NIT: since you touching this could you please s/new/alloc] init] But ideally this should be a separate CL :)
https://codereview.chromium.org/1972013003/diff/140001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/context_menu/context_menu_coordinator.h (right): https://codereview.chromium.org/1972013003/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/context_menu/context_menu_coordinator.h:28: // Adds an item at the end of the menu. What happens if this is called after -start is called? After -stop is called? https://codereview.chromium.org/1972013003/diff/140001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/context_menu/context_menu_coordinator_unittest.mm (right): https://codereview.chromium.org/1972013003/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/context_menu/context_menu_coordinator_unittest.mm:20: [window_ makeKeyAndVisible]; Does the destructor need to restore the previous key window, or does destroying a key window revert that to the app's main window? https://codereview.chromium.org/1972013003/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/context_menu/context_menu_coordinator_unittest.mm:44: // Tests the context menu dismissal. Also test that destroying the coordinator without calling -stop does the same thing.
https://codereview.chromium.org/1972013003/diff/140001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/context_menu/context_menu_coordinator.h (right): https://codereview.chromium.org/1972013003/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/context_menu/context_menu_coordinator.h:28: // Adds an item at the end of the menu. On 2016/05/21 09:39:58, marq wrote: > What happens if this is called after -start is called? After -stop is called? I've updated the comment and code to early return if |addItemWith...| is called after start. https://codereview.chromium.org/1972013003/diff/140001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/context_menu/context_menu_coordinator.mm (right): https://codereview.chromium.org/1972013003/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/context_menu/context_menu_coordinator.mm:9: #import "base/strings/sys_string_conversions.h" On 2016/05/20 21:26:45, Eugene But wrote: > s/import/include because @class NSString; is behind ifdefs Done. https://codereview.chromium.org/1972013003/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/context_menu/context_menu_coordinator.mm:43: #pragma mark - Object Lifecycle On 2016/05/20 21:26:45, Eugene But wrote: > Optional NIT: Add line-breaks after pragma marks Done. https://codereview.chromium.org/1972013003/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/context_menu/context_menu_coordinator.mm:52: void (^actionHandler)(UIAlertAction*) = ^(UIAlertAction*) { On 2016/05/20 21:26:45, Eugene But wrote: > Optional NIT: Drop |actionHandler| variable and inline the block. Done. https://codereview.chromium.org/1972013003/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/context_menu/context_menu_coordinator.mm:78: [self setVisible:NO]; On 2016/05/20 21:26:45, Eugene But wrote: > self.visible = NO; for consistency Done. https://codereview.chromium.org/1972013003/diff/140001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/context_menu/context_menu_coordinator_unittest.mm (right): https://codereview.chromium.org/1972013003/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/context_menu/context_menu_coordinator_unittest.mm:20: [window_ makeKeyAndVisible]; On 2016/05/21 09:39:58, marq wrote: > Does the destructor need to restore the previous key window, or does destroying > a key window revert that to the app's main window? It looks like the last window which was created and set as the key window remains the key window. Note however, the pattern here seems to be what the other unittests do (context_menu_controller_unittest for example). The current pattern is that the test sets up the window they will use. We could likely use the default window instead, but it should likely be a separate effort to gain consistency across the tests. https://codereview.chromium.org/1972013003/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/context_menu/context_menu_coordinator_unittest.mm:44: // Tests the context menu dismissal. On 2016/05/21 09:39:58, marq wrote: > Also test that destroying the coordinator without calling -stop does the same > thing. Done. https://codereview.chromium.org/1972013003/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/context_menu/context_menu_coordinator_unittest.mm:78: isKindOfClass:UIAlertController.class]); On 2016/05/20 21:26:46, Eugene But wrote: > [UIAlertController class] Done. https://codereview.chromium.org/1972013003/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/context_menu/context_menu_coordinator_unittest.mm:79: UIAlertController* alert_controller = static_cast<UIAlertController*>( On 2016/05/20 21:26:46, Eugene But wrote: > ObjCCastStrict Done. https://codereview.chromium.org/1972013003/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/context_menu/context_menu_coordinator_unittest.mm:84: [alert_controller.actions On 2016/05/20 21:26:45, Eugene But wrote: > Why not a for loop? great question! switched to simpler for loop. https://codereview.chromium.org/1972013003/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/context_menu/context_menu_coordinator_unittest.mm:114: [[alert_controller.actions firstObject] style]); On 2016/05/20 21:26:46, Eugene But wrote: > NIT: alert_controller.actions.firstObject for consistency Done. https://codereview.chromium.org/1972013003/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/context_menu/context_menu_coordinator_unittest.mm:117: // Test that the ContextMenuParams are used to display context menu. On 2016/05/20 21:26:46, Eugene But wrote: > s/Test/Tests Done. https://codereview.chromium.org/1972013003/diff/140001/ios/web/web_state/web_... File ios/web/web_state/web_state_delegate_bridge_unittest.mm (right): https://codereview.chromium.org/1972013003/diff/140001/ios/web/web_state/web_... ios/web/web_state/web_state_delegate_bridge_unittest.mm:56: contextMenuParams.view.reset([UIView new]); On 2016/05/20 21:26:46, Eugene But wrote: > NIT: since you touching this could you please s/new/alloc] init] > > But ideally this should be a separate CL :) I'll fix and make a seperate cl :)
LGTM with one more comment nit. https://codereview.chromium.org/1972013003/diff/200001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/context_menu/context_menu_coordinator.h (right): https://codereview.chromium.org/1972013003/diff/200001/ios/chrome/browser/ui/... ios/chrome/browser/ui/context_menu/context_menu_coordinator.h:33: // Dismisses displayed context menu. Can you clarify more the expectations around the object lifecycle in relation to -stop and -start? Is it expected that -start and -stop can be called repeatedly on the same object? Or should an object that receives -stop expect to be deallocated soon afterwards?
BTW, CL description still references *wrangler*, so you may want to update it :)
https://codereview.chromium.org/1972013003/diff/200001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/context_menu/context_menu_coordinator.h (right): https://codereview.chromium.org/1972013003/diff/200001/ios/chrome/browser/ui/... ios/chrome/browser/ui/context_menu/context_menu_coordinator.h:33: // Dismisses displayed context menu. On 2016/05/24 07:01:45, marq wrote: > Can you clarify more the expectations around the object lifecycle in relation to > -stop and -start? Is it expected that -start and -stop can be called repeatedly > on the same object? Or should an object that receives -stop expect to be > deallocated soon afterwards? I would expect this object to only used with one start/stop cycle and then deallocated. I'll clarify this a bit in the comments.
Description was changed from ========== Add ContextMenuWrangler. ContextMenuWrangler replaces ContextMenuController with an updated interface that accepts ContextMenuParams instead of the intermediate ContextMenuHolder object. BUG=none ========== to ========== ContextMenuCoordinator replaces ContextMenuController with an updated interface that accepts ContextMenuParams instead of the intermediate ContextMenuHolder object. BUG=614389 ==========
The CQ bit was checked by michaeldo@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jyquinn@chromium.org, eugenebut@chromium.org Link to the patchset: https://codereview.chromium.org/1972013003/#ps200001 (title: "Revert web_state_delegate_bridge_unittest.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1972013003/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1972013003/200001
The CQ bit was unchecked by michaeldo@chromium.org
The CQ bit was checked by michaeldo@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eugenebut@chromium.org, marq@chromium.org, jyquinn@chromium.org Link to the patchset: https://codereview.chromium.org/1972013003/#ps220001 (title: "Rebase. Update comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1972013003/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1972013003/220001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-simulator on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by michaeldo@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eugenebut@chromium.org, marq@chromium.org, jyquinn@chromium.org Link to the patchset: https://codereview.chromium.org/1972013003/#ps240001 (title: "Fix unittest when running on iPhone.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1972013003/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1972013003/240001
Message was sent while issue was closed.
Description was changed from ========== ContextMenuCoordinator replaces ContextMenuController with an updated interface that accepts ContextMenuParams instead of the intermediate ContextMenuHolder object. BUG=614389 ========== to ========== ContextMenuCoordinator replaces ContextMenuController with an updated interface that accepts ContextMenuParams instead of the intermediate ContextMenuHolder object. BUG=614389 ==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== ContextMenuCoordinator replaces ContextMenuController with an updated interface that accepts ContextMenuParams instead of the intermediate ContextMenuHolder object. BUG=614389 ========== to ========== ContextMenuCoordinator replaces ContextMenuController with an updated interface that accepts ContextMenuParams instead of the intermediate ContextMenuHolder object. BUG=614389 Committed: https://crrev.com/94187267a370bf4c0950e3e4e68e4f360510e166 Cr-Commit-Position: refs/heads/master@{#395652} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/94187267a370bf4c0950e3e4e68e4f360510e166 Cr-Commit-Position: refs/heads/master@{#395652} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
