Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2072003002:
Desktop Capture Picker Window New UI For Mac (Closed)
DescriptionDesktop Capture Picker Window New UI For Mac
This CL develops the new Mac UI for Desktop Capture Picker window.
The main changes are
1. Separate the items of different source types into different
browser view.
2. Use table view rather than image view for tab capture, because
we do not have HD preview for tab.
BUG=602478, 618796
Committed: https://crrev.com/f8a7f3f0519e401ddadc437dfce377be5d43ed46
Cr-Commit-Position: refs/heads/master@{#403223}
Patch Set 1 #
Total comments: 70
Patch Set 2 : Comment Fix #
Total comments: 50
Patch Set 3 : Comment Fix #
Total comments: 18
Patch Set 4 : Comment Fix #Patch Set 5 : Unittest #
Total comments: 27
Patch Set 6 : Unittest fix #Patch Set 7 : Rebase #Messages
Total messages: 29 (11 generated)
Description was changed from ========== Desktop Capture Picker Window New UI For Mac Rename xxx Const Fix crash due to less than 3 types Fix crash when flag is off Fix compile issue Revert "NSTabView" This reverts commit bfd6ec7ca2373b7c11a5cc45cb4bb6d9ab5d5f87. NSTabView : Mock Pass List Into Observer Callback# Desktop Capture Picker Window Mac UI Topology BUG= patch from issue 2018083002 at patchset 120001 (http://crrev.com/2018083002#ps120001) ========== to ========== Desktop Capture Picker Window New UI For Mac This CL develops the new Mac UI for Desktop Capture Picker window. The main changes are 1. Separate the items of different source types into different browser view. 2. Use table view rather than image view for tab capture, because we do not have HD preview for tab. BUG=602478, 618796 ==========
qiangchen@chromium.org changed reviewers: + tapted@chromium.org
This is the implementation of #6 in crbug/618796, in which we use a NSSegmentedControl for source type switching, and we centralize the NSSegmentedControl.
https://codereview.chromium.org/2072003002/diff/1/chrome/browser/ui/cocoa/med... File chrome/browser/ui/cocoa/media_picker/desktop_media_picker_bridge.h (right): https://codereview.chromium.org/2072003002/diff/1/chrome/browser/ui/cocoa/med... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_bridge.h:14: - (void)sourceAddedForList:(DesktopMediaList*)list AtIndex:(int)index; AtIndex -> atIndex (or just index) - more below. Also From, To https://codereview.chromium.org/2072003002/diff/1/chrome/browser/ui/cocoa/med... File chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.h (right): https://codereview.chromium.org/2072003002/diff/1/chrome/browser/ui/cocoa/med... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.h:52: std::unique_ptr<DesktopMediaList> screen_list_; These need to be screenList_ naming style. Same with the rest https://codereview.chromium.org/2072003002/diff/1/chrome/browser/ui/cocoa/med... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.h:80: - (id)initWithScreenList:(std::unique_ptr<DesktopMediaList>)screen_list arguments should be screenList style too https://codereview.chromium.org/2072003002/diff/1/chrome/browser/ui/cocoa/med... File chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm (right): https://codereview.chromium.org/2072003002/diff/1/chrome/browser/ui/cocoa/med... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:33: const int kInitialContentWidth = 620; UI geometry constants in Cocoa should all be CGFloat https://codereview.chromium.org/2072003002/diff/1/chrome/browser/ui/cocoa/med... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:162: // Create segmented button nit: full stop https://codereview.chromium.org/2072003002/diff/1/chrome/browser/ui/cocoa/med... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:165: NSInteger segment_count = segmentCount - more below https://codereview.chromium.org/2072003002/diff/1/chrome/browser/ui/cocoa/med... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:210: setFrameOrigin:NSMakePoint((kInitialContentWidth - control_width) / 2, controlWidth can be an odd number, so dividing by 2 might align to a half-pixel on non-retina. Perhaps use NSIntegralRect https://codereview.chromium.org/2072003002/diff/1/chrome/browser/ui/cocoa/med... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:217: screenBrowser_.reset([[IKImageBrowserView alloc] initWithFrame:NSZeroRect]); This is too much boilerplate - can you make a helper function? https://codereview.chromium.org/2072003002/diff/1/chrome/browser/ui/cocoa/med... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:242: NSTableColumn* icon_column = this should be scoped_nsobject - currently it's a memory leak. More below https://codereview.chromium.org/2072003002/diff/1/chrome/browser/ui/cocoa/med... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:243: [[NSTableColumn alloc] initWithIdentifier:@"icon"]; @"icon" and @"title" should be constants https://codereview.chromium.org/2072003002/diff/1/chrome/browser/ui/cocoa/med... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:321: if (segment_index == 1) segmentIndex should end up in a separate method. segmentCount would be clearer here https://codereview.chromium.org/2072003002/diff/1/chrome/browser/ui/cocoa/med... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:331: } 200 lines is too much for one function - please split it up "fits on a screen" is generally a good guideline. Perhaps AddSegmentedControl(..), AddSourceBrowsers(..), AddButtons(..) https://codereview.chromium.org/2072003002/diff/1/chrome/browser/ui/cocoa/med... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:334: // Signal the media_list to start sending thumbnails. |bridge_| is used as the update comment https://codereview.chromium.org/2072003002/diff/1/chrome/browser/ui/cocoa/med... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:384: int segment = [sourceTypeControl_ selectedSegment]; nit: int -> NSInteger https://codereview.chromium.org/2072003002/diff/1/chrome/browser/ui/cocoa/med... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:385: content::DesktopMediaID::Type selected_type = selectedType https://codereview.chromium.org/2072003002/diff/1/chrome/browser/ui/cocoa/med... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:390: NSIndexSet* indexes = [tabBrowser_ selectedRowIndexes]; This logic should be in a helper function which is used by tableDoubleClick as well. And probably other cases. E.g. something like - (void)reportSelectedResult { NSUInteger selectedIndex; content::DesktopMediaID::Type source = [self selectedMediaType:&index]; NSMutableArray** items; [self browserForSource:source, &items]; DesktopMediaPickerItem* item = [items objectAtIndex:selectedIndex]; [self reportResult:[item sourceID]]; [self close]; } Probably all the calls to [self reportResult] can go through this or something similar, except for the ones in windowWillClose: and cancel: See other comments for where I mention selectedMediaType and browserForSource https://codereview.chromium.org/2072003002/diff/1/chrome/browser/ui/cocoa/med... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:424: [[sourceTypeControl_ cell] tagForSegment:segment]); This should be a helper function. E.g. - (content::DesktopMediaID::Type)selectedMediaType; use it in sharePressed, imageBrowserSelectionDidChange as well https://codereview.chromium.org/2072003002/diff/1/chrome/browser/ui/cocoa/med... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:425: if (selected_type == content::DesktopMediaID::TYPE_SCREEN) { this should be a switch statement https://codereview.chromium.org/2072003002/diff/1/chrome/browser/ui/cocoa/med... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:441: notificationWithName:@"selectionChange" pass nil - the notification isn't used https://codereview.chromium.org/2072003002/diff/1/chrome/browser/ui/cocoa/med... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:446: } I suspect you need to do something like [sharebutton_ setEnabled:foo] to ensure a user doesn't select something then switch sources https://codereview.chromium.org/2072003002/diff/1/chrome/browser/ui/cocoa/med... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:448: - (void)tableDoubleClick:(id)sender { declare this in the private interface https://codereview.chromium.org/2072003002/diff/1/chrome/browser/ui/cocoa/med... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:499: else // if (browser == windowBrowser_) remove comment https://codereview.chromium.org/2072003002/diff/1/chrome/browser/ui/cocoa/med... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:500: return [window_items_ count]; no else after return https://codereview.chromium.org/2072003002/diff/1/chrome/browser/ui/cocoa/med... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:503: - (id)imageBrowser:(IKImageBrowserView *)browser remove space before * - more below https://codereview.chromium.org/2072003002/diff/1/chrome/browser/ui/cocoa/med... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:505: if (browser == screenBrowser_) Looks like another good case for a helper function - (NSMutableArray*)dataForBrowser:(IKImageBrowserView*) https://codereview.chromium.org/2072003002/diff/1/chrome/browser/ui/cocoa/med... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:553: if (cell == nil) { nit: !cell. more below https://codereview.chromium.org/2072003002/diff/1/chrome/browser/ui/cocoa/med... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:554: cell = [[NSImageView alloc] this is leaked https://codereview.chromium.org/2072003002/diff/1/chrome/browser/ui/cocoa/med... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:556: cell.identifier = @"icon"; don't use dot notation https://codereview.chromium.org/2072003002/diff/1/chrome/browser/ui/cocoa/med... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:577: if ([[sourceTypeControl_ cell] tagForSegment:segment] != can this happen? DCHECK? https://codereview.chromium.org/2072003002/diff/1/chrome/browser/ui/cocoa/med... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:584: if ([indexes count] == 0) remove https://codereview.chromium.org/2072003002/diff/1/chrome/browser/ui/cocoa/med... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:594: if (list == screen_list_.get()) { this should be a helper function e.g. id browserForList:(DesktopMediaList*)list (NSMutableArray**)items; use it in sourceRemovedForList and sourceMovedForList, sourceThumbnailChangedForList, etc. https://codereview.chromium.org/2072003002/diff/1/chrome/browser/ui/cocoa/med... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:637: } else { // if (list == window_list_.get()) remove comment https://codereview.chromium.org/2072003002/diff/1/chrome/browser/ui/cocoa/med... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:644: byExtendingSelection:FALSE]; FALSE -> NO https://codereview.chromium.org/2072003002/diff/1/chrome/browser/ui/cocoa/med... File chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller_unittest.mm (right): https://codereview.chromium.org/2072003002/diff/1/chrome/browser/ui/cocoa/med... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller_unittest.mm:5: #import "chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller_deprecated.h" You need to test the new interface as well - this effectively just deletes all the tests.
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
Thanks for review. Added some helper functions, as we have several representations for source type, such as Browser_, List_ etc, and thus I put content::DesktopMediaID::Type at center position, rather than implementing too many helpers. Also notices NSTableView does not memorize the user selection during reload:, and thus add some logic to make that happen. Unittest is still under work. Publish the main code change first. https://codereview.chromium.org/2072003002/diff/1/chrome/browser/ui/cocoa/med... File chrome/browser/ui/cocoa/media_picker/desktop_media_picker_bridge.h (right): https://codereview.chromium.org/2072003002/diff/1/chrome/browser/ui/cocoa/med... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_bridge.h:14: - (void)sourceAddedForList:(DesktopMediaList*)list AtIndex:(int)index; On 2016/06/20 12:18:34, tapted wrote: > AtIndex -> atIndex (or just index) - more below. Also From, To Done. https://codereview.chromium.org/2072003002/diff/1/chrome/browser/ui/cocoa/med... File chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.h (right): https://codereview.chromium.org/2072003002/diff/1/chrome/browser/ui/cocoa/med... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.h:52: std::unique_ptr<DesktopMediaList> screen_list_; On 2016/06/20 12:18:34, tapted wrote: > These need to be screenList_ naming style. Same with the rest Done. https://codereview.chromium.org/2072003002/diff/1/chrome/browser/ui/cocoa/med... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.h:80: - (id)initWithScreenList:(std::unique_ptr<DesktopMediaList>)screen_list On 2016/06/20 12:18:34, tapted wrote: > arguments should be screenList style too Done. https://codereview.chromium.org/2072003002/diff/1/chrome/browser/ui/cocoa/med... File chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm (right): https://codereview.chromium.org/2072003002/diff/1/chrome/browser/ui/cocoa/med... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:33: const int kInitialContentWidth = 620; On 2016/06/20 12:18:34, tapted wrote: > UI geometry constants in Cocoa should all be CGFloat Done. https://codereview.chromium.org/2072003002/diff/1/chrome/browser/ui/cocoa/med... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:162: // Create segmented button On 2016/06/20 12:18:35, tapted wrote: > nit: full stop Done. https://codereview.chromium.org/2072003002/diff/1/chrome/browser/ui/cocoa/med... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:165: NSInteger segment_count = On 2016/06/20 12:18:35, tapted wrote: > segmentCount - more below Done. https://codereview.chromium.org/2072003002/diff/1/chrome/browser/ui/cocoa/med... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:210: setFrameOrigin:NSMakePoint((kInitialContentWidth - control_width) / 2, On 2016/06/20 12:18:35, tapted wrote: > controlWidth can be an odd number, so dividing by 2 might align to a half-pixel > on non-retina. Perhaps use NSIntegralRect Done. https://codereview.chromium.org/2072003002/diff/1/chrome/browser/ui/cocoa/med... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:217: screenBrowser_.reset([[IKImageBrowserView alloc] initWithFrame:NSZeroRect]); On 2016/06/20 12:18:35, tapted wrote: > This is too much boilerplate - can you make a helper function? Done. https://codereview.chromium.org/2072003002/diff/1/chrome/browser/ui/cocoa/med... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:242: NSTableColumn* icon_column = On 2016/06/20 12:18:35, tapted wrote: > this should be scoped_nsobject - currently it's a memory leak. More below Done. https://codereview.chromium.org/2072003002/diff/1/chrome/browser/ui/cocoa/med... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:243: [[NSTableColumn alloc] initWithIdentifier:@"icon"]; On 2016/06/20 12:18:34, tapted wrote: > @"icon" and @"title" should be constants Done. https://codereview.chromium.org/2072003002/diff/1/chrome/browser/ui/cocoa/med... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:321: if (segment_index == 1) On 2016/06/20 12:18:34, tapted wrote: > segmentIndex should end up in a separate method. segmentCount would be clearer > here N/A now. And I found this logic is actually incorrect. We should put this in the "if (XXXList_)". https://codereview.chromium.org/2072003002/diff/1/chrome/browser/ui/cocoa/med... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:331: } On 2016/06/20 12:18:35, tapted wrote: > 200 lines is too much for one function - please split it up "fits on a screen" > is generally a good guideline. Perhaps AddSegmentedControl(..), > AddSourceBrowsers(..), AddButtons(..) Acknowledged. https://codereview.chromium.org/2072003002/diff/1/chrome/browser/ui/cocoa/med... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:334: // Signal the media_list to start sending thumbnails. |bridge_| is used as the On 2016/06/20 12:18:36, tapted wrote: > update comment Done. https://codereview.chromium.org/2072003002/diff/1/chrome/browser/ui/cocoa/med... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:384: int segment = [sourceTypeControl_ selectedSegment]; On 2016/06/20 12:18:35, tapted wrote: > nit: int -> NSInteger Done. https://codereview.chromium.org/2072003002/diff/1/chrome/browser/ui/cocoa/med... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:385: content::DesktopMediaID::Type selected_type = On 2016/06/20 12:18:35, tapted wrote: > selectedType Done. https://codereview.chromium.org/2072003002/diff/1/chrome/browser/ui/cocoa/med... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:390: NSIndexSet* indexes = [tabBrowser_ selectedRowIndexes]; On 2016/06/20 12:18:34, tapted wrote: > This logic should be in a helper function which is used by tableDoubleClick as > well. And probably other cases. > > E.g. something like > > - (void)reportSelectedResult { > NSUInteger selectedIndex; > content::DesktopMediaID::Type source = [self selectedMediaType:&index]; > NSMutableArray** items; > [self browserForSource:source, &items]; > DesktopMediaPickerItem* item = [items objectAtIndex:selectedIndex]; > [self reportResult:[item sourceID]]; > [self close]; > } > > > Probably all the calls to [self reportResult] can go through this or something > similar, except for the ones in windowWillClose: and cancel: > > See other comments for where I mention selectedMediaType and browserForSource Done. https://codereview.chromium.org/2072003002/diff/1/chrome/browser/ui/cocoa/med... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:424: [[sourceTypeControl_ cell] tagForSegment:segment]); On 2016/06/20 12:18:34, tapted wrote: > This should be a helper function. E.g. > > - (content::DesktopMediaID::Type)selectedMediaType; > > use it in sharePressed, imageBrowserSelectionDidChange as well Done. https://codereview.chromium.org/2072003002/diff/1/chrome/browser/ui/cocoa/med... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:425: if (selected_type == content::DesktopMediaID::TYPE_SCREEN) { On 2016/06/20 12:18:36, tapted wrote: > this should be a switch statement Done. https://codereview.chromium.org/2072003002/diff/1/chrome/browser/ui/cocoa/med... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:441: notificationWithName:@"selectionChange" On 2016/06/20 12:18:35, tapted wrote: > pass nil - the notification isn't used Done. https://codereview.chromium.org/2072003002/diff/1/chrome/browser/ui/cocoa/med... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:446: } On 2016/06/20 12:18:35, tapted wrote: > I suspect you need to do something like > > [sharebutton_ setEnabled:foo] to ensure a user doesn't select something then > switch sources In XXXDidChange(), Share button status will be updated. https://codereview.chromium.org/2072003002/diff/1/chrome/browser/ui/cocoa/med... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:448: - (void)tableDoubleClick:(id)sender { On 2016/06/20 12:18:35, tapted wrote: > declare this in the private interface Done. https://codereview.chromium.org/2072003002/diff/1/chrome/browser/ui/cocoa/med... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:499: else // if (browser == windowBrowser_) On 2016/06/20 12:18:35, tapted wrote: > remove comment Done. https://codereview.chromium.org/2072003002/diff/1/chrome/browser/ui/cocoa/med... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:500: return [window_items_ count]; On 2016/06/20 12:18:35, tapted wrote: > no else after return Done. https://codereview.chromium.org/2072003002/diff/1/chrome/browser/ui/cocoa/med... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:503: - (id)imageBrowser:(IKImageBrowserView *)browser On 2016/06/20 12:18:35, tapted wrote: > remove space before * - more below Done. https://codereview.chromium.org/2072003002/diff/1/chrome/browser/ui/cocoa/med... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:505: if (browser == screenBrowser_) On 2016/06/20 12:18:34, tapted wrote: > Looks like another good case for a helper function > > - (NSMutableArray*)dataForBrowser:(IKImageBrowserView*) Done. https://codereview.chromium.org/2072003002/diff/1/chrome/browser/ui/cocoa/med... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:553: if (cell == nil) { On 2016/06/20 12:18:35, tapted wrote: > nit: !cell. more below Done. https://codereview.chromium.org/2072003002/diff/1/chrome/browser/ui/cocoa/med... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:554: cell = [[NSImageView alloc] On 2016/06/20 12:18:35, tapted wrote: > this is leaked Used scoped_nsobject now. But I found I have to release it for return value, otherwise crash. So I think NSTableView takes over the life time control of the cell view. https://codereview.chromium.org/2072003002/diff/1/chrome/browser/ui/cocoa/med... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:556: cell.identifier = @"icon"; On 2016/06/20 12:18:34, tapted wrote: > don't use dot notation Done. https://codereview.chromium.org/2072003002/diff/1/chrome/browser/ui/cocoa/med... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:577: if ([[sourceTypeControl_ cell] tagForSegment:segment] != On 2016/06/20 12:18:35, tapted wrote: > can this happen? DCHECK? Done. https://codereview.chromium.org/2072003002/diff/1/chrome/browser/ui/cocoa/med... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:584: if ([indexes count] == 0) On 2016/06/20 12:18:35, tapted wrote: > remove Done. https://codereview.chromium.org/2072003002/diff/1/chrome/browser/ui/cocoa/med... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:594: if (list == screen_list_.get()) { On 2016/06/20 12:18:34, tapted wrote: > this should be a helper function e.g. > > id browserForList:(DesktopMediaList*)list (NSMutableArray**)items; > > use it in sourceRemovedForList and sourceMovedForList, > sourceThumbnailChangedForList, etc. Done. https://codereview.chromium.org/2072003002/diff/1/chrome/browser/ui/cocoa/med... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:637: } else { // if (list == window_list_.get()) On 2016/06/20 12:18:35, tapted wrote: > remove comment Done. https://codereview.chromium.org/2072003002/diff/1/chrome/browser/ui/cocoa/med... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:644: byExtendingSelection:FALSE]; On 2016/06/20 12:18:34, tapted wrote: > FALSE -> NO Done. https://codereview.chromium.org/2072003002/diff/1/chrome/browser/ui/cocoa/med... File chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller_unittest.mm (right): https://codereview.chromium.org/2072003002/diff/1/chrome/browser/ui/cocoa/med... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller_unittest.mm:5: #import "chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller_deprecated.h" On 2016/06/20 12:18:36, tapted wrote: > You need to test the new interface as well - this effectively just deletes all > the tests. Yep, I delay that after your initial code review, in case you have some structural suggestion.
https://codereview.chromium.org/2072003002/diff/1/chrome/browser/ui/cocoa/med... File chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm (right): https://codereview.chromium.org/2072003002/diff/1/chrome/browser/ui/cocoa/med... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:554: cell = [[NSImageView alloc] On 2016/06/21 23:31:28, qiangchenC wrote: > On 2016/06/20 12:18:35, tapted wrote: > > this is leaked > > Used scoped_nsobject now. > > But I found I have to release it for return value, otherwise crash. > So I think NSTableView takes over the life time control of the cell view. see comment on the latest patchset https://codereview.chromium.org/2072003002/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.h (right): https://codereview.chromium.org/2072003002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.h:48: // |screen_items_|, |window_items_| and |tab_items_|, and to render in update comment (e.g. screen_items_ -> screenItems_) https://codereview.chromium.org/2072003002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.h:57: // Arrays of |DesktopMediaPickerItem| used as data for |screenBrowser|, nit: screenBrowser -> screenBrowser_. also tab https://codereview.chromium.org/2072003002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.h:63: // C++ bridge to use as an observer to |screen_list_|, |window_list_| and update https://codereview.chromium.org/2072003002/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm (right): https://codereview.chromium.org/2072003002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:31: namespace { consider a using content::DesktopMediaID; before this, to remove lots of uninteresting `content::` from the file https://codereview.chromium.org/2072003002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:48: NSString* const kIConColumnIdentifier = @"icon"; kICon -> kIcon If you want something shorter, kIconId / kTitleId would be fine too https://codereview.chromium.org/2072003002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:53: @interface DesktopMediaPickerController (Private) remove "Private" -- i.e. `DesktopMediaPickerController ()` - this is a special category for private stuff and will do extra compiler checks, e.g., for methods declared but not defined https://codereview.chromium.org/2072003002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:186: [[[self window] contentView] setAutoresizesSubviews:YES]; [[self window] contentView] -> content? https://codereview.chromium.org/2072003002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:193: - (void)createTypeButtonAtOrigin:(NSPoint)origin { declare in @interface Foo () And ensure the definition order matches declaration order. i.e. - init - dealloc - methods declared in .h, in order - methods declared in private @interface Foo (), in order - overridden methods from the parent class ancestry - methods from implemented protocols, in the order they're listed (with methods ordered the same as in the protocol declaration). every method definition should have a declaration in one of these places (but you don't re-declare stuff inherited or implemented from protocols) see go/objcstyle for details https://codereview.chromium.org/2072003002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:252: screenBrowser_.reset([[IKImageBrowserView alloc] initWithFrame:NSZeroRect]); by `this is too much boilerplate` I meant you should have a helper method like - (IKImageBrowserView*)makeBrowserView { scoped_nsobject<IKImageBrowserView> browser( .. alloc/init); [browser setFoo, setBar, setBaz.. return makeBrowserView.autorelease(); } https://codereview.chromium.org/2072003002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:261: [[self window] makeFirstResponder:screenBrowser_]; How about once, at the end of initializeContentsWithAppName: [[self window] makeFirstResponder: [self browserViewForType:[self selectedSourceType]]]; https://codereview.chromium.org/2072003002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:287: base::scoped_nsobject<NSTableColumn> icon_column( icon_column -> iconColumn more below. Please do a pass over your code after uploading to rietveld - you should be able to spot this kind of stuff and it's good practice for doing your own code reviews https://codereview.chromium.org/2072003002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:305: // Create a scroll view to host the image browser. image browser -> browsers https://codereview.chromium.org/2072003002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:436: [self tableViewSelectionDidChange:nil]; is this invoked automatically? (comment?) https://codereview.chromium.org/2072003002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:444: NSIndexSet* indexes = [tabBrowser_ selectedRowIndexes]; Can this just call [self sharePressed:sender] https://codereview.chromium.org/2072003002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:479: - (content::DesktopMediaID::Type)selectedSourceType { These should all be declared in `@interface DesktopMediaPickerController ()` https://codereview.chromium.org/2072003002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:488: else if (list == windowList_.get()) no `else`. just `if` https://codereview.chromium.org/2072003002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:496: else if (browser == windowBrowser_.get()) no `else` https://codereview.chromium.org/2072003002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:616: - (NSView*)tableView:(NSTableView*)table nit: blank line before https://codereview.chromium.org/2072003002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:621: base::scoped_nsobject<NSImageView> cell( nit: cell -> view. (cell is confusing because NSTableView has modes where it populates with cells or views, but you've chosen the view mode) https://codereview.chromium.org/2072003002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:622: [table makeViewWithIdentifier:kIConColumnIdentifier owner:self]); [table makeViewWithIdentifier .. returns an item with a refcount of 0 - you need to call `retain` on it before passing it to the scoped_nsobject constructor base::scoped_nsobject<NSImageView> cell( [[table makeViewWithIdentifier:kIconId owner:self] retain]); https://codereview.chromium.org/2072003002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:629: return cell.release(); release -> autorelease (otherwise it's a leak) https://codereview.chromium.org/2072003002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:633: base::scoped_nsobject<NSTextField> cell( cell -> view https://codereview.chromium.org/2072003002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:642: return cell.release(); autorelease, and retain above. https://codereview.chromium.org/2072003002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:690: [self selectedIndexForType:content::DesktopMediaID::TYPE_WEB_CONTENTS]; basee sourceType - it's shorter
Fixed. Unittest still under working. https://codereview.chromium.org/2072003002/diff/1/chrome/browser/ui/cocoa/med... File chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm (right): https://codereview.chromium.org/2072003002/diff/1/chrome/browser/ui/cocoa/med... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:554: cell = [[NSImageView alloc] On 2016/06/22 07:08:41, tapted wrote: > On 2016/06/21 23:31:28, qiangchenC wrote: > > On 2016/06/20 12:18:35, tapted wrote: > > > this is leaked > > > > Used scoped_nsobject now. > > > > But I found I have to release it for return value, otherwise crash. > > So I think NSTableView takes over the life time control of the cell view. > > see comment on the latest patchset Acknowledged. https://codereview.chromium.org/2072003002/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.h (right): https://codereview.chromium.org/2072003002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.h:48: // |screen_items_|, |window_items_| and |tab_items_|, and to render in On 2016/06/22 07:08:41, tapted wrote: > update comment (e.g. screen_items_ -> screenItems_) Done. https://codereview.chromium.org/2072003002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.h:57: // Arrays of |DesktopMediaPickerItem| used as data for |screenBrowser|, On 2016/06/22 07:08:41, tapted wrote: > nit: screenBrowser -> screenBrowser_. also tab Done. https://codereview.chromium.org/2072003002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.h:63: // C++ bridge to use as an observer to |screen_list_|, |window_list_| and On 2016/06/22 07:08:41, tapted wrote: > update Done. https://codereview.chromium.org/2072003002/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm (right): https://codereview.chromium.org/2072003002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:31: namespace { On 2016/06/22 07:08:42, tapted wrote: > consider a > > using content::DesktopMediaID; > > before this, to remove lots of uninteresting `content::` from the file Done. https://codereview.chromium.org/2072003002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:48: NSString* const kIConColumnIdentifier = @"icon"; On 2016/06/22 07:08:42, tapted wrote: > kICon -> kIcon > > If you want something shorter, kIconId / kTitleId would be fine too Done. https://codereview.chromium.org/2072003002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:53: @interface DesktopMediaPickerController (Private) On 2016/06/22 07:08:42, tapted wrote: > remove "Private" -- i.e. `DesktopMediaPickerController ()` - this is a special > category for private stuff and will do extra compiler checks, e.g., for methods > declared but not defined Done. https://codereview.chromium.org/2072003002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:186: [[[self window] contentView] setAutoresizesSubviews:YES]; On 2016/06/22 07:08:42, tapted wrote: > [[self window] contentView] -> content? Done. https://codereview.chromium.org/2072003002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:193: - (void)createTypeButtonAtOrigin:(NSPoint)origin { On 2016/06/22 07:08:42, tapted wrote: > declare in @interface Foo () > > And ensure the definition order matches declaration order. i.e. > > - init > - dealloc > - methods declared in .h, in order > - methods declared in private @interface Foo (), in order > - overridden methods from the parent class ancestry > - methods from implemented protocols, in the order they're listed (with methods > ordered the same as in the protocol declaration). > > every method definition should have a declaration in one of these places (but > you don't re-declare stuff inherited or implemented from protocols) > > see go/objcstyle for details Done. https://codereview.chromium.org/2072003002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:252: screenBrowser_.reset([[IKImageBrowserView alloc] initWithFrame:NSZeroRect]); On 2016/06/22 07:08:42, tapted wrote: > by `this is too much boilerplate` I meant you should have a helper method like > > - (IKImageBrowserView*)makeBrowserView { > scoped_nsobject<IKImageBrowserView> browser( .. alloc/init); > [browser setFoo, setBar, setBaz.. > return makeBrowserView.autorelease(); > } Done. But as screenBrowser_ and windowBrowser_ are scoped_nsobject, we could not wrap an object in the autorelease pool. Otherwise crash. Or should we change screenBrowser_ and windowBrowser_ to raw pointers? I can see both conventions are used in this picker window. https://codereview.chromium.org/2072003002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:261: [[self window] makeFirstResponder:screenBrowser_]; On 2016/06/22 07:08:42, tapted wrote: > How about once, at the end of initializeContentsWithAppName: > > [[self window] makeFirstResponder: > [self browserViewForType:[self selectedSourceType]]]; Done. https://codereview.chromium.org/2072003002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:287: base::scoped_nsobject<NSTableColumn> icon_column( On 2016/06/22 07:08:42, tapted wrote: > icon_column -> iconColumn more below. Please do a pass over your code after > uploading to rietveld - you should be able to spot this kind of stuff and it's > good practice for doing your own code reviews Done. https://codereview.chromium.org/2072003002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:305: // Create a scroll view to host the image browser. On 2016/06/22 07:08:42, tapted wrote: > image browser -> browsers Done. https://codereview.chromium.org/2072003002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:436: [self tableViewSelectionDidChange:nil]; On 2016/06/22 07:08:41, tapted wrote: > is this invoked automatically? (comment?) Done. https://codereview.chromium.org/2072003002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:444: NSIndexSet* indexes = [tabBrowser_ selectedRowIndexes]; On 2016/06/22 07:08:42, tapted wrote: > Can this just call [self sharePressed:sender] Actually we can do simpler: just bind sharePressed: to double click action. https://codereview.chromium.org/2072003002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:479: - (content::DesktopMediaID::Type)selectedSourceType { On 2016/06/22 07:08:42, tapted wrote: > These should all be declared in `@interface DesktopMediaPickerController ()` Done. https://codereview.chromium.org/2072003002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:488: else if (list == windowList_.get()) On 2016/06/22 07:08:42, tapted wrote: > no `else`. just `if` Done. https://codereview.chromium.org/2072003002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:496: else if (browser == windowBrowser_.get()) On 2016/06/22 07:08:42, tapted wrote: > no `else` Done. https://codereview.chromium.org/2072003002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:616: - (NSView*)tableView:(NSTableView*)table On 2016/06/22 07:08:42, tapted wrote: > nit: blank line before Done. https://codereview.chromium.org/2072003002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:621: base::scoped_nsobject<NSImageView> cell( On 2016/06/22 07:08:42, tapted wrote: > nit: cell -> view. (cell is confusing because NSTableView has modes where it > populates with cells or views, but you've chosen the view mode) Done. https://codereview.chromium.org/2072003002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:622: [table makeViewWithIdentifier:kIConColumnIdentifier owner:self]); On 2016/06/22 07:08:42, tapted wrote: > [table makeViewWithIdentifier .. returns an item with a refcount of 0 - you need > to call `retain` on it before passing it to the scoped_nsobject constructor > > base::scoped_nsobject<NSImageView> cell( > [[table makeViewWithIdentifier:kIconId owner:self] retain]); Done. https://codereview.chromium.org/2072003002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:629: return cell.release(); On 2016/06/22 07:08:42, tapted wrote: > release -> autorelease (otherwise it's a leak) Done. https://codereview.chromium.org/2072003002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:633: base::scoped_nsobject<NSTextField> cell( On 2016/06/22 07:08:42, tapted wrote: > cell -> view Done. https://codereview.chromium.org/2072003002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:642: return cell.release(); On 2016/06/22 07:08:42, tapted wrote: > autorelease, and retain above. Change back to not use scoped_nsobject. Move autorelease to just after init. Because "createTextFieldWithText:" returns an object that is in the auto release pool, if we autorelease it again, crash. https://codereview.chromium.org/2072003002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:690: [self selectedIndexForType:content::DesktopMediaID::TYPE_WEB_CONTENTS]; On 2016/06/22 07:08:42, tapted wrote: > basee sourceType - it's shorter Done.
https://codereview.chromium.org/2072003002/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm (right): https://codereview.chromium.org/2072003002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:642: return cell.release(); On 2016/06/22 23:54:24, qiangchenC wrote: > On 2016/06/22 07:08:42, tapted wrote: > > autorelease, and retain above. > > Change back to not use scoped_nsobject. > Move autorelease to just after init. > Because "createTextFieldWithText:" returns an object that is in the auto release > pool, if we autorelease it again, crash. autoreleasing twice is fine, you just need to retain it again beforehand. https://codereview.chromium.org/2072003002/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm (right): https://codereview.chromium.org/2072003002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:279: screenBrowser_.reset( the scoped_nsobject constructor, and reset, expect to receive an object with an outstanding reference count for scoped_nsobject to "assume" ownership of But in ObjectiveC, methods *returning* an object should NOT have an outstanding reference count, unless that method is the object's "init", "copy" or "retain" methods. So when initializing a scoped_nsobject with something immediately from [[Foo alloc] init..], there is no "retain" necessary. Here, the scoped_nsobject is initialized from the result of a function, so you need to add a retain. Either scoped_nsobject.reset([[self createFoo:..] retain]); or scoped_nsobject.reset([self createFoo:..], base::scoped_policy::RETAIN); the first way is already in use in this file, so that's what you should do here. https://codereview.chromium.org/2072003002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:392: IKImageBrowserView* browser = Use scoped_nsobject and autorelease - it should be consistent with createButtonWithTitle https://codereview.chromium.org/2072003002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:631: iconView = [[[NSImageView alloc] things should always be `alloced` into a scoped_nsobject. We try to avoid calls to -[NSObject retain] or -[NSObject autorelease], or -[NSObject release] since it's easy to get wrong. Calling scoped_nsobject::autorelease() is OK. Having a call to [NSObject retain] inside a call to a scoped_nsobject constructor or reset() call can be avoided with base::scoped_policy::RETAIN, but it's also usually OK. But outside of these contexts, there should never be explicit calls to release/retain/autorelease. So that things are consistent, perhaps also declare [self createEmptyImageView], which does the scoped_nsobject, .. , autorelease() dance. Then it's fine not to use scoped_nsobject here. But an "alloc" not attached to a scoped_nsobject should be avoided. https://codereview.chromium.org/2072003002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:640: NSTextField* titleView = so removing scoped_nsobject here is OK, since nothing is being allocated. https://codereview.chromium.org/2072003002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:641: [[[table makeViewWithIdentifier:kTitleId owner:self] retain] autorelease]; but here the retain] autorelease] does nothing. This line should just be NSTextField* titleView = [table makeViewWithIdentifier:kTitleId owner:self];
(a few more nits - looking forward to tests :) https://codereview.chromium.org/2072003002/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm (right): https://codereview.chromium.org/2072003002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:53: using content::DesktopMediaID; nit: this typically comes immediately after the #includes https://codereview.chromium.org/2072003002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:226: sourceTypeControl_.reset([[NSSegmentedControl alloc] init]); nit: initWthFrame:NSZeroRect (to invoke the designated initializer inherited from NSView) https://codereview.chromium.org/2072003002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:265: [content addSubview:sourceTypeControl_]; nit: just make this [[self window] contentView] addSubview:sourceTypeControl_] no need for |content| https://codereview.chromium.org/2072003002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:374: [textField setFont:[NSFont systemFontOfSize:13]]; nit: 13 should be a constant, if it's coming from a spec. Otherwise it can probably be [NSFont systemFontSize]
https://codereview.chromium.org/2072003002/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm (right): https://codereview.chromium.org/2072003002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:642: return cell.release(); On 2016/06/23 02:49:48, tapted wrote: > On 2016/06/22 23:54:24, qiangchenC wrote: > > On 2016/06/22 07:08:42, tapted wrote: > > > autorelease, and retain above. > > > > Change back to not use scoped_nsobject. > > Move autorelease to just after init. > > Because "createTextFieldWithText:" returns an object that is in the auto > release > > pool, if we autorelease it again, crash. > > autoreleasing twice is fine, you just need to retain it again beforehand. Acknowledged. https://codereview.chromium.org/2072003002/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm (right): https://codereview.chromium.org/2072003002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:53: using content::DesktopMediaID; On 2016/06/23 03:38:14, tapted wrote: > nit: this typically comes immediately after the #includes Done. https://codereview.chromium.org/2072003002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:226: sourceTypeControl_.reset([[NSSegmentedControl alloc] init]); On 2016/06/23 03:38:15, tapted wrote: > nit: initWthFrame:NSZeroRect (to invoke the designated initializer inherited > from NSView) Done. https://codereview.chromium.org/2072003002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:265: [content addSubview:sourceTypeControl_]; On 2016/06/23 03:38:14, tapted wrote: > nit: just make this [[self window] contentView] addSubview:sourceTypeControl_] > > no need for |content| Done. https://codereview.chromium.org/2072003002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:279: screenBrowser_.reset( On 2016/06/23 02:49:48, tapted wrote: > the scoped_nsobject constructor, and reset, expect to receive an object with an > outstanding reference count for scoped_nsobject to "assume" ownership of > > But in ObjectiveC, methods *returning* an object should NOT have an outstanding > reference count, unless that method is the object's "init", "copy" or "retain" > methods. > > So when initializing a scoped_nsobject with something immediately from [[Foo > alloc] init..], there is no "retain" necessary. > > Here, the scoped_nsobject is initialized from the result of a function, so you > need to add a retain. Either > > scoped_nsobject.reset([[self createFoo:..] retain]); > > or > > scoped_nsobject.reset([self createFoo:..], base::scoped_policy::RETAIN); > > > the first way is already in use in this file, so that's what you should do here. Done. https://codereview.chromium.org/2072003002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:374: [textField setFont:[NSFont systemFontOfSize:13]]; On 2016/06/23 03:38:14, tapted wrote: > nit: 13 should be a constant, if it's coming from a spec. Otherwise it can > probably be [NSFont systemFontSize] Done. Actually this is the existing code. As we hardcodes other size, we'd better use a hardcode size here. https://codereview.chromium.org/2072003002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:392: IKImageBrowserView* browser = On 2016/06/23 02:49:48, tapted wrote: > Use scoped_nsobject and autorelease - it should be consistent with > createButtonWithTitle Done. https://codereview.chromium.org/2072003002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:631: iconView = [[[NSImageView alloc] On 2016/06/23 02:49:48, tapted wrote: > things should always be `alloced` into a scoped_nsobject. We try to avoid calls > to -[NSObject retain] or -[NSObject autorelease], or -[NSObject release] since > it's easy to get wrong. Calling scoped_nsobject::autorelease() is OK. Having a > call to [NSObject retain] inside a call to a scoped_nsobject constructor or > reset() call can be avoided with base::scoped_policy::RETAIN, but it's also > usually OK. But outside of these contexts, there should never be explicit calls > to release/retain/autorelease. > > So that things are consistent, perhaps also declare [self createEmptyImageView], > which does the scoped_nsobject, .. , autorelease() dance. Then it's fine not to > use scoped_nsobject here. But an "alloc" not attached to a scoped_nsobject > should be avoided. Acknowledged. https://codereview.chromium.org/2072003002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:640: NSTextField* titleView = On 2016/06/23 02:49:48, tapted wrote: > so removing scoped_nsobject here is OK, since nothing is being allocated. Done. https://codereview.chromium.org/2072003002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:641: [[[table makeViewWithIdentifier:kTitleId owner:self] retain] autorelease]; On 2016/06/23 02:49:48, tapted wrote: > but here the retain] autorelease] does nothing. This line should just be > > NSTextField* titleView = [table makeViewWithIdentifier:kTitleId owner:self]; Done.
Unit test done.
https://codereview.chromium.org/2072003002/diff/120001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller_unittest.mm (right): https://codereview.chromium.org/2072003002/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller_unittest.mm:13: #include "chrome/browser/ui/cocoa/media_picker/desktop_media_picker_item.h" nit: import https://codereview.chromium.org/2072003002/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller_unittest.mm:16: a `using content::DesktopMediaID` here would probably be handy https://codereview.chromium.org/2072003002/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller_unittest.mm:18: content::DesktopMediaID(content::DesktopMediaID::TYPE_WINDOW, id) This is not a good reason to use macros see http://go/cppguide#Preprocessor_Macros. You can add helper methods in an anonymous namespace namespace { content::DesktopMediaID MakeWindow(DesktopMediaID::Id id) { return content::DesktopMediaID(content::DesktopMediaID::TYPE_WINDOW, id); } edit: Something even better, you can put this on the test harness like void AddTab(DesktopMediaID::Id id) { tab_list_->AddSourceByFullMediaID( content::DesktopMediaID(content::DesktopMediaID::TYPE_WEB_CONTENTS, id); } void AddWindow(..) { window_list_->AddSource.. window_list_->SetSourceThumbnail(id); } and remove a lot of boilerplate https://codereview.chromium.org/2072003002/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller_unittest.mm:122: [controller_ performSelector:[control action] withObject:control]; nit: controller_ -> [control target] https://codereview.chromium.org/2072003002/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller_unittest.mm:172: [[controller_ audioShareCheckbox] setState:NSOffState]; comment about this? https://codereview.chromium.org/2072003002/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller_unittest.mm:176: } At a minimum, the ClickShare test should test each media type. Or there should be a test case for each type, like ClickShareTab ClickShareWindow ClickShareScreen https://codereview.chromium.org/2072003002/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller_unittest.mm:186: Something should get selected here - it's not very interesting otherwise https://codereview.chromium.org/2072003002/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller_unittest.mm:208: ChangeType(content::DesktopMediaID::TYPE_WEB_CONTENTS); Does the WebContents type have thumbnails or just icons? - this probably isn't the appropriate one to test here, but there should be a test for web contents changing the title updating the contents of the table row https://codereview.chromium.org/2072003002/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller_unittest.mm:269: } the other media types have very different codepaths for move as well now - you should add a test with screen or window. edit: or perhaps just use screen or window here, since tabs have some coverage in TabBrowserFocusAlgorithm for remove https://codereview.chromium.org/2072003002/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller_unittest.mm:303: id_with_audio.audio_share = true; shouldn't this be in an EXPECT_TRUE after WaitForCallback() to ensure it matches the checkbox state? there should probably be a test to EXPECT_FALSE when the checkbox is unticked too https://codereview.chromium.org/2072003002/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller_unittest.mm:307: NSIndexSet* indexSet = [NSIndexSet indexSetWithIndex:0]; indexSet -> index_set (this doesn't lie between @implementation/@end, so -> hacker_style for new declarations) https://codereview.chromium.org/2072003002/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller_unittest.mm:328: NSIndexSet* indexSet = [NSIndexSet indexSetWithIndex:1]; index_set https://codereview.chromium.org/2072003002/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller_unittest.mm:333: NSUInteger selectedIndex = [[browser selectedRowIndexes] firstIndex]; selected_index
Patchset #6 (id:140001) has been deleted
fix the unittest https://codereview.chromium.org/2072003002/diff/120001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller_unittest.mm (right): https://codereview.chromium.org/2072003002/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller_unittest.mm:13: #include "chrome/browser/ui/cocoa/media_picker/desktop_media_picker_item.h" On 2016/06/29 12:46:20, tapted wrote: > nit: import Done. https://codereview.chromium.org/2072003002/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller_unittest.mm:16: On 2016/06/29 12:46:20, tapted wrote: > a `using content::DesktopMediaID` here would probably be handy Done. https://codereview.chromium.org/2072003002/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller_unittest.mm:18: content::DesktopMediaID(content::DesktopMediaID::TYPE_WINDOW, id) On 2016/06/29 12:46:20, tapted wrote: > This is not a good reason to use macros see > http://go/cppguide#Preprocessor_Macros. You can add helper methods in an > anonymous namespace > > namespace { > > content::DesktopMediaID MakeWindow(DesktopMediaID::Id id) { > return content::DesktopMediaID(content::DesktopMediaID::TYPE_WINDOW, id); > } > > edit: Something even better, you can put this on the test harness like > > > void AddTab(DesktopMediaID::Id id) { > tab_list_->AddSourceByFullMediaID( > content::DesktopMediaID(content::DesktopMediaID::TYPE_WEB_CONTENTS, id); > } > > void AddWindow(..) { > window_list_->AddSource.. > window_list_->SetSourceThumbnail(id); > } > > and remove a lot of boilerplate Done. https://codereview.chromium.org/2072003002/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller_unittest.mm:122: [controller_ performSelector:[control action] withObject:control]; On 2016/06/29 12:46:19, tapted wrote: > nit: controller_ -> [control target] Done. https://codereview.chromium.org/2072003002/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller_unittest.mm:172: [[controller_ audioShareCheckbox] setState:NSOffState]; On 2016/06/29 12:46:19, tapted wrote: > comment about this? Done. https://codereview.chromium.org/2072003002/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller_unittest.mm:176: } On 2016/06/29 12:46:19, tapted wrote: > At a minimum, the ClickShare test should test each media type. Or there should > be a test case for each type, like > > ClickShareTab > ClickShareWindow > ClickShareScreen > Done. https://codereview.chromium.org/2072003002/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller_unittest.mm:186: On 2016/06/29 12:46:20, tapted wrote: > Something should get selected here - it's not very interesting otherwise Done. https://codereview.chromium.org/2072003002/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller_unittest.mm:208: ChangeType(content::DesktopMediaID::TYPE_WEB_CONTENTS); On 2016/06/29 12:46:19, tapted wrote: > Does the WebContents type have thumbnails or just icons? - this probably isn't > the appropriate one to test here, but there should be a test for web contents > changing the title updating the contents of the table row Just icon, but the way we process it is the same as thumbnail, and the icon can change on the fly. https://codereview.chromium.org/2072003002/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller_unittest.mm:269: } On 2016/06/29 12:46:19, tapted wrote: > the other media types have very different codepaths for move as well now - you > should add a test with screen or window. edit: or perhaps just use screen or > window here, since tabs have some coverage in TabBrowserFocusAlgorithm for > remove Done. https://codereview.chromium.org/2072003002/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller_unittest.mm:303: id_with_audio.audio_share = true; On 2016/06/29 12:46:19, tapted wrote: > shouldn't this be in an EXPECT_TRUE after WaitForCallback() to ensure it matches > the checkbox state? > > there should probably be a test to EXPECT_FALSE when the checkbox is unticked > too No. The ID is passed by value. So the local variable will not be changed. https://codereview.chromium.org/2072003002/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller_unittest.mm:307: NSIndexSet* indexSet = [NSIndexSet indexSetWithIndex:0]; On 2016/06/29 12:46:20, tapted wrote: > indexSet -> index_set (this doesn't lie between @implementation/@end, so -> > hacker_style for new declarations) Done. https://codereview.chromium.org/2072003002/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller_unittest.mm:328: NSIndexSet* indexSet = [NSIndexSet indexSetWithIndex:1]; On 2016/06/29 12:46:19, tapted wrote: > index_set Done. https://codereview.chromium.org/2072003002/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller_unittest.mm:333: NSUInteger selectedIndex = [[browser selectedRowIndexes] firstIndex]; On 2016/06/29 12:46:19, tapted wrote: > selected_index Done.
lgtm https://codereview.chromium.org/2072003002/diff/120001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller_unittest.mm (right): https://codereview.chromium.org/2072003002/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller_unittest.mm:303: id_with_audio.audio_share = true; On 2016/06/29 18:15:04, qiangchenC wrote: > On 2016/06/29 12:46:19, tapted wrote: > > shouldn't this be in an EXPECT_TRUE after WaitForCallback() to ensure it > matches > > the checkbox state? > > > > there should probably be a test to EXPECT_FALSE when the checkbox is unticked > > too > > No. The ID is passed by value. So the local variable will not be changed. Ah, gotcha. It looks kinda like audio_share = true is being passed in, rather than being checked on the reported value only. So this would be clearer with lines 360-361 moved down closer to where they're first used, just before the EXPECT_EQ(id_with_audio, source_reported_); Perhaps with a comment too like // The reported value should match, except the share option should be now be set.
The CQ bit was checked by qiangchen@chromium.org
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: ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
The CQ bit was checked by qiangchen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org Link to the patchset: https://codereview.chromium.org/2072003002/#ps180001 (title: "Rebase")
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 ========== Desktop Capture Picker Window New UI For Mac This CL develops the new Mac UI for Desktop Capture Picker window. The main changes are 1. Separate the items of different source types into different browser view. 2. Use table view rather than image view for tab capture, because we do not have HD preview for tab. BUG=602478, 618796 ========== to ========== Desktop Capture Picker Window New UI For Mac This CL develops the new Mac UI for Desktop Capture Picker window. The main changes are 1. Separate the items of different source types into different browser view. 2. Use table view rather than image view for tab capture, because we do not have HD preview for tab. BUG=602478, 618796 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:180001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Desktop Capture Picker Window New UI For Mac This CL develops the new Mac UI for Desktop Capture Picker window. The main changes are 1. Separate the items of different source types into different browser view. 2. Use table view rather than image view for tab capture, because we do not have HD preview for tab. BUG=602478, 618796 ========== to ========== Desktop Capture Picker Window New UI For Mac This CL develops the new Mac UI for Desktop Capture Picker window. The main changes are 1. Separate the items of different source types into different browser view. 2. Use table view rather than image view for tab capture, because we do not have HD preview for tab. BUG=602478, 618796 Committed: https://crrev.com/f8a7f3f0519e401ddadc437dfce377be5d43ed46 Cr-Commit-Position: refs/heads/master@{#403223} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/f8a7f3f0519e401ddadc437dfce377be5d43ed46 Cr-Commit-Position: refs/heads/master@{#403223} |
