Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2550493003:
Add new device ID unit tests to UserMediaClientImpl. (Closed)
|
Created:
4 years ago by Guido Urdaneta Modified:
4 years ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, posciak+watch_chromium.org, jam, feature-media-reviews_chromium.org, darin-cc_chromium.org, mcasas+watch+vc_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd new device ID unit tests to UserMediaClientImpl.
Also remove test-specific code path and other minor cleanups.
This CL is preparatory for the new spec-compliant MediaStream constraint processing.
BUG=657733
Committed: https://crrev.com/d7af8155b1a7a903929b2a932a8a4d807eac487c
Cr-Commit-Position: refs/heads/master@{#436320}
Patch Set 1 #
Total comments: 4
Patch Set 2 : hta@'s comments #
Total comments: 3
Messages
Total messages: 26 (16 generated)
Patchset #1 (id:1) has been deleted
The CQ bit was checked by guidou@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Add new device ID unit tests to UserMediaClientImpl. Also remove test-specific code path and other minor cleanups. This CL is preparatory for the new spec-compliant MediaStream constraint processing. BUG=657733 ========== to ========== Add new device ID unit tests to UserMediaClientImpl. Also remove test-specific code path and other minor cleanups. This CL is preparatory for the new spec-compliant MediaStream constraint processing. BUG=657733 ==========
guidou@chromium.org changed reviewers: + hbos@chromium.org, hta@chromium.org
Great stuff! I think you can make the tests more readable by using MockConstraintFactory to build your constraints. https://codereview.chromium.org/2550493003/diff/20001/content/renderer/media/... File content/renderer/media/user_media_client_impl_unittest.cc (right): https://codereview.chromium.org/2550493003/diff/20001/content/renderer/media/... content/renderer/media/user_media_client_impl_unittest.cc:52: return vector; I think (you'll have to check) that you can do this function in one line as result = blink::WebVector(1, &value) but the constructors of blink::WebVector are really hard to read. https://codereview.chromium.org/2550493003/diff/20001/content/renderer/media/... content/renderer/media/user_media_client_impl_unittest.cc:59: const blink::WebString& advanced_ideal_value) { Any particular reason to not use a mock_constraint_factory.h factory here? That's a bit more flexible wrt adding ore than one constraint of each type.
Patchset #2 (id:40001) has been deleted
https://codereview.chromium.org/2550493003/diff/20001/content/renderer/media/... File content/renderer/media/user_media_client_impl_unittest.cc (right): https://codereview.chromium.org/2550493003/diff/20001/content/renderer/media/... content/renderer/media/user_media_client_impl_unittest.cc:52: return vector; On 2016/12/03 12:31:17, hta - Chromium wrote: > I think (you'll have to check) that you can do this function in one line as > > result = blink::WebVector(1, &value) > > but the constructors of blink::WebVector are really hard to read. Done. https://codereview.chromium.org/2550493003/diff/20001/content/renderer/media/... content/renderer/media/user_media_client_impl_unittest.cc:59: const blink::WebString& advanced_ideal_value) { On 2016/12/03 12:31:17, hta - Chromium wrote: > Any particular reason to not use a mock_constraint_factory.h factory here? > That's a bit more flexible wrt adding ore than one constraint of each type. Done. Didn't realize how useful it is.
The CQ bit was checked by guidou@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm but would be nice to have a comment about the bug that became obvious to me in review. https://codereview.chromium.org/2550493003/diff/60001/content/renderer/media/... File content/renderer/media/user_media_client_impl_unittest.cc (right): https://codereview.chromium.org/2550493003/diff/60001/content/renderer/media/... content/renderer/media/user_media_client_impl_unittest.cc:41: factory.AddAdvanced(); Hm - I take it this is necessary to have the constraint not evaluate to "false". That shouldn't be needed. Let it stand for now; I'll file a bug for it. https://bugs.chromium.org/p/chromium/issues/detail?id=671208
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm. I'm curious about if there are integration tests for this, considering tests are done with fake stuff. https://codereview.chromium.org/2550493003/diff/60001/content/renderer/media/... File content/renderer/media/user_media_client_impl.cc (right): https://codereview.chromium.org/2550493003/diff/60001/content/renderer/media/... content/renderer/media/user_media_client_impl.cc:249: user_media_request.ownerDocument().frame())); I take it that this is still needed even though the other test-specific code path was removed?
The CQ bit was checked by guidou@chromium.org
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": 60001, "attempt_start_ts": 1480955841059230,
"parent_rev": "22a7fe33b9b3c61a79036521de926d1bd813e2b4", "commit_rev":
"0e274cb67778035d44408e0187b155988a0063c6"}
Message was sent while issue was closed.
Description was changed from ========== Add new device ID unit tests to UserMediaClientImpl. Also remove test-specific code path and other minor cleanups. This CL is preparatory for the new spec-compliant MediaStream constraint processing. BUG=657733 ========== to ========== Add new device ID unit tests to UserMediaClientImpl. Also remove test-specific code path and other minor cleanups. This CL is preparatory for the new spec-compliant MediaStream constraint processing. BUG=657733 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Add new device ID unit tests to UserMediaClientImpl. Also remove test-specific code path and other minor cleanups. This CL is preparatory for the new spec-compliant MediaStream constraint processing. BUG=657733 ========== to ========== Add new device ID unit tests to UserMediaClientImpl. Also remove test-specific code path and other minor cleanups. This CL is preparatory for the new spec-compliant MediaStream constraint processing. BUG=657733 Committed: https://crrev.com/d7af8155b1a7a903929b2a932a8a4d807eac487c Cr-Commit-Position: refs/heads/master@{#436320} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/d7af8155b1a7a903929b2a932a8a4d807eac487c Cr-Commit-Position: refs/heads/master@{#436320}
Message was sent while issue was closed.
On 2016/12/05 16:30:32, hbos_chromium wrote: > lgtm. > > I'm curious about if there are integration tests for this, considering tests are > done with fake stuff. > These tests exercise this code (the UMCI part), although they use the old syntax. https://cs.chromium.org/chromium/src/content/browser/webrtc/webrtc_getusermed...
Message was sent while issue was closed.
https://codereview.chromium.org/2550493003/diff/60001/content/renderer/media/... File content/renderer/media/user_media_client_impl.cc (right): https://codereview.chromium.org/2550493003/diff/60001/content/renderer/media/... content/renderer/media/user_media_client_impl.cc:249: user_media_request.ownerDocument().frame())); On 2016/12/05 16:30:31, hbos_chromium wrote: > I take it that this is still needed even though the other test-specific code > path was removed? For some definition of needed :) It doesn't seem to hurt. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
