Replace MediaStreamUIController with MediaStreamUIProxy.

Replace MediaStreamUIController with MediaStreamUIProxy.

Previously a single object MediaStreamUIController was used to control UI for
all streams. Replaced it with a per-stream MediaStreamUIProxy that simplifies
code in many places.
Also moved media request queueing logic from content layer to chrome. Now
different types of requests may be queued differently (e.g. there is no
reason to block screen capture requests on webcam infobar).

This change was previously landed in 197222 and reverted in 197242

TBR=tommi@chromium.org

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

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

[Sergey Ulanov, 2013-06-04 02:45:17]

I still don't know what in this CL causes the tests on Mac Lion builds bots to
fail - I was never able to reproduce it locally or on try bots.
I suspect it may have to do with how fake devices are enumerated, so I've
changed FakeMediaStreamUIProxy::RequestAccess() to use the same logic that was
used previously in MediaStreamController. Patch Set 1 contains original change.
Patch Set 2 - the same change with the old fake device enumeration logic.

[Sergey Ulanov, 2013-06-04 17:44:24]

+vrk@, while Tommi is on OOO.
Victoria, this CL has been already reviewed at
https://codereview.chromium.org/13989003 but it was reverted because it caused
test failures on Mac Lion for unknown reason. Please review the diff between
patchset 1 and patchset 4.

[vrk (LEFT CHROMIUM), 2013-06-04 20:57:09]

lgtm

[Sergey Ulanov, 2013-06-04 21:07:23]

Committed patchset #4 manually as r204044 (presubmit successful).

[Sergey Ulanov, 2013-06-05 21:15:41]

This change was reverted because it caused failures in webrtc pyauto tests. In
Patch Set 5 I fixed the problem. There were two bugs: 
 1. MediaStreamUIProxy didn't handle the case when it's destroyed before the
request is processed - fixed it by using WeakPtr<> when creating callback for
RequestMediaAccessPermission().
 2. MediaCaptureDevicesDispatcher::OnAccessRequestResponse() now processes
queued requests asynchronously to make sure calling infobar is not destroyed
while it's being accepted/cancelled.

Also added unittests for MediaStreamUIProxy.
Victoria, PTAL.

[Sergey Ulanov, 2013-06-06 19:29:41]

+miu@ 
Yuri, since you are familiar with this code, can you please review the diff
between patch set 4 and patch set 5.

[miu, 2013-06-06 20:03:01]

https://codereview.chromium.org/16342002/diff/70001/content/browser/renderer_...
File content/browser/renderer_host/media/media_stream_ui_proxy.cc (right):

https://codereview.chromium.org/16342002/diff/70001/content/browser/renderer_...
content/browser/renderer_host/media/media_stream_ui_proxy.cc:66:
render_delegate_ = host->GetDelegate();
In this "set once lazily" scheme, will render_delegate_ point to the correct
instance for future method calls?  If, in future method calls, the request has a
different render_process_id or render_view_id, could the delegate be a different
instance?  It seems the method should always evaluate
RenderViewHostImpl::FromID(...)->GetDelegate().

