Desktop Capture Picker Window New UI For Mac

chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm

Index: chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm
diff --git a/chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm b/chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm
index 4e8d5c8446a07d4d17c8a1843e8ef204033f5135..f7cb6cf58f331baeaf96cba44ecd7708a12f6542 100644
--- a/chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm
+++ b/chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm
@@ -14,7 +14,9 @@
 #import "chrome/browser/ui/cocoa/key_equivalent_constants.h"
 #import "chrome/browser/ui/cocoa/media_picker/desktop_media_picker_item.h"
 #include "chrome/common/chrome_switches.h"
+#include "chrome/grit/chromium_strings.h"
 #include "chrome/grit/generated_resources.h"
+#include "chrome/grit/google_chrome_strings.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/web_contents.h"
@@ -33,9 +35,14 @@ const int kMinimumContentWidth = 500;
 const int kMinimumContentHeight = 390;
 const int kThumbnailWidth = 150;
 const int kThumbnailHeight = 150;
+const int kSingleScreenWidth = 300;
+const int kSingleScreenHeight = 300;
 const int kFramePadding = 20;
 const int kControlSpacing = 10;
 const int kExcessButtonPadding = 6;
+const int kRowHeight = 20;
+const int kRowWidth = 500;
+const int kIconWidth = 20;
 
 }  // namespace
 
@@ -83,27 +90,28 @@ const int kExcessButtonPadding = 6;
   if ((self = [super initWithWindow:window])) {
     [parent addChildWindow:window ordered:NSWindowAbove];
     [window setDelegate:self];
+    if (screen_list) {
+      screen_list_ = std::move(screen_list);
+      screen_items_.reset([[NSMutableArray alloc] init]);
+    }
+
+    if (window_list) {
+      window_list_ = std::move(window_list);
+      window_list_->SetViewDialogWindowId(content::DesktopMediaID(
+          content::DesktopMediaID::TYPE_WINDOW, [window windowNumber]));
+      window_items_.reset([[NSMutableArray alloc] init]);
+    }
+
+    if (tab_list) {
+      tab_list_ = std::move(tab_list);
+      tab_items_.reset([[NSMutableArray alloc] init]);
+    }
+
     [self initializeContentsWithAppName:appName
                              targetName:targetName
                            requestAudio:requestAudio];
-    std::vector<std::unique_ptr<DesktopMediaList>> media_lists;
-    if (screen_list)
-      media_lists.push_back(std::move(screen_list));
-
-    if (window_list)
-      media_lists.push_back(std::move(window_list));
-
-    if (tab_list)
-      media_lists.push_back(std::move(tab_list));
-
-    if (media_lists.size() > 1)
-      media_list_.reset(new CombinedDesktopMediaList(media_lists));
-    else
-      media_list_ = std::move(media_lists[0]);
-    media_list_->SetViewDialogWindowId(content::DesktopMediaID(
-       content::DesktopMediaID::TYPE_WINDOW, [window windowNumber]));
     doneCallback_ = callback;
-    items_.reset([[NSMutableArray alloc] init]);
+
     bridge_.reset(new DesktopMediaPickerBridge(self));
   }
   return self;
@@ -112,8 +120,12 @@ const int kExcessButtonPadding = 6;
 - (void)dealloc {
   [shareButton_ setTarget:nil];
   [cancelButton_ setTarget:nil];
-  [sourceBrowser_ setDelegate:nil];
-  [sourceBrowser_ setDataSource:nil];
+  [screenBrowser_ setDelegate:nil];
+  [screenBrowser_ setDataSource:nil];
+  [windowBrowser_ setDelegate:nil];
+  [windowBrowser_ setDataSource:nil];
+  [tabBrowser_ setDataSource:nil];
+  [tabBrowser_ setDelegate:nil];
   [[self window] close];
   [super dealloc];
 }
@@ -129,8 +141,7 @@ const int kExcessButtonPadding = 6;
   NSPoint origin = NSMakePoint(kFramePadding, kFramePadding);
 
   // Set the dialog's title.
-  NSString* titleText = l10n_util::GetNSStringF(
-      IDS_DESKTOP_MEDIA_PICKER_TITLE_DEPRECATED, appName);
+  NSString* titleText = l10n_util::GetNSString(IDS_DESKTOP_MEDIA_PICKER_TITLE);
   [[self window] setTitle:titleText];
 
   // Set the dialog's description.
