Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
151593005:
Implement permission bubble view for Cocoa. (Closed)
|
Created:
6 years, 10 months ago by leng Modified:
6 years, 9 months ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdds classes to implement the permissions bubble on the Mac.
PermissionBubbleCocoa is the Mac implementation of the platform-
independent PermissionBubbleView. It creates the Cocoa implementation
when requested.
PermissionBubbleController is the Cocoa implementation for the
PermissionBubbleCocoa. It is a UIViewController which displays the bubble,
and closes itself.
The PermissionBubbleCocoa object is owned by the BrowserWindowController,
and provided to the current PermissionBubbleManager (tied to WebContents).
The PermissionBubbleManager then uses the implementation of
PermissionBubbleView to display the UI when required.
Also adds MockPermissionBubbleDelegate for testing, and tests for
PermissionBubbleController.
BUG=None
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=254635
Patch Set 1 #Patch Set 2 : added a few comments #
Total comments: 88
Patch Set 3 : addressed feedback #Patch Set 4 : more feedback addressed #
Total comments: 23
Patch Set 5 : moved to cocoa/website_settings. #Patch Set 6 : use ConstrainedWindowButton #Patch Set 7 : Fixed variable name #
Total comments: 18
Patch Set 8 : better button sizing & layout #Patch Set 9 : rebase #Patch Set 10 : post-merge cleanup #Patch Set 11 : rebase #Patch Set 12 : Ensure only one delegate owns the view at a time #
Total comments: 4
Messages
Total messages: 68 (0 generated)
Rachel, Could you take a look before I send this to the owners, please? Thanks! -Susanna
Thank you for providing a comprehensive test set! (Also, you already have a cocoa OWNER on the review :) https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/browser_window_controller.mm (right): https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser_window_controller.mm:1601: PermissionBubbleManager::FromWebContents(contents)->SetView( Is it ok for the PBM to not have its view NULLED out if another manager uses the view? https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/permission_bubble_cocoa.h (right): https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/permission_bubble_cocoa.h:27: bool custommization_mode) OVERRIDE; nit: probably one 'm' too many :) https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/permission_bubble_cocoa.mm (right): https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/permission_bubble_cocoa.mm:43: if (bubbleController_) No need for if. (messages to nil are ignored) https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/permission_bubble_controller.h (right): https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/permission_bubble_controller.h:10: #include "base/memory/scoped_ptr.h" Not used https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/permission_bubble_controller.h:12: #include "chrome/browser/ui/website_settings/permission_bubble_delegate.h" Not needed - just forward declare https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/permission_bubble_controller.h:21: std::vector<base::scoped_nsobject<NSView> > checkboxes_; NSMutableArray, please. https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/permission_bubble_controller.h:33: - (void)showAtAnchor:(NSPoint)anchor Please document class methods. https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/permission_bubble_controller.mm (right): https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/permission_bubble_controller.mm:11: #include "base/strings/utf_string_conversions.h" I don't think you're using these https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/permission_bubble_controller.mm:38: const int kButtonBackgroundColor = 239; Is that shared across platforms? Would it be worth to have a "theme" file? https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/permission_bubble_controller.mm:42: @interface PermissionBubbleController (PrivateAPI) just @interface PermissionBubbleController () https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/permission_bubble_controller.mm:45: - (void)ok; NSButton actions always take a sender parameter, i.e - (void)ok:(id)sender; https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/permission_bubble_controller.mm:48: - (void)allow; nit: Button callbacks except ok/cancel are usually prefixed with "on", i.e. "onAllow:", "onBlock:", etc. https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/permission_bubble_controller.mm:96: base::scoped_nsobject<NSMutableArray> subViews([[NSMutableArray alloc] init]); Curious: Why a separate NSMutableArray, instead of adding the views directly? https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/permission_bubble_controller.mm:120: yOffset += [permissionView frame].size.height; NSHeight([permissionView frame]) https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/permission_bubble_controller.mm:121: maxContentsWidth = fmax(maxContentsWidth, Please use std::max Alternatively, you can simply create all the views, and then loop over them: NSRect contentFrame = NSZeroRect for (NSView* view in subviews) contentFrame = NSUnionRect(contentFrame, [view frame]); Simplifies a lot of bookkeeping https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/permission_bubble_controller.mm:130: (kButtonHeight - [customizeButton frame].size.height) * 0.5f; You might want to ceil/floor this so the button is not on a half-pixel https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/permission_bubble_controller.mm:136: CGFloat windowWidth = fmax(windowFrame.size.width, NSWidth(windowFrame) - we like to pretend we don't know the structure of an NSRect :) You might want to do a search/replace through the file :) https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/permission_bubble_controller.mm:154: kHorizontalPadding * 0.5f; I assume kHorizontalPadding/2 is the gap you want between buttons? Mind calling it out as kButtonGap? https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/permission_bubble_controller.mm:170: windowFrame.size.height = yOffset + kVerticalPadding; Please use -frameRectForContentRect to convert the rect into a frame that allows for potential window decorations. https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/permission_bubble_controller.mm:187: // TODO(leng): Make the appropriate call when it's working. When what is working? https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/permission_bubble_controller.mm:191: label += request->GetMessageTextFragment(); Can that if/else collapse, or is that related to the TODO? https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/permission_bubble_controller.mm:210: [titleView setFont:[NSFont systemFontOfSize:15.0]]; Might want to call this out as constant https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/permission_bubble_controller.mm:223: [checkbox setState:checked ? NSOnState : NSOffState]; nit: Parens around ternary https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/permission_bubble_controller.mm:237: attributes:@{ NSForegroundColorAttributeName : linkColor }]]; Is the customizeButton supposed to be an actual link? There is HyperlinkTextView for that. https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/permission_bubble_controller.mm:246: NSRect frame = { {0, 0}, {kButtonWidth, kButtonHeight} }; NSRect frame = NSMakeRect(0, 0, kButtonWidth, kButtonHeight); https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/permission_bubble_controller.mm:264: if (delegate_) I'd prefer a DCHECK delegate_ - I don't think this could ever be NULL? https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/permission_bubble_controller.mm:288: if (iter != checkboxes_.end()) { Again, probably DCHECK. Can never happen https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/permission_bubble_controller.mm:289: NSButton *checkbox = (NSButton*)(*iter); base::ObjCCast or base::ObjCCastStrict when casting NS* values. The latter DCHECKs the cast worked, which is probably preferrable here. https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/permission_bubble_controller.mm:295: @end // implementation BookmarkBubbleController(ExposedForUnitTesting) ExposedForUnitTesting? https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/permission_bubble_controller_unittest.mm (right): https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/permission_bubble_controller_unittest.mm:51: NSButton* button = (NSButton*)child; base::ObjCCast https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/permission_bubble_controller_unittest.mm:69: // match is required. For a single request, expect an exact match. You can probably always test hasSuffix and skip the whole exactMatch thing https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/permission_bubble_controller_unittest.mm:101: [controller_ showAtAnchor:NSMakePoint(0, 0) NSZeroPoint https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/permission_bubble_controller_unittest.mm:108: NSTextField* textField = FindTextFieldWithString(bubble, permission_a, true); Since the fixture has controller_, and you never search outside of bubble, I'd move extraction of the bubble view into the Find* routines - less parameters. Together with eliminating |exact|, this can now be a single line - EXPECT_TRUE(FindTextFieldWithString(permission_a)); https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/permission_bubble_controller_unittest.mm:127: delegate_a.SetText(permission_a); Is it possible to take the text in the ctor? And do the UTF conversion there too? I.e. MockPermissionBubbleDelegate a("Permission A"); ? 9 lines turn into 3 :) https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/permission_bubble_controller_unittest.mm:207: TEST_F(PermissionBubbleControllerTest, OK) { I'm not sure you need these tests - all of these functions just call a member on the delegate. Not sure that warrants a test Sorry for misleading you with my answer, I thought there'd be more logic in those functions. https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/permission_bubble_controller_unittest.mm:276: // the NSButton, expect the same toggle value as was set originally. You can simulate a button press by [button performClick:nil]; That should flip the checkboxes, too. (And if you'd like to keep testing the delegate calls from allow etc, that might be a better way to test - a bit more code is covered) https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/websit... File chrome/browser/ui/website_settings/mock_permission_bubble_delegate.h (right): https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/websit... chrome/browser/ui/website_settings/mock_permission_bubble_delegate.h:12: class MockPermissionBubbleDelegate : public PermissionBubbleDelegate { I'd probably just roll this into the unittest, unless you expect it to be reused elsewhere?
https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/browser_window_controller.mm (right): https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser_window_controller.mm:1601: PermissionBubbleManager::FromWebContents(contents)->SetView( On 2014/01/31 22:31:51, groby wrote: > Is it ok for the PBM to not have its view NULLED out if another manager uses the > view? It would be safer to NULL out view for the previous manager. How would I find the previous contents, to retrieve the previous manager? Or should I plumb something from the other end? i.e. have the view tell its outgoing delegate(the manager) that it's being pre-empted when the view is assigned a new delegate (manager)? https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/permission_bubble_cocoa.h (right): https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/permission_bubble_cocoa.h:27: bool custommization_mode) OVERRIDE; On 2014/01/31 22:31:51, groby wrote: > nit: probably one 'm' too many :) Done. https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/permission_bubble_cocoa.mm (right): https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/permission_bubble_cocoa.mm:43: if (bubbleController_) On 2014/01/31 22:31:51, groby wrote: > No need for if. (messages to nil are ignored) Too many years in C++... :) Done. https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/permission_bubble_controller.h (right): https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/permission_bubble_controller.h:10: #include "base/memory/scoped_ptr.h" On 2014/01/31 22:31:51, groby wrote: > Not used Done. https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/permission_bubble_controller.h:12: #include "chrome/browser/ui/website_settings/permission_bubble_delegate.h" On 2014/01/31 22:31:51, groby wrote: > Not needed - just forward declare Maybe I just don't understand how to forward-declare a class-specific class? I tried: class PermissionBubbleView::Delegate; but it didn't work. https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/permission_bubble_controller.h:21: std::vector<base::scoped_nsobject<NSView> > checkboxes_; On 2014/01/31 22:31:51, groby wrote: > NSMutableArray, please. Done. https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/permission_bubble_controller.h:33: - (void)showAtAnchor:(NSPoint)anchor On 2014/01/31 22:31:51, groby wrote: > Please document class methods. Done. https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/permission_bubble_controller.mm (right): https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/permission_bubble_controller.mm:11: #include "base/strings/utf_string_conversions.h" On 2014/01/31 22:31:51, groby wrote: > I don't think you're using these Only the sys_strings one. Done. https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/permission_bubble_controller.mm:38: const int kButtonBackgroundColor = 239; On 2014/01/31 22:31:51, groby wrote: > Is that shared across platforms? Would it be worth to have a "theme" file? I don't know. I took the color from the mocks, so I suspect so. I'm not familiar with "theme" files - what could I look at for an example? https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/permission_bubble_controller.mm:42: @interface PermissionBubbleController (PrivateAPI) On 2014/01/31 22:31:51, groby wrote: > just > > @interface PermissionBubbleController () Done. https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/permission_bubble_controller.mm:45: - (void)ok; On 2014/01/31 22:31:51, groby wrote: > NSButton actions always take a sender parameter, i.e > - (void)ok:(id)sender; Done. https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/permission_bubble_controller.mm:48: - (void)allow; On 2014/01/31 22:31:51, groby wrote: > nit: Button callbacks except ok/cancel are usually prefixed with "on", i.e. > "onAllow:", "onBlock:", etc. Done. https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/permission_bubble_controller.mm:96: base::scoped_nsobject<NSMutableArray> subViews([[NSMutableArray alloc] init]); On 2014/01/31 22:31:51, groby wrote: > Curious: Why a separate NSMutableArray, instead of adding the views directly? I was copying the behavior from another class. Since I can't remember which class that was, I've changed it. Because of this change, I added a DCHECK to ensure that this function is never called when there are already subviews. https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/permission_bubble_controller.mm:120: yOffset += [permissionView frame].size.height; On 2014/01/31 22:31:51, groby wrote: > NSHeight([permissionView frame]) > Done. https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/permission_bubble_controller.mm:121: maxContentsWidth = fmax(maxContentsWidth, On 2014/01/31 22:31:51, groby wrote: > Please use std::max > > Alternatively, you can simply create all the views, and then loop over them: > NSRect contentFrame = NSZeroRect > for (NSView* view in subviews) > contentFrame = NSUnionRect(contentFrame, [view frame]); > > Simplifies a lot of bookkeeping I like it - thanks! Done. https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/permission_bubble_controller.mm:130: (kButtonHeight - [customizeButton frame].size.height) * 0.5f; On 2014/01/31 22:31:51, groby wrote: > You might want to ceil/floor this so the button is not on a half-pixel Done. https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/permission_bubble_controller.mm:136: CGFloat windowWidth = fmax(windowFrame.size.width, On 2014/01/31 22:31:51, groby wrote: > NSWidth(windowFrame) - we like to pretend we don't know the structure of an > NSRect :) > > You might want to do a search/replace through the file :) I didn't know that the structure of NSRect was so hush-hush. :) Done. https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/permission_bubble_controller.mm:154: kHorizontalPadding * 0.5f; On 2014/01/31 22:31:51, groby wrote: > I assume kHorizontalPadding/2 is the gap you want between buttons? Mind calling > it out as kButtonGap? Done. https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/permission_bubble_controller.mm:170: windowFrame.size.height = yOffset + kVerticalPadding; On 2014/01/31 22:31:51, groby wrote: > Please use -frameRectForContentRect to convert the rect into a frame that allows > for potential window decorations. Good catch. I've changed the name of the variable to make the distinction clearer (for myself, mostly). https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/permission_bubble_controller.mm:187: // TODO(leng): Make the appropriate call when it's working. On 2014/01/31 22:31:51, groby wrote: > When what is working? When the correct text-query function is working. I added a comment - any clearer? https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/permission_bubble_controller.mm:191: label += request->GetMessageTextFragment(); On 2014/01/31 22:31:51, groby wrote: > Can that if/else collapse, or is that related to the TODO? Related to the TODO. https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/permission_bubble_controller.mm:210: [titleView setFont:[NSFont systemFontOfSize:15.0]]; On 2014/01/31 22:31:51, groby wrote: > Might want to call this out as constant Done. https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/permission_bubble_controller.mm:223: [checkbox setState:checked ? NSOnState : NSOffState]; On 2014/01/31 22:31:51, groby wrote: > nit: Parens around ternary Done. https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/permission_bubble_controller.mm:237: attributes:@{ NSForegroundColorAttributeName : linkColor }]]; On 2014/01/31 22:31:51, groby wrote: > Is the customizeButton supposed to be an actual link? There is HyperlinkTextView > for that. I tried that, but not only is it significantly more code, I don't know how to remove the underline from a HyperlinkTextView. The mocks don't have the text underlined. https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/permission_bubble_controller.mm:246: NSRect frame = { {0, 0}, {kButtonWidth, kButtonHeight} }; On 2014/01/31 22:31:51, groby wrote: > NSRect frame = NSMakeRect(0, 0, kButtonWidth, kButtonHeight); Done. https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/permission_bubble_controller.mm:264: if (delegate_) On 2014/01/31 22:31:51, groby wrote: > I'd prefer a DCHECK delegate_ - I don't think this could ever be NULL? It was when I was first testing - I have an outstanding CL to fix that. Changed to DCHECK. https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/permission_bubble_controller.mm:288: if (iter != checkboxes_.end()) { On 2014/01/31 22:31:51, groby wrote: > Again, probably DCHECK. Can never happen Done. https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/permission_bubble_controller.mm:289: NSButton *checkbox = (NSButton*)(*iter); On 2014/01/31 22:31:51, groby wrote: > base::ObjCCast or base::ObjCCastStrict when casting NS* values. > > The latter DCHECKs the cast worked, which is probably preferrable here. I had no idea about ObjCCast and ObjCCastStrict. Nice. Done. https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/permission_bubble_controller.mm:295: @end // implementation BookmarkBubbleController(ExposedForUnitTesting) On 2014/01/31 22:31:51, groby wrote: > ExposedForUnitTesting? BookmarkBubbleController? Now you know how I started this file... :) https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/permission_bubble_controller_unittest.mm (right): https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/permission_bubble_controller_unittest.mm:51: NSButton* button = (NSButton*)child; On 2014/01/31 22:31:51, groby wrote: > base::ObjCCast Done. https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/permission_bubble_controller_unittest.mm:69: // match is required. For a single request, expect an exact match. On 2014/01/31 22:31:51, groby wrote: > You can probably always test hasSuffix and skip the whole exactMatch thing You're right. Done. https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/permission_bubble_controller_unittest.mm:101: [controller_ showAtAnchor:NSMakePoint(0, 0) On 2014/01/31 22:31:51, groby wrote: > NSZeroPoint Done. https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/permission_bubble_controller_unittest.mm:108: NSTextField* textField = FindTextFieldWithString(bubble, permission_a, true); On 2014/01/31 22:31:51, groby wrote: > Since the fixture has controller_, and you never search outside of bubble, I'd > move extraction of the bubble view into the Find* routines - less parameters. > > Together with eliminating |exact|, this can now be a single line - > EXPECT_TRUE(FindTextFieldWithString(permission_a)); Done. It's much simpler now, thanks! https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/permission_bubble_controller_unittest.mm:127: delegate_a.SetText(permission_a); On 2014/01/31 22:31:51, groby wrote: > Is it possible to take the text in the ctor? And do the UTF conversion there > too? > I.e. MockPermissionBubbleDelegate a("Permission A"); ? > > 9 lines turn into 3 :) Done. Fewer lines of code is better, especially in tests! https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/permission_bubble_controller_unittest.mm:207: TEST_F(PermissionBubbleControllerTest, OK) { On 2014/01/31 22:31:51, groby wrote: > I'm not sure you need these tests - all of these functions just call a member on > the delegate. Not sure that warrants a test > > Sorry for misleading you with my answer, I thought there'd be more logic in > those functions. I changed them all to push the button, so they're testing that the UI has been hooked up properly. I prefer to leave them, in case something gets disconnected somehow. https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/permission_bubble_controller_unittest.mm:276: // the NSButton, expect the same toggle value as was set originally. On 2014/01/31 22:31:51, groby wrote: > You can simulate a button press by [button performClick:nil]; > > That should flip the checkboxes, too. (And if you'd like to keep testing the > delegate calls from allow etc, that might be a better way to test - a bit more > code is covered) Done - Thanks for the suggestion! https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/websit... File chrome/browser/ui/website_settings/mock_permission_bubble_delegate.h (right): https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/websit... chrome/browser/ui/website_settings/mock_permission_bubble_delegate.h:12: class MockPermissionBubbleDelegate : public PermissionBubbleDelegate { On 2014/01/31 22:31:51, groby wrote: > I'd probably just roll this into the unittest, unless you expect it to be reused > elsewhere? I was hoping it could be used for the views version of the UI. But I'm happy to put in the test file, if you think that's unlikely.
https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/websit... File chrome/browser/ui/website_settings/mock_permission_bubble_delegate.h (right): https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/websit... chrome/browser/ui/website_settings/mock_permission_bubble_delegate.h:12: class MockPermissionBubbleDelegate : public PermissionBubbleDelegate { On 2014/02/03 22:38:04, leng wrote: > On 2014/01/31 22:31:51, groby wrote: > > I'd probably just roll this into the unittest, unless you expect it to be > reused > > elsewhere? > > I was hoping it could be used for the views version of the UI. But I'm happy to > put in the test file, if you think that's unlikely. Actually, a similar mock already exists in permission_bubble_manager_unittest.cc. After I submit this CL, I'll change that test to use this mock, adjusting the mock to work for both test suites.
Sorry, a few more additions :( Mostly nits, but the ConstrainedWindowButton question is slightly bigger. https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/browser_window_controller.mm (right): https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser_window_controller.mm:1601: PermissionBubbleManager::FromWebContents(contents)->SetView( On 2014/02/03 22:38:04, leng wrote: > On 2014/01/31 22:31:51, groby wrote: > > Is it ok for the PBM to not have its view NULLED out if another manager uses > the > > view? > > It would be safer to NULL out view for the previous manager. How would I find > the previous contents, to retrieve the previous manager? Or should I plumb > something from the other end? i.e. have the view tell its outgoing delegate(the > manager) that it's being pre-empted when the view is assigned a new delegate > (manager)? Ugh. I missed that we only get the new contents. On Views/GTK, we got both old and new. pkasting actually left a bug in browser_window_cocoa.mm wondering _why_ we handle things differently.[1] Maybe worth following up w/ Peter if there's already a bug on it, but not in scope for this CL. If there is no bug, I'd file one, though. I'm uncomfortable with stuffing this pointer into every manager in existence. (IMHO, we shouldn't have a shared view, but that's another pig to skin...) https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/permission_bubble_controller.h (right): https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/permission_bubble_controller.h:12: #include "chrome/browser/ui/website_settings/permission_bubble_delegate.h" On 2014/02/03 22:38:04, leng wrote: > On 2014/01/31 22:31:51, groby wrote: > > Not needed - just forward declare > > Maybe I just don't understand how to forward-declare a class-specific class? > I tried: > class PermissionBubbleView::Delegate; > but it didn't work. Ah - _that_ delegate is in permission_bubble_view.h, which still needs to be included. (No fwd dcl for nested classes...) But permission_bubble_delegate.h is for the PermissionBubbleDelegate, which can be forward declared. (Not that those two names are confusing, or anything :) https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/permission_bubble_controller.mm (right): https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/permission_bubble_controller.mm:38: const int kButtonBackgroundColor = 239; On 2014/02/03 22:38:04, leng wrote: > On 2014/01/31 22:31:51, groby wrote: > > Is that shared across platforms? Would it be worth to have a "theme" file? > > I don't know. I took the color from the mocks, so I suspect so. I'm not > familiar with "theme" files - what could I look at for an example? "theme" file is fancy speak for a shared header with those constants :) https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/permission_bubble_controller.mm:187: // TODO(leng): Make the appropriate call when it's working. On 2014/02/03 22:38:04, leng wrote: > On 2014/01/31 22:31:51, groby wrote: > > When what is working? > > When the correct text-query function is working. I added a comment - any > clearer? Much - thank you! https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/permission_bubble_controller.mm:187: // TODO(leng): Make the appropriate call when it's working. On 2014/02/03 22:38:04, leng wrote: > On 2014/01/31 22:31:51, groby wrote: > > When what is working? > > When the correct text-query function is working. I added a comment - any > clearer? Much - thank you! https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/permission_bubble_controller.mm:237: attributes:@{ NSForegroundColorAttributeName : linkColor }]]; On 2014/02/03 22:38:04, leng wrote: > On 2014/01/31 22:31:51, groby wrote: > > Is the customizeButton supposed to be an actual link? There is > HyperlinkTextView > > for that. > > I tried that, but not only is it significantly more code, I don't know how to > remove the underline from a HyperlinkTextView. The mocks don't have the text > underlined. Odd - it should be very simple: linkView([[HyperLinkTextView alloc] initWithFrameNSZeroRect];) [linkView setMessage:@"Customize" font:[NSFont labelFontOfSize:0] color:linkColor] Also, we should probably talk to the UX guys about links without underline. It's... weird. Feel free to file a bug against UX and we can revisit this later. (If we want all Chrome UI to not show underlines, we should fix HyperLinkText View) https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/permission_bubble_controller.mm:295: @end // implementation BookmarkBubbleController(ExposedForUnitTesting) On 2014/02/03 22:38:04, leng wrote: > On 2014/01/31 22:31:51, groby wrote: > > ExposedForUnitTesting? > > BookmarkBubbleController? > Now you know how I started this file... :) Heh - I didn't even notice that part. Bad reviewer! No cookie! ;) https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/permission_bubble_controller.mm:295: @end // implementation BookmarkBubbleController(ExposedForUnitTesting) On 2014/02/03 22:38:04, leng wrote: > On 2014/01/31 22:31:51, groby wrote: > > ExposedForUnitTesting? > > BookmarkBubbleController? > Now you know how I started this file... :) Heh - I didn't even notice that part. Bad reviewer! No cookie! ;) https://codereview.chromium.org/151593005/diff/280001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/permission_bubble_cocoa.mm (right): https://codereview.chromium.org/151593005/diff/280001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/permission_bubble_cocoa.mm:26: [[parent_window_ windowController] locationBarBridge]; I'm surprised - does this not warn at compile time about possibly not handling message locationBarBridge? (Since it's only handled by BrowserWindowController) https://codereview.chromium.org/151593005/diff/280001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/permission_bubble_controller.h (right): https://codereview.chromium.org/151593005/diff/280001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/permission_bubble_controller.h:7: #include <vector> Not needed any more https://codereview.chromium.org/151593005/diff/280001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/permission_bubble_controller.mm (right): https://codereview.chromium.org/151593005/diff/280001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/permission_bubble_controller.mm:107: for (auto it = requests.begin(); it != requests.end(); it++) { I envy our Cocoa code the ability to use auto... Wish all of Chrome could https://codereview.chromium.org/151593005/diff/280001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/permission_bubble_controller.mm:144: bubbleFrame.size.width += 250; Please call out as constant - I'm not sure why 250, and I suspect it's a gap we reserve somewhere in the mock? https://codereview.chromium.org/151593005/diff/280001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/permission_bubble_controller.mm:269: [[button cell] setBackgroundColor:buttonBackgroundColor]; I'm wondering about the backgroundcolor thing. Looking at the mocks, it looks like we're using the same buttons as ConstrainedWindowButton which have nifty gradients. Please double-check with UX and fix if that's the case. https://codereview.chromium.org/151593005/diff/280001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/permission_bubble_controller.mm:296: NSUInteger index = [checkboxes_ indexOfObject:checkbox]; If you want to avoid indexOfObject:, you can instead use setTag:/tag to have a value that's directly stored on the object. (It's a nit. Both work, but setTag is to me a bit more idiomatic) https://codereview.chromium.org/151593005/diff/280001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/permission_bubble_controller_unittest.mm (right): https://codereview.chromium.org/151593005/diff/280001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/permission_bubble_controller_unittest.mm:59: NSView* parent = base::mac::ObjCCast<NSView>([controller_ bubble]); Since parent _must_ be an NSView, ObjCCastStrict is probably the better choice. https://codereview.chromium.org/151593005/diff/280001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/permission_bubble_controller_unittest.mm:63: NSButton* button = base::mac::ObjCCast<NSButton>(child); You can skip the isKindOfClass - ObjCCast will return nil if the cast fails, and sending a message to nil evaluates to nil (or false) - so NSButton* button = base::mac::ObjCCast<NSButton>(child); if ([title isEqualToString:[button title]]) { return button; } is all you need. https://codereview.chromium.org/151593005/diff/280001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/permission_bubble_controller_unittest.mm:73: NSView* parent = base::mac::ObjCCast<NSView>([controller_ bubble]); See above on strict https://codereview.chromium.org/151593005/diff/280001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/permission_bubble_controller_unittest.mm:77: NSTextField* textField = base::mac::ObjCCast<NSTextField>(child); See above on isKindOfClass https://codereview.chromium.org/151593005/diff/280001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/permission_bubble_controller_unittest.mm:119: MockPermissionBubbleDelegate delegate_a(kPermissionA); You _could_ factor out the MockDelegate stuff into the fixture: void AddDelegate(const std::string& title) { requests_.push_back(new MockPermissionDelegate(title)); EXPECT_CALL(*requests_.back(), GetMessageTextFragment()).Times(1)); } (plus deleting all objects in dtor - STLDeleteElements(requests_);)
https://codereview.chromium.org/151593005/diff/280001/chrome/browser/ui/websi... File chrome/browser/ui/website_settings/mock_permission_bubble_delegate.h (right): https://codereview.chromium.org/151593005/diff/280001/chrome/browser/ui/websi... chrome/browser/ui/website_settings/mock_permission_bubble_delegate.h:12: class MockPermissionBubbleDelegate : public PermissionBubbleDelegate { FYI. There is already a MockDelegate in the permission_bubble_manager_unittest.cc (https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...) Maybe we change the test to use this mock here, or move the MockDelegate from the test into a separate file.
I wonder whether there should be a website_settings subdirectory in chrome/browser/ui/cocoa/ now. Similiar to chrome/browser/ui/views/website_settings.
Thank you for catching the missing cocoa/website_settings directory - yes, please add.
I've added the ui/cocoa/website_settings directory. Should I move ui/cocoa/website_settings_bubble_controller* into that directory in a separate CL? https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/browser_window_controller.mm (right): https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser_window_controller.mm:1601: PermissionBubbleManager::FromWebContents(contents)->SetView( On 2014/02/04 02:39:29, groby wrote: > On 2014/02/03 22:38:04, leng wrote: > > On 2014/01/31 22:31:51, groby wrote: > > > Is it ok for the PBM to not have its view NULLED out if another manager uses > > the > > > view? > > > > It would be safer to NULL out view for the previous manager. How would I > find > > the previous contents, to retrieve the previous manager? Or should I plumb > > something from the other end? i.e. have the view tell its outgoing > delegate(the > > manager) that it's being pre-empted when the view is assigned a new delegate > > (manager)? > > Ugh. I missed that we only get the new contents. On Views/GTK, we got both old > and new. pkasting actually left a bug in browser_window_cocoa.mm wondering _why_ > we handle things differently.[1] > > Maybe worth following up w/ Peter if there's already a bug on it, but not in > scope for this CL. If there is no bug, I'd file one, though. I'm uncomfortable > with stuffing this pointer into every manager in existence. (IMHO, we shouldn't > have a shared view, but that's another pig to skin...) > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... I filed a bug, and added a comment. We'll see what Peter thinks. https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/permission_bubble_controller.h (right): https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/permission_bubble_controller.h:12: #include "chrome/browser/ui/website_settings/permission_bubble_delegate.h" On 2014/02/04 02:39:29, groby wrote: > On 2014/02/03 22:38:04, leng wrote: > > On 2014/01/31 22:31:51, groby wrote: > > > Not needed - just forward declare > > > > Maybe I just don't understand how to forward-declare a class-specific class? > > I tried: > > class PermissionBubbleView::Delegate; > > but it didn't work. > > Ah - _that_ delegate is in permission_bubble_view.h, which still needs to be > included. (No fwd dcl for nested classes...) > > But permission_bubble_delegate.h is for the PermissionBubbleDelegate, which can > be forward declared. (Not that those two names are confusing, or anything :) AUGH! Silly names. Done. https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/permission_bubble_controller.mm (right): https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/permission_bubble_controller.mm:38: const int kButtonBackgroundColor = 239; On 2014/02/04 02:39:29, groby wrote: > On 2014/02/03 22:38:04, leng wrote: > > On 2014/01/31 22:31:51, groby wrote: > > > Is that shared across platforms? Would it be worth to have a "theme" file? > > > > I don't know. I took the color from the mocks, so I suspect so. I'm not > > familiar with "theme" files - what could I look at for an example? > > "theme" file is fancy speak for a shared header with those constants :) I see. Thanks. :) This is no longer relevant, though, as I switched to using ConstrainedWindowButton, as you suggested. https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/permission_bubble_controller.mm:237: attributes:@{ NSForegroundColorAttributeName : linkColor }]]; On 2014/02/04 02:39:29, groby wrote: > On 2014/02/03 22:38:04, leng wrote: > > On 2014/01/31 22:31:51, groby wrote: > > > Is the customizeButton supposed to be an actual link? There is > > HyperlinkTextView > > > for that. > > > > I tried that, but not only is it significantly more code, I don't know how to > > remove the underline from a HyperlinkTextView. The mocks don't have the text > > underlined. > > Odd - it should be very simple: > > linkView([[HyperLinkTextView alloc] initWithFrameNSZeroRect];) > [linkView setMessage:@"Customize" font:[NSFont labelFontOfSize:0] > color:linkColor] > > Also, we should probably talk to the UX guys about links without underline. > It's... weird. > > Feel free to file a bug against UX and we can revisit this later. (If we want > all Chrome UI to not show underlines, we should fix HyperLinkText View) So I think I ignored, or glossed over, the question you asked initially - it's not actually a link. It's supposed to look like one, but it changes the UI within the bubble - it does not pull up a website. I think that's the difference you're curious about. I've left it as a NSButton, because it doesn't actually show a website, but I'm happy to change it if you like. https://codereview.chromium.org/151593005/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/permission_bubble_controller.mm:295: @end // implementation BookmarkBubbleController(ExposedForUnitTesting) On 2014/02/04 02:39:29, groby wrote: > On 2014/02/03 22:38:04, leng wrote: > > On 2014/01/31 22:31:51, groby wrote: > > > ExposedForUnitTesting? > > > > BookmarkBubbleController? > > Now you know how I started this file... :) > > Heh - I didn't even notice that part. Bad reviewer! No cookie! ;) Am I supposed to pay you with cookies? Are you allergic to nuts? https://codereview.chromium.org/151593005/diff/280001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/permission_bubble_cocoa.mm (right): https://codereview.chromium.org/151593005/diff/280001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/permission_bubble_cocoa.mm:26: [[parent_window_ windowController] locationBarBridge]; On 2014/02/04 02:39:30, groby wrote: > I'm surprised - does this not warn at compile time about possibly not handling > message locationBarBridge? (Since it's only handled by BrowserWindowController) Nope. <shrug>? https://codereview.chromium.org/151593005/diff/280001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/permission_bubble_controller.h (right): https://codereview.chromium.org/151593005/diff/280001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/permission_bubble_controller.h:7: #include <vector> On 2014/02/04 02:39:30, groby wrote: > Not needed any more Done. https://codereview.chromium.org/151593005/diff/280001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/permission_bubble_controller.mm (right): https://codereview.chromium.org/151593005/diff/280001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/permission_bubble_controller.mm:144: bubbleFrame.size.width += 250; On 2014/02/04 02:39:30, groby wrote: > Please call out as constant - I'm not sure why 250, and I suspect it's a gap we > reserve somewhere in the mock? That was a testing leftover... thanks for catching it. https://codereview.chromium.org/151593005/diff/280001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/permission_bubble_controller.mm:269: [[button cell] setBackgroundColor:buttonBackgroundColor]; On 2014/02/04 02:39:30, groby wrote: > I'm wondering about the backgroundcolor thing. Looking at the mocks, it looks > like we're using the same buttons as ConstrainedWindowButton which have nifty > gradients. Please double-check with UX and fix if that's the case. I did - and we're using the version with the borders. I've changed it to be a ConstrainedWindowButton, thanks for the suggestion! https://codereview.chromium.org/151593005/diff/280001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/permission_bubble_controller.mm:296: NSUInteger index = [checkboxes_ indexOfObject:checkbox]; On 2014/02/04 02:39:30, groby wrote: > If you want to avoid indexOfObject:, you can instead use setTag:/tag to have a > value that's directly stored on the object. (It's a nit. Both work, but setTag > is to me a bit more idiomatic) Done. https://codereview.chromium.org/151593005/diff/280001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/permission_bubble_controller_unittest.mm (right): https://codereview.chromium.org/151593005/diff/280001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/permission_bubble_controller_unittest.mm:59: NSView* parent = base::mac::ObjCCast<NSView>([controller_ bubble]); On 2014/02/04 02:39:30, groby wrote: > Since parent _must_ be an NSView, ObjCCastStrict is probably the better choice. Done. https://codereview.chromium.org/151593005/diff/280001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/permission_bubble_controller_unittest.mm:63: NSButton* button = base::mac::ObjCCast<NSButton>(child); On 2014/02/04 02:39:30, groby wrote: > You can skip the isKindOfClass - ObjCCast will return nil if the cast fails, and > sending a message to nil evaluates to nil (or false) - so > > NSButton* button = base::mac::ObjCCast<NSButton>(child); > if ([title isEqualToString:[button title]]) { > return button; > } > > is all you need. Cool! Done. https://codereview.chromium.org/151593005/diff/280001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/permission_bubble_controller_unittest.mm:73: NSView* parent = base::mac::ObjCCast<NSView>([controller_ bubble]); On 2014/02/04 02:39:30, groby wrote: > See above on strict Done. https://codereview.chromium.org/151593005/diff/280001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/permission_bubble_controller_unittest.mm:77: NSTextField* textField = base::mac::ObjCCast<NSTextField>(child); On 2014/02/04 02:39:30, groby wrote: > See above on isKindOfClass Done. https://codereview.chromium.org/151593005/diff/280001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/permission_bubble_controller_unittest.mm:119: MockPermissionBubbleDelegate delegate_a(kPermissionA); On 2014/02/04 02:39:30, groby wrote: > You _could_ factor out the MockDelegate stuff into the fixture: > > void AddDelegate(const std::string& title) { > requests_.push_back(new MockPermissionDelegate(title)); > EXPECT_CALL(*requests_.back(), GetMessageTextFragment()).Times(1)); > } > > (plus deleting all objects in dtor - STLDeleteElements(requests_);) Another good suggestion. I did the same with acceptStates, though without the deleting of the objects. :) https://codereview.chromium.org/151593005/diff/280001/chrome/browser/ui/websi... File chrome/browser/ui/website_settings/mock_permission_bubble_delegate.h (right): https://codereview.chromium.org/151593005/diff/280001/chrome/browser/ui/websi... chrome/browser/ui/website_settings/mock_permission_bubble_delegate.h:12: class MockPermissionBubbleDelegate : public PermissionBubbleDelegate { On 2014/02/04 09:53:38, markusheintz_ wrote: > FYI. There is already a MockDelegate in the > permission_bubble_manager_unittest.cc > > (https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...) > > Maybe we change the test to use this mock here, or move the MockDelegate from > the test into a separate file. > Yes, I saw that after I'd added this version. I'd prefer to make that change in a separate CL, to combine the mocks, after this one is pushed.
Thanks a lot for adding a chrome/browser/ui/cocoa/website_settings sub dir. Regarding the MockPermissionBubbleDelegate: I'm ok if you merge the two mocks in a separate CL. I'll defer to Rachel for the rest of the review, as my Cocoa knowledge is quite limited.
@groby - ping?
LGTM w/ nits https://codereview.chromium.org/151593005/diff/550002/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa.h (right): https://codereview.chromium.org/151593005/diff/550002/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa.h:41: DISALLOW_COPY_AND_ASSIGN(PermissionBubbleCocoa); nit: #include "base/macros.h" https://codereview.chromium.org/151593005/diff/550002/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa.mm (right): https://codereview.chromium.org/151593005/diff/550002/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa.mm:25: LocationBarViewMac* location_bar = nit: Move down in front of NSPoint - declare close to use. https://codereview.chromium.org/151593005/diff/550002/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm (right): https://codereview.chromium.org/151593005/diff/550002/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm:70: [[InfoBubbleWindow alloc] initWithContentRect:NSMakeRect(0, 0, 240, 150) nit: since showAtAnchor computes a new window size anyways, you might want to use ui::kWindowSizeDeterminedLater https://codereview.chromium.org/151593005/diff/550002/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm:103: NSRect bubbleFrame = [[self window] frame]; [[self window] contentRectForFrameRect:[[self window] frame]]; But I don't think you need the contentRect here at all, this will work with NSZeroRect too I think. Or force the 240x150 size here instead. https://codereview.chromium.org/151593005/diff/550002/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm:115: setTag:(it - requests.begin())]; setTag:index https://codereview.chromium.org/151593005/diff/550002/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm:135: (kButtonHeight - std::ceil(NSHeight([customizeButton frame])) * 0.5f); I think the braces for ceil are off - you want to have ceil(height / 2), since you could otherwise get half pixels. https://codereview.chromium.org/151593005/diff/550002/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm:149: allowOrOkButton.reset([[self buttonWithTitle:@"OK" You should at some point i18n the text constants - feel free to do in follow up CL, but please do soon-ish. https://codereview.chromium.org/151593005/diff/550002/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm:236: [checkbox setFrameOrigin:NSMakePoint(0, kCheckboxYAdjustment)]; since you're setting the permissionView's origin, do checkboxes still need to have their origin set? https://codereview.chromium.org/151593005/diff/550002/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm:258: NSRect frame = NSMakeRect(0, 0, kButtonWidth, kButtonHeight); N.B.: This will break for internationalized strings. You might want to use sizeToFit and then change the width to accommodate the larger of the strings.
Making the sizing more robust for the buttons made me realize it should be more robust for the layout, as well. PTAL, thanks! https://codereview.chromium.org/151593005/diff/550002/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa.h (right): https://codereview.chromium.org/151593005/diff/550002/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa.h:41: DISALLOW_COPY_AND_ASSIGN(PermissionBubbleCocoa); On 2014/02/11 20:10:06, groby wrote: > nit: #include "base/macros.h" Done. https://codereview.chromium.org/151593005/diff/550002/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa.mm (right): https://codereview.chromium.org/151593005/diff/550002/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa.mm:25: LocationBarViewMac* location_bar = On 2014/02/11 20:10:06, groby wrote: > nit: Move down in front of NSPoint - declare close to use. Done. https://codereview.chromium.org/151593005/diff/550002/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm (right): https://codereview.chromium.org/151593005/diff/550002/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm:70: [[InfoBubbleWindow alloc] initWithContentRect:NSMakeRect(0, 0, 240, 150) On 2014/02/11 20:10:06, groby wrote: > nit: since showAtAnchor computes a new window size anyways, you might want to > use ui::kWindowSizeDeterminedLater Done. https://codereview.chromium.org/151593005/diff/550002/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm:103: NSRect bubbleFrame = [[self window] frame]; On 2014/02/11 20:10:06, groby wrote: > [[self window] contentRectForFrameRect:[[self window] frame]]; > But I don't think you need the contentRect here at all, this will work with > NSZeroRect too I think. Or force the 240x150 size here instead. I used NSZeroRect and added padding for it with the NSUnionRect, below. https://codereview.chromium.org/151593005/diff/550002/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm:115: setTag:(it - requests.begin())]; On 2014/02/11 20:10:06, groby wrote: > setTag:index Done. https://codereview.chromium.org/151593005/diff/550002/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm:135: (kButtonHeight - std::ceil(NSHeight([customizeButton frame])) * 0.5f); On 2014/02/11 20:10:06, groby wrote: > I think the braces for ceil are off - you want to have ceil(height / 2), since > you could otherwise get half pixels. Good catch! The ceil() was off, actually. Fixed. https://codereview.chromium.org/151593005/diff/550002/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm:149: allowOrOkButton.reset([[self buttonWithTitle:@"OK" On 2014/02/11 20:10:06, groby wrote: > You should at some point i18n the text constants - feel free to do in follow up > CL, but please do soon-ish. Yup, will do asap in a follow-up CL. https://codereview.chromium.org/151593005/diff/550002/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm:236: [checkbox setFrameOrigin:NSMakePoint(0, kCheckboxYAdjustment)]; On 2014/02/11 20:10:06, groby wrote: > since you're setting the permissionView's origin, do checkboxes still need to > have their origin set? Yes - the calling function just adds to the existing frame origin, it doesn't reset it from scratch. This is done so that if the user selects 'customize' the text does not appear to move - the bullet points are simply replaced by the checkboxes. Under the hood, it requires a slightly different origin. https://codereview.chromium.org/151593005/diff/550002/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm:258: NSRect frame = NSMakeRect(0, 0, kButtonWidth, kButtonHeight); On 2014/02/11 20:10:06, groby wrote: > N.B.: This will break for internationalized strings. You might want to use > sizeToFit and then change the width to accommodate the larger of the strings. Good catch. Done.
Still LGTM :) (In general, if I post LGTM w/nits, feel free to commit immediately after uploading the nit fixes unless you want another look at it. Happy to do the review, but trust you to do the right thing :)
On 2014/02/12 19:28:02, groby wrote: > Still LGTM :) (In general, if I post LGTM w/nits, feel free to commit > immediately after uploading the nit fixes unless you want another look at it. > Happy to do the review, but trust you to do the right thing :) Thanks! :) I thought that I'd changed the logic enough to warrant you taking another look.
The CQ bit was checked by leng@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/leng@chromium.org/151593005/640001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
On 2014/02/12 23:32:33, I haz the power (commit-bot) wrote: > Retried try job too often on chromium_presubmit for step(s) presubmit > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p... @markusheintz - OWNERS for chrome/browser/ui/website_settings/, please!
On 2014/02/12 23:34:51, leng wrote: > On 2014/02/12 23:32:33, I haz the power (commit-bot) wrote: > > Retried try job too often on chromium_presubmit for step(s) presubmit > > > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p... > > @markusheintz - OWNERS for chrome/browser/ui/website_settings/, please! ping? Markus, I still need an OWNERS approval for chrome/browser/ui/website_settings. Thanks!
On 2014/02/14 18:24:42, leng wrote: > On 2014/02/12 23:34:51, leng wrote: > > On 2014/02/12 23:32:33, I haz the power (commit-bot) wrote: > > > Retried try job too often on chromium_presubmit for step(s) presubmit > > > > > > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p... > > > > @markusheintz - OWNERS for chrome/browser/ui/website_settings/, please! > > ping? > > Markus, I still need an OWNERS approval for chrome/browser/ui/website_settings. > Thanks! LGTM
The CQ bit was checked by leng@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/leng@chromium.org/151593005/990001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel, mac_chromium_rel
The CQ bit was checked by leng@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/leng@chromium.org/151593005/1180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel
The CQ bit was checked by leng@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/leng@chromium.org/151593005/1180001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for chrome/browser/ui/cocoa/browser_window_controller.mm:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file chrome/browser/ui/cocoa/browser_window_controller.mm
Hunk #1 FAILED at 63.
Hunk #2 succeeded at 418 (offset -1 lines).
Hunk #3 succeeded at 1589 (offset -2 lines).
1 out of 3 hunks FAILED -- saving rejects to file
chrome/browser/ui/cocoa/browser_window_controller.mm.rej
Patch: chrome/browser/ui/cocoa/browser_window_controller.mm
Index: chrome/browser/ui/cocoa/browser_window_controller.mm
diff --git a/chrome/browser/ui/cocoa/browser_window_controller.mm
b/chrome/browser/ui/cocoa/browser_window_controller.mm
index
55cf89d0840a8b7ca7c25980cad2524250cd862d..6b7300b11dd5ca8188f171525a64bcdb639395ad
100644
--- a/chrome/browser/ui/cocoa/browser_window_controller.mm
+++ b/chrome/browser/ui/cocoa/browser_window_controller.mm
@@ -63,12 +63,14 @@
#import "chrome/browser/ui/cocoa/tabs/tab_strip_view.h"
#import "chrome/browser/ui/cocoa/tabs/tab_view.h"
#import "chrome/browser/ui/cocoa/toolbar/toolbar_controller.h"
+#include "chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa.h"
#include "chrome/browser/ui/fullscreen/fullscreen_controller.h"
#include "chrome/browser/ui/omnibox/location_bar.h"
#include "chrome/browser/ui/tabs/dock_info.h"
#include "chrome/browser/ui/tabs/tab_strip_model.h"
#include "chrome/browser/ui/tabs/tab_strip_model_delegate.h"
#include "chrome/browser/ui/toolbar/encoding_menu_controller.h"
+#include "chrome/browser/ui/website_settings/permission_bubble_manager.h"
#include "chrome/browser/ui/window_sizer/window_sizer.h"
#include "chrome/common/chrome_switches.h"
#include "chrome/common/profile_management_switches.h"
@@ -419,6 +421,9 @@ enum {
// Create the bridge for the status bubble.
statusBubble_ = new StatusBubbleMac([self window], self);
+ // Create the permissions bubble.
+ permissionBubbleCocoa_.reset(new PermissionBubbleCocoa([self window]));
+
// Register for application hide/unhide notifications.
[[NSNotificationCenter defaultCenter]
addObserver:self
@@ -1588,6 +1593,14 @@ enum {
[infoBarContainerController_ changeWebContents:contents];
+ // No need to remove previous bubble. It will close itself.
+ // TODO(leng): The PermissionBubbleManager for the previous contents should
+ // have SetView(NULL) called. Fix this when the previous contents are
+ // available here or move to BrowserWindowCocoa::OnActiveTabChanged().
+ // crbug.com/340720
+ PermissionBubbleManager::FromWebContents(contents)->SetView(
+ permissionBubbleCocoa_.get());
+
// Must do this after bookmark and infobar updates to avoid
// unnecesary resize in contents.
[devToolsController_ updateDevToolsForWebContents:contents
The CQ bit was checked by leng@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/leng@chromium.org/151593005/1400001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel
The CQ bit was checked by groby@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/leng@chromium.org/151593005/1400001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel
The CQ bit was checked by leng@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/leng@chromium.org/151593005/1400001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/leng@chromium.org/151593005/1400001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel
On 2014/02/28 04:30:06, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: mac_chromium_rel @groby - PTAL It turns out the mac test failures actually were catching a bug. It's what we discussed earlier - because of the way the Mac switches tabs, the previous WebContents is unknown. So the PermissionBubbleManager for the previous WebContents could still be pointing to a PermissionBubbleView, which has since become invalid. I've changed this CL to be more robust from within PermissionBubbleCocoa: it notifies the delegate when it's deleted, and when the delegate is being replaced. I don't think that this negates the need to revisit the way Mac switches tabs, but I see no harm in making the class relationships here more robust.
https://codereview.chromium.org/151593005/diff/1600001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa.mm (right): https://codereview.chromium.org/151593005/diff/1600001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa.mm:50: if (delegate_ == delegate) I'm confused - this seems to re-attach an existing bubble to a new delegate? Doesn't that leave contents from the previous bubble around? I'd feel better if we closed and re-opened a bubble when a delegate changes. https://codereview.chromium.org/151593005/diff/1600001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa.mm:58: bubbleController_ = nil; delegate_->SetView(NULL) here? I'm not clear on the lifetimes of the involved objects. Maybe document in header?
https://codereview.chromium.org/151593005/diff/1600001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa.mm (right): https://codereview.chromium.org/151593005/diff/1600001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa.mm:50: if (delegate_ == delegate) On 2014/03/03 18:34:05, groby wrote: > I'm confused - this seems to re-attach an existing bubble to a new delegate? > > Doesn't that leave contents from the previous bubble around? I'd feel better if > we closed and re-opened a bubble when a delegate changes. It's just here as an early return. The delegate itself should control the closing and re-showing of the bubbles. https://codereview.chromium.org/151593005/diff/1600001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa.mm:58: bubbleController_ = nil; On 2014/03/03 18:34:05, groby wrote: > delegate_->SetView(NULL) here? I'm not clear on the lifetimes of the involved > objects. Maybe document in header? No, because the delegate might still have 'control' of the view, and re-open it with a different request.
LGTM for permission_bubble_cocoa.mm (Based on my understanding of offline communication with leng@, that was the only file that changed. Please don't submit if I misunderstood that)
On 2014/03/03 19:03:08, groby wrote: > LGTM for permission_bubble_cocoa.mm (Based on my understanding of offline > communication with leng@, that was the only file that changed. Please don't > submit if I misunderstood that) That's correct - this patch only changed permission_bubble_cocoa.mm.
The CQ bit was checked by leng@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/leng@chromium.org/151593005/1600001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_rel for step(s) app_list_unittests, ash_unittests, aura_unittests, cacheinvalidation_unittests, cc_unittests, check_deps, check_deps2git, chrome_elf_unittests, chromedriver_unittests, components_unittests, compositor_unittests, content_browsertests, content_unittests, crypto_unittests, events_unittests, gfx_unittests, google_apis_unittests, gpu_unittests, installer_util_unittests, ipc_tests, jingle_unittests, media_unittests, mini_installer_test, nacl_integration, ppapi_unittests, printing_unittests, remoting_unittests, sql_unittests, sync_integration_tests, sync_unit_tests, telemetry_perf_unittests, telemetry_unittests, views_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
The CQ bit was checked by leng@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/leng@chromium.org/151593005/1600001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/leng@chromium.org/151593005/1600001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_rel for step(s) app_list_unittests, ash_unittests, aura_unittests, base_unittests, browser_tests, cacheinvalidation_unittests, cc_unittests, check_deps, check_deps2git, chrome_elf_unittests, chromedriver_unittests, components_unittests, compositor_unittests, content_browsertests, content_unittests, crypto_unittests, events_unittests, gfx_unittests, google_apis_unittests, gpu_unittests, installer_util_unittests, interactive_ui_tests, ipc_tests, jingle_unittests, media_unittests, mini_installer_test, nacl_integration, net_unittests, ppapi_unittests, printing_unittests, remoting_unittests, sql_unittests, sync_integration_tests, sync_unit_tests, telemetry_perf_unittests, telemetry_unittests, unit_tests, views_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
The CQ bit was checked by leng@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/leng@chromium.org/151593005/1600001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_rel for step(s) base_unittests, interactive_ui_tests, net_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
The CQ bit was checked by leng@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/leng@chromium.org/151593005/1600001
Message was sent while issue was closed.
Change committed as 254635 |
