Desktop Capture Picker New UI: Non Mac Appearance Change

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

Index: chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc
diff --git a/chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc b/chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc
index fe6f58e401224b8387566890b39c5867381e173c..c9e370930eb6f0cb40ad1ff0a2d67735539a06a3 100644
--- a/chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc
+++ b/chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc
@@ -23,11 +23,19 @@ const int kDesktopMediaSourceViewGroupId = 1;
 
 DesktopMediaListView::DesktopMediaListView(
     DesktopMediaPickerDialogView* parent,
-    std::unique_ptr<DesktopMediaList> media_list)
-    : parent_(parent), media_list_(std::move(media_list)), weak_factory_(this) {
+    std::unique_ptr<DesktopMediaList> media_list,
+    DesktopMediaSourceViewStyle single_style,
+    DesktopMediaSourceViewStyle generic_style)
+    : parent_(parent),
+      media_list_(std::move(media_list)),
+      single_style_(single_style),
+      generic_style_(generic_style),
+      weak_factory_(this) {
   media_list_->SetThumbnailSize(
-      gfx::Size(DesktopMediaPickerDialogView::kThumbnailWidth,
-                DesktopMediaPickerDialogView::kThumbnailHeight));
+      gfx::Size(single_style_.image_position.width() -

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

Shouldn't we use |generic_style_| if |media_list_| has multiple items?

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

In the constructor, we have no idea how many items "will be" in the media_list.
They are later inserted though calling OnSourceAdded, etc.

+                    2 * single_style_.selection_border_thickness,
+                single_style_.image_position.height() -
+                    2 * single_style_.selection_border_thickness));
   SetFocusBehavior(FocusBehavior::ALWAYS);
 }
 