@@ -148,26 +159,110 @@ const int kExcessButtonPadding = 6;
   [content addSubview:description];
   origin.y += NSHeight([description frame]) + kControlSpacing;
 
-  // Create the image browser.
-  sourceBrowser_.reset([[IKImageBrowserView alloc] initWithFrame:NSZeroRect]);
+  // Create segmented button

[tapted, 2016/06/20 12:18:35]

nit: full stop

[qiangchen, 2016/06/21 23:31:28]

Done.

+  sourceTypeControl_.reset([[NSSegmentedControl alloc] init]);
+
+  NSInteger segment_count =

[tapted, 2016/06/20 12:18:35]

segmentCount - more below

[qiangchen, 2016/06/21 23:31:28]

Done.

+      (screen_list_ ? 1 : 0) + (window_list_ ? 1 : 0) + (tab_list_ ? 1 : 0);
+  [sourceTypeControl_ setSegmentCount:segment_count];
+  NSInteger segment_index = 0;
+
+  if (screen_list_) {
+    [sourceTypeControl_
+          setLabel:l10n_util::GetNSString(
+                       IDS_DESKTOP_MEDIA_PICKER_SOURCE_TYPE_SCREEN)
+        forSegment:segment_index];
+
+    [[sourceTypeControl_ cell] setTag:content::DesktopMediaID::TYPE_SCREEN
+                           forSegment:segment_index];
+    ++segment_index;
+  }
+
+  if (window_list_) {
+    [sourceTypeControl_
+          setLabel:l10n_util::GetNSString(
+                       IDS_DESKTOP_MEDIA_PICKER_SOURCE_TYPE_WINDOW)
+        forSegment:segment_index];
+    [[sourceTypeControl_ cell] setTag:content::DesktopMediaID::TYPE_WINDOW
+                           forSegment:segment_index];
+    ++segment_index;
+  }
+
+  if (tab_list_) {
+    [sourceTypeControl_ setLabel:l10n_util::GetNSString(
+                                     IDS_DESKTOP_MEDIA_PICKER_SOURCE_TYPE_TAB)
+                      forSegment:segment_index];
+    [[sourceTypeControl_ cell] setTag:content::DesktopMediaID::TYPE_WEB_CONTENTS
+                           forSegment:segment_index];
+    ++segment_index;
+  }
+  [sourceTypeControl_ setAction:@selector(typeButtonPressed:)];
+
+  [[sourceTypeControl_ cell] setTrackingMode:NSSegmentSwitchTrackingSelectOne];
+
+  [content addSubview:sourceTypeControl_];
+
+  [sourceTypeControl_ sizeToFit];
+  [sourceTypeControl_ setAutoresizingMask:NSViewMaxXMargin | NSViewMinXMargin];
+  CGFloat control_width = NSWidth([sourceTypeControl_ frame]);
+
+  [sourceTypeControl_
+      setFrameOrigin:NSMakePoint((kInitialContentWidth - control_width) / 2,

[tapted, 2016/06/20 12:18:35]

controlWidth can be an odd number, so dividing by 2 might align to a half-pixel
on non-retina. Perhaps use NSIntegralRect

[qiangchen, 2016/06/21 23:31:28]

Done.

+                                 origin.y)];
+  origin.y += NSHeight([sourceTypeControl_ frame]) + kControlSpacing;
+
   NSUInteger cellStyle = IKCellsStyleShadowed | IKCellsStyleTitled;
