Desktop Capture Picker New UI: Split File

Desktop Capture Picker New UI: Split File

This CL is the preliminary change for detailed appearance
change of desktop capture picker UI.

We separate DesktopMediaListView and DesktopMediaSourceView
into their own files, as we anticipated that after appearance
change, the code size of those two classes will be too large
to squeeze in DesktopMediaPickerDialogView's file.

BUG=602478
Committed: https://crrev.com/1152870b3290f647999811619b92b2f37c81af92
Cr-Commit-Position: refs/heads/master@{#394204}

[qiangchen, 2016-05-12 22:39:36]

Patchset #1 (id:1) has been deleted

[qiangchen, 2016-05-12 22:39:42]

Patchset #1 (id:20001) has been deleted

[qiangchen, 2016-05-13 20:35:58]

Patchset #2 (id:60001) has been deleted

[qiangchen, 2016-05-13 20:52:55]

Patchset #1 (id:40001) has been deleted

[qiangchen, 2016-05-13 21:09:24]

Description was changed from

==========
Desktop Capture Picker New UI: Non Mac Appearance Change

BUG=602478
==========

to

==========
Desktop Capture Picker New UI: Non Mac Appearance Change

This CL changes the detailed appearance of desktop capture 
picker UI.

We separate DesktopMediaListView and DesktopMediaSourceView
into their own files.

We created a struct to control the appearance of DesktopMediaListView
and DesktopMediaSourceView.

BUG=602478
==========

[qiangchen, 2016-05-13 21:09:24]

qiangchen@chromium.org changed reviewers:
+ msw@chromium.org

[qiangchen, 2016-05-13 22:42:23]

Patchset #2 (id:100001) has been deleted

[qiangchen, 2016-05-13 22:45:25]

Patchset #2 (id:120001) has been deleted

[qiangchen, 2016-05-13 22:58:28]

Patchset #2 (id:140001) has been deleted

[qiangchen, 2016-05-13 23:04:29]

Patchset #2 (id:160001) has been deleted

[qiangchen, 2016-05-13 23:08:34]

Patchset #1 (id:80001) has been deleted

[qiangchen, 2016-05-13 23:12:57]

Patchset #2 (id:200001) has been deleted

[qiangchen, 2016-05-13 23:13:03]

Patchset #2 (id:220001) has been deleted

[qiangchen, 2016-05-13 23:15:00]

Description was changed from

==========
Desktop Capture Picker New UI: Non Mac Appearance Change

This CL changes the detailed appearance of desktop capture 
picker UI.

We separate DesktopMediaListView and DesktopMediaSourceView
into their own files.

We created a struct to control the appearance of DesktopMediaListView
and DesktopMediaSourceView.

BUG=602478
==========

to

==========
Desktop Capture Picker New UI: Non Mac Appearance Change

This CL changes the detailed appearance of desktop capture 
picker UI.

We separate DesktopMediaListView and DesktopMediaSourceView
into their own files.

We created a struct to control the appearance of DesktopMediaListView, so that
we can apply different style
for different list view instances.
and DesktopMediaSourceView.

BUG=602478
==========

[qiangchen, 2016-05-13 23:15:13]

Description was changed from

==========
Desktop Capture Picker New UI: Non Mac Appearance Change

This CL changes the detailed appearance of desktop capture 
picker UI.

We separate DesktopMediaListView and DesktopMediaSourceView
into their own files.

We created a struct to control the appearance of DesktopMediaListView, so that
we can apply different style
for different list view instances.
and DesktopMediaSourceView.

BUG=602478
==========

to

==========
Desktop Capture Picker New UI: Non Mac Appearance Change

This CL changes the detailed appearance of desktop capture 
picker UI.

We separate DesktopMediaListView and DesktopMediaSourceView
into their own files.

We created a struct to control the appearance of
DesktopMediaListView, so that we can apply different style
for different list view instances.
and DesktopMediaSourceView.

BUG=602478
==========

[qiangchen, 2016-05-13 23:32:09]

The CL changes the appearance of the DesktopMediaSourceView.

I separate the big classes into their own files in patch 1. 

Patch 2 takes the actual change. 

For your convenience, you can focus on the diff between patch 1 and patch 2 to
see the essential change.

Thanks.

[msw, 2016-05-14 00:10:55]

Please do the file move in a separate (prerequisite or subsequent) CL.

[qiangchen, 2016-05-16 15:56:07]

Patchset #2 (id:240001) has been deleted

[qiangchen, 2016-05-16 15:58:42]

Description was changed from

==========
Desktop Capture Picker New UI: Non Mac Appearance Change

This CL changes the detailed appearance of desktop capture 
picker UI.

We separate DesktopMediaListView and DesktopMediaSourceView
into their own files.

We created a struct to control the appearance of
DesktopMediaListView, so that we can apply different style
for different list view instances.
and DesktopMediaSourceView.

BUG=602478
==========

to

==========
Desktop Capture Picker New UI: Split File

This CL is the preliminary change for detailed appearance
change of desktop capture picker UI.

We separate DesktopMediaListView and DesktopMediaSourceView
into their own files, as we anticipated that after appearance
change, the code size of those two classes will be too large
to squeeze in DesktopMediaPickerDialogView's file.

BUG=602478
==========

[qiangchen, 2016-05-16 15:59:30]

Here it is. I just deleted patch 2, and patch 1 is naturally a file splitting
CL.

[msw, 2016-05-16 17:49:47]

https://codereview.chromium.org/1978633002/diff/180001/chrome/browser/ui/view...
File chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc (right):

https://codereview.chromium.org/1978633002/diff/180001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc:19: const int
kThumbnailWidth = 160;
Keep any constants needed across multiple files in desktop_media_picker_views.h
as DesktopMediaPickerViews::kThumbnailWidth public static class constants (or
somewhere else), just define them once so they can be shared. (You can keep any
that are just needed in one file in that file)

https://codereview.chromium.org/1978633002/diff/180001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc:34: int
GetMediaListViewHeightForRows(size_t rows) {
Ditto; make this a static DesktopMediaPickerDialogView function or similar.

https://codereview.chromium.org/1978633002/diff/180001/chrome/browser/ui/view...
File chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.h
(right):

https://codereview.chromium.org/1978633002/diff/180001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.h:10:
#include "chrome/browser/ui/views/desktop_capture/desktop_media_list_view.h"
Just forward declare the classes here, don't include these files.

https://codereview.chromium.org/1978633002/diff/180001/chrome/browser/ui/view...
File chrome/browser/ui/views/desktop_capture/desktop_media_source_view.cc
(right):

https://codereview.chromium.org/1978633002/diff/180001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_source_view.cc:12:
#include "ui/views/border.h"
nit: Remove this

https://codereview.chromium.org/1978633002/diff/180001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_source_view.cc:20: const
int kThumbnailWidth = 160;
ditto

https://codereview.chromium.org/1978633002/diff/180001/chrome/browser/ui/view...
File chrome/browser/ui/views/desktop_capture/desktop_media_source_view.h
(right):

https://codereview.chromium.org/1978633002/diff/180001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_source_view.h:9: #include
"ui/gfx/text_constants.h"
nit: remove this

[qiangchen, 2016-05-16 21:39:37]

Fixed. Thanks.

https://codereview.chromium.org/1978633002/diff/180001/chrome/browser/ui/view...
File chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc (right):

https://codereview.chromium.org/1978633002/diff/180001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc:19: const int
kThumbnailWidth = 160;
On 2016/05/16 17:49:46, msw wrote:
> Keep any constants needed across multiple files in
desktop_media_picker_views.h
> as DesktopMediaPickerViews::kThumbnailWidth public static class constants (or
> somewhere else), just define them once so they can be shared. (You can keep
any
> that are just needed in one file in that file)

Done.

https://codereview.chromium.org/1978633002/diff/180001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc:34: int
GetMediaListViewHeightForRows(size_t rows) {
On 2016/05/16 17:49:46, msw wrote:
> Ditto; make this a static DesktopMediaPickerDialogView function or similar.

Just inline all calls to this function. As it will not be needed in next CL.

https://codereview.chromium.org/1978633002/diff/180001/chrome/browser/ui/view...
File chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.h
(right):

https://codereview.chromium.org/1978633002/diff/180001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.h:10:
#include "chrome/browser/ui/views/desktop_capture/desktop_media_list_view.h"
On 2016/05/16 17:49:47, msw wrote:
> Just forward declare the classes here, don't include these files.

Done.

https://codereview.chromium.org/1978633002/diff/180001/chrome/browser/ui/view...
File chrome/browser/ui/views/desktop_capture/desktop_media_source_view.cc
(right):

https://codereview.chromium.org/1978633002/diff/180001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_source_view.cc:12:
#include "ui/views/border.h"
On 2016/05/16 17:49:47, msw wrote:
> nit: Remove this

Done.

https://codereview.chromium.org/1978633002/diff/180001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_source_view.cc:20: const
int kThumbnailWidth = 160;
On 2016/05/16 17:49:47, msw wrote:
> ditto

Done.

https://codereview.chromium.org/1978633002/diff/180001/chrome/browser/ui/view...
File chrome/browser/ui/views/desktop_capture/desktop_media_source_view.h
(right):

https://codereview.chromium.org/1978633002/diff/180001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_source_view.h:9: #include
"ui/gfx/text_constants.h"
On 2016/05/16 17:49:47, msw wrote:
> nit: remove this

Done.

[msw, 2016-05-16 22:49:25]

https://codereview.chromium.org/1978633002/diff/260001/chrome/browser/ui/view...
File chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.h
(right):

https://codereview.chromium.org/1978633002/diff/260001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.h:71: static
constexpr int kThumbnailWidth = 160;
I don't see 'constexpr' used elsewhere like this in chromium code, and I'm not
sure that's what it's intended for, use 'const' instead.

https://codereview.chromium.org/1978633002/diff/260001/chrome/browser/ui/view...
File chrome/browser/ui/views/desktop_capture/desktop_media_source_view.cc
(right):

https://codereview.chromium.org/1978633002/diff/260001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_source_view.cc:75: //
return nullptr;
Remove this

[qiangchen, 2016-05-16 23:49:42]

Fixed.

https://codereview.chromium.org/1978633002/diff/260001/chrome/browser/ui/view...
File chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.h
(right):

https://codereview.chromium.org/1978633002/diff/260001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.h:71: static
constexpr int kThumbnailWidth = 160;
On 2016/05/16 22:49:25, msw wrote:
> I don't see 'constexpr' used elsewhere like this in chromium code, and I'm not
> sure that's what it's intended for, use 'const' instead.

const is actually a variable that only allows assignment once.
constexpr is a constant whose value is known at compiling time, and thus
compiler can do more optimization.

not a big difference here.

Changed to const for consistency with other code.

https://codereview.chromium.org/1978633002/diff/260001/chrome/browser/ui/view...
File chrome/browser/ui/views/desktop_capture/desktop_media_source_view.cc
(right):

https://codereview.chromium.org/1978633002/diff/260001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_source_view.cc:75: //
return nullptr;
On 2016/05/16 22:49:25, msw wrote:
> Remove this

Done.

Ah, this is my debug code. I am not sure why compiler does not allow me to
initialize a "const char*" in declaration.

[msw, 2016-05-17 17:36:06]

lgtm

[qiangchen, 2016-05-17 17:52:44]

The CQ bit was checked by qiangchen@chromium.org

[commit-bot: I haz the power, 2016-05-17 17:53:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1978633002/280001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1978633002/280001

[commit-bot: I haz the power, 2016-05-17 18:07:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-17 18:07:56]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[qiangchen, 2016-05-17 20:04:46]

The CQ bit was checked by qiangchen@chromium.org

[qiangchen, 2016-05-17 20:04:46]

The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org
Link to the patchset: https://codereview.chromium.org/1978633002/#ps300001
(title: "Unittest")

[commit-bot: I haz the power, 2016-05-17 20:06:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1978633002/300001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1978633002/300001

[commit-bot: I haz the power, 2016-05-17 20:15:03]

Description was changed from

==========
Desktop Capture Picker New UI: Split File

This CL is the preliminary change for detailed appearance
change of desktop capture picker UI.

We separate DesktopMediaListView and DesktopMediaSourceView
into their own files, as we anticipated that after appearance
change, the code size of those two classes will be too large
to squeeze in DesktopMediaPickerDialogView's file.

BUG=602478
==========

to

==========
Desktop Capture Picker New UI: Split File

This CL is the preliminary change for detailed appearance
change of desktop capture picker UI.

We separate DesktopMediaListView and DesktopMediaSourceView
into their own files, as we anticipated that after appearance
change, the code size of those two classes will be too large
to squeeze in DesktopMediaPickerDialogView's file.

BUG=602478
==========

[commit-bot: I haz the power, 2016-05-17 20:15:04]

Committed patchset #4 (id:300001)

[commit-bot: I haz the power, 2016-05-17 20:16:50]

Description was changed from

==========
Desktop Capture Picker New UI: Split File

This CL is the preliminary change for detailed appearance
change of desktop capture picker UI.

We separate DesktopMediaListView and DesktopMediaSourceView
into their own files, as we anticipated that after appearance
change, the code size of those two classes will be too large
to squeeze in DesktopMediaPickerDialogView's file.

BUG=602478
==========

to

==========
Desktop Capture Picker New UI: Split File

This CL is the preliminary change for detailed appearance
change of desktop capture picker UI.

We separate DesktopMediaListView and DesktopMediaSourceView
into their own files, as we anticipated that after appearance
change, the code size of those two classes will be too large
to squeeze in DesktopMediaPickerDialogView's file.

BUG=602478

Committed: https://crrev.com/1152870b3290f647999811619b92b2f37c81af92
Cr-Commit-Position: refs/heads/master@{#394204}
==========

[commit-bot: I haz the power, 2016-05-17 20:16:51]

Patchset 4 (id:??) landed as
https://crrev.com/1152870b3290f647999811619b92b2f37c81af92
Cr-Commit-Position: refs/heads/master@{#394204}