@@ -59,42 +67,44 @@ DesktopMediaSourceView* DesktopMediaListView::GetSelection() {
 }
 
 gfx::Size DesktopMediaListView::GetPreferredSize() const {
-  int total_rows =
-      (child_count() + DesktopMediaPickerDialogView::kListColumns - 1) /
-      DesktopMediaPickerDialogView::kListColumns;
-  return gfx::Size(DesktopMediaPickerDialogView::kTotalListWidth,
-                   DesktopMediaPickerDialogView::kListItemHeight * total_rows);
+  const int child_num = child_count();

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

nit: just inline child_count() below and remove child_num.

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

N/A

+  const DesktopMediaSourceViewStyle* style =

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

optional nit: add a const ref convenience accessor for this: const
DesktopMediaSourceViewStyle& DesktopMediaListView::style() { return
child_count() > 1 ? generic_style_ : single_style_; }

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

Done.

I added a pointer field active_style_ and SwitchStyle function to set its value,
rather than the function you suggested. This way I can avoid calling
child_count() repeatedly, and no worry when calling in OnSourceRemoved and
OnSourceAdded, as in these two functions we should distinguish old child_count
with new child_count.

+      child_num > 1 ? &generic_style_ : &single_style_;
+
+  int total_rows = (child_num + style->columns - 1) / style->columns;
+  return gfx::Size(style->columns * style->item_size.width(),
+                   total_rows * style->item_size.height());
 }
 
 void DesktopMediaListView::Layout() {
   int x = 0;
   int y = 0;
 
+  const DesktopMediaSourceViewStyle* style =
+      child_count() > 1 ? &generic_style_ : &single_style_;
+
   for (int i = 0; i < child_count(); ++i) {
-    if (x + DesktopMediaPickerDialogView::kListItemWidth >
-        DesktopMediaPickerDialogView::kTotalListWidth) {
+    if (i > 0 && i % style->columns == 0) {
       x = 0;
-      y += DesktopMediaPickerDialogView::kListItemHeight;
+      y += style->item_size.height();
     }
 
     View* source_view = child_at(i);

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

optional nit: inline this and remove |source_view|

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

Done.

-    source_view->SetBounds(x, y, DesktopMediaPickerDialogView::kListItemWidth,
-                           DesktopMediaPickerDialogView::kListItemHeight);
+    source_view->SetBounds(x, y, style->item_size.width(),
+                           style->item_size.height());
 
-    x += DesktopMediaPickerDialogView::kListItemWidth;
+    x += style->item_size.width();
   }
-
-  y += DesktopMediaPickerDialogView::kListItemHeight;
 }
 
 bool DesktopMediaListView::OnKeyPressed(const ui::KeyEvent& event) {
   int position_increment = 0;
   switch (event.key_code()) {
     case ui::VKEY_UP:
-      position_increment = -DesktopMediaPickerDialogView::kListColumns;
+      position_increment = -generic_style_.columns;
       break;
     case ui::VKEY_DOWN:
-      position_increment = DesktopMediaPickerDialogView::kListColumns;
+      position_increment = generic_style_.columns;
       break;
     case ui::VKEY_LEFT:
       position_increment = -1;
@@ -137,21 +147,38 @@ bool DesktopMediaListView::OnKeyPressed(const ui::KeyEvent& event) {
 
 void DesktopMediaListView::OnSourceAdded(DesktopMediaList* list, int index) {
   const DesktopMediaList::Source& source = media_list_->GetSource(index);
+
+  DesktopMediaSourceViewStyle* style =
+      child_count() ? &generic_style_ : &single_style_;

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

Shouldn't this be child_count() > 1? Use the accessor.

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

This clause is executed before the source really added to UI, so
child_count()==0 will result in child_count()==1 afterward.

+
   DesktopMediaSourceView* source_view =
-      new DesktopMediaSourceView(this, source.id);
+      new DesktopMediaSourceView(this, source.id, *style);
+
   source_view->SetName(source.name);
   source_view->SetGroup(kDesktopMediaSourceViewGroupId);
   AddChildViewAt(source_view, index);
 
-  PreferredSizeChanged();
-
-  if (child_count() % DesktopMediaPickerDialogView::kListColumns == 1)
+  if ((child_count() - 1) % generic_style_.columns == 0)

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

optional nit: if (child_count() % generic_style_.columns == 1)

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

Nope. When style_.columns==1 that way will not work.

     parent_->OnMediaListRowsChanged();
 
+  // We need to switch other item's style

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

nit: "Apply multi-item styling when the second source is added."

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

N/A now

+  if (child_count() == 2) {
+    media_list_->SetThumbnailSize(
+        gfx::Size(generic_style_.image_position.width() -
+                      2 * generic_style_.selection_border_thickness,

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

Use |style| instead of |generic_style_| throughout. It'd be even nicer if you
could remove the style struct's |selection_border_thickness| member altogether
and use the source_view->border().GetInsets() as needed.

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

N/A

For using GetInsets: we can only set one size for the whole list, however the
border.GetInsets will only work for the selected source view. So it is easier to
just read the struct's parameter 

P.S. (The SetThumbnailSize actually sets the capture size, it is used to
optimize performance, e.g. no need to capture the full HD picture for just a
thumbnail preview.)

+                  generic_style_.image_position.height() -
+                      2 * generic_style_.selection_border_thickness));
+    for (int i = 0; i < child_count(); i++)

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

Since there are only two items, and the newly added one was just constructed
with the correct style, just do the following (like in OnSourceRemoved):
    static_cast<DesktopMediaSourceView*>(child_at(0))->SetStyle(generic_style_);

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

N/A now.

+      static_cast<DesktopMediaSourceView*>(child_at(i))
+          ->SetStyle(generic_style_);
+  }
+
   // Auto select the first screen.
   if (index == 0 && source.id.type == DesktopMediaID::TYPE_SCREEN)
     source_view->RequestFocus();
 
+  PreferredSizeChanged();

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

This is generally only needed when we add a row, but I guess the single->multi
style for screens also needs this (unless we use the same style for single/multi
item, as I suggest).

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

It is necessary regardless of the row change.
We need this to trigger Layout(), so that the newly added source view is
visible.

+
   std::string autoselect_source =
       base::CommandLine::ForCurrentProcess()->GetSwitchValueASCII(
           switches::kAutoSelectDesktopCaptureSource);
@@ -172,6 +199,7 @@ void DesktopMediaListView::OnSourceRemoved(DesktopMediaList* list, int index) {
   DCHECK(view);
   DCHECK_EQ(view->GetClassName(),
             DesktopMediaSourceView::kDesktopMediaSourceViewClassName);
+
   bool was_selected = view->is_selected();
   RemoveChildView(view);
   delete view;
@@ -179,10 +207,20 @@ void DesktopMediaListView::OnSourceRemoved(DesktopMediaList* list, int index) {
   if (was_selected)
     OnSelectionChanged();
 
-  PreferredSizeChanged();
-
-  if (child_count() % DesktopMediaPickerDialogView::kListColumns == 0)
+  if (child_count() % generic_style_.columns == 0)
     parent_->OnMediaListRowsChanged();
+
+  // Switch to single style.

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

nit: Apply single-item styling when the second source is removed.

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

Done.

+  if (child_count() == 1) {
+    static_cast<DesktopMediaSourceView*>(child_at(0))->SetStyle(single_style_);
+    media_list_->SetThumbnailSize(
+        gfx::Size(single_style_.image_position.width() -
+                      2 * single_style_.selection_border_thickness,
+                  single_style_.image_position.height() -
+                      2 * single_style_.selection_border_thickness));
+  }
+
+  PreferredSizeChanged();
 }
 
 void DesktopMediaListView::OnSourceMoved(DesktopMediaList* list,