-  [sourceBrowser_ setDelegate:self];
-  [sourceBrowser_ setDataSource:self];
-  [sourceBrowser_ setCellsStyleMask:cellStyle];
-  [sourceBrowser_ setCellSize:NSMakeSize(kThumbnailWidth, kThumbnailHeight)];
-  [sourceBrowser_ setAllowsMultipleSelection:NO];
+  // Create the image browser.
+  if (screen_list_) {
+    screenBrowser_.reset([[IKImageBrowserView alloc] initWithFrame:NSZeroRect]);

[tapted, 2016/06/20 12:18:35]

This is too much boilerplate - can you make a helper function?

[qiangchen, 2016/06/21 23:31:27]

Done.

+    [screenBrowser_ setDelegate:self];
+    [screenBrowser_ setDataSource:self];
+    [screenBrowser_ setCellsStyleMask:cellStyle];
+    [screenBrowser_
+        setCellSize:NSMakeSize(kSingleScreenWidth, kSingleScreenHeight)];
+    [screenBrowser_ setAllowsMultipleSelection:NO];
+  }
+
+  if (window_list_) {
+    windowBrowser_.reset([[IKImageBrowserView alloc] initWithFrame:NSZeroRect]);
+    [windowBrowser_ setDelegate:self];
+    [windowBrowser_ setDataSource:self];
+    [windowBrowser_ setCellsStyleMask:cellStyle];
+    [windowBrowser_ setCellSize:NSMakeSize(kThumbnailWidth, kThumbnailHeight)];
+    [windowBrowser_ setAllowsMultipleSelection:NO];
+  }
+
+  if (tab_list_) {
+    tabBrowser_.reset([[NSTableView alloc] initWithFrame:NSZeroRect]);
+    [tabBrowser_ setDelegate:self];
+    [tabBrowser_ setDataSource:self];
+    [tabBrowser_ setAllowsMultipleSelection:NO];
+    [tabBrowser_ setRowHeight:kRowHeight];
+    [tabBrowser_ setDoubleAction:@selector(tableDoubleClick:)];
+    NSTableColumn* icon_column =

[tapted, 2016/06/20 12:18:35]

this should be scoped_nsobject - currently it's a memory leak. More below

[qiangchen, 2016/06/21 23:31:27]

Done.

+        [[NSTableColumn alloc] initWithIdentifier:@"icon"];

[tapted, 2016/06/20 12:18:34]

@"icon" and @"title" should be constants

[qiangchen, 2016/06/21 23:31:27]

Done.

+    [icon_column setEditable:NO];
+    [icon_column setWidth:kIconWidth];
+    [tabBrowser_ addTableColumn:icon_column];
+    NSTableColumn* title_column =
+        [[NSTableColumn alloc] initWithIdentifier:@"title"];
+    [title_column setEditable:NO];
+    [title_column setWidth:kRowWidth];
+    [tabBrowser_ addTableColumn:title_column];
+    [tabBrowser_ setHeaderView:nil];
+  }
 
   // Create a scroll view to host the image browser.
   NSRect imageBrowserScrollFrame = NSMakeRect(
       origin.x, origin.y, kPaddedWidth, 350);
-  base::scoped_nsobject<NSScrollView> imageBrowserScroll(
+  imageBrowserScroll_.reset(
       [[NSScrollView alloc] initWithFrame:imageBrowserScrollFrame]);
-  [imageBrowserScroll setHasVerticalScroller:YES];
-  [imageBrowserScroll setDocumentView:sourceBrowser_];
-  [imageBrowserScroll setBorderType:NSBezelBorder];
-  [imageBrowserScroll setAutoresizingMask:
-      NSViewWidthSizable | NSViewHeightSizable];
-  [content addSubview:imageBrowserScroll];
+  [imageBrowserScroll_ setHasVerticalScroller:YES];
+  [imageBrowserScroll_ setBorderType:NSBezelBorder];
+  [imageBrowserScroll_
+      setAutoresizingMask:NSViewWidthSizable | NSViewHeightSizable];
+  [content addSubview:imageBrowserScroll_];
+
   origin.y += NSHeight(imageBrowserScrollFrame) + kControlSpacing;
 
   // Create a checkbox for audio sharing.
@@ -181,7 +276,6 @@ const int kExcessButtonPadding = 6;
     [audioShareCheckbox_
         setTitle:l10n_util::GetNSString(IDS_DESKTOP_MEDIA_PICKER_AUDIO_SHARE)];
     [audioShareCheckbox_ sizeToFit];
-    [audioShareCheckbox_ setEnabled:NO];
     [audioShareCheckbox_
         setToolTip:l10n_util::GetNSString(
                        IDS_DESKTOP_MEDIA_PICKER_AUDIO_SHARE_TOOLTIP_MAC)];
@@ -223,15 +317,38 @@ const int kExcessButtonPadding = 6;
       NSMakeSize(kMinimumContentWidth, kMinimumContentHeight)];
   [[[self window] contentView] setAutoresizesSubviews:YES];
 
-  // Make sourceBrowser_ get keyboard focus.
-  [[self window] makeFirstResponder:sourceBrowser_];
+  // Make source browser get keyboard focus.
+  if (segment_index == 1)

