Removed the use of WebFrame::frameForCurrentContext() in MediaStreamCenter::didStopLocalMediaStream

Removed the use of WebFrame::frameForCurrentContext() in MediaStreamCenter::didStopLocalMediaStream

BUG=150755
TEST= Goto https://apprtc.appspot.com/?r=&debug=loopback and let the page use camera and microphone. Hit ctrl-shift-j to open up the java script console. type localMediaStream.stop() to stop the local mediasream. Make sure the camera is no longer used.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=158004

[perkj_chrome, 2012-09-19 12:54:10]

No hurry. Whenever you have time?

[tommi (sloooow) - chröme, 2012-09-21 09:18:03]

http://codereview.chromium.org/10947030/diff/2001/content/renderer/media/medi...
File content/renderer/media/media_stream_center.cc (right):

http://codereview.chromium.org/10947030/diff/2001/content/renderer/media/medi...
content/renderer/media/media_stream_center.cc:112: return
extra_data->RunStreamStopCallback();
no need for return statements in void functions

http://codereview.chromium.org/10947030/diff/2001/content/renderer/media/medi...
File content/renderer/media/media_stream_extra_data.h (right):

http://codereview.chromium.org/10947030/diff/2001/content/renderer/media/medi...
content/renderer/media/media_stream_extra_data.h:36: void
RunStreamStopCallback();
should this be public?

If it does, then it feels to me that the callback is being stored in the wrong
place.  Can it instead be stored with the class that needs to call
RunStreamStopCallback?

http://codereview.chromium.org/10947030/diff/2001/content/renderer/media/medi...
File content/renderer/media/media_stream_impl.cc (right):

