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..e4cb6199fdf58125fc128ad67163c31bffe2c412 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"
@@ -28,14 +30,23 @@
 
 namespace {

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

consider a

using content::DesktopMediaID;

before this, to remove lots of uninteresting `content::` from the file

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

Done.

 
-const int kInitialContentWidth = 620;
-const int kMinimumContentWidth = 500;
-const int kMinimumContentHeight = 390;
-const int kThumbnailWidth = 150;
-const int kThumbnailHeight = 150;
-const int kFramePadding = 20;
-const int kControlSpacing = 10;
-const int kExcessButtonPadding = 6;
+const CGFloat kInitialContentWidth = 620;
+const CGFloat kMinimumContentWidth = 500;
+const CGFloat kMinimumContentHeight = 390;
+const CGFloat kThumbnailWidth = 150;
+const CGFloat kThumbnailHeight = 150;
+const CGFloat kSingleScreenWidth = 300;
+const CGFloat kSingleScreenHeight = 300;
+const CGFloat kFramePadding = 20;
+const CGFloat kControlSpacing = 10;
+const CGFloat kExcessButtonPadding = 6;
+const CGFloat kRowHeight = 20;
+const CGFloat kRowWidth = 500;
+const CGFloat kIconWidth = 20;
+const CGFloat kPaddedWidth = kInitialContentWidth - (kFramePadding * 2);
+
+NSString* const kIConColumnIdentifier = @"icon";

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

kICon -> kIcon

If you want something shorter, kIconId / kTitleId would be fine too

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

Done.

+NSString* const kTitleColumnIdentifier = @"title";
 
 }  // namespace
 
@@ -59,14 +70,15 @@ const int kExcessButtonPadding = 6;
 // Action handlers.
 - (void)sharePressed:(id)sender;
 - (void)cancelPressed:(id)sender;
+- (void)tableDoubleClick:(id)sender;
 
 @end
 
 @implementation DesktopMediaPickerController
 
-- (id)initWithScreenList:(std::unique_ptr<DesktopMediaList>)screen_list
-              windowList:(std::unique_ptr<DesktopMediaList>)window_list
-                 tabList:(std::unique_ptr<DesktopMediaList>)tab_list
+- (id)initWithScreenList:(std::unique_ptr<DesktopMediaList>)screenList
+              windowList:(std::unique_ptr<DesktopMediaList>)windowList
+                 tabList:(std::unique_ptr<DesktopMediaList>)tabList
                   parent:(NSWindow*)parent
                 callback:(const DesktopMediaPicker::DoneCallback&)callback
                  appName:(const base::string16&)appName
@@ -83,27 +95,28 @@ const int kExcessButtonPadding = 6;
   if ((self = [super initWithWindow:window])) {
     [parent addChildWindow:window ordered:NSWindowAbove];
     [window setDelegate:self];
+    if (screenList) {
+      screenList_ = std::move(screenList);
+      screenItems_.reset([[NSMutableArray alloc] init]);
+    }
+
+    if (windowList) {
+      windowList_ = std::move(windowList);
+      windowList_->SetViewDialogWindowId(content::DesktopMediaID(
+          content::DesktopMediaID::TYPE_WINDOW, [window windowNumber]));
+      windowItems_.reset([[NSMutableArray alloc] init]);
+    }
+
+    if (tabList) {
+      tabList_ = std::move(tabList);
+      tabItems_.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 +125,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];
 }