[tapted, 2016/06/20 12:18:34]

segmentIndex should end up in a separate method. segmentCount would be clearer
here

[qiangchen, 2016/06/21 23:31:27]

N/A now.

And I found this logic is actually incorrect. We should put this in the "if
(XXXList_)".

+    [[self window] makeFirstResponder:screenBrowser_];
+  else if (segment_index == 2)
+    [[self window] makeFirstResponder:windowBrowser_];
+  else
+    [[self window] makeFirstResponder:tabBrowser_];
+
+  // Initialize the type selection at the first segment.
+  [sourceTypeControl_ setSelected:YES forSegment:0];
+  [self typeButtonPressed:sourceTypeControl_];
 }

[tapted, 2016/06/20 12:18:35]

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(..)

[qiangchen, 2016/06/21 23:31:28]

Acknowledged.

 
 - (void)showWindow:(id)sender {
   // Signal the media_list to start sending thumbnails. |bridge_| is used as the

[tapted, 2016/06/20 12:18:36]

update comment

[qiangchen, 2016/06/21 23:31:27]

Done.

   // observer, and will forward notifications to this object.
-  media_list_->SetThumbnailSize(gfx::Size(kThumbnailWidth, kThumbnailHeight));
-  media_list_->StartUpdating(bridge_.get());
+  if (screen_list_) {
+    screen_list_->SetThumbnailSize(
+        gfx::Size(kSingleScreenWidth, kSingleScreenHeight));
+    screen_list_->StartUpdating(bridge_.get());
+  }
+
+  if (window_list_) {
+    window_list_->SetThumbnailSize(
+        gfx::Size(kThumbnailWidth, kThumbnailHeight));
+    window_list_->StartUpdating(bridge_.get());
+  }
+
+  if (tab_list_) {
+    tab_list_->SetThumbnailSize(gfx::Size(kIconWidth, kRowHeight));
+    tab_list_->StartUpdating(bridge_.get());
+  }
 
   [self.window center];
   [super showWindow:sender];
@@ -242,7 +359,7 @@ const int kExcessButtonPadding = 6;
     return;
   }
 
-  sourceID.audio_share = [audioShareCheckbox_ isEnabled] &&
+  sourceID.audio_share = ![audioShareCheckbox_ isHidden] &&
                          [audioShareCheckbox_ state] == NSOnState;
 
   // If the media source is an tab, activate it.
@@ -264,10 +381,33 @@ const int kExcessButtonPadding = 6;
 }
 
 - (void)sharePressed:(id)sender {
-  NSIndexSet* indexes = [sourceBrowser_ selectionIndexes];
+  int segment = [sourceTypeControl_ selectedSegment];

[tapted, 2016/06/20 12:18:35]

nit: int -> NSInteger

[qiangchen, 2016/06/21 23:31:28]

Done.

+  content::DesktopMediaID::Type selected_type =

[tapted, 2016/06/20 12:18:35]

selectedType

[qiangchen, 2016/06/21 23:31:26]

Done.

+      static_cast<content::DesktopMediaID::Type>(
+          [[sourceTypeControl_ cell] tagForSegment:segment]);
+
+  if (selected_type == content::DesktopMediaID::TYPE_WEB_CONTENTS) {
+    NSIndexSet* indexes = [tabBrowser_ selectedRowIndexes];

[tapted, 2016/06/20 12:18:34]

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

[qiangchen, 2016/06/21 23:31:27]

Done.

+    NSUInteger selectedIndex = [indexes firstIndex];
+    DesktopMediaPickerItem* item = [tab_items_ objectAtIndex:selectedIndex];
+    [self reportResult:[item sourceID]];
+    [self close];
+    return;
+  }
+
+  NSMutableArray* items;
+  IKImageBrowserView* browser;
+
+  if (selected_type == content::DesktopMediaID::TYPE_SCREEN) {
+    items = screen_items_;
+    browser = screenBrowser_;
+  } else {
+    items = window_items_;
+    browser = windowBrowser_;
+  }
+  NSIndexSet* indexes = [browser selectionIndexes];
   NSUInteger selectedIndex = [indexes firstIndex];
-  DesktopMediaPickerItem* item =
-      [items_ objectAtIndex:selectedIndex];
+  DesktopMediaPickerItem* item = [items objectAtIndex:selectedIndex];
   [self reportResult:[item sourceID]];
   [self close];
 }
