Show meaningful name for cameras

Show meaningful name for cameras

The camera names in settings are composed by display_name + model_id.
When there are front and back cameras, the user can not tell which is
which by the model ID. After this change, it shows
display_name + model_id + facing_info

BUG=chrome-os-partner:62956
TEST=Verify that display_name + model_id + facing_info shows in settings

Review-Url: https://codereview.chromium.org/2703393007
Cr-Commit-Position: refs/heads/master@{#454531}
Committed: https://chromium.googlesource.com/chromium/src/+/75a64969dd5f72c9ff2fff22fd3638a95fe800fe

[shenghao, 2017-02-22 03:15:18]

shenghao@chromium.org changed reviewers:
+ posciak@chromium.org, tommi@chromium.org, wuchengli@chromium.org

[mtomasz, 2017-02-22 03:20:37]

mtomasz@chromium.org changed reviewers:
+ mtomasz@chromium.org

[mtomasz, 2017-02-22 03:20:37]

https://codereview.chromium.org/2703393007/diff/1/media/capture/video/video_c...
File media/capture/video/video_capture_device_descriptor.cc (right):

https://codereview.chromium.org/2703393007/diff/1/media/capture/video/video_c...
media/capture/video/video_capture_device_descriptor.cc:108: return
GetNameAndModel() + " (" + facing_info + ")";
If human readable, then should we localize? If hard to localize here, then maybe
easier to localize in UI (settings). In such case, maybe GetHumanReadableName is
not needed at all.

[tommi (sloooow) - chröme, 2017-02-22 10:41:04]

https://codereview.chromium.org/2703393007/diff/1/media/capture/video/video_c...
File media/capture/video/video_capture_device_descriptor.cc (right):

https://codereview.chromium.org/2703393007/diff/1/media/capture/video/video_c...
media/capture/video/video_capture_device_descriptor.cc:108: return
GetNameAndModel() + " (" + facing_info + ")";
On 2017/02/22 03:20:37, mtomasz wrote:
> If human readable, then should we localize? If hard to localize here, then
maybe
> easier to localize in UI (settings). In such case, maybe GetHumanReadableName
is
> not needed at all.

I don't think we have a way of knowing what language the name and model will be
in and these names are not localized by us.  Hmm... given that, is adding the
facing info to the label, is useful or if it will be confusing for users?

[shenghao, 2017-02-22 18:44:23]

On 2017/02/22 10:41:04, tommi (krómíum) wrote:
>
https://codereview.chromium.org/2703393007/diff/1/media/capture/video/video_c...
> File media/capture/video/video_capture_device_descriptor.cc (right):
> 
>
https://codereview.chromium.org/2703393007/diff/1/media/capture/video/video_c...
> media/capture/video/video_capture_device_descriptor.cc:108: return
> GetNameAndModel() + " (" + facing_info + ")";
> On 2017/02/22 03:20:37, mtomasz wrote:
> > If human readable, then should we localize? If hard to localize here, then
> maybe
> > easier to localize in UI (settings). In such case, maybe
GetHumanReadableName
> is
> > not needed at all.
> 
> I don't think we have a way of knowing what language the name and model will
be
> in and these names are not localized by us.  Hmm... given that, is adding the
> facing info to the label, is useful or if it will be confusing for users?

I plan to add facing strings into grd file and use the localization mechanism
there.
The facing info is very useful to the user. Currently the name shown in settings
are:

Webcam (1234:2345)
Webcam (4567:5678)

The user can't make sense of the vid:pid pair (model ID). What's meaningful to
them is the facing info.

[tommi (sloooow) - chröme, 2017-02-22 18:46:39]

On 2017/02/22 18:44:23, shenghao wrote:
> On 2017/02/22 10:41:04, tommi (krómíum) wrote:
> >
>
https://codereview.chromium.org/2703393007/diff/1/media/capture/video/video_c...
> > File media/capture/video/video_capture_device_descriptor.cc (right):
> > 
> >
>
https://codereview.chromium.org/2703393007/diff/1/media/capture/video/video_c...
> > media/capture/video/video_capture_device_descriptor.cc:108: return
> > GetNameAndModel() + " (" + facing_info + ")";
> > On 2017/02/22 03:20:37, mtomasz wrote:
> > > If human readable, then should we localize? If hard to localize here, then
> > maybe
> > > easier to localize in UI (settings). In such case, maybe
> GetHumanReadableName
> > is
> > > not needed at all.
> > 
> > I don't think we have a way of knowing what language the name and model will
> be
> > in and these names are not localized by us.  Hmm... given that, is adding
the
> > facing info to the label, is useful or if it will be confusing for users?
> 
> I plan to add facing strings into grd file and use the localization mechanism
> there.
> The facing info is very useful to the user. Currently the name shown in
settings
> are:
> 
> Webcam (1234:2345)
> Webcam (4567:5678)
> 
> The user can't make sense of the vid:pid pair (model ID). What's meaningful to
> them is the facing info.

Ah, yeah that's pretty terrible :)  Is this on CrOS?
In any case lgtm to this change as it looks like it will clearly be an
improvement.

[mcasas, 2017-02-22 19:03:58]

mcasas@chromium.org changed reviewers:
+ mcasas@chromium.org

[mcasas, 2017-02-22 19:03:59]

drive by

https://codereview.chromium.org/2703393007/diff/1/media/capture/video/video_c...
File media/capture/video/video_capture_device_descriptor.cc (right):

https://codereview.chromium.org/2703393007/diff/1/media/capture/video/video_c...
media/capture/video/video_capture_device_descriptor.cc:12: "", "user-facing",
"world-facing"};
nit: move the anonymous namespace inside media, 
 s/VIDEO_FACING_STRINGS/kVideoFacingNames/