@@ -122,15 +139,13 @@ const int kExcessButtonPadding = 6;
                            targetName:(const base::string16&)targetName
                          requestAudio:(bool)requestAudio {
   // Use flipped coordinates to facilitate manual layout.
-  const CGFloat kPaddedWidth = kInitialContentWidth - (kFramePadding * 2);
   base::scoped_nsobject<FlippedView> content(
       [[FlippedView alloc] initWithFrame:NSZeroRect]);
   [[self window] setContentView:content];
   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,52 +163,178 @@ const int kExcessButtonPadding = 6;
   [content addSubview:description];
   origin.y += NSHeight([description frame]) + kControlSpacing;
 
-  // Create the image browser.
-  sourceBrowser_.reset([[IKImageBrowserView alloc] initWithFrame:NSZeroRect]);
-  NSUInteger cellStyle = IKCellsStyleShadowed | IKCellsStyleTitled;
-  [sourceBrowser_ setDelegate:self];
-  [sourceBrowser_ setDataSource:self];
-  [sourceBrowser_ setCellsStyleMask:cellStyle];
-  [sourceBrowser_ setCellSize:NSMakeSize(kThumbnailWidth, kThumbnailHeight)];
-  [sourceBrowser_ setAllowsMultipleSelection:NO];
+  [self createTypeButtonAtOrigin:origin];
+  origin.y += NSHeight([sourceTypeControl_ frame]) + kControlSpacing;
+
+  [self createSourceViewsAtOrigin:origin];
+  origin.y += NSHeight([imageBrowserScroll_ frame]) + kControlSpacing;
 
-  // Create a scroll view to host the image browser.
-  NSRect imageBrowserScrollFrame = NSMakeRect(
-      origin.x, origin.y, kPaddedWidth, 350);
-  base::scoped_nsobject<NSScrollView> imageBrowserScroll(
-      [[NSScrollView alloc] initWithFrame:imageBrowserScrollFrame]);
-  [imageBrowserScroll setHasVerticalScroller:YES];
-  [imageBrowserScroll setDocumentView:sourceBrowser_];
-  [imageBrowserScroll setBorderType:NSBezelBorder];
-  [imageBrowserScroll setAutoresizingMask:
-      NSViewWidthSizable | NSViewHeightSizable];
-  [content addSubview:imageBrowserScroll];
-  origin.y += NSHeight(imageBrowserScrollFrame) + kControlSpacing;
-
-  // Create a checkbox for audio sharing.
   if (requestAudio) {
-    audioShareCheckbox_.reset([[NSButton alloc] initWithFrame:NSZeroRect]);
-    [audioShareCheckbox_ setFrameOrigin:origin];
-    [audioShareCheckbox_
-        setAutoresizingMask:NSViewMaxXMargin | NSViewMinYMargin];
-    [audioShareCheckbox_ setButtonType:NSSwitchButton];
-    audioShareState_ = NSOnState;
-    [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)];
-    [content addSubview:audioShareCheckbox_];
+    [self createAudioCheckboxAtOrigin:origin];
     origin.y += NSHeight([audioShareCheckbox_ frame]) + kControlSpacing;
   }
 
+  [self createActionButtonsAtOrigin:origin];
+  origin.y +=
+      kFramePadding + (NSHeight([cancelButton_ frame]) - kExcessButtonPadding);
+
+  // Resize window to fit.
+  [content setAutoresizesSubviews:NO];
+  [[self window] setContentSize:NSMakeSize(kInitialContentWidth, origin.y)];
+  [[self window] setContentMinSize:NSMakeSize(kMinimumContentWidth,
+                                              kMinimumContentHeight)];
+  [[[self window] contentView] setAutoresizesSubviews:YES];

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

[[self window] contentView]  -> content?

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

Done.

