Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1473393003:
Add chooser permission UI code for Mac (Closed)
|
Created:
5 years ago by juncai Modified:
5 years ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, msramek+watch_chromium.org, tfarina, jam, raymes+watch_chromium.org, darin-cc_chromium.org, mkwst+moarreviews-renderer_chromium.org, markusheintz_ Base URL:
https://chromium.googlesource.com/chromium/src.git@wpu_1 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd chooser permission UI code for Mac
This patch added chooser permission UI code for Mac.
BUG=492204
Committed: https://crrev.com/f68bb8302e9140e9f521511fe723de4f4278418f
Cr-Commit-Position: refs/heads/master@{#364857}
Patch Set 1 : added chooser permission UI code for Mac #Patch Set 2 : merged changes from master to fix merge conflicts #
Total comments: 12
Patch Set 3 : address reillyg@'s comments #Patch Set 4 : modified cocoa code since Close() function was added at ChooserBubbleDelegate #Patch Set 5 : updated code to calculate width for table column #
Total comments: 6
Patch Set 6 : merged changes from upstream, address reillyg@'s comments #
Total comments: 4
Patch Set 7 : address reillyg@'s comments #
Total comments: 22
Patch Set 8 : address felt@'s comments #Patch Set 9 : address palmer@'s comments #
Total comments: 2
Patch Set 10 : address palmer@'s comments #
Total comments: 14
Messages
Total messages: 29 (8 generated)
juncai@chromium.org changed reviewers: + reillyg@chromium.org
Please review this patch.
Thank you for your patience. I should have looked at this earlier. https://codereview.chromium.org/1473393003/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/website_settings/chooser_bubble_ui_cocoa.mm (right): https://codereview.chromium.org/1473393003/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/chooser_bubble_ui_cocoa.mm:355: // that was previously selected, if so, disselect it. s/disselect/deselect/ https://codereview.chromium.org/1473393003/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/chooser_bubble_ui_cocoa.mm:369: } There might be some obscure bool -> BOOL conversion rule that makes this invalid but I'd rather see: [tableView_ setEnabled:!device_names.empty()]; https://codereview.chromium.org/1473393003/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/chooser_bubble_ui_cocoa.mm:378: [connectButton_ setEnabled:YES]; This one will definitely work: [connectButton_ setEnabled:[tableView_ numberofSelectedRows] > 0]; https://codereview.chromium.org/1473393003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/website_settings/chooser_bubble_ui_view_creator.cc (right): https://codereview.chromium.org/1473393003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/chooser_bubble_ui_view_creator.cc:10: scoped_ptr<BubbleUi> ChooserBubbleDelegate::CreateBubbleUi( No need to put these in separate files. Just implement ChooserBubbleDelegate::BuildBubbleUi in chooser_bubble_ui_view.cc and chooser_bubble_ui_cocoa.mm. https://codereview.chromium.org/1473393003/diff/20001/chrome/browser/ui/websi... File chrome/browser/ui/website_settings/chooser_bubble_delegate.cc (right): https://codereview.chromium.org/1473393003/diff/20001/chrome/browser/ui/websi... chrome/browser/ui/website_settings/chooser_bubble_delegate.cc:21: return bubble_ui.Pass(); No need for a local, just: return CreateBubbleUi(browser_, chooser_options_.get(), this); https://codereview.chromium.org/1473393003/diff/20001/chrome/browser/ui/websi... File chrome/browser/ui/website_settings/chooser_bubble_delegate.h (right): https://codereview.chromium.org/1473393003/diff/20001/chrome/browser/ui/websi... chrome/browser/ui/website_settings/chooser_bubble_delegate.h:32: Browser* browser_; Require subclasses to pass this to the constructor instead of making it a protected field.
https://codereview.chromium.org/1473393003/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/website_settings/chooser_bubble_ui_cocoa.mm (right): https://codereview.chromium.org/1473393003/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/chooser_bubble_ui_cocoa.mm:355: // that was previously selected, if so, disselect it. On 2015/12/02 19:25:53, Reilly Grant wrote: > s/disselect/deselect/ Done. https://codereview.chromium.org/1473393003/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/chooser_bubble_ui_cocoa.mm:369: } On 2015/12/02 19:25:53, Reilly Grant wrote: > There might be some obscure bool -> BOOL conversion rule that makes this invalid > but I'd rather see: > > [tableView_ setEnabled:!device_names.empty()]; Done. https://codereview.chromium.org/1473393003/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/chooser_bubble_ui_cocoa.mm:378: [connectButton_ setEnabled:YES]; On 2015/12/02 19:25:53, Reilly Grant wrote: > This one will definitely work: > > [connectButton_ setEnabled:[tableView_ numberofSelectedRows] > 0]; Done. https://codereview.chromium.org/1473393003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/website_settings/chooser_bubble_ui_view_creator.cc (right): https://codereview.chromium.org/1473393003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/chooser_bubble_ui_view_creator.cc:10: scoped_ptr<BubbleUi> ChooserBubbleDelegate::CreateBubbleUi( On 2015/12/02 19:25:53, Reilly Grant wrote: > No need to put these in separate files. Just implement > ChooserBubbleDelegate::BuildBubbleUi in chooser_bubble_ui_view.cc and > chooser_bubble_ui_cocoa.mm. Done. https://codereview.chromium.org/1473393003/diff/20001/chrome/browser/ui/websi... File chrome/browser/ui/website_settings/chooser_bubble_delegate.cc (right): https://codereview.chromium.org/1473393003/diff/20001/chrome/browser/ui/websi... chrome/browser/ui/website_settings/chooser_bubble_delegate.cc:21: return bubble_ui.Pass(); On 2015/12/02 19:25:53, Reilly Grant wrote: > No need for a local, just: > > return CreateBubbleUi(browser_, chooser_options_.get(), this); Done. https://codereview.chromium.org/1473393003/diff/20001/chrome/browser/ui/websi... File chrome/browser/ui/website_settings/chooser_bubble_delegate.h (right): https://codereview.chromium.org/1473393003/diff/20001/chrome/browser/ui/websi... chrome/browser/ui/website_settings/chooser_bubble_delegate.h:32: Browser* browser_; On 2015/12/02 19:25:53, Reilly Grant wrote: > Require subclasses to pass this to the constructor instead of making it a > protected field. Done.
https://codereview.chromium.org/1473393003/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/website_settings/chooser_bubble_ui_cocoa.mm (right): https://codereview.chromium.org/1473393003/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/chooser_bubble_ui_cocoa.mm:59: return bubble_ui.Pass(); You can shorten this to: return make_scoped_ptr(new ChooserBubbleUiCocoa( browser, chooser_options, chooser_bubble_delegate)); https://codereview.chromium.org/1473393003/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/chooser_bubble_ui_cocoa.mm:228: titleView_.reset([[self bubbleTitle] retain]); retain should not be necessary when assigning to a scoped_nsobject here and below. https://codereview.chromium.org/1473393003/diff/80001/chrome/browser/ui/websi... File chrome/browser/ui/website_settings/chooser_bubble_delegate.h (right): https://codereview.chromium.org/1473393003/diff/80001/chrome/browser/ui/websi... chrome/browser/ui/website_settings/chooser_bubble_delegate.h:26: ChooserBubbleDelegate* chooser_bubble_delegate); This function does not need to be static. If it is made non-static then the |chooser_bubble_delegate| parameter will be unnecessary.
https://codereview.chromium.org/1473393003/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/website_settings/chooser_bubble_ui_cocoa.mm (right): https://codereview.chromium.org/1473393003/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/chooser_bubble_ui_cocoa.mm:59: return bubble_ui.Pass(); On 2015/12/08 19:26:57, Reilly Grant wrote: > You can shorten this to: > > return make_scoped_ptr(new ChooserBubbleUiCocoa( > browser, chooser_options, chooser_bubble_delegate)); Done. https://codereview.chromium.org/1473393003/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/chooser_bubble_ui_cocoa.mm:228: titleView_.reset([[self bubbleTitle] retain]); On 2015/12/08 19:26:57, Reilly Grant wrote: > retain should not be necessary when assigning to a scoped_nsobject here and > below. Done. https://codereview.chromium.org/1473393003/diff/80001/chrome/browser/ui/websi... File chrome/browser/ui/website_settings/chooser_bubble_delegate.h (right): https://codereview.chromium.org/1473393003/diff/80001/chrome/browser/ui/websi... chrome/browser/ui/website_settings/chooser_bubble_delegate.h:26: ChooserBubbleDelegate* chooser_bubble_delegate); On 2015/12/08 19:26:57, Reilly Grant wrote: > This function does not need to be static. If it is made non-static then the > |chooser_bubble_delegate| parameter will be unnecessary. Done.
lgtm with nits https://codereview.chromium.org/1473393003/diff/100001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/website_settings/chooser_bubble_ui_cocoa.mm (right): https://codereview.chromium.org/1473393003/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/website_settings/chooser_bubble_ui_cocoa.mm:125: // Returns an autoreleased NSButton displaying a cancel button. nit: This comment is out of date and not very useful because the return type is documented by the signature. I suggest "Creates the "connect" button." Similar for above. https://codereview.chromium.org/1473393003/diff/100001/chrome/browser/usb/usb... File chrome/browser/usb/usb_chooser_bubble_delegate.cc (right): https://codereview.chromium.org/1473393003/diff/100001/chrome/browser/usb/usb... chrome/browser/usb/usb_chooser_bubble_delegate.cc:56: DCHECK(browser); nit: This DCHECK isn't really necessary because the parent class does it too and this constructor doesn't access |browser|.
juncai@chromium.org changed reviewers: + felt@chromium.org, palmer@chromium.org
felt@chromium.org: Please review changes in chrome/browser/ui/views/ chrome/browser/ui/website_settings/ palmer@chromium.org: Please review changes in chrome/browser/ui/cocoa/website_settings/
https://codereview.chromium.org/1473393003/diff/100001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/website_settings/chooser_bubble_ui_cocoa.mm (right): https://codereview.chromium.org/1473393003/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/website_settings/chooser_bubble_ui_cocoa.mm:125: // Returns an autoreleased NSButton displaying a cancel button. On 2015/12/09 18:56:41, Reilly Grant wrote: > nit: This comment is out of date and not very useful because the return type is > documented by the signature. I suggest "Creates the "connect" button." Similar > for above. Done. https://codereview.chromium.org/1473393003/diff/100001/chrome/browser/usb/usb... File chrome/browser/usb/usb_chooser_bubble_delegate.cc (right): https://codereview.chromium.org/1473393003/diff/100001/chrome/browser/usb/usb... chrome/browser/usb/usb_chooser_bubble_delegate.cc:56: DCHECK(browser); On 2015/12/09 18:56:41, Reilly Grant wrote: > nit: This DCHECK isn't really necessary because the parent class does it too and > this constructor doesn't access |browser|. Done.
https://codereview.chromium.org/1473393003/diff/120001/chrome/browser/ui/webs... File chrome/browser/ui/website_settings/chooser_bubble_delegate.cc (right): https://codereview.chromium.org/1473393003/diff/120001/chrome/browser/ui/webs... chrome/browser/ui/website_settings/chooser_bubble_delegate.cc:11: DCHECK(browser_); suggestion: move this DCHECK closer to where |browser| is used https://codereview.chromium.org/1473393003/diff/120001/chrome/browser/usb/usb... File chrome/browser/usb/usb_chooser_bubble_delegate.cc (left): https://codereview.chromium.org/1473393003/diff/120001/chrome/browser/usb/usb... chrome/browser/usb/usb_chooser_bubble_delegate.cc:83: scoped_ptr<BubbleUi> UsbChooserBubbleDelegate::BuildBubbleUi() { why add this in https://codereview.chromium.org/1408193003/ and then immediately delete it here?
https://codereview.chromium.org/1473393003/diff/120001/chrome/browser/ui/webs... File chrome/browser/ui/website_settings/chooser_bubble_delegate.cc (right): https://codereview.chromium.org/1473393003/diff/120001/chrome/browser/ui/webs... chrome/browser/ui/website_settings/chooser_bubble_delegate.cc:11: DCHECK(browser_); On 2015/12/10 02:17:24, felt wrote: > suggestion: move this DCHECK closer to where |browser| is used Done. https://codereview.chromium.org/1473393003/diff/120001/chrome/browser/usb/usb... File chrome/browser/usb/usb_chooser_bubble_delegate.cc (left): https://codereview.chromium.org/1473393003/diff/120001/chrome/browser/usb/usb... chrome/browser/usb/usb_chooser_bubble_delegate.cc:83: scoped_ptr<BubbleUi> UsbChooserBubbleDelegate::BuildBubbleUi() { On 2015/12/10 02:17:24, felt wrote: > why add this in https://codereview.chromium.org/1408193003/ and then immediately > delete it here? Done.
https://codereview.chromium.org/1473393003/diff/120001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/website_settings/chooser_bubble_ui_cocoa.h (right): https://codereview.chromium.org/1473393003/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/website_settings/chooser_bubble_ui_cocoa.h:20: // permission. It can be used by WebUsb, WebBluetooth. Nits: The first sentence should have the 2 clauses joined by a semicolon, or should be 2 sentences. Is table view a specific class? If so, spell it the way the class is named ("... uses |TableView|..." or whatever). "WebUSB or WebBluetooth" (i.e the names of the APIs) or "|WebUsb| or |WebBluetooth|" (if those are the names of classes). https://codereview.chromium.org/1473393003/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/website_settings/chooser_bubble_ui_cocoa.h:38: // Called when |chooser_bubble_ui_controller_| is closing. Passive voice/leaving out the agent is confusing here. What code calls it? https://codereview.chromium.org/1473393003/diff/120001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/website_settings/chooser_bubble_ui_cocoa.mm (right): https://codereview.chromium.org/1473393003/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/website_settings/chooser_bubble_ui_cocoa.mm:90: // Returns true of the browser has support for the location bar. Typo: "true if" https://codereview.chromium.org/1473393003/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/website_settings/chooser_bubble_ui_cocoa.mm:94: // Update table view when chooser options were initialized. Nit: This one and those below: "Updates |tableView_| when..." https://codereview.chromium.org/1473393003/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/website_settings/chooser_bubble_ui_cocoa.mm:128: // Called when the 'Connect' button is pressed. Nit: Consistently use either double or single quotes; consistently capitalize Close. https://codereview.chromium.org/1473393003/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/website_settings/chooser_bubble_ui_cocoa.mm:145: DCHECK(browser); According to the documentation, |browser| and |bridge| must be non-nil. So in addition to DCHECK, maybe return early or do something else if a Release build passes nil? https://codereview.chromium.org/1473393003/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/website_settings/chooser_bubble_ui_cocoa.mm:184: DCHECK(bridge_); Are these DCHECKs redundant, given the checks in the designated initializer? https://codereview.chromium.org/1473393003/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/website_settings/chooser_bubble_ui_cocoa.mm:226: // 'x' button in the upper-right-hand corner. Do we need an X button, given that we also have Cancel? https://codereview.chromium.org/1473393003/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/website_settings/chooser_bubble_ui_cocoa.mm:328: if (rowIndex >= 0 && rowIndex < static_cast<int>(device_names.size())) { int nit: should be static_cast<NSInteger>.
https://codereview.chromium.org/1473393003/diff/120001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/website_settings/chooser_bubble_ui_cocoa.h (right): https://codereview.chromium.org/1473393003/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/website_settings/chooser_bubble_ui_cocoa.h:20: // permission. It can be used by WebUsb, WebBluetooth. On 2015/12/10 23:26:10, palmer wrote: > Nits: The first sentence should have the 2 clauses joined by a semicolon, or > should be 2 sentences. > > Is table view a specific class? If so, spell it the way the class is named ("... > uses |TableView|..." or whatever). > > "WebUSB or WebBluetooth" (i.e the names of the APIs) or "|WebUsb| or > |WebBluetooth|" (if those are the names of classes). Done. https://codereview.chromium.org/1473393003/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/website_settings/chooser_bubble_ui_cocoa.h:38: // Called when |chooser_bubble_ui_controller_| is closing. On 2015/12/10 23:26:10, palmer wrote: > Passive voice/leaving out the agent is confusing here. What code calls it? ChooserBubbleUiController's windowWillClose calls it when the window needs to be closed. It is similar to: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... and it is called here: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... https://codereview.chromium.org/1473393003/diff/120001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/website_settings/chooser_bubble_ui_cocoa.mm (right): https://codereview.chromium.org/1473393003/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/website_settings/chooser_bubble_ui_cocoa.mm:90: // Returns true of the browser has support for the location bar. On 2015/12/10 23:26:10, palmer wrote: > Typo: "true if" Done. https://codereview.chromium.org/1473393003/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/website_settings/chooser_bubble_ui_cocoa.mm:94: // Update table view when chooser options were initialized. On 2015/12/10 23:26:10, palmer wrote: > Nit: This one and those below: "Updates |tableView_| when..." Done. https://codereview.chromium.org/1473393003/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/website_settings/chooser_bubble_ui_cocoa.mm:128: // Called when the 'Connect' button is pressed. On 2015/12/10 23:26:10, palmer wrote: > Nit: Consistently use either double or single quotes; consistently capitalize > Close. Done. https://codereview.chromium.org/1473393003/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/website_settings/chooser_bubble_ui_cocoa.mm:145: DCHECK(browser); On 2015/12/10 23:26:10, palmer wrote: > According to the documentation, |browser| and |bridge| must be non-nil. So in > addition to DCHECK, maybe return early or do something else if a Release build > passes nil? Done. https://codereview.chromium.org/1473393003/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/website_settings/chooser_bubble_ui_cocoa.mm:184: DCHECK(bridge_); On 2015/12/10 23:26:10, palmer wrote: > Are these DCHECKs redundant, given the checks in the designated initializer? Done. https://codereview.chromium.org/1473393003/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/website_settings/chooser_bubble_ui_cocoa.mm:226: // 'x' button in the upper-right-hand corner. On 2015/12/10 23:26:10, palmer wrote: > Do we need an X button, given that we also have Cancel? Done. https://codereview.chromium.org/1473393003/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/website_settings/chooser_bubble_ui_cocoa.mm:328: if (rowIndex >= 0 && rowIndex < static_cast<int>(device_names.size())) { On 2015/12/10 23:26:10, palmer wrote: > int nit: should be static_cast<NSInteger>. Done.
LGTM. Thanks! https://codereview.chromium.org/1473393003/diff/160001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/website_settings/chooser_bubble_ui_cocoa.h (right): https://codereview.chromium.org/1473393003/diff/160001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/website_settings/chooser_bubble_ui_cocoa.h:20: // permission. It can be used by "WebUsb", "WebBluetooth". Nit: "It can be used by the WebUSB or WebBluetooth APIs."
https://codereview.chromium.org/1473393003/diff/160001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/website_settings/chooser_bubble_ui_cocoa.h (right): https://codereview.chromium.org/1473393003/diff/160001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/website_settings/chooser_bubble_ui_cocoa.h:20: // permission. It can be used by "WebUsb", "WebBluetooth". On 2015/12/11 21:35:57, palmer wrote: > Nit: "It can be used by the WebUSB or WebBluetooth APIs." Done.
The CQ bit was checked by juncai@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from reillyg@chromium.org, palmer@chromium.org Link to the patchset: https://codereview.chromium.org/1473393003/#ps180001 (title: "address plamer@'s comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1473393003/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1473393003/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by juncai@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1473393003/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1473393003/180001
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Add chooser permission UI code for Mac This patch added chooser permission UI code for Mac. BUG=492204 ========== to ========== Add chooser permission UI code for Mac This patch added chooser permission UI code for Mac. BUG=492204 Committed: https://crrev.com/f68bb8302e9140e9f521511fe723de4f4278418f Cr-Commit-Position: refs/heads/master@{#364857} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/f68bb8302e9140e9f521511fe723de4f4278418f Cr-Commit-Position: refs/heads/master@{#364857}
Message was sent while issue was closed.
rsesek@chromium.org changed reviewers: + rsesek@chromium.org
Message was sent while issue was closed.
Some drive-by comments, including a leak that could cause crashes. palmer@: For non-trivial Cocoa patches, there really should be someone from cocoa/OWNERS approving this, per https://codereview.chromium.org/1308983002#msg15. https://codereview.chromium.org/1473393003/diff/180001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/website_settings/chooser_bubble_ui_cocoa.mm (right): https://codereview.chromium.org/1473393003/diff/180001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/website_settings/chooser_bubble_ui_cocoa.mm:70: ChooserBubbleDelegate* chooser_bubble_delegate_; // Weak. naming: chooserBubbleDelegate_ https://codereview.chromium.org/1473393003/diff/180001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/website_settings/chooser_bubble_ui_cocoa.mm:76: (ChooserBubbleDelegate*)chooser_bubble_delegate naming: chooserBubbleDelegate_ https://codereview.chromium.org/1473393003/diff/180001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/website_settings/chooser_bubble_ui_cocoa.mm:142: if (browser == nil || chooser_bubble_delegate == nil || bridge == nil) This is also DCHECK'd, so I'd remove this. https://codereview.chromium.org/1473393003/diff/180001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/website_settings/chooser_bubble_ui_cocoa.mm:163: [center addObserver:self This observer is leaked and should be removed either in -dealloc or windowWillClose:. https://codereview.chromium.org/1473393003/diff/180001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/website_settings/chooser_bubble_ui_cocoa.mm:191: [view setSubviews:@[]]; What's the rationale behind this? https://codereview.chromium.org/1473393003/diff/180001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/website_settings/chooser_bubble_ui_cocoa.mm:366: LocationBarViewMac* location_bar = naming: locationBar https://codereview.chromium.org/1473393003/diff/180001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/website_settings/chooser_bubble_ui_cocoa.mm:474: if (chooser_bubble_ui_controller_) { You don't need to nil-check ObjC objects, as messaging nil is a no-op.
Message was sent while issue was closed.
Made changes in another patch: https://codereview.chromium.org/1528543004/ https://codereview.chromium.org/1473393003/diff/180001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/website_settings/chooser_bubble_ui_cocoa.mm (right): https://codereview.chromium.org/1473393003/diff/180001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/website_settings/chooser_bubble_ui_cocoa.mm:70: ChooserBubbleDelegate* chooser_bubble_delegate_; // Weak. On 2015/12/14 23:13:48, Robert Sesek wrote: > naming: chooserBubbleDelegate_ Done. https://codereview.chromium.org/1473393003/diff/180001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/website_settings/chooser_bubble_ui_cocoa.mm:76: (ChooserBubbleDelegate*)chooser_bubble_delegate On 2015/12/14 23:13:48, Robert Sesek wrote: > naming: chooserBubbleDelegate_ Done. https://codereview.chromium.org/1473393003/diff/180001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/website_settings/chooser_bubble_ui_cocoa.mm:142: if (browser == nil || chooser_bubble_delegate == nil || bridge == nil) On 2015/12/14 23:13:48, Robert Sesek wrote: > This is also DCHECK'd, so I'd remove this. Done. https://codereview.chromium.org/1473393003/diff/180001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/website_settings/chooser_bubble_ui_cocoa.mm:163: [center addObserver:self On 2015/12/14 23:13:48, Robert Sesek wrote: > This observer is leaked and should be removed either in -dealloc or > windowWillClose:. Done. https://codereview.chromium.org/1473393003/diff/180001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/website_settings/chooser_bubble_ui_cocoa.mm:191: [view setSubviews:@[]]; On 2015/12/14 23:13:48, Robert Sesek wrote: > What's the rationale behind this? Done. https://codereview.chromium.org/1473393003/diff/180001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/website_settings/chooser_bubble_ui_cocoa.mm:366: LocationBarViewMac* location_bar = On 2015/12/14 23:13:48, Robert Sesek wrote: > naming: locationBar Done. https://codereview.chromium.org/1473393003/diff/180001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/website_settings/chooser_bubble_ui_cocoa.mm:474: if (chooser_bubble_ui_controller_) { On 2015/12/14 23:13:48, Robert Sesek wrote: > You don't need to nil-check ObjC objects, as messaging nil is a no-op. Done. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