and s/"world-facing"/"environment-facing"/
to comply with 
https://www.w3.org/TR/mediacapture-streams/#idl-def-VideoFacingModeEnum

https://codereview.chromium.org/2703393007/diff/1/media/capture/video/video_c...
media/capture/video/video_capture_device_descriptor.cc:107: }
nit: no {} for one-line bodies.

[shenghao, 2017-02-24 01:11:39]

https://codereview.chromium.org/2703393007/diff/1/media/capture/video/video_c...
File media/capture/video/video_capture_device_descriptor.cc (right):

https://codereview.chromium.org/2703393007/diff/1/media/capture/video/video_c...
media/capture/video/video_capture_device_descriptor.cc:12: "", "user-facing",
"world-facing"};
On 2017/02/22 19:03:59, mcasas wrote:
> nit: move the anonymous namespace inside media, 
>  s/VIDEO_FACING_STRINGS/kVideoFacingNames/
> and s/"world-facing"/"environment-facing"/
> to comply with 
> https://www.w3.org/TR/mediacapture-streams/#idl-def-VideoFacingModeEnum

Deleted

https://codereview.chromium.org/2703393007/diff/1/media/capture/video/video_c...
media/capture/video/video_capture_device_descriptor.cc:107: }
On 2017/02/22 19:03:59, mcasas wrote:
> nit: no {} for one-line bodies.

Done.

https://codereview.chromium.org/2703393007/diff/1/media/capture/video/video_c...
media/capture/video/video_capture_device_descriptor.cc:108: return
GetNameAndModel() + " (" + facing_info + ")";
On 2017/02/22 10:41:04, tommi (krómíum) wrote:
> On 2017/02/22 03:20:37, mtomasz wrote:
> > If human readable, then should we localize? If hard to localize here, then
> maybe
> > easier to localize in UI (settings). In such case, maybe
GetHumanReadableName
> is
> > not needed at all.
> 
> I don't think we have a way of knowing what language the name and model will
be
> in and these names are not localized by us.  Hmm... given that, is adding the
> facing info to the label, is useful or if it will be confusing for users?

Replied in previous comment.

[shenghao, 2017-02-24 01:17:36]

Description was changed from

==========
Show meaningful name for cameras

The camera names in settings are composed by display_name + model_id.
When there are front and back cameras, the user can not tell which is
which by the model ID. After this change, it shows
display_name + model_id + facing_info

BUG=chrome-os-partner:62956
TEST=Verify that display_name + model_id + facing_info shows in settings
==========

to

==========
Show meaningful name for cameras

The camera names in settings are composed by display_name + model_id.
When there are front and back cameras, the user can not tell which is
which by the model ID. After this change, it shows
display_name + model_id + facing_info

BUG=chrome-os-partner:62956
TEST=Verify that display_name + model_id + facing_info shows in settings
==========

[shenghao, 2017-02-24 01:17:36]

shenghao@chromium.org changed reviewers:
+ rockot@chromium.org

[mtomasz, 2017-02-27 01:27:36]

https://codereview.chromium.org/2703393007/diff/40001/media/capture/video/DEPS
File media/capture/video/DEPS (right):

https://codereview.chromium.org/2703393007/diff/40001/media/capture/video/DEP...
media/capture/video/DEPS:2: "+extensions/strings/grit/extensions_strings.h",
This dependency looks bad. Media shouldn't depend on extensions and UI. I know I
suggested l10util earlier, but I don't think it's right.

Could we move the translation to somewhere higher, eg.
https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/settings/setting...

[shenghao, 2017-03-01 09:49:16]