+
+  // Initialize the type selection at the first segment.
+  [sourceTypeControl_ setSelected:YES forSegment:0];
+  [self typeButtonPressed:sourceTypeControl_];
+}
+
+- (void)createTypeButtonAtOrigin:(NSPoint)origin {

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

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

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

Done.

+  FlippedView* content = [[self window] contentView];
+  // Create segmented button.
+  sourceTypeControl_.reset([[NSSegmentedControl alloc] init]);
+
+  NSInteger segmentCount =
+      (screenList_ ? 1 : 0) + (windowList_ ? 1 : 0) + (tabList_ ? 1 : 0);
+  [sourceTypeControl_ setSegmentCount:segmentCount];
+  NSInteger segmentIndex = 0;
+
+  if (screenList_) {
+    [sourceTypeControl_
+          setLabel:l10n_util::GetNSString(
+                       IDS_DESKTOP_MEDIA_PICKER_SOURCE_TYPE_SCREEN)
+        forSegment:segmentIndex];
+
+    [[sourceTypeControl_ cell] setTag:content::DesktopMediaID::TYPE_SCREEN
+                           forSegment:segmentIndex];
+    ++segmentIndex;
+  }
+
+  if (windowList_) {
+    [sourceTypeControl_
+          setLabel:l10n_util::GetNSString(
+                       IDS_DESKTOP_MEDIA_PICKER_SOURCE_TYPE_WINDOW)
+        forSegment:segmentIndex];
+    [[sourceTypeControl_ cell] setTag:content::DesktopMediaID::TYPE_WINDOW
+                           forSegment:segmentIndex];
+    ++segmentIndex;
+  }
+
+  if (tabList_) {
+    [sourceTypeControl_ setLabel:l10n_util::GetNSString(
+                                     IDS_DESKTOP_MEDIA_PICKER_SOURCE_TYPE_TAB)
+                      forSegment:segmentIndex];
+    [[sourceTypeControl_ cell] setTag:content::DesktopMediaID::TYPE_WEB_CONTENTS
+                           forSegment:segmentIndex];
+  }
+  [sourceTypeControl_ setAction:@selector(typeButtonPressed:)];
+
+  [[sourceTypeControl_ cell] setTrackingMode:NSSegmentSwitchTrackingSelectOne];
+
+  [content addSubview:sourceTypeControl_];
+
+  [sourceTypeControl_ sizeToFit];
+  [sourceTypeControl_ setAutoresizingMask:NSViewMaxXMargin | NSViewMinXMargin];
+  CGFloat controlWidth = NSWidth([sourceTypeControl_ frame]);
+  CGFloat controlHeight = NSHeight([sourceTypeControl_ frame]);
+  NSRect centerFrame = NSMakeRect((kInitialContentWidth - controlWidth) / 2,
+                                  origin.y, controlWidth, controlHeight);
+
+  [sourceTypeControl_ setFrame:NSIntegralRect(centerFrame)];
+}
+
+- (void)createSourceViewsAtOrigin:(NSPoint)origin {
+  NSUInteger cellStyle = IKCellsStyleShadowed | IKCellsStyleTitled;
+  bool focusSet = false;
+
+  if (screenList_) {
+    screenBrowser_.reset([[IKImageBrowserView alloc] initWithFrame:NSZeroRect]);

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

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();
}

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

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.

+    [screenBrowser_ setDelegate:self];
+    [screenBrowser_ setDataSource:self];
+    [screenBrowser_ setCellsStyleMask:cellStyle];
+    [screenBrowser_
+        setCellSize:NSMakeSize(kSingleScreenWidth, kSingleScreenHeight)];
+    [screenBrowser_ setAllowsMultipleSelection:NO];
+
+    if (!focusSet) {
+      [[self window] makeFirstResponder:screenBrowser_];

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

How about once, at the end of initializeContentsWithAppName:

  [[self window] makeFirstResponder:
      [self browserViewForType:[self selectedSourceType]]];

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

Done.

+      focusSet = true;
+    }
+  }
+
+  if (windowList_) {
+    windowBrowser_.reset([[IKImageBrowserView alloc] initWithFrame:NSZeroRect]);
+    [windowBrowser_ setDelegate:self];
+    [windowBrowser_ setDataSource:self];
+    [windowBrowser_ setCellsStyleMask:cellStyle];
+    [windowBrowser_ setCellSize:NSMakeSize(kThumbnailWidth, kThumbnailHeight)];
+    [windowBrowser_ setAllowsMultipleSelection:NO];
+
+    if (!focusSet) {
+      [[self window] makeFirstResponder:windowBrowser_];
+      focusSet = true;
+    }
+  }
+
+  if (tabList_) {
+    tabBrowser_.reset([[NSTableView alloc] initWithFrame:NSZeroRect]);
+    [tabBrowser_ setDelegate:self];
+    [tabBrowser_ setDataSource:self];
+    [tabBrowser_ setAllowsMultipleSelection:NO];
+    [tabBrowser_ setRowHeight:kRowHeight];
+    [tabBrowser_ setDoubleAction:@selector(tableDoubleClick:)];
+    base::scoped_nsobject<NSTableColumn> icon_column(

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

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

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

Done.

+        [[NSTableColumn alloc] initWithIdentifier:kIConColumnIdentifier]);
+    [icon_column setEditable:NO];
+    [icon_column setWidth:kIconWidth];
+    [tabBrowser_ addTableColumn:icon_column];
+    base::scoped_nsobject<NSTableColumn> title_column(
+        [[NSTableColumn alloc] initWithIdentifier:kTitleColumnIdentifier]);
+    [title_column setEditable:NO];
+    [title_column setWidth:kRowWidth];
+    [tabBrowser_ addTableColumn:title_column];
+    [tabBrowser_ setHeaderView:nil];
+
+    if (!focusSet) {
+      [[self window] makeFirstResponder:tabBrowser_];
+      focusSet = true;
+    }
+  }
+
+  // Create a scroll view to host the image browser.

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

