Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
242443005:
Changes cocoa implementation of permission bubble to better match mocks. (Closed)
|
Created:
6 years, 8 months ago by leng Modified:
6 years, 7 months ago CC:
chromium-reviews, markusheintz_ Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionChanges cocoa implementation of permission bubble to better match mocks.
Specifically, when the bubble is in 'customize' mode, the UI to change
the setting of a permission (allow/deny) will be a drop-down menu rather
than a checkbox.
BUG=None
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=266689
Patch Set 1 #
Total comments: 24
Patch Set 2 : feedback #Patch Set 3 : feedback #Patch Set 4 : one more edit #
Total comments: 8
Patch Set 5 : a few more edits #
Messages
Total messages: 26 (0 generated)
https://codereview.chromium.org/242443005/diff/1/chrome/browser/ui/website_se... File chrome/browser/ui/website_settings/permission_menu_model.cc (right): https://codereview.chromium.org/242443005/diff/1/chrome/browser/ui/website_se... chrome/browser/ui/website_settings/permission_menu_model.cc:61: l10n_util::GetStringUTF16(IDS_PERMISSION_ALLOW)); This looks good. I'd set them to the page info constants since when that gets redone they'll be the same, but this is fine -- we'll redo page-info and use this, probably, right?
https://codereview.chromium.org/242443005/diff/1/chrome/browser/ui/website_se... File chrome/browser/ui/website_settings/permission_menu_model.cc (right): https://codereview.chromium.org/242443005/diff/1/chrome/browser/ui/website_se... chrome/browser/ui/website_settings/permission_menu_model.cc:61: l10n_util::GetStringUTF16(IDS_PERMISSION_ALLOW)); On 2014/04/18 17:17:02, Greg Billock wrote: > This looks good. I'd set them to the page info constants since when that gets > redone they'll be the same, but this is fine -- we'll redo page-info and use > this, probably, right? Yes, I agree. When we redo the page-info bubble, we'll want to change the strings. I just found the page-info strings looked really wrong here.
https://codereview.chromium.org/242443005/diff/1/chrome/browser/ui/cocoa/webs... File chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm (right): https://codereview.chromium.org/242443005/diff/1/chrome/browser/ui/cocoa/webs... chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm:112: [[self cell] selectItemAtIndex:menuModel_->GetIndexOfCommandId(setting)]; I don't think you need the cell for selectItemAtIndex: https://codereview.chromium.org/242443005/diff/1/chrome/browser/ui/cocoa/webs... chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm:116: [self setFrameSize:[PermissionSelectorButton sizeForTitle:[self title] I'm not happy with the tight coupling between the two classes. Any way the to have the controller do the sizing? (Also - does this still need sizeToFit above?) https://codereview.chromium.org/242443005/diff/1/chrome/browser/ui/cocoa/webs... chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm:140: - (NSView*)menuForRequest:(PermissionBubbleRequest*)request menuViewForRequest? https://codereview.chromium.org/242443005/diff/1/chrome/browser/ui/cocoa/webs... chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm:164: - (void)alignCenterOf:(NSView*)viewA verticallyToCenterOf:(NSView*)viewB; I'd probably do that as a standalone function, because it doesn't require self. (It's also a very generic function which maybe one day can be lifted out...) https://codereview.chromium.org/242443005/diff/1/chrome/browser/ui/cocoa/webs... chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm:281: NSView* menu = base::mac::ObjCCast<NSView>(permissionMenus[0]); Menu 0 is OK? We don't have varying sizes? Also: Didn't the NSUnionRect above take care of incorporating the menu size? https://codereview.chromium.org/242443005/diff/1/chrome/browser/ui/cocoa/webs... chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm:424: withCallback:^(int index, bool allow) { Why not just pass down the blockDelegate and save ourselves an NSBlock in the process? (Prebind it with index, that way the button doesn't need to keep track of that) If you want to make bigger changes, there's this solution: Have a standalone function: void AllowCallback(BlockDelegate* delegate, int index, const WebsiteSettingsUI::PermissionInfo& permission) { delegate_->ToggleAccept(index, permission.setting == CONTENT_SETTING_ALLOW); } And turn that into PermissionMenuModel::ChangeCallback via callback = base::Bind(AllowCallback, base::Unretained(delegate_), index) (We could save ourselves even more messiness if ToggleAccept took a PermissionInfo.) https://codereview.chromium.org/242443005/diff/1/chrome/browser/ui/cocoa/webs... chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm:478: - (void)alignCenterOf:(NSView*)viewA verticallyToCenterOf:(NSView*)viewB { Grumble. I wish we had helper functions for this. https://codereview.chromium.org/242443005/diff/1/chrome/browser/ui/cocoa/webs... File chrome/browser/ui/cocoa/website_settings/permission_bubble_controller_unittest.mm (right): https://codereview.chromium.org/242443005/diff/1/chrome/browser/ui/cocoa/webs... chrome/browser/ui/cocoa/website_settings/permission_bubble_controller_unittest.mm:92: NSView* parent = base::mac::ObjCCastStrict<NSView>([controller_ bubble]); No need to cast - the bubble is always an NSView https://codereview.chromium.org/242443005/diff/1/chrome/browser/ui/cocoa/webs... chrome/browser/ui/cocoa/website_settings/permission_bubble_controller_unittest.mm:94: NSButton* button = base::mac::ObjCCast<NSButton>(child); Shorter & less casts: Class button_class = want_nspopup ? [NSPopUpButton class] : [NSButtonClass] for (NSButton* button in [parent subviews]) { if (![button isMemberOfClass:button_class]) continue; if ([title isEqualToString:[button title]) return button; } isMemberOfClass restricts to specific classes - subclass instances are false. https://codereview.chromium.org/242443005/diff/1/chrome/browser/ui/cocoa/webs... chrome/browser/ui/cocoa/website_settings/permission_bubble_controller_unittest.mm:123: NSMenu* menu = [base::mac::ObjCCast<NSPopUpButton>(menu_button) menu]; I think you want strict here, or even pass in an NSPopUpButton - this does not make sense for other buttons? https://codereview.chromium.org/242443005/diff/1/chrome/browser/ui/cocoa/webs... File chrome/browser/ui/cocoa/website_settings/permission_selector_button.mm (right): https://codereview.chromium.org/242443005/diff/1/chrome/browser/ui/cocoa/webs... chrome/browser/ui/cocoa/website_settings/permission_selector_button.mm:53: + (NSSize)sizeForTitle:(NSString*)title forButton:(NSButton*)button { If you can at all, I think I'd move that onto the controller.
https://codereview.chromium.org/242443005/diff/1/chrome/browser/ui/cocoa/webs... File chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm (right): https://codereview.chromium.org/242443005/diff/1/chrome/browser/ui/cocoa/webs... chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm:112: [[self cell] selectItemAtIndex:menuModel_->GetIndexOfCommandId(setting)]; On 2014/04/18 18:18:18, groby wrote: > I don't think you need the cell for selectItemAtIndex: The first time I tried, it didn't work. Must've had something else wrong. Done. https://codereview.chromium.org/242443005/diff/1/chrome/browser/ui/cocoa/webs... chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm:116: [self setFrameSize:[PermissionSelectorButton sizeForTitle:[self title] On 2014/04/18 18:18:18, groby wrote: > I'm not happy with the tight coupling between the two classes. Any way the to > have the controller do the sizing? (Also - does this still need sizeToFit > above?) As per our conversation offline, I'll make a follow-up CL that moves website_settings_bubble_controller into website_settings/ so that both controllers can share a helper function. Code is copied, with a TODO, for now. https://codereview.chromium.org/242443005/diff/1/chrome/browser/ui/cocoa/webs... chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm:140: - (NSView*)menuForRequest:(PermissionBubbleRequest*)request On 2014/04/18 18:18:18, groby wrote: > menuViewForRequest? Would you like me to change the other names, as well? If not, I'd rather keep this - it matches 'labelForRequest' and 'buttonWithTitle', which both also return NSViews. https://codereview.chromium.org/242443005/diff/1/chrome/browser/ui/cocoa/webs... chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm:164: - (void)alignCenterOf:(NSView*)viewA verticallyToCenterOf:(NSView*)viewB; On 2014/04/18 18:18:18, groby wrote: > I'd probably do that as a standalone function, because it doesn't require self. > (It's also a very generic function which maybe one day can be lifted out...) Done. https://codereview.chromium.org/242443005/diff/1/chrome/browser/ui/cocoa/webs... chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm:281: NSView* menu = base::mac::ObjCCast<NSView>(permissionMenus[0]); On 2014/04/18 18:18:18, groby wrote: > Menu 0 is OK? We don't have varying sizes? > > Also: Didn't the NSUnionRect above take care of incorporating the menu size? Changed to use a max width. The NSUnionRect, above, does not handle this. Until we adjust the menu horizontally, the union won't help the overall width - the frame's origin is at (0, y). https://codereview.chromium.org/242443005/diff/1/chrome/browser/ui/cocoa/webs... chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm:424: withCallback:^(int index, bool allow) { On 2014/04/18 18:18:18, groby wrote: > Why not just pass down the blockDelegate and save ourselves an NSBlock in the > process? (Prebind it with index, that way the button doesn't need to keep track > of that) > > If you want to make bigger changes, there's this solution: > > Have a standalone function: > void AllowCallback(BlockDelegate* delegate, int index, const > WebsiteSettingsUI::PermissionInfo& permission) > { > delegate_->ToggleAccept(index, permission.setting == > CONTENT_SETTING_ALLOW); > } > And turn that into PermissionMenuModel::ChangeCallback via > callback = base::Bind(AllowCallback, base::Unretained(delegate_), index) > > (We could save ourselves even more messiness if ToggleAccept took a > PermissionInfo.) Good idea. Done. I did the simplest version, which I find most straightforward. https://codereview.chromium.org/242443005/diff/1/chrome/browser/ui/cocoa/webs... chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm:478: - (void)alignCenterOf:(NSView*)viewA verticallyToCenterOf:(NSView*)viewB { On 2014/04/18 18:18:18, groby wrote: > Grumble. I wish we had helper functions for this. Me, too. https://codereview.chromium.org/242443005/diff/1/chrome/browser/ui/cocoa/webs... File chrome/browser/ui/cocoa/website_settings/permission_bubble_controller_unittest.mm (right): https://codereview.chromium.org/242443005/diff/1/chrome/browser/ui/cocoa/webs... chrome/browser/ui/cocoa/website_settings/permission_bubble_controller_unittest.mm:92: NSView* parent = base::mac::ObjCCastStrict<NSView>([controller_ bubble]); On 2014/04/18 18:18:18, groby wrote: > No need to cast - the bubble is always an NSView Made NSButton to avoid a cast. https://codereview.chromium.org/242443005/diff/1/chrome/browser/ui/cocoa/webs... chrome/browser/ui/cocoa/website_settings/permission_bubble_controller_unittest.mm:94: NSButton* button = base::mac::ObjCCast<NSButton>(child); On 2014/04/18 18:18:18, groby wrote: > Shorter & less casts: > > Class button_class = want_nspopup ? [NSPopUpButton class] : [NSButtonClass] > for (NSButton* button in [parent subviews]) { > if (![button isMemberOfClass:button_class]) > continue; > if ([title isEqualToString:[button title]) > return button; > } > > isMemberOfClass restricts to specific classes - subclass instances are false. I like it. I need to use ConstrainedWindowButton as the non-NSPopUpButton, because none of these are straight NSButtons. https://codereview.chromium.org/242443005/diff/1/chrome/browser/ui/cocoa/webs... chrome/browser/ui/cocoa/website_settings/permission_bubble_controller_unittest.mm:123: NSMenu* menu = [base::mac::ObjCCast<NSPopUpButton>(menu_button) menu]; On 2014/04/18 18:18:18, groby wrote: > I think you want strict here, or even pass in an NSPopUpButton - this does not > make sense for other buttons? Done. https://codereview.chromium.org/242443005/diff/1/chrome/browser/ui/cocoa/webs... File chrome/browser/ui/cocoa/website_settings/permission_selector_button.mm (right): https://codereview.chromium.org/242443005/diff/1/chrome/browser/ui/cocoa/webs... chrome/browser/ui/cocoa/website_settings/permission_selector_button.mm:53: + (NSSize)sizeForTitle:(NSString*)title forButton:(NSButton*)button { On 2014/04/18 18:18:18, groby wrote: > If you can at all, I think I'd move that onto the controller. I'll move this to a shared location in a subsequent CL.
ping?
LGTM w/ a few tiny nits Sorry for the endless delay :( https://codereview.chromium.org/242443005/diff/60001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm (right): https://codereview.chromium.org/242443005/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm:96: if (self = [super initWithFrame:NSMakeRect(0, 0, 1, 1) pullsDown:NO]) { Why not NSZeroRect? https://codereview.chromium.org/242443005/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm:104: base::BindBlock(^(const WebsiteSettingsUI::PermissionInfo& permission) { I just realized we created an ObjC block for the ONLY reason that C++ doesn't support lambda yet. I feel evil :) https://codereview.chromium.org/242443005/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm:116: // an ugly amount of whitespace between the title and the arrows. Ah. That's because NSPopUpButton's sizeToFit fits to the *largest* element, instead of the currently selected one. Probably worth mentioning. https://codereview.chromium.org/242443005/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm:119: NSDictionary* textAttributes = @{NSFontAttributeName : [self font]}; You could call [[self attributedTitle] attributesAtIndex:0 effectiveRange:NULL] to get the current attributes instead. Means that if Apple ever adds weird attributes we get them for free :)
https://codereview.chromium.org/242443005/diff/60001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm (right): https://codereview.chromium.org/242443005/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm:96: if (self = [super initWithFrame:NSMakeRect(0, 0, 1, 1) pullsDown:NO]) { On 2014/04/25 19:25:28, groby wrote: > Why not NSZeroRect? Done. https://codereview.chromium.org/242443005/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm:104: base::BindBlock(^(const WebsiteSettingsUI::PermissionInfo& permission) { On 2014/04/25 19:25:28, groby wrote: > I just realized we created an ObjC block for the ONLY reason that C++ doesn't > support lambda yet. I feel evil :) >:) https://codereview.chromium.org/242443005/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm:116: // an ugly amount of whitespace between the title and the arrows. On 2014/04/25 19:25:28, groby wrote: > Ah. That's because NSPopUpButton's sizeToFit fits to the *largest* element, > instead of the currently selected one. Probably worth mentioning. > Done. https://codereview.chromium.org/242443005/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm:119: NSDictionary* textAttributes = @{NSFontAttributeName : [self font]}; On 2014/04/25 19:25:28, groby wrote: > You could call > [[self attributedTitle] attributesAtIndex:0 effectiveRange:NULL] > > to get the current attributes instead. > > Means that if Apple ever adds weird attributes we get them for free :) Done.
Still 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/242443005/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on chromium_presubmit
On 2014/04/25 23:57:44, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > tryserver.chromium on chromium_presubmit gbillock - OWNERS for permission_menu_model.cc, please.
On 2014/04/25 23:59:05, leng wrote: > On 2014/04/25 23:57:44, I haz the power (commit-bot) wrote: > > Try jobs failed on following builders: > > tryserver.chromium on chromium_presubmit > > gbillock - OWNERS for permission_menu_model.cc, please. 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/242443005/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on android_dbg_triggered_tests
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/242443005/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on android_dbg_triggered_tests
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/242443005/80001
Message was sent while issue was closed.
Change committed as 266689 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
