Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2826233002:
[subresource_filter] Mac UI updated and xib replaced with code based layout. (Closed)
|
Created:
3 years, 8 months ago by shivanisha Modified:
3 years, 7 months ago CC:
chromium-reviews, engedy, mac-reviews_chromium.org, melandory, subresource-filter-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[subresource_filter] Mac UI updated and xib replaced with code based layout.
- Replaces xib based layout with code based layout.
- Updates the UI as per the latest mocks.
BUG=689992
Review-Url: https://codereview.chromium.org/2826233002
Cr-Commit-Position: refs/heads/master@{#469979}
Committed: https://chromium.googlesource.com/chromium/src/+/da2a48d9f7403a1fbee3a5984d442a49ef14a01c
Patch Set 1 : Initial patch #
Total comments: 37
Patch Set 2 : Feedback addressed. #
Total comments: 26
Patch Set 3 : Feedback addressed , tests added. #
Total comments: 8
Patch Set 4 : Feedback addressed #
Total comments: 5
Patch Set 5 : csharrison feedback addressed #Messages
Total messages: 56 (37 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
The CQ bit was checked by shivanisha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by shivanisha@chromium.org to run a CQ dry run
Patchset #1 (id:40001) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Chromenolia Mac UI to use code based layout instead of xib based layout. BUG= ========== to ========== Chromenolia Mac UI. Does the following: - Replaces xib based layout with code based layout. - Updates the UI as per the latest mocks. BUG=None ==========
shivanisha@chromium.org changed reviewers: + rsesek@chromium.org
Description was changed from ========== Chromenolia Mac UI. Does the following: - Replaces xib based layout with code based layout. - Updates the UI as per the latest mocks. BUG=None ========== to ========== - Replaces xib based layout with code based layout. - Updates the UI as per the latest mocks. BUG=None ==========
rsesek@, PTAL, thanks! The 1st patch does not have tests but I figure I'll start the code review as this is my first attempt at Mac UI. For tests, do you suggest unit tests on the lines of https://cs.chromium.org/chromium/src/chrome/browser/ui/cocoa/translate/transl... ?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Overall, a very nice start. Mostly comments about how to structure the class/subclass relationship. On 2017/05/02 19:22:40, shivanisha wrote: > For tests, do you suggest unit tests on the lines of > https://cs.chromium.org/chromium/src/chrome/browser/ui/cocoa/translate/transl... > ? Yes, that's a good reference. Keep in mind that you'll only need to test things specific to your controller (or functionality you're adding to the ContentSettingBubbleController). https://codereview.chromium.org/2826233002/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/content_settings/content_setting_bubble_cocoa.h (right): https://codereview.chromium.org/2826233002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/content_settings/content_setting_bubble_cocoa.h:64: IBOutlet NSButton* manageCheckbox_; Since this is only used in the subclass, I'd recommend moving the instance variable to there. https://codereview.chromium.org/2826233002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/content_settings/content_setting_bubble_cocoa.h:68: std::unique_ptr<ContentSettingBubbleModel> contentSettingBubbleModel_; nit: blank line after https://codereview.chromium.org/2826233002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/content_settings/content_setting_bubble_cocoa.h:84: // Initializes the controller using the model. Ditto the comments about ownership of the model from below. https://codereview.chromium.org/2826233002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/content_settings/content_setting_bubble_cocoa.h:101: - (void)initManageDoneButtons; It's a little odd exposing just this method (it needs a comment, at the least). Perhaps add a method named -relayoutDialog that calls all the private -initXXX methods? https://codereview.chromium.org/2826233002/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/content_settings/content_setting_bubble_cocoa.mm (right): https://codereview.chromium.org/2826233002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/content_settings/content_setting_bubble_cocoa.mm:32: #import "ui/base/cocoa/window_size_constants.h" Unused https://codereview.chromium.org/2826233002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/content_settings/content_setting_bubble_cocoa.mm:324: [window frameRectForContentRect:NSMakeRect(196, 376, 316, 134)]; Why is this done here? https://codereview.chromium.org/2826233002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/content_settings/content_setting_bubble_cocoa.mm:356: + (ContentSettingBubbleController*)getControllerForModel: naming: call this allocControllerForModel: to make it explicit that this is returning a strong reference https://codereview.chromium.org/2826233002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/content_settings/content_setting_bubble_cocoa.mm:409: - (void)initalizeManageCheckbox { spelling: initialize https://codereview.chromium.org/2826233002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/content_settings/content_setting_bubble_cocoa.mm:884: contentSettingBubbleModel_->AsSubresourceFilterBubbleModel()) nit: add braces since the condition is multi-line https://codereview.chromium.org/2826233002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/content_settings/content_setting_bubble_cocoa.mm:991: bool is_checked = [sender state] == NSOnState; naming: isChecked (use camelCase in ObjC methods) https://codereview.chromium.org/2826233002/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/subresource_filter/subresource_filter_bubble_controller.h (right): https://codereview.chromium.org/2826233002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/subresource_filter/subresource_filter_bubble_controller.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. 2017 https://codereview.chromium.org/2826233002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/subresource_filter/subresource_filter_bubble_controller.h:10: #include <memory> Unused https://codereview.chromium.org/2826233002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/subresource_filter/subresource_filter_bubble_controller.h:12: #include "base/mac/scoped_nsobject.h" Unused. https://codereview.chromium.org/2826233002/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/subresource_filter/subresource_filter_bubble_controller.mm (right): https://codereview.chromium.org/2826233002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/subresource_filter/subresource_filter_bubble_controller.mm:1: // Copyright 2014 The Chromium Authors. All rights reserved. 2017 https://codereview.chromium.org/2826233002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/subresource_filter/subresource_filter_bubble_controller.mm:6: #import "chrome/browser/ui/cocoa/subresource_filter/subresource_filter_bubble_controller.h" This should come first in the #import/#include block and have a blank line after it. https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes https://codereview.chromium.org/2826233002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/subresource_filter/subresource_filter_bubble_controller.mm:23: titleLabel_ = Each instance variable that is alloc/init'd here needs to be autoreleased. In the XIB case, the reference is owned by the XIB. But here, nothing is going to release these, so they will leak. https://codereview.chromium.org/2826233002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/subresource_filter/subresource_filter_bubble_controller.mm:24: [[NSTextField alloc] initWithFrame:NSMakeRect(17, 120, 282, 14)]; Should the x coordinate here be 18, so it's left-aligned properly? https://codereview.chromium.org/2826233002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/subresource_filter/subresource_filter_bubble_controller.mm:39: [manageCheckbox_ setAction:@selector(manageCheckboxChecked:)]; Should the on/off state of the checkbox be set from the model?
csharrison@chromium.org changed reviewers: + csharrison@chromium.org
Drive by: can you link to bug 689992 and post a screenshot of the UI there?
Description was changed from ========== - Replaces xib based layout with code based layout. - Updates the UI as per the latest mocks. BUG=None ========== to ========== - Replaces xib based layout with code based layout. - Updates the UI as per the latest mocks. BUG=689992 ==========
The CQ bit was checked by shivanisha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2826233002/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/content_settings/content_setting_bubble_cocoa.h (right): https://codereview.chromium.org/2826233002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/content_settings/content_setting_bubble_cocoa.h:64: IBOutlet NSButton* manageCheckbox_; On 2017/05/02 at 21:51:47, Robert Sesek wrote: > Since this is only used in the subclass, I'd recommend moving the instance variable to there. done https://codereview.chromium.org/2826233002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/content_settings/content_setting_bubble_cocoa.h:68: std::unique_ptr<ContentSettingBubbleModel> contentSettingBubbleModel_; On 2017/05/02 at 21:51:47, Robert Sesek wrote: > nit: blank line after done https://codereview.chromium.org/2826233002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/content_settings/content_setting_bubble_cocoa.h:84: // Initializes the controller using the model. On 2017/05/02 at 21:51:47, Robert Sesek wrote: > Ditto the comments about ownership of the model from below. done https://codereview.chromium.org/2826233002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/content_settings/content_setting_bubble_cocoa.h:101: - (void)initManageDoneButtons; On 2017/05/02 at 21:51:47, Robert Sesek wrote: > It's a little odd exposing just this method (it needs a comment, at the least). > > Perhaps add a method named -relayoutDialog that calls all the private -initXXX methods? Added a function called layoutView which is also overridden by the sub class and invoked in awakeFromNib as well. https://codereview.chromium.org/2826233002/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/content_settings/content_setting_bubble_cocoa.mm (right): https://codereview.chromium.org/2826233002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/content_settings/content_setting_bubble_cocoa.mm:18: #import "chrome/browser/ui/cocoa/info_bubble_window.h" removed this unused include https://codereview.chromium.org/2826233002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/content_settings/content_setting_bubble_cocoa.mm:32: #import "ui/base/cocoa/window_size_constants.h" On 2017/05/02 at 21:51:47, Robert Sesek wrote: > Unused removed https://codereview.chromium.org/2826233002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/content_settings/content_setting_bubble_cocoa.mm:324: [window frameRectForContentRect:NSMakeRect(196, 376, 316, 134)]; On 2017/05/02 at 21:51:47, Robert Sesek wrote: > Why is this done here? Ah, remnant of earlier patch. Thanks, removed. https://codereview.chromium.org/2826233002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/content_settings/content_setting_bubble_cocoa.mm:356: + (ContentSettingBubbleController*)getControllerForModel: On 2017/05/02 at 21:51:47, Robert Sesek wrote: > naming: call this allocControllerForModel: to make it explicit that this is returning a strong reference done https://codereview.chromium.org/2826233002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/content_settings/content_setting_bubble_cocoa.mm:409: - (void)initalizeManageCheckbox { On 2017/05/02 at 21:51:47, Robert Sesek wrote: > spelling: initialize done. Also moved this to the subclass https://codereview.chromium.org/2826233002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/content_settings/content_setting_bubble_cocoa.mm:884: contentSettingBubbleModel_->AsSubresourceFilterBubbleModel()) On 2017/05/02 at 21:51:47, Robert Sesek wrote: > nit: add braces since the condition is multi-line done https://codereview.chromium.org/2826233002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/content_settings/content_setting_bubble_cocoa.mm:991: bool is_checked = [sender state] == NSOnState; On 2017/05/02 at 21:51:47, Robert Sesek wrote: > naming: isChecked (use camelCase in ObjC methods) done https://codereview.chromium.org/2826233002/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/subresource_filter/subresource_filter_bubble_controller.h (right): https://codereview.chromium.org/2826233002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/subresource_filter/subresource_filter_bubble_controller.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2017/05/02 at 21:51:47, Robert Sesek wrote: > 2017 done in both new files. https://codereview.chromium.org/2826233002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/subresource_filter/subresource_filter_bubble_controller.h:10: #include <memory> On 2017/05/02 at 21:51:47, Robert Sesek wrote: > Unused removed https://codereview.chromium.org/2826233002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/subresource_filter/subresource_filter_bubble_controller.h:12: #include "base/mac/scoped_nsobject.h" On 2017/05/02 at 21:51:47, Robert Sesek wrote: > Unused. removed https://codereview.chromium.org/2826233002/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/subresource_filter/subresource_filter_bubble_controller.mm (right): https://codereview.chromium.org/2826233002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/subresource_filter/subresource_filter_bubble_controller.mm:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2017/05/02 at 21:51:48, Robert Sesek wrote: > 2017 done https://codereview.chromium.org/2826233002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/subresource_filter/subresource_filter_bubble_controller.mm:6: #import "chrome/browser/ui/cocoa/subresource_filter/subresource_filter_bubble_controller.h" On 2017/05/02 at 21:51:48, Robert Sesek wrote: > This should come first in the #import/#include block and have a blank line after it. > > https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes done https://codereview.chromium.org/2826233002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/subresource_filter/subresource_filter_bubble_controller.mm:23: titleLabel_ = On 2017/05/02 at 21:51:48, Robert Sesek wrote: > Each instance variable that is alloc/init'd here needs to be autoreleased. In the XIB case, the reference is owned by the XIB. But here, nothing is going to release these, so they will leak. Done https://codereview.chromium.org/2826233002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/subresource_filter/subresource_filter_bubble_controller.mm:24: [[NSTextField alloc] initWithFrame:NSMakeRect(17, 120, 282, 14)]; On 2017/05/02 at 21:51:48, Robert Sesek wrote: > Should the x coordinate here be 18, so it's left-aligned properly? done https://codereview.chromium.org/2826233002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/subresource_filter/subresource_filter_bubble_controller.mm:39: [manageCheckbox_ setAction:@selector(manageCheckboxChecked:)]; On 2017/05/02 at 21:51:48, Robert Sesek wrote: > Should the on/off state of the checkbox be set from the model? Good idea, though currently the logic is simple enough to not need to get the value from the model. Starts with a default off state and changes when the user changes it.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Mostly some nits. I like the split you made for -layoutView. https://codereview.chromium.org/2826233002/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/content_settings/content_setting_bubble_cocoa.h (right): https://codereview.chromium.org/2826233002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/content_settings/content_setting_bubble_cocoa.h:85: // |settingsBubbleModel| but not of the other objects. Add a note that this is used for subclasses and that most callers should use the class convenience constructor. https://codereview.chromium.org/2826233002/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/content_settings/content_setting_bubble_cocoa.mm (left): https://codereview.chromium.org/2826233002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/content_settings/content_setting_bubble_cocoa.mm:243: // Autoreleases itself on bubble close. Leave this comment here. https://codereview.chromium.org/2826233002/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/content_settings/content_setting_bubble_cocoa.mm (right): https://codereview.chromium.org/2826233002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/content_settings/content_setting_bubble_cocoa.mm:216: + (ContentSettingBubbleController*)getControllerForModel: Rename this too (the @implementation doesn't currently match). https://codereview.chromium.org/2826233002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/content_settings/content_setting_bubble_cocoa.mm:824: if (!manageButton_ && !doneButton_) Move this condition up to the beginning of the method. https://codereview.chromium.org/2826233002/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/subresource_filter/subresource_filter_bubble_controller.h (right): https://codereview.chromium.org/2826233002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/subresource_filter/subresource_filter_bubble_controller.h:8: #import <Cocoa/Cocoa.h> nit: blank line after https://codereview.chromium.org/2826233002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/subresource_filter/subresource_filter_bubble_controller.h:11: //@class ContentSettingBubbleController; Remove https://codereview.chromium.org/2826233002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/subresource_filter/subresource_filter_bubble_controller.h:20: @interface SubresourceFilterBubbleController : ContentSettingBubbleController { The modern Cocoa style is to not declare instance variables in the header. You can instead move this into the .mm file by changing the line: "@interface SubresourceFilterBubbleController (Private)" To: " @interface SubresourceFilterBubbleController () { NSButton* manageCheckbox_; } " https://codereview.chromium.org/2826233002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/subresource_filter/subresource_filter_bubble_controller.h:22: IBOutlet NSButton* manageCheckbox_; |IBOutlet| is magic used for XIB files, so you don't need it here. https://codereview.chromium.org/2826233002/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/subresource_filter/subresource_filter_bubble_controller.mm (right): https://codereview.chromium.org/2826233002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/subresource_filter/subresource_filter_bubble_controller.mm:5: #import "chrome/browser/ui/cocoa/subresource_filter/subresource_filter_bubble_controller.h" nit: blank line after https://codereview.chromium.org/2826233002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/subresource_filter/subresource_filter_bubble_controller.mm:47: - (void)loadView { method ordering: Move this above initializeManageCheckbox since it's called first. https://codereview.chromium.org/2826233002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/subresource_filter/subresource_filter_bubble_controller.mm:53: [titleLabel_ autorelease]; You can switch the |autorelease| calls to direct |release|, since the |addSubview:| takes ownership. https://codereview.chromium.org/2826233002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/subresource_filter/subresource_filter_bubble_controller.mm:79: - (id)initWithModel:(ContentSettingBubbleModel*)contentSettingBubbleModel method ordering: Move this to be the first method https://codereview.chromium.org/2826233002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/subresource_filter/subresource_filter_bubble_controller.mm:105: - (IBAction)manageCheckboxChecked:(id)sender { Similar to |IBOutlet|, you can drop the |IBAction|.
The CQ bit was checked by shivanisha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL, thanks! https://codereview.chromium.org/2826233002/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/content_settings/content_setting_bubble_cocoa.h (right): https://codereview.chromium.org/2826233002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/content_settings/content_setting_bubble_cocoa.h:85: // |settingsBubbleModel| but not of the other objects. On 2017/05/03 at 22:05:11, Robert Sesek wrote: > Add a note that this is used for subclasses and that most callers should use the class convenience constructor. done https://codereview.chromium.org/2826233002/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/content_settings/content_setting_bubble_cocoa.mm (left): https://codereview.chromium.org/2826233002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/content_settings/content_setting_bubble_cocoa.mm:243: // Autoreleases itself on bubble close. On 2017/05/03 at 22:05:11, Robert Sesek wrote: > Leave this comment here. done https://codereview.chromium.org/2826233002/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/content_settings/content_setting_bubble_cocoa.mm (right): https://codereview.chromium.org/2826233002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/content_settings/content_setting_bubble_cocoa.mm:216: + (ContentSettingBubbleController*)getControllerForModel: On 2017/05/03 at 22:05:11, Robert Sesek wrote: > Rename this too (the @implementation doesn't currently match). oops, done https://codereview.chromium.org/2826233002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/content_settings/content_setting_bubble_cocoa.mm:824: if (!manageButton_ && !doneButton_) On 2017/05/03 at 22:05:11, Robert Sesek wrote: > Move this condition up to the beginning of the method. done and made this condition as if (!done_button_) return; https://codereview.chromium.org/2826233002/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/subresource_filter/subresource_filter_bubble_controller.h (right): https://codereview.chromium.org/2826233002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/subresource_filter/subresource_filter_bubble_controller.h:8: #import <Cocoa/Cocoa.h> On 2017/05/03 at 22:05:11, Robert Sesek wrote: > nit: blank line after done https://codereview.chromium.org/2826233002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/subresource_filter/subresource_filter_bubble_controller.h:11: //@class ContentSettingBubbleController; On 2017/05/03 at 22:05:11, Robert Sesek wrote: > Remove done https://codereview.chromium.org/2826233002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/subresource_filter/subresource_filter_bubble_controller.h:20: @interface SubresourceFilterBubbleController : ContentSettingBubbleController { On 2017/05/03 at 22:05:11, Robert Sesek wrote: > The modern Cocoa style is to not declare instance variables in the header. You can instead move this into the .mm file by changing the line: > > "@interface SubresourceFilterBubbleController (Private)" > > To: > > " > @interface SubresourceFilterBubbleController () { > NSButton* manageCheckbox_; > } > " done https://codereview.chromium.org/2826233002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/subresource_filter/subresource_filter_bubble_controller.h:22: IBOutlet NSButton* manageCheckbox_; On 2017/05/03 at 22:05:11, Robert Sesek wrote: > |IBOutlet| is magic used for XIB files, so you don't need it here. done https://codereview.chromium.org/2826233002/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/subresource_filter/subresource_filter_bubble_controller.mm (right): https://codereview.chromium.org/2826233002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/subresource_filter/subresource_filter_bubble_controller.mm:5: #import "chrome/browser/ui/cocoa/subresource_filter/subresource_filter_bubble_controller.h" On 2017/05/03 at 22:05:11, Robert Sesek wrote: > nit: blank line after done https://codereview.chromium.org/2826233002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/subresource_filter/subresource_filter_bubble_controller.mm:47: - (void)loadView { On 2017/05/03 at 22:05:11, Robert Sesek wrote: > method ordering: Move this above initializeManageCheckbox since it's called first. done https://codereview.chromium.org/2826233002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/subresource_filter/subresource_filter_bubble_controller.mm:53: [titleLabel_ autorelease]; On 2017/05/03 at 22:05:11, Robert Sesek wrote: > You can switch the |autorelease| calls to direct |release|, since the |addSubview:| takes ownership. Done. So it will be released when subview goes out of scope, right? https://codereview.chromium.org/2826233002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/subresource_filter/subresource_filter_bubble_controller.mm:79: - (id)initWithModel:(ContentSettingBubbleModel*)contentSettingBubbleModel On 2017/05/03 at 22:05:11, Robert Sesek wrote: > method ordering: Move this to be the first method done https://codereview.chromium.org/2826233002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/subresource_filter/subresource_filter_bubble_controller.mm:105: - (IBAction)manageCheckboxChecked:(id)sender { On 2017/05/03 at 22:05:11, Robert Sesek wrote: > Similar to |IBOutlet|, you can drop the |IBAction|. done
The latest patch also includes tests.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Just a few comments in the test. Very nice work! https://codereview.chromium.org/2826233002/diff/100001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/content_settings/content_setting_bubble_cocoa_browsertest.mm (right): https://codereview.chromium.org/2826233002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/content_settings/content_setting_bubble_cocoa_browsertest.mm:134: (SubresourceFilterBubbleController*)controller; C-style casts are banned in Chromium, but base::mac::ObjCCast will work here. https://codereview.chromium.org/2826233002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/content_settings/content_setting_bubble_cocoa_browsertest.mm:139: EXPECT_TRUE( Use EXPECT_NSEQ to get a better failure message if the expectation trips (it'll print the actual vs. expected values, just like EXPECT_EQ). https://codereview.chromium.org/2826233002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/content_settings/content_setting_bubble_cocoa_browsertest.mm:161: IN_PROC_BROWSER_TEST_F(ContentSettingBubbleControllerTest, Same comments above as in this test. https://codereview.chromium.org/2826233002/diff/100001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/subresource_filter/subresource_filter_bubble_controller.h (right): https://codereview.chromium.org/2826233002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/subresource_filter/subresource_filter_bubble_controller.h:19: @interface SubresourceFilterBubbleController : ContentSettingBubbleController { nit: since there are no public instance variables, you can omit the curly braces
The CQ bit was checked by shivanisha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL, thanks! https://codereview.chromium.org/2826233002/diff/100001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/content_settings/content_setting_bubble_cocoa_browsertest.mm (right): https://codereview.chromium.org/2826233002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/content_settings/content_setting_bubble_cocoa_browsertest.mm:134: (SubresourceFilterBubbleController*)controller; On 2017/05/05 at 16:53:34, Robert Sesek wrote: > C-style casts are banned in Chromium, but base::mac::ObjCCast will work here. done https://codereview.chromium.org/2826233002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/content_settings/content_setting_bubble_cocoa_browsertest.mm:139: EXPECT_TRUE( On 2017/05/05 at 16:53:34, Robert Sesek wrote: > Use EXPECT_NSEQ to get a better failure message if the expectation trips (it'll print the actual vs. expected values, just like EXPECT_EQ). done https://codereview.chromium.org/2826233002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/content_settings/content_setting_bubble_cocoa_browsertest.mm:161: IN_PROC_BROWSER_TEST_F(ContentSettingBubbleControllerTest, On 2017/05/05 at 16:53:34, Robert Sesek wrote: > Same comments above as in this test. done https://codereview.chromium.org/2826233002/diff/100001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/subresource_filter/subresource_filter_bubble_controller.h (right): https://codereview.chromium.org/2826233002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/subresource_filter/subresource_filter_bubble_controller.h:19: @interface SubresourceFilterBubbleController : ContentSettingBubbleController { On 2017/05/05 at 16:53:34, Robert Sesek wrote: > nit: since there are no public instance variables, you can omit the curly braces done
LGTM!
On 2017/05/05 at 18:25:54, rsesek wrote: > LGTM! rsesek@, Thanks for the review!
Nice work Shivani! A few small nits and would you mind fixing the typo in the subject line? LGTM https://codereview.chromium.org/2826233002/diff/120001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/subresource_filter/subresource_filter_bubble_controller.h (right): https://codereview.chromium.org/2826233002/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/subresource_filter/subresource_filter_bubble_controller.h:16: // filtered and presents the checkbox to enable those resources. If the check s/enable those resources/reload the page https://codereview.chromium.org/2826233002/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/subresource_filter/subresource_filter_bubble_controller.h:18: // the ability to reload the page with the content we've blocked previously. s/with the content we've blocked previously/with filtering disabled/
On 2017/05/05 18:59:31, Charlie Harrison wrote: > Nice work Shivani! > > A few small nits and would you mind fixing the typo in the subject line? Oh, on that point as well: the "subject" line in Rietveld does not make it into the commit message in git. So please copy the subject to be the first line of the description.
Description was changed from ========== - Replaces xib based layout with code based layout. - Updates the UI as per the latest mocks. BUG=689992 ========== to ========== [subresource_filter] Mac UI updation and xib replaced with code based layout. - Replaces xib based layout with code based layout. - Updates the UI as per the latest mocks. BUG=689992 ==========
Description was changed from ========== [subresource_filter] Mac UI updation and xib replaced with code based layout. - Replaces xib based layout with code based layout. - Updates the UI as per the latest mocks. BUG=689992 ========== to ========== [subresource_filter] Mac UI update and xib replaced with code based layout. - Replaces xib based layout with code based layout. - Updates the UI as per the latest mocks. BUG=689992 ==========
Description was changed from ========== [subresource_filter] Mac UI update and xib replaced with code based layout. - Replaces xib based layout with code based layout. - Updates the UI as per the latest mocks. BUG=689992 ========== to ========== [subresource_filter] Mac UI updated and xib replaced with code based layout. - Replaces xib based layout with code based layout. - Updates the UI as per the latest mocks. BUG=689992 ==========
Thanks csharrison!
https://codereview.chromium.org/2826233002/diff/120001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/subresource_filter/subresource_filter_bubble_controller.h (right): https://codereview.chromium.org/2826233002/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/subresource_filter/subresource_filter_bubble_controller.h:16: // filtered and presents the checkbox to enable those resources. If the check On 2017/05/05 at 18:59:31, Charlie Harrison wrote: > s/enable those resources/reload the page done https://codereview.chromium.org/2826233002/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/subresource_filter/subresource_filter_bubble_controller.h:18: // the ability to reload the page with the content we've blocked previously. On 2017/05/05 at 18:59:31, Charlie Harrison wrote: > s/with the content we've blocked previously/with filtering disabled/ done https://codereview.chromium.org/2826233002/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/subresource_filter/subresource_filter_bubble_controller.h:18: // the ability to reload the page with the content we've blocked previously. On 2017/05/05 at 18:59:31, Charlie Harrison wrote: > s/with the content we've blocked previously/with filtering disabled/ done
The CQ bit was checked by shivanisha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by shivanisha@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org, csharrison@chromium.org Link to the patchset: https://codereview.chromium.org/2826233002/#ps140001 (title: "csharrison feedback addressed")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by shivanisha@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1494252693500710,
"parent_rev": "da7c77096c29ccd804917018dc74bebd925e164a", "commit_rev":
"da2a48d9f7403a1fbee3a5984d442a49ef14a01c"}
Message was sent while issue was closed.
Description was changed from ========== [subresource_filter] Mac UI updated and xib replaced with code based layout. - Replaces xib based layout with code based layout. - Updates the UI as per the latest mocks. BUG=689992 ========== to ========== [subresource_filter] Mac UI updated and xib replaced with code based layout. - Replaces xib based layout with code based layout. - Updates the UI as per the latest mocks. BUG=689992 Review-Url: https://codereview.chromium.org/2826233002 Cr-Commit-Position: refs/heads/master@{#469979} Committed: https://chromium.googlesource.com/chromium/src/+/da2a48d9f7403a1fbee3a5984d44... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:140001) as https://chromium.googlesource.com/chromium/src/+/da2a48d9f7403a1fbee3a5984d44... |
