Add CombinedDesktopMediaList class

This CL adds CombinedDesktopMediaList class and its associated unit tests. It intends to support chrome tab capture for JavaScript API chooseDesktopMedia().

Design document is in
https://docs.google.com/document/d/1zZOiayDzVkuIQ3twEdiE6S-LTXz9fWOGKjbMIsLTcAk/edit#heading=h.cetuprbl19fv

BUG=557222
Committed: https://crrev.com/2716e8e3bf7a9f216de1ece328037fef76e64a27
Cr-Commit-Position: refs/heads/master@{#372228}

[GeorgeZ, 2016-01-22 21:21:55]

Description was changed from

==========
little


little


little


extension


add 6 new files


observer interface change


observer interface change

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

to

==========
This CL adds TabDesktopMediaList and CombinedDesktopMediaList classes and
associated unit tests. It intends to support chrome tab capture for JavaScript
API chooseDesktopMedia().


Design document is in
https://docs.google.com/document/d/1zZOiayDzVkuIQ3twEdiE6S-LTXz9fWOGKjbMIsLTc...

BUG=557222
==========

[GeorgeZ, 2016-01-25 21:05:35]

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

[GeorgeZ, 2016-01-25 21:05:36]

Sergeyu and Qiang,

Would you please have an review of this CL.

Thanks,

George

[qiangchen, 2016-01-26 01:01:50]

Some suggestions.

Haven't looked at unittests. Will do that later.

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

https://codereview.chromium.org/1622733002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/desktop_capture/desktop_capture_base.cc:102:
break;
No need to break after return;

https://codereview.chromium.org/1622733002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/desktop_capture/desktop_capture_base.cc:129: if
(!screen_window_list) {
Not your fault.

But I think it would be better to change the whole block as
#if defined...

#else

#endif

rather than doing a runtime check.

https://codereview.chromium.org/1622733002/diff/40001/chrome/browser/media/ta...
File chrome/browser/media/tab_desktop_media_list.cc (right):

https://codereview.chromium.org/1622733002/diff/40001/chrome/browser/media/ta...
chrome/browser/media/tab_desktop_media_list.cc:128:
tab_map.insert(std::make_pair(t, SourceDescription(media_id, title)));
simpler to use [] operator?

https://codereview.chromium.org/1622733002/diff/40001/chrome/browser/media/ta...
chrome/browser/media/tab_desktop_media_list.cc:139: favicon_map.insert(
simpler to use []?
favicon_map[media_id] = favicon.

https://codereview.chromium.org/1622733002/diff/40001/chrome/browser/media/ta...
File chrome/browser/media/tab_desktop_media_list.h (right):

https://codereview.chromium.org/1622733002/diff/40001/chrome/browser/media/ta...
chrome/browser/media/tab_desktop_media_list.h:23: void
SetFakeSourcesForTesting(std::vector<SourceDescription>& sources,
Put ForTesting function in private and add the class that needs to call this
function as TabDesktopMediaList's friend.

https://codereview.chromium.org/1622733002/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/media_picker/desktop_media_picker_bridge.mm
(right):

https://codereview.chromium.org/1622733002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_bridge.mm:15: void
DesktopMediaPickerBridge::OnSourceAdded(DesktopMediaList* list,
According to style guide.
Comment the unused parameter variable name. For example:

DesktopMediaList* /*list*/

Do the same for the other functions in this file.

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

https://codereview.chromium.org/1622733002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/desktop_media_picker_views.cc:314: void
DesktopMediaListView::OnSourceAdded(DesktopMediaList* list, int index) {
Comment the unused parameter variable.

Ditto for the following of this file.

https://codereview.chromium.org/1622733002/diff/40001/content/public/browser/...
File content/public/browser/desktop_media_id.cc (right):

https://codereview.chromium.org/1622733002/diff/40001/content/public/browser/...
content/public/browser/desktop_media_id.cc:105: std::tie(other.type, other.id,
other.web_contents_id);
Nice catch.

[Sergey Ulanov, 2016-01-26 03:32:34]

Can this be split into two CL's? I don't think there is any reason these two
classes need to be added in a single CL.

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

https://codereview.chromium.org/1622733002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/desktop_media_picker_views.cc:314: void
DesktopMediaListView::OnSourceAdded(DesktopMediaList* list, int index) {
On 2016/01/26 01:01:50, qiangchenC wrote:
> Comment the unused parameter variable.
Why? I don't think we comment about unused parameters anywhere else, and I don't
see how that would be useful?

> 
> Ditto for the following of this file.

[qiangchen, 2016-01-26 17:42:04]

replied on the commenting of unused parameter variable.

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

https://codereview.chromium.org/1622733002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/desktop_media_picker_views.cc:314: void
DesktopMediaListView::OnSourceAdded(DesktopMediaList* list, int index) {
On 2016/01/26 03:32:33, Sergey Ulanov wrote:
> On 2016/01/26 01:01:50, qiangchenC wrote:
> > Comment the unused parameter variable.
> Why? I don't think we comment about unused parameters anywhere else, and I
don't
> see how that would be useful?
> 
> > 
> > Ditto for the following of this file.

Style guide https://google.github.io/styleguide/cppguide.html.

Search "unused", you will find the section stating this.

[Sergey Ulanov, 2016-01-26 17:55:10]

On 2016/01/26 17:42:04, qiangchenC wrote:
> replied on the commenting of unused parameter variable.
> 
>
https://codereview.chromium.org/1622733002/diff/40001/chrome/browser/ui/views...
> File chrome/browser/ui/views/desktop_media_picker_views.cc (right):
> 
>
https://codereview.chromium.org/1622733002/diff/40001/chrome/browser/ui/views...
> chrome/browser/ui/views/desktop_media_picker_views.cc:314: void
> DesktopMediaListView::OnSourceAdded(DesktopMediaList* list, int index) {
> On 2016/01/26 03:32:33, Sergey Ulanov wrote:
> > On 2016/01/26 01:01:50, qiangchenC wrote:
> > > Comment the unused parameter variable.
> > Why? I don't think we comment about unused parameters anywhere else, and I
> don't
> > see how that would be useful?
> > 
> > > 
> > > Ditto for the following of this file.
> 
> Style guide https://google.github.io/styleguide/cppguide.html.
> 
> Search "unused", you will find the section stating this.

If I understand the style guide correctly it says that:
 - the name may be omitted if it's not used and the meaning is obvious.
 - the name must not be omitted if it's not obvious.
 - the name may be commented if it's not used. I think this provision is
necessary because some compiler would warn about unused parameters otherwise.
This doesn't apply in chromium.

I don't see it saying anywhere that name of unused parameters must be commented
and I've never seen unused parameters commented in chromium.

[Sergey Ulanov, 2016-01-26 18:06:54]

https://codereview.chromium.org/1622733002/diff/40001/chrome/browser/media/co...
File chrome/browser/media/combined_desktop_media_list.cc (right):

https://codereview.chromium.org/1622733002/diff/40001/chrome/browser/media/co...
chrome/browser/media/combined_desktop_media_list.cc:8:
std::vector<scoped_ptr<DesktopMediaList>>& media_lists) {
reference parameters can only be const. In this case you want to pass ownership
of the lists, so this shouldn't be a reference.

https://codereview.chromium.org/1622733002/diff/40001/chrome/browser/media/co...
chrome/browser/media/combined_desktop_media_list.cc:11:
media_lists_.push_back(std::move(media_lists[i]));
Don't need the loop, just move the whole list:
 media_lists_ = std::move(media_lists)

https://codereview.chromium.org/1622733002/diff/40001/chrome/browser/media/co...
chrome/browser/media/combined_desktop_media_list.cc:23: for (size_t i = 0; i <
media_lists_.size(); i++)
Here and everywhere in this file use range-based for loop:
http://en.cppreference.com/w/cpp/language/range-for

https://codereview.chromium.org/1622733002/diff/40001/chrome/browser/media/co...
chrome/browser/media/combined_desktop_media_list.cc:58: DCHECK(0) << "Index is
invalid.";
NOTREACHED()

https://codereview.chromium.org/1622733002/diff/40001/chrome/browser/media/co...
chrome/browser/media/combined_desktop_media_list.cc:72: }
Add NOTREACHED(). Same for methods below.

https://codereview.chromium.org/1622733002/diff/40001/chrome/browser/media/co...
File chrome/browser/media/combined_desktop_media_list.h (right):

https://codereview.chromium.org/1622733002/diff/40001/chrome/browser/media/co...
chrome/browser/media/combined_desktop_media_list.h:14: // Combination of
different types of DesktopMediaLists.
nit: they don't have to be two different types, just some lists.
The comment should also explain how they are being combined into one, i.e. that
the lists are concatenated.

https://codereview.chromium.org/1622733002/diff/40001/chrome/browser/media/ta...
File chrome/browser/media/tab_desktop_media_list.cc (right):

https://codereview.chromium.org/1622733002/diff/40001/chrome/browser/media/ta...
chrome/browser/media/tab_desktop_media_list.cc:30: uint32_t value =
base::SuperFastHash(
Use base::Hash() instead of SuperFastHash().
Super-fast hash is a crappy hash function (it's not that fast and has many
collisions, see
http://programmers.stackexchange.com/questions/49550/which-hashing-algorithm-...)
and there is no reason to hard-code dependency on it here.

https://codereview.chromium.org/1622733002/diff/40001/chrome/browser/media/ta...
chrome/browser/media/tab_desktop_media_list.cc:39: return gfx::ImageSkia();
Why return an empty image here? Can this be a DCHECK? I don't think we expect
thumbnail size to be smaller than 20x20

https://codereview.chromium.org/1622733002/diff/40001/chrome/browser/media/ta...
chrome/browser/media/tab_desktop_media_list.cc:47: canvas.drawARGB(255, 255,
255, 255);
Why is it white instead of transparent? This may not look right when the
background in the picker dialog isn't white

https://codereview.chromium.org/1622733002/diff/40001/chrome/browser/media/ta...
chrome/browser/media/tab_desktop_media_list.cc:63: const double scale = fmin(3.0
/ 4 * result.width() / favicon.Width(),
std::min() instead of fmin()

https://codereview.chromium.org/1622733002/diff/40001/chrome/browser/media/ta...
chrome/browser/media/tab_desktop_media_list.cc:66: dest_rect.set(result.width()
/ 2 - favicon.Width() * scale / 2,     // left
Combine this with the previous line:

SkRect dest_rect = SkRect::MakeLTRB(....);

https://codereview.chromium.org/1622733002/diff/40001/chrome/browser/media/ta...
chrome/browser/media/tab_desktop_media_list.cc:70: const scoped_ptr<SkImage>
temp_image(
call it 'bitmap'? "temp_" doesn't make sense.

https://codereview.chromium.org/1622733002/diff/40001/chrome/browser/media/ta...
chrome/browser/media/tab_desktop_media_list.cc:153: // For unit tests.
This doesn't look like the right way to unittest this code. The unittests
execute almost no code that actually needs to be tested and the production code
is polluted with test-specific logic. I think it should be possible to create
fake profile with fake tabs in the unittests and then you wouldn't need
SetFakeSourcesForTesting()

[qiangchen, 2016-01-26 20:43:10]

Some suggestions on unit test.

In particular use of VerifyAndClearExpectations is tricky.

https://codereview.chromium.org/1622733002/diff/40001/chrome/browser/media/co...
File chrome/browser/media/combined_desktop_media_list_unittest.cc (right):

https://codereview.chromium.org/1622733002/diff/40001/chrome/browser/media/co...
chrome/browser/media/combined_desktop_media_list_unittest.cc:21: static const
int kDefaultSouceCount = 2;  // >=2.
kDefaultSourceCount

https://codereview.chromium.org/1622733002/diff/40001/chrome/browser/media/co...
chrome/browser/media/combined_desktop_media_list_unittest.cc:148: void Verify()
{
This function is more like initialize than verify.

Or can we merge it into the constructor?

https://codereview.chromium.org/1622733002/diff/40001/chrome/browser/media/co...
chrome/browser/media/combined_desktop_media_list_unittest.cc:197:
testing::Mock::VerifyAndClearExpectations(&observer_);
This is unsafe, it is better to let observer_ to have a public function called 
VerifyAndClearExpectations(). Implemented in this way

{
    testing::Mock::VerifyAndClearExpectations(this);
}

And we call observer_.VerifyAndClearExpectations() here.

Reason is that raw value of &observer_ could be different from this.

https://codereview.chromium.org/1622733002/diff/40001/chrome/browser/media/ta...
File chrome/browser/media/tab_desktop_media_list_unittest.cc (right):

https://codereview.chromium.org/1622733002/diff/40001/chrome/browser/media/ta...
chrome/browser/media/tab_desktop_media_list_unittest.cc:22: static const int
kDefaultSouceCount = 2;  // >=2.
kDefaultSourceCount

https://codereview.chromium.org/1622733002/diff/40001/chrome/browser/media/ta...
chrome/browser/media/tab_desktop_media_list_unittest.cc:136:
testing::Mock::VerifyAndClearExpectations(&observer_);
Again, make this to a function of observer_.

[GeorgeZ, 2016-01-26 22:34:33]

Description was changed from

==========
This CL adds TabDesktopMediaList and CombinedDesktopMediaList classes and
associated unit tests. It intends to support chrome tab capture for JavaScript
API chooseDesktopMedia().


Design document is in
https://docs.google.com/document/d/1zZOiayDzVkuIQ3twEdiE6S-LTXz9fWOGKjbMIsLTc...

BUG=557222
==========

to

==========
This CL adds CombinedDesktopMediaList class and its associated unit tests. It
intends to support chrome tab capture for JavaScript API chooseDesktopMedia().

Design document is in
https://docs.google.com/document/d/1zZOiayDzVkuIQ3twEdiE6S-LTXz9fWOGKjbMIsLTc...

BUG=557222
==========

[GeorgeZ, 2016-01-27 00:17:39]

Sergey and Qiang,

A patch was uploaded to address your comments. Tab related code had been removed
from this CL. The next CL will add them back.

Thanks,

George

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

https://codereview.chromium.org/1622733002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/desktop_capture/desktop_capture_base.cc:102:
break;
On 2016/01/26 01:01:50, qiangchenC wrote:
> No need to break after return;

Done.

https://codereview.chromium.org/1622733002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/desktop_capture/desktop_capture_base.cc:129: if
(!screen_window_list) {
On 2016/01/26 01:01:50, qiangchenC wrote:
> Not your fault.
> 
> But I think it would be better to change the whole block as
> #if defined...
> 
> #else
> 
> #endif
> 
> rather than doing a runtime check.

I am not sure whether USE_ASH case will never use NativeDesktopMediaList() since
I see the similar code in
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re....

https://codereview.chromium.org/1622733002/diff/40001/chrome/browser/media/co...
File chrome/browser/media/combined_desktop_media_list.cc (right):

https://codereview.chromium.org/1622733002/diff/40001/chrome/browser/media/co...
chrome/browser/media/combined_desktop_media_list.cc:8:
std::vector<scoped_ptr<DesktopMediaList>>& media_lists) {
On 2016/01/26 18:06:54, Sergey Ulanov wrote:
> reference parameters can only be const. In this case you want to pass
ownership
> of the lists, so this shouldn't be a reference.

The reference is to avoid the copy of media_lists which will lead to compile
error, since it tries to copy scoped_ptrs. Const will not work here, since
media_list will lose its scoped_ptrs.

Please provide further instruction for this case.

https://codereview.chromium.org/1622733002/diff/40001/chrome/browser/media/co...
chrome/browser/media/combined_desktop_media_list.cc:11:
media_lists_.push_back(std::move(media_lists[i]));
On 2016/01/26 18:06:54, Sergey Ulanov wrote:
> Don't need the loop, just move the whole list:
>  media_lists_ = std::move(media_lists)

Done.

https://codereview.chromium.org/1622733002/diff/40001/chrome/browser/media/co...
chrome/browser/media/combined_desktop_media_list.cc:23: for (size_t i = 0; i <
media_lists_.size(); i++)
On 2016/01/26 18:06:54, Sergey Ulanov wrote:
> Here and everywhere in this file use range-based for loop:
> http://en.cppreference.com/w/cpp/language/range-for

Done.

https://codereview.chromium.org/1622733002/diff/40001/chrome/browser/media/co...
chrome/browser/media/combined_desktop_media_list.cc:58: DCHECK(0) << "Index is
invalid.";
On 2016/01/26 18:06:54, Sergey Ulanov wrote:
> NOTREACHED()

Done.

https://codereview.chromium.org/1622733002/diff/40001/chrome/browser/media/co...
chrome/browser/media/combined_desktop_media_list.cc:72: }
On 2016/01/26 18:06:54, Sergey Ulanov wrote:
> Add NOTREACHED(). Same for methods below.

Done.

https://codereview.chromium.org/1622733002/diff/40001/chrome/browser/media/co...
File chrome/browser/media/combined_desktop_media_list.h (right):

https://codereview.chromium.org/1622733002/diff/40001/chrome/browser/media/co...
chrome/browser/media/combined_desktop_media_list.h:14: // Combination of
different types of DesktopMediaLists.
On 2016/01/26 18:06:54, Sergey Ulanov wrote:
> nit: they don't have to be two different types, just some lists.
> The comment should also explain how they are being combined into one, i.e.
that
> the lists are concatenated.

Done.

https://codereview.chromium.org/1622733002/diff/40001/chrome/browser/media/co...
File chrome/browser/media/combined_desktop_media_list_unittest.cc (right):

https://codereview.chromium.org/1622733002/diff/40001/chrome/browser/media/co...
chrome/browser/media/combined_desktop_media_list_unittest.cc:21: static const
int kDefaultSouceCount = 2;  // >=2.
On 2016/01/26 20:43:09, qiangchenC wrote:
> kDefaultSourceCount

Done.

https://codereview.chromium.org/1622733002/diff/40001/chrome/browser/media/co...
chrome/browser/media/combined_desktop_media_list_unittest.cc:148: void Verify()
{
On 2016/01/26 20:43:09, qiangchenC wrote:
> This function is more like initialize than verify.
> 
> Or can we merge it into the constructor?

Done.

https://codereview.chromium.org/1622733002/diff/40001/chrome/browser/media/co...
chrome/browser/media/combined_desktop_media_list_unittest.cc:197:
testing::Mock::VerifyAndClearExpectations(&observer_);
On 2016/01/26 20:43:09, qiangchenC wrote:
> This is unsafe, it is better to let observer_ to have a public function called

> VerifyAndClearExpectations(). Implemented in this way
> 
> {
>     testing::Mock::VerifyAndClearExpectations(this);
> }
> 
> And we call observer_.VerifyAndClearExpectations() here.
> 
> Reason is that raw value of &observer_ could be different from this.

Done.

https://codereview.chromium.org/1622733002/diff/40001/chrome/browser/media/ta...
File chrome/browser/media/tab_desktop_media_list.cc (right):

https://codereview.chromium.org/1622733002/diff/40001/chrome/browser/media/ta...
chrome/browser/media/tab_desktop_media_list.cc:30: uint32_t value =
base::SuperFastHash(
On 2016/01/26 18:06:54, Sergey Ulanov wrote:
> Use base::Hash() instead of SuperFastHash().
> Super-fast hash is a crappy hash function (it's not that fast and has many
> collisions, see
>
http://programmers.stackexchange.com/questions/49550/which-hashing-algorithm-...)
> and there is no reason to hard-code dependency on it here.

Done.

https://codereview.chromium.org/1622733002/diff/40001/chrome/browser/media/ta...
chrome/browser/media/tab_desktop_media_list.cc:39: return gfx::ImageSkia();
On 2016/01/26 18:06:54, Sergey Ulanov wrote:
> Why return an empty image here? Can this be a DCHECK? I don't think we expect
> thumbnail size to be smaller than 20x20

Done.

https://codereview.chromium.org/1622733002/diff/40001/chrome/browser/media/ta...
chrome/browser/media/tab_desktop_media_list.cc:47: canvas.drawARGB(255, 255,
255, 255);
On 2016/01/26 18:06:54, Sergey Ulanov wrote:
> Why is it white instead of transparent? This may not look right when the
> background in the picker dialog isn't white

I modified as required. Please refer to
https://docs.google.com/document/d/1VaVZyT3NlvcFXD0q3-r8PAMVKYOMHw8CjGBSzwGnO...
for an example.

https://codereview.chromium.org/1622733002/diff/40001/chrome/browser/media/ta...
chrome/browser/media/tab_desktop_media_list.cc:63: const double scale = fmin(3.0
/ 4 * result.width() / favicon.Width(),
On 2016/01/26 18:06:54, Sergey Ulanov wrote:
> std::min() instead of fmin()

Done.

https://codereview.chromium.org/1622733002/diff/40001/chrome/browser/media/ta...
chrome/browser/media/tab_desktop_media_list.cc:66: dest_rect.set(result.width()
/ 2 - favicon.Width() * scale / 2,     // left
On 2016/01/26 18:06:54, Sergey Ulanov wrote:
> Combine this with the previous line:
> 
> SkRect dest_rect = SkRect::MakeLTRB(....);

Done.

https://codereview.chromium.org/1622733002/diff/40001/chrome/browser/media/ta...
chrome/browser/media/tab_desktop_media_list.cc:70: const scoped_ptr<SkImage>
temp_image(
On 2016/01/26 18:06:54, Sergey Ulanov wrote:
> call it 'bitmap'? "temp_" doesn't make sense.

Done.

https://codereview.chromium.org/1622733002/diff/40001/chrome/browser/media/ta...
chrome/browser/media/tab_desktop_media_list.cc:128:
tab_map.insert(std::make_pair(t, SourceDescription(media_id, title)));
On 2016/01/26 01:01:50, qiangchenC wrote:
> simpler to use [] operator?

struct SourceDescription does have default constructor. Therefore use [] will
lead to compile error.

https://codereview.chromium.org/1622733002/diff/40001/chrome/browser/media/ta...
chrome/browser/media/tab_desktop_media_list.cc:139: favicon_map.insert(
On 2016/01/26 01:01:50, qiangchenC wrote:
> simpler to use []?
> favicon_map[media_id] = favicon.

Done.

https://codereview.chromium.org/1622733002/diff/40001/chrome/browser/media/ta...
chrome/browser/media/tab_desktop_media_list.cc:153: // For unit tests.
On 2016/01/26 18:06:54, Sergey Ulanov wrote:
> This doesn't look like the right way to unittest this code. The unittests
> execute almost no code that actually needs to be tested and the production
code
> is polluted with test-specific logic. I think it should be possible to create
> fake profile with fake tabs in the unittests and then you wouldn't need
> SetFakeSourcesForTesting()

Since tab related code will be removed from this CL, this issue will be
addressed in next cl.

https://codereview.chromium.org/1622733002/diff/40001/chrome/browser/media/ta...
File chrome/browser/media/tab_desktop_media_list.h (right):

https://codereview.chromium.org/1622733002/diff/40001/chrome/browser/media/ta...
chrome/browser/media/tab_desktop_media_list.h:23: void
SetFakeSourcesForTesting(std::vector<SourceDescription>& sources,
On 2016/01/26 01:01:50, qiangchenC wrote:
> Put ForTesting function in private and add the class that needs to call this
> function as TabDesktopMediaList's friend.

Done.

https://codereview.chromium.org/1622733002/diff/40001/chrome/browser/media/ta...
File chrome/browser/media/tab_desktop_media_list_unittest.cc (right):

https://codereview.chromium.org/1622733002/diff/40001/chrome/browser/media/ta...
chrome/browser/media/tab_desktop_media_list_unittest.cc:22: static const int
kDefaultSouceCount = 2;  // >=2.
On 2016/01/26 20:43:10, qiangchenC wrote:
> kDefaultSourceCount

Done.

https://codereview.chromium.org/1622733002/diff/40001/chrome/browser/media/ta...
chrome/browser/media/tab_desktop_media_list_unittest.cc:136:
testing::Mock::VerifyAndClearExpectations(&observer_);
On 2016/01/26 20:43:10, qiangchenC wrote:
> Again, make this to a function of observer_.

Done.

[Sergey Ulanov, 2016-01-27 00:36:31]

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

https://codereview.chromium.org/1622733002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/desktop_capture/desktop_capture_base.cc:150: new
CombinedDesktopMediaList(media_lists));
Does this need to be part of this CL? media_lists always contains 1 element
here, so CombinedDesktopMediaList doesn't make sense.

https://codereview.chromium.org/1622733002/diff/60001/chrome/browser/media/co...
File chrome/browser/media/combined_desktop_media_list.cc (right):

https://codereview.chromium.org/1622733002/diff/60001/chrome/browser/media/co...
chrome/browser/media/combined_desktop_media_list.cc:10: media_lists_ =
std::move(media_lists);
This can be moved to the initializer list instead of contructor body.

https://codereview.chromium.org/1622733002/diff/60001/chrome/browser/media/co...
chrome/browser/media/combined_desktop_media_list.cc:16: for (auto& list :
media_lists_)
const auto&?

https://codereview.chromium.org/1622733002/diff/60001/chrome/browser/media/co...
File chrome/browser/media/combined_desktop_media_list_unittest.cc (right):

https://codereview.chromium.org/1622733002/diff/60001/chrome/browser/media/co...
chrome/browser/media/combined_desktop_media_list_unittest.cc:21: static const
int kDefaultSourceCount = 2;  // >=2.
The comment at the end is confusing. Remove it?

https://codereview.chromium.org/1622733002/diff/60001/chrome/browser/media/co...
chrome/browser/media/combined_desktop_media_list_unittest.cc:60:
static_cast<uint8_t>(i));
Don't need this cast - the parameter is integer.

https://codereview.chromium.org/1622733002/diff/60001/chrome/browser/media/co...
chrome/browser/media/combined_desktop_media_list_unittest.cc:63:
~FakeDesktopMediaListBaseImpl() override{};
remove ;
add space before {

https://codereview.chromium.org/1622733002/diff/60001/chrome/browser/media/co...
chrome/browser/media/combined_desktop_media_list_unittest.cc:244:
message_loop_.Run();
I think you want to call SetUpdatePeriod() for all fake sources to make sure
they update frequently. Otherwise this test will be taking several seconds to
run, which is not ideal.

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

https://codereview.chromium.org/1622733002/diff/60001/chrome/browser/media/de...
chrome/browser/media/desktop_media_list_base.h:19: struct SourceDescription {
Why does it need to be public.

https://codereview.chromium.org/1622733002/diff/60001/content/public/browser/...
File content/public/browser/desktop_media_id.cc (right):

https://codereview.chromium.org/1622733002/diff/60001/content/public/browser/...
content/public/browser/desktop_media_id.cc:105: std::tie(other.type, other.id,
other.web_contents_id);
this is not related to this CL. I don't mind keeping it here, but mention this
fix in the description.

[GeorgeZ, 2016-01-27 01:12:25]

Description was changed from

==========
This CL adds CombinedDesktopMediaList class and its associated unit tests. It
intends to support chrome tab capture for JavaScript API chooseDesktopMedia().

Design document is in
https://docs.google.com/document/d/1zZOiayDzVkuIQ3twEdiE6S-LTXz9fWOGKjbMIsLTc...

BUG=557222
==========

to

==========
This CL adds CombinedDesktopMediaList class and its associated unit tests. It
intends to support chrome tab capture for JavaScript API chooseDesktopMedia().

This CL also fixed a bug in desktop_media_id.cc. The bug was assocated with
function bool DesktopMediaID::operator<(const DesktopMediaID& other) const.

Design document is in
https://docs.google.com/document/d/1zZOiayDzVkuIQ3twEdiE6S-LTXz9fWOGKjbMIsLTc...

BUG=557222
==========

[GeorgeZ, 2016-01-27 16:57:43]

Description was changed from

==========
This CL adds CombinedDesktopMediaList class and its associated unit tests. It
intends to support chrome tab capture for JavaScript API chooseDesktopMedia().

This CL also fixed a bug in desktop_media_id.cc. The bug was assocated with
function bool DesktopMediaID::operator<(const DesktopMediaID& other) const.

Design document is in
https://docs.google.com/document/d/1zZOiayDzVkuIQ3twEdiE6S-LTXz9fWOGKjbMIsLTc...

BUG=557222
==========

to

==========
This CL adds CombinedDesktopMediaList class and its associated unit tests. It
intends to support chrome tab capture for JavaScript API chooseDesktopMedia().

Design document is in
https://docs.google.com/document/d/1zZOiayDzVkuIQ3twEdiE6S-LTXz9fWOGKjbMIsLTc...

BUG=557222
==========

[GeorgeZ, 2016-01-27 22:57:17]

Sergey,

Patches were uploaded to address your comments.

Thanks,

George

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

https://codereview.chromium.org/1622733002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/desktop_capture/desktop_capture_base.cc:150: new
CombinedDesktopMediaList(media_lists));
On 2016/01/27 00:36:31, Sergey Ulanov wrote:
> Does this need to be part of this CL? media_lists always contains 1 element
> here, so CombinedDesktopMediaList doesn't make sense.

This makes CombinedDesktopMediaList be part of live workflow of desktop media
share. Therefore, beside unit-tests, I can manually test it to make sure that
desktop media share is still working.

I will revert this file so that CombinedDesktopMediaList will detached from
workflow.

https://codereview.chromium.org/1622733002/diff/60001/chrome/browser/media/co...
File chrome/browser/media/combined_desktop_media_list.cc (right):

https://codereview.chromium.org/1622733002/diff/60001/chrome/browser/media/co...
chrome/browser/media/combined_desktop_media_list.cc:10: media_lists_ =
std::move(media_lists);
On 2016/01/27 00:36:31, Sergey Ulanov wrote:
> This can be moved to the initializer list instead of contructor body.

Done.

https://codereview.chromium.org/1622733002/diff/60001/chrome/browser/media/co...
chrome/browser/media/combined_desktop_media_list.cc:16: for (auto& list :
media_lists_)
On 2016/01/27 00:36:31, Sergey Ulanov wrote:
> const auto&?

Done.

https://codereview.chromium.org/1622733002/diff/60001/chrome/browser/media/co...
File chrome/browser/media/combined_desktop_media_list_unittest.cc (right):

https://codereview.chromium.org/1622733002/diff/60001/chrome/browser/media/co...
chrome/browser/media/combined_desktop_media_list_unittest.cc:21: static const
int kDefaultSourceCount = 2;  // >=2.
On 2016/01/27 00:36:31, Sergey Ulanov wrote:
> The comment at the end is confusing. Remove it?

Done.

https://codereview.chromium.org/1622733002/diff/60001/chrome/browser/media/co...
chrome/browser/media/combined_desktop_media_list_unittest.cc:60:
static_cast<uint8_t>(i));
On 2016/01/27 00:36:31, Sergey Ulanov wrote:
> Don't need this cast - the parameter is integer.

Done.

https://codereview.chromium.org/1622733002/diff/60001/chrome/browser/media/co...
chrome/browser/media/combined_desktop_media_list_unittest.cc:63:
~FakeDesktopMediaListBaseImpl() override{};
On 2016/01/27 00:36:31, Sergey Ulanov wrote:
> remove ;
> add space before {

Done.

https://codereview.chromium.org/1622733002/diff/60001/chrome/browser/media/co...
chrome/browser/media/combined_desktop_media_list_unittest.cc:244:
message_loop_.Run();
On 2016/01/27 00:36:31, Sergey Ulanov wrote:
> I think you want to call SetUpdatePeriod() for all fake sources to make sure
> they update frequently. Otherwise this test will be taking several seconds to
> run, which is not ideal.

in lines 53&54 
explicit FakeDesktopMediaListBaseImpl(DesktopMediaID::Type type)
      : DesktopMediaListBase(base::TimeDelta::FromMilliseconds(1)),

The update period is set to 1 millisecond.

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

https://codereview.chromium.org/1622733002/diff/60001/chrome/browser/media/de...
chrome/browser/media/desktop_media_list_base.h:19: struct SourceDescription {
On 2016/01/27 00:36:31, Sergey Ulanov wrote:
> Why does it need to be public.

Good catch. This struct was used in unit test TabDesktopMediaListTest. I move it
back to protected.

https://codereview.chromium.org/1622733002/diff/60001/content/public/browser/...
File content/public/browser/desktop_media_id.cc (right):

https://codereview.chromium.org/1622733002/diff/60001/content/public/browser/...
content/public/browser/desktop_media_id.cc:105: std::tie(other.type, other.id,
other.web_contents_id);
On 2016/01/27 00:36:31, Sergey Ulanov wrote:
> this is not related to this CL. I don't mind keeping it here, but mention this
> fix in the description.

Revert this change and put it into another CL.

[qiangchen, 2016-01-28 18:40:03]

lgtm

[Sergey Ulanov, 2016-01-28 21:16:15]

lgtm

[GeorgeZ, 2016-01-28 21:37:17]

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

[GeorgeZ, 2016-01-28 21:37:18]

Msw,

As an owner, would you please have a review of 
chrome/browser/ui/views/desktop_media_picker_views.h
chrome/browser/ui/views/desktop_media_picker_views.cc

Thanks,

George

[msw, 2016-01-28 23:32:50]

c/b/ui/views rubber stamp lgtm (didn't look elsewhere)

[GeorgeZ, 2016-01-28 23:52:52]

The CQ bit was checked by gyzhou@chromium.org

[commit-bot: I haz the power, 2016-01-28 23:53:54]

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

[commit-bot: I haz the power, 2016-01-29 00:51:09]

Description was changed from

==========
This CL adds CombinedDesktopMediaList class and its associated unit tests. It
intends to support chrome tab capture for JavaScript API chooseDesktopMedia().

Design document is in
https://docs.google.com/document/d/1zZOiayDzVkuIQ3twEdiE6S-LTXz9fWOGKjbMIsLTc...

BUG=557222
==========

to

==========
This CL adds CombinedDesktopMediaList class and its associated unit tests. It
intends to support chrome tab capture for JavaScript API chooseDesktopMedia().

Design document is in
https://docs.google.com/document/d/1zZOiayDzVkuIQ3twEdiE6S-LTXz9fWOGKjbMIsLTc...

BUG=557222
==========

[commit-bot: I haz the power, 2016-01-29 00:51:11]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-01-29 00:52:26]

Description was changed from

==========
This CL adds CombinedDesktopMediaList class and its associated unit tests. It
intends to support chrome tab capture for JavaScript API chooseDesktopMedia().

Design document is in
https://docs.google.com/document/d/1zZOiayDzVkuIQ3twEdiE6S-LTXz9fWOGKjbMIsLTc...

BUG=557222
==========

to

==========
This CL adds CombinedDesktopMediaList class and its associated unit tests. It
intends to support chrome tab capture for JavaScript API chooseDesktopMedia().

Design document is in
https://docs.google.com/document/d/1zZOiayDzVkuIQ3twEdiE6S-LTXz9fWOGKjbMIsLTc...

BUG=557222

Committed: https://crrev.com/2716e8e3bf7a9f216de1ece328037fef76e64a27
Cr-Commit-Position: refs/heads/master@{#372228}
==========

[commit-bot: I haz the power, 2016-01-29 00:52:27]

Patchset 10 (id:??) landed as
https://crrev.com/2716e8e3bf7a9f216de1ece328037fef76e64a27
Cr-Commit-Position: refs/heads/master@{#372228}