@@ -277,6 +417,42 @@ const int kExcessButtonPadding = 6;
   [self close];
 }
 
+- (void)typeButtonPressed:(id)sender {
+  int segment = [sender selectedSegment];
+  content::DesktopMediaID::Type selected_type =
+      static_cast<content::DesktopMediaID::Type>(
+          [[sourceTypeControl_ cell] tagForSegment:segment]);

[tapted, 2016/06/20 12:18:34]

This should be a helper function. E.g.

- (content::DesktopMediaID::Type)selectedMediaType;

use it in sharePressed, imageBrowserSelectionDidChange as well

[qiangchen, 2016/06/21 23:31:27]

Done.

+  if (selected_type == content::DesktopMediaID::TYPE_SCREEN) {

[tapted, 2016/06/20 12:18:36]

this should be a switch statement

[qiangchen, 2016/06/21 23:31:26]

Done.

+    [imageBrowserScroll_ setDocumentView:screenBrowser_];
+    [screenBrowser_ reloadData];
+    [self imageBrowserSelectionDidChange:screenBrowser_];
+    [audioShareCheckbox_ setHidden:YES];
+  } else if (selected_type == content::DesktopMediaID::TYPE_WINDOW) {
+    [imageBrowserScroll_ setDocumentView:windowBrowser_];
+    [windowBrowser_ reloadData];
+    [self imageBrowserSelectionDidChange:windowBrowser_];
+
+    [audioShareCheckbox_ setHidden:YES];
+  } else {
+    [imageBrowserScroll_ setDocumentView:tabBrowser_];
+    [tabBrowser_ reloadData];
+    [self
+        tableViewSelectionDidChange:[NSNotification
+                                        notificationWithName:@"selectionChange"

[tapted, 2016/06/20 12:18:35]

pass nil - the notification isn't used

[qiangchen, 2016/06/21 23:31:27]

Done.

+                                                      object:tabBrowser_]];
+
+    [audioShareCheckbox_ setHidden:NO];
+  }
+}

[tapted, 2016/06/20 12:18:35]

I suspect you need to do something like

[sharebutton_ setEnabled:foo] to ensure a user doesn't select something then
switch sources

[qiangchen, 2016/06/21 23:31:28]

In XXXDidChange(), Share button status will be updated.