image browser -> browsers

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

Done.

+  NSRect imageBrowserScrollFrame =
+      NSMakeRect(origin.x, origin.y, kPaddedWidth, 350);
+  imageBrowserScroll_.reset(
+      [[NSScrollView alloc] initWithFrame:imageBrowserScrollFrame]);
+  [imageBrowserScroll_ setHasVerticalScroller:YES];
+  [imageBrowserScroll_ setBorderType:NSBezelBorder];
+  [imageBrowserScroll_
+      setAutoresizingMask:NSViewWidthSizable | NSViewHeightSizable];
+  [[[self window] contentView] addSubview:imageBrowserScroll_];
+}
+
+- (void)createAudioCheckboxAtOrigin:(NSPoint)origin {
+  audioShareCheckbox_.reset([[NSButton alloc] initWithFrame:NSZeroRect]);
+  [audioShareCheckbox_ setFrameOrigin:origin];
+  [audioShareCheckbox_ setAutoresizingMask:NSViewMaxXMargin | NSViewMinYMargin];
+  [audioShareCheckbox_ setButtonType:NSSwitchButton];
+  [audioShareCheckbox_ setState:NSOnState];
+  [audioShareCheckbox_
+      setTitle:l10n_util::GetNSString(IDS_DESKTOP_MEDIA_PICKER_AUDIO_SHARE)];
+  [audioShareCheckbox_ sizeToFit];
+  [[[self window] contentView] addSubview:audioShareCheckbox_];
+}
+
+- (void)createActionButtonsAtOrigin:(NSPoint)origin {
+  FlippedView* content = [[self window] contentView];
+
   // Create the share button.
-  shareButton_ = [self createButtonWithTitle:l10n_util::GetNSString(
-      IDS_DESKTOP_MEDIA_PICKER_SHARE)];
+  shareButton_ =
+      [self createButtonWithTitle:l10n_util::GetNSString(
+                                      IDS_DESKTOP_MEDIA_PICKER_SHARE)];
   origin.x = kInitialContentWidth - kFramePadding -
-      (NSWidth([shareButton_ frame]) - kExcessButtonPadding);
+             (NSWidth([shareButton_ frame]) - kExcessButtonPadding);
   [shareButton_ setEnabled:NO];
   [shareButton_ setFrameOrigin:origin];
   [shareButton_ setAutoresizingMask:NSViewMinXMargin | NSViewMinYMargin];
@@ -206,32 +347,35 @@ const int kExcessButtonPadding = 6;
   cancelButton_ =
       [self createButtonWithTitle:l10n_util::GetNSString(IDS_CANCEL)];
   origin.x -= kControlSpacing +
-      (NSWidth([cancelButton_ frame]) - (kExcessButtonPadding * 2));
+              (NSWidth([cancelButton_ frame]) - (kExcessButtonPadding * 2));
   [cancelButton_ setFrameOrigin:origin];
   [cancelButton_ setAutoresizingMask:NSViewMinXMargin | NSViewMinYMargin];
   [cancelButton_ setTarget:self];
   [cancelButton_ setKeyEquivalent:kKeyEquivalentEscape];
   [cancelButton_ setAction:@selector(cancelPressed:)];
   [content addSubview:cancelButton_];
-  origin.y += kFramePadding +
-      (NSHeight([cancelButton_ frame]) - kExcessButtonPadding);
-
-  // Resize window to fit.
-  [[[self window] contentView] setAutoresizesSubviews:NO];
-  [[self window] setContentSize:NSMakeSize(kInitialContentWidth, origin.y)];
-  [[self window] setContentMinSize:
-      NSMakeSize(kMinimumContentWidth, kMinimumContentHeight)];
-  [[[self window] contentView] setAutoresizesSubviews:YES];
-
-  // Make sourceBrowser_ get keyboard focus.
-  [[self window] makeFirstResponder:sourceBrowser_];
 }
 