http://codereview.chromium.org/10947030/diff/2001/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:201: &description,
base::Bind(&MediaStreamImpl::OnLocalMediaStreamStop,
indent

http://codereview.chromium.org/10947030/diff/2001/content/renderer/media/medi...
File content/renderer/media/media_stream_impl_unittest.cc (right):

http://codereview.chromium.org/10947030/diff/2001/content/renderer/media/medi...
content/renderer/media/media_stream_impl_unittest.cc:44: using
MediaStreamImpl::OnLocalMediaStreamStop;
empty line above.

Is this really needed though?  This class inherits from MediaStreamImpl, right? 
(I'm sure there's something I'm missing here)

[perkj_chrome, 2012-09-21 10:47:10]

PTAL

http://codereview.chromium.org/10947030/diff/2001/content/renderer/media/medi...
File content/renderer/media/media_stream_center.cc (right):

http://codereview.chromium.org/10947030/diff/2001/content/renderer/media/medi...
content/renderer/media/media_stream_center.cc:112: return
extra_data->RunStreamStopCallback();
On 2012/09/21 09:18:03, tommi wrote:
> no need for return statements in void functions

Done.

http://codereview.chromium.org/10947030/diff/2001/content/renderer/media/medi...
File content/renderer/media/media_stream_extra_data.h (right):

http://codereview.chromium.org/10947030/diff/2001/content/renderer/media/medi...
content/renderer/media/media_stream_extra_data.h:36: void
RunStreamStopCallback();
ok- changed this into a just passing around data. 
On 2012/09/21 09:18:03, tommi wrote:
> should this be public?
> 
> If it does, then it feels to me that the callback is being stored in the wrong
> place.  Can it instead be stored with the class that needs to call
> RunStreamStopCallback?

http://codereview.chromium.org/10947030/diff/2001/content/renderer/media/medi...
File content/renderer/media/media_stream_impl.cc (right):

http://codereview.chromium.org/10947030/diff/2001/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:201: &description,
base::Bind(&MediaStreamImpl::OnLocalMediaStreamStop,
On 2012/09/21 09:18:03, tommi wrote:
> indent

? What is wrong with this?

http://codereview.chromium.org/10947030/diff/2001/content/renderer/media/medi...
File content/renderer/media/media_stream_impl_unittest.cc (right):

http://codereview.chromium.org/10947030/diff/2001/content/renderer/media/medi...
content/renderer/media/media_stream_impl_unittest.cc:44: using
MediaStreamImpl::OnLocalMediaStreamStop;
Yes- unless I change OnLocalMediaStream to be public. 
On 2012/09/21 09:18:03, tommi wrote:
> empty line above.
> 
> Is this really needed though?  This class inherits from MediaStreamImpl,
right? 
> (I'm sure there's something I'm missing here)

[tommi (sloooow) - chröme, 2012-09-21 11:23:53]

http://codereview.chromium.org/10947030/diff/2001/content/renderer/media/medi...
File content/renderer/media/media_stream_impl.cc (right):

http://codereview.chromium.org/10947030/diff/2001/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:201: &description,
base::Bind(&MediaStreamImpl::OnLocalMediaStreamStop,
On 2012/09/21 10:47:10, perkj wrote:
> On 2012/09/21 09:18:03, tommi wrote:
> > indent
> 
> ? What is wrong with this?

MyFunction(
bar, baz);

should in Chrome be

MyFunction(
    bar, baz);

Also, since that uses 4 space indent, then the indent of the nested base::Bind()
call should also do that.  I.e. avoid mixing indent styles.

http://codereview.chromium.org/10947030/diff/2001/content/renderer/media/medi...
File content/renderer/media/media_stream_impl_unittest.cc (right):

http://codereview.chromium.org/10947030/diff/2001/content/renderer/media/medi...
content/renderer/media/media_stream_impl_unittest.cc:44: using
MediaStreamImpl::OnLocalMediaStreamStop;
On 2012/09/21 10:47:10, perkj wrote:
> Yes- unless I change OnLocalMediaStream to be public. 
> On 2012/09/21 09:18:03, tommi wrote:
> > empty line above.
> > 
> > Is this really needed though?  This class inherits from MediaStreamImpl,
> right? 
> > (I'm sure there's something I'm missing here)
> 

Ah, it's not public in the parent class then.  OK, can you add a comment that
explains why this is here then?

http://codereview.chromium.org/10947030/diff/10001/content/renderer/media/med...
File content/renderer/media/media_stream_center.cc (right):

http://codereview.chromium.org/10947030/diff/10001/content/renderer/media/med...
content/renderer/media/media_stream_center.cc:116: if
(!extra_data->stream_stop_callback().is_null()) {
empty line above and no {} for single line if's.

http://codereview.chromium.org/10947030/diff/10001/content/renderer/media/med...
File content/renderer/media/media_stream_extra_data.h (right):

http://codereview.chromium.org/10947030/diff/10001/content/renderer/media/med...
content/renderer/media/media_stream_extra_data.h:32: const StreamStopCallback&
stream_stop_callback() {
Sorry, but this is more confusing.
Let's go back to the other approach you had but instead of naming the function
RunLocalStreamStopCallback, call it something that indicates that an event has
occurred like OnLocalStreamStopped().
The difference is that the getter approach and the 'Run' method, both expose an
implementation detail of this class.  An event on the other hand does not
promise anything about how the event will be handled so the implementation is
opaque as far as the caller is concerned.

[perkj_chrome, 2012-09-21 11:56:52]

ok- lets try again.

http://codereview.chromium.org/10947030/diff/10001/content/renderer/media/med...
File content/renderer/media/media_stream_center.cc (right):

http://codereview.chromium.org/10947030/diff/10001/content/renderer/media/med...
content/renderer/media/media_stream_center.cc:116: if
(!extra_data->stream_stop_callback().is_null()) {
On 2012/09/21 11:23:53, tommi wrote:
> empty line above and no {} for single line if's.

Done.

http://codereview.chromium.org/10947030/diff/10001/content/renderer/media/med...
File content/renderer/media/media_stream_extra_data.h (right):

http://codereview.chromium.org/10947030/diff/10001/content/renderer/media/med...
content/renderer/media/media_stream_extra_data.h:32: const StreamStopCallback&
stream_stop_callback() {
On 2012/09/21 11:23:53, tommi wrote:
> Sorry, but this is more confusing.
> Let's go back to the other approach you had but instead of naming the function
> RunLocalStreamStopCallback, call it something that indicates that an event has
> occurred like OnLocalStreamStopped().
> The difference is that the getter approach and the 'Run' method, both expose
an
> implementation detail of this class.  An event on the other hand does not
> promise anything about how the event will be handled so the implementation is
> opaque as far as the caller is concerned.

Done.

[tommi (sloooow) - chröme, 2012-09-21 11:58:56]

lgtm! thanks

[commit-bot: I haz the power, 2012-09-21 13:14:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/perkj@chromium.org/10947030/8002

[commit-bot: I haz the power, 2012-09-21 16:46:14]

Change committed as 158004