Desktop Capture Picker New UI: Preliminary Refactor

Desktop Capture Picker New UI: Preliminary Refactor

This CL essentially changes the signature of DesktopMediaPicker::Show to take three media lists. The reason doing this is for new UI of the picker window.

BUG=602478
Committed: https://crrev.com/0e89b88cb035447d45781c15c85f140ae8b47518
Cr-Commit-Position: refs/heads/master@{#387706}

[qiangchen, 2016-04-11 22:57:28]

Description was changed from

==========
Desktop Capture Picker New UI: Preliminary Refactor

BUG=
==========

to

==========
Desktop Capture Picker New UI: Preliminary Refactor

This CL essentially changes the signature of DesktopMediaPicker::Show to accept
three media lists. The reason doing this is for new UI of the picker window.

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

[qiangchen, 2016-04-11 22:59:19]

Patchset #2 (id:20001) has been deleted

[qiangchen, 2016-04-11 23:34:48]

Description was changed from

==========
Desktop Capture Picker New UI: Preliminary Refactor

This CL essentially changes the signature of DesktopMediaPicker::Show to accept
three media lists. The reason doing this is for new UI of the picker window.

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

to

==========
Desktop Capture Picker New UI: Preliminary Refactor

This CL essentially changes the signature of DesktopMediaPicker::Show to take
three media lists. The reason doing this is for new UI of the picker window.

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

[qiangchen, 2016-04-12 17:48:47]

Patchset #1 (id:1) has been deleted

[qiangchen, 2016-04-12 19:42:51]

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

[qiangchen, 2016-04-12 19:42:51]

A preliminary change for the new picker window UI design.

For detail, you can refer to the bug, where a design doc is attached.

[msw, 2016-04-13 00:04:02]

lgtm with nits

https://codereview.chromium.org/1880693002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/desktop_capture/desktop_capture_apitest.cc
(right):

https://codereview.chromium.org/1880693002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/desktop_capture/desktop_capture_apitest.cc:110: if
(current_test_ >= tests_count_) {
nit: just return the empty vector, if this is just a test safeguard?

https://codereview.chromium.org/1880693002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/desktop_capture/desktop_capture_base.cc
(right):

https://codereview.chromium.org/1880693002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/desktop_capture/desktop_capture_base.cc:134:
media_lists = g_picker_factory->CreateModel(show_screens, show_windows,
nit: combine this assignment statement with the decl above.

https://codereview.chromium.org/1880693002/diff/40001/chrome/browser/media/de...
File chrome/browser/media/desktop_media_picker.h (right):

https://codereview.chromium.org/1880693002/diff/40001/chrome/browser/media/de...
chrome/browser/media/desktop_media_picker.h:36: // provided by |list| and calls
|done_callback| when user chooses one of the
nit: update comment (mentions |list|)

[qiangchen, 2016-04-14 17:08:40]

Fix the nit.

https://codereview.chromium.org/1880693002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/desktop_capture/desktop_capture_apitest.cc
(right):

https://codereview.chromium.org/1880693002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/desktop_capture/desktop_capture_apitest.cc:110: if
(current_test_ >= tests_count_) {
On 2016/04/13 00:04:02, msw wrote:
> nit: just return the empty vector, if this is just a test safeguard?

media_lists here must have three items in it, otherwise when the caller calls
media_lists[2], it will crash.

https://codereview.chromium.org/1880693002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/desktop_capture/desktop_capture_base.cc
(right):

https://codereview.chromium.org/1880693002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/desktop_capture/desktop_capture_base.cc:134:
media_lists = g_picker_factory->CreateModel(show_screens, show_windows,
On 2016/04/13 00:04:02, msw wrote:
> nit: combine this assignment statement with the decl above.

Done.

https://codereview.chromium.org/1880693002/diff/40001/chrome/browser/media/de...
File chrome/browser/media/desktop_media_picker.h (right):

https://codereview.chromium.org/1880693002/diff/40001/chrome/browser/media/de...
chrome/browser/media/desktop_media_picker.h:36: // provided by |list| and calls
|done_callback| when user chooses one of the
On 2016/04/13 00:04:02, msw wrote:
> nit: update comment (mentions |list|)

Done.

[Sergey Ulanov, 2016-04-14 22:39:52]

lgtm after my comments are addressed

https://codereview.chromium.org/1880693002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/desktop_media_picker_views.cc (right):

https://codereview.chromium.org/1880693002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/desktop_media_picker_views.cc:421: media_list =
std::move(media_lists[0]);
DCHECK that media_lists is not empty.

https://codereview.chromium.org/1880693002/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/api/desktop_capture/desktop_capture_apitest.cc
(right):

https://codereview.chromium.org/1880693002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/desktop_capture/desktop_capture_apitest.cc:103:
std::vector<std::unique_ptr<DesktopMediaList>> CreateModel(
This expected to return vector of size 3. It would be more appropriate to use
std::tuple<> or std::array<> for the result

https://codereview.chromium.org/1880693002/diff/60001/chrome/browser/media/de...
File chrome/browser/media/desktop_media_picker.h (right):

https://codereview.chromium.org/1880693002/diff/60001/chrome/browser/media/de...
chrome/browser/media/desktop_media_picker.h:47: bool request_audio,
nit, not for this cl: there 4 parameters can be bundled together in a struct.

https://codereview.chromium.org/1880693002/diff/60001/chrome/browser/ui/cocoa...
File
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller_unittest.mm
(right):

https://codereview.chromium.org/1880693002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller_unittest.mm:51:
tab_list_(nullptr) {}
Add initializers where these members are defined.

https://codereview.chromium.org/1880693002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller_unittest.mm:95:
bool callback_called_;
 = false;

https://codereview.chromium.org/1880693002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller_unittest.mm:97:
FakeDesktopMediaList* screen_list_;
 = nullptr;

[qiangchen, 2016-04-15 19:33:30]

fixed

https://codereview.chromium.org/1880693002/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/api/desktop_capture/desktop_capture_apitest.cc
(right):

https://codereview.chromium.org/1880693002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/desktop_capture/desktop_capture_apitest.cc:103:
std::vector<std::unique_ptr<DesktopMediaList>> CreateModel(
On 2016/04/14 22:39:52, Sergey Ulanov wrote:
> This expected to return vector of size 3. It would be more appropriate to use
> std::tuple<> or std::array<> for the result

Done.

https://codereview.chromium.org/1880693002/diff/60001/chrome/browser/media/de...
File chrome/browser/media/desktop_media_picker.h (right):

https://codereview.chromium.org/1880693002/diff/60001/chrome/browser/media/de...
chrome/browser/media/desktop_media_picker.h:47: bool request_audio,
On 2016/04/14 22:39:52, Sergey Ulanov wrote:
> nit, not for this cl: there 4 parameters can be bundled together in a struct.

Good suggestion.

https://codereview.chromium.org/1880693002/diff/60001/chrome/browser/ui/cocoa...
File
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller_unittest.mm
(right):

https://codereview.chromium.org/1880693002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller_unittest.mm:51:
tab_list_(nullptr) {}
On 2016/04/14 22:39:52, Sergey Ulanov wrote:
> Add initializers where these members are defined.

Done.

https://codereview.chromium.org/1880693002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller_unittest.mm:95:
bool callback_called_;
On 2016/04/14 22:39:52, Sergey Ulanov wrote:
>  = false;

Done.

https://codereview.chromium.org/1880693002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller_unittest.mm:97:
FakeDesktopMediaList* screen_list_;
On 2016/04/14 22:39:52, Sergey Ulanov wrote:
>  = nullptr;

Done.

[qiangchen, 2016-04-15 19:33:40]

The CQ bit was checked by qiangchen@chromium.org

[qiangchen, 2016-04-15 19:33:41]

The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org,
sergeyu@chromium.org
Link to the patchset: https://codereview.chromium.org/1880693002/#ps80001
(title: "Comment")

[commit-bot: I haz the power, 2016-04-15 19:34:13]

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

[Sergey Ulanov, 2016-04-15 19:49:24]

https://codereview.chromium.org/1880693002/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/api/desktop_capture/desktop_capture_apitest.cc
(right):

https://codereview.chromium.org/1880693002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/desktop_capture/desktop_capture_apitest.cc:113:
media_lists[2] = nullptr;
unique_ptr<> should be initialized to nullptr by default.

https://codereview.chromium.org/1880693002/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/api/desktop_capture/desktop_capture_base.h
(right):

https://codereview.chromium.org/1880693002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/desktop_capture/desktop_capture_base.h:30: virtual
std::array<std::unique_ptr<DesktopMediaList>, 3> CreateModel(
add a typedef for the result type?

[qiangchen, 2016-04-15 19:53:37]

The CQ bit was unchecked by qiangchen@chromium.org

[qiangchen, 2016-04-15 20:11:27]

Fixed

https://codereview.chromium.org/1880693002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/desktop_media_picker_views.cc (right):

https://codereview.chromium.org/1880693002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/desktop_media_picker_views.cc:421: media_list =
std::move(media_lists[0]);
On 2016/04/14 22:39:52, Sergey Ulanov wrote:
> DCHECK that media_lists is not empty.

Done.

https://codereview.chromium.org/1880693002/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/api/desktop_capture/desktop_capture_apitest.cc
(right):

https://codereview.chromium.org/1880693002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/desktop_capture/desktop_capture_apitest.cc:113:
media_lists[2] = nullptr;
On 2016/04/15 19:49:24, Sergey Ulanov wrote:
> unique_ptr<> should be initialized to nullptr by default.

Done.

https://codereview.chromium.org/1880693002/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/api/desktop_capture/desktop_capture_base.h
(right):

https://codereview.chromium.org/1880693002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/desktop_capture/desktop_capture_base.h:30: virtual
std::array<std::unique_ptr<DesktopMediaList>, 3> CreateModel(
On 2016/04/15 19:49:24, Sergey Ulanov wrote:
> add a typedef for the result type?

Done.

[qiangchen, 2016-04-15 20:55:40]

The CQ bit was checked by qiangchen@chromium.org

[qiangchen, 2016-04-15 20:55:40]

The patchset sent to the CQ was uploaded after l-g-t-m from
sergeyu@chromium.org, msw@chromium.org
Link to the patchset: https://codereview.chromium.org/1880693002/#ps100001
(title: "Nit")

[commit-bot: I haz the power, 2016-04-15 20:56:18]

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

[commit-bot: I haz the power, 2016-04-15 21:40:46]

Description was changed from

==========
Desktop Capture Picker New UI: Preliminary Refactor

This CL essentially changes the signature of DesktopMediaPicker::Show to take
three media lists. The reason doing this is for new UI of the picker window.

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

to

==========
Desktop Capture Picker New UI: Preliminary Refactor

This CL essentially changes the signature of DesktopMediaPicker::Show to take
three media lists. The reason doing this is for new UI of the picker window.

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

[commit-bot: I haz the power, 2016-04-15 21:40:48]

Committed patchset #4 (id:100001)

[commit-bot: I haz the power, 2016-04-15 21:42:35]

Description was changed from

==========
Desktop Capture Picker New UI: Preliminary Refactor

This CL essentially changes the signature of DesktopMediaPicker::Show to take
three media lists. The reason doing this is for new UI of the picker window.

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

to

==========
Desktop Capture Picker New UI: Preliminary Refactor

This CL essentially changes the signature of DesktopMediaPicker::Show to take
three media lists. The reason doing this is for new UI of the picker window.

BUG=602478

Committed: https://crrev.com/0e89b88cb035447d45781c15c85f140ae8b47518
Cr-Commit-Position: refs/heads/master@{#387706}
==========

[commit-bot: I haz the power, 2016-04-15 21:42:36]

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