Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
564423002:
Update MockWebUserMediaClient for requestSources. (Closed)
|
Created:
6 years, 3 months ago by Henrik Grunell Modified:
6 years, 3 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, jochen+watch_chromium.org, mkwst+moarreviews-shell_chromium.org, mkwst+moarreviews-content_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionUpdate MockWebUserMediaClient for requestSources.
Blink CL https://codereview.chromium.org/559423002/ depends on this.
BUG=406094
Committed: https://crrev.com/0865a0889a9bc2d65046497a7ad2cb2792f38b29
Cr-Commit-Position: refs/heads/master@{#295090}
Patch Set 1 #Patch Set 2 : Rebase #
Total comments: 12
Patch Set 3 : Code review fixes. Rebase. #Patch Set 4 : Compile fix Android. #
Messages
Total messages: 21 (8 generated)
grunell@chromium.org changed reviewers: + pfeldman@chromium.org
grunell@chromium.org changed reviewers: + tkent@chromium.org
grunell@chromium.org changed reviewers: - tkent@chromium.org
grunell@chromium.org changed reviewers: + jochen@chromium.org - pfeldman@chromium.org
mkwst@chromium.org changed reviewers: + mkwst@chromium.org
Some drive-by comments. You'll still need a content/shell OWNER (like Jochen) to do a pass over the CL, of course. :) https://codereview.chromium.org/564423002/diff/20001/content/shell/renderer/t... File content/shell/renderer/test_runner/mock_web_user_media_client.cc (right): https://codereview.chromium.org/564423002/diff/20001/content/shell/renderer/t... content/shell/renderer/test_runner/mock_web_user_media_client.cc:215: const size_t size = 2; I see that other bits of this file use the same pattern, but it feels pretty hacky. I think it would be cleaner to adopt a struct-based pattern, such as you'd find in https://code.google.com/p/chromium/codesearch#chromium/src/net/base/net_util_.... That's less likely to get out of sync than this pattern. https://codereview.chromium.org/564423002/diff/20001/content/shell/renderer/t... content/shell/renderer/test_runner/mock_web_user_media_client.cc:225: WebSourceInfo::VideoFacingModeEnvironment); Is this enough flexibility for you? It might make sense (in a future CL) to add some write capabilities to the mock so you can set up scenarios you care about testing. https://codereview.chromium.org/564423002/diff/20001/content/shell/renderer/t... File content/shell/renderer/test_runner/mock_web_user_media_client.h (right): https://codereview.chromium.org/564423002/diff/20001/content/shell/renderer/t... content/shell/renderer/test_runner/mock_web_user_media_client.h:29: const blink::WebMediaStreamTrackSourcesRequest&) OVERRIDE; Nit: Drop the OVERRIDE from blink API overrides.
https://codereview.chromium.org/564423002/diff/20001/content/shell/renderer/t... File content/shell/renderer/test_runner/mock_web_user_media_client.cc (right): https://codereview.chromium.org/564423002/diff/20001/content/shell/renderer/t... content/shell/renderer/test_runner/mock_web_user_media_client.cc:215: const size_t size = 2; On 2014/09/16 12:56:09, Mike West wrote: > I see that other bits of this file use the same pattern, but it feels pretty > hacky. I think it would be cleaner to adopt a struct-based pattern, such as > you'd find in > https://code.google.com/p/chromium/codesearch#chromium/src/net/base/net_util_.... > That's less likely to get out of sync than this pattern. This will be used in layout tests, can it be controlled from there? The example is a Chrome unit test. https://codereview.chromium.org/564423002/diff/20001/content/shell/renderer/t... content/shell/renderer/test_runner/mock_web_user_media_client.cc:225: WebSourceInfo::VideoFacingModeEnvironment); On 2014/09/16 12:56:09, Mike West wrote: > Is this enough flexibility for you? It might make sense (in a future CL) to add > some write capabilities to the mock so you can set up scenarios you care about > testing. Well, this will be used in layout tests, can it be controlled from there? Despite that, note that the getSources JS API is going away so I think this is enough. https://codereview.chromium.org/564423002/diff/20001/content/shell/renderer/t... File content/shell/renderer/test_runner/mock_web_user_media_client.h (right): https://codereview.chromium.org/564423002/diff/20001/content/shell/renderer/t... content/shell/renderer/test_runner/mock_web_user_media_client.h:29: const blink::WebMediaStreamTrackSourcesRequest&) OVERRIDE; On 2014/09/16 12:56:10, Mike West wrote: > Nit: Drop the OVERRIDE from blink API overrides. Done.
https://codereview.chromium.org/564423002/diff/20001/content/shell/renderer/t... File content/shell/renderer/test_runner/mock_web_user_media_client.cc (right): https://codereview.chromium.org/564423002/diff/20001/content/shell/renderer/t... content/shell/renderer/test_runner/mock_web_user_media_client.cc:215: const size_t size = 2; On 2014/09/16 13:22:35, Henrik Grunell wrote: > On 2014/09/16 12:56:09, Mike West wrote: > > I see that other bits of this file use the same pattern, but it feels pretty > > hacky. I think it would be cleaner to adopt a struct-based pattern, such as > > you'd find in > > > https://code.google.com/p/chromium/codesearch#chromium/src/net/base/net_util_.... > > That's less likely to get out of sync than this pattern. > > This will be used in layout tests, can it be controlled from there? The example > is a Chrome unit test. No. My point was that if you're going to hard-code things into the Mock, it'd be better to do so in a way that doesn't force you to hard-code both the data and the size of the data. https://codereview.chromium.org/564423002/diff/20001/content/shell/renderer/t... content/shell/renderer/test_runner/mock_web_user_media_client.cc:225: WebSourceInfo::VideoFacingModeEnvironment); On 2014/09/16 13:22:35, Henrik Grunell wrote: > On 2014/09/16 12:56:09, Mike West wrote: > > Is this enough flexibility for you? It might make sense (in a future CL) to > add > > some write capabilities to the mock so you can set up scenarios you care about > > testing. > > Well, this will be used in layout tests, can it be controlled from there? There are a number of mocks with setter methods that bind to TestRunner in order to expose some bits and pieces to layout tests. Look at the various set* methods bound in TestRunnerBindings::GetObjectTemplateBuilder for some examples. I think that might be a route to explore for you in the future. > Despite that, note that the getSources JS API is going away so I think this is > enough. I'd accept that. :)
lgtm assuming mike is happy https://codereview.chromium.org/564423002/diff/20001/content/shell/renderer/t... File content/shell/renderer/test_runner/mock_web_user_media_client.cc (right): https://codereview.chromium.org/564423002/diff/20001/content/shell/renderer/t... content/shell/renderer/test_runner/mock_web_user_media_client.cc:118: }; nit. disallow_copy_and_assign
LGTM, assuming that the next patchset addresses the override and disallow-copy nits.
https://codereview.chromium.org/564423002/diff/20001/content/shell/renderer/t... File content/shell/renderer/test_runner/mock_web_user_media_client.cc (right): https://codereview.chromium.org/564423002/diff/20001/content/shell/renderer/t... content/shell/renderer/test_runner/mock_web_user_media_client.cc:118: }; On 2014/09/16 14:19:14, jochen wrote: > nit. disallow_copy_and_assign Done. https://codereview.chromium.org/564423002/diff/20001/content/shell/renderer/t... content/shell/renderer/test_runner/mock_web_user_media_client.cc:215: const size_t size = 2; On 2014/09/16 13:36:29, Mike West wrote: > On 2014/09/16 13:22:35, Henrik Grunell wrote: > > On 2014/09/16 12:56:09, Mike West wrote: > > > I see that other bits of this file use the same pattern, but it feels pretty > > > hacky. I think it would be cleaner to adopt a struct-based pattern, such as > > > you'd find in > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/net/base/net_util_.... > > > That's less likely to get out of sync than this pattern. > > > > This will be used in layout tests, can it be controlled from there? The > example > > is a Chrome unit test. > > No. My point was that if you're going to hard-code things into the Mock, it'd be > better to do so in a way that doesn't force you to hard-code both the data and > the size of the data. Ah, of course. :) Done. https://codereview.chromium.org/564423002/diff/20001/content/shell/renderer/t... content/shell/renderer/test_runner/mock_web_user_media_client.cc:225: WebSourceInfo::VideoFacingModeEnvironment); On 2014/09/16 13:36:29, Mike West wrote: > On 2014/09/16 13:22:35, Henrik Grunell wrote: > > On 2014/09/16 12:56:09, Mike West wrote: > > > Is this enough flexibility for you? It might make sense (in a future CL) to > > add > > > some write capabilities to the mock so you can set up scenarios you care > about > > > testing. > > > > Well, this will be used in layout tests, can it be controlled from there? > > There are a number of mocks with setter methods that bind to TestRunner in order > to expose some bits and pieces to layout tests. Look at the various set* methods > bound in TestRunnerBindings::GetObjectTemplateBuilder for some examples. I think > that might be a route to explore for you in the future. OK, thanks for the pointer. > > > Despite that, note that the getSources JS API is going away so I think this is > > enough. > > I'd accept that. :) OK, I'll leave as is then.
The CQ bit was checked by grunell@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/564423002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg_tests_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
The CQ bit was checked by grunell@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/564423002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as 1f144c9055224f2550a064de87e69e13976a4b65
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/0865a0889a9bc2d65046497a7ad2cb2792f38b29 Cr-Commit-Position: refs/heads/master@{#295090} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