+
+- (void)tableDoubleClick:(id)sender {

[tapted, 2016/06/20 12:18:35]

declare this in the private interface

[qiangchen, 2016/06/21 23:31:27]

Done.

+  NSIndexSet* indexes = [tabBrowser_ selectedRowIndexes];
+  NSUInteger selectedIndex = [indexes firstIndex];
+  DesktopMediaPickerItem* item = [tab_items_ objectAtIndex:selectedIndex];
+  [self reportResult:[item sourceID]];
+  [self close];
+}
+
 - (NSTextField*)createTextFieldWithText:(NSString*)text
                              frameWidth:(CGFloat)width {
   NSRect frame = NSMakeRect(0, 0, width, 1);
@@ -318,82 +494,122 @@ const int kExcessButtonPadding = 6;
 #pragma mark IKImageBrowserDataSource
 
 - (NSUInteger)numberOfItemsInImageBrowser:(IKImageBrowserView*)browser {
-  return [items_ count];
+  if (browser == screenBrowser_)
+    return [screen_items_ count];
+  else  // if (browser == windowBrowser_)

[tapted, 2016/06/20 12:18:35]

remove comment

[qiangchen, 2016/06/21 23:31:27]

Done.

+    return [window_items_ count];

[tapted, 2016/06/20 12:18:35]

no else after return

[qiangchen, 2016/06/21 23:31:28]

Done.

 }
 
 - (id)imageBrowser:(IKImageBrowserView *)browser

[tapted, 2016/06/20 12:18:35]

remove space before * - more below

[qiangchen, 2016/06/21 23:31:28]

Done.

        itemAtIndex:(NSUInteger)index {
-  return [items_ objectAtIndex:index];
+  if (browser == screenBrowser_)

[tapted, 2016/06/20 12:18:34]

Looks like another good case for a helper function

- (NSMutableArray*)dataForBrowser:(IKImageBrowserView*)

[qiangchen, 2016/06/21 23:31:28]

Done.

+    return [screen_items_ objectAtIndex:index];
+  else  // if (browser == windowBrowser_)
+    return [window_items_ objectAtIndex:index];
 }
 
 #pragma mark IKImageBrowserDelegate
 
 - (void)imageBrowser:(IKImageBrowserView *)browser
       cellWasDoubleClickedAtIndex:(NSUInteger)index {
-  DesktopMediaPickerItem* item = [items_ objectAtIndex:index];
+  DesktopMediaPickerItem* item;
+  if (browser == screenBrowser_)
+    item = [screen_items_ objectAtIndex:index];
+  else  // if (browser == windowBrowser_)
+    item = [window_items_ objectAtIndex:index];
   [self reportResult:[item sourceID]];
   [self close];
 }
 
 - (void)imageBrowserSelectionDidChange:(IKImageBrowserView*)browser {
-  NSIndexSet* indexes = [sourceBrowser_ selectionIndexes];
+  int segment = [sourceTypeControl_ selectedSegment];
+  content::DesktopMediaID::Type selected_type =
+      static_cast<content::DesktopMediaID::Type>(
+          [[sourceTypeControl_ cell] tagForSegment:segment]);
+  if (selected_type != content::DesktopMediaID::TYPE_SCREEN &&
+      browser == screenBrowser_)
+    return;
+  if (selected_type != content::DesktopMediaID::TYPE_WINDOW &&
+      browser == windowBrowser_)
+    return;
+  NSIndexSet* indexes = [browser selectionIndexes];
 
   // Enable or disable the OK button based on whether we have a selection.
   [shareButton_ setEnabled:([indexes count] > 0)];
+}
+
+#pragma mark NSTableViewDataSource
+- (NSInteger)numberOfRowsInTableView:(NSTableView*)table {
+  return [tab_items_ count];
+}
 
-  // Enable or disable the checkbox based on whether we can support audio for
-  // the selected source.
-  // On Mac, the checkbox will enabled for tab sharing, namely
-  // TYPE_WEB_CONTENTS.
-  if ([indexes count] == 0) {
-    if ([audioShareCheckbox_ isEnabled]) {
-      [audioShareCheckbox_ setEnabled:NO];
-      audioShareState_ = [audioShareCheckbox_ state];
-      [audioShareCheckbox_ setState:NSOffState];
+#pragma mark NSTableViewDelegate
+- (NSView*)tableView:(NSTableView*)table
+    viewForTableColumn:(NSTableColumn*)column
+                   row:(NSInteger)rowIndex {
+  if ([[column identifier] isEqualToString:@"icon"]) {
+    NSImage* image = [[tab_items_ objectAtIndex:rowIndex] imageRepresentation];
+    NSImageView* cell = [table makeViewWithIdentifier:@"icon" owner:self];
+    if (cell == nil) {

[tapted, 2016/06/20 12:18:35]

nit: !cell. more below

[qiangchen, 2016/06/21 23:31:27]

Done.

+      cell = [[NSImageView alloc]

[tapted, 2016/06/20 12:18:35]

this is leaked

[qiangchen, 2016/06/21 23:31:28]

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.

[tapted, 2016/06/22 07:08:41]

see comment on the latest patchset

[qiangchen, 2016/06/22 23:54:23]

Acknowledged.

+          initWithFrame:NSMakeRect(0, 0, kIconWidth, kRowWidth)];
+      cell.identifier = @"icon";

[tapted, 2016/06/20 12:18:34]

don't use dot notation

[qiangchen, 2016/06/21 23:31:27]

Done.

     }
-    [audioShareCheckbox_
-        setToolTip:l10n_util::GetNSString(
-                       IDS_DESKTOP_MEDIA_PICKER_AUDIO_SHARE_TOOLTIP_MAC)];
-    return;
+    [cell setImage:image];
+    return cell;
   }
 
-  NSUInteger selectedIndex = [indexes firstIndex];
-  DesktopMediaPickerItem* item = [items_ objectAtIndex:selectedIndex];
-  switch ([item sourceID].type) {
-    case content::DesktopMediaID::TYPE_SCREEN:
-    case content::DesktopMediaID::TYPE_WINDOW:
-      if ([audioShareCheckbox_ isEnabled]) {
-        [audioShareCheckbox_ setEnabled:NO];
-        audioShareState_ = [audioShareCheckbox_ state];
-        [audioShareCheckbox_ setState:NSOffState];
-      }
-      [audioShareCheckbox_
-          setToolTip:l10n_util::GetNSString(
-                         IDS_DESKTOP_MEDIA_PICKER_AUDIO_SHARE_TOOLTIP_MAC)];
-      break;
-    case content::DesktopMediaID::TYPE_WEB_CONTENTS:
-      if (![audioShareCheckbox_ isEnabled]) {
-        [audioShareCheckbox_ setEnabled:YES];
-        [audioShareCheckbox_ setState:audioShareState_];
-      }
-      [audioShareCheckbox_ setToolTip:@""];
-      break;
-    case content::DesktopMediaID::TYPE_NONE:
-      NOTREACHED();
+  NSString* string = [[tab_items_ objectAtIndex:rowIndex] imageTitle];
+
+  NSTextField* cell = [table makeViewWithIdentifier:@"title" owner:self];
+  if (cell == nil) {
+    cell =
+        [self createTextFieldWithText:string frameWidth:kMinimumContentWidth];
+    cell.identifier = @"title";
+  } else {
+    [cell setStringValue:string];
   }
+  return cell;
+}
+
+- (void)tableViewSelectionDidChange:(NSNotification*)notification {
+  int segment = [sourceTypeControl_ selectedSegment];
+  if ([[sourceTypeControl_ cell] tagForSegment:segment] !=

[tapted, 2016/06/20 12:18:35]

can this happen? DCHECK?

[qiangchen, 2016/06/21 23:31:28]

Done.

+      content::DesktopMediaID::TYPE_WEB_CONTENTS)
+    return;
+  NSIndexSet* indexes = [tabBrowser_ selectedRowIndexes];
+
+  // Enable or disable the OK button based on whether we have a selection.
+  [shareButton_ setEnabled:([indexes count] > 0)];
+  if ([indexes count] == 0)

[tapted, 2016/06/20 12:18:35]

remove

[qiangchen, 2016/06/21 23:31:27]

Done.

+    return;
 }
 
 #pragma mark DesktopMediaPickerObserver
 
-- (void)sourceAddedAtIndex:(int)index {
-  const DesktopMediaList::Source& source = media_list_->GetSource(index);
+- (void)sourceAddedForList:(DesktopMediaList*)list AtIndex:(int)index {
+  NSMutableArray* items;
+  id browser;
+
+  if (list == screen_list_.get()) {

[tapted, 2016/06/20 12:18:34]

this should be a helper function e.g.

id browserForList:(DesktopMediaList*)list (NSMutableArray**)items;

use it in sourceRemovedForList and sourceMovedForList,
sourceThumbnailChangedForList, etc.

[qiangchen, 2016/06/21 23:31:27]

Done.

+    items = screen_items_;
+    browser = screenBrowser_;
+  } else if (list == window_list_.get()) {
+    items = window_items_;
+    browser = windowBrowser_;
+  } else {
+    items = tab_items_;
+    browser = tabBrowser_;
+  }
+
+  const DesktopMediaList::Source& source = list->GetSource(index);
   NSString* imageTitle = base::SysUTF16ToNSString(source.name);
   base::scoped_nsobject<DesktopMediaPickerItem> item(
       [[DesktopMediaPickerItem alloc] initWithSourceId:source.id
                                               imageUID:++lastImageUID_
                                             imageTitle:imageTitle]);
-  [items_ insertObject:item atIndex:index];
-  [sourceBrowser_ reloadData];
+  [items insertObject:item atIndex:index];
+  [browser reloadData];
 
   NSString* autoselectSource = base::SysUTF8ToNSString(
       base::CommandLine::ForCurrentProcess()->GetSwitchValueASCII(
@@ -405,38 +621,99 @@ const int kExcessButtonPadding = 6;
   }
 }
 
-- (void)sourceRemovedAtIndex:(int)index {
-  if ([[sourceBrowser_ selectionIndexes] containsIndex:index]) {
+- (void)sourceRemovedForList:(DesktopMediaList*)list AtIndex:(int)index {
+  NSMutableArray* items;
+  IKImageBrowserView* browser;
+
+  if (list == tab_list_.get()) {
+    [tab_items_ removeObjectAtIndex:index];
+    [tabBrowser_ reloadData];
+    return;
+  }
+
+  if (list == screen_list_.get()) {
+    items = screen_items_;
+    browser = screenBrowser_.get();
+  } else {  // if (list == window_list_.get())

[tapted, 2016/06/20 12:18:35]

remove comment

[qiangchen, 2016/06/21 23:31:27]

Done.

+    items = window_items_;
+    browser = windowBrowser_.get();
+  }
+  if ([[browser selectionIndexes] containsIndex:index]) {
     // Selected item was removed. Clear selection.
-    [sourceBrowser_ setSelectionIndexes:[NSIndexSet indexSet]
-                      byExtendingSelection:FALSE];
+    [browser setSelectionIndexes:[NSIndexSet indexSet]
+            byExtendingSelection:FALSE];

[tapted, 2016/06/20 12:18:34]

FALSE -> NO

[qiangchen, 2016/06/21 23:31:28]

Done.

   }
-  [items_ removeObjectAtIndex:index];
-  [sourceBrowser_ reloadData];
+  [items removeObjectAtIndex:index];
+  [browser reloadData];
 }
 
-- (void)sourceMovedFrom:(int)oldIndex to:(int)newIndex {
+- (void)sourceMovedForList:(DesktopMediaList*)list
+                      From:(int)oldIndex
+                        to:(int)newIndex {
+  NSMutableArray* items;
+  id browser;
+
+  if (list == screen_list_.get()) {
+    items = screen_items_;
+    browser = screenBrowser_;
+  } else if (list == window_list_.get()) {
+    items = window_items_;
+    browser = windowBrowser_;
+  } else {
+    items = tab_items_;
+    browser = tabBrowser_;
+  }
+
   base::scoped_nsobject<DesktopMediaPickerItem> item(
-      [[items_ objectAtIndex:oldIndex] retain]);
-  [items_ removeObjectAtIndex:oldIndex];
-  [items_ insertObject:item atIndex:newIndex];
-  [sourceBrowser_ reloadData];
+      [[items objectAtIndex:oldIndex] retain]);
+  [items removeObjectAtIndex:oldIndex];
+  [items insertObject:item atIndex:newIndex];
+  [browser reloadData];
 }
 
-- (void)sourceNameChangedAtIndex:(int)index {
-  DesktopMediaPickerItem* item = [items_ objectAtIndex:index];
-  const DesktopMediaList::Source& source = media_list_->GetSource(index);
+- (void)sourceNameChangedForList:(DesktopMediaList*)list AtIndex:(int)index {
+  NSMutableArray* items;
+  id browser;
+
+  if (list == screen_list_.get()) {
+    items = screen_items_;
+    browser = screenBrowser_;
+  } else if (list == window_list_.get()) {
+    items = window_items_;
+    browser = windowBrowser_;
+  } else {
+    items = tab_items_;
+    browser = tabBrowser_;
+  }
+
+  DesktopMediaPickerItem* item = [items objectAtIndex:index];
+  const DesktopMediaList::Source& source = list->GetSource(index);
   [item setImageTitle:base::SysUTF16ToNSString(source.name)];
-  [sourceBrowser_ reloadData];
+  [browser reloadData];
 }
 
-- (void)sourceThumbnailChangedAtIndex:(int)index {
-  const DesktopMediaList::Source& source = media_list_->GetSource(index);
+- (void)sourceThumbnailChangedForList:(DesktopMediaList*)list
+                              AtIndex:(int)index {
+  NSMutableArray* items;
+  id browser;
+
+  if (list == screen_list_.get()) {
+    items = screen_items_;
+    browser = screenBrowser_;
+  } else if (list == window_list_.get()) {
+    items = window_items_;
+    browser = windowBrowser_;
+  } else {
+    items = tab_items_;
+    browser = tabBrowser_;
+  }
+
+  const DesktopMediaList::Source& source = list->GetSource(index);
   NSImage* image = gfx::NSImageFromImageSkia(source.thumbnail);
 
-  DesktopMediaPickerItem* item = [items_ objectAtIndex:index];
+  DesktopMediaPickerItem* item = [items objectAtIndex:index];
   [item setImageRepresentation:image];
-  [sourceBrowser_ reloadData];
+  [browser reloadData];
 }
 
 @end  // @interface DesktopMediaPickerController