On 2017/02/27 01:27:36, mtomasz wrote:
> https://codereview.chromium.org/2703393007/diff/40001/media/capture/video/DEPS
> File media/capture/video/DEPS (right):
> 
>
https://codereview.chromium.org/2703393007/diff/40001/media/capture/video/DEP...
> media/capture/video/DEPS:2: "+extensions/strings/grit/extensions_strings.h",
> This dependency looks bad. Media shouldn't depend on extensions and UI. I know
I
> suggested l10util earlier, but I don't think it's right.
> 
> Could we move the translation to somewhere higher, eg.
>
https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/settings/setting...

I added LOG(ERROR) to print out debug message in
MediaDevicesSelectionHandler::UpdateDevicesMenu() and discovered that it's never
called.
May you suggest other place that video device menu in setting is generated?

[Ken Rockot(use gerrit already), 2017-03-01 17:48:04]

I'm assuming you want me to review //extensions, in which case, rs lgtm

[shenghao, 2017-03-02 07:19:57]

shenghao@chromium.org changed reviewers:
+ dbeam@chromium.org

[shenghao, 2017-03-02 07:20:13]

https://codereview.chromium.org/2703393007/diff/40001/media/capture/video/DEPS
File media/capture/video/DEPS (right):

https://codereview.chromium.org/2703393007/diff/40001/media/capture/video/DEP...
media/capture/video/DEPS:2: "+extensions/strings/grit/extensions_strings.h",
On 2017/02/27 01:27:36, mtomasz wrote:
> This dependency looks bad. Media shouldn't depend on extensions and UI. I know
I
> suggested l10util earlier, but I don't think it's right.
> 
> Could we move the translation to somewhere higher, eg.
>
https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/settings/setting...

Done.

[mtomasz, 2017-03-02 07:49:54]

lgtm with nits, thanks!

https://codereview.chromium.org/2703393007/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/media_devices_selection_handler.cc (right):

https://codereview.chromium.org/2703393007/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/media_devices_selection_handler.cc:148: case
media::VideoFacingMode::NUM_MEDIA_VIDEO_FACING_MODES:
nit: Add NOTREACHED()?

https://codereview.chromium.org/2703393007/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/media_devices_selection_handler.cc:154: return
device.name + " (" + facing_info + ")";
This may provide broken strings in some languages, eg. in some languages
different brackets may be used. Could you add least add a TODO?

https://codereview.chromium.org/2703393007/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/media_devices_selection_handler.h (right):

https://codereview.chromium.org/2703393007/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/media_devices_selection_handler.h:50:
std::string GetDeviceDisplayName(const content::MediaStreamDevice& device);
nit: Can this method be const?

https://codereview.chromium.org/2703393007/diff/60001/chrome/browser/ui/webui...
File
chrome/browser/ui/webui/settings/settings_media_devices_selection_handler.cc
(right):

https://codereview.chromium.org/2703393007/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/settings_media_devices_selection_handler.cc:158:
break;
ditto

https://codereview.chromium.org/2703393007/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/settings_media_devices_selection_handler.cc:161:
if (facing_info.empty())
ditto

https://codereview.chromium.org/2703393007/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/settings_media_devices_selection_handler.cc:161:
if (facing_info.empty())
This string will never be empty. Can we have a DCHECK instead?

https://codereview.chromium.org/2703393007/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/settings_media_devices_selection_handler.cc:163:
return device.name + " (" + facing_info + ")";
ditto

https://codereview.chromium.org/2703393007/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/settings_media_devices_selection_handler.h
(right):

https://codereview.chromium.org/2703393007/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/settings_media_devices_selection_handler.h:54:
std::string GetDeviceDisplayName(const content::MediaStreamDevice& device);
nit: Can this method be const?

[shenghao, 2017-03-02 12:43:41]

https://codereview.chromium.org/2703393007/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/media_devices_selection_handler.cc (right):

https://codereview.chromium.org/2703393007/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/media_devices_selection_handler.cc:148: case
media::VideoFacingMode::NUM_MEDIA_VIDEO_FACING_MODES:
On 2017/03/02 07:49:53, mtomasz wrote:
> nit: Add NOTREACHED()?

MEDIA_VIDEO_FACING_NONE is possible. I added NOTREACHED() for
NUM_MEDIA_VIDEO_FACING_MODES.

https://codereview.chromium.org/2703393007/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/media_devices_selection_handler.cc:154: return
device.name + " (" + facing_info + ")";
On 2017/03/02 07:49:53, mtomasz wrote:
> This may provide broken strings in some languages, eg. in some languages
> different brackets may be used. Could you add least add a TODO?

I would just delete the brackets.

https://codereview.chromium.org/2703393007/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/media_devices_selection_handler.h (right):

https://codereview.chromium.org/2703393007/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/media_devices_selection_handler.h:50:
std::string GetDeviceDisplayName(const content::MediaStreamDevice& device);
On 2017/03/02 07:49:53, mtomasz wrote:
> nit: Can this method be const?

Done.

