|
|
Created:
3 years, 6 months ago by hlundin-chromium Modified:
3 years, 6 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionWebrtcAudioPrivateSetAudioExperimentsFunction dummy implementation
This change introduces a dummy implementation of
WebrtcAudioPrivateSetAudioExperimentsFunction::RunAsync() which is
invoked when Chrome is build without support for WebRTC
(enable_webrtc=false).
BUG=730019, 708475
Review-Url: https://codereview.chromium.org/2949563002
Cr-Commit-Position: refs/heads/master@{#481532}
Committed: https://chromium.googlesource.com/chromium/src/+/66e6a51773c74189058e55c8b7e63353b4f469f8
Patch Set 1 #
Total comments: 3
Patch Set 2 : Set an error #Messages
Total messages: 16 (6 generated)
Description was changed from ========== WebrtcAudioPrivateSetAudioExperimentsFunction dummy implementation This change introduces a dummy implementation of WebrtcAudioPrivateSetAudioExperimentsFunction::RunAsync() which is invoked when Chrome is build without support for WebRTC (enable_webrtc=false). BUG=730019,708475 ========== to ========== WebrtcAudioPrivateSetAudioExperimentsFunction dummy implementation This change introduces a dummy implementation of WebrtcAudioPrivateSetAudioExperimentsFunction::RunAsync() which is invoked when Chrome is build without support for WebRTC (enable_webrtc=false). BUG=730019,708475 ==========
hlundin@chromium.org changed reviewers: + grunell@chromium.org, rdevlin.cronin@chromium.org
Henrik, Devlin, PTAL at this change. Also, the chrome/browser/extensions/api/webrtc_audio_private/ folder does not have an OWNERS file. Many (most?) of the other extensions/api folders have it. Would you like me to add an OWNERS file? Thanks!
On 2017/06/19 12:38:32, hlundin-chromium wrote: > Henrik, Devlin, > > PTAL at this change. > > Also, the chrome/browser/extensions/api/webrtc_audio_private/ folder does not > have an OWNERS file. Many (most?) of the other extensions/api folders have it. > Would you like me to add an OWNERS file? > > Thanks! I think an OWNERS file makes sense, but it's not that big of a deal. Interesting that no other of the webrtc_* extension API implementations need that build flag. None of these APIs are relevant if webrtc is disabled, so maybe long term they shouldn't be available at all? Wdyt Devlin? Short-term: lgtm
On 2017/06/19 20:21:08, Henrik Grunell wrote: > On 2017/06/19 12:38:32, hlundin-chromium wrote: > > Henrik, Devlin, > > > > PTAL at this change. > > > > Also, the chrome/browser/extensions/api/webrtc_audio_private/ folder does not > > have an OWNERS file. Many (most?) of the other extensions/api folders have it. > > Would you like me to add an OWNERS file? > > > > Thanks! > > I think an OWNERS file makes sense, but it's not that big of a deal. > > Interesting that no other of the webrtc_* extension API implementations need > that build flag. None of these APIs are relevant if webrtc is disabled, so maybe > long term they shouldn't be available at all? Wdyt Devlin? > > Short-term: lgtm webrtc_logging_private_api.cc switches on build level to an alternative source file with only stubs: https://cs.chromium.org/chromium/src/chrome/browser/extensions/BUILD.gn?type=.... I thought about this approach, but since all but one of the methods in webrtc_audio_private_api.cc seemed to work fine without WebRTC support, I did not go down that route.
On 2017/06/20 06:52:32, hlundin-chromium wrote: > On 2017/06/19 20:21:08, Henrik Grunell wrote: > > On 2017/06/19 12:38:32, hlundin-chromium wrote: > > > Henrik, Devlin, > > > > > > PTAL at this change. > > > > > > Also, the chrome/browser/extensions/api/webrtc_audio_private/ folder does > not > > > have an OWNERS file. Many (most?) of the other extensions/api folders have > it. > > > Would you like me to add an OWNERS file? > > > > > > Thanks! > > > > I think an OWNERS file makes sense, but it's not that big of a deal. > > > > Interesting that no other of the webrtc_* extension API implementations need > > that build flag. None of these APIs are relevant if webrtc is disabled, so > maybe > > long term they shouldn't be available at all? Wdyt Devlin? > > > > Short-term: lgtm > > webrtc_logging_private_api.cc switches on build level to an alternative source > file with only stubs: > https://cs.chromium.org/chromium/src/chrome/browser/extensions/BUILD.gn?type=.... > > I thought about this approach, but since all but one of the methods in > webrtc_audio_private_api.cc seemed to work > fine without WebRTC support, I did not go down that route. Ah, I see. I'm fine with that.
https://codereview.chromium.org/2949563002/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.cc (right): https://codereview.chromium.org/2949563002/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.cc:325: SendResponse(false); Should we set an error?
https://codereview.chromium.org/2949563002/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.cc (right): https://codereview.chromium.org/2949563002/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.cc:325: SendResponse(false); On 2017/06/20 17:24:34, Devlin wrote: > Should we set an error? Good point. Yes. Do SetError("Not supported"); as in the stub https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/webrtc_log...
PTAL again. Thanks! https://codereview.chromium.org/2949563002/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.cc (right): https://codereview.chromium.org/2949563002/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.cc:325: SendResponse(false); On 2017/06/21 14:02:50, Henrik Grunell OOO back Jun 26 wrote: > On 2017/06/20 17:24:34, Devlin wrote: > > Should we set an error? > > Good point. Yes. Do > > SetError("Not supported"); > > as in the stub > https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/webrtc_log... Done.
lgtm
The CQ bit was checked by hlundin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from grunell@chromium.org Link to the patchset: https://codereview.chromium.org/2949563002/#ps20001 (title: "Set an error")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 20001, "attempt_start_ts": 1498139731201580, "parent_rev": "e0dbd7d9c9f8280d8be5e76d7c568beacd89333a", "commit_rev": "66e6a51773c74189058e55c8b7e63353b4f469f8"}
Message was sent while issue was closed.
Description was changed from ========== WebrtcAudioPrivateSetAudioExperimentsFunction dummy implementation This change introduces a dummy implementation of WebrtcAudioPrivateSetAudioExperimentsFunction::RunAsync() which is invoked when Chrome is build without support for WebRTC (enable_webrtc=false). BUG=730019,708475 ========== to ========== WebrtcAudioPrivateSetAudioExperimentsFunction dummy implementation This change introduces a dummy implementation of WebrtcAudioPrivateSetAudioExperimentsFunction::RunAsync() which is invoked when Chrome is build without support for WebRTC (enable_webrtc=false). BUG=730019,708475 Review-Url: https://codereview.chromium.org/2949563002 Cr-Commit-Position: refs/heads/master@{#481532} Committed: https://chromium.googlesource.com/chromium/src/+/66e6a51773c74189058e55c8b7e6... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/66e6a51773c74189058e55c8b7e6... |