https://codereview.chromium.org/16342002/diff/70001/content/browser/renderer_...
content/browser/renderer_host/media/media_stream_ui_proxy.cc:118: void
MediaStreamUIProxy::SetRenderViewHostDelegateForTests(
Consider using injection (i.e., argument to the constructor of this class)
instead of a separate method to force a test delegate instance.

https://codereview.chromium.org/16342002/diff/70001/content/browser/renderer_...
File content/browser/renderer_host/media/media_stream_ui_proxy_unittest.cc
(right):

https://codereview.chromium.org/16342002/diff/70001/content/browser/renderer_...
content/browser/renderer_host/media/media_stream_ui_proxy_unittest.cc:63:
proxy_.reset(new MediaStreamUIProxy());
Yeah, just use injection (suggested in other comment):

  proxy_.reset(new MediaStreamUIProxy(&delegate_));

https://codereview.chromium.org/16342002/diff/70001/content/browser/renderer_...
content/browser/renderer_host/media/media_stream_ui_proxy_unittest.cc:83: return
Can this just be:

  return expected == arg;

Since MediaStreamRequest is a struct, C++ should generate a default operator==()
for you.  The risk in doing it yourself is that this code could become
out-of-sync with that in media_stream_request.h.

[Sergey Ulanov, 2013-06-06 21:40:03]

https://codereview.chromium.org/16342002/diff/70001/content/browser/renderer_...
File content/browser/renderer_host/media/media_stream_ui_proxy.cc (right):

https://codereview.chromium.org/16342002/diff/70001/content/browser/renderer_...
content/browser/renderer_host/media/media_stream_ui_proxy.cc:66:
render_delegate_ = host->GetDelegate();
On 2013/06/06 20:03:01, Yuri wrote:
> In this "set once lazily" scheme, will render_delegate_ point to the correct
> instance for future method calls?  If, in future method calls, the request has
a
> different render_process_id or render_view_id, could the delegate be a
different
> instance?  It seems the method should always evaluate
> RenderViewHostImpl::FromID(...)->GetDelegate().

This method should only be called only once per instance of the class (new
instance is created for each request), so it doesn't matter. Anyway, I fixed
this method to never set render_delegate_.

https://codereview.chromium.org/16342002/diff/70001/content/browser/renderer_...
content/browser/renderer_host/media/media_stream_ui_proxy.cc:118: void
MediaStreamUIProxy::SetRenderViewHostDelegateForTests(
On 2013/06/06 20:03:01, Yuri wrote:
> Consider using injection (i.e., argument to the constructor of this class)
> instead of a separate method to force a test delegate instance.

Done.

https://codereview.chromium.org/16342002/diff/70001/content/browser/renderer_...
File content/browser/renderer_host/media/media_stream_ui_proxy_unittest.cc
(right):

https://codereview.chromium.org/16342002/diff/70001/content/browser/renderer_...
content/browser/renderer_host/media/media_stream_ui_proxy_unittest.cc:63:
proxy_.reset(new MediaStreamUIProxy());
On 2013/06/06 20:03:01, Yuri wrote:
> Yeah, just use injection (suggested in other comment):
> 
>   proxy_.reset(new MediaStreamUIProxy(&delegate_));

Done.

https://codereview.chromium.org/16342002/diff/70001/content/browser/renderer_...
content/browser/renderer_host/media/media_stream_ui_proxy_unittest.cc:83: return
On 2013/06/06 20:03:01, Yuri wrote:
> Can this just be:
> 
>   return expected == arg;
> 
> Since MediaStreamRequest is a struct, C++ should generate a default
operator==()
> for you.  The risk in doing it yourself is that this code could become
> out-of-sync with that in media_stream_request.h.

Nope, there is no implicit comparison operator for structs, I would have to
declare it explicitly (e.g. see
http://stackoverflow.com/questions/5740310/no-operator-found-while-comparing-...)

[miu, 2013-06-06 23:05:23]

lgtm

https://codereview.chromium.org/16342002/diff/70001/content/browser/renderer_...
File content/browser/renderer_host/media/media_stream_ui_proxy_unittest.cc
(right):

https://codereview.chromium.org/16342002/diff/70001/content/browser/renderer_...
content/browser/renderer_host/media/media_stream_ui_proxy_unittest.cc:83: return
On 2013/06/06 21:40:03, Sergey Ulanov wrote:
> On 2013/06/06 20:03:01, Yuri wrote:
> > Can this just be:
> > 
> >   return expected == arg;
> > 
> > Since MediaStreamRequest is a struct, C++ should generate a default
> operator==()
> > for you.  The risk in doing it yourself is that this code could become
> > out-of-sync with that in media_stream_request.h.
> 
> Nope, there is no implicit comparison operator for structs, I would have to
> declare it explicitly (e.g. see
>
http://stackoverflow.com/questions/5740310/no-operator-found-while-comparing-...)

Ah, my mistake.  Not sure why I keep thinking C++ provides it.  Maybe because it
provides operator=() and a copy ctor by default...

I'll leave it up to you, but it would probably be safer to implement this code
as an operator==() method in the MediaStreamRequest struct in
media_stream_request.h.  Someone down the line might change the struct without
changing your code here.

https://codereview.chromium.org/16342002/diff/83001/content/browser/renderer_...
File content/browser/renderer_host/media/media_stream_ui_proxy.cc (right):

https://codereview.chromium.org/16342002/diff/83001/content/browser/renderer_...
content/browser/renderer_host/media/media_stream_ui_proxy.cc:31:
RenderViewHostDelegate* render_delegate_;
nit: Here too: Rename to test_render_delegate_ for clarity, and use type
"RenderViewHostDelegate* const" since the member is not changed in any methods.

https://codereview.chromium.org/16342002/diff/83001/content/browser/renderer_...
File content/browser/renderer_host/media/media_stream_ui_proxy.h (right):

https://codereview.chromium.org/16342002/diff/83001/content/browser/renderer_...
content/browser/renderer_host/media/media_stream_ui_proxy.h:58:
RenderViewHostDelegate* renderer_delegate_;
nit: Rename to test_render_delegate_ for clarity, and use type
"RenderViewHostDelegate* const" since the member is not changed in any methods.

[commit-bot: I haz the power, 2013-06-06 23:15:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/16342002/21

[commit-bot: I haz the power, 2013-06-07 00:02:30]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_clang_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[Sergey Ulanov, 2013-06-07 00:37:09]

https://codereview.chromium.org/16342002/diff/70001/content/browser/renderer_...
File content/browser/renderer_host/media/media_stream_ui_proxy_unittest.cc
(right):

https://codereview.chromium.org/16342002/diff/70001/content/browser/renderer_...
content/browser/renderer_host/media/media_stream_ui_proxy_unittest.cc:83: return
On 2013/06/06 23:05:23, Yuri wrote:
> On 2013/06/06 21:40:03, Sergey Ulanov wrote:
> > On 2013/06/06 20:03:01, Yuri wrote:
> > > Can this just be:
> > > 
> > >   return expected == arg;
> > > 
> > > Since MediaStreamRequest is a struct, C++ should generate a default
> > operator==()
> > > for you.  The risk in doing it yourself is that this code could become
> > > out-of-sync with that in media_stream_request.h.
> > 
> > Nope, there is no implicit comparison operator for structs, I would have to
> > declare it explicitly (e.g. see
> >
>
http://stackoverflow.com/questions/5740310/no-operator-found-while-comparing-...)
> 
> Ah, my mistake.  Not sure why I keep thinking C++ provides it.  Maybe because
it
> provides operator=() and a copy ctor by default...
> 
> I'll leave it up to you, but it would probably be safer to implement this code
> as an operator==() method in the MediaStreamRequest struct in
> media_stream_request.h.  Someone down the line might change the struct without
> changing your code here.

I'd rather leave it as is - code style discourages operator overloading. I could
add Equals() method, but this unittest would be the only place that needs it,
and it isn't crucial for it to compare all members.

https://codereview.chromium.org/16342002/diff/83001/content/browser/renderer_...
File content/browser/renderer_host/media/media_stream_ui_proxy.cc (right):

https://codereview.chromium.org/16342002/diff/83001/content/browser/renderer_...
content/browser/renderer_host/media/media_stream_ui_proxy.cc:31:
RenderViewHostDelegate* render_delegate_;
On 2013/06/06 23:05:23, Yuri wrote:
> nit: Here too: Rename to test_render_delegate_ for clarity, and use type
> "RenderViewHostDelegate* const" since the member is not changed in any
methods.

Done.

https://codereview.chromium.org/16342002/diff/83001/content/browser/renderer_...
File content/browser/renderer_host/media/media_stream_ui_proxy.h (right):

https://codereview.chromium.org/16342002/diff/83001/content/browser/renderer_...
content/browser/renderer_host/media/media_stream_ui_proxy.h:58:
RenderViewHostDelegate* renderer_delegate_;
On 2013/06/06 23:05:23, Yuri wrote:
> nit: Rename to test_render_delegate_ for clarity, and use type
> "RenderViewHostDelegate* const" since the member is not changed in any
methods.

removed this member - it wasn't used anyway.

[commit-bot: I haz the power, 2013-06-07 00:38:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/16342002/99002

[commit-bot: I haz the power, 2013-06-07 04:29:48]

Retried try job too often on win_rel for step(s) content_browsertests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-06-07 06:56:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/16342002/99002

[commit-bot: I haz the power, 2013-06-07 13:12:56]

Change committed as 204808