+#pragma mark Event Actions
+
 - (void)showWindow:(id)sender {
-  // Signal the media_list to start sending thumbnails. |bridge_| is used as the
-  // observer, and will forward notifications to this object.
-  media_list_->SetThumbnailSize(gfx::Size(kThumbnailWidth, kThumbnailHeight));
-  media_list_->StartUpdating(bridge_.get());
+  // Signal the source lists to start sending thumbnails. |bridge_| is used as
+  // the observer, and will forward notifications to this object.
+  if (screenList_) {
+    screenList_->SetThumbnailSize(
+        gfx::Size(kSingleScreenWidth, kSingleScreenHeight));
+    screenList_->StartUpdating(bridge_.get());
+  }
+
+  if (windowList_) {
+    windowList_->SetThumbnailSize(gfx::Size(kThumbnailWidth, kThumbnailHeight));
+    windowList_->StartUpdating(bridge_.get());
+  }
+
+  if (tabList_) {
+    tabList_->SetThumbnailSize(gfx::Size(kIconWidth, kRowHeight));
+    tabList_->StartUpdating(bridge_.get());
+  }
 
   [self.window center];
   [super showWindow:sender];
@@ -242,7 +386,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 +408,10 @@ const int kExcessButtonPadding = 6;
 }
 
 - (void)sharePressed:(id)sender {
-  NSIndexSet* indexes = [sourceBrowser_ selectionIndexes];
-  NSUInteger selectedIndex = [indexes firstIndex];
-  DesktopMediaPickerItem* item =
-      [items_ objectAtIndex:selectedIndex];
+  content::DesktopMediaID::Type selectedType = [self selectedSourceType];
+  NSMutableArray* items = [self itemSetForType:selectedType];
+  NSInteger selectedIndex = [self selectedIndexForType:selectedType];
+  DesktopMediaPickerItem* item = [items objectAtIndex:selectedIndex];
   [self reportResult:[item sourceID]];
   [self close];
 }
@@ -277,6 +421,33 @@ const int kExcessButtonPadding = 6;
   [self close];
 }
 
+- (void)typeButtonPressed:(id)sender {
+  content::DesktopMediaID::Type selectedType = [self selectedSourceType];
+  id browser = [self browserViewForType:selectedType];
+
+  [audioShareCheckbox_
+      setHidden:selectedType != content::DesktopMediaID::TYPE_WEB_CONTENTS];
+  [imageBrowserScroll_ setDocumentView:browser];
+
+  if (selectedType == content::DesktopMediaID::TYPE_WEB_CONTENTS) {
+    NSInteger selectedIndex = [self selectedIndexForType:selectedType];
+    [tabBrowser_ reloadData];
+    [self setTabBrowserIndex:selectedIndex];
+    [self tableViewSelectionDidChange:nil];

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

is this invoked automatically? (comment?)

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

Done.

+  } else {
+    [browser reloadData];
+    [self imageBrowserSelectionDidChange:browser];
+  }
+}
+
+- (void)tableDoubleClick:(id)sender {
+  NSIndexSet* indexes = [tabBrowser_ selectedRowIndexes];

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

Can this just call [self sharePressed:sender]

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

Actually we can do simpler: just bind sharePressed: to double click action.

+  NSUInteger selectedIndex = [indexes firstIndex];
+  DesktopMediaPickerItem* item = [tabItems_ objectAtIndex:selectedIndex];
+  [self reportResult:[item sourceID]];
+  [self close];
+}
+
 - (NSTextField*)createTextFieldWithText:(NSString*)text
                              frameWidth:(CGFloat)width {
   NSRect frame = NSMakeRect(0, 0, width, 1);
@@ -303,6 +474,90 @@ const int kExcessButtonPadding = 6;
   return button.autorelease();
 }
 
