Use reference instead of pointer for media stream device request

Use reference instead of pointer for media stream device request
This is a simplified merge of r145282 into branch 1180.
Since the pointer will be out of scope when posting a task, need to use reference.
Also MediaStreamInfoBarDelegate needs to keep a copy of request, instead of a pointer.

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

[no longer working on chromium, 2012-07-11 21:59:12]

lgtm

[tommi (sloooow) - chröme, 2012-07-12 08:38:53]

https://chromiumcodereview.appspot.com/10692174/diff/1/content/browser/render...
File content/browser/renderer_host/media/media_stream_device_settings.cc
(right):

https://chromiumcodereview.appspot.com/10692174/diff/1/content/browser/render...
content/browser/renderer_host/media/media_stream_device_settings.cc:130:
host->GetDelegate()->RequestMediaAccessPermission(&request, callback);
Thanks for the quick fix - qq - should we also change
RequestMediaAccessPermission to take const& and not a pointer?  Just to reduce
the chance of implementors making incorrect assumptions?

[wjia(left Chromium), 2012-07-12 14:19:15]

On 2012/07/12 08:38:53, tommi wrote:
>
https://chromiumcodereview.appspot.com/10692174/diff/1/content/browser/render...
> File content/browser/renderer_host/media/media_stream_device_settings.cc
> (right):
> 
>
https://chromiumcodereview.appspot.com/10692174/diff/1/content/browser/render...
> content/browser/renderer_host/media/media_stream_device_settings.cc:130:
> host->GetDelegate()->RequestMediaAccessPermission(&request, callback);
> Thanks for the quick fix - qq - should we also change
> RequestMediaAccessPermission to take const& and not a pointer?  Just to reduce
> the chance of implementors making incorrect assumptions?

I have talked about that with xians@ when preparing the patch. Right now, the
caller doesn't pass NULL pointer to RequestMediaAccessPermission. Since this is
a merge directly into beta branch, a minimum patch is expected. Therefore, the
API of RequestMediaAccessPermission is not changed. Otherwise, more files such
as browser.cc has to be touched.

If it's really desired, we can make an additional patch for that.

[tommi (sloooow) - chröme, 2012-07-12 14:21:08]

Thanks.  Yeah, I was more thinking about trunk.  I think this is good as an
on-branch fix.


On Thu, Jul 12, 2012 at 4:19 PM, <wjia@chromium.org> wrote:

