Video Capture Capabilities available on chrome://media-internals

On the chrome://media-internals web page, display a table of
video capture devices and supported formats.

Each change to the list of devices (e.g. plug/unplug)
automatically refreshes that part of the page.

Also allows copying the data in JSON format via window prompt.

BUG=344882
Committed: https://crrev.com/7196356552ee0c796e3b42f307a8bb1535205e59
Cr-Commit-Position: refs/heads/master@{#300108}

[burnik, 2014-10-14 12:19:16]

burnik@chromium.org changed reviewers:
+ mcasas@chromium.org, perkj@chromium.org, xians@chromium.org

[burnik, 2014-10-14 12:19:50]

Still a few rough edges. 
I suggest applying the patch to see it in action. :-)

[mcasas, 2014-10-14 13:44:12]

Few nits. Plz complete media_internals_unittest.cc.

https://codereview.chromium.org/643343004/diff/1/content/browser/media/media_...
File content/browser/media/media_internals.cc (right):

https://codereview.chromium.org/643343004/diff/1/content/browser/media/media_...
content/browser/media/media_internals.cc:268: // TODO(mcasas): Remove the
following printout when sending the capabilities
Remove TODO /rewrite comment

https://codereview.chromium.org/643343004/diff/1/content/browser/media/media_...
content/browser/media/media_internals.cc:275: 
Remove empty line.

https://codereview.chromium.org/643343004/diff/1/content/browser/media/media_...
File content/browser/media/media_internals.h (right):

https://codereview.chromium.org/643343004/diff/1/content/browser/media/media_...
content/browser/media/media_internals.h:50: // TODO(burnik): Update comment.
TODO..

https://codereview.chromium.org/643343004/diff/1/content/browser/media/media_...
File content/browser/media/media_internals_proxy.cc (right):

https://codereview.chromium.org/643343004/diff/1/content/browser/media/media_...
content/browser/media/media_internals_proxy.cc:142: // TODO(mcasas): Collect
capabilities of Video Capture devices.
Remove comment.

https://codereview.chromium.org/643343004/diff/1/content/browser/resources/me...
File content/browser/resources/media/media_internals.css (right):

https://codereview.chromium.org/643343004/diff/1/content/browser/resources/me...
content/browser/resources/media/media_internals.css:12: font-family:Arial;
Please make the whole font smaller. It doesn't fit well now
that there's more info on the page.

[burnik, 2014-10-15 12:05:23]

Nits done.

[burnik, 2014-10-15 13:45:01]

Updated unit test

[burnik, 2014-10-15 13:45:58]

Updated the unit test as well. Hoping to have green bots and no typos for non
Linux/ChromeOS. :-)

https://codereview.chromium.org/643343004/diff/1/content/browser/media/media_...
File content/browser/media/media_internals.cc (right):

https://codereview.chromium.org/643343004/diff/1/content/browser/media/media_...
content/browser/media/media_internals.cc:268: // TODO(mcasas): Remove the
following printout when sending the capabilities
On 2014/10/14 13:44:11, mcasas wrote:
> Remove TODO /rewrite comment

Done.

https://codereview.chromium.org/643343004/diff/1/content/browser/media/media_...
content/browser/media/media_internals.cc:275: 
On 2014/10/14 13:44:11, mcasas wrote:
> Remove empty line.

Done.

https://codereview.chromium.org/643343004/diff/1/content/browser/media/media_...
File content/browser/media/media_internals.h (right):

https://codereview.chromium.org/643343004/diff/1/content/browser/media/media_...
content/browser/media/media_internals.h:50: // TODO(burnik): Update comment.
On 2014/10/14 13:44:11, mcasas wrote:
> TODO..

Done. The comment seems ok to me. It's the same thing as the audio cached data.

https://codereview.chromium.org/643343004/diff/1/content/browser/media/media_...
File content/browser/media/media_internals_proxy.cc (right):

https://codereview.chromium.org/643343004/diff/1/content/browser/media/media_...
content/browser/media/media_internals_proxy.cc:142: // TODO(mcasas): Collect
capabilities of Video Capture devices.
On 2014/10/14 13:44:12, mcasas wrote:
> Remove comment.

Done.

https://codereview.chromium.org/643343004/diff/1/content/browser/resources/me...
File content/browser/resources/media/media_internals.css (right):