+#pragma mark Data Retrieve Helper
+
+- (content::DesktopMediaID::Type)selectedSourceType {

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

These should all be declared in `@interface DesktopMediaPickerController ()`

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

Done.

+  NSInteger segment = [sourceTypeControl_ selectedSegment];
+  return static_cast<content::DesktopMediaID::Type>(
+      [[sourceTypeControl_ cell] tagForSegment:segment]);
+}
+
+- (content::DesktopMediaID::Type)sourceTypeForList:(DesktopMediaList*)list {
+  if (list == screenList_.get())
+    return content::DesktopMediaID::TYPE_SCREEN;
+  else if (list == windowList_.get())

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

no `else`. just `if`

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

Done.

+    return content::DesktopMediaID::TYPE_WINDOW;
+  return content::DesktopMediaID::TYPE_WEB_CONTENTS;
+}
+
+- (content::DesktopMediaID::Type)sourceTypeForBrowser:(id)browser {
+  if (browser == screenBrowser_.get())
+    return content::DesktopMediaID::TYPE_SCREEN;
+  else if (browser == windowBrowser_.get())

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

no `else`

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

Done.

+    return content::DesktopMediaID::TYPE_WINDOW;
+  return content::DesktopMediaID::TYPE_WEB_CONTENTS;
+}
+
+- (id)browserViewForType:(content::DesktopMediaID::Type)sourceType {
+  switch (sourceType) {
+    case content::DesktopMediaID::TYPE_SCREEN:
+      return screenBrowser_;
+    case content::DesktopMediaID::TYPE_WINDOW:
+      return windowBrowser_;
+    case content::DesktopMediaID::TYPE_WEB_CONTENTS:
+      return tabBrowser_;
+    case content::DesktopMediaID::TYPE_NONE:
+      NOTREACHED();
+      return nil;
+  }
+}
+
+- (NSMutableArray*)itemSetForType:(content::DesktopMediaID::Type)sourceType {
+  switch (sourceType) {
+    case content::DesktopMediaID::TYPE_SCREEN:
+      return screenItems_;
+    case content::DesktopMediaID::TYPE_WINDOW:
+      return windowItems_;
+    case content::DesktopMediaID::TYPE_WEB_CONTENTS:
+      return tabItems_;
+    case content::DesktopMediaID::TYPE_NONE:
+      NOTREACHED();
+      return nil;
+  }
+}
+
+- (NSInteger)selectedIndexForType:(content::DesktopMediaID::Type)sourceType {
+  NSIndexSet* indexes = nil;
+  switch (sourceType) {
+    case content::DesktopMediaID::TYPE_SCREEN:
+      indexes = [screenBrowser_ selectionIndexes];
+      break;
+    case content::DesktopMediaID::TYPE_WINDOW:
+      indexes = [windowBrowser_ selectionIndexes];
+      break;
+    case content::DesktopMediaID::TYPE_WEB_CONTENTS:
+      indexes = [tabBrowser_ selectedRowIndexes];
+      break;
+    case content::DesktopMediaID::TYPE_NONE:
+      NOTREACHED();
+  }
+
+  if ([indexes count] == 0)
+    return -1;
+  return [indexes firstIndex];
+}
+
+- (void)setTabBrowserIndex:(NSInteger)index {
+  NSIndexSet* indexes;
+
+  if (index < 0)
+    indexes = [NSIndexSet indexSet];
+  else
+    indexes = [NSIndexSet indexSetWithIndex:index];
+
+  [tabBrowser_ selectRowIndexes:indexes byExtendingSelection:NO];
+}
+
 #pragma mark NSWindowDelegate
 
 - (void)windowWillClose:(NSNotification*)notification {
@@ -318,82 +573,102 @@ const int kExcessButtonPadding = 6;
 #pragma mark IKImageBrowserDataSource
 
 - (NSUInteger)numberOfItemsInImageBrowser:(IKImageBrowserView*)browser {
-  return [items_ count];
+  content::DesktopMediaID::Type sourceType =
+      [self sourceTypeForBrowser:browser];
+  NSMutableArray* items = [self itemSetForType:sourceType];
+  return [items count];
 }
 
-- (id)imageBrowser:(IKImageBrowserView *)browser
-       itemAtIndex:(NSUInteger)index {
-  return [items_ objectAtIndex:index];
+- (id)imageBrowser:(IKImageBrowserView*)browser itemAtIndex:(NSUInteger)index {
+  content::DesktopMediaID::Type sourceType =
+      [self sourceTypeForBrowser:browser];
+  NSMutableArray* items = [self itemSetForType:sourceType];
+  return [items objectAtIndex:index];
 }
 
 #pragma mark IKImageBrowserDelegate
 
-- (void)imageBrowser:(IKImageBrowserView *)browser
-      cellWasDoubleClickedAtIndex:(NSUInteger)index {
-  DesktopMediaPickerItem* item = [items_ objectAtIndex:index];
+- (void)imageBrowser:(IKImageBrowserView*)browser
+    cellWasDoubleClickedAtIndex:(NSUInteger)index {
+  DesktopMediaPickerItem* item;
+  if (browser == screenBrowser_)
+    item = [screenItems_ objectAtIndex:index];
+  else
+    item = [windowItems_ objectAtIndex:index];
   [self reportResult:[item sourceID]];
   [self close];
 }
 
 - (void)imageBrowserSelectionDidChange:(IKImageBrowserView*)browser {
-  NSIndexSet* indexes = [sourceBrowser_ selectionIndexes];
-
+  content::DesktopMediaID::Type selectedType = [self selectedSourceType];
+  NSInteger selectedIndex = [self selectedIndexForType:selectedType];
   // Enable or disable the OK button based on whether we have a selection.
-  [shareButton_ setEnabled:([indexes count] > 0)];
+  [shareButton_ setEnabled:(selectedIndex >= 0)];
+}
 
-  // 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 NSTableViewDataSource
+
+- (NSInteger)numberOfRowsInTableView:(NSTableView*)table {
+  return [tabItems_ count];
+}
+
+#pragma mark NSTableViewDelegate
+- (NSView*)tableView:(NSTableView*)table

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

nit: blank line before

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

Done.

+    viewForTableColumn:(NSTableColumn*)column
+                   row:(NSInteger)rowIndex {
+  if ([[column identifier] isEqualToString:kIConColumnIdentifier]) {
+    NSImage* image = [[tabItems_ objectAtIndex:rowIndex] imageRepresentation];
+    base::scoped_nsobject<NSImageView> cell(

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

nit: cell -> view. (cell is confusing because NSTableView has modes where it
populates with cells or views, but you've chosen the view mode)

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

Done.

+        [table makeViewWithIdentifier:kIConColumnIdentifier owner:self]);

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

[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]);

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

Done.

+    if (!cell) {
+      cell.reset([[NSImageView alloc]
+          initWithFrame:NSMakeRect(0, 0, kIconWidth, kRowWidth)]);
+      [cell setIdentifier:kIConColumnIdentifier];
     }
-    [audioShareCheckbox_
-        setToolTip:l10n_util::GetNSString(
-                       IDS_DESKTOP_MEDIA_PICKER_AUDIO_SHARE_TOOLTIP_MAC)];
-    return;
+    [cell setImage:image];
+    return cell.release();

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

release -> autorelease (otherwise it's a leak)

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

Done.

   }
 
-  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 = [[tabItems_ objectAtIndex:rowIndex] imageTitle];
+  base::scoped_nsobject<NSTextField> cell(

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

cell -> view

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

Done.

+      [table makeViewWithIdentifier:kTitleColumnIdentifier owner:self]);
+  if (!cell) {
+    cell.reset(
+        [self createTextFieldWithText:string frameWidth:kMinimumContentWidth]);
+    [cell setIdentifier:kTitleColumnIdentifier];
+  } else {
+    [cell setStringValue:string];
   }
+  return cell.release();

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

autorelease, and retain above.

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

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.

[tapted, 2016/06/23 02:49:48]

autoreleasing twice is fine, you just need to retain it again beforehand.

[qiangchen, 2016/06/23 17:49:37]

Acknowledged.

+}
+
+- (void)tableViewSelectionDidChange:(NSNotification*)notification {
+  NSIndexSet* indexes = [tabBrowser_ selectedRowIndexes];
+
+  // Enable or disable the OK button based on whether we have a selection.
+  [shareButton_ setEnabled:([indexes count] > 0)];
 }
 
 #pragma mark DesktopMediaPickerObserver
 
-- (void)sourceAddedAtIndex:(int)index {
-  const DesktopMediaList::Source& source = media_list_->GetSource(index);
+- (void)sourceAddedForList:(DesktopMediaList*)list atIndex:(int)index {
+  content::DesktopMediaID::Type sourceType = [self sourceTypeForList:list];
+  NSMutableArray* items = [self itemSetForType:sourceType];
+  id browser = [self browserViewForType:sourceType];
+  NSInteger selectedIndex = [self selectedIndexForType:sourceType];
+  if (selectedIndex >= index)
+    ++selectedIndex;
+
+  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];
+  if (sourceType == content::DesktopMediaID::TYPE_WEB_CONTENTS)
+    [self setTabBrowserIndex:selectedIndex];
 
   NSString* autoselectSource = base::SysUTF8ToNSString(
       base::CommandLine::ForCurrentProcess()->GetSwitchValueASCII(
@@ -405,38 +680,86 @@ const int kExcessButtonPadding = 6;
   }
 }
 
-- (void)sourceRemovedAtIndex:(int)index {
-  if ([[sourceBrowser_ selectionIndexes] containsIndex:index]) {
+- (void)sourceRemovedForList:(DesktopMediaList*)list atIndex:(int)index {
+  content::DesktopMediaID::Type sourceType = [self sourceTypeForList:list];
+  NSMutableArray* items = [self itemSetForType:sourceType];
+  id browser = [self browserViewForType:sourceType];
+
+  if (sourceType == content::DesktopMediaID::TYPE_WEB_CONTENTS) {
+    NSInteger selectedIndex =
+        [self selectedIndexForType:content::DesktopMediaID::TYPE_WEB_CONTENTS];

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

basee sourceType - it's shorter

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

Done.

+    if (selectedIndex > index)
+      --selectedIndex;
+    else if (selectedIndex == index)
+      selectedIndex = -1;
+    [tabItems_ removeObjectAtIndex:index];
+    [tabBrowser_ reloadData];
+    [self setTabBrowserIndex:selectedIndex];
+    return;
+  }
+
+  if ([[browser selectionIndexes] containsIndex:index]) {
     // Selected item was removed. Clear selection.
-    [sourceBrowser_ setSelectionIndexes:[NSIndexSet indexSet]
-                      byExtendingSelection:FALSE];
+    [browser setSelectionIndexes:[NSIndexSet indexSet] byExtendingSelection:NO];
   }
-  [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 {
+  content::DesktopMediaID::Type sourceType = [self sourceTypeForList:list];
+  NSMutableArray* items = [self itemSetForType:sourceType];
+  id browser = [self browserViewForType:sourceType];
+  NSInteger selectedIndex = [self selectedIndexForType:sourceType];
+  if (selectedIndex > oldIndex && selectedIndex <= newIndex)
+    --selectedIndex;
+  else if (selectedIndex < oldIndex && selectedIndex >= newIndex)
+    ++selectedIndex;
+  else if (selectedIndex == oldIndex)
+    selectedIndex = newIndex;
+
   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];
+
+  if (sourceType == content::DesktopMediaID::TYPE_WEB_CONTENTS)
+    [self setTabBrowserIndex:selectedIndex];
 }
 
-- (void)sourceNameChangedAtIndex:(int)index {
-  DesktopMediaPickerItem* item = [items_ objectAtIndex:index];
-  const DesktopMediaList::Source& source = media_list_->GetSource(index);
+- (void)sourceNameChangedForList:(DesktopMediaList*)list atIndex:(int)index {
+  content::DesktopMediaID::Type sourceType = [self sourceTypeForList:list];
+  NSMutableArray* items = [self itemSetForType:sourceType];
+  id browser = [self browserViewForType:sourceType];
+  NSInteger selectedIndex = [self selectedIndexForType:sourceType];
+
+  DesktopMediaPickerItem* item = [items objectAtIndex:index];
+  const DesktopMediaList::Source& source = list->GetSource(index);
   [item setImageTitle:base::SysUTF16ToNSString(source.name)];
-  [sourceBrowser_ reloadData];
+  [browser reloadData];
+  if (sourceType == content::DesktopMediaID::TYPE_WEB_CONTENTS)
+    [self setTabBrowserIndex:selectedIndex];
 }
 
-- (void)sourceThumbnailChangedAtIndex:(int)index {
-  const DesktopMediaList::Source& source = media_list_->GetSource(index);
+- (void)sourceThumbnailChangedForList:(DesktopMediaList*)list
+                              atIndex:(int)index {
+  content::DesktopMediaID::Type sourceType = [self sourceTypeForList:list];
+  NSMutableArray* items = [self itemSetForType:sourceType];
+  id browser = [self browserViewForType:sourceType];
+  NSInteger selectedIndex = [self selectedIndexForType:sourceType];
+
+  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];
+
+  if (sourceType == content::DesktopMediaID::TYPE_WEB_CONTENTS)
+    [self setTabBrowserIndex:selectedIndex];
 }
 
 @end  // @interface DesktopMediaPickerController