> Reviewers: xians1, tommi,
>
> Message:
>
> On 2012/07/12 08:38:53, tommi wrote:
>
> https://chromiumcodereview.**appspot.com/10692174/diff/1/**
>
content/browser/renderer_host/**media/media_stream_device_**settings.cc<https://chromiumcodereview.appspot.com/10692174/diff/1/content/browser/renderer_host/media/media_stream_device_settings.cc>
>
>> File content/browser/renderer_host/**media/media_stream_device_**
>> settings.cc
>> (right):
>>
>
>
> https://chromiumcodereview.**appspot.com/10692174/diff/1/**
> content/browser/renderer_host/**media/media_stream_device_**
>
settings.cc#newcode130<https://chromiumcodereview.appspot.com/10692174/diff/1/content/browser/renderer_host/media/media_stream_device_settings.cc#newcode130>
>
>> content/browser/renderer_host/**media/media_stream_device_**
>> settings.cc:130:
>> host->GetDelegate()->**RequestMediaAccessPermission(&**request,
>> callback);
>> Thanks for the quick fix - qq - should we also change
>> RequestMediaAccessPermission to take const& and not a pointer?  Just to
>> reduce
>> the chance of implementors making incorrect assumptions?
>>
>
> I have talked about that with xians@ when preparing the patch. Right now,
> the
> caller doesn't pass NULL pointer to RequestMediaAccessPermission. Since
> this is
> a merge directly into beta branch, a minimum patch is expected. Therefore,
> the
> API of RequestMediaAccessPermission is not changed. Otherwise, more files
> such
> as browser.cc has to be touched.
>
> If it's really desired, we can make an additional patch for that.
>
> Description:
> Use reference instead of pointer for media stream device request
> This is a simplified merge of r145282 into branch 1180.
> Since the pointer will be out of scope when posting a task, need to use
> reference.
> Also MediaStreamInfoBarDelegate needs to keep a copy of request, instead
> of a
> pointer.
>
> BUG=135043
> Committed: https://src.chromium.org/**viewvc/chrome?view=rev&**
>
revision=146225<https://src.chromium.org/viewvc/chrome?view=rev&revision=146225>
>
> Please review this at
https://chromiumcodereview.**appspot.com/10692174/<https://chromiumcodereview...
>
> SVN Base:
svn://svn.chromium.org/chrome/**branches/1180/src/<http://svn.chromium.org/chrome/branches/1180/src/>
>
> Affected files:
>   M     chrome/browser/ui/media_**stream_infobar_delegate.h
>   M     chrome/browser/ui/media_**stream_infobar_delegate.cc
>   M     content/browser/renderer_host/**media/media_stream_device_**
> settings.cc
>
>
> Index: chrome/browser/ui/media_**stream_infobar_delegate.cc
> ==============================**==============================**=======
> --- chrome/browser/ui/media_**stream_infobar_delegate.cc  (revision
> 146197)
> +++ chrome/browser/ui/media_**stream_infobar_delegate.cc  (working copy)
> @@ -37,12 +37,11 @@
>      const content::MediaStreamRequest* request,
>      const content::**MediaResponseCallback& callback)
>      : InfoBarDelegate(tab_helper),
> -      request_(request),
> +      request_(*request),
>        callback_(callback) {
> -  DCHECK(request_);
> -  has_audio_ = request_->devices.count(
> +  has_audio_ = request_.devices.count(
>        content::MEDIA_STREAM_DEVICE_**TYPE_AUDIO_CAPTURE) != 0;
> -  has_video_ = request_->devices.count(
> +  has_video_ = request_.devices.count(
>        content::MEDIA_STREAM_DEVICE_**TYPE_VIDEO_CAPTURE) != 0;
>  }
>
> @@ -54,8 +53,8 @@
>    if (!has_audio_)
>      return content::MediaStreamDevices();
>    content::MediaStreamDeviceMap:**:const_iterator it =
> -      request_->devices.find(**content::MEDIA_STREAM_DEVICE_**
> TYPE_AUDIO_CAPTURE);
> -  DCHECK(it != request_->devices.end());
> +      request_.devices.find(content:**:MEDIA_STREAM_DEVICE_TYPE_**
> AUDIO_CAPTURE);
> +  DCHECK(it != request_.devices.end());
>    return it->second;
>  }
>
> @@ -64,13 +63,13 @@
>    if (!has_video_)
>      return content::MediaStreamDevices();
>    content::MediaStreamDeviceMap:**:const_iterator it =
> -      request_->devices.find(**content::MEDIA_STREAM_DEVICE_**
> TYPE_VIDEO_CAPTURE);
> -  DCHECK(it != request_->devices.end());
> +      request_.devices.find(content:**:MEDIA_STREAM_DEVICE_TYPE_**
> VIDEO_CAPTURE);
> +  DCHECK(it != request_.devices.end());
>    return it->second;
>  }
>
>  const GURL& MediaStreamInfoBarDelegate::**GetSecurityOrigin() const {
> -  return request_->security_origin;
> +  return request_.security_origin;
>  }
>
>  void MediaStreamInfoBarDelegate::**Accept(const std::string& audio_id,
> @@ -99,8 +98,8 @@
>      content::MediaStreamDevices* devices) {
>    DCHECK(devices);
>    content::MediaStreamDeviceMap:**:const_iterator device_it =
> -      request_->devices.find(type);
> -  if (device_it != request_->devices.end()) {
> +      request_.devices.find(type);
> +  if (device_it != request_.devices.end()) {
>      content::MediaStreamDevices::**const_iterator it = std::find_if(
>          device_it->second.begin(), device_it->second.end(),
> DeviceIdEquals(id));
>      if (it != device_it->second.end())
> Index: chrome/browser/ui/media_**stream_infobar_delegate.h
> ==============================**==============================**=======
> --- chrome/browser/ui/media_**stream_infobar_delegate.h   (revision
> 146197)
> +++ chrome/browser/ui/media_**stream_infobar_delegate.h   (working copy)
> @@ -65,7 +65,7 @@
>    virtual MediaStreamInfoBarDelegate* AsMediaStreamInfoBarDelegate()
> OVERRIDE;
>
>    // The original request for access to devices.
> -  const content::MediaStreamRequest* request_;
> +  const content::MediaStreamRequest request_;
>
>    // The callback that needs to be Run to notify WebRTC of whether access
> to
>    // audio/video devices was granted or not.
> Index: content/browser/renderer_host/**media/media_stream_device_**
> settings.cc
> ==============================**==============================**=======
> --- content/browser/renderer_host/**media/media_stream_device_**settings.cc
> (revision 146197)
> +++ content/browser/renderer_host/**media/media_stream_device_**settings.cc
> (working copy)
> @@ -112,14 +112,14 @@
>
>  // Sends the request to the appropriate WebContents.
>  void DoDeviceRequest(
> -    const MediaStreamDeviceSettingsReque**st* request,
> +    const MediaStreamDeviceSettingsReque**st& request,
>      const content::**MediaResponseCallback& callback) {
>    DCHECK(BrowserThread::**CurrentlyOn(BrowserThread::UI)**);
>
>    // Send the permission request to the web contents.
>    content::RenderViewHostImpl* host =
> -      content::RenderViewHostImpl::**FromID(request->render_**process_id,
> -                                          request->render_view_id);
> +      content::RenderViewHostImpl::**FromID(request.render_process_**id,
> +                                          request.render_view_id);
>
>    // Tab may have gone away.
>    if (!host || !host->GetDelegate()) {
> @@ -127,7 +127,7 @@
>      return;
>    }
>
> -  host->GetDelegate()->**RequestMediaAccessPermission(**request,
> callback);
> +  host->GetDelegate()->**RequestMediaAccessPermission(&**request,
> callback);
>  }
>
>  }  // namespace
> @@ -166,7 +166,17 @@
>
>    SettingsRequests::iterator request_it = requests_.find(label);
>    if (request_it != requests_.end()) {
> +    // Proceed the next pending request for the same page.
>      MediaStreamDeviceSettingsReque**st* request = request_it->second;
> +    std::string new_label = FindReadyRequestForView(**
> request->render_view_id,
> +
>  request->render_process_id);
> +    if (!new_label.empty()) {
> +      PostRequestToUi(new_label);
> +    }
> +
> +    // TODO(xians): Post a cancel request on UI thread to dismiss the
> infobar
> +    // if request has been sent to the UI.
> +    // Remove the request from the queue.
>      requests_.erase(request_it);
>      delete request;
>    }
> @@ -252,6 +262,10 @@
>    DCHECK(BrowserThread::**CurrentlyOn(BrowserThread::IO)**);
>
>    SettingsRequests::iterator req = requests_.find(label);
> +  // Return if the request has been removed.
> +  if (req == requests_.end())
> +    return;
> +
>    DCHECK(req != requests_.end()) << "Invalid request label.";
>    DCHECK(requester_);
>    MediaStreamDeviceSettingsReque**st* request = req->second;
> @@ -356,7 +370,7 @@
>
>    BrowserThread::PostTask(
>        BrowserThread::UI, FROM_HERE,
> -      base::Bind(&DoDeviceRequest, request, callback));
> +      base::Bind(&DoDeviceRequest, *request, callback));
>  }
>
>  }  // namespace media_stream
>
>
>