Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2257743003:
Add signal strength indicator icon to WebBluetooth chooser on Mac (Closed)
|
Created:
4 years, 4 months ago by juncai Modified:
4 years, 3 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, tfarina, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@bluetooth_signal_strength Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd signal strength indicator icon to WebBluetooth chooser on Mac
I uploaded a screenshot on the issue page.
BUG=629689
Committed: https://crrev.com/8b985248b1ce2570d6ec84b211b1de14247127d1
Cr-Commit-Position: refs/heads/master@{#414599}
Patch Set 1 : added signal strength indicator icon to WebBluetooth chooser on Mac #
Total comments: 26
Patch Set 2 : address comments #Patch Set 3 : address more comments #
Total comments: 8
Patch Set 4 : address more comments #Messages
Total messages: 36 (22 generated)
The CQ bit was checked by juncai@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...
juncai@chromium.org changed reviewers: + jyasskin@chromium.org, msw@chromium.org, rsesek@chromium.org
rsesek@chromium.org: Please review changes in //chrome/browser/ui/cocoa/ msw@chromium.org: Please review changes in //chrome/browser/ui/views/ jyasskin@chromium.org: Please review changes in //chrome/browser/chooser_controller/
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2257743003/diff/1/chrome/browser/chooser_cont... File chrome/browser/chooser_controller/mock_chooser_controller.cc (left): https://codereview.chromium.org/2257743003/diff/1/chrome/browser/chooser_cont... chrome/browser/chooser_controller/mock_chooser_controller.cc:99: auto it = std::find(option_names_.begin(), option_names_.end(), option_name); Replace this with find_if and a lambda, instead of switching to a for loop. https://codereview.chromium.org/2257743003/diff/1/chrome/browser/ui/cocoa/cho... File chrome/browser/ui/cocoa/chooser_content_view_cocoa.mm (right): https://codereview.chromium.org/2257743003/diff/1/chrome/browser/ui/cocoa/cho... chrome/browser/ui/cocoa/chooser_content_view_cocoa.mm:54: const int signalStrengthLevelImageIds[5] = {IDR_SIGNAL_0_BAR, IDR_SIGNAL_1_BAR, Use kConstant naming: https://google.github.io/styleguide/cppguide.html#Constant_Names https://codereview.chromium.org/2257743003/diff/1/chrome/browser/ui/cocoa/cho... chrome/browser/ui/cocoa/chooser_content_view_cocoa.mm:115: kVerticalPadding, Shouldn't this be kHorizontalPadding? https://codereview.chromium.org/2257743003/diff/1/chrome/browser/ui/cocoa/cho... chrome/browser/ui/cocoa/chooser_content_view_cocoa.mm:495: if (chooserController_->ShouldShowIconBeforeText() && numOptions > 0) { Dumb question: why is createTableRowView called with a particular rowIndex if there are no options? https://codereview.chromium.org/2257743003/diff/1/chrome/browser/ui/cocoa/cho... chrome/browser/ui/cocoa/chooser_content_view_cocoa.mm:504: initWithText:[self optionAtIndex:rowIndex] This indentation looks kinda weird. https://codereview.chromium.org/2257743003/diff/1/chrome/browser/ui/cocoa/ext... File chrome/browser/ui/cocoa/extensions/chooser_dialog_cocoa_controller.mm (right): https://codereview.chromium.org/2257743003/diff/1/chrome/browser/ui/cocoa/ext... chrome/browser/ui/cocoa/extensions/chooser_dialog_cocoa_controller.mm:49: viewForTableColumn:(NSTableColumn*)tableColumn Another dumb question: why does this take a tableView and tableColumn that it doesn't use? https://codereview.chromium.org/2257743003/diff/1/chrome/browser/ui/cocoa/ext... File chrome/browser/ui/cocoa/extensions/chooser_dialog_cocoa_controller_unittest.mm (right): https://codereview.chromium.org/2257743003/diff/1/chrome/browser/ui/cocoa/ext... chrome/browser/ui/cocoa/extensions/chooser_dialog_cocoa_controller_unittest.mm:130: CheckTableRowView( I'm not a big fan of helper functions like this, because the call doesn't say what it's checking. Instead, you have two options: 1) Write a couple separate helper functions to pull the image and text out of a row of table_view_, and a helper function to get the image for a given signal strength, and write the assertions against the image and text inline in the function. 2) Write functions of the form ExpectSignalStrengthImageIs(table_view_, row, 3) and ExpectRowTextIs(table_view_, row, "Text") where it's clear exactly what the expectation is. https://codereview.chromium.org/2257743003/diff/1/chrome/browser/ui/cocoa/ext... chrome/browser/ui/cocoa/extensions/chooser_dialog_cocoa_controller_unittest.mm:176: EXPECT_EQ(4, table_view_.numberOfRows); For these tests that are trying to check that the right image is being used for all 5 levels, I think you can skip all the numberOfRows…selectedRow assertions. Just check the image. https://codereview.chromium.org/2257743003/diff/1/chrome/browser/ui/cocoa/ext... chrome/browser/ui/cocoa/extensions/chooser_dialog_cocoa_controller_unittest.mm:718: CheckTableRowView(0, MockChooserController::kSignalStrengthLevel2Bar, @"d"); You don't need to add signal strength icon assertions to every test. Most can just check that the right row is being used using the row's text.
Description was changed from ========== Add signal strength indicator icon to WebBluetooth chooser on Mac This CL modified code to add signal strength indicator icon to WebBluetooth chooser. I uploaded a screenshot on the issue page. BUG=629689 ========== to ========== Add signal strength indicator icon to WebBluetooth chooser on Mac I uploaded a screenshot on the issue page. BUG=629689 ==========
The CQ bit was checked by juncai@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/2257743003/diff/1/chrome/browser/chooser_cont... File chrome/browser/chooser_controller/mock_chooser_controller.cc (left): https://codereview.chromium.org/2257743003/diff/1/chrome/browser/chooser_cont... chrome/browser/chooser_controller/mock_chooser_controller.cc:99: auto it = std::find(option_names_.begin(), option_names_.end(), option_name); On 2016/08/19 18:30:05, Jeffrey Yasskin wrote: > Replace this with find_if and a lambda, instead of switching to a for loop. Done. https://codereview.chromium.org/2257743003/diff/1/chrome/browser/ui/cocoa/cho... File chrome/browser/ui/cocoa/chooser_content_view_cocoa.mm (right): https://codereview.chromium.org/2257743003/diff/1/chrome/browser/ui/cocoa/cho... chrome/browser/ui/cocoa/chooser_content_view_cocoa.mm:54: const int signalStrengthLevelImageIds[5] = {IDR_SIGNAL_0_BAR, IDR_SIGNAL_1_BAR, On 2016/08/19 18:30:05, Jeffrey Yasskin wrote: > Use kConstant naming: > https://google.github.io/styleguide/cppguide.html#Constant_Names Done. https://codereview.chromium.org/2257743003/diff/1/chrome/browser/ui/cocoa/cho... chrome/browser/ui/cocoa/chooser_content_view_cocoa.mm:115: kVerticalPadding, On 2016/08/19 18:30:05, Jeffrey Yasskin wrote: > Shouldn't this be kHorizontalPadding? Done. https://codereview.chromium.org/2257743003/diff/1/chrome/browser/ui/cocoa/cho... chrome/browser/ui/cocoa/chooser_content_view_cocoa.mm:495: if (chooserController_->ShouldShowIconBeforeText() && numOptions > 0) { On 2016/08/19 18:30:05, Jeffrey Yasskin wrote: > Dumb question: why is createTableRowView called with a particular rowIndex if > there are no options? If numOptions == 0, the actual number rows is 1 since it needs to display "Bluetooth devices not found" or "Devices not found" in the table view, so still need to call createTableRowView to create a row, and a DCHECK checks the rowIndex is always 0 in this case: https://cs.chromium.org/chromium/src/chrome/browser/ui/cocoa/chooser_content_... https://codereview.chromium.org/2257743003/diff/1/chrome/browser/ui/cocoa/cho... chrome/browser/ui/cocoa/chooser_content_view_cocoa.mm:504: initWithText:[self optionAtIndex:rowIndex] On 2016/08/19 18:30:05, Jeffrey Yasskin wrote: > This indentation looks kinda weird. I used "git cl format" to format it. I guess here the two ":" need to be aligned, and "signalStrengthLevel:level]);" has longer string length before the ":" than "initWithText:[self optionAtIndex:rowIndex]", so signalStrengthLevel:level]); is indented 4 characters. https://codereview.chromium.org/2257743003/diff/1/chrome/browser/ui/cocoa/ext... File chrome/browser/ui/cocoa/extensions/chooser_dialog_cocoa_controller.mm (right): https://codereview.chromium.org/2257743003/diff/1/chrome/browser/ui/cocoa/ext... chrome/browser/ui/cocoa/extensions/chooser_dialog_cocoa_controller.mm:49: viewForTableColumn:(NSTableColumn*)tableColumn On 2016/08/19 18:30:05, Jeffrey Yasskin wrote: > Another dumb question: why does this take a tableView and tableColumn that it > doesn't use? https://developer.apple.com/library/mac/documentation/Cocoa/Conceptual/TableV... https://developer.apple.com/library/mac/documentation/Cocoa/Reference/NSTable...: I guess it is like a virtual function that user needs to implement to customize the table view, and the virtual function's signature is fixed. https://codereview.chromium.org/2257743003/diff/1/chrome/browser/ui/cocoa/ext... File chrome/browser/ui/cocoa/extensions/chooser_dialog_cocoa_controller_unittest.mm (right): https://codereview.chromium.org/2257743003/diff/1/chrome/browser/ui/cocoa/ext... chrome/browser/ui/cocoa/extensions/chooser_dialog_cocoa_controller_unittest.mm:130: CheckTableRowView( On 2016/08/19 18:30:05, Jeffrey Yasskin wrote: > I'm not a big fan of helper functions like this, because the call doesn't say > what it's checking. Instead, you have two options: > > 1) Write a couple separate helper functions to pull the image and text out of a > row of table_view_, and a helper function to get the image for a given signal > strength, and write the assertions against the image and text inline in the > function. > > 2) Write functions of the form ExpectSignalStrengthImageIs(table_view_, row, 3) > and ExpectRowTextIs(table_view_, row, "Text") where it's clear exactly what the > expectation is. Done. https://codereview.chromium.org/2257743003/diff/1/chrome/browser/ui/cocoa/ext... chrome/browser/ui/cocoa/extensions/chooser_dialog_cocoa_controller_unittest.mm:176: EXPECT_EQ(4, table_view_.numberOfRows); On 2016/08/19 18:30:05, Jeffrey Yasskin wrote: > For these tests that are trying to check that the right image is being used for > all 5 levels, I think you can skip all the numberOfRows…selectedRow assertions. > Just check the image. Done. https://codereview.chromium.org/2257743003/diff/1/chrome/browser/ui/cocoa/ext... chrome/browser/ui/cocoa/extensions/chooser_dialog_cocoa_controller_unittest.mm:718: CheckTableRowView(0, MockChooserController::kSignalStrengthLevel2Bar, @"d"); On 2016/08/19 18:30:05, Jeffrey Yasskin wrote: > You don't need to add signal strength icon assertions to every test. Most can > just check that the right row is being used using the row's text. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM. Thanks! https://codereview.chromium.org/2257743003/diff/1/chrome/browser/ui/cocoa/cho... File chrome/browser/ui/cocoa/chooser_content_view_cocoa.mm (right): https://codereview.chromium.org/2257743003/diff/1/chrome/browser/ui/cocoa/cho... chrome/browser/ui/cocoa/chooser_content_view_cocoa.mm:495: if (chooserController_->ShouldShowIconBeforeText() && numOptions > 0) { On 2016/08/22 19:18:20, juncai wrote: > On 2016/08/19 18:30:05, Jeffrey Yasskin wrote: > > Dumb question: why is createTableRowView called with a particular rowIndex if > > there are no options? > > If numOptions == 0, the actual number rows is 1 since it needs to display > "Bluetooth devices not found" or "Devices not found" in the table view, so still > need to call createTableRowView to create a row, and a DCHECK checks the > rowIndex is always 0 in this case: > https://cs.chromium.org/chromium/src/chrome/browser/ui/cocoa/chooser_content_... Ah, I see. Could you comment that here? Like "This function is called with numOptions==0 to show the "not found" message." https://codereview.chromium.org/2257743003/diff/1/chrome/browser/ui/cocoa/cho... chrome/browser/ui/cocoa/chooser_content_view_cocoa.mm:504: initWithText:[self optionAtIndex:rowIndex] On 2016/08/22 19:18:20, juncai wrote: > On 2016/08/19 18:30:05, Jeffrey Yasskin wrote: > > This indentation looks kinda weird. > I used "git cl format" to format it. I guess here the two ":" need to be > aligned, and "signalStrengthLevel:level]);" has longer string length before the > ":" than "initWithText:[self optionAtIndex:rowIndex]", so > signalStrengthLevel:level]); is indented 4 characters. If `git cl format` did it, it's fine with me. :) https://codereview.chromium.org/2257743003/diff/1/chrome/browser/ui/cocoa/ext... File chrome/browser/ui/cocoa/extensions/chooser_dialog_cocoa_controller.mm (right): https://codereview.chromium.org/2257743003/diff/1/chrome/browser/ui/cocoa/ext... chrome/browser/ui/cocoa/extensions/chooser_dialog_cocoa_controller.mm:49: viewForTableColumn:(NSTableColumn*)tableColumn On 2016/08/22 19:18:20, juncai wrote: > On 2016/08/19 18:30:05, Jeffrey Yasskin wrote: > > Another dumb question: why does this take a tableView and tableColumn that it > > doesn't use? > > https://developer.apple.com/library/mac/documentation/Cocoa/Conceptual/TableV... > > https://developer.apple.com/library/mac/documentation/Cocoa/Reference/NSTable...: > > I guess it is like a virtual function that user needs to implement to customize > the table view, and the virtual function's signature is fixed. Yep, that's it. I missed that it was implementing a protocol. It'd be nice if Chrome had a convention for documenting the protocols a class is implementing, but if we don't, it's not worth inventing one here. https://codereview.chromium.org/2257743003/diff/1/chrome/browser/ui/cocoa/ext... File chrome/browser/ui/cocoa/extensions/chooser_dialog_cocoa_controller_unittest.mm (right): https://codereview.chromium.org/2257743003/diff/1/chrome/browser/ui/cocoa/ext... chrome/browser/ui/cocoa/extensions/chooser_dialog_cocoa_controller_unittest.mm:130: CheckTableRowView( On 2016/08/22 19:18:20, juncai wrote: > On 2016/08/19 18:30:05, Jeffrey Yasskin wrote: > > I'm not a big fan of helper functions like this, because the call doesn't say > > what it's checking. Instead, you have two options: > > > > 1) Write a couple separate helper functions to pull the image and text out of > a > > row of table_view_, and a helper function to get the image for a given signal > > strength, and write the assertions against the image and text inline in the > > function. > > > > 2) Write functions of the form ExpectSignalStrengthImageIs(table_view_, row, > 3) > > and ExpectRowTextIs(table_view_, row, "Text") where it's clear exactly what > the > > expectation is. > > Done. Much better, thanks!
c/b/ui/views lgtm
The CQ bit was checked by juncai@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/2257743003/diff/1/chrome/browser/ui/cocoa/cho... File chrome/browser/ui/cocoa/chooser_content_view_cocoa.mm (right): https://codereview.chromium.org/2257743003/diff/1/chrome/browser/ui/cocoa/cho... chrome/browser/ui/cocoa/chooser_content_view_cocoa.mm:495: if (chooserController_->ShouldShowIconBeforeText() && numOptions > 0) { On 2016/08/22 21:59:24, Jeffrey Yasskin wrote: > On 2016/08/22 19:18:20, juncai wrote: > > On 2016/08/19 18:30:05, Jeffrey Yasskin wrote: > > > Dumb question: why is createTableRowView called with a particular rowIndex > if > > > there are no options? > > > > If numOptions == 0, the actual number rows is 1 since it needs to display > > "Bluetooth devices not found" or "Devices not found" in the table view, so > still > > need to call createTableRowView to create a row, and a DCHECK checks the > > rowIndex is always 0 in this case: > > > https://cs.chromium.org/chromium/src/chrome/browser/ui/cocoa/chooser_content_... > > Ah, I see. Could you comment that here? Like "This function is called with > numOptions==0 to show the "not found" message." Done. https://codereview.chromium.org/2257743003/diff/1/chrome/browser/ui/cocoa/cho... chrome/browser/ui/cocoa/chooser_content_view_cocoa.mm:504: initWithText:[self optionAtIndex:rowIndex] On 2016/08/22 21:59:24, Jeffrey Yasskin wrote: > On 2016/08/22 19:18:20, juncai wrote: > > On 2016/08/19 18:30:05, Jeffrey Yasskin wrote: > > > This indentation looks kinda weird. > > I used "git cl format" to format it. I guess here the two ":" need to be > > aligned, and "signalStrengthLevel:level]);" has longer string length before > the > > ":" than "initWithText:[self optionAtIndex:rowIndex]", so > > signalStrengthLevel:level]); is indented 4 characters. > > If `git cl format` did it, it's fine with me. :) Acknowledged. https://codereview.chromium.org/2257743003/diff/1/chrome/browser/ui/cocoa/ext... File chrome/browser/ui/cocoa/extensions/chooser_dialog_cocoa_controller.mm (right): https://codereview.chromium.org/2257743003/diff/1/chrome/browser/ui/cocoa/ext... chrome/browser/ui/cocoa/extensions/chooser_dialog_cocoa_controller.mm:49: viewForTableColumn:(NSTableColumn*)tableColumn On 2016/08/22 21:59:24, Jeffrey Yasskin wrote: > On 2016/08/22 19:18:20, juncai wrote: > > On 2016/08/19 18:30:05, Jeffrey Yasskin wrote: > > > Another dumb question: why does this take a tableView and tableColumn that > it > > > doesn't use? > > > > > https://developer.apple.com/library/mac/documentation/Cocoa/Conceptual/TableV... > > > > > https://developer.apple.com/library/mac/documentation/Cocoa/Reference/NSTable...: > > > > I guess it is like a virtual function that user needs to implement to > customize > > the table view, and the virtual function's signature is fixed. > > Yep, that's it. I missed that it was implementing a protocol. It'd be nice if > Chrome had a convention for documenting the protocols a class is implementing, > but if we don't, it's not worth inventing one here. Acknowledged. https://codereview.chromium.org/2257743003/diff/1/chrome/browser/ui/cocoa/ext... File chrome/browser/ui/cocoa/extensions/chooser_dialog_cocoa_controller_unittest.mm (right): https://codereview.chromium.org/2257743003/diff/1/chrome/browser/ui/cocoa/ext... chrome/browser/ui/cocoa/extensions/chooser_dialog_cocoa_controller_unittest.mm:130: CheckTableRowView( On 2016/08/22 21:59:24, Jeffrey Yasskin wrote: > On 2016/08/22 19:18:20, juncai wrote: > > On 2016/08/19 18:30:05, Jeffrey Yasskin wrote: > > > I'm not a big fan of helper functions like this, because the call doesn't > say > > > what it's checking. Instead, you have two options: > > > > > > 1) Write a couple separate helper functions to pull the image and text out > of > > a > > > row of table_view_, and a helper function to get the image for a given > signal > > > strength, and write the assertions against the image and text inline in the > > > function. > > > > > > 2) Write functions of the form ExpectSignalStrengthImageIs(table_view_, row, > > 3) > > > and ExpectRowTextIs(table_view_, row, "Text") where it's clear exactly what > > the > > > expectation is. > > > > Done. > > Much better, thanks! Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2257743003/diff/40001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/chooser_content_view_cocoa.h (right): https://codereview.chromium.org/2257743003/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/chooser_content_view_cocoa.h:74: - (NSView*)createTableRowView:(NSInteger)rowIndex; "create" named methods should return strong references (scoped_nsobject) by convention. https://codereview.chromium.org/2257743003/diff/40001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/chooser_content_view_cocoa.mm (right): https://codereview.chromium.org/2257743003/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/chooser_content_view_cocoa.mm:62: @interface TableRowView : NSView { Since ObjC is a flat, global namespace, I'd make this class name a little more verbose (ChooserContentTableRowView or something). https://codereview.chromium.org/2257743003/diff/40001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/extensions/chooser_dialog_cocoa_controller_unittest.mm (right): https://codereview.chromium.org/2257743003/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/extensions/chooser_dialog_cocoa_controller_unittest.mm:30: const int signalStrengthLevelImageIds[5] = {IDR_SIGNAL_0_BAR, IDR_SIGNAL_1_BAR, naming: under_scores in C++
https://codereview.chromium.org/2257743003/diff/40001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/extensions/chooser_dialog_cocoa_controller_unittest.mm (right): https://codereview.chromium.org/2257743003/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/extensions/chooser_dialog_cocoa_controller_unittest.mm:30: const int signalStrengthLevelImageIds[5] = {IDR_SIGNAL_0_BAR, IDR_SIGNAL_1_BAR, On 2016/08/24 18:47:37, Robert Sesek wrote: > naming: under_scores in C++ I missed this in my first pass, but it should probably be a kConstant, right? It could be constexpr too, but I forget whether our conventions have moved that direction.
https://codereview.chromium.org/2257743003/diff/40001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/extensions/chooser_dialog_cocoa_controller_unittest.mm (right): https://codereview.chromium.org/2257743003/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/extensions/chooser_dialog_cocoa_controller_unittest.mm:30: const int signalStrengthLevelImageIds[5] = {IDR_SIGNAL_0_BAR, IDR_SIGNAL_1_BAR, On 2016/08/24 18:52:23, Jeffrey Yasskin wrote: > On 2016/08/24 18:47:37, Robert Sesek wrote: > > naming: under_scores in C++ > > I missed this in my first pass, but it should probably be a kConstant, right? It > could be constexpr too, but I forget whether our conventions have moved that > direction. Oh, yes kSignalStrengthLevelImageIds is probably the right name.
The CQ bit was checked by juncai@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/2257743003/diff/40001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/chooser_content_view_cocoa.h (right): https://codereview.chromium.org/2257743003/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/chooser_content_view_cocoa.h:74: - (NSView*)createTableRowView:(NSInteger)rowIndex; On 2016/08/24 18:47:37, Robert Sesek wrote: > "create" named methods should return strong references (scoped_nsobject) by > convention. Done. https://codereview.chromium.org/2257743003/diff/40001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/chooser_content_view_cocoa.mm (right): https://codereview.chromium.org/2257743003/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/chooser_content_view_cocoa.mm:62: @interface TableRowView : NSView { On 2016/08/24 18:47:37, Robert Sesek wrote: > Since ObjC is a flat, global namespace, I'd make this class name a little more > verbose (ChooserContentTableRowView or something). Done. https://codereview.chromium.org/2257743003/diff/40001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/extensions/chooser_dialog_cocoa_controller_unittest.mm (right): https://codereview.chromium.org/2257743003/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/extensions/chooser_dialog_cocoa_controller_unittest.mm:30: const int signalStrengthLevelImageIds[5] = {IDR_SIGNAL_0_BAR, IDR_SIGNAL_1_BAR, On 2016/08/24 18:53:07, Robert Sesek wrote: > On 2016/08/24 18:52:23, Jeffrey Yasskin wrote: > > On 2016/08/24 18:47:37, Robert Sesek wrote: > > > naming: under_scores in C++ > > > > I missed this in my first pass, but it should probably be a kConstant, right? > It > > could be constexpr too, but I forget whether our conventions have moved that > > direction. > > Oh, yes kSignalStrengthLevelImageIds is probably the right name. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by juncai@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org, jyasskin@chromium.org Link to the patchset: https://codereview.chromium.org/2257743003/#ps60001 (title: "address more comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add signal strength indicator icon to WebBluetooth chooser on Mac I uploaded a screenshot on the issue page. BUG=629689 ========== to ========== Add signal strength indicator icon to WebBluetooth chooser on Mac I uploaded a screenshot on the issue page. BUG=629689 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Add signal strength indicator icon to WebBluetooth chooser on Mac I uploaded a screenshot on the issue page. BUG=629689 ========== to ========== Add signal strength indicator icon to WebBluetooth chooser on Mac I uploaded a screenshot on the issue page. BUG=629689 Committed: https://crrev.com/8b985248b1ce2570d6ec84b211b1de14247127d1 Cr-Commit-Position: refs/heads/master@{#414599} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/8b985248b1ce2570d6ec84b211b1de14247127d1 Cr-Commit-Position: refs/heads/master@{#414599} |