https://codereview.chromium.org/643343004/diff/1/content/browser/resources/me...
content/browser/resources/media/media_internals.css:12: font-family:Arial;
On 2014/10/14 13:44:12, mcasas wrote:
> Please make the whole font smaller. It doesn't fit well now
> that there's more info on the page.

Done.

[burnik, 2014-10-15 13:50:23]

Unit test: Remove scoped_ptr for expected_list.

[mcasas, 2014-10-15 13:54:43]

LGTM modulo my comments.

https://codereview.chromium.org/643343004/diff/40001/content/browser/media/me...
File content/browser/media/media_internals_unittest.cc (right):

https://codereview.chromium.org/643343004/diff/40001/content/browser/media/me...
content/browser/media/media_internals_unittest.cc:35: // or for video
capabilities: "<JavaScript Function Name>([{<JSON>}]);".
<JavaScript Function Name>([{<JSON>}]);
is repeated.

https://codereview.chromium.org/643343004/diff/40001/content/browser/media/me...
content/browser/media/media_internals_unittest.cc:69: base::ListValue *
expected_list){
const base::ListValue& ?
Also, method const.

https://codereview.chromium.org/643343004/diff/40001/content/browser/media/me...
content/browser/media/media_internals_unittest.cc:151: "resolution: 1280x720,
fps: 30.000000, pixel format: I420");
Prefer to compose this on the fly calling format_hd->ToString().

[burnik, 2014-10-15 14:02:05]

s/ExpectInteger/ExpectInt

[burnik, 2014-10-15 14:21:43]

Nits

[burnik, 2014-10-15 14:21:57]

Nits done.

https://codereview.chromium.org/643343004/diff/40001/content/browser/media/me...
File content/browser/media/media_internals_unittest.cc (right):

https://codereview.chromium.org/643343004/diff/40001/content/browser/media/me...
content/browser/media/media_internals_unittest.cc:35: // or for video
capabilities: "<JavaScript Function Name>([{<JSON>}]);".
On 2014/10/15 13:54:42, mcasas wrote:
> <JavaScript Function Name>([{<JSON>}]);
> is repeated.

Look more closely.

https://codereview.chromium.org/643343004/diff/40001/content/browser/media/me...
content/browser/media/media_internals_unittest.cc:69: base::ListValue *
expected_list){
On 2014/10/15 13:54:42, mcasas wrote:
> const base::ListValue& ?
> Also, method const.

Done.

https://codereview.chromium.org/643343004/diff/40001/content/browser/media/me...
content/browser/media/media_internals_unittest.cc:151: "resolution: 1280x720,
fps: 30.000000, pixel format: I420");
On 2014/10/15 13:54:42, mcasas wrote:
> Prefer to compose this on the fly calling format_hd->ToString().

Done.

[perkj_chrome, 2014-10-15 15:05:16]

https://codereview.chromium.org/643343004/diff/100001/content/browser/media/m...
File content/browser/media/media_internals.cc (right):

https://codereview.chromium.org/643343004/diff/100001/content/browser/media/m...
content/browser/media/media_internals.cc:239: const
media::VideoCaptureDeviceInfos& video_capture_device_infos) {
isn't this on io too. In that case- add DCHECKS for that and remove the use of
lock.

https://codereview.chromium.org/643343004/diff/100001/content/browser/media/m...
content/browser/media/media_internals.cc:242: base::AutoLock auto_lock(lock_);
why is a lock needed? What thread is this and what thread are you protecting
against. You should be able to post this to the right thread instead.

https://codereview.chromium.org/643343004/diff/100001/content/browser/media/m...
File content/browser/media/media_internals.h (right):

https://codereview.chromium.org/643343004/diff/100001/content/browser/media/m...
content/browser/media/media_internals.h:50: // TODO(burnik): Update comment.
fix now please.

https://codereview.chromium.org/643343004/diff/100001/content/browser/media/m...
File content/browser/media/media_internals_proxy.cc (right):

https://codereview.chromium.org/643343004/diff/100001/content/browser/media/m...
content/browser/media/media_internals_proxy.cc:141:
MediaInternals::GetInstance()->SendVideoCaptureCapabilities();
This is on IO.

https://codereview.chromium.org/643343004/diff/100001/content/browser/resourc...
File content/browser/resources/media/client_renderer.js (right):

https://codereview.chromium.org/643343004/diff/100001/content/browser/resourc...
content/browser/resources/media/client_renderer.js:126: 
nit: remove empty line.

https://codereview.chromium.org/643343004/diff/100001/content/browser/resourc...
content/browser/resources/media/client_renderer.js:146: if ( (typeof value) ==
(typeof []) ) {
I don't think you are supposed to have space in  ( ( or ) )

[no longer working on chromium, 2014-10-15 15:08:03]

I don't think the current reviewer list is enough, you should involve
reviewer(s) who are familiar with the media_internal/js/html/css code.

[burnik, 2014-10-15 15:42:06]

Remove thread locks, add thread checks. JS nits.

[burnik, 2014-10-15 15:43:13]

Thread checks and locks done.

https://codereview.chromium.org/643343004/diff/100001/content/browser/media/m...
File content/browser/media/media_internals.cc (right):

https://codereview.chromium.org/643343004/diff/100001/content/browser/media/m...
content/browser/media/media_internals.cc:239: const
media::VideoCaptureDeviceInfos& video_capture_device_infos) {
On 2014/10/15 15:05:16, perkj wrote:
> isn't this on io too. In that case- add DCHECKS for that and remove the use of
> lock.

Done.

https://codereview.chromium.org/643343004/diff/100001/content/browser/media/m...
content/browser/media/media_internals.cc:242: base::AutoLock auto_lock(lock_);
On 2014/10/15 15:05:16, perkj wrote:
> why is a lock needed? What thread is this and what thread are you protecting
> against. You should be able to post this to the right thread instead.

You're right. I forgot the fact that both device enumeration and callbacks from
JS live on the IO thread.

https://codereview.chromium.org/643343004/diff/100001/content/browser/media/m...
File content/browser/media/media_internals.h (right):

https://codereview.chromium.org/643343004/diff/100001/content/browser/media/m...
content/browser/media/media_internals.h:50: // TODO(burnik): Update comment.
On 2014/10/15 15:05:16, perkj wrote:
> fix now please.

Done.

https://codereview.chromium.org/643343004/diff/100001/content/browser/media/m...
File content/browser/media/media_internals_proxy.cc (right):

https://codereview.chromium.org/643343004/diff/100001/content/browser/media/m...
content/browser/media/media_internals_proxy.cc:141:
MediaInternals::GetInstance()->SendVideoCaptureCapabilities();
On 2014/10/15 15:05:16, perkj wrote:
> This is on IO.

Done.

https://codereview.chromium.org/643343004/diff/100001/content/browser/resourc...
File content/browser/resources/media/client_renderer.js (right):

https://codereview.chromium.org/643343004/diff/100001/content/browser/resourc...
content/browser/resources/media/client_renderer.js:126: 
On 2014/10/15 15:05:16, perkj wrote:
> nit: remove empty line.

Done.

https://codereview.chromium.org/643343004/diff/100001/content/browser/resourc...
content/browser/resources/media/client_renderer.js:146: if ( (typeof value) ==
(typeof []) ) {
On 2014/10/15 15:05:16, perkj wrote:
> I don't think you are supposed to have space in  ( ( or ) )

Done.

[burnik, 2014-10-15 15:44:22]

burnik@chromium.org changed reviewers:
+ tommi@chromium.org, xhwang@chromium.org

[burnik, 2014-10-15 15:44:42]

+xhwang
+tommi

[burnik, 2014-10-15 15:47:05]

burnik@chromium.org changed reviewers:
+ wolenetz@chromium.org

[burnik, 2014-10-15 15:47:18]

+ wolenetz

[perkj_chrome, 2014-10-16 08:35:28]

c++ lgtm if the RunLoop is removed.

https://codereview.chromium.org/643343004/diff/120001/content/browser/media/m...
File content/browser/media/media_internals_unittest.cc (right):

https://codereview.chromium.org/643343004/diff/120001/content/browser/media/m...
content/browser/media/media_internals_unittest.cc:141:
base::RunLoop().RunUntilIdle();
Why RunRLoop? The code is syncronouse right?

[burnik, 2014-10-16 09:39:41]

Remove mcasas' legacy.

[burnik, 2014-10-16 09:40:33]

Done.

https://codereview.chromium.org/643343004/diff/120001/content/browser/media/m...
File content/browser/media/media_internals_unittest.cc (right):

https://codereview.chromium.org/643343004/diff/120001/content/browser/media/m...
content/browser/media/media_internals_unittest.cc:141:
base::RunLoop().RunUntilIdle();
On 2014/10/16 08:35:28, perkj wrote:
> Why RunRLoop? The code is syncronouse right?

Legacy. Removed.

[no longer working on chromium, 2014-10-16 11:10:40]

I defer the review to other reviewers since I am not familiar with those
media_internal code.

[burnik, 2014-10-16 11:16:11]

burnik@chromium.org changed reviewers:
- xians@chromium.org

[burnik, 2014-10-16 11:25:54]

Style fix in unit test

[xhwang, 2014-10-16 17:54:18]

lgtm % a few questions

https://codereview.chromium.org/643343004/diff/160001/content/browser/media/m...
File content/browser/media/media_internals.cc (right):

https://codereview.chromium.org/643343004/diff/160001/content/browser/media/m...
content/browser/media/media_internals.cc:231: }
Just a question: audio_streams_cached_data_ is protected by a lock, we don't
need it for video_capture_capabilities_cached_data_?

https://codereview.chromium.org/643343004/diff/160001/content/browser/media/m...
File content/browser/media/media_internals_unittest.cc (right):

https://codereview.chromium.org/643343004/diff/160001/content/browser/media/m...
content/browser/media/media_internals_unittest.cc:70: const base::ListValue *
actual_list;
Remove space before *

https://codereview.chromium.org/643343004/diff/160001/content/browser/resourc...
File content/browser/resources/media/client_renderer.js (right):

https://codereview.chromium.org/643343004/diff/160001/content/browser/resourc...
content/browser/resources/media/client_renderer.js:125:
redrawVideoCaptureCapabilities: function( videoCaptureCapabilities, keys ) {
nit: Remove extra spaces

[burnik, 2014-10-16 18:28:44]

Nits @ wolenetz

[burnik, 2014-10-16 18:29:56]

There was a discussion on locks/threading with perkj, see patchset 7. :-)

https://codereview.chromium.org/643343004/diff/160001/content/browser/media/m...
File content/browser/media/media_internals.cc (right):

https://codereview.chromium.org/643343004/diff/160001/content/browser/media/m...
content/browser/media/media_internals.cc:231: }
On 2014/10/16 17:54:17, xhwang wrote:
> Just a question: audio_streams_cached_data_ is protected by a lock, we don't
> need it for video_capture_capabilities_cached_data_?

No. All calls for device enumeration and update requests from JS run on the
Browser IO thread.

Audio streams can get updated on any thread.

https://codereview.chromium.org/643343004/diff/160001/content/browser/media/m...
File content/browser/media/media_internals_unittest.cc (right):

https://codereview.chromium.org/643343004/diff/160001/content/browser/media/m...
content/browser/media/media_internals_unittest.cc:70: const base::ListValue *
actual_list;
On 2014/10/16 17:54:17, xhwang wrote:
> Remove space before *

Done.

https://codereview.chromium.org/643343004/diff/160001/content/browser/resourc...
File content/browser/resources/media/client_renderer.js (right):

https://codereview.chromium.org/643343004/diff/160001/content/browser/resourc...
content/browser/resources/media/client_renderer.js:125:
redrawVideoCaptureCapabilities: function( videoCaptureCapabilities, keys ) {
On 2014/10/16 17:54:17, xhwang wrote:
> nit: Remove extra spaces

Done. And all double quotes are now single.

[burnik, 2014-10-17 14:01:11]

burnik@chromium.org changed reviewers:
- wolenetz@chromium.org

[burnik, 2014-10-17 14:02:46]

-wolenetz
No response in over 24hrs.

[tommi (sloooow) - chröme, 2014-10-17 14:06:57]

nice! lgtm.

[burnik, 2014-10-17 14:07:59]

The CQ bit was checked by burnik@chromium.org

[commit-bot: I haz the power, 2014-10-17 14:09:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/643343004/170001

[commit-bot: I haz the power, 2014-10-17 14:57:29]

Committed patchset #10 (id:170001)

[commit-bot: I haz the power, 2014-10-17 14:58:20]

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