Desktop Capture Picker New UI: Non Mac Appearance Change

chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc

Index: chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc
diff --git a/chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc b/chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc
index fbc8f56a2f4be05b77fa0aa65cc4bb578a370e33..fde9b91683eff3551cc0f5ab1d56e03b414fd6af 100644
--- a/chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc
+++ b/chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc
@@ -75,13 +75,36 @@ DesktopMediaPickerDialogView::DesktopMediaPickerDialogView(
   if (screen_list) {
     source_types_.push_back(DesktopMediaID::TYPE_SCREEN);
 
+    const DesktopMediaSourceViewStyle kSingleScreenStyle(

[msw, 2016/05/20 23:26:36]

Hard-coding all these values is a fairly strange approach, though I suppose the
mocks giving four different appearances for somewhat similar lists of items is
also quite odd. I wonder what this would look like if we had less shared code
for the lists and items, but perhaps that's not a good approach either... +Scott
for high-level impressions here. Mocks at:
https://docs.google.com/presentation/d/1xIgk_xNpa-4yBSR4DIcoDLfBFigu7SJwFWOdI...

[qiangchen, 2016/05/24 00:03:37]

My understanding of the different styles is:

For tab, it is infeasible to get a clear preview, and we only have an icon, so
we use the "list" style.
For window, we do not change anything.
For screen, in general, the user will only have very few number of screens, and
thus displaying it/them in larger size looks better.

However different those appearance looks, the underlying UI event handling
mechanism is identical, and thus I think it is a good approach to for different
styles to share code. Introducing the style struct parameter actually makes the
UI easier to adjust.

+        1,                                       // columns
+        gfx::Size(360, 280),                     // item_size
+        gfx::Rect(20, 220, 320, 40),             // label_position

[msw, 2016/05/20 23:26:37]

Does this work well enough for internationalization (tall fonts for other
languages, etc?)

[qiangchen, 2016/05/24 00:03:37]

It should work, as I use the same height, and not shrink the width.

+        gfx::HorizontalAlignment::ALIGN_CENTER,  // text_alignment
+        gfx::Rect(20, 20, 320, 200),             // image_position
+        2,                                       // selection_border_thickness
+        5,                                       // paint_inset_horizontal
+        5);                                      // paint_inset_vertical
+
+    const DesktopMediaSourceViewStyle kGenericScreenStyle(
+        2,                                       // columns
+        gfx::Size(270, 220),                     // item_size
+        gfx::Rect(15, 165, 240, 40),             // label_position
+        gfx::HorizontalAlignment::ALIGN_CENTER,  // text_alignment
+        gfx::Rect(15, 15, 240, 150),             // image_position
+        2,                                       // selection_border_thickness
+        5,                                       // paint_inset_horizontal
+        5);                                      // paint_inset_vertical
+
     views::ScrollView* screen_scroll_view =
         views::ScrollView::CreateScrollViewWithBorder();
-    list_views_.push_back(
-        new DesktopMediaListView(this, std::move(screen_list)));
+    list_views_.push_back(new DesktopMediaListView(
+        this, std::move(screen_list), kSingleScreenStyle, kGenericScreenStyle));
 
     screen_scroll_view->SetContents(list_views_.back());
-    screen_scroll_view->ClipHeightTo(kListItemHeight, kListItemHeight * 2);
+    screen_scroll_view->ClipHeightTo(
+        kGenericScreenStyle.item_size.height(),
+        kGenericScreenStyle.item_size.height() * 2);
+    screen_scroll_view->set_hide_horizontal_scrollbar(true);

[msw, 2016/05/20 23:26:36]

Does this mean that we're overflowing the available horizontal space? Shouldn't
we be able to avoid this by making sure the expected items per row fit in the
available width? (ditto in the two cases below)

[qiangchen, 2016/05/24 00:03:37]

Actually the sizes are guaranteed not to overflow. But without this clause, we
are going to see the horizontal scrollbar.

Found the reason, it seems DesktopMediaListView::GetPreferredSize()'s width does
not affect scroll_view's inner content size.

Then we need to call SetSize in DesktopMediaListView::Layout().

We still need to hide horizontal bar for tab list, as we want the hover shade to
be the full row.

[msw, 2016/05/24 18:54:25]

I mentioned that calling SetSize in Layout seems wrong in an earlier review:
https://codereview.chromium.org/1932413002/diff/120001/chrome/browser/ui/view...
Perhaps it's necessary, as you suggest, because of a bug in ScrollView? What
size does the view get if you skip calling SetSize in Layout? I'd rather have
this call |set_hide_horizontal_scrollbar| here than have Layout call SetSize.

[qiangchen, 2016/05/25 16:24:08]

Without set_hide_horizontal_scrollbar here and SetSize() in Layout(), we will
see the horizontal bar, although the movement range is very small. It is totally
unnecessary to have it, as we designed all the sizes and guaranteed no overflow.

I guess it might be a bug in ScrollView. As in my investigation, the height of
Preferred Size will determine whether we have the vertical bar or not, but the
width of Preferred Size does nothing.

     pane_->AddTab(
         l10n_util::GetStringUTF16(IDS_DESKTOP_MEDIA_PICKER_SOURCE_TYPE_SCREEN),
         screen_scroll_view);
@@ -90,13 +113,26 @@ DesktopMediaPickerDialogView::DesktopMediaPickerDialogView(
 
   if (window_list) {
     source_types_.push_back(DesktopMediaID::TYPE_WINDOW);
+
+    const DesktopMediaSourceViewStyle kWindowStyle(
+        3,                                       // columns
+        gfx::Size(180, 160),                     // item_size
+        gfx::Rect(10, 110, 160, 40),             // label_position
+        gfx::HorizontalAlignment::ALIGN_CENTER,  // text_alignment
+        gfx::Rect(8, 8, 164, 104),               // image_position
+        2,                                       // selection_border_thickness
+        5,                                       // paint_inset_horizontal
+        5);                                      // paint_inset_vertical
+
     views::ScrollView* window_scroll_view =
         views::ScrollView::CreateScrollViewWithBorder();
-    list_views_.push_back(
-        new DesktopMediaListView(this, std::move(window_list)));
+    list_views_.push_back(new DesktopMediaListView(this, std::move(window_list),
+                                                   kWindowStyle, kWindowStyle));
 
     window_scroll_view->SetContents(list_views_.back());
-    window_scroll_view->ClipHeightTo(kListItemHeight, kListItemHeight * 2);
+    window_scroll_view->ClipHeightTo(kWindowStyle.item_size.height(),
+                                     kWindowStyle.item_size.height() * 2);
+    window_scroll_view->set_hide_horizontal_scrollbar(true);
 
     pane_->AddTab(
         l10n_util::GetStringUTF16(IDS_DESKTOP_MEDIA_PICKER_SOURCE_TYPE_WINDOW),
@@ -106,12 +142,26 @@ DesktopMediaPickerDialogView::DesktopMediaPickerDialogView(
 
   if (tab_list) {
     source_types_.push_back(DesktopMediaID::TYPE_WEB_CONTENTS);
+
+    const DesktopMediaSourceViewStyle kTabStyle(
+        1,                                     // columns
+        gfx::Size(600, 30),                    // item_size
+        gfx::Rect(46, 0, 490, 30),             // label_position
+        gfx::HorizontalAlignment::ALIGN_LEFT,  // text_alignment
+        gfx::Rect(10, 2, 26, 26),              // image_position
+        1,                                     // selection_border_thickness
+        0,                                     // paint_inset_horizontal
+        0);                                    // paint_inset_vertical
+
     views::ScrollView* tab_scroll_view =
         views::ScrollView::CreateScrollViewWithBorder();
-    list_views_.push_back(new DesktopMediaListView(this, std::move(tab_list)));
+    list_views_.push_back(new DesktopMediaListView(this, std::move(tab_list),
+                                                   kTabStyle, kTabStyle));
 
     tab_scroll_view->SetContents(list_views_.back());
-    tab_scroll_view->ClipHeightTo(kListItemHeight, kListItemHeight * 2);
+    tab_scroll_view->ClipHeightTo(kTabStyle.item_size.height(),
+                                  kTabStyle.item_size.height() * 2);
+    tab_scroll_view->set_hide_horizontal_scrollbar(true);
 
     pane_->AddTab(
         l10n_util::GetStringUTF16(IDS_DESKTOP_MEDIA_PICKER_SOURCE_TYPE_TAB),