Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1912853004:
Media Recorder: Enable content browser test on Android. (Closed)
|
Created:
4 years, 8 months ago by cpaulin (no longer in chrome) Modified:
4 years, 7 months ago CC:
chromium-reviews, jam, tnakamura+watch_chromium.org, phoglund+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMedia Recorder: Enable content browser test on Android.
This CL enables MediaStream Recorder content browser test on
Android devices. No new tests were added.
BUG=605798, 561068
TEST=On trybots
Committed: https://crrev.com/ff0ea18cf90aa0af082e4749a4b4a75acb5fa330
Cr-Commit-Position: refs/heads/master@{#390767}
Patch Set 1 : Enable test on android with one exception. #
Total comments: 6
Patch Set 2 : Addressed mcasas@'s comments. #
Total comments: 2
Patch Set 3 : Addressed nit from phoglund@. #Messages
Total messages: 16 (7 generated)
Description was changed from ========== Media Recorder: Enable content browser test on Android. This CL enables MediaStream Recorder content browser test on Android devices. No new tests were added. BUG=605798 TEST=On trybots ========== to ========== Media Recorder: Enable content browser test on Android. This CL enables MediaStream Recorder content browser test on Android devices. No new tests were added. BUG=605798, 561068 TEST=On trybots ==========
Patchset #1 (id:1) has been deleted
cpaulin@chromium.org changed reviewers: + mcasas@chromium.org, phoglund@chromium.org
Hi, This CL enables Media Recorder content browser tests on Android, all tests are enabled except the peer connection test which has known issues with tab crashing (see code for details).
LGTM if bots are happy. https://codereview.chromium.org/1912853004/diff/20001/content/browser/media/w... File content/browser/media/webrtc/webrtc_media_recorder_browsertest.cc (right): https://codereview.chromium.org/1912853004/diff/20001/content/browser/media/w... content/browser/media/webrtc/webrtc_media_recorder_browsertest.cc:15: // TODO(cpaulin): when crbug.com/585242 is fixed, enable peer connection http://crbug.com/... https://codereview.chromium.org/1912853004/diff/20001/content/browser/media/w... content/browser/media/webrtc/webrtc_media_recorder_browsertest.cc:41: // Turn on the flags we need. nit: Remove this comment. https://codereview.chromium.org/1912853004/diff/20001/content/browser/media/w... content/browser/media/webrtc/webrtc_media_recorder_browsertest.cc:48: switches::kEnableBlinkFeatures, kBlinkFeaturesNeeded); nit: inline "GetUserMedia" here, no need to extract it to another constant.
Addressed mcasas@'s comments in patch set 2 https://codereview.chromium.org/1912853004/diff/20001/content/browser/media/w... File content/browser/media/webrtc/webrtc_media_recorder_browsertest.cc (right): https://codereview.chromium.org/1912853004/diff/20001/content/browser/media/w... content/browser/media/webrtc/webrtc_media_recorder_browsertest.cc:15: // TODO(cpaulin): when crbug.com/585242 is fixed, enable peer connection On 2016/04/22 23:14:13, mcasas wrote: > http://crbug.com/... Done. https://codereview.chromium.org/1912853004/diff/20001/content/browser/media/w... content/browser/media/webrtc/webrtc_media_recorder_browsertest.cc:41: // Turn on the flags we need. On 2016/04/22 23:14:12, mcasas wrote: > nit: Remove this comment. Done. https://codereview.chromium.org/1912853004/diff/20001/content/browser/media/w... content/browser/media/webrtc/webrtc_media_recorder_browsertest.cc:48: switches::kEnableBlinkFeatures, kBlinkFeaturesNeeded); On 2016/04/22 23:14:12, mcasas wrote: > nit: inline "GetUserMedia" here, no need to extract it to another constant. Done.
lgtm https://codereview.chromium.org/1912853004/diff/40001/content/browser/media/w... File content/browser/media/webrtc/webrtc_media_recorder_browsertest.cc (right): https://codereview.chromium.org/1912853004/diff/40001/content/browser/media/w... content/browser/media/webrtc/webrtc_media_recorder_browsertest.cc:13: Move this down to the test method now that it just affects one test method and not the whole class.
Replied to phoglund's comment https://codereview.chromium.org/1912853004/diff/40001/content/browser/media/w... File content/browser/media/webrtc/webrtc_media_recorder_browsertest.cc (right): https://codereview.chromium.org/1912853004/diff/40001/content/browser/media/w... content/browser/media/webrtc/webrtc_media_recorder_browsertest.cc:13: On 2016/04/25 06:40:00, phoglund_chrome wrote: > Move this down to the test method now that it just affects one test method and > not the whole class. I am not sure I understand what you want: usually we define the tests exceptions at the top of the .cc file. I don't see why I should move it to lower position in the file, unless you mean something else?
On 2016/04/29 17:13:38, cpaulin wrote: > Replied to phoglund's comment > > https://codereview.chromium.org/1912853004/diff/40001/content/browser/media/w... > File content/browser/media/webrtc/webrtc_media_recorder_browsertest.cc (right): > > https://codereview.chromium.org/1912853004/diff/40001/content/browser/media/w... > content/browser/media/webrtc/webrtc_media_recorder_browsertest.cc:13: > On 2016/04/25 06:40:00, phoglund_chrome wrote: > > Move this down to the test method now that it just affects one test method and > > not the whole class. > > I am not sure I understand what you want: usually we define the tests exceptions > at the top of the .cc file. I don't see why I should move it to lower position > in the file, unless you mean something else? I checked other examples and yes I moved the android exception close to the test.
The CQ bit was checked by cpaulin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mcasas@chromium.org, phoglund@chromium.org Link to the patchset: https://codereview.chromium.org/1912853004/#ps60001 (title: "Addressed nit from phoglund@.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1912853004/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1912853004/60001
Message was sent while issue was closed.
Description was changed from ========== Media Recorder: Enable content browser test on Android. This CL enables MediaStream Recorder content browser test on Android devices. No new tests were added. BUG=605798, 561068 TEST=On trybots ========== to ========== Media Recorder: Enable content browser test on Android. This CL enables MediaStream Recorder content browser test on Android devices. No new tests were added. BUG=605798, 561068 TEST=On trybots ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/ff0ea18cf90aa0af082e4749a4b4a75acb5fa330 Cr-Commit-Position: refs/heads/master@{#390767}
Message was sent while issue was closed.
Description was changed from ========== Media Recorder: Enable content browser test on Android. This CL enables MediaStream Recorder content browser test on Android devices. No new tests were added. BUG=605798, 561068 TEST=On trybots ========== to ========== Media Recorder: Enable content browser test on Android. This CL enables MediaStream Recorder content browser test on Android devices. No new tests were added. BUG=605798, 561068 TEST=On trybots Committed: https://crrev.com/ff0ea18cf90aa0af082e4749a4b4a75acb5fa330 Cr-Commit-Position: refs/heads/master@{#390767} ========== |
