Desktop Capture Picker New UI: Non Mac Appearance Change

chrome/browser/ui/views/desktop_capture/desktop_media_source_view.h

Index: chrome/browser/ui/views/desktop_capture/desktop_media_source_view.h
diff --git a/chrome/browser/ui/views/desktop_capture/desktop_media_source_view.h b/chrome/browser/ui/views/desktop_capture/desktop_media_source_view.h
index 8c8867f84589f1356bdb0caddb9edd12de415b07..2bafc2db453e2f29dd6852a0745b7c3f950be8f2 100644
--- a/chrome/browser/ui/views/desktop_capture/desktop_media_source_view.h
+++ b/chrome/browser/ui/views/desktop_capture/desktop_media_source_view.h
@@ -6,6 +6,7 @@
 #define CHROME_BROWSER_UI_VIEWS_DESKTOP_CAPTURE_DESKTOP_MEDIA_SOURCE_VIEW_H_
 
 #include "content/public/browser/desktop_media_id.h"
+#include "ui/gfx/text_constants.h"
 #include "ui/views/view.h"
 
 namespace views {
@@ -15,14 +16,45 @@ class Label;
 
 class DesktopMediaListView;
 
+// Controls the appearance of DesktopMediaSourceView.
+struct DesktopMediaSourceViewStyle {
+  DesktopMediaSourceViewStyle();

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

Can we remove the default constructor?

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

Done.

+  DesktopMediaSourceViewStyle(const DesktopMediaSourceViewStyle& style);

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

Can we use the default copy ctor? "... = default;"

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

Done.

+  DesktopMediaSourceViewStyle(int columns,
+                              const gfx::Size& item_size,
+                              const gfx::Rect& label_position,
+                              gfx::HorizontalAlignment text_alignment,
+                              const gfx::Rect& image_position,
+                              int selection_border_thickness,
+                              int paint_inset_horizontal,
+                              int paint_inset_vertical);
+
+  int columns;
+
+  gfx::Size item_size;
+
+  gfx::Rect label_position;

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

nit: |label_rect| for position *and size* data passed via rect.

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

Done.

+  gfx::HorizontalAlignment text_alignment;
+
+  gfx::Rect image_position;

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

nit: |image_rect| for position *and size* data passed via rect.

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

Done.

+  int selection_border_thickness;

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

|selection_border_thickness| is 2 for everything except tabs, where it's 1. I
suggest reducing this code complexity by using a const, at the cost of not
matching mocks pixel-for-pixel in this case.

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

I think it is better to make it a variable. 
Now I've changed the thickness for single screen to 4.

As a too-thick border on a small image would look ugly,
and a too-thin border on a large image would look less noticable.

+
+  int paint_inset_horizontal;

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

Your horizontal and vertical values always match, and this value is actually
used to inset the source view's focus rectangle, as far as I can tell... Change
these two into values into a single int named |focus_rectangle_inset| or
similar. Does focusing a source view automatically cause it to be selected
within the list? If so, this could be called |selection_border_inset| or
similar, and we might want to consolidate the way we paint the selection border
in DesktopMediaSourceView::SetSelected and the focus rect in
DesktopMediaSourceView::OnPaint...

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

Done.

+  int paint_inset_vertical;
+};
+
 // View used for each item in DesktopMediaListView. Shows a single desktop media
 // source as a thumbnail with the title under it.
 class DesktopMediaSourceView : public views::View {
  public:
   DesktopMediaSourceView(DesktopMediaListView* parent,
-                         content::DesktopMediaID source_id);
+                         content::DesktopMediaID source_id,
+                         DesktopMediaSourceViewStyle style);
   ~DesktopMediaSourceView() override;
 
+  // Update style.

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

nit: Used to update the style when the number of available items changes.

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

Done.

+  void SetStyle(DesktopMediaSourceViewStyle style);

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

I hope we won't need this function! (if single==generic)

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

N/A as that is controlled on the list level.
Now the current code is that if single==generic, this function will not be
called.

+
   // Updates thumbnail and title from |source|.
   void SetName(const base::string16& name);
   void SetThumbnail(const gfx::ImageSkia& thumbnail);
@@ -42,6 +74,8 @@ class DesktopMediaSourceView : public views::View {
   void OnFocus() override;
   void OnBlur() override;
   bool OnMousePressed(const ui::MouseEvent& event) override;
+  void OnMouseEntered(const ui::MouseEvent& event) override;
+  void OnMouseExited(const ui::MouseEvent& event) override;
   void OnGestureEvent(ui::GestureEvent* event) override;
 
   static const char* kDesktopMediaSourceViewClassName;
@@ -52,13 +86,18 @@ class DesktopMediaSourceView : public views::View {
   // (if any).
   void SetSelected(bool selected);
 
+  // Updates hover state of the element, and the appearance.
+  void SetHovered(bool hovered);
+
   DesktopMediaListView* parent_;
   content::DesktopMediaID source_id_;
 
+  DesktopMediaSourceViewStyle style_;
   views::ImageView* image_view_;
   views::Label* label_;
 
   bool selected_;
+  bool hovered_;
 
   DISALLOW_COPY_AND_ASSIGN(DesktopMediaSourceView);
 };