Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
585263002:
Change two media request "invalid state" results. (Closed)
|
Created:
6 years, 3 months ago by Henrik Grunell Modified:
6 years, 3 months ago CC:
chromium-reviews, posciak+watch_chromium.org, feature-media-reviews_chromium.org, wjia+watch_chromium.org, mcasas+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionChange two media request "invalid state" results.
* Changed one to "permission denied" for screen capture.
* Changed one to "no hardware" for platform app.
BUG=416233
Committed: https://crrev.com/606c8414da4e0576ac5f0e3da3a82431bfd68ece
Cr-Commit-Position: refs/heads/master@{#296238}
Patch Set 1 #
Total comments: 7
Patch Set 2 : Code review. #Patch Set 3 : Rebase #
Total comments: 4
Patch Set 4 : Code review. #Messages
Total messages: 14 (2 generated)
grunell@chromium.org changed reviewers: + sergeyu@chromium.org, tommi@chromium.org
Let's see if you agree with these changes. :)
https://codereview.chromium.org/585263002/diff/1/chrome/browser/media/media_c... File chrome/browser/media/media_capture_devices_dispatcher.cc (right): https://codereview.chromium.org/585263002/diff/1/chrome/browser/media/media_c... chrome/browser/media/media_capture_devices_dispatcher.cc:634: // permission. or !screen_capture_enabled or !origin_is_secure? in either case, I think that 'permission denied' would be the appropriate error code (unless we add a specific one for screen capture). https://codereview.chromium.org/585263002/diff/1/chrome/browser/media/media_c... chrome/browser/media/media_capture_devices_dispatcher.cc:727: : content::MEDIA_DEVICE_INVALID_STATE; instead of invalid state, should it be permission denied if neither video nor audio is allowed? https://codereview.chromium.org/585263002/diff/1/chrome/browser/media/media_c... chrome/browser/media/media_capture_devices_dispatcher.cc:768: devices.empty() ? content::MEDIA_DEVICE_INVALID_STATE : did you mean to use |result| here?
https://codereview.chromium.org/585263002/diff/1/chrome/browser/media/media_c... File chrome/browser/media/media_capture_devices_dispatcher.cc (right): https://codereview.chromium.org/585263002/diff/1/chrome/browser/media/media_c... chrome/browser/media/media_capture_devices_dispatcher.cc:634: // permission. On 2014/09/20 14:45:23, tommi wrote: > or !screen_capture_enabled or !origin_is_secure? Ah, right. > in either case, I think that 'permission denied' would be the appropriate error > code (unless we add a specific one for screen capture). I'd prefer to have the permission denied for user action, though I do think we use it for non-user action cases in other places already. I changed it so that invalid state is kept for the cases you pointed out. Wdyt? I could add another denied result in a follow-up that's used for non-user action denial such as this case. https://codereview.chromium.org/585263002/diff/1/chrome/browser/media/media_c... chrome/browser/media/media_capture_devices_dispatcher.cc:727: : content::MEDIA_DEVICE_INVALID_STATE; On 2014/09/20 14:45:23, tommi wrote: > instead of invalid state, should it be permission denied if neither video nor > audio is allowed? Same reply as above, as the todo also says.
https://codereview.chromium.org/585263002/diff/40001/chrome/browser/media/med... File chrome/browser/media/media_capture_devices_dispatcher.cc (right): https://codereview.chromium.org/585263002/diff/40001/chrome/browser/media/med... chrome/browser/media/media_capture_devices_dispatcher.cc:732: content::MediaStreamRequestResult result = I don't see this used anywhere below. Did you want to pass |result| when calling the |callback|? https://codereview.chromium.org/585263002/diff/40001/chrome/browser/media/med... chrome/browser/media/media_capture_devices_dispatcher.cc:733: audio_allowed || video_allowed ? content::MEDIA_DEVICE_NO_HARDWARE add parentheses around audio_allowed || video_allowed for readability
https://codereview.chromium.org/585263002/diff/40001/chrome/browser/media/med... File chrome/browser/media/media_capture_devices_dispatcher.cc (right): https://codereview.chromium.org/585263002/diff/40001/chrome/browser/media/med... chrome/browser/media/media_capture_devices_dispatcher.cc:732: content::MediaStreamRequestResult result = On 2014/09/22 17:19:22, Sergey Ulanov wrote: > I don't see this used anywhere below. Did you want to pass |result| when calling > the |callback|? Haha, thanks for catching. Yes. Fixed. https://codereview.chromium.org/585263002/diff/40001/chrome/browser/media/med... chrome/browser/media/media_capture_devices_dispatcher.cc:733: audio_allowed || video_allowed ? content::MEDIA_DEVICE_NO_HARDWARE On 2014/09/22 17:19:22, Sergey Ulanov wrote: > add parentheses around audio_allowed || video_allowed for readability Done.
https://codereview.chromium.org/585263002/diff/1/chrome/browser/media/media_c... File chrome/browser/media/media_capture_devices_dispatcher.cc (right): https://codereview.chromium.org/585263002/diff/1/chrome/browser/media/media_c... chrome/browser/media/media_capture_devices_dispatcher.cc:768: devices.empty() ? content::MEDIA_DEVICE_INVALID_STATE : On 2014/09/20 14:45:23, tommi wrote: > did you mean to use |result| here? I guess you missed this comment?
https://codereview.chromium.org/585263002/diff/1/chrome/browser/media/media_c... File chrome/browser/media/media_capture_devices_dispatcher.cc (right): https://codereview.chromium.org/585263002/diff/1/chrome/browser/media/media_c... chrome/browser/media/media_capture_devices_dispatcher.cc:768: devices.empty() ? content::MEDIA_DEVICE_INVALID_STATE : On 2014/09/22 20:15:02, tommi wrote: > On 2014/09/20 14:45:23, tommi wrote: > > did you mean to use |result| here? > > I guess you missed this comment? Yep. It's fixed now.
Ping Tommi.
The CQ bit was checked by tommi@chromium.org
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/585263002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as 4a989b780c7dfdfa8edf9887559071595e2f5539
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/606c8414da4e0576ac5f0e3da3a82431bfd68ece Cr-Commit-Position: refs/heads/master@{#296238} |
