Desktop Capture Picker Window New UI For Mac

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

Index: chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller_unittest.mm
diff --git a/chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller_unittest.mm b/chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller_unittest.mm
index 663d5d9cf1b3efea550be3bc8e51a5b11a121fcc..68241bf8bbbcd70bc7ad297f1d4913e49fde5996 100644
--- a/chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller_unittest.mm
+++ b/chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller_unittest.mm
@@ -10,19 +10,44 @@
 #include "chrome/browser/media/desktop_media_list_observer.h"
 #include "chrome/browser/media/fake_desktop_media_list.h"
 #import "chrome/browser/ui/cocoa/cocoa_test_helper.h"
+#include "chrome/browser/ui/cocoa/media_picker/desktop_media_picker_item.h"

[tapted, 2016/06/29 12:46:20]

nit: import

[qiangchen, 2016/06/29 18:15:04]

Done.

 #include "content/public/test/test_browser_thread_bundle.h"
 #include "testing/gtest_mac.h"
 

[tapted, 2016/06/29 12:46:20]

a `using content::DesktopMediaID` here would probably be handy

[qiangchen, 2016/06/29 18:15:03]

Done.

+#define MAKE_WINDOW_ID(id) \
+  content::DesktopMediaID(content::DesktopMediaID::TYPE_WINDOW, id)

[tapted, 2016/06/29 12:46:20]

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

[qiangchen, 2016/06/29 18:15:04]

Done.

+#define MAKE_SCREEN_ID(id) \
+  content::DesktopMediaID(content::DesktopMediaID::TYPE_SCREEN, id)
+#define MAKE_TAB_ID(id) \
+  content::DesktopMediaID(content::DesktopMediaID::TYPE_WEB_CONTENTS, id)
+
 @interface DesktopMediaPickerController (ExposedForTesting)
-- (IKImageBrowserView*)sourceBrowser;
+- (IKImageBrowserView*)screenBrowser;
+- (IKImageBrowserView*)windowBrowser;
+- (NSTableView*)tabBrowser;
+- (NSSegmentedControl*)sourceTypeControl;
 - (NSButton*)shareButton;
 - (NSButton*)audioShareCheckbox;
-- (NSArray*)items;
+- (NSArray*)screenItems;
+- (NSArray*)windowItems;
+- (NSArray*)tabItems;
 @end
 
 @implementation DesktopMediaPickerController (ExposedForTesting)
