Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2858353002:
MediaPerceptionPrivate API impl and testing. (Closed)
|
Created:
3 years, 7 months ago by Luke Sorenson Modified:
3 years, 7 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionMediaPerceptionPrivate API impl and testing.
BUG=709180
Review-Url: https://codereview.chromium.org/2858353002
Cr-Commit-Position: refs/heads/master@{#474260}
Committed: https://chromium.googlesource.com/chromium/src/+/247725c00faa32d43ce0d8d8833edf4167b36ee1
Patch Set 1 #Patch Set 2 : Addressing comments on API implementation. #
Total comments: 62
Patch Set 3 : Addressing reviewer comments. #Patch Set 4 : More verbose error messages now. #Patch Set 5 : ImageFrameProtoToIdl and unittest #
Total comments: 46
Patch Set 6 : Addressing more comments. #Patch Set 7 : Removing enums.xml from this change. #
Total comments: 22
Patch Set 8 : Implemented CallbackStatus enum, AnalyticsProcessState enum, and addressed #
Total comments: 13
Patch Set 9 : Addressed comments, mediaPerceptionPrivate not visible to browser tests however. #Patch Set 10 : Browser tests pass with Toni's patch. #Patch Set 11 : Working on unittest for MediaPerceptionAPIManager #Patch Set 12 : MediaPerceptionAPIManagerTest working. #
Total comments: 16
Patch Set 13 : Addressed comments and all tests passing. #
Total comments: 60
Patch Set 14 : Addressed comments. #
Total comments: 14
Patch Set 15 : Addressed comments (added TestUpstartClient). #
Total comments: 10
Patch Set 16 : Addressed comments. #
Total comments: 42
Patch Set 17 : Addressed reviewer comments. #
Total comments: 14
Patch Set 18 : Addressing reviewer comments. #
Total comments: 8
Patch Set 19 : Addressed comments. #
Total comments: 20
Patch Set 20 : Addressed comments on unittest. #
Total comments: 2
Patch Set 21 : Addressed final outstanding comment. #
Total comments: 2
Patch Set 22 : Removed (c) from copyright in conversion_utils_unittest.cc #Patch Set 23 : Moved the tests to is_chromeos in extensions/browser/BUILD.gn #Patch Set 24 : Added media_perception_proto as direct dependency of test targets. #Depends on Patchset: Messages
Total messages: 79 (26 generated)
Description was changed from ========== MediaPerceptionPrivate API impl and testing. BUG=709180 ========== to ========== MediaPerceptionPrivate API impl and testing. BUG=709180 ==========
CL 3/3 with API implementation, unit and integration tests.
https://codereview.chromium.org/2858353002/diff/20001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/media_perception_private/diagnostics/runtest.js (right): https://codereview.chromium.org/2858353002/diff/20001/chrome/test/data/extens... chrome/test/data/extensions/api_test/media_perception_private/diagnostics/runtest.js:9: I'd rewrite this as chrome.test.runTests([ function getDiagnostics() { chrome.mediaPerceptionPrivate.getDiagnostics( chrome.test.callbackPass(function(result) { chrome.test.assertEq(...); })); } ]); https://codereview.chromium.org/2858353002/diff/20001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/media_perception_private/media_perception/manifest.json (right): https://codereview.chromium.org/2858353002/diff/20001/chrome/test/data/extens... chrome/test/data/extensions/api_test/media_perception_private/media_perception/manifest.json:4: { generate public - private key pair (e.g. I think packing the test extension in chrome://extensions should do the job), and set "key" here - this will force a specific extension ID for the test app, then in C++ you can add --whitelisted-extension-id <extension-id> flag to the command line (in SetUpCommandLine override of the test class). This will get you around the whitelist restriction on the API in tests :) or you can steal the key for some other private API test extension ;) https://codereview.chromium.org/2858353002/diff/20001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/media_perception_private/media_perception/runtest.js (right): https://codereview.chromium.org/2858353002/diff/20001/chrome/test/data/extens... chrome/test/data/extensions/api_test/media_perception_private/media_perception/runtest.js:10: chrome.mediaPerceptionPrivate.onMediaPerception, setState directly causing an event to be dispathed seems to automagical :) I'd do something like: chrome.test.runTests([ // the first subtest is probably not required function runMediaPerceptionServiceTest() { chrome.mediaPerceptionPrivate.setState({ status: 'RUNNING' }, chrome.test.callbackPass(function(response) { chrome.test.assertEq(response, {status: 'RUNNING'}); }))); }, function mediaPerceptionListenerTest() { chrome.test.listenOnce( chrome.mediaPeceptionPrivate.onMediaPerception, function(evt) { chrome.test.assertEq(evt.timestamp, 1); }); chrome.test.sendMessage('mediaPerceptionListenerSet'); // in C++ test would wait for mediaPerceptionListenerSet message, and invoke a method on // fake client to trigger a fake mediaPerception signal // this is a little more work, but it makes the test flow clearer, and more future prone } ]) https://codereview.chromium.org/2858353002/diff/20001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/media_perception_private/state/runtest.js (right): https://codereview.chromium.org/2858353002/diff/20001/chrome/test/data/extens... chrome/test/data/extensions/api_test/media_perception_private/state/runtest.js:25: chrome.test.assertEq(response.status, kStatusEnum); I don't think it's worth having this as a separate method. wdyt about: chrome.test.runTests([ function setStateTest() { chrome.mmp.setState('RUNNING', chrome.test.callbackPass(function(state) { chrome.test.assertEq({status: 'RUNNING'}, state); })) }, function getStateTest() { chrome.mpp.getState(chrome.test.callbackPass(function(state) { chrome.test.assertEq({status: 'RUNNING'}, state); })); }, ]); Also, can you add more negative cases - e.g. invalid state, setting deviceContext when not allowed, etc, https://codereview.chromium.org/2858353002/diff/20001/extensions/browser/api/... File extensions/browser/api/media_perception_private/media_perception_api_manager.cc (right): https://codereview.chromium.org/2858353002/diff/20001/extensions/browser/api/... extensions/browser/api/media_perception_private/media_perception_api_manager.cc:22: void PointProtoToIdl(const mri::Point& point, make this return unique_ptr<media_perception::Point> https://codereview.chromium.org/2858353002/diff/20001/extensions/browser/api/... extensions/browser/api/media_perception_private/media_perception_api_manager.cc:55: switch (entity.type()) { consider moving this to a helper method ConvertEntotyType or something along those lines https://codereview.chromium.org/2858353002/diff/20001/extensions/browser/api/... extensions/browser/api/media_perception_private/media_perception_api_manager.cc:62: default: case mri::Entity::UNSPECIFIED: https://codereview.chromium.org/2858353002/diff/20001/extensions/browser/api/... extensions/browser/api/media_perception_private/media_perception_api_manager.cc:71: BoundingBoxProtoToIdl(entity.bounding_box(), e_result.bounding_box.get()); e_result.bounding_box = BoundingBoxProtoToIdl(entity.bounding_box()); // and have BoundingBoxProtoToIdl return a unique_ptr https://codereview.chromium.org/2858353002/diff/20001/extensions/browser/api/... extensions/browser/api/media_perception_private/media_perception_api_manager.cc:158: // Status unset. NOTREACHED() << "Status not set." https://codereview.chromium.org/2858353002/diff/20001/extensions/browser/api/... extensions/browser/api/media_perception_private/media_perception_api_manager.cc:206: g_factory = LAZY_INSTANCE_INITIALIZER; I think this should be leaky https://codereview.chromium.org/2858353002/diff/20001/extensions/browser/api/... extensions/browser/api/media_perception_private/media_perception_api_manager.cc:228: // TODO(lasoren): Verify this gets called at the right time. What do you mean by this? https://codereview.chromium.org/2858353002/diff/20001/extensions/browser/api/... extensions/browser/api/media_perception_private/media_perception_api_manager.cc:253: if (state_proto.status() != mri::State::RUNNING && This can probably be a DCHECK. https://codereview.chromium.org/2858353002/diff/20001/extensions/browser/api/... extensions/browser/api/media_perception_private/media_perception_api_manager.cc:261: if (!analytics_process_running_) { will we ever need to stop the process (from the app)? Either way, isn't SUSPENDED state defiend as state where the naalytics process is running, but not active? Wouldn't that mean StartMediaAnalytics has to be called in that case as well? https://codereview.chromium.org/2858353002/diff/20001/extensions/browser/api/... extensions/browser/api/media_perception_private/media_perception_api_manager.cc:267: base::Unretained(this), callback, state_proto)); Use weak ptr here (and elsewhere where you call into dbus_client)? https://codereview.chromium.org/2858353002/diff/20001/extensions/browser/api/... extensions/browser/api/media_perception_private/media_perception_api_manager.cc:299: bool succeeded) { if (!success) { callback.Run(false, media_perception::State); return; } analytics_process_running_ = true; SetStateInternal(callback, state); https://codereview.chromium.org/2858353002/diff/20001/extensions/browser/api/... extensions/browser/api/media_perception_private/media_perception_api_manager.cc:317: callback.Run(false, std::move(state)); just: callback.Run(false, media_perception::State); (the state.status is irelevant, given that the api function responds with an error when success is false). https://codereview.chromium.org/2858353002/diff/20001/extensions/browser/api/... extensions/browser/api/media_perception_private/media_perception_api_manager.cc:321: callback.Run(true, std::move(state)); callback.Run(true, StateProtoToIdl(state_proto)); https://codereview.chromium.org/2858353002/diff/20001/extensions/browser/api/... extensions/browser/api/media_perception_private/media_perception_api_manager.cc:331: return; same as for StateCallback applies https://codereview.chromium.org/2858353002/diff/20001/extensions/browser/api/... extensions/browser/api/media_perception_private/media_perception_api_manager.cc:340: if (!router || !router->HasEventListener( I think you'd be fine without HasEventListener check. https://codereview.chromium.org/2858353002/diff/20001/extensions/browser/api/... File extensions/browser/api/media_perception_private/media_perception_api_manager.h (right): https://codereview.chromium.org/2858353002/diff/20001/extensions/browser/api/... extensions/browser/api/media_perception_private/media_perception_api_manager.h:29: const mri::MediaPerception& media_perception); if you want to make these public, maybe moove them to a separate file? https://codereview.chromium.org/2858353002/diff/20001/extensions/browser/api/... extensions/browser/api/media_perception_private/media_perception_api_manager.h:98: bool analytics_process_running_; disallow copy and assign https://codereview.chromium.org/2858353002/diff/20001/extensions/browser/api/... File extensions/browser/api/media_perception_private/media_perception_private_api.cc (right): https://codereview.chromium.org/2858353002/diff/20001/extensions/browser/api/... extensions/browser/api/media_perception_private/media_perception_private_api.cc:24: if (!manager) { Should this ever happen? Wound DCHECK be more appropriate? https://codereview.chromium.org/2858353002/diff/20001/extensions/browser/api/... extensions/browser/api/media_perception_private/media_perception_private_api.cc:36: Respond(Error("Failed to getState.")); return; https://codereview.chromium.org/2858353002/diff/20001/extensions/browser/api/... extensions/browser/api/media_perception_private/media_perception_private_api.cc:55: return RespondNow(Error("Settable statuses are RUNNING and SUSPENDED.")); Status can be set only to RUNNING or SUSPEND. Also, can you check the device context is set only in uninitialized or suspedned state? https://codereview.chromium.org/2858353002/diff/20001/extensions/browser/api/... extensions/browser/api/media_perception_private/media_perception_private_api.cc:73: Respond(Error("Failed to setState.")); return; https://codereview.chromium.org/2858353002/diff/20001/extensions/browser/api/... extensions/browser/api/media_perception_private/media_perception_private_api.cc:101: Respond(Error("Failed to getDiagnostics.")); return https://codereview.chromium.org/2858353002/diff/20001/extensions/browser/api/... File extensions/browser/api/media_perception_private/media_perception_private_apitest.cc (right): https://codereview.chromium.org/2858353002/diff/20001/extensions/browser/api/... extensions/browser/api/media_perception_private/media_perception_private_apitest.cc:20: ASSERT_TRUE(RunExtensionTest("media_perception_private/state")) << message_; can you run this as platform app test? given that the API is targeted at apps?
Thanks for the review! PTAL (uploading new patch set now) https://codereview.chromium.org/2858353002/diff/20001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/media_perception_private/diagnostics/runtest.js (right): https://codereview.chromium.org/2858353002/diff/20001/chrome/test/data/extens... chrome/test/data/extensions/api_test/media_perception_private/diagnostics/runtest.js:9: On 2017/05/09 01:28:36, tbarzic wrote: > I'd rewrite this as > > chrome.test.runTests([ > function getDiagnostics() { > chrome.mediaPerceptionPrivate.getDiagnostics( > chrome.test.callbackPass(function(result) { > chrome.test.assertEq(...); > })); > } > ]); Done. https://codereview.chromium.org/2858353002/diff/20001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/media_perception_private/media_perception/manifest.json (right): https://codereview.chromium.org/2858353002/diff/20001/chrome/test/data/extens... chrome/test/data/extensions/api_test/media_perception_private/media_perception/manifest.json:4: { On 2017/05/09 01:28:36, tbarzic wrote: > generate public - private key pair (e.g. I think packing the test extension in > chrome://extensions should do the job), and set "key" here - this will force a > specific extension ID for the test app, then in C++ you can add > --whitelisted-extension-id <extension-id> flag to the command line (in > SetUpCommandLine override of the test class). This will get you around the > whitelist restriction on the API in tests :) > > or you can steal the key for some other private API test extension ;) Done. https://codereview.chromium.org/2858353002/diff/20001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/media_perception_private/media_perception/runtest.js (right): https://codereview.chromium.org/2858353002/diff/20001/chrome/test/data/extens... chrome/test/data/extensions/api_test/media_perception_private/media_perception/runtest.js:10: chrome.mediaPerceptionPrivate.onMediaPerception, On 2017/05/09 01:28:36, tbarzic wrote: > setState directly causing an event to be dispathed seems to automagical :) > > I'd do something like: > > chrome.test.runTests([ > // the first subtest is probably not required > function runMediaPerceptionServiceTest() { > chrome.mediaPerceptionPrivate.setState({ > status: 'RUNNING' > }, chrome.test.callbackPass(function(response) { > chrome.test.assertEq(response, {status: 'RUNNING'}); > }))); > }, > function mediaPerceptionListenerTest() { > chrome.test.listenOnce( > chrome.mediaPeceptionPrivate.onMediaPerception, > function(evt) { > chrome.test.assertEq(evt.timestamp, 1); > }); > > chrome.test.sendMessage('mediaPerceptionListenerSet'); > // in C++ test would wait for mediaPerceptionListenerSet message, and > invoke a method on > // fake client to trigger a fake mediaPerception signal > // this is a little more work, but it makes the test flow clearer, and more > future prone > } > ]) Done. https://codereview.chromium.org/2858353002/diff/20001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/media_perception_private/state/runtest.js (right): https://codereview.chromium.org/2858353002/diff/20001/chrome/test/data/extens... chrome/test/data/extensions/api_test/media_perception_private/state/runtest.js:25: chrome.test.assertEq(response.status, kStatusEnum); On 2017/05/09 01:28:36, tbarzic wrote: > I don't think it's worth having this as a separate method. > > wdyt about: > > chrome.test.runTests([ function setStateTest() { > chrome.mmp.setState('RUNNING', chrome.test.callbackPass(function(state) { > chrome.test.assertEq({status: 'RUNNING'}, state); > })) > }, > > function getStateTest() { > chrome.mpp.getState(chrome.test.callbackPass(function(state) { > chrome.test.assertEq({status: 'RUNNING'}, state); > })); > }, > ]); > > > Also, can you add more negative cases - e.g. invalid state, setting > deviceContext when not allowed, etc, Done. Fine with getting rid of the callback method, but I think its more readable to runTests as defined methods when the number of tests is larger. https://codereview.chromium.org/2858353002/diff/20001/extensions/browser/api/... File extensions/browser/api/media_perception_private/media_perception_api_manager.cc (right): https://codereview.chromium.org/2858353002/diff/20001/extensions/browser/api/... extensions/browser/api/media_perception_private/media_perception_api_manager.cc:22: void PointProtoToIdl(const mri::Point& point, On 2017/05/09 01:28:36, tbarzic wrote: > make this return unique_ptr<media_perception::Point> Done. https://codereview.chromium.org/2858353002/diff/20001/extensions/browser/api/... extensions/browser/api/media_perception_private/media_perception_api_manager.cc:55: switch (entity.type()) { On 2017/05/09 01:28:36, tbarzic wrote: > consider moving this to a helper method > ConvertEntotyType or something along those lines Done. https://codereview.chromium.org/2858353002/diff/20001/extensions/browser/api/... extensions/browser/api/media_perception_private/media_perception_api_manager.cc:62: default: On 2017/05/09 01:28:36, tbarzic wrote: > case mri::Entity::UNSPECIFIED: > Done. https://codereview.chromium.org/2858353002/diff/20001/extensions/browser/api/... extensions/browser/api/media_perception_private/media_perception_api_manager.cc:71: BoundingBoxProtoToIdl(entity.bounding_box(), e_result.bounding_box.get()); On 2017/05/09 01:28:36, tbarzic wrote: > e_result.bounding_box = BoundingBoxProtoToIdl(entity.bounding_box()); > // and have BoundingBoxProtoToIdl return a unique_ptr Done. https://codereview.chromium.org/2858353002/diff/20001/extensions/browser/api/... extensions/browser/api/media_perception_private/media_perception_api_manager.cc:158: // Status unset. On 2017/05/09 01:28:36, tbarzic wrote: > NOTREACHED() << "Status not set." Done. https://codereview.chromium.org/2858353002/diff/20001/extensions/browser/api/... extensions/browser/api/media_perception_private/media_perception_api_manager.cc:206: g_factory = LAZY_INSTANCE_INITIALIZER; On 2017/05/09 01:28:36, tbarzic wrote: > I think this should be leaky Done. https://codereview.chromium.org/2858353002/diff/20001/extensions/browser/api/... extensions/browser/api/media_perception_private/media_perception_api_manager.cc:228: // TODO(lasoren): Verify this gets called at the right time. On 2017/05/09 01:28:36, tbarzic wrote: > What do you mean by this? I was curious when the destructor for this class is actually called and tested this. Removing the TODO. https://codereview.chromium.org/2858353002/diff/20001/extensions/browser/api/... extensions/browser/api/media_perception_private/media_perception_api_manager.cc:253: if (state_proto.status() != mri::State::RUNNING && On 2017/05/09 01:28:36, tbarzic wrote: > This can probably be a DCHECK. Done. https://codereview.chromium.org/2858353002/diff/20001/extensions/browser/api/... extensions/browser/api/media_perception_private/media_perception_api_manager.cc:261: if (!analytics_process_running_) { On 2017/05/09 01:28:36, tbarzic wrote: > will we ever need to stop the process (from the app)? > > Either way, isn't SUSPENDED state defiend as state where the naalytics process > is running, but not active? Wouldn't that mean StartMediaAnalytics has to be > called in that case as well? > From a simplicity and security standpoint, we didn't want the app to be capable of explicitly launching and killing the process, but rather that the process is launched and killed at the appropriate time and the app doesn't need to care. https://codereview.chromium.org/2858353002/diff/20001/extensions/browser/api/... extensions/browser/api/media_perception_private/media_perception_api_manager.cc:267: base::Unretained(this), callback, state_proto)); On 2017/05/09 01:28:37, tbarzic wrote: > Use weak ptr here (and elsewhere where you call into dbus_client)? Done. https://codereview.chromium.org/2858353002/diff/20001/extensions/browser/api/... extensions/browser/api/media_perception_private/media_perception_api_manager.cc:299: bool succeeded) { On 2017/05/09 01:28:36, tbarzic wrote: > if (!success) { > callback.Run(false, media_perception::State); > return; > } > analytics_process_running_ = true; > SetStateInternal(callback, state); Done. https://codereview.chromium.org/2858353002/diff/20001/extensions/browser/api/... extensions/browser/api/media_perception_private/media_perception_api_manager.cc:317: callback.Run(false, std::move(state)); On 2017/05/09 01:28:36, tbarzic wrote: > just: > callback.Run(false, media_perception::State); > > (the state.status is irelevant, given that the api function responds with an > error when success is false). Is there any precedent for / is it possible to respond with both and error and a State object (so that I can provide things like TIMEOUT as status back to the app)? https://codereview.chromium.org/2858353002/diff/20001/extensions/browser/api/... extensions/browser/api/media_perception_private/media_perception_api_manager.cc:321: callback.Run(true, std::move(state)); On 2017/05/09 01:28:36, tbarzic wrote: > callback.Run(true, StateProtoToIdl(state_proto)); Done. https://codereview.chromium.org/2858353002/diff/20001/extensions/browser/api/... extensions/browser/api/media_perception_private/media_perception_api_manager.cc:331: return; On 2017/05/09 01:28:36, tbarzic wrote: > same as for StateCallback applies Done. https://codereview.chromium.org/2858353002/diff/20001/extensions/browser/api/... extensions/browser/api/media_perception_private/media_perception_api_manager.cc:340: if (!router || !router->HasEventListener( On 2017/05/09 01:28:36, tbarzic wrote: > I think you'd be fine without HasEventListener check. Does it hurt to leave the check in? Can help with debugging? https://codereview.chromium.org/2858353002/diff/20001/extensions/browser/api/... File extensions/browser/api/media_perception_private/media_perception_api_manager.h (right): https://codereview.chromium.org/2858353002/diff/20001/extensions/browser/api/... extensions/browser/api/media_perception_private/media_perception_api_manager.h:29: const mri::MediaPerception& media_perception); On 2017/05/09 01:28:37, tbarzic wrote: > if you want to make these public, maybe moove them to a separate file? Moving to separate files. https://codereview.chromium.org/2858353002/diff/20001/extensions/browser/api/... extensions/browser/api/media_perception_private/media_perception_api_manager.h:98: bool analytics_process_running_; On 2017/05/09 01:28:37, tbarzic wrote: > disallow copy and assign Done. https://codereview.chromium.org/2858353002/diff/20001/extensions/browser/api/... File extensions/browser/api/media_perception_private/media_perception_private_api.cc (right): https://codereview.chromium.org/2858353002/diff/20001/extensions/browser/api/... extensions/browser/api/media_perception_private/media_perception_private_api.cc:24: if (!manager) { On 2017/05/09 01:28:37, tbarzic wrote: > Should this ever happen? Wound DCHECK be more appropriate? Don't think so. Replaced. https://codereview.chromium.org/2858353002/diff/20001/extensions/browser/api/... extensions/browser/api/media_perception_private/media_perception_private_api.cc:36: Respond(Error("Failed to getState.")); On 2017/05/09 01:28:37, tbarzic wrote: > return; Done. https://codereview.chromium.org/2858353002/diff/20001/extensions/browser/api/... extensions/browser/api/media_perception_private/media_perception_private_api.cc:55: return RespondNow(Error("Settable statuses are RUNNING and SUSPENDED.")); On 2017/05/09 01:28:37, tbarzic wrote: > Status can be set only to RUNNING or SUSPEND. > Also, can you check the device context is set only in uninitialized or suspedned > state? Done. https://codereview.chromium.org/2858353002/diff/20001/extensions/browser/api/... extensions/browser/api/media_perception_private/media_perception_private_api.cc:73: Respond(Error("Failed to setState.")); On 2017/05/09 01:28:37, tbarzic wrote: > return; Done. https://codereview.chromium.org/2858353002/diff/20001/extensions/browser/api/... extensions/browser/api/media_perception_private/media_perception_private_api.cc:101: Respond(Error("Failed to getDiagnostics.")); On 2017/05/09 01:28:37, tbarzic wrote: > return Done. https://codereview.chromium.org/2858353002/diff/20001/extensions/browser/api/... File extensions/browser/api/media_perception_private/media_perception_private_apitest.cc (right): https://codereview.chromium.org/2858353002/diff/20001/extensions/browser/api/... extensions/browser/api/media_perception_private/media_perception_private_apitest.cc:20: ASSERT_TRUE(RunExtensionTest("media_perception_private/state")) << message_; On 2017/05/09 01:28:37, tbarzic wrote: > can you run this as platform app test? given that the API is targeted at apps? Done.
https://codereview.chromium.org/2858353002/diff/20001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/media_perception_private/state/runtest.js (right): https://codereview.chromium.org/2858353002/diff/20001/chrome/test/data/extens... chrome/test/data/extensions/api_test/media_perception_private/state/runtest.js:25: chrome.test.assertEq(response.status, kStatusEnum); On 2017/05/09 21:05:44, Luke Sorenson wrote: > On 2017/05/09 01:28:36, tbarzic wrote: > > I don't think it's worth having this as a separate method. > > > > wdyt about: > > > > chrome.test.runTests([ function setStateTest() { > > chrome.mmp.setState('RUNNING', chrome.test.callbackPass(function(state) { > > chrome.test.assertEq({status: 'RUNNING'}, state); > > })) > > }, > > > > function getStateTest() { > > chrome.mpp.getState(chrome.test.callbackPass(function(state) { > > chrome.test.assertEq({status: 'RUNNING'}, state); > > })); > > }, > > ]); > > > > > > Also, can you add more negative cases - e.g. invalid state, setting > > deviceContext when not allowed, etc, > > Done. Fine with getting rid of the callback method, but I think its more > readable to runTests as defined methods when the number of tests is larger. yes, but a couple should be OK to inline https://codereview.chromium.org/2858353002/diff/20001/extensions/browser/api/... File extensions/browser/api/media_perception_private/media_perception_api_manager.cc (right): https://codereview.chromium.org/2858353002/diff/20001/extensions/browser/api/... extensions/browser/api/media_perception_private/media_perception_api_manager.cc:317: callback.Run(false, std::move(state)); On 2017/05/09 21:05:44, Luke Sorenson wrote: > On 2017/05/09 01:28:36, tbarzic wrote: > > just: > > callback.Run(false, media_perception::State); > > > > (the state.status is irelevant, given that the api function responds with an > > error when success is false). > > Is there any precedent for / is it possible to respond with both and error and a > State object (so that I can provide things like TIMEOUT as status back to the > app)? yes you can. My preferred approach would be for this to return a status enum (instead of bool succeeded), and then the api function to set an appropriate error. Though, you could just have the function normally respond with the state object set to error (instead of responding with an error) https://codereview.chromium.org/2858353002/diff/20001/extensions/browser/api/... extensions/browser/api/media_perception_private/media_perception_api_manager.cc:340: if (!router || !router->HasEventListener( On 2017/05/09 21:05:44, Luke Sorenson wrote: > On 2017/05/09 01:28:36, tbarzic wrote: > > I think you'd be fine without HasEventListener check. > > Does it hurt to leave the check in? Can help with debugging? it doesn't really hurt, but it's not that useful either :)
Thanks, added more verbose error messages as well. https://codereview.chromium.org/2858353002/diff/20001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/media_perception_private/state/runtest.js (right): https://codereview.chromium.org/2858353002/diff/20001/chrome/test/data/extens... chrome/test/data/extensions/api_test/media_perception_private/state/runtest.js:25: chrome.test.assertEq(response.status, kStatusEnum); On 2017/05/09 21:19:14, tbarzic wrote: > On 2017/05/09 21:05:44, Luke Sorenson wrote: > > On 2017/05/09 01:28:36, tbarzic wrote: > > > I don't think it's worth having this as a separate method. > > > > > > wdyt about: > > > > > > chrome.test.runTests([ function setStateTest() { > > > chrome.mmp.setState('RUNNING', chrome.test.callbackPass(function(state) > { > > > chrome.test.assertEq({status: 'RUNNING'}, state); > > > })) > > > }, > > > > > > function getStateTest() { > > > chrome.mpp.getState(chrome.test.callbackPass(function(state) { > > > chrome.test.assertEq({status: 'RUNNING'}, state); > > > })); > > > }, > > > ]); > > > > > > > > > Also, can you add more negative cases - e.g. invalid state, setting > > > deviceContext when not allowed, etc, > > > > Done. Fine with getting rid of the callback method, but I think its more > > readable to runTests as defined methods when the number of tests is larger. > > yes, but a couple should be OK to inline I've got more than a couple now :) https://codereview.chromium.org/2858353002/diff/20001/extensions/browser/api/... File extensions/browser/api/media_perception_private/media_perception_api_manager.cc (right): https://codereview.chromium.org/2858353002/diff/20001/extensions/browser/api/... extensions/browser/api/media_perception_private/media_perception_api_manager.cc:317: callback.Run(false, std::move(state)); On 2017/05/09 21:19:14, tbarzic wrote: > On 2017/05/09 21:05:44, Luke Sorenson wrote: > > On 2017/05/09 01:28:36, tbarzic wrote: > > > just: > > > callback.Run(false, media_perception::State); > > > > > > (the state.status is irelevant, given that the api function responds with an > > > error when success is false). > > > > Is there any precedent for / is it possible to respond with both and error and > a > > State object (so that I can provide things like TIMEOUT as status back to the > > app)? > > yes you can. > My preferred approach would be for this to return a status enum (instead of bool > succeeded), and then the api function to set an appropriate error. > > Though, you could just have the function normally respond with the state object > set to error (instead of responding with an error) Acknowledged. https://codereview.chromium.org/2858353002/diff/20001/extensions/browser/api/... extensions/browser/api/media_perception_private/media_perception_api_manager.cc:340: if (!router || !router->HasEventListener( On 2017/05/09 21:19:14, tbarzic wrote: > On 2017/05/09 21:05:44, Luke Sorenson wrote: > > On 2017/05/09 01:28:36, tbarzic wrote: > > > I think you'd be fine without HasEventListener check. > > > > Does it hurt to leave the check in? Can help with debugging? > > it doesn't really hurt, but it's not that useful either :) Acknowledged.
https://codereview.chromium.org/2858353002/diff/20001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/media_perception_private/state/runtest.js (right): https://codereview.chromium.org/2858353002/diff/20001/chrome/test/data/extens... chrome/test/data/extensions/api_test/media_perception_private/state/runtest.js:25: chrome.test.assertEq(response.status, kStatusEnum); On 2017/05/10 18:50:16, Luke Sorenson wrote: > On 2017/05/09 21:19:14, tbarzic wrote: > > On 2017/05/09 21:05:44, Luke Sorenson wrote: > > > On 2017/05/09 01:28:36, tbarzic wrote: > > > > I don't think it's worth having this as a separate method. > > > > > > > > wdyt about: > > > > > > > > chrome.test.runTests([ function setStateTest() { > > > > chrome.mmp.setState('RUNNING', > chrome.test.callbackPass(function(state) > > { > > > > chrome.test.assertEq({status: 'RUNNING'}, state); > > > > })) > > > > }, > > > > > > > > function getStateTest() { > > > > chrome.mpp.getState(chrome.test.callbackPass(function(state) { > > > > chrome.test.assertEq({status: 'RUNNING'}, state); > > > > })); > > > > }, > > > > ]); > > > > > > > > > > > > Also, can you add more negative cases - e.g. invalid state, setting > > > > deviceContext when not allowed, etc, > > > > > > Done. Fine with getting rid of the callback method, but I think its more > > > readable to runTests as defined methods when the number of tests is larger. > > > > yes, but a couple should be OK to inline > > I've got more than a couple now :) still, not too many - I find the inlined version easier to parse. but up to you. https://codereview.chromium.org/2858353002/diff/80001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/media_perception_private/diagnostics/runtest.js (right): https://codereview.chromium.org/2858353002/diff/80001/chrome/test/data/extens... chrome/test/data/extensions/api_test/media_perception_private/diagnostics/runtest.js:8: chrome.mediaPerceptionPrivate.setState( can you move this to another test, to make sure getDiagnostics does not get called before the state callback is invoked? also, reformat this as: chrome.mediaPerceptionPrivate.setState({ status: 'RUNNING' }, chrome.testCallbackPass(function(state) { })) https://codereview.chromium.org/2858353002/diff/80001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/media_perception_private/media_perception/runtest.js (right): https://codereview.chromium.org/2858353002/diff/80001/chrome/test/data/extens... chrome/test/data/extensions/api_test/media_perception_private/media_perception/runtest.js:11: chrome.mediaPerceptionPrivate.setState( same feedback as previous test https://codereview.chromium.org/2858353002/diff/80001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/media_perception_private/state/runtest.js (right): https://codereview.chromium.org/2858353002/diff/80001/chrome/test/data/extens... chrome/test/data/extensions/api_test/media_perception_private/state/runtest.js:5: let kStatusEnum = 'RUNNING' can you just create new state object in tests? https://codereview.chromium.org/2858353002/diff/80001/chrome/test/data/extens... chrome/test/data/extensions/api_test/media_perception_private/state/runtest.js:9: state.deviceContext = 'Just a string.'; can you set this to something little more meaningful? like: 'device_context'. https://codereview.chromium.org/2858353002/diff/80001/chrome/test/data/extens... chrome/test/data/extensions/api_test/media_perception_private/state/runtest.js:24: state.status = 'UNINITIALIZED'; what about other states? https://codereview.chromium.org/2858353002/diff/80001/chrome/test/data/extens... chrome/test/data/extensions/api_test/media_perception_private/state/runtest.js:33: const error = 'Only provide deviceContext with SetState RUNNING.'; can you test there's an error if the current state is RUNNING? https://codereview.chromium.org/2858353002/diff/80001/extensions/browser/api/... File extensions/browser/api/media_perception_private/conversion_utils.h (right): https://codereview.chromium.org/2858353002/diff/80001/extensions/browser/api/... extensions/browser/api/media_perception_private/conversion_utils.h:5: #ifndef EXTENSIONS_BROWSER_API_MEDIA_PERCEPTION_PRIVATE_CONVERSION_UTILS_H_ s/utils/util/ https://codereview.chromium.org/2858353002/diff/80001/extensions/browser/api/... File extensions/browser/api/media_perception_private/media_perception_api_manager.cc (right): https://codereview.chromium.org/2858353002/diff/80001/extensions/browser/api/... extensions/browser/api/media_perception_private/media_perception_api_manager.cc:60: media_perception::State state_unitialized; can you add a test for this? https://codereview.chromium.org/2858353002/diff/80001/extensions/browser/api/... extensions/browser/api/media_perception_private/media_perception_api_manager.cc:77: if (!analytics_process_running_) { if (analytics_process_running_) { SetStateInternal(); return; } if (state_proto.status() == mri::State::RUNNIN) { // request start process return; } // error callback https://codereview.chromium.org/2858353002/diff/80001/extensions/browser/api/... extensions/browser/api/media_perception_private/media_perception_api_manager.cc:81: dbus_client->StartMediaAnalytics( what if SetState is called before this returns? e.g. SetState(RUNNING) call UpstartClient SetState(RUNNING) we should probably not make a duplicate StartMediaAnalytics call here https://codereview.chromium.org/2858353002/diff/80001/extensions/browser/api/... extensions/browser/api/media_perception_private/media_perception_api_manager.cc:118: analytics_process_running_ = false; do you need this? https://codereview.chromium.org/2858353002/diff/80001/extensions/browser/api/... extensions/browser/api/media_perception_private/media_perception_api_manager.cc:135: callback.Run(false, std::move(state)); instead of passing error code via state, can you change |success| to an enum with error codes. Or just return state (with not success parameter) and forward it to the app https://codereview.chromium.org/2858353002/diff/80001/extensions/browser/api/... extensions/browser/api/media_perception_private/media_perception_api_manager.cc:157: LOG(WARNING) << "Router null or no event listener when receiving a " Remove this log, no listener being set seems like a real possibility here, we don't want to spew logs if that happens I'd change this whole if to: DCHECK(router); https://codereview.chromium.org/2858353002/diff/80001/extensions/browser/api/... File extensions/browser/api/media_perception_private/media_perception_private_api.cc (right): https://codereview.chromium.org/2858353002/diff/80001/extensions/browser/api/... extensions/browser/api/media_perception_private/media_perception_private_api.cc:19: return "Failed to get or set state because the D-Bus communication " can you add constants for the error strigns, I think it would make this easier to read. https://codereview.chromium.org/2858353002/diff/80001/extensions/browser/api/... extensions/browser/api/media_perception_private/media_perception_private_api.cc:72: // Check that the desired status is settable. I'd remove the comment, it's evident from the code what's going on. https://codereview.chromium.org/2858353002/diff/80001/extensions/browser/api/... extensions/browser/api/media_perception_private/media_perception_private_api.cc:78: // Check that device context is only provided with SetState RUNNING. nit: new line before this https://codereview.chromium.org/2858353002/diff/80001/extensions/browser/api/... File extensions/browser/api/media_perception_private/media_perception_private_apitest.cc (right): https://codereview.chromium.org/2858353002/diff/80001/extensions/browser/api/... extensions/browser/api/media_perception_private/media_perception_private_apitest.cc:28: void InitializeDbusClient() { This is not really initializing D bus client, maybe rename this to FakeMEdiaAnalyticsClient* GetMediaAnalyticsClient() https://codereview.chromium.org/2858353002/diff/80001/extensions/browser/api/... extensions/browser/api/media_perception_private/media_perception_private_apitest.cc:29: dbus_client_ = reinterpret_cast<chromeos::FakeMediaAnalyticsClient*>( Can you check if you can use DBusThreadManager::GetSetterForTesting to inject a fake media analytics client? https://codereview.chromium.org/2858353002/diff/80001/extensions/browser/api/... extensions/browser/api/media_perception_private/media_perception_private_apitest.cc:33: ~MediaPerceptionPrivateApiTest() override {} move this after ctor; use = default where possible https://codereview.chromium.org/2858353002/diff/80001/extensions/browser/api/... extensions/browser/api/media_perception_private/media_perception_private_apitest.cc:35: chromeos::FakeMediaAnalyticsClient* dbus_client_; DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/2858353002/diff/80001/extensions/browser/api/... extensions/browser/api/media_perception_private/media_perception_private_apitest.cc:65: dbus_client_->SetFrameId(1); do you need this for this test?
The CQ bit was checked by lasoren@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: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Thanks, addressed comments. PTAL https://codereview.chromium.org/2858353002/diff/20001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/media_perception_private/state/runtest.js (right): https://codereview.chromium.org/2858353002/diff/20001/chrome/test/data/extens... chrome/test/data/extensions/api_test/media_perception_private/state/runtest.js:25: chrome.test.assertEq(response.status, kStatusEnum); On 2017/05/11 00:38:26, tbarzic wrote: > On 2017/05/10 18:50:16, Luke Sorenson wrote: > > On 2017/05/09 21:19:14, tbarzic wrote: > > > On 2017/05/09 21:05:44, Luke Sorenson wrote: > > > > On 2017/05/09 01:28:36, tbarzic wrote: > > > > > I don't think it's worth having this as a separate method. > > > > > > > > > > wdyt about: > > > > > > > > > > chrome.test.runTests([ function setStateTest() { > > > > > chrome.mmp.setState('RUNNING', > > chrome.test.callbackPass(function(state) > > > { > > > > > chrome.test.assertEq({status: 'RUNNING'}, state); > > > > > })) > > > > > }, > > > > > > > > > > function getStateTest() { > > > > > chrome.mpp.getState(chrome.test.callbackPass(function(state) { > > > > > chrome.test.assertEq({status: 'RUNNING'}, state); > > > > > })); > > > > > }, > > > > > ]); > > > > > > > > > > > > > > > Also, can you add more negative cases - e.g. invalid state, setting > > > > > deviceContext when not allowed, etc, > > > > > > > > Done. Fine with getting rid of the callback method, but I think its more > > > > readable to runTests as defined methods when the number of tests is > larger. > > > > > > yes, but a couple should be OK to inline > > > > I've got more than a couple now :) > > still, not too many - I find the inlined version easier to parse. but up to you. Acknowledged. https://codereview.chromium.org/2858353002/diff/80001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/media_perception_private/diagnostics/runtest.js (right): https://codereview.chromium.org/2858353002/diff/80001/chrome/test/data/extens... chrome/test/data/extensions/api_test/media_perception_private/diagnostics/runtest.js:8: chrome.mediaPerceptionPrivate.setState( On 2017/05/11 00:38:27, tbarzic wrote: > can you move this to another test, to make sure getDiagnostics does not get > called before the state callback is invoked? > > also, reformat this as: > chrome.mediaPerceptionPrivate.setState({ > status: 'RUNNING' > }, chrome.testCallbackPass(function(state) { > > })) Done. https://codereview.chromium.org/2858353002/diff/80001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/media_perception_private/media_perception/runtest.js (right): https://codereview.chromium.org/2858353002/diff/80001/chrome/test/data/extens... chrome/test/data/extensions/api_test/media_perception_private/media_perception/runtest.js:11: chrome.mediaPerceptionPrivate.setState( On 2017/05/11 00:38:27, tbarzic wrote: > same feedback as previous test Done. https://codereview.chromium.org/2858353002/diff/80001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/media_perception_private/state/runtest.js (right): https://codereview.chromium.org/2858353002/diff/80001/chrome/test/data/extens... chrome/test/data/extensions/api_test/media_perception_private/state/runtest.js:5: let kStatusEnum = 'RUNNING' On 2017/05/11 00:38:27, tbarzic wrote: > can you just create new state object in tests? Done. https://codereview.chromium.org/2858353002/diff/80001/chrome/test/data/extens... chrome/test/data/extensions/api_test/media_perception_private/state/runtest.js:9: state.deviceContext = 'Just a string.'; On 2017/05/11 00:38:27, tbarzic wrote: > can you set this to something little more meaningful? > like: 'device_context'. Done. https://codereview.chromium.org/2858353002/diff/80001/chrome/test/data/extens... chrome/test/data/extensions/api_test/media_perception_private/state/runtest.js:24: state.status = 'UNINITIALIZED'; On 2017/05/11 00:38:27, tbarzic wrote: > what about other states? Done. https://codereview.chromium.org/2858353002/diff/80001/chrome/test/data/extens... chrome/test/data/extensions/api_test/media_perception_private/state/runtest.js:33: const error = 'Only provide deviceContext with SetState RUNNING.'; On 2017/05/11 00:38:27, tbarzic wrote: > can you test there's an error if the current state is RUNNING? That isn't an error case. Setting the desired state to RUNNING when the process is already RUNNING still returns back state RUNNING. https://codereview.chromium.org/2858353002/diff/80001/extensions/browser/api/... File extensions/browser/api/media_perception_private/conversion_utils.h (right): https://codereview.chromium.org/2858353002/diff/80001/extensions/browser/api/... extensions/browser/api/media_perception_private/conversion_utils.h:5: #ifndef EXTENSIONS_BROWSER_API_MEDIA_PERCEPTION_PRIVATE_CONVERSION_UTILS_H_ On 2017/05/11 00:38:27, tbarzic wrote: > s/utils/util/ The file is conversion_utils.h I don't think there is a misspelling here. Are you asking me to rename the file to conversion_util.h? https://codereview.chromium.org/2858353002/diff/80001/extensions/browser/api/... File extensions/browser/api/media_perception_private/media_perception_api_manager.cc (right): https://codereview.chromium.org/2858353002/diff/80001/extensions/browser/api/... extensions/browser/api/media_perception_private/media_perception_api_manager.cc:60: media_perception::State state_unitialized; On 2017/05/11 00:38:27, tbarzic wrote: > can you add a test for this? Done. https://codereview.chromium.org/2858353002/diff/80001/extensions/browser/api/... extensions/browser/api/media_perception_private/media_perception_api_manager.cc:77: if (!analytics_process_running_) { On 2017/05/11 00:38:27, tbarzic wrote: > if (analytics_process_running_) { > SetStateInternal(); > return; > } > > if (state_proto.status() == mri::State::RUNNIN) { > // request start process > return; > } > > // error callback Done. https://codereview.chromium.org/2858353002/diff/80001/extensions/browser/api/... extensions/browser/api/media_perception_private/media_perception_api_manager.cc:81: dbus_client->StartMediaAnalytics( On 2017/05/11 00:38:27, tbarzic wrote: > what if SetState is called before this returns? > e.g. > > SetState(RUNNING) > call UpstartClient > SetState(RUNNING) > > we should probably not make a duplicate StartMediaAnalytics call here As the client of the API, we can be sure to wait for the callback before calling SetState(RUNNING) a second time. However, I do see the value in hardening the API. What do you recommend for ensuring we dont make a duplicate call. A mutex locked member variable in MediaPerceptionAPIManager? https://codereview.chromium.org/2858353002/diff/80001/extensions/browser/api/... extensions/browser/api/media_perception_private/media_perception_api_manager.cc:118: analytics_process_running_ = false; On 2017/05/11 00:38:27, tbarzic wrote: > do you need this? Done. https://codereview.chromium.org/2858353002/diff/80001/extensions/browser/api/... extensions/browser/api/media_perception_private/media_perception_api_manager.cc:135: callback.Run(false, std::move(state)); On 2017/05/11 00:38:27, tbarzic wrote: > instead of passing error code via state, can you change |success| to an enum > with error codes. > > Or just return state (with not success parameter) and forward it to the app The success parameter allows me to easily determine if I need to call Respond(Error()). Would be strange to have another set of success enums in addition to the failure enums in State.Status, especially because we want the analytics process to eventually support providing errors back over the State proto. https://codereview.chromium.org/2858353002/diff/80001/extensions/browser/api/... extensions/browser/api/media_perception_private/media_perception_api_manager.cc:157: LOG(WARNING) << "Router null or no event listener when receiving a " On 2017/05/11 00:38:27, tbarzic wrote: > Remove this log, no listener being set seems like a real possibility here, we > don't want to spew logs if that happens > > I'd change this whole if to: > DCHECK(router); Done. https://codereview.chromium.org/2858353002/diff/80001/extensions/browser/api/... File extensions/browser/api/media_perception_private/media_perception_private_api.cc (right): https://codereview.chromium.org/2858353002/diff/80001/extensions/browser/api/... extensions/browser/api/media_perception_private/media_perception_private_api.cc:19: return "Failed to get or set state because the D-Bus communication " On 2017/05/11 00:38:27, tbarzic wrote: > can you add constants for the error strigns, I think it would make this easier > to read. Done. https://codereview.chromium.org/2858353002/diff/80001/extensions/browser/api/... extensions/browser/api/media_perception_private/media_perception_private_api.cc:72: // Check that the desired status is settable. On 2017/05/11 00:38:27, tbarzic wrote: > I'd remove the comment, it's evident from the code what's going on. Done. https://codereview.chromium.org/2858353002/diff/80001/extensions/browser/api/... extensions/browser/api/media_perception_private/media_perception_private_api.cc:78: // Check that device context is only provided with SetState RUNNING. On 2017/05/11 00:38:27, tbarzic wrote: > nit: new line before this Done. https://codereview.chromium.org/2858353002/diff/80001/extensions/browser/api/... File extensions/browser/api/media_perception_private/media_perception_private_apitest.cc (right): https://codereview.chromium.org/2858353002/diff/80001/extensions/browser/api/... extensions/browser/api/media_perception_private/media_perception_private_apitest.cc:28: void InitializeDbusClient() { On 2017/05/11 00:38:27, tbarzic wrote: > This is not really initializing D bus client, maybe rename this to > FakeMEdiaAnalyticsClient* GetMediaAnalyticsClient() Done. https://codereview.chromium.org/2858353002/diff/80001/extensions/browser/api/... extensions/browser/api/media_perception_private/media_perception_private_apitest.cc:29: dbus_client_ = reinterpret_cast<chromeos::FakeMediaAnalyticsClient*>( On 2017/05/11 00:38:27, tbarzic wrote: > Can you check if you can use DBusThreadManager::GetSetterForTesting to inject a > fake media analytics client? Done. https://codereview.chromium.org/2858353002/diff/80001/extensions/browser/api/... extensions/browser/api/media_perception_private/media_perception_private_apitest.cc:33: ~MediaPerceptionPrivateApiTest() override {} On 2017/05/11 00:38:27, tbarzic wrote: > move this after ctor; > use = default where possible Done. https://codereview.chromium.org/2858353002/diff/80001/extensions/browser/api/... extensions/browser/api/media_perception_private/media_perception_private_apitest.cc:35: chromeos::FakeMediaAnalyticsClient* dbus_client_; On 2017/05/11 00:38:27, tbarzic wrote: > DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/2858353002/diff/80001/extensions/browser/api/... extensions/browser/api/media_perception_private/media_perception_private_apitest.cc:65: dbus_client_->SetFrameId(1); On 2017/05/11 00:38:27, tbarzic wrote: > do you need this for this test? Done.
https://codereview.chromium.org/2858353002/diff/80001/extensions/browser/api/... File extensions/browser/api/media_perception_private/media_perception_api_manager.cc (right): https://codereview.chromium.org/2858353002/diff/80001/extensions/browser/api/... extensions/browser/api/media_perception_private/media_perception_api_manager.cc:81: dbus_client->StartMediaAnalytics( On 2017/05/11 23:57:59, Luke Sorenson wrote: > On 2017/05/11 00:38:27, tbarzic wrote: > > what if SetState is called before this returns? > > e.g. > > > > SetState(RUNNING) > > call UpstartClient > > SetState(RUNNING) > > > > we should probably not make a duplicate StartMediaAnalytics call here > > As the client of the API, we can be sure to wait for the callback before calling > SetState(RUNNING) a second time. However, I do see the value in hardening the > API. What do you recommend for ensuring we dont make a duplicate call. A mutex > locked member variable in MediaPerceptionAPIManager? nope, let's avoid mutex variables. you could change analytics_process_running_ to an enum state: IDLE -> STARTING -> STARTED when you get a request when this state is STARTING either raise an error, or set the target state to the new one, and cache the callback for the state. e.g. if (upstart_state_ == IDLE) { if (state != RUNNING) { // respond unitialized return; } state_after_upstart_ = state; upstart_callback_ = {callback}; // invoke start up media perception } else if (upstart_state_ == STARTING) { state_after_upstart = state; upstart_callbacks_.push_back(callback); } else { SetStateInternal(callback, state_proto); } void UpstartCallback(bool success) { if (success) { SetState( // callback that will run all callback in upstart_callbacks_) return; } else // run all callbacks in upstart_callbacks_ with an error state } https://codereview.chromium.org/2858353002/diff/80001/extensions/browser/api/... extensions/browser/api/media_perception_private/media_perception_api_manager.cc:135: callback.Run(false, std::move(state)); On 2017/05/11 23:57:59, Luke Sorenson wrote: > On 2017/05/11 00:38:27, tbarzic wrote: > > instead of passing error code via state, can you change |success| to an enum > > with error codes. > > > > Or just return state (with not success parameter) and forward it to the app > > The success parameter allows me to easily determine if I need to call > Respond(Error()). Would be strange to have another set of success enums in > addition to the failure enums in State.Status, especially because we want the > analytics process to eventually support providing errors back over the State > proto. well you already have two success enums - only one happens to be {TRUE, FALSE}. why not expand this to report a better error to the app. returning a bool for success, and then stuffing the error code into the |state| so the calling code can fetch it from there is confusing (and conflates dbus service state with failure to request service state to be set). Error reported by analytics process is different - I do not think you should raise extension function error in that case (as you already don't), since setState method succeeded, and it returned actual service state. https://codereview.chromium.org/2858353002/diff/120001/extensions/browser/api... File extensions/browser/api/media_perception_private/conversion_utils.cc (right): https://codereview.chromium.org/2858353002/diff/120001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils.cc:18: p_result->x = base::MakeUnique<double>(point.x()); nit: remove {} for single line ifs/fors through out the file https://codereview.chromium.org/2858353002/diff/120001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils.cc:50: default: do you need this default? If not I'd remove it, so you don;t forget to update this switch if a new value is added. (though, you might want to add NOTREACHED() after the switch) https://codereview.chromium.org/2858353002/diff/120001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils.cc:80: new int(frame_perception.frame_width_in_px())); replace these with MakeUnique https://codereview.chromium.org/2858353002/diff/120001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils.cc:110: default: as before, no default, just list all possible values; and add a NOTREACHED https://codereview.chromium.org/2858353002/diff/120001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils.cc:216: mp_result.frame_perceptions.reset( MakeUnique https://codereview.chromium.org/2858353002/diff/120001/extensions/browser/api... File extensions/browser/api/media_perception_private/media_perception_api_manager.h (right): https://codereview.chromium.org/2858353002/diff/120001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager.h:3: // found in the LICENSE file. can you add some tests for this? e.g. the flow when the state is initially set to RUNNING - that the upstart client gets called test how upstart success/failure is handled how failure to call media perception dbus service is handled etc. fell free to ping me for pointers about how to set up unittests with fake dbus clients/browser context keyed apis. https://codereview.chromium.org/2858353002/diff/120001/extensions/browser/api... File extensions/browser/api/media_perception_private/media_perception_private_api.cc (right): https://codereview.chromium.org/2858353002/diff/120001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_private_api.cc:36: // process. I don't think you need to do this (when an error is reported by the service, it's not really an error in the extension function invocation, so the function should not throw an exception)
Implemented CallbackStatus enum, AnalyticsProcessState enum, and addressed comments. PTAL https://codereview.chromium.org/2858353002/diff/80001/extensions/browser/api/... File extensions/browser/api/media_perception_private/media_perception_api_manager.cc (right): https://codereview.chromium.org/2858353002/diff/80001/extensions/browser/api/... extensions/browser/api/media_perception_private/media_perception_api_manager.cc:81: dbus_client->StartMediaAnalytics( On 2017/05/12 01:00:21, tbarzic wrote: > On 2017/05/11 23:57:59, Luke Sorenson wrote: > > On 2017/05/11 00:38:27, tbarzic wrote: > > > what if SetState is called before this returns? > > > e.g. > > > > > > SetState(RUNNING) > > > call UpstartClient > > > SetState(RUNNING) > > > > > > we should probably not make a duplicate StartMediaAnalytics call here > > > > As the client of the API, we can be sure to wait for the callback before > calling > > SetState(RUNNING) a second time. However, I do see the value in hardening the > > API. What do you recommend for ensuring we dont make a duplicate call. A mutex > > locked member variable in MediaPerceptionAPIManager? > > nope, let's avoid mutex variables. > > you could change analytics_process_running_ to an enum state: > IDLE -> STARTING -> STARTED > > when you get a request when this state is STARTING either raise an error, > or set the target state to the new one, and cache the callback for the state. > > e.g. > if (upstart_state_ == IDLE) { > if (state != RUNNING) { > // respond unitialized > return; > } > state_after_upstart_ = state; > upstart_callback_ = {callback}; > // invoke start up media perception > } else if (upstart_state_ == STARTING) { > state_after_upstart = state; > upstart_callbacks_.push_back(callback); > } else { > SetStateInternal(callback, state_proto); > } > > > void UpstartCallback(bool success) { > if (success) { > SetState( // callback that will run all callback in upstart_callbacks_) > return; > } > else > // run all callbacks in upstart_callbacks_ with an error state > } > > Done. https://codereview.chromium.org/2858353002/diff/80001/extensions/browser/api/... extensions/browser/api/media_perception_private/media_perception_api_manager.cc:135: callback.Run(false, std::move(state)); On 2017/05/12 01:00:21, tbarzic wrote: > On 2017/05/11 23:57:59, Luke Sorenson wrote: > > On 2017/05/11 00:38:27, tbarzic wrote: > > > instead of passing error code via state, can you change |success| to an enum > > > with error codes. > > > > > > Or just return state (with not success parameter) and forward it to the app > > > > The success parameter allows me to easily determine if I need to call > > Respond(Error()). Would be strange to have another set of success enums in > > addition to the failure enums in State.Status, especially because we want the > > analytics process to eventually support providing errors back over the State > > proto. > > well you already have two success enums - only one happens to be {TRUE, FALSE}. > why not expand this to report a better error to the app. > returning a bool for success, and then stuffing the error code into the |state| > so the calling code > can fetch it from there is confusing (and conflates dbus service state with > failure to request service state to be set). > > Error reported by analytics process is different - I do not think you should > raise > extension function error in that case (as you already don't), since setState > method succeeded, > and it returned actual service state. Done. https://codereview.chromium.org/2858353002/diff/120001/extensions/browser/api... File extensions/browser/api/media_perception_private/conversion_utils.cc (right): https://codereview.chromium.org/2858353002/diff/120001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils.cc:18: p_result->x = base::MakeUnique<double>(point.x()); On 2017/05/12 01:00:22, tbarzic wrote: > nit: remove {} for single line ifs/fors through out the file Either is allowed under the style guide and in this case, I think removing {} decreases readability. https://codereview.chromium.org/2858353002/diff/120001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils.cc:50: default: On 2017/05/12 01:00:22, tbarzic wrote: > do you need this default? If not I'd remove it, so you don;t forget to update > this switch if a new value is added. > > (though, you might want to add NOTREACHED() after the switch) Since, we're converting from a proto, where the value is optional, I would prefer not to require type or not allow for UNSPECIFIED. https://codereview.chromium.org/2858353002/diff/120001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils.cc:80: new int(frame_perception.frame_width_in_px())); On 2017/05/12 01:00:22, tbarzic wrote: > replace these with MakeUnique Done. https://codereview.chromium.org/2858353002/diff/120001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils.cc:110: default: On 2017/05/12 01:00:22, tbarzic wrote: > as before, no default, just list all possible values; > and add a NOTREACHED Same answer as above w.r.t proto convensions. https://codereview.chromium.org/2858353002/diff/120001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils.cc:216: mp_result.frame_perceptions.reset( On 2017/05/12 01:00:22, tbarzic wrote: > MakeUnique Done. https://codereview.chromium.org/2858353002/diff/120001/extensions/browser/api... File extensions/browser/api/media_perception_private/media_perception_api_manager.h (right): https://codereview.chromium.org/2858353002/diff/120001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager.h:3: // found in the LICENSE file. On 2017/05/12 01:00:22, tbarzic wrote: > can you add some tests for this? > e.g. the flow when the state is initially set to RUNNING - that the upstart > client gets called > test how upstart success/failure is handled > how failure to call media perception dbus service is handled etc. > > fell free to ping me for pointers about how to set up unittests with fake dbus > clients/browser context keyed apis. Right now the browser API tests cover half of the cases you mention I think. I can add some additional fake dbus client features so that the browser tests can trigger the errors you describe, but I'm not sure that another unittest for this class specifically is required to verify the functionality you describe. What do you think? https://codereview.chromium.org/2858353002/diff/120001/extensions/browser/api... File extensions/browser/api/media_perception_private/media_perception_private_api.cc (right): https://codereview.chromium.org/2858353002/diff/120001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_private_api.cc:36: // process. On 2017/05/12 01:00:22, tbarzic wrote: > I don't think you need to do this (when an error is reported by the service, > it's not really an error in the extension function invocation, so the function > should not throw an exception) Acknowledged. Changed this function to use CallbackStatus enum.
https://codereview.chromium.org/2858353002/diff/120001/extensions/browser/api... File extensions/browser/api/media_perception_private/conversion_utils.cc (right): https://codereview.chromium.org/2858353002/diff/120001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils.cc:18: p_result->x = base::MakeUnique<double>(point.x()); On 2017/05/12 17:07:30, Luke Sorenson wrote: > On 2017/05/12 01:00:22, tbarzic wrote: > > nit: remove {} for single line ifs/fors through out the file > > Either is allowed under the style guide and in this case, I think removing {} > decreases readability. I disagree (though, that might be due to being used to Chrome code base) https://codereview.chromium.org/2858353002/diff/120001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils.cc:50: default: On 2017/05/12 17:07:30, Luke Sorenson wrote: > On 2017/05/12 01:00:22, tbarzic wrote: > > do you need this default? If not I'd remove it, so you don;t forget to update > > this switch if a new value is added. > > > > (though, you might want to add NOTREACHED() after the switch) > > Since, we're converting from a proto, where the value is optional, I would > prefer not to require type or not allow for UNSPECIFIED. Doing this would do the same thing you're doing, but would fail o compile if you add a value to the EntityType enum without updating the conversion: if (entity.has_type()) { switch(entity.type()) { case mri::Entity::FACE: return ...; case mri::Entity::PERSON: return ...; case mri::Entity::UNSPECIFIED: return ...; } NOTREACHED() << "Unknown entity type " << entity.type(); return media_perception::ENTITY_TYPE_UNSPECIFIED;; } return media_perception::ENTITY_TYPE_UNSPECIFIED;; } https://codereview.chromium.org/2858353002/diff/140001/extensions/BUILD.gn File extensions/BUILD.gn (right): https://codereview.chromium.org/2858353002/diff/140001/extensions/BUILD.gn#ne... extensions/BUILD.gn:259: "browser/api/media_perception_private/media_perception_private_apitest.cc", the api tests should be run under extensions_browsertests not browser_tests Please add this to browser tests in extensions/browser/BUILD.gn https://codereview.chromium.org/2858353002/diff/140001/extensions/browser/api... File extensions/browser/api/media_perception_private/media_perception_api_manager.h (right): https://codereview.chromium.org/2858353002/diff/140001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager.h:17: enum CallbackStatus { use enum class extension::CallbackStatus is very generic name - it can easily cause a naming conflict Can you make this public member of MediaPerceptionAPIManager https://codereview.chromium.org/2858353002/diff/140001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager.h:23: PROCESS_IDLE, nit: I'd add ERROR as name prefix or suffix - to make it clear it maps to an error status :) https://codereview.chromium.org/2858353002/diff/140001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager.h:31: enum AnalyticsProcessState { enum class also, make this private member of MediaPerceptionAPIManager https://codereview.chromium.org/2858353002/diff/140001/extensions/browser/api... File extensions/browser/api/media_perception_private/media_perception_private_apitest.cc (right): https://codereview.chromium.org/2858353002/diff/140001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_private_apitest.cc:18: class MediaPerceptionPrivateApiTest : public ExtensionApiTest { please use https://cs.chromium.org/chromium/src/extensions/shell/test/shell_apitest.h
rkc@google.com changed reviewers: + rkc@google.com
https://codereview.chromium.org/2858353002/diff/120001/extensions/browser/api... File extensions/browser/api/media_perception_private/conversion_utils.cc (right): https://codereview.chromium.org/2858353002/diff/120001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils.cc:18: p_result->x = base::MakeUnique<double>(point.x()); On 2017/05/12 18:59:19, tbarzic wrote: > On 2017/05/12 17:07:30, Luke Sorenson wrote: > > On 2017/05/12 01:00:22, tbarzic wrote: > > > nit: remove {} for single line ifs/fors through out the file > > > > Either is allowed under the style guide and in this case, I think removing {} > > decreases readability. > > I disagree (though, that might be due to being used to Chrome code base) Consistency is more important than individual opinion. The prevalent pattern within the Chromium source code is to not use {} for single line if statements. Please follow that to maintain consistency.
https://codereview.chromium.org/2858353002/diff/140001/extensions/browser/api... File extensions/browser/api/media_perception_private/media_perception_private_apitest.cc (right): https://codereview.chromium.org/2858353002/diff/140001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_private_apitest.cc:37: base::WrapUnique(media_analytics_client_)); both bare new and WrapUnique are discouraged in favour of base::MakeUnique. You can do this instead: auto media_analytics_client = base::MakeUnique<FakeMediaClient>(); media_analytics_client_ = media_analytics_client.get(); dbus_setter->SetMediaAnalyticsClient(std::move(media_analytics_client)); https://codereview.chromium.org/2858353002/diff/140001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_private_apitest.cc:76: media_analytics_client_->FireMediaPerceptionEvent(media_perception)); you have to wait for the apitest to send test result message. You can use extensions::ResultCatcher for this: create it before you launch app, and then at the end: EXPECT_TRUE(result_catcher.GetNextResult()) << result_catcher.mesasge();
Addressed comments, actively working to make browser tests function again. https://codereview.chromium.org/2858353002/diff/120001/extensions/browser/api... File extensions/browser/api/media_perception_private/conversion_utils.cc (right): https://codereview.chromium.org/2858353002/diff/120001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils.cc:18: p_result->x = base::MakeUnique<double>(point.x()); On 2017/05/12 19:23:55, rkc1 wrote: > On 2017/05/12 18:59:19, tbarzic wrote: > > On 2017/05/12 17:07:30, Luke Sorenson wrote: > > > On 2017/05/12 01:00:22, tbarzic wrote: > > > > nit: remove {} for single line ifs/fors through out the file > > > > > > Either is allowed under the style guide and in this case, I think removing > {} > > > decreases readability. > > > > I disagree (though, that might be due to being used to Chrome code base) > > Consistency is more important than individual opinion. The prevalent pattern > within the Chromium source code is to not use {} for single line if statements. > Please follow that to maintain consistency. Done. https://codereview.chromium.org/2858353002/diff/120001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils.cc:50: default: On 2017/05/12 18:59:19, tbarzic wrote: > On 2017/05/12 17:07:30, Luke Sorenson wrote: > > On 2017/05/12 01:00:22, tbarzic wrote: > > > do you need this default? If not I'd remove it, so you don;t forget to > update > > > this switch if a new value is added. > > > > > > (though, you might want to add NOTREACHED() after the switch) > > > > Since, we're converting from a proto, where the value is optional, I would > > prefer not to require type or not allow for UNSPECIFIED. > > Doing this would do the same thing you're doing, but would fail o compile if you > add a value to the EntityType enum without updating the conversion: > > if (entity.has_type()) { > switch(entity.type()) { > case mri::Entity::FACE: > return ...; > case mri::Entity::PERSON: > return ...; > case mri::Entity::UNSPECIFIED: > return ...; > } > NOTREACHED() << "Unknown entity type " << entity.type(); > return media_perception::ENTITY_TYPE_UNSPECIFIED;; > } > > return media_perception::ENTITY_TYPE_UNSPECIFIED;; > } Done. https://codereview.chromium.org/2858353002/diff/120001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils.cc:110: default: On 2017/05/12 17:07:30, Luke Sorenson wrote: > On 2017/05/12 01:00:22, tbarzic wrote: > > as before, no default, just list all possible values; > > and add a NOTREACHED > > Same answer as above w.r.t proto convensions. Added NOTREACHED here as well. https://codereview.chromium.org/2858353002/diff/140001/extensions/browser/api... File extensions/browser/api/media_perception_private/media_perception_api_manager.h (right): https://codereview.chromium.org/2858353002/diff/140001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager.h:17: enum CallbackStatus { On 2017/05/12 18:59:20, tbarzic wrote: > use enum class > > extension::CallbackStatus is very generic name - it can easily cause a naming > conflict > Can you make this public member of MediaPerceptionAPIManager Done. https://codereview.chromium.org/2858353002/diff/140001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager.h:23: PROCESS_IDLE, On 2017/05/12 18:59:20, tbarzic wrote: > nit: I'd add ERROR as name prefix or suffix - to make it clear it maps to an > error status :) Done. https://codereview.chromium.org/2858353002/diff/140001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager.h:31: enum AnalyticsProcessState { On 2017/05/12 18:59:20, tbarzic wrote: > enum class > > also, make this private member of MediaPerceptionAPIManager Done. https://codereview.chromium.org/2858353002/diff/140001/extensions/browser/api... File extensions/browser/api/media_perception_private/media_perception_private_apitest.cc (right): https://codereview.chromium.org/2858353002/diff/140001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_private_apitest.cc:18: class MediaPerceptionPrivateApiTest : public ExtensionApiTest { On 2017/05/12 18:59:20, tbarzic wrote: > please use > https://cs.chromium.org/chromium/src/extensions/shell/test/shell_apitest.h Done. https://codereview.chromium.org/2858353002/diff/140001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_private_apitest.cc:37: base::WrapUnique(media_analytics_client_)); On 2017/05/12 19:52:10, tbarzic wrote: > both bare new and WrapUnique are discouraged in favour of base::MakeUnique. > > You can do this instead: > > auto media_analytics_client = base::MakeUnique<FakeMediaClient>(); > media_analytics_client_ = media_analytics_client.get(); > dbus_setter->SetMediaAnalyticsClient(std::move(media_analytics_client)); Done. https://codereview.chromium.org/2858353002/diff/140001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_private_apitest.cc:76: media_analytics_client_->FireMediaPerceptionEvent(media_perception)); On 2017/05/12 19:52:10, tbarzic wrote: > you have to wait for the apitest to send test result message. > > You can use extensions::ResultCatcher for this: > create it before you launch app, and then at the end: > EXPECT_TRUE(result_catcher.GetNextResult()) << result_catcher.mesasge(); Done.
https://codereview.chromium.org/2858353002/diff/120001/extensions/browser/api... File extensions/browser/api/media_perception_private/media_perception_api_manager.h (right): https://codereview.chromium.org/2858353002/diff/120001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager.h:3: // found in the LICENSE file. On 2017/05/12 17:07:31, Luke Sorenson wrote: > On 2017/05/12 01:00:22, tbarzic wrote: > > can you add some tests for this? > > e.g. the flow when the state is initially set to RUNNING - that the upstart > > client gets called > > test how upstart success/failure is handled > > how failure to call media perception dbus service is handled etc. > > > > fell free to ping me for pointers about how to set up unittests with fake dbus > > clients/browser context keyed apis. > > Right now the browser API tests cover half of the cases you mention I think. I > can add some additional fake dbus client features so that the browser tests can > trigger the errors you describe, but I'm not sure that another unittest for this > class specifically is required to verify the functionality you describe. > > What do you think? I'd prefer to test those cases in unit_tests - it would be faster to run and more focused (and I imagine easier to add new test cases, as you would avoid communication layer between test app and test code).
Added MediaPerceptionAPIManagerTest. PTAL https://codereview.chromium.org/2858353002/diff/120001/extensions/browser/api... File extensions/browser/api/media_perception_private/media_perception_api_manager.h (right): https://codereview.chromium.org/2858353002/diff/120001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager.h:3: // found in the LICENSE file. On 2017/05/15 22:05:50, tbarzic wrote: > On 2017/05/12 17:07:31, Luke Sorenson wrote: > > On 2017/05/12 01:00:22, tbarzic wrote: > > > can you add some tests for this? > > > e.g. the flow when the state is initially set to RUNNING - that the upstart > > > client gets called > > > test how upstart success/failure is handled > > > how failure to call media perception dbus service is handled etc. > > > > > > fell free to ping me for pointers about how to set up unittests with fake > dbus > > > clients/browser context keyed apis. > > > > Right now the browser API tests cover half of the cases you mention I think. I > > can add some additional fake dbus client features so that the browser tests > can > > trigger the errors you describe, but I'm not sure that another unittest for > this > > class specifically is required to verify the functionality you describe. > > > > What do you think? > > I'd prefer to test those cases in unit_tests - it would be faster to run and > more focused (and I imagine easier to add new test cases, as you would avoid > communication layer between test app and test code). Done.
lasoren@chromium.org changed reviewers: + stevenjb@chromium.org
Added stevenjb for chromeos/dbus owner review.
https://codereview.chromium.org/2858353002/diff/220001/chromeos/dbus/fake_ups... File chromeos/dbus/fake_upstart_client.h (right): https://codereview.chromium.org/2858353002/diff/220001/chromeos/dbus/fake_ups... chromeos/dbus/fake_upstart_client.h:32: // upstart failure case. no need for the "Used for testing" comment. https://codereview.chromium.org/2858353002/diff/220001/extensions/browser/api... File extensions/browser/api/media_perception_private/media_perception_api_manager_unittest.cc (right): https://codereview.chromium.org/2858353002/diff/220001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager_unittest.cc:44: chromeos::DBusThreadManager::Initialize(); you don't need this :) https://codereview.chromium.org/2858353002/diff/220001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager_unittest.cc:57: std::unique_ptr<content::TestBrowserContext> browser_context_; content::TestBrowserContext browser_context_; https://codereview.chromium.org/2858353002/diff/220001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager_unittest.cc:63: content::TestBrowserThreadBundle thread_bundle_; nit: move up with browser_context https://codereview.chromium.org/2858353002/diff/220001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager_unittest.cc:64: }; Add DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/2858353002/diff/220001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager_unittest.cc:66: void SetStateUpstartFailureCallback(CallbackStatus status, move before test calss, and put it into an anonymous namespace https://codereview.chromium.org/2858353002/diff/220001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager_unittest.cc:75: manager_->SetState(state, base::Bind(&SetStateUpstartFailureCallback)); given that SetState is async you should use run loop here too (e.g. see if the test fails if you change SetStateUpstartFailureCallback to expect something else). What I like to do is: void RecordStatusAndRunClosure(Closure closure, CallbackStatus* status, CallbackStatus result_status, State result_state) { *status = result_status; closure.Run(); } CallbackStatus SetState(state) { base::RunLoop run_loop; CallbackStatus status; manager_->SetState(state, base::Bind(&RecordStatusAndRunClosure, run_loop.QuitClosure(), &status)); run_loop.Run(); return status; } TEST_F() { EXPECT_EQ(STATUS, SetState(state)); } Though, I'm OK with inlining run_loop handing into tests (might make tests easier to follow). https://codereview.chromium.org/2858353002/diff/220001/extensions/test/data/m... File extensions/test/data/media_perception_private/diagnostics/manifest.json (right): https://codereview.chromium.org/2858353002/diff/220001/extensions/test/data/m... extensions/test/data/media_perception_private/diagnostics/manifest.json:16: "kiosk_only": true you don't need kiosk_only
chromeos/dbus lgtm
Addressed comments! PTAL https://codereview.chromium.org/2858353002/diff/220001/chromeos/dbus/fake_ups... File chromeos/dbus/fake_upstart_client.h (right): https://codereview.chromium.org/2858353002/diff/220001/chromeos/dbus/fake_ups... chromeos/dbus/fake_upstart_client.h:32: // upstart failure case. On 2017/05/16 20:56:30, tbarzic wrote: > no need for the "Used for testing" comment. Done. https://codereview.chromium.org/2858353002/diff/220001/extensions/browser/api... File extensions/browser/api/media_perception_private/media_perception_api_manager_unittest.cc (right): https://codereview.chromium.org/2858353002/diff/220001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager_unittest.cc:44: chromeos::DBusThreadManager::Initialize(); On 2017/05/16 20:56:31, tbarzic wrote: > you don't need this :) Done. https://codereview.chromium.org/2858353002/diff/220001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager_unittest.cc:57: std::unique_ptr<content::TestBrowserContext> browser_context_; On 2017/05/16 20:56:31, tbarzic wrote: > content::TestBrowserContext browser_context_; Done. https://codereview.chromium.org/2858353002/diff/220001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager_unittest.cc:63: content::TestBrowserThreadBundle thread_bundle_; On 2017/05/16 20:56:30, tbarzic wrote: > nit: move up with browser_context Done. https://codereview.chromium.org/2858353002/diff/220001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager_unittest.cc:64: }; On 2017/05/16 20:56:31, tbarzic wrote: > Add DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/2858353002/diff/220001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager_unittest.cc:66: void SetStateUpstartFailureCallback(CallbackStatus status, On 2017/05/16 20:56:31, tbarzic wrote: > move before test calss, and put it into an anonymous namespace Done. https://codereview.chromium.org/2858353002/diff/220001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager_unittest.cc:75: manager_->SetState(state, base::Bind(&SetStateUpstartFailureCallback)); On 2017/05/16 20:56:31, tbarzic wrote: > given that SetState is async you should use run loop here too (e.g. see if the > test fails if you change SetStateUpstartFailureCallback to expect something > else). > > What I like to do is: > > void RecordStatusAndRunClosure(Closure closure, > CallbackStatus* status, > CallbackStatus result_status, > State result_state) { > *status = result_status; > closure.Run(); > } > > CallbackStatus SetState(state) { > base::RunLoop run_loop; > CallbackStatus status; > manager_->SetState(state, > base::Bind(&RecordStatusAndRunClosure, > run_loop.QuitClosure(), > &status)); > run_loop.Run(); > return status; > } > > > TEST_F() { > EXPECT_EQ(STATUS, SetState(state)); > } > > > Though, I'm OK with inlining run_loop handing into tests (might make tests > easier to follow). Done. https://codereview.chromium.org/2858353002/diff/220001/extensions/test/data/m... File extensions/test/data/media_perception_private/diagnostics/manifest.json (right): https://codereview.chromium.org/2858353002/diff/220001/extensions/test/data/m... extensions/test/data/media_perception_private/diagnostics/manifest.json:16: "kiosk_only": true On 2017/05/16 20:56:31, tbarzic wrote: > you don't need kiosk_only Done.
https://codereview.chromium.org/2858353002/diff/240001/extensions/browser/api... File extensions/browser/api/media_perception_private/conversion_utils.cc (right): https://codereview.chromium.org/2858353002/diff/240001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils.cc:28: std::unique_ptr<media_perception::BoundingBox> bbox_result = rename this to result or use bounding_box instead of bbox This goes for the rest of the file - you should use full words for names https://codereview.chromium.org/2858353002/diff/240001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils.cc:92: base::MakeUnique<std::vector<media_perception::Entity>>(); I'd consider making the arrays required in the idls, unless the distinction between null and empty arrays is important to the app https://codereview.chromium.org/2858353002/diff/240001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils.cc:93: for (const auto& entity : frame_perception.entity()) { nit: no {} here https://codereview.chromium.org/2858353002/diff/240001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils.cc:148: ps_result.image_frame = base::MakeUnique<media_perception::ImageFrame>( I don't think you need to MakeUnique here = ImageFrameProtoToIdl already returns a unique_ptr https://codereview.chromium.org/2858353002/diff/240001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils.cc:173: default: no default here. also, you should convert the UNSPECIFIED state to something here. https://codereview.chromium.org/2858353002/diff/240001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils.cc:200: default: no default https://codereview.chromium.org/2858353002/diff/240001/extensions/browser/api... File extensions/browser/api/media_perception_private/conversion_utils.h (right): https://codereview.chromium.org/2858353002/diff/240001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils.h:11: namespace media_perception = extensions::api::media_perception_private; Use full namespace in this file. https://codereview.chromium.org/2858353002/diff/240001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils.h:16: // media_perception_private.idl) to be returned to the JavaScript frontend. nit: remove "to be returned ..." (here and elsewhere in the file) https://codereview.chromium.org/2858353002/diff/240001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils.h:17: media_perception::State StateProtoToIdl(const mri::State& state); nit: maybe wrap these with media_perception namespace https://codereview.chromium.org/2858353002/diff/240001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils.h:23: // Converts incoming (over D-Bus channel) MediaPerception proto messages nit: Converts MediaPerception proto to ... https://codereview.chromium.org/2858353002/diff/240001/extensions/browser/api... File extensions/browser/api/media_perception_private/conversion_utils_unittest.cc (right): https://codereview.chromium.org/2858353002/diff/240001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils_unittest.cc:22: frame_perception->set_frame_height_in_px(4); move helper methods to namespace {} https://codereview.chromium.org/2858353002/diff/240001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils_unittest.cc:34: b_one->mutable_top_left()->set_y(11); can you tests cases where some of these are null? https://codereview.chromium.org/2858353002/diff/240001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils_unittest.cc:93: TEST(ConversionUtilsTest, MediaPerceptionProtoToIdl) { rename tests to MediaPerceptionConversionUtilsTest https://codereview.chromium.org/2858353002/diff/240001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils_unittest.cc:100: media_perception::MediaPerception mp_result = add a new line after initialization is done also, rename the variable (mp is not that meaningful) https://codereview.chromium.org/2858353002/diff/240001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils_unittest.cc:103: const auto& fp_result = (*mp_result.frame_perceptions)[0]; assert that frame perception is set and its size is 1 Also, just call VelidateFramePerceptionResult(mp_result.frame_perceptions->at(0)); https://codereview.chromium.org/2858353002/diff/240001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils_unittest.cc:120: ASSERT_THAT(*d_result.perception_samples, testing::SizeIs(kNumSamples)); ASSERT_EQ(kNumSamples, d_result.perception_samples->size()); https://codereview.chromium.org/2858353002/diff/240001/extensions/browser/api... File extensions/browser/api/media_perception_private/media_perception_api_manager.cc (right): https://codereview.chromium.org/2858353002/diff/240001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager.cc:66: if (analytics_process_state_ == AnalyticsProcessState::LAUNCHING) { this could probably be uninitialized as well https://codereview.chromium.org/2858353002/diff/240001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager.cc:83: << "Cannot set state to something other than RUNNING or SUSPENDED."; nit: new line here https://codereview.chromium.org/2858353002/diff/240001/extensions/browser/api... File extensions/browser/api/media_perception_private/media_perception_api_manager.h (right): https://codereview.chromium.org/2858353002/diff/240001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager.h:13: namespace media_perception = extensions::api::media_perception_private; remove this https://codereview.chromium.org/2858353002/diff/240001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager.h:30: // for reformat the comment. https://codereview.chromium.org/2858353002/diff/240001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager.h:44: // Abstracts away D-Bus communication and underlying protos. remove this line https://codereview.chromium.org/2858353002/diff/240001/extensions/browser/api... File extensions/browser/api/media_perception_private/media_perception_api_manager_unittest.cc (right): https://codereview.chromium.org/2858353002/diff/240001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager_unittest.cc:100: } Can you add a test for SetState/GetState while UpstartClient is starting media analytics process https://codereview.chromium.org/2858353002/diff/240001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager_unittest.cc:108: EXPECT_EQ(CallbackStatus::DBUS_ERROR, GetState(manager_.get())); test what happens with set state in this case. https://codereview.chromium.org/2858353002/diff/240001/extensions/browser/api... File extensions/browser/api/media_perception_private/media_perception_private_api.cc (right): https://codereview.chromium.org/2858353002/diff/240001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_private_api.cc:18: "API call failed because the D-Bus communication with the analytics " can you make the error message less verbose. e.g. Service unreachable. Service not running. Service busy (or Service starting) https://codereview.chromium.org/2858353002/diff/240001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_private_api.cc:53: DCHECK(manager) << "Can't get manager."; I think you can remove the DCHECK. If it's null this will just crash. https://codereview.chromium.org/2858353002/diff/240001/extensions/browser/api... File extensions/browser/api/media_perception_private/media_perception_private_apitest.cc (right): https://codereview.chromium.org/2858353002/diff/240001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_private_apitest.cc:48: private: https://codereview.chromium.org/2858353002/diff/240001/extensions/test/data/m... File extensions/test/data/media_perception_private/diagnostics/runtest.js (right): https://codereview.chromium.org/2858353002/diff/240001/extensions/test/data/m... extensions/test/data/media_perception_private/diagnostics/runtest.js:11: chrome.test.assertEq(state.status, 'RUNNING'); assertEq({status: 'RUNNING'}, state); (and generally, the expected value should be the first arg) https://codereview.chromium.org/2858353002/diff/240001/extensions/test/data/m... extensions/test/data/media_perception_private/diagnostics/runtest.js:18: response.perceptionSamples[0].framePerception.frameId, 1); assertEq({ perceptionSamples: [{ framePerception: { frameId: 1 } }] }, response); And I'd consider adding more data to the diagnostics data https://codereview.chromium.org/2858353002/diff/240001/extensions/test/data/m... File extensions/test/data/media_perception_private/state/runtest.js (right): https://codereview.chromium.org/2858353002/diff/240001/extensions/test/data/m... extensions/test/data/media_perception_private/state/runtest.js:5: remove extra new line
Thanks Toni, addressed comments, PTAL https://codereview.chromium.org/2858353002/diff/240001/extensions/browser/api... File extensions/browser/api/media_perception_private/conversion_utils.cc (right): https://codereview.chromium.org/2858353002/diff/240001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils.cc:28: std::unique_ptr<media_perception::BoundingBox> bbox_result = On 2017/05/17 21:04:41, tbarzic wrote: > rename this to result > or use > bounding_box instead of bbox > > This goes for the rest of the file - you should use full words for names Done. https://codereview.chromium.org/2858353002/diff/240001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils.cc:92: base::MakeUnique<std::vector<media_perception::Entity>>(); On 2017/05/17 21:04:41, tbarzic wrote: > I'd consider making the arrays required in the idls, unless the distinction > between null and empty arrays is important to the app The distinction is important. It's the difference between no detected entities and the face detection module not having run at all. https://codereview.chromium.org/2858353002/diff/240001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils.cc:93: for (const auto& entity : frame_perception.entity()) { On 2017/05/17 21:04:41, tbarzic wrote: > nit: no {} here Done. https://codereview.chromium.org/2858353002/diff/240001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils.cc:148: ps_result.image_frame = base::MakeUnique<media_perception::ImageFrame>( On 2017/05/17 21:04:41, tbarzic wrote: > I don't think you need to MakeUnique here = ImageFrameProtoToIdl already returns > a unique_ptr I need it because ImageFrameProtoToIdl returns an ImageFrame object not a unique ptr. https://codereview.chromium.org/2858353002/diff/240001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils.cc:173: default: On 2017/05/17 21:04:41, tbarzic wrote: > no default here. > also, you should convert the UNSPECIFIED state to something here. Removed default. Not sure that I should be converting UNSPECIFIED to something else? https://codereview.chromium.org/2858353002/diff/240001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils.cc:200: default: On 2017/05/17 21:04:41, tbarzic wrote: > no default Are you saying to remove the NOTREACHED() as well? https://codereview.chromium.org/2858353002/diff/240001/extensions/browser/api... File extensions/browser/api/media_perception_private/conversion_utils.h (right): https://codereview.chromium.org/2858353002/diff/240001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils.h:11: namespace media_perception = extensions::api::media_perception_private; On 2017/05/17 21:04:41, tbarzic wrote: > Use full namespace in this file. Done. https://codereview.chromium.org/2858353002/diff/240001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils.h:16: // media_perception_private.idl) to be returned to the JavaScript frontend. On 2017/05/17 21:04:41, tbarzic wrote: > nit: remove "to be returned ..." > (here and elsewhere in the file) Done. https://codereview.chromium.org/2858353002/diff/240001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils.h:17: media_perception::State StateProtoToIdl(const mri::State& state); On 2017/05/17 21:04:41, tbarzic wrote: > nit: maybe wrap these with media_perception namespace Done. https://codereview.chromium.org/2858353002/diff/240001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils.h:23: // Converts incoming (over D-Bus channel) MediaPerception proto messages On 2017/05/17 21:04:41, tbarzic wrote: > nit: > Converts MediaPerception proto to ... Done. https://codereview.chromium.org/2858353002/diff/240001/extensions/browser/api... File extensions/browser/api/media_perception_private/conversion_utils_unittest.cc (right): https://codereview.chromium.org/2858353002/diff/240001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils_unittest.cc:22: frame_perception->set_frame_height_in_px(4); On 2017/05/17 21:04:42, tbarzic wrote: > move helper methods to namespace {} Done. https://codereview.chromium.org/2858353002/diff/240001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils_unittest.cc:34: b_one->mutable_top_left()->set_y(11); On 2017/05/17 21:04:41, tbarzic wrote: > can you tests cases where some of these are null? Done. https://codereview.chromium.org/2858353002/diff/240001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils_unittest.cc:93: TEST(ConversionUtilsTest, MediaPerceptionProtoToIdl) { On 2017/05/17 21:04:41, tbarzic wrote: > rename tests to MediaPerceptionConversionUtilsTest Done. https://codereview.chromium.org/2858353002/diff/240001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils_unittest.cc:100: media_perception::MediaPerception mp_result = On 2017/05/17 21:04:41, tbarzic wrote: > add a new line after initialization is done > > also, rename the variable (mp is not that meaningful) Done. https://codereview.chromium.org/2858353002/diff/240001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils_unittest.cc:103: const auto& fp_result = (*mp_result.frame_perceptions)[0]; On 2017/05/17 21:04:41, tbarzic wrote: > assert that frame perception is set and its size is 1 > > Also, just call > VelidateFramePerceptionResult(mp_result.frame_perceptions->at(0)); Done. https://codereview.chromium.org/2858353002/diff/240001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils_unittest.cc:120: ASSERT_THAT(*d_result.perception_samples, testing::SizeIs(kNumSamples)); On 2017/05/17 21:04:41, tbarzic wrote: > ASSERT_EQ(kNumSamples, d_result.perception_samples->size()); For testing the size of stl objects, I think I am supposed to use testing::SizeIs https://codereview.chromium.org/2858353002/diff/240001/extensions/browser/api... File extensions/browser/api/media_perception_private/media_perception_api_manager.cc (right): https://codereview.chromium.org/2858353002/diff/240001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager.cc:66: if (analytics_process_state_ == AnalyticsProcessState::LAUNCHING) { On 2017/05/17 21:04:42, tbarzic wrote: > this could probably be uninitialized as well Prefer PROCESS_LAUNCING_ERROR because then the frontend knows that that RUNNING was already called and the process is starting. https://codereview.chromium.org/2858353002/diff/240001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager.cc:83: << "Cannot set state to something other than RUNNING or SUSPENDED."; On 2017/05/17 21:04:42, tbarzic wrote: > nit: new line here Done. https://codereview.chromium.org/2858353002/diff/240001/extensions/browser/api... File extensions/browser/api/media_perception_private/media_perception_api_manager.h (right): https://codereview.chromium.org/2858353002/diff/240001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager.h:13: namespace media_perception = extensions::api::media_perception_private; On 2017/05/17 21:04:42, tbarzic wrote: > remove this Done. https://codereview.chromium.org/2858353002/diff/240001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager.h:30: // for On 2017/05/17 21:04:42, tbarzic wrote: > reformat the comment. Done. https://codereview.chromium.org/2858353002/diff/240001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager.h:44: // Abstracts away D-Bus communication and underlying protos. On 2017/05/17 21:04:42, tbarzic wrote: > remove this line Done. https://codereview.chromium.org/2858353002/diff/240001/extensions/browser/api... File extensions/browser/api/media_perception_private/media_perception_api_manager_unittest.cc (right): https://codereview.chromium.org/2858353002/diff/240001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager_unittest.cc:100: } On 2017/05/17 21:04:42, tbarzic wrote: > Can you add a test for SetState/GetState while UpstartClient is starting media > analytics process Done. https://codereview.chromium.org/2858353002/diff/240001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager_unittest.cc:108: EXPECT_EQ(CallbackStatus::DBUS_ERROR, GetState(manager_.get())); On 2017/05/17 21:04:42, tbarzic wrote: > test what happens with set state in this case. Done. https://codereview.chromium.org/2858353002/diff/240001/extensions/browser/api... File extensions/browser/api/media_perception_private/media_perception_private_api.cc (right): https://codereview.chromium.org/2858353002/diff/240001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_private_api.cc:18: "API call failed because the D-Bus communication with the analytics " On 2017/05/17 21:04:42, tbarzic wrote: > can you make the error message less verbose. > > e.g. > Service unreachable. > > Service not running. > > Service busy (or Service starting) Done. https://codereview.chromium.org/2858353002/diff/240001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_private_api.cc:53: DCHECK(manager) << "Can't get manager."; On 2017/05/17 21:04:42, tbarzic wrote: > I think you can remove the DCHECK. If it's null this will just crash. Done. https://codereview.chromium.org/2858353002/diff/240001/extensions/browser/api... File extensions/browser/api/media_perception_private/media_perception_private_apitest.cc (right): https://codereview.chromium.org/2858353002/diff/240001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_private_apitest.cc:48: On 2017/05/17 21:04:42, tbarzic wrote: > private: Done. https://codereview.chromium.org/2858353002/diff/240001/extensions/test/data/m... File extensions/test/data/media_perception_private/diagnostics/runtest.js (right): https://codereview.chromium.org/2858353002/diff/240001/extensions/test/data/m... extensions/test/data/media_perception_private/diagnostics/runtest.js:11: chrome.test.assertEq(state.status, 'RUNNING'); On 2017/05/17 21:04:42, tbarzic wrote: > assertEq({status: 'RUNNING'}, state); > > (and generally, the expected value should be the first arg) Done. https://codereview.chromium.org/2858353002/diff/240001/extensions/test/data/m... extensions/test/data/media_perception_private/diagnostics/runtest.js:18: response.perceptionSamples[0].framePerception.frameId, 1); On 2017/05/17 21:04:42, tbarzic wrote: > assertEq({ > perceptionSamples: [{ > framePerception: { > frameId: 1 > } > }] > }, response); > > And I'd consider adding more data to the diagnostics data I think that is covered in the ProtoToIdl test. Otherwise, adding more data here is just verifying that the idl -> JS dictionary conversion works. https://codereview.chromium.org/2858353002/diff/240001/extensions/test/data/m... File extensions/test/data/media_perception_private/state/runtest.js (right): https://codereview.chromium.org/2858353002/diff/240001/extensions/test/data/m... extensions/test/data/media_perception_private/state/runtest.js:5: On 2017/05/17 21:04:42, tbarzic wrote: > remove extra new line Done.
https://codereview.chromium.org/2858353002/diff/240001/extensions/browser/api... File extensions/browser/api/media_perception_private/conversion_utils_unittest.cc (right): https://codereview.chromium.org/2858353002/diff/240001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils_unittest.cc:120: ASSERT_THAT(*d_result.perception_samples, testing::SizeIs(kNumSamples)); On 2017/05/17 23:07:48, Luke Sorenson wrote: > On 2017/05/17 21:04:41, tbarzic wrote: > > ASSERT_EQ(kNumSamples, d_result.perception_samples->size()); > > For testing the size of stl objects, I think I am supposed to use > testing::SizeIs testing::SizeIs is not really common in Chrome - it seems to be used only in handful of tests. https://codereview.chromium.org/2858353002/diff/260001/chromeos/dbus/fake_ups... File chromeos/dbus/fake_upstart_client.h (right): https://codereview.chromium.org/2858353002/diff/260001/chromeos/dbus/fake_ups... chromeos/dbus/fake_upstart_client.h:33: bool start_media_analytics_will_succeed_; So, I'd do this differently (my main concern is that we don';t really want to test start-up stalling - we want the request to be eventually handled) :) I'd change StartMediaAnalytics() to enqueue "start requests", and provide a hook for a test to verify the request was received, and to run response: (though, instead of adding this to FakeUpstartClient, I'd consider creating a test FakeUpstartClient override specific to your tests - if you bake this here, you'll probably want to guard this behavior behind a bool variable) i.e. std::queue<UpstartCallback> pending_upstart_requests_; void FakeUpstartClient::StartMediaAnalyticsClient(callback) { peding_upstart_requets_.push(callback); } bool FakeUpstartClient::HandleNextUpstart(bool succeed) { if (pending_upstart_requests_.empty()) return false; UpstartCallback callback = pending_upstart_requests_.front(); pending_upstart_requests_.pop(); if (!succeed) { callback.Run(succeed); return true; } //default StartMediaAnalytics(callback); } https://codereview.chromium.org/2858353002/diff/260001/chromeos/dbus/proto/me... File chromeos/dbus/proto/media_perception.proto (left): https://codereview.chromium.org/2858353002/diff/260001/chromeos/dbus/proto/me... chromeos/dbus/proto/media_perception.proto:34: TIMEOUT = 1; // Unable to reach media analysis process. Do you have to do this? Even if it's unused, it's weird to see status value jump from 0 to 2. https://codereview.chromium.org/2858353002/diff/260001/extensions/browser/api... File extensions/browser/api/media_perception_private/conversion_utils.cc (right): https://codereview.chromium.org/2858353002/diff/260001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils.cc:126: if (image_frame.has_data_length()) nit: {} now that the body has more than a line (similarly for few other places) https://codereview.chromium.org/2858353002/diff/260001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils.cc:173: NOTREACHED() << "Status not set."; no NOTREACHED here - STATUS_UNSPECIFIED seems to be a valid status value; let the app deal with it and I still think you should set state to something here - e.g. introduce new enum value - UNKNOWN https://codereview.chromium.org/2858353002/diff/260001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils.cc:175: } add notreached after the switch (and this would be easier to do if you moved this to a helper method) StatusProtoToIdl() { switch() { case x: return X; ... } NOTREACHED(); return; } https://codereview.chromium.org/2858353002/diff/260001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils.cc:199: default: put notreached after the switch (see my comment for StateProtoToIdl) But yeah, Id prefer no NOTREACHED over adding a default here https://codereview.chromium.org/2858353002/diff/260001/extensions/browser/api... File extensions/browser/api/media_perception_private/media_perception_api_manager.cc (right): https://codereview.chromium.org/2858353002/diff/260001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager.cc:135: LOG(ERROR) << "Failed to start media analytics process via Upstart."; Can you remove this log - if you really want to report this state, reporting a special error would be more useful (e.g. PROCESS_FAILED_TO_START_ERROR)
Added TestUpstartClient and addressed other comments! PTAL https://codereview.chromium.org/2858353002/diff/240001/extensions/browser/api... File extensions/browser/api/media_perception_private/conversion_utils_unittest.cc (right): https://codereview.chromium.org/2858353002/diff/240001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils_unittest.cc:120: ASSERT_THAT(*d_result.perception_samples, testing::SizeIs(kNumSamples)); On 2017/05/18 19:28:01, tbarzic wrote: > On 2017/05/17 23:07:48, Luke Sorenson wrote: > > On 2017/05/17 21:04:41, tbarzic wrote: > > > ASSERT_EQ(kNumSamples, d_result.perception_samples->size()); > > > > For testing the size of stl objects, I think I am supposed to use > > testing::SizeIs > > testing::SizeIs is not really common in Chrome - it seems to be used only in > handful of tests. Done. https://codereview.chromium.org/2858353002/diff/260001/chromeos/dbus/fake_ups... File chromeos/dbus/fake_upstart_client.h (right): https://codereview.chromium.org/2858353002/diff/260001/chromeos/dbus/fake_ups... chromeos/dbus/fake_upstart_client.h:33: bool start_media_analytics_will_succeed_; On 2017/05/18 19:28:01, tbarzic wrote: > So, I'd do this differently (my main concern is that we don';t really want to > test start-up stalling - we want the request to be eventually handled) :) > > I'd change StartMediaAnalytics() to enqueue "start requests", > and provide a hook for a test to verify the request was received, and to run > response: > (though, instead of adding this to FakeUpstartClient, I'd consider creating a > test FakeUpstartClient override specific to your tests - if you bake this here, > you'll probably want to guard this behavior behind a bool variable) > > i.e. > > std::queue<UpstartCallback> pending_upstart_requests_; > > void FakeUpstartClient::StartMediaAnalyticsClient(callback) { > peding_upstart_requets_.push(callback); > } > > bool FakeUpstartClient::HandleNextUpstart(bool succeed) { > if (pending_upstart_requests_.empty()) > return false; > > UpstartCallback callback = pending_upstart_requests_.front(); > pending_upstart_requests_.pop(); > > if (!succeed) { > callback.Run(succeed); > return true; > } > > //default StartMediaAnalytics(callback); > } Done. https://codereview.chromium.org/2858353002/diff/260001/chromeos/dbus/proto/me... File chromeos/dbus/proto/media_perception.proto (left): https://codereview.chromium.org/2858353002/diff/260001/chromeos/dbus/proto/me... chromeos/dbus/proto/media_perception.proto:34: TIMEOUT = 1; // Unable to reach media analysis process. On 2017/05/18 19:28:01, tbarzic wrote: > Do you have to do this? > > Even if it's unused, it's weird to see status value jump from 0 to 2. Sorry, forgot to adjust the numbers as well. Making this proto match the google3 version. https://codereview.chromium.org/2858353002/diff/260001/extensions/browser/api... File extensions/browser/api/media_perception_private/conversion_utils.cc (right): https://codereview.chromium.org/2858353002/diff/260001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils.cc:126: if (image_frame.has_data_length()) On 2017/05/18 19:28:01, tbarzic wrote: > nit: {} now that the body has more than a line (similarly for few other places) Done. This is why we just include them regardless in non-Chrome land :) https://codereview.chromium.org/2858353002/diff/260001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils.cc:173: NOTREACHED() << "Status not set."; On 2017/05/18 19:28:02, tbarzic wrote: > no NOTREACHED here - STATUS_UNSPECIFIED seems to be a valid status value; let > the app deal with it > > and I still think you should set state to something here - e.g. introduce new > enum value - UNKNOWN Done. https://codereview.chromium.org/2858353002/diff/260001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils.cc:175: } On 2017/05/18 19:28:01, tbarzic wrote: > add notreached after the switch > > (and this would be easier to do if you moved this to a helper method) > > StatusProtoToIdl() { > switch() { > case x: > return X; > ... > } > > NOTREACHED(); > return; > } Done. https://codereview.chromium.org/2858353002/diff/260001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils.cc:199: default: On 2017/05/18 19:28:01, tbarzic wrote: > put notreached after the switch (see my comment for StateProtoToIdl) > > But yeah, Id prefer no NOTREACHED over adding a default here Done. https://codereview.chromium.org/2858353002/diff/260001/extensions/browser/api... File extensions/browser/api/media_perception_private/media_perception_api_manager.cc (right): https://codereview.chromium.org/2858353002/diff/260001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager.cc:135: LOG(ERROR) << "Failed to start media analytics process via Upstart."; On 2017/05/18 19:28:02, tbarzic wrote: > Can you remove this log - if you really want to report this state, reporting a > special error would be more useful > (e.g. PROCESS_FAILED_TO_START_ERROR) Done.
https://codereview.chromium.org/2858353002/diff/280001/chromeos/dbus/fake_ups... File chromeos/dbus/fake_upstart_client.h (left): https://codereview.chromium.org/2858353002/diff/280001/chromeos/dbus/fake_ups... chromeos/dbus/fake_upstart_client.h:28: private: keep this https://codereview.chromium.org/2858353002/diff/280001/chromeos/dbus/test_ups... File chromeos/dbus/test_upstart_client.h (right): https://codereview.chromium.org/2858353002/diff/280001/chromeos/dbus/test_ups... chromeos/dbus/test_upstart_client.h:10: #include "chromeos/dbus/fake_upstart_client.h" can you make this class private to the test that uses it - i.e. move it to media_perception_manager_unittest.cc https://codereview.chromium.org/2858353002/diff/280001/extensions/browser/api... File extensions/browser/api/media_perception_private/media_perception_api_manager_unittest.cc (right): https://codereview.chromium.org/2858353002/diff/280001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager_unittest.cc:39: CallbackStatus SetState(MediaPerceptionAPIManager* manager, nit: consider renaming this to SetStateAndWaitForResponse (same with GetState) https://codereview.chromium.org/2858353002/diff/280001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager_unittest.cc:114: manager_->SetState(state, base::Bind(&StallStateCallback)); Pass in RecordStatusAndRunClosure, and make sure this returns when you call HandleNextUpstartRequest https://codereview.chromium.org/2858353002/diff/280001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager_unittest.cc:130: auto upstart_client = base::MakeUnique<chromeos::FakeUpstartClient>(); Can you use TestUpstartClient here as well (but have it respond automatically) TestUpstartClient : public FakeUpstartClient { // Call this from UpstartStall/UpstartFailure void set_enqueue_requests(bool enqueue_requests) {enqueue_requests_ = enqueue_requests; } private: bool enqueue_requests_ = false; } Or just don't use SetState in this test.
Addressed comments. PTAL https://codereview.chromium.org/2858353002/diff/280001/chromeos/dbus/fake_ups... File chromeos/dbus/fake_upstart_client.h (left): https://codereview.chromium.org/2858353002/diff/280001/chromeos/dbus/fake_ups... chromeos/dbus/fake_upstart_client.h:28: private: On 2017/05/18 21:39:15, tbarzic wrote: > keep this Done. https://codereview.chromium.org/2858353002/diff/280001/chromeos/dbus/test_ups... File chromeos/dbus/test_upstart_client.h (right): https://codereview.chromium.org/2858353002/diff/280001/chromeos/dbus/test_ups... chromeos/dbus/test_upstart_client.h:10: #include "chromeos/dbus/fake_upstart_client.h" On 2017/05/18 21:39:15, tbarzic wrote: > can you make this class private to the test that uses it - i.e. move it to > media_perception_manager_unittest.cc Done. https://codereview.chromium.org/2858353002/diff/280001/extensions/browser/api... File extensions/browser/api/media_perception_private/media_perception_api_manager_unittest.cc (right): https://codereview.chromium.org/2858353002/diff/280001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager_unittest.cc:39: CallbackStatus SetState(MediaPerceptionAPIManager* manager, On 2017/05/18 21:39:15, tbarzic wrote: > nit: consider renaming this to SetStateAndWaitForResponse > > (same with GetState) Done. https://codereview.chromium.org/2858353002/diff/280001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager_unittest.cc:114: manager_->SetState(state, base::Bind(&StallStateCallback)); On 2017/05/18 21:39:15, tbarzic wrote: > Pass in RecordStatusAndRunClosure, and make sure this returns when you call > HandleNextUpstartRequest Done. https://codereview.chromium.org/2858353002/diff/280001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager_unittest.cc:130: auto upstart_client = base::MakeUnique<chromeos::FakeUpstartClient>(); On 2017/05/18 21:39:15, tbarzic wrote: > Can you use TestUpstartClient here as well (but have it respond automatically) > > TestUpstartClient : public FakeUpstartClient { > > // Call this from UpstartStall/UpstartFailure > void set_enqueue_requests(bool enqueue_requests) {enqueue_requests_ = > enqueue_requests; } > private: > bool enqueue_requests_ = false; > } > > > Or just don't use SetState in this test. Done.
mostly looks good https://codereview.chromium.org/2858353002/diff/300001/chromeos/BUILD.gn File chromeos/BUILD.gn (right): https://codereview.chromium.org/2858353002/diff/300001/chromeos/BUILD.gn#newc... chromeos/BUILD.gn:259: "dbus/test_upstart_client.h", rm https://codereview.chromium.org/2858353002/diff/300001/extensions/browser/api... File extensions/browser/api/media_perception_private/conversion_utils.cc (right): https://codereview.chromium.org/2858353002/diff/300001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils.cc:83: nit: to be consistent with the rest of the function, rm nl https://codereview.chromium.org/2858353002/diff/300001/extensions/browser/api... File extensions/browser/api/media_perception_private/conversion_utils_unittest.cc (right): https://codereview.chromium.org/2858353002/diff/300001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils_unittest.cc:15: namespace { nit: nl https://codereview.chromium.org/2858353002/diff/300001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils_unittest.cc:23: mri::Entity* entity_one = frame_perception->add_entity(); nit: nl around different items https://codereview.chromium.org/2858353002/diff/300001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils_unittest.cc:102: CHECK(media_perception_result.frame_perceptions); no CHECKS in test code -> ASSERT_TRUE() https://codereview.chromium.org/2858353002/diff/300001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils_unittest.cc:103: ASSERT_THAT(1, media_perception_result.frame_perceptions->size()); ASSERT_EQ https://codereview.chromium.org/2858353002/diff/300001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils_unittest.cc:122: ASSERT_THAT(kNumSamples, diagnostics_result.perception_samples->size()); ASSERT_EQ https://codereview.chromium.org/2858353002/diff/300001/extensions/browser/api... File extensions/browser/api/media_perception_private/media_perception_api_manager.cc (right): https://codereview.chromium.org/2858353002/diff/300001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager.cc:75: callback.Run(CallbackStatus::SUCCESS, std::move(state_unitialized)); typo: uninitialized https://codereview.chromium.org/2858353002/diff/300001/extensions/browser/api... File extensions/browser/api/media_perception_private/media_perception_api_manager.h (right): https://codereview.chromium.org/2858353002/diff/300001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager.h:20: enum class CallbackStatus { nit: move this before ctor/dtor https://codereview.chromium.org/2858353002/diff/300001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager.h:27: // The media analytics process has been launched via Upstart, but waiting nit: The media analytics process is still being launched via Upstart service. https://codereview.chromium.org/2858353002/diff/300001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager.h:41: using APIStateCallback = base::Callback<void( nit: callback typedefs up, after enums https://codereview.chromium.org/2858353002/diff/300001/extensions/browser/api... File extensions/browser/api/media_perception_private/media_perception_api_manager_unittest.cc (right): https://codereview.chromium.org/2858353002/diff/300001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager_unittest.cc:25: class CHROMEOS_EXPORT TestUpstartClient : public FakeUpstartClient { no export, move the class to namespace {} (inside chromeos namespace) https://codereview.chromium.org/2858353002/diff/300001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager_unittest.cc:62: TestUpstartClient::TestUpstartClient() : enqueue_requests_(false) {} you can just inline these https://codereview.chromium.org/2858353002/diff/300001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager_unittest.cc:140: content::TestBrowserContext browser_context_; add private: and move parts that are not needed by tests to private block (e.g. browser_context_, thread_bundle_) https://codereview.chromium.org/2858353002/diff/300001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager_unittest.cc:161: EXPECT_EQ(CallbackStatus::PROCESS_IDLE_ERROR, status); verify that SetState(RUNNING) again will go through to the upstart client https://codereview.chromium.org/2858353002/diff/300001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager_unittest.cc:173: EXPECT_EQ(CallbackStatus::PROCESS_LAUNCHING_ERROR, add a new line before this https://codereview.chromium.org/2858353002/diff/300001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager_unittest.cc:179: EXPECT_EQ(SUCCESS, status); https://codereview.chromium.org/2858353002/diff/300001/extensions/browser/api... File extensions/browser/api/media_perception_private/media_perception_private_api.cc (right): https://codereview.chromium.org/2858353002/diff/300001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_private_api.cc:18: you can remove lines between error consts https://codereview.chromium.org/2858353002/diff/300001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_private_api.cc:31: default: no default
Thanks! Addressed comments. PTAL https://codereview.chromium.org/2858353002/diff/300001/chromeos/BUILD.gn File chromeos/BUILD.gn (right): https://codereview.chromium.org/2858353002/diff/300001/chromeos/BUILD.gn#newc... chromeos/BUILD.gn:259: "dbus/test_upstart_client.h", On 2017/05/19 03:16:15, tbarzic wrote: > rm Done. https://codereview.chromium.org/2858353002/diff/300001/extensions/browser/api... File extensions/browser/api/media_perception_private/conversion_utils.cc (right): https://codereview.chromium.org/2858353002/diff/300001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils.cc:83: On 2017/05/19 03:16:15, tbarzic wrote: > nit: to be consistent with the rest of the function, rm nl Done. https://codereview.chromium.org/2858353002/diff/300001/extensions/browser/api... File extensions/browser/api/media_perception_private/conversion_utils_unittest.cc (right): https://codereview.chromium.org/2858353002/diff/300001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils_unittest.cc:15: namespace { On 2017/05/19 03:16:15, tbarzic wrote: > nit: nl Done. https://codereview.chromium.org/2858353002/diff/300001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils_unittest.cc:23: mri::Entity* entity_one = frame_perception->add_entity(); On 2017/05/19 03:16:15, tbarzic wrote: > nit: nl around different items Done. https://codereview.chromium.org/2858353002/diff/300001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils_unittest.cc:102: CHECK(media_perception_result.frame_perceptions); On 2017/05/19 03:16:15, tbarzic wrote: > no CHECKS in test code -> ASSERT_TRUE() Done. https://codereview.chromium.org/2858353002/diff/300001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils_unittest.cc:103: ASSERT_THAT(1, media_perception_result.frame_perceptions->size()); On 2017/05/19 03:16:16, tbarzic wrote: > ASSERT_EQ Done. https://codereview.chromium.org/2858353002/diff/300001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils_unittest.cc:122: ASSERT_THAT(kNumSamples, diagnostics_result.perception_samples->size()); On 2017/05/19 03:16:16, tbarzic wrote: > ASSERT_EQ Ah, this is the reason for using testing::SizeIs - getting "error: comparison of integers of different signs: 'const int' and 'const unsigned long'" I think its much cleaner to use testing::SizeIs than casting the const int or using some new variable. If I change kNumSamples to const unsigned long then I also need to change all of the "int i"s in my for loops to unsigned longs as well. https://codereview.chromium.org/2858353002/diff/300001/extensions/browser/api... File extensions/browser/api/media_perception_private/media_perception_api_manager.cc (right): https://codereview.chromium.org/2858353002/diff/300001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager.cc:75: callback.Run(CallbackStatus::SUCCESS, std::move(state_unitialized)); On 2017/05/19 03:16:16, tbarzic wrote: > typo: uninitialized Done. https://codereview.chromium.org/2858353002/diff/300001/extensions/browser/api... File extensions/browser/api/media_perception_private/media_perception_api_manager.h (right): https://codereview.chromium.org/2858353002/diff/300001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager.h:20: enum class CallbackStatus { On 2017/05/19 03:16:16, tbarzic wrote: > nit: move this before ctor/dtor Done. https://codereview.chromium.org/2858353002/diff/300001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager.h:27: // The media analytics process has been launched via Upstart, but waiting On 2017/05/19 03:16:16, tbarzic wrote: > nit: The media analytics process is still being launched via Upstart service. Done. https://codereview.chromium.org/2858353002/diff/300001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager.h:41: using APIStateCallback = base::Callback<void( On 2017/05/19 03:16:16, tbarzic wrote: > nit: callback typedefs up, after enums Done. https://codereview.chromium.org/2858353002/diff/300001/extensions/browser/api... File extensions/browser/api/media_perception_private/media_perception_api_manager_unittest.cc (right): https://codereview.chromium.org/2858353002/diff/300001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager_unittest.cc:25: class CHROMEOS_EXPORT TestUpstartClient : public FakeUpstartClient { On 2017/05/19 03:16:16, tbarzic wrote: > no export, move the class to namespace {} (inside chromeos namespace) Done. https://codereview.chromium.org/2858353002/diff/300001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager_unittest.cc:62: TestUpstartClient::TestUpstartClient() : enqueue_requests_(false) {} On 2017/05/19 03:16:16, tbarzic wrote: > you can just inline these I get a [chromium-style] error when I do. It requires that they not be inlined. https://codereview.chromium.org/2858353002/diff/300001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager_unittest.cc:140: content::TestBrowserContext browser_context_; On 2017/05/19 03:16:16, tbarzic wrote: > add private: and move parts that are not needed by tests to private block (e.g. > browser_context_, thread_bundle_) Done. https://codereview.chromium.org/2858353002/diff/300001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager_unittest.cc:161: EXPECT_EQ(CallbackStatus::PROCESS_IDLE_ERROR, status); On 2017/05/19 03:16:16, tbarzic wrote: > verify that SetState(RUNNING) again will go through to the upstart client Done. https://codereview.chromium.org/2858353002/diff/300001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager_unittest.cc:173: EXPECT_EQ(CallbackStatus::PROCESS_LAUNCHING_ERROR, On 2017/05/19 03:16:16, tbarzic wrote: > add a new line before this Done. https://codereview.chromium.org/2858353002/diff/300001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager_unittest.cc:179: On 2017/05/19 03:16:16, tbarzic wrote: > EXPECT_EQ(SUCCESS, status); Done. https://codereview.chromium.org/2858353002/diff/300001/extensions/browser/api... File extensions/browser/api/media_perception_private/media_perception_private_api.cc (right): https://codereview.chromium.org/2858353002/diff/300001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_private_api.cc:18: On 2017/05/19 03:16:16, tbarzic wrote: > you can remove lines between error consts Done. https://codereview.chromium.org/2858353002/diff/300001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_private_api.cc:31: default: On 2017/05/19 03:16:16, tbarzic wrote: > no default Done.
https://codereview.chromium.org/2858353002/diff/300001/extensions/browser/api... File extensions/browser/api/media_perception_private/conversion_utils_unittest.cc (right): https://codereview.chromium.org/2858353002/diff/300001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils_unittest.cc:122: ASSERT_THAT(kNumSamples, diagnostics_result.perception_samples->size()); On 2017/05/19 13:41:57, Luke Sorenson wrote: > On 2017/05/19 03:16:16, tbarzic wrote: > > ASSERT_EQ > > Ah, this is the reason for using testing::SizeIs - getting "error: comparison of > integers of different signs: 'const int' and 'const unsigned long'" > > I think its much cleaner to use testing::SizeIs than casting the const int or > using some new variable. If I change kNumSamples to const unsigned long then I > also need to change all of the "int i"s in my for loops to unsigned longs as > well. I prefer using size_t for kNumSamples, or static_cast<size_t>(kNumSamples) in the assert. Actually, when I look at this test, you should probably avoid for loop in the first place, thus avoiding identical elements in the arrays. For example this test would not fail if the entries in as array are inverted in the converted struct. https://codereview.chromium.org/2858353002/diff/300001/extensions/browser/api... File extensions/browser/api/media_perception_private/media_perception_api_manager_unittest.cc (right): https://codereview.chromium.org/2858353002/diff/300001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager_unittest.cc:62: TestUpstartClient::TestUpstartClient() : enqueue_requests_(false) {} On 2017/05/19 13:41:58, Luke Sorenson wrote: > On 2017/05/19 03:16:16, tbarzic wrote: > > you can just inline these > > I get a [chromium-style] error when I do. It requires that they not be inlined. You shouldn't (well, you would get it if this was in a header file). https://codereview.chromium.org/2858353002/diff/320001/extensions/browser/api... File extensions/browser/api/media_perception_private/conversion_utils_unittest.cc (right): https://codereview.chromium.org/2858353002/diff/320001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils_unittest.cc:52: EXPECT_EQ(*frame_perception_result.frame_height_in_px, 4); can you make this a little more readable :) https://codereview.chromium.org/2858353002/diff/320001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils_unittest.cc:58: const auto& entity_result_two = (*frame_perception_result.entities)[1]; ASSERT entities size before accessing them https://codereview.chromium.org/2858353002/diff/320001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils_unittest.cc:108: ASSERT_EQ(1ul, media_perception_result.frame_perceptions->size()); 1u https://codereview.chromium.org/2858353002/diff/320001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils_unittest.cc:130: const auto& perception_sample_result = you should use auto only when the type is obvious - this is not the case here. https://codereview.chromium.org/2858353002/diff/320001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils_unittest.cc:135: ValidateFramePerceptionResult(frame_perception_result); it would be nicer if the test output which sample did not match expectations in case of test failure. https://codereview.chromium.org/2858353002/diff/320001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils_unittest.cc:162: state.device_context.reset(new std::string(kTestDeviceContext)); avoid new
Thanks Toni, PTAL https://codereview.chromium.org/2858353002/diff/300001/extensions/browser/api... File extensions/browser/api/media_perception_private/conversion_utils_unittest.cc (right): https://codereview.chromium.org/2858353002/diff/300001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils_unittest.cc:122: ASSERT_THAT(kNumSamples, diagnostics_result.perception_samples->size()); On 2017/05/19 17:41:04, tbarzic wrote: > On 2017/05/19 13:41:57, Luke Sorenson wrote: > > On 2017/05/19 03:16:16, tbarzic wrote: > > > ASSERT_EQ > > > > Ah, this is the reason for using testing::SizeIs - getting "error: comparison > of > > integers of different signs: 'const int' and 'const unsigned long'" > > > > I think its much cleaner to use testing::SizeIs than casting the const int or > > using some new variable. If I change kNumSamples to const unsigned long then I > > also need to change all of the "int i"s in my for loops to unsigned longs as > > well. > > I prefer using size_t for kNumSamples, or static_cast<size_t>(kNumSamples) in > the assert. > > Actually, when I look at this test, you should probably avoid for loop in the > first place, thus avoiding identical elements in the arrays. For example this > test would not fail if the entries in as array are inverted in the converted > struct. Changed to size_t for kNumSamples and included an index in InitializeFakeImageFrameData and ValidateFakeImageFrameData to verify that the array entries are not inverted. https://codereview.chromium.org/2858353002/diff/300001/extensions/browser/api... File extensions/browser/api/media_perception_private/media_perception_api_manager_unittest.cc (right): https://codereview.chromium.org/2858353002/diff/300001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager_unittest.cc:62: TestUpstartClient::TestUpstartClient() : enqueue_requests_(false) {} On 2017/05/19 17:41:04, tbarzic wrote: > On 2017/05/19 13:41:58, Luke Sorenson wrote: > > On 2017/05/19 03:16:16, tbarzic wrote: > > > you can just inline these > > > > I get a [chromium-style] error when I do. It requires that they not be > inlined. > > You shouldn't (well, you would get it if this was in a header file). Ah, before I did try to inline them in the header file. Done. https://codereview.chromium.org/2858353002/diff/320001/extensions/browser/api... File extensions/browser/api/media_perception_private/conversion_utils_unittest.cc (right): https://codereview.chromium.org/2858353002/diff/320001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils_unittest.cc:52: EXPECT_EQ(*frame_perception_result.frame_height_in_px, 4); On 2017/05/19 17:41:04, tbarzic wrote: > can you make this a little more readable :) Done. https://codereview.chromium.org/2858353002/diff/320001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils_unittest.cc:58: const auto& entity_result_two = (*frame_perception_result.entities)[1]; On 2017/05/19 17:41:04, tbarzic wrote: > ASSERT entities size before accessing them Done. https://codereview.chromium.org/2858353002/diff/320001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils_unittest.cc:108: ASSERT_EQ(1ul, media_perception_result.frame_perceptions->size()); On 2017/05/19 17:41:04, tbarzic wrote: > 1u Done. https://codereview.chromium.org/2858353002/diff/320001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils_unittest.cc:130: const auto& perception_sample_result = On 2017/05/19 17:41:04, tbarzic wrote: > you should use auto only when the type is obvious - this is not the case here. Done. https://codereview.chromium.org/2858353002/diff/320001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils_unittest.cc:135: ValidateFramePerceptionResult(frame_perception_result); On 2017/05/19 17:41:04, tbarzic wrote: > it would be nicer if the test output which sample did not match expectations in > case of test failure. > Do you know of a good way to do that? I could just add a LOG before the Validate functions run but that's probably not ideal. I could also use EXPECT_NO_FATAL_FAILURE and then replace all the EXPECTs with ASSERTs in the validate functions, but that's not a great solution either because if multiple members are wrong you would only uncover one at a time. https://codereview.chromium.org/2858353002/diff/320001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils_unittest.cc:162: state.device_context.reset(new std::string(kTestDeviceContext)); On 2017/05/19 17:41:04, tbarzic wrote: > avoid new Done.
https://codereview.chromium.org/2858353002/diff/320001/extensions/browser/api... File extensions/browser/api/media_perception_private/conversion_utils_unittest.cc (right): https://codereview.chromium.org/2858353002/diff/320001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils_unittest.cc:135: ValidateFramePerceptionResult(frame_perception_result); On 2017/05/19 18:27:16, Luke Sorenson wrote: > On 2017/05/19 17:41:04, tbarzic wrote: > > it would be nicer if the test output which sample did not match expectations > in > > case of test failure. > > > > Do you know of a good way to do that? > > I could just add a LOG before the Validate functions run but that's probably > not ideal. I could also use EXPECT_NO_FATAL_FAILURE and then replace all the > EXPECTs with ASSERTs in the validate functions, but that's not a great solution > either because if multiple members are wrong you would only uncover one at a > time. I think SCOPED_TRACE(message); should do the job https://codereview.chromium.org/2858353002/diff/340001/extensions/browser/api... File extensions/browser/api/media_perception_private/conversion_utils_unittest.cc (right): https://codereview.chromium.org/2858353002/diff/340001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils_unittest.cc:9: #include "testing/gmock/include/gmock/gmock.h" do you need gmock? https://codereview.chromium.org/2858353002/diff/340001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils_unittest.cc:67: const auto& bounding_box_result_one = *entity_result_one.bounding_box; can you move this up with the rest of entity_result_one expectations https://codereview.chromium.org/2858353002/diff/340001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils_unittest.cc:68: EXPECT_EQ(*(*bounding_box_result_one.top_left).x, 10); bounding_box_result_one->top_left->x? also, can you assert top_lef and x are set? note that another way to test this is to test frame_perception_result.ToValue() against expected dictionary value: #include "extensions/common/value_builder.h" std::unique_ptr<base::DictionaryValue> expected = DictionaryBuilder() .Set("frameId", 2) .Set("entities", ListBuilder() .Append(DictionaryBuilder()....Build()) .Build()) .Build(); // (note: use SetBoolean when setting bool dictionary values) EXPECT_EQ(*expected, *frame_perception_result.ToValue());
Addressed comments, PTAL https://codereview.chromium.org/2858353002/diff/320001/extensions/browser/api... File extensions/browser/api/media_perception_private/conversion_utils_unittest.cc (right): https://codereview.chromium.org/2858353002/diff/320001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils_unittest.cc:135: ValidateFramePerceptionResult(frame_perception_result); On 2017/05/19 20:22:38, tbarzic wrote: > On 2017/05/19 18:27:16, Luke Sorenson wrote: > > On 2017/05/19 17:41:04, tbarzic wrote: > > > it would be nicer if the test output which sample did not match expectations > > in > > > case of test failure. > > > > > > > Do you know of a good way to do that? > > > > I could just add a LOG before the Validate functions run but that's probably > > not ideal. I could also use EXPECT_NO_FATAL_FAILURE and then replace all the > > EXPECTs with ASSERTs in the validate functions, but that's not a great > solution > > either because if multiple members are wrong you would only uncover one at a > > time. > > I think SCOPED_TRACE(message); should do the job Done. https://codereview.chromium.org/2858353002/diff/340001/extensions/browser/api... File extensions/browser/api/media_perception_private/conversion_utils_unittest.cc (right): https://codereview.chromium.org/2858353002/diff/340001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils_unittest.cc:9: #include "testing/gmock/include/gmock/gmock.h" On 2017/05/19 20:22:38, tbarzic wrote: > do you need gmock? Done. https://codereview.chromium.org/2858353002/diff/340001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils_unittest.cc:67: const auto& bounding_box_result_one = *entity_result_one.bounding_box; On 2017/05/19 20:22:38, tbarzic wrote: > can you move this up with the rest of entity_result_one expectations Done. https://codereview.chromium.org/2858353002/diff/340001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils_unittest.cc:68: EXPECT_EQ(*(*bounding_box_result_one.top_left).x, 10); On 2017/05/19 20:22:38, tbarzic wrote: > bounding_box_result_one->top_left->x? > also, can you assert top_lef and x are set? > > note that another way to test this is to test frame_perception_result.ToValue() > against expected dictionary value: > > #include "extensions/common/value_builder.h" > > std::unique_ptr<base::DictionaryValue> expected = > DictionaryBuilder() > .Set("frameId", 2) > .Set("entities", > ListBuilder() > .Append(DictionaryBuilder()....Build()) > .Build()) > .Build(); > // (note: use SetBoolean when setting bool dictionary values) > > EXPECT_EQ(*expected, *frame_perception_result.ToValue()); Done.
just few small improvements to test code left https://codereview.chromium.org/2858353002/diff/340001/extensions/browser/api... File extensions/browser/api/media_perception_private/conversion_utils_unittest.cc (right): https://codereview.chromium.org/2858353002/diff/340001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils_unittest.cc:9: #include "testing/gmock/include/gmock/gmock.h" On 2017/05/19 21:45:22, Luke Sorenson wrote: > On 2017/05/19 20:22:38, tbarzic wrote: > > do you need gmock? > > Done. not done? :) https://codereview.chromium.org/2858353002/diff/360001/extensions/browser/api... File extensions/browser/api/media_perception_private/conversion_utils_unittest.cc (right): https://codereview.chromium.org/2858353002/diff/360001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils_unittest.cc:51: EXPECT_EQ(*frame_perception_result.frame_id, 2); ASSERT_TRUE(frame_perception_result.frame_id); (similar for the rest) https://codereview.chromium.org/2858353002/diff/360001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils_unittest.cc:57: const auto& entity_result_one = (*frame_perception_result.entities)[0]; frame_perception_result.entities->at(0); https://codereview.chromium.org/2858353002/diff/360001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils_unittest.cc:60: EXPECT_EQ(entity_result_one.type, media_perception::ENTITY_TYPE_FACE); bounding_box_result_one here, before you move on to testing the second entry https://codereview.chromium.org/2858353002/diff/360001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils_unittest.cc:66: const auto* bounding_box_result_one = entity_result_one.bounding_box.get(); can you please use auto only where the type can easily be deduced? https://codereview.chromium.org/2858353002/diff/360001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils_unittest.cc:69: ASSERT_TRUE(bounding_box_result_one->top_left); assert that bounding_box_result_one is set. https://codereview.chromium.org/2858353002/diff/360001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils_unittest.cc:85: image_frame->set_width(index); if you want to include index in the result, frame_perception_result.frame_id would be more natural option :) https://codereview.chromium.org/2858353002/diff/360001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils_unittest.cc:141: SCOPED_TRACE(scope_message); SCOPED_TRACE(testing::Message() << "Sample number " << i); https://codereview.chromium.org/2858353002/diff/360001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils_unittest.cc:143: (*diagnostics_result.perception_samples)[i]; const media_perception::FramePerception* frame_perception_result = diagnostincs_result.perception_samples->at(i).frame_perception.get()); ASSERT_TRUE(frame_perception_result); ValudateFramePerceptionResult(*frame_perception_result); ASSERT_TRUE(frame_perception_result->image_frame); ValidateFakeImageFrameData(i, *frame_perception->image_frame_result); https://codereview.chromium.org/2858353002/diff/360001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils_unittest.cc:164: EXPECT_EQ(*state_result.device_context, kTestDeviceContext); ASSERT_TRUE(state_result.device_context) https://codereview.chromium.org/2858353002/diff/360001/extensions/browser/api... File extensions/browser/api/media_perception_private/media_perception_api_manager_unittest.cc (right): https://codereview.chromium.org/2858353002/diff/360001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager_unittest.cc:64: void TestUpstartClient::StartMediaAnalytics(const UpstartCallback& callback) { nit: you should inline this, too
Thanks Toni, addressed comments on unit test. PTAL https://codereview.chromium.org/2858353002/diff/340001/extensions/browser/api... File extensions/browser/api/media_perception_private/conversion_utils_unittest.cc (right): https://codereview.chromium.org/2858353002/diff/340001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils_unittest.cc:9: #include "testing/gmock/include/gmock/gmock.h" On 2017/05/22 17:40:56, tbarzic wrote: > On 2017/05/19 21:45:22, Luke Sorenson wrote: > > On 2017/05/19 20:22:38, tbarzic wrote: > > > do you need gmock? > > > > Done. > > not done? :) Oops, weird definitely remember removing that. https://codereview.chromium.org/2858353002/diff/360001/extensions/browser/api... File extensions/browser/api/media_perception_private/conversion_utils_unittest.cc (right): https://codereview.chromium.org/2858353002/diff/360001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils_unittest.cc:51: EXPECT_EQ(*frame_perception_result.frame_id, 2); On 2017/05/22 17:40:57, tbarzic wrote: > ASSERT_TRUE(frame_perception_result.frame_id); > (similar for the rest) Done. https://codereview.chromium.org/2858353002/diff/360001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils_unittest.cc:57: const auto& entity_result_one = (*frame_perception_result.entities)[0]; On 2017/05/22 17:40:57, tbarzic wrote: > frame_perception_result.entities->at(0); Done. https://codereview.chromium.org/2858353002/diff/360001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils_unittest.cc:60: EXPECT_EQ(entity_result_one.type, media_perception::ENTITY_TYPE_FACE); On 2017/05/22 17:40:57, tbarzic wrote: > bounding_box_result_one here, before you move on to testing the second entry Done. https://codereview.chromium.org/2858353002/diff/360001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils_unittest.cc:66: const auto* bounding_box_result_one = entity_result_one.bounding_box.get(); On 2017/05/22 17:40:57, tbarzic wrote: > can you please use auto only where the type can easily be deduced? Done. https://codereview.chromium.org/2858353002/diff/360001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils_unittest.cc:69: ASSERT_TRUE(bounding_box_result_one->top_left); On 2017/05/22 17:40:57, tbarzic wrote: > assert that bounding_box_result_one is set. Done. https://codereview.chromium.org/2858353002/diff/360001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils_unittest.cc:85: image_frame->set_width(index); On 2017/05/22 17:40:57, tbarzic wrote: > if you want to include index in the result, frame_perception_result.frame_id > would be more natural option :) Done. https://codereview.chromium.org/2858353002/diff/360001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils_unittest.cc:141: SCOPED_TRACE(scope_message); On 2017/05/22 17:40:57, tbarzic wrote: > SCOPED_TRACE(testing::Message() << "Sample number " << i); Done. https://codereview.chromium.org/2858353002/diff/360001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils_unittest.cc:143: (*diagnostics_result.perception_samples)[i]; On 2017/05/22 17:40:57, tbarzic wrote: > const media_perception::FramePerception* frame_perception_result = > diagnostincs_result.perception_samples->at(i).frame_perception.get()); > ASSERT_TRUE(frame_perception_result); > ValudateFramePerceptionResult(*frame_perception_result); > > ASSERT_TRUE(frame_perception_result->image_frame); > ValidateFakeImageFrameData(i, *frame_perception->image_frame_result); > Done. https://codereview.chromium.org/2858353002/diff/360001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils_unittest.cc:164: EXPECT_EQ(*state_result.device_context, kTestDeviceContext); On 2017/05/22 17:40:57, tbarzic wrote: > ASSERT_TRUE(state_result.device_context) Done. https://codereview.chromium.org/2858353002/diff/360001/extensions/browser/api... File extensions/browser/api/media_perception_private/media_perception_api_manager_unittest.cc (right): https://codereview.chromium.org/2858353002/diff/360001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager_unittest.cc:64: void TestUpstartClient::StartMediaAnalytics(const UpstartCallback& callback) { On 2017/05/22 17:40:57, tbarzic wrote: > nit: you should inline this, too Done.
lgtm https://codereview.chromium.org/2858353002/diff/380001/extensions/browser/api... File extensions/browser/api/media_perception_private/conversion_utils_unittest.cc (right): https://codereview.chromium.org/2858353002/diff/380001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils_unittest.cc:82: ASSERT_TRUE(bounding_box_result_one->top_left->x); nit: I'd put ASSERT_TRUE and EXPECT_EQ for a property together: ASSERT_TRUE(bounding_box_result_one->top_left->x); EXPECT_EQ(10, ...->x); ASSERT_TRUE(...->y); EXPECT_EQ(11, ...->y)
Addressed final comment. Thanks for the lgtm! https://codereview.chromium.org/2858353002/diff/380001/extensions/browser/api... File extensions/browser/api/media_perception_private/conversion_utils_unittest.cc (right): https://codereview.chromium.org/2858353002/diff/380001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils_unittest.cc:82: ASSERT_TRUE(bounding_box_result_one->top_left->x); On 2017/05/22 21:27:14, tbarzic wrote: > nit: I'd put ASSERT_TRUE and EXPECT_EQ for a property together: > ASSERT_TRUE(bounding_box_result_one->top_left->x); > EXPECT_EQ(10, ...->x); > > ASSERT_TRUE(...->y); > EXPECT_EQ(11, ...->y) Done.
The CQ bit was checked by lasoren@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from stevenjb@chromium.org, tbarzic@chromium.org Link to the patchset: https://codereview.chromium.org/2858353002/#ps400001 (title: "Addressed final outstanding comment.")
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
lasoren@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
Adding Devlin for OWNER lgtm on extensions/browser/BUILD.gn and extensions/browser/api/BUILD.gn
https://codereview.chromium.org/2858353002/diff/400001/extensions/browser/api... File extensions/browser/api/media_perception_private/conversion_utils_unittest.cc (right): https://codereview.chromium.org/2858353002/diff/400001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils_unittest.cc:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. nit: remove (c)
Removed (c) from copyright in conversion_utils_unittest.cc https://codereview.chromium.org/2858353002/diff/400001/extensions/browser/api... File extensions/browser/api/media_perception_private/conversion_utils_unittest.cc (right): https://codereview.chromium.org/2858353002/diff/400001/extensions/browser/api... extensions/browser/api/media_perception_private/conversion_utils_unittest.cc:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. On 2017/05/23 17:36:27, tbarzic wrote: > nit: remove (c) Done.
extensions/browser/api/BUILD.gn + extensions/browser/BUILD.gn lgtm
The CQ bit was checked by lasoren@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from stevenjb@chromium.org, tbarzic@chromium.org Link to the patchset: https://codereview.chromium.org/2858353002/#ps420001 (title: "Removed (c) from copyright in conversion_utils_unittest.cc")
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
Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by lasoren@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from stevenjb@chromium.org, rdevlin.cronin@chromium.org, tbarzic@chromium.org Link to the patchset: https://codereview.chromium.org/2858353002/#ps440001 (title: "Moved the tests to is_chromeos in extensions/browser/BUILD.gn")
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
Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by lasoren@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from stevenjb@chromium.org, rdevlin.cronin@chromium.org, tbarzic@chromium.org Link to the patchset: https://codereview.chromium.org/2858353002/#ps460001 (title: "Added media_perception_proto as direct dependency of test targets.")
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
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by lasoren@chromium.org
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
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by lasoren@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": 460001, "attempt_start_ts": 1495625609884740,
"parent_rev": "e353755ef5c7e109ddf2370ebacc39377924fc55", "commit_rev":
"247725c00faa32d43ce0d8d8833edf4167b36ee1"}
Message was sent while issue was closed.
Description was changed from ========== MediaPerceptionPrivate API impl and testing. BUG=709180 ========== to ========== MediaPerceptionPrivate API impl and testing. BUG=709180 Review-Url: https://codereview.chromium.org/2858353002 Cr-Commit-Position: refs/heads/master@{#474260} Committed: https://chromium.googlesource.com/chromium/src/+/247725c00faa32d43ce0d8d8833e... ==========
Message was sent while issue was closed.
Committed patchset #24 (id:460001) as https://chromium.googlesource.com/chromium/src/+/247725c00faa32d43ce0d8d8833e...
Message was sent while issue was closed.
A revert of this CL (patchset #24 id:460001) has been created in https://codereview.chromium.org/2899223003/ by vitaliii@chromium.org. The reason for reverting is: This CL does not compile on ChromiumOS x86-generic. Error: [10333/23428] CXX obj/extensions/browser/api/api/media_perception_private_api.o FAILED: obj/extensions/browser/api/api/media_perception_private_api.o /b/c/goma_client/gomacc i686-pc-linux-gnu-g++ -B/b/c/b/ChromiumOS_x86_generic_Compile/.cros_cache/chrome-sdk/tarballs/x86-generic+9536.0.0+target_toolchain/usr/x86_64-pc-linux-gnu/i686-pc-linux-gnu/binutils-bin/2.25.51-gold -MMD -MF obj/extensions/browser/api/api/media_perception_private_api.o.d -DV8_DEPRECATION_WARNINGS -DUSE_UDEV -DUSE_ASH=1 -DUSE_AURA=1 -DUSE_NSS_CERTS=1 -DUSE_OZONE=1 -DFULL_SAFE_BROWSING -DSAFE_BROWSING_CSD -DSAFE_BROWSING_DB_LOCAL -DCHROMIUM_BUILD -DFIELDTRIAL_TESTING_ENABLED -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D_FORTIFY_SOURCE=2 -DOS_CHROMEOS -DNDEBUG -DNVALGRIND -DDYNAMIC_ANNOTATIONS_ENABLED=0 -DUSE_EGL -DTOOLKIT_VIEWS=1 -DV8_USE_EXTERNAL_STARTUP_DATA -DU_USING_ICU_NAMESPACE=0 -DU_ENABLE_DYLOAD=0 -DU_STATIC_IMPLEMENTATION -DICU_UTIL_DATA_IMPL=ICU_UTIL_DATA_FILE -DUCHAR_TYPE=uint16_t -DSK_IGNORE_LINEONLY_AA_CONVEX_PATH_OPTS -DSK_HAS_PNG_LIBRARY -DSK_HAS_WEBP_LIBRARY -DSK_HAS_JPEG_LIBRARY -DSK_SUPPORT_GPU=1 -DGOOGLE_PROTOBUF_NO_RTTI -DGOOGLE_PROTOBUF_NO_STATIC_INITIALIZER -DHAVE_PTHREAD -I../.. -Igen -I../../third_party/libwebp -I../../third_party/khronos -I../../gpu -I../../third_party/ced/src -I../../third_party/icu/source/common -I../../third_party/icu/source/i18n -I../../third_party/libwebm/source -I../../skia/config -I../../skia/ext -I../../third_party/skia/include/c -I../../third_party/skia/include/config -I../../third_party/skia/include/core -I../../third_party/skia/include/effects -I../../third_party/skia/include/encode -I../../third_party/skia/include/images -I../../third_party/skia/include/lazy -I../../third_party/skia/include/pathops -I../../third_party/skia/include/pdf -I../../third_party/skia/include/pipe -I../../third_party/skia/include/ports -I../../third_party/skia/include/utils -I../../third_party/skia/third_party/vulkan -I../../third_party/skia/include/gpu -I../../third_party/skia/src/gpu -I../../third_party/skia/src/sksl -I../../third_party/protobuf/src -Igen/protoc_out -I../../third_party/protobuf/src -I../../third_party/boringssl/src/include -I/b/c/b/ChromiumOS_x86_generic_Compile/.cros_cache/chrome-sdk/tarballs/x86-generic+9536.0.0+sysroot_chromeos-base_chromeos-chrome.tar.xz/usr/include/nss -I/b/c/b/ChromiumOS_x86_generic_Compile/.cros_cache/chrome-sdk/tarballs/x86-generic+9536.0.0+sysroot_chromeos-base_chromeos-chrome.tar.xz/usr/include/nspr -Igen -I../../third_party/WebKit -Igen/third_party/WebKit -I../../v8/include -Igen/v8/include -Igen -I/b/c/b/ChromiumOS_x86_generic_Compile/.cros_cache/chrome-sdk/tarballs/x86-generic+9536.0.0+sysroot_chromeos-base_chromeos-chrome.tar.xz/usr/include/dbus-1.0 -I/b/c/b/ChromiumOS_x86_generic_Compile/.cros_cache/chrome-sdk/tarballs/x86-generic+9536.0.0+sysroot_chromeos-base_chromeos-chrome.tar.xz/usr/lib/dbus-1.0/include -fno-strict-aliasing -Wno-builtin-macro-redefined -D__DATE__= -D__TIME__= -D__TIMESTAMP__= -funwind-tables -fPIC -pipe -B../../third_party/binutils/Linux_x64/Release/bin -m32 -msse2 -mfpmath=sse -mmmx -pthread -Wall -Werror -Wno-unused-local-typedefs -Wno-maybe-uninitialized -Wno-missing-field-initializers -Wno-unused-parameter -O2 -fno-ident -fdata-sections -ffunction-sections -fno-omit-frame-pointer -g2 -gsplit-dwarf --sysroot=../../../.cros_cache/chrome-sdk/tarballs/x86-generic+9536.0.0+sysroot_chromeos-base_chromeos-chrome.tar.xz -fvisibility=hidden -std=gnu++11 -Wno-narrowing -fno-rtti -fno-exceptions -fvisibility-inlines-hidden -march=i686 -pipe -march=i686 -pipe -pipe -march=i686 -mfpmath=sse -mmmx -msse -msse2 -msse3 -D__google_stl_debug_vector=1 -c ../../extensions/browser/api/media_perception_private/media_perception_private_api.cc -o obj/extensions/browser/api/api/media_perception_private_api.o ../../extensions/browser/api/media_perception_private/media_perception_private_api.cc: In function 'std::string extensions::{anonymous}::CallbackStatusToErrorMessage(const CallbackStatus&)': ../../extensions/browser/api/media_perception_private/media_perception_private_api.cc:30:1: error: control reaches end of non-void function [-Werror=return-type] } ^ cc1plus.elf: all warnings being treated as errors.
Message was sent while issue was closed.
Findit (https://goo.gl/kROfz5) confirmed this CL at revision 474260 as the culprit for failures in the build cycles as shown on: https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb... |