https://codereview.chromium.org/2703393007/diff/60001/chrome/browser/ui/webui...
File
chrome/browser/ui/webui/settings/settings_media_devices_selection_handler.cc
(right):

https://codereview.chromium.org/2703393007/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/settings_media_devices_selection_handler.cc:158:
break;
On 2017/03/02 07:49:53, mtomasz wrote:
> ditto

Done.

https://codereview.chromium.org/2703393007/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/settings_media_devices_selection_handler.cc:161:
if (facing_info.empty())
On 2017/03/02 07:49:53, mtomasz wrote:
> ditto

It would be empty in the MEDIA_VIDEO_FACING_NONE case.

https://codereview.chromium.org/2703393007/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/settings_media_devices_selection_handler.cc:163:
return device.name + " (" + facing_info + ")";
On 2017/03/02 07:49:53, mtomasz wrote:
> ditto

Done.

https://codereview.chromium.org/2703393007/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/settings_media_devices_selection_handler.h
(right):

https://codereview.chromium.org/2703393007/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/settings_media_devices_selection_handler.h:54:
std::string GetDeviceDisplayName(const content::MediaStreamDevice& device);
On 2017/03/02 07:49:53, mtomasz wrote:
> nit: Can this method be const?

Done.

[Dan Beam, 2017-03-03 02:14:25]

lgtm

https://codereview.chromium.org/2703393007/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/media_devices_selection_handler.h (right):

https://codereview.chromium.org/2703393007/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/media_devices_selection_handler.h:49: 
doc comment (and in duplicate /settings/ version)

[shenghao, 2017-03-03 02:24:59]

The CQ bit was checked by shenghao@chromium.org

[shenghao, 2017-03-03 02:25:00]

The patchset sent to the CQ was uploaded after l-g-t-m from tommi@chromium.org,
rockot@chromium.org, mtomasz@chromium.org, dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/2703393007/#ps100001
(title: "Add comments")

[shenghao, 2017-03-03 02:25:11]

https://codereview.chromium.org/2703393007/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/media_devices_selection_handler.h (right):

https://codereview.chromium.org/2703393007/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/media_devices_selection_handler.h:49: 
On 2017/03/03 02:14:25, Dan Beam wrote:
> doc comment (and in duplicate /settings/ version)

Done.

[commit-bot: I haz the power, 2017-03-03 02:25:24]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-03 02:30:15]

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

[commit-bot: I haz the power, 2017-03-03 02:30:17]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[shenghao, 2017-03-03 03:41:18]

The CQ bit was checked by shenghao@chromium.org

[shenghao, 2017-03-03 03:41:19]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
tommi@chromium.org, dbeam@chromium.org, mtomasz@chromium.org
Link to the patchset: https://codereview.chromium.org/2703393007/#ps120001
(title: "Moved logic behind ENABLE_EXTENSIONS flag")

[commit-bot: I haz the power, 2017-03-03 03:41:28]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-03 04:09:28]

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

[commit-bot: I haz the power, 2017-03-03 04:09:29]

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

[shenghao, 2017-03-03 05:37:11]

The CQ bit was checked by shenghao@chromium.org

[shenghao, 2017-03-03 05:37:11]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
tommi@chromium.org, dbeam@chromium.org, mtomasz@chromium.org
Link to the patchset: https://codereview.chromium.org/2703393007/#ps140001
(title: "fix trybot")

[commit-bot: I haz the power, 2017-03-03 05:37:29]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-03 06:37:25]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1488519431441280,
"parent_rev": "762dcf004c4d65c416992d3c6ad4323a9303e9f0", "commit_rev":
"75a64969dd5f72c9ff2fff22fd3638a95fe800fe"}

[commit-bot: I haz the power, 2017-03-03 06:38:05]

Description was changed from

==========
Show meaningful name for cameras

The camera names in settings are composed by display_name + model_id.
When there are front and back cameras, the user can not tell which is
which by the model ID. After this change, it shows
display_name + model_id + facing_info

BUG=chrome-os-partner:62956
TEST=Verify that display_name + model_id + facing_info shows in settings
==========

to

==========
Show meaningful name for cameras

The camera names in settings are composed by display_name + model_id.
When there are front and back cameras, the user can not tell which is
which by the model ID. After this change, it shows
display_name + model_id + facing_info

BUG=chrome-os-partner:62956
TEST=Verify that display_name + model_id + facing_info shows in settings

Review-Url: https://codereview.chromium.org/2703393007
Cr-Commit-Position: refs/heads/master@{#454531}
Committed:
https://chromium.googlesource.com/chromium/src/+/75a64969dd5f72c9ff2fff22fd36...
==========

[commit-bot: I haz the power, 2017-03-03 06:38:06]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/75a64969dd5f72c9ff2fff22fd36...