-- (IKImageBrowserView*)sourceBrowser {
-  return sourceBrowser_;
+- (IKImageBrowserView*)screenBrowser {
+  return screenBrowser_;
+}
+
+- (IKImageBrowserView*)windowBrowser {
+  return windowBrowser_;
+}
+
+- (NSTableView*)tabBrowser {
+  return tabBrowser_;
+}
+
+- (NSSegmentedControl*)sourceTypeControl {
+  return sourceTypeControl_;
 }
 
 - (NSButton*)shareButton {
@@ -37,9 +62,18 @@
   return audioShareCheckbox_;
 }
 
-- (NSArray*)items {
-  return items_;
+- (NSArray*)screenItems {
+  return screenItems_;
+}
+
+- (NSArray*)windowItems {
+  return windowItems_;
+}
+
+- (NSArray*)tabItems {
+  return tabItems_;
 }
+
 @end
 
 class DesktopMediaPickerControllerTest : public CocoaTest {
@@ -80,6 +114,14 @@ class DesktopMediaPickerControllerTest : public CocoaTest {
     return callback_called_;
   }
 
+  void ChangeType(content::DesktopMediaID::Type sourceType) {
+    NSSegmentedControl* control = [controller_ sourceTypeControl];
+    [control selectSegmentWithTag:sourceType];
+    // [control selectSegmentWithTag] does not trigger handler, so we need to
+    // trigger it manually.
+    [controller_ performSelector:[control action] withObject:control];

[tapted, 2016/06/29 12:46:19]

nit: controller_ -> [control target]

[qiangchen, 2016/06/29 18:15:04]

Done.

+  }
+
  protected:
   void OnResult(content::DesktopMediaID source) {
     EXPECT_FALSE(callback_called_);
@@ -98,12 +140,13 @@ class DesktopMediaPickerControllerTest : public CocoaTest {
 
 TEST_F(DesktopMediaPickerControllerTest, ShowAndDismiss) {
   [controller_ showWindow:nil];
+  ChangeType(content::DesktopMediaID::TYPE_SCREEN);
 
-  window_list_->AddSource(0);
-  window_list_->AddSource(1);
-  window_list_->SetSourceThumbnail(1);
+  screen_list_->AddSourceByFullMediaID(MAKE_SCREEN_ID(0));
+  screen_list_->AddSourceByFullMediaID(MAKE_SCREEN_ID(1));
+  screen_list_->SetSourceThumbnail(1);
 
-  NSArray* items = [controller_ items];
+  NSArray* items = [controller_ screenItems];
   EXPECT_EQ(2U, [items count]);
   EXPECT_NSEQ(@"0", [[items objectAtIndex:0] imageTitle]);
   EXPECT_EQ(nil, [[items objectAtIndex:0] imageRepresentation]);
@@ -113,31 +156,32 @@ TEST_F(DesktopMediaPickerControllerTest, ShowAndDismiss) {
 
 TEST_F(DesktopMediaPickerControllerTest, ClickShare) {
   [controller_ showWindow:nil];
+  ChangeType(content::DesktopMediaID::TYPE_WEB_CONTENTS);
+  tab_list_->AddSourceByFullMediaID(MAKE_TAB_ID(0));
+  tab_list_->SetSourceThumbnail(0);
+  tab_list_->AddSourceByFullMediaID(MAKE_TAB_ID(1));
+  tab_list_->SetSourceThumbnail(1);
 
-  window_list_->AddSource(0);
-  window_list_->SetSourceThumbnail(0);
-  window_list_->AddSource(1);
-  window_list_->SetSourceThumbnail(1);
-
-  EXPECT_EQ(2U, [[controller_ items] count]);
+  EXPECT_EQ(2U, [[controller_ tabItems] count]);
   EXPECT_FALSE([[controller_ shareButton] isEnabled]);
 
   NSIndexSet* indexSet = [NSIndexSet indexSetWithIndex:1];
-  [[controller_ sourceBrowser] setSelectionIndexes:indexSet
-                              byExtendingSelection:NO];
+  [[controller_ tabBrowser] selectRowIndexes:indexSet byExtendingSelection:NO];
   EXPECT_TRUE([[controller_ shareButton] isEnabled]);
 
+  [[controller_ audioShareCheckbox] setState:NSOffState];

[tapted, 2016/06/29 12:46:19]

comment about this?

[qiangchen, 2016/06/29 18:15:04]

Done.

   [[controller_ shareButton] performClick:nil];
   EXPECT_TRUE(WaitForCallback());
-  EXPECT_EQ(window_list_->GetSource(1).id, source_reported_);
+  EXPECT_EQ(tab_list_->GetSource(1).id, source_reported_);
 }

[tapted, 2016/06/29 12:46:19]

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

[qiangchen, 2016/06/29 18:15:04]

Done.

 
 TEST_F(DesktopMediaPickerControllerTest, ClickCancel) {
   [controller_ showWindow:nil];
+  ChangeType(content::DesktopMediaID::TYPE_WINDOW);
 
-  window_list_->AddSource(0);
+  window_list_->AddSourceByFullMediaID(MAKE_WINDOW_ID(0));
   window_list_->SetSourceThumbnail(0);
-  window_list_->AddSource(1);
+  window_list_->AddSourceByFullMediaID(MAKE_WINDOW_ID(1));
   window_list_->SetSourceThumbnail(1);
 

[tapted, 2016/06/29 12:46:20]

Something should get selected here - it's not very interesting otherwise

[qiangchen, 2016/06/29 18:15:04]

Done.

   [[controller_ cancelButton] performClick:nil];
@@ -147,11 +191,12 @@ TEST_F(DesktopMediaPickerControllerTest, ClickCancel) {
 
 TEST_F(DesktopMediaPickerControllerTest, CloseWindow) {
   [controller_ showWindow:nil];
+  ChangeType(content::DesktopMediaID::TYPE_SCREEN);
 
-  window_list_->AddSource(0);
-  window_list_->SetSourceThumbnail(0);
-  window_list_->AddSource(1);
-  window_list_->SetSourceThumbnail(1);
+  screen_list_->AddSourceByFullMediaID(MAKE_SCREEN_ID(0));
+  screen_list_->SetSourceThumbnail(0);
+  screen_list_->AddSourceByFullMediaID(MAKE_SCREEN_ID(1));
+  screen_list_->SetSourceThumbnail(1);
 
   [controller_ close];
   EXPECT_TRUE(WaitForCallback());
@@ -160,29 +205,31 @@ TEST_F(DesktopMediaPickerControllerTest, CloseWindow) {
 
 TEST_F(DesktopMediaPickerControllerTest, UpdateThumbnail) {
   [controller_ showWindow:nil];
+  ChangeType(content::DesktopMediaID::TYPE_WEB_CONTENTS);

[tapted, 2016/06/29 12:46:19]

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

[qiangchen, 2016/06/29 18:15:04]

Just icon, but the way we process it is the same as thumbnail, and the icon can
change on the fly. 

 
-  window_list_->AddSource(0);
-  window_list_->SetSourceThumbnail(0);
-  window_list_->AddSource(1);
-  window_list_->SetSourceThumbnail(1);
+  tab_list_->AddSourceByFullMediaID(MAKE_TAB_ID(0));
+  tab_list_->SetSourceThumbnail(0);
+  tab_list_->AddSourceByFullMediaID(MAKE_TAB_ID(1));
+  tab_list_->SetSourceThumbnail(1);
 
-  NSArray* items = [controller_ items];
+  NSArray* items = [controller_ tabItems];
   EXPECT_EQ(2U, [items count]);
   NSUInteger version = [[items objectAtIndex:0] imageVersion];
 
-  window_list_->SetSourceThumbnail(0);
+  tab_list_->SetSourceThumbnail(0);
   EXPECT_NE(version, [[items objectAtIndex:0] imageVersion]);
 }
 
 TEST_F(DesktopMediaPickerControllerTest, UpdateName) {
   [controller_ showWindow:nil];
+  ChangeType(content::DesktopMediaID::TYPE_WINDOW);
 
-  window_list_->AddSource(0);
+  window_list_->AddSourceByFullMediaID(MAKE_WINDOW_ID(0));
   window_list_->SetSourceThumbnail(0);
-  window_list_->AddSource(1);
+  window_list_->AddSourceByFullMediaID(MAKE_WINDOW_ID(1));
   window_list_->SetSourceThumbnail(1);
 
-  NSArray* items = [controller_ items];
+  NSArray* items = [controller_ windowItems];
   EXPECT_EQ(2U, [items count]);
   NSUInteger version = [[items objectAtIndex:0] imageVersion];
 
@@ -192,30 +239,32 @@ TEST_F(DesktopMediaPickerControllerTest, UpdateName) {
 
 TEST_F(DesktopMediaPickerControllerTest, RemoveSource) {
   [controller_ showWindow:nil];
+  ChangeType(content::DesktopMediaID::TYPE_SCREEN);
 
-  window_list_->AddSource(0);
-  window_list_->AddSource(1);
-  window_list_->AddSource(2);
-  window_list_->SetSourceName(1, base::ASCIIToUTF16("foo"));
+  screen_list_->AddSourceByFullMediaID(MAKE_SCREEN_ID(0));
+  screen_list_->AddSourceByFullMediaID(MAKE_SCREEN_ID(1));
+  screen_list_->AddSourceByFullMediaID(MAKE_SCREEN_ID(2));
+  screen_list_->SetSourceName(1, base::ASCIIToUTF16("foo"));
 
-  NSArray* items = [controller_ items];
+  NSArray* items = [controller_ screenItems];
   EXPECT_EQ(3U, [items count]);
   EXPECT_NSEQ(@"foo", [[items objectAtIndex:1] imageTitle]);
 }
 
 TEST_F(DesktopMediaPickerControllerTest, MoveSource) {
   [controller_ showWindow:nil];
+  ChangeType(content::DesktopMediaID::TYPE_WEB_CONTENTS);
 
-  window_list_->AddSource(0);
-  window_list_->AddSource(1);
-  window_list_->SetSourceName(1, base::ASCIIToUTF16("foo"));
-  NSArray* items = [controller_ items];
+  tab_list_->AddSourceByFullMediaID(MAKE_TAB_ID(0));
+  tab_list_->AddSourceByFullMediaID(MAKE_TAB_ID(1));
+  tab_list_->SetSourceName(1, base::ASCIIToUTF16("foo"));
+  NSArray* items = [controller_ tabItems];
   EXPECT_NSEQ(@"foo", [[items objectAtIndex:1] imageTitle]);
 
-  window_list_->MoveSource(1, 0);
+  tab_list_->MoveSource(1, 0);
   EXPECT_NSEQ(@"foo", [[items objectAtIndex:0] imageTitle]);
 
-  window_list_->MoveSource(0, 1);
+  tab_list_->MoveSource(0, 1);
   EXPECT_NSEQ(@"foo", [[items objectAtIndex:1] imageTitle]);
 }

[tapted, 2016/06/29 12:46:19]

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

[qiangchen, 2016/06/29 18:15:04]

Done.

 
@@ -225,35 +274,78 @@ TEST_F(DesktopMediaPickerControllerTest, MoveSource) {
 TEST_F(DesktopMediaPickerControllerTest, AudioShareCheckboxState) {
   [controller_ showWindow:nil];
 
-  screen_list_->AddSourceByFullMediaID(
-      content::DesktopMediaID(content::DesktopMediaID::TYPE_SCREEN, 0));
-  window_list_->AddSourceByFullMediaID(
-      content::DesktopMediaID(content::DesktopMediaID::TYPE_WINDOW, 1));
-  tab_list_->AddSourceByFullMediaID(
-      content::DesktopMediaID(content::DesktopMediaID::TYPE_WEB_CONTENTS, 2));
+  screen_list_->AddSourceByFullMediaID(MAKE_SCREEN_ID(0));
+  window_list_->AddSourceByFullMediaID(MAKE_WINDOW_ID(1));
+  tab_list_->AddSourceByFullMediaID(MAKE_TAB_ID(2));
 
   NSButton* checkbox = [controller_ audioShareCheckbox];
-  EXPECT_EQ(NO, [checkbox isEnabled]);
-
-  NSIndexSet* index_set = [NSIndexSet indexSetWithIndex:0];
-  [checkbox setEnabled:YES];
-  [[controller_ sourceBrowser] setSelectionIndexes:index_set
-                              byExtendingSelection:NO];
-  EXPECT_EQ(NO, [checkbox isEnabled]);
-
-  index_set = [NSIndexSet indexSetWithIndex:1];
-  [checkbox setEnabled:YES];
-  [[controller_ sourceBrowser] setSelectionIndexes:index_set
-                              byExtendingSelection:NO];
-  EXPECT_EQ(NO, [checkbox isEnabled]);
-
-  index_set = [NSIndexSet indexSetWithIndex:2];
-  [[controller_ sourceBrowser] setSelectionIndexes:index_set
-                              byExtendingSelection:NO];
-  EXPECT_EQ(YES, [checkbox isEnabled]);
-
-  index_set = [NSIndexSet indexSet];
-  [[controller_ sourceBrowser] setSelectionIndexes:index_set
-                              byExtendingSelection:NO];
-  EXPECT_EQ(NO, [checkbox isEnabled]);
+  EXPECT_EQ(YES, [checkbox isHidden]);
+
+  [checkbox setHidden:NO];
+  ChangeType(content::DesktopMediaID::TYPE_WINDOW);
+  EXPECT_EQ(YES, [checkbox isHidden]);
+
+  [checkbox setHidden:YES];
+  ChangeType(content::DesktopMediaID::TYPE_WEB_CONTENTS);
+  EXPECT_EQ(NO, [checkbox isHidden]);
+
+  [checkbox setHidden:NO];
+  ChangeType(content::DesktopMediaID::TYPE_SCREEN);
+  EXPECT_EQ(YES, [checkbox isHidden]);
+}
+
+TEST_F(DesktopMediaPickerControllerTest, TabShareWithAudio) {
+  [controller_ showWindow:nil];
+  ChangeType(content::DesktopMediaID::TYPE_WEB_CONTENTS);
+
+  content::DesktopMediaID origin_id = MAKE_TAB_ID(123);
+  content::DesktopMediaID id_with_audio = origin_id;
+  id_with_audio.audio_share = true;

[tapted, 2016/06/29 12:46:19]

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

[qiangchen, 2016/06/29 18:15:04]

No. The ID is passed by value. So the local variable will not be changed.

[tapted, 2016/06/30 00:37:01]

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.

+
+  tab_list_->AddSourceByFullMediaID(origin_id);
+
+  NSIndexSet* indexSet = [NSIndexSet indexSetWithIndex:0];

[tapted, 2016/06/29 12:46:20]

indexSet -> index_set (this doesn't lie between @implementation/@end, so ->
hacker_style for new declarations)

[qiangchen, 2016/06/29 18:15:04]

Done.

+  [[controller_ tabBrowser] selectRowIndexes:indexSet byExtendingSelection:NO];
+  EXPECT_TRUE([[controller_ shareButton] isEnabled]);
+
+  [[controller_ shareButton] performClick:nil];
+
+  EXPECT_TRUE(WaitForCallback());
+  EXPECT_EQ(id_with_audio, source_reported_);
+}
+
+TEST_F(DesktopMediaPickerControllerTest, TabBrowserFocusAlgorithm) {
+  [controller_ showWindow:nil];
+  ChangeType(content::DesktopMediaID::TYPE_WEB_CONTENTS);
+  tab_list_->AddSourceByFullMediaID(MAKE_TAB_ID(0));
+  tab_list_->AddSourceByFullMediaID(MAKE_TAB_ID(1));
+  tab_list_->AddSourceByFullMediaID(MAKE_TAB_ID(2));
+  tab_list_->AddSourceByFullMediaID(MAKE_TAB_ID(3));
+
+  NSArray* items = [controller_ tabItems];
+  NSTableView* browser = [controller_ tabBrowser];
+
+  NSIndexSet* indexSet = [NSIndexSet indexSetWithIndex:1];

[tapted, 2016/06/29 12:46:19]

index_set

[qiangchen, 2016/06/29 18:15:04]

Done.

+  [browser selectRowIndexes:indexSet byExtendingSelection:NO];
+
+  // Move source [0, 1, 2, 3]-->[1, 2, 3, 0]
+  tab_list_->MoveSource(0, 3);
+  NSUInteger selectedIndex = [[browser selectedRowIndexes] firstIndex];

[tapted, 2016/06/29 12:46:19]

selected_index

[qiangchen, 2016/06/29 18:15:04]

Done.

+  EXPECT_EQ(1, [[items objectAtIndex:selectedIndex] sourceID].id);
+
+  // Move source [1, 2, 3, 0]-->[3, 1, 2, 0]
+  tab_list_->MoveSource(2, 0);
+  selectedIndex = [[browser selectedRowIndexes] firstIndex];
+  EXPECT_EQ(1, [[items objectAtIndex:selectedIndex] sourceID].id);
+
+  // Remove a source [3, 1, 2, 0]-->[1, 2, 0]
+  tab_list_->RemoveSource(0);
+  selectedIndex = [[browser selectedRowIndexes] firstIndex];
+  EXPECT_EQ(1, [[items objectAtIndex:selectedIndex] sourceID].id);
+
+  // Change source type back and forth, browser should memorize the selection.
+  ChangeType(content::DesktopMediaID::TYPE_SCREEN);
+  ChangeType(content::DesktopMediaID::TYPE_WEB_CONTENTS);
+  selectedIndex = [[browser selectedRowIndexes] firstIndex];
+  EXPECT_EQ(1, [[items objectAtIndex:selectedIndex] sourceID].id);
 }