Add flag in device enumeration request to control label clearing and don't clear for pepper.

Add flag in device enumeration request to control label clearing and don't clear for pepper.

The crash in the bug seems to be triggered by empty labels. This should avoid the crash if that's the case.

R=dmichael@chromium.org, kenrb@chromium.org
TBR=xians@chromium.com

BUG=383074
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278184

[Henrik Grunell, 2014-06-11 11:18:38]

Sorry, forgot the review division:

kenrb: content/common/media/media_stream_messages.h
dmichael: content/renderer/pepper/pepper_media_device_manager.cc
tommi: the rest

[tommi (sloooow) - chröme, 2014-06-11 12:45:15]

It feels a bit strange to me to have a flag in the renderer process that says
whether or not the labels will be visible or not.  Shouldn't that control be
fully in the browser process?

[Henrik Grunell, 2014-06-11 12:55:24]

On 2014/06/11 12:45:15, tommi wrote:
> It feels a bit strange to me to have a flag in the renderer process that says
> whether or not the labels will be visible or not.  Shouldn't that control be
> fully in the browser process?

I agree. I wanted to quickly revert to the previous behavior for pepper to avoid
the bug 383074 which seems to happen due to the change. The best would of course
to fix the bug itself, but that's for others to do.

[kenrb, 2014-06-11 13:19:43]

I agree with the concern that tommi raises, but lgtm for the IPC change. It is
okay if this is temporary.

[tommi (sloooow) - chröme, 2014-06-11 17:11:45]

On 2014/06/11 12:55:24, Henrik Grunell wrote:
> On 2014/06/11 12:45:15, tommi wrote:
> > It feels a bit strange to me to have a flag in the renderer process that
says
> > whether or not the labels will be visible or not.  Shouldn't that control be
> > fully in the browser process?
> 
> I agree. I wanted to quickly revert to the previous behavior for pepper to
avoid
> the bug 383074 which seems to happen due to the change. The best would of
course
> to fix the bug itself, but that's for others to do.

OK, so do you want to land the CL as is, file a bug for moving the flag out from
the renderer process and tackle that next?

[dmichael (off chromium), 2014-06-11 17:22:07]

pepper lgtm, but agreed that browser-side control would be better.

https://codereview.chromium.org/323313003/diff/1/content/renderer/pepper/pepp...
File content/renderer/pepper/pepper_media_device_manager.cc (right):

https://codereview.chromium.org/323313003/diff/1/content/renderer/pepper/pepp...
content/renderer/pepper/pepper_media_device_manager.cc:58: false);
I tend to prefer enum parameters to make code self-documenting.

In absence of that, please add a comment here to explain what false means.:
false /* hide_labels_if_no_access */);

[Henrik Grunell, 2014-06-11 19:01:54]

On 2014/06/11 17:11:45, tommi wrote:
> On 2014/06/11 12:55:24, Henrik Grunell wrote:
> > On 2014/06/11 12:45:15, tommi wrote:
> > > It feels a bit strange to me to have a flag in the renderer process that
> says
> > > whether or not the labels will be visible or not.  Shouldn't that control
be
> > > fully in the browser process?
> > 
> > I agree. I wanted to quickly revert to the previous behavior for pepper to
> avoid
> > the bug 383074 which seems to happen due to the change. The best would of
> course
> > to fix the bug itself, but that's for others to do.
> 
> OK, so do you want to land the CL as is, file a bug for moving the flag out
from
> the renderer process and tackle that next?

Yes, what I would like is:
- Land this CL.
- Someone fixes bug 383074.
- Pepper team gives OK for hiding labels if no access is granted.
- Revert this CL.

[Henrik Grunell, 2014-06-18 22:14:00]

Committed patchset #2 manually as r278184 (presubmit successful).

[Henrik Grunell, 2014-06-18 22:24:58]

Shijing, please review this. After chatting with amineer@ I decided to commit
this with tbr.

The plan is to
1. See if this avoids the crash. (If no, revert it. No more action.)
2. If yes, decide on a label privacy story for pepper. Empty or not?
3. Implement a proper handling based on that decision.

[no longer working on chromium, 2014-06-24 13:45:18]

On 2014/06/18 22:24:58, Henrik Grunell wrote:
> Shijing, please review this. After chatting with amineer@ I decided to commit
> this with tbr.
> 
> The plan is to
> 1. See if this avoids the crash. (If no, revert it. No more action.)
> 2. If yes, decide on a label privacy story for pepper. Empty or not?
> 3. Implement a proper handling based on that decision.

lgtm