Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2791983004:
DBus MediaAnalyticsClient and media_perception pb. (Closed)
|
Created:
3 years, 8 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. |
DescriptionDBus MediaAnalyticsClient and media_perception pb.
BUG=709180
Review-Url: https://codereview.chromium.org/2791983004
Cr-Commit-Position: refs/heads/master@{#471869}
Committed: https://chromium.googlesource.com/chromium/src/+/f933419fa6f3b7a34e6f035595b8f2dad44ce34c
Patch Set 1 #Patch Set 2 #Patch Set 3 #Patch Set 4 : Added API permissions #Patch Set 5 : Ran histograms python script #Patch Set 6 : Fixed a bug in permissions file #Patch Set 7 : Updated IDL with MediaPerception dictionary #Patch Set 8 : Fixed some bugs and added event to event histogram #Patch Set 9 : BrowserContextKeyedAPI and working event firing #Patch Set 10 : Using Create convenience function for onMediaPerception #Patch Set 11 : Saving state in API manager POC #Patch Set 12 : Hotrod permissions whitelist #Patch Set 13 : Initial unit test #Patch Set 14 : C++ test impl and target #
Total comments: 4
Patch Set 15 : Added TODOs #Patch Set 16 : One additional TODO #Patch Set 17 : GetDebugFrame Function added #Patch Set 18 : getDiagnosticsBuffer idl and skeleton. #Patch Set 19 : media_perception.proto to deserialize incoming D-Bus messages. #Patch Set 20 : MediaPerceptionProtoToIdl method #Patch Set 21 : Update idl to match media_perception proto #Patch Set 22 : Merge with master #Patch Set 23 : Merge with master #Patch Set 24 : Integrate weigua@ D-Bus code #Patch Set 25 : Added GetDiagnostics D-Bus method call and renamed Diagnostics API #Patch Set 26 : Proto update and started unittest #Patch Set 27 : Unit test for MediaPerceptionProtoToIdl #Patch Set 28 : Fleshing out API, saving state #Patch Set 29 : Implementation complete, need to write fake D-Bus client and integration test #Patch Set 30 : Ready for review! #Patch Set 31 : Deleted an old TODO #Patch Set 32 : Uncommented API permissions #Patch Set 33 : Upstart process management #
Total comments: 56
Patch Set 34 : Addressing reviewer comments, will split out D-Bus code into separate change next #Patch Set 35 : Fixing compile error #
Total comments: 2
Patch Set 36 : Integrate weigua@ Upstart POC and handle D-Bus Upstart callbacks #
Total comments: 33
Patch Set 37 : Separating API + IDL into new CL #Patch Set 38 : Fixing compile error. #
Total comments: 34
Patch Set 39 : Addressing comments on D-Bus code. #
Total comments: 2
Patch Set 40 : Addressing more comments. #
Total comments: 23
Patch Set 41 : Fake client has custom method for firing media perception event. #
Total comments: 7
Patch Set 42 : Addressed comments. #Patch Set 43 : Updating media_perception.proto for for GetDiagnostics. #
Total comments: 4
Patch Set 44 : Addressing comments. #
Total comments: 32
Patch Set 45 : Addressing nits. #
Total comments: 16
Patch Set 46 : Adressing final comments. #Patch Set 47 : Rebased with origin/master #Patch Set 48 : Changing the type of a couple vars to const. #Dependent Patchsets: Messages
Total messages: 60 (23 generated)
Description was changed from ========== First pass at mediaPerceptionPrivate idl BUG= ========== to ========== mediaPerceptionPrivate API initial idl and impl. BUG= ==========
mattfrazier@chromium.org changed reviewers: + mattfrazier@chromium.org
I believe you included webcamprivate_idl by mistake (just a whitespace change) (Also I'm just figuring out how to use chromium codereveiw)
On 2017/04/05 11:55:50, mattfrazier wrote: > I believe you included webcamprivate_idl by mistake (just a whitespace change) > > (Also I'm just figuring out how to use chromium codereveiw) I actually removed some errant spaces from that line in webcam_private.idl so I meant to include it in this CL. (Figuring this out as I go as well)
lasoren@chromium.org changed reviewers: + bmayer@chromium.org
lasoren@chromium.org changed reviewers: + weigua@chromium.org
lasoren@chromium.org changed reviewers: + sque@chromium.org
Just a few nits otherwise LGTM. Will let Simon hit the LGTM after he reviews since he has more experience with chromium. https://codereview.chromium.org/2791983004/diff/250001/extensions/browser/api... File extensions/browser/api/media_perception_private/media_perception_api_manager.cc (right): https://codereview.chromium.org/2791983004/diff/250001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager.cc:15: // static Static comments are typically discouraged in google3 so remove these unless its common for the chromium codebase in which case stick to the convention. https://codereview.chromium.org/2791983004/diff/250001/extensions/browser/api... File extensions/browser/api/media_perception_private/media_perception_api_manager.h (right): https://codereview.chromium.org/2791983004/diff/250001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager.h:33: mpp::Status status_; Could this be made a private or protected member with setter/getters for status to keep the interface clean?
Thanks for the review bmayer@, addressed your comments https://codereview.chromium.org/2791983004/diff/250001/extensions/browser/api... File extensions/browser/api/media_perception_private/media_perception_api_manager.cc (right): https://codereview.chromium.org/2791983004/diff/250001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager.cc:15: // static On 2017/04/13 17:41:59, bmayer wrote: > Static comments are typically discouraged in google3 so remove these unless its > common for the chromium codebase in which case stick to the convention. I saw this // static comment in other BrowserContext-based Factory methods so I included it. For example: https://cs.chromium.org/chromium/src/extensions/browser/api/clipboard/clipboa... https://codereview.chromium.org/2791983004/diff/250001/extensions/browser/api... File extensions/browser/api/media_perception_private/media_perception_api_manager.h (right): https://codereview.chromium.org/2791983004/diff/250001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager.h:33: mpp::Status status_; On 2017/04/13 17:41:59, bmayer wrote: > Could this be made a private or protected member with setter/getters for status > to keep the interface clean? Yes, this should definitely be private especially when the state/status is tied to the rest of the system. This will change when the D-Bus interface is added. I'll add an explicit TODO to make this change.
Have you seen this? go/new-chrome-api There's some reviews to go through when adding an API.
lasoren@chromium.org changed reviewers: + lasoren@chromium.org
lasoren@chromium.org changed reviewers: + rkc@chromium.org, tbarzic@chromium.org - lasoren@chromium.org
Description was changed from ========== mediaPerceptionPrivate API initial idl and impl. BUG= ========== to ========== mediaPerceptionPrivate API initial idl and impl. BUG=https://crbug.com/709180 ==========
Description was changed from ========== mediaPerceptionPrivate API initial idl and impl. BUG=https://crbug.com/709180 ========== to ========== mediaPerceptionPrivate API initial idl and impl. BUG=709180 ==========
I think it would be nice if you could separate dbus client code into a separate cl. https://codereview.chromium.org/2791983004/diff/630001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/media_perception_private/media_perception/runtest.js (right): https://codereview.chromium.org/2791983004/diff/630001/chrome/test/data/exten... chrome/test/data/extensions/api_test/media_perception_private/media_perception/runtest.js:7: let statusEnum = "STARTED" I'd inline these. https://codereview.chromium.org/2791983004/diff/630001/chrome/test/data/exten... chrome/test/data/extensions/api_test/media_perception_private/media_perception/runtest.js:12: state, chrome.test.callback(stateCallback)); chrome.test.callbackPass instead of chrome.test.callback? https://codereview.chromium.org/2791983004/diff/630001/chrome/test/data/exten... chrome/test/data/extensions/api_test/media_perception_private/media_perception/runtest.js:15: function stateCallback(response) { can you inline this? https://codereview.chromium.org/2791983004/diff/630001/chrome/test/data/exten... chrome/test/data/extensions/api_test/media_perception_private/media_perception/runtest.js:21: function(response) { You can do this as: chrome.test.listenOnce( chrome.mediaPerceptionPrivate.onMediaPerceptions, function(evt) { chrome.test.assertEq(evt.timestamp, 1); }); https://codereview.chromium.org/2791983004/diff/630001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/media_perception_private/state/runtest.js (right): https://codereview.chromium.org/2791983004/diff/630001/chrome/test/data/exten... chrome/test/data/extensions/api_test/media_perception_private/state/runtest.js:5: let statusEnum = "SUSPENDED" kStatusEnum https://codereview.chromium.org/2791983004/diff/630001/chromeos/dbus/media_an... File chromeos/dbus/media_analytics_client.cc (right): https://codereview.chromium.org/2791983004/diff/630001/chromeos/dbus/media_an... chromeos/dbus/media_analytics_client.cc:30: const char kMediaPerceptionInterface[] = "org.chromium.MediaPerception"; these should probably go to third_party/cros_system_api/dbus/service_constants.h (note that this is pulled via DEPS from Chromium OS) https://codereview.chromium.org/2791983004/diff/630001/chromeos/dbus/proto/me... File chromeos/dbus/proto/media_perception.proto (right): https://codereview.chromium.org/2791983004/diff/630001/chromeos/dbus/proto/me... chromeos/dbus/proto/media_perception.proto:7: // The output of a Drishti media perception graph. Implicitly tied to the is this shared with Chrome OS? If so, third_party/cros_system_api/dbus/ would probably be a better location for the proto. note that third_party/cros_system_api/ is pulled via DEPS from Chrome OS - you'd have to add the proto to //src/platform/system_api/dbus in Chrome OS source tree, and update DEPS to pull in new version of the repo (https://cs.chromium.org/chromium/src/DEPS?rcl=8cabc77f6204866cbcdc8d581e130f2...) https://codereview.chromium.org/2791983004/diff/630001/extensions/browser/api... File extensions/browser/api/media_perception_private/media_perception_api_manager.cc (right): https://codereview.chromium.org/2791983004/diff/630001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager.cc:14: namespace mpp = extensions::api::media_perception_private; can you use more meaningful name here? https://codereview.chromium.org/2791983004/diff/630001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager.cc:37: bbox_result->top_left.reset(new mpp::Point()); suiggestion: use base::MakeUnique to create unique_ptrs https://codereview.chromium.org/2791983004/diff/630001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager.cc:218: api_state_callback_ = callback; This won't work if there is another request before the current one finishes. You can bind callback as an argument to the callback passed to dbus_client->State: dbus_client->State(nullptr, 0, base::Bind(&MediaPerceptionAPIManager::StateCallback, base::Unretained(this), callback)); Then have: void MediaPerceptionAPIManager::StateCallback(const APIStateCallback& callback, bool succeeded, const uint8_t* bytes, size_t length) { // Do stuff } https://codereview.chromium.org/2791983004/diff/630001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager.cc:232: dbus_client->StartMediaAnalytics(base::Bind( doing this if state is anything else than STARTED seems wrong. https://codereview.chromium.org/2791983004/diff/630001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager.cc:267: // TODO(lasoren): Consider propagating error up to the frontend. you should invoke the API callback so the reference to the API function is released (and the app doesn't hang) https://codereview.chromium.org/2791983004/diff/630001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager.cc:288: if (state_proto.status() == mri::State::STARTED) { This seems a little too magical :/ Does this mean that an app would have to poll GetState until it gets "STARTED" state in order to receive media perception events? Can this handler be added when MediaPerceptionAPIManager is created, or maybe when an app adds OnMediaPerception listener? https://codereview.chromium.org/2791983004/diff/630001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager.cc:293: base::Unretained(this))); Can we use weak ptr here instead of unretained. Also, the handler should probably be unset when this is destructed. https://codereview.chromium.org/2791983004/diff/630001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager.cc:304: api_get_diagnostics_callback_.Run(false, std::move(diagnostics)); can we pass in the callback as GetDiagnosticsCallback arg and make GetDisagnosticsCalblack standalone function? https://codereview.chromium.org/2791983004/diff/630001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager.cc:308: if (!diagnostics_proto.ParseFromArray(bytes, length)) { I wonder if proto parsing should be done by dbus client (before invoking the callback)? https://codereview.chromium.org/2791983004/diff/630001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager.cc:321: if (router && router->HasEventListener(mpp::OnMediaPerception::kEventName)) { if (!router || !router->HasEventListener(...)) return; https://codereview.chromium.org/2791983004/diff/630001/extensions/common/api/... File extensions/common/api/media_perception_private.idl (right): https://codereview.chromium.org/2791983004/diff/630001/extensions/common/api/... extensions/common/api/media_perception_private.idl:7: [implemented_in = "extensions/browser/api/media_perception_private/media_perception_private_api.h"] this is chromeos only, right? can you add platforms=("chromeos") https://codereview.chromium.org/2791983004/diff/630001/extensions/common/api/... extensions/common/api/media_perception_private.idl:11: ERROR, // An error occurred that needs to be propagated to the frontend. Can you put the comments above the values? Also, I'd add a new line after each enum value (for readability). https://codereview.chromium.org/2791983004/diff/630001/extensions/common/api/... extensions/common/api/media_perception_private.idl:84: // A single framePerception or possibly array of framePerceptions. The type indicates that this will always be an array. https://codereview.chromium.org/2791983004/diff/630001/extensions/common/api/... extensions/common/api/media_perception_private.idl:91: // colorspace is defined in the same way as SimpleImage::ColorSpace. What's SimpleImage? https://codereview.chromium.org/2791983004/diff/630001/extensions/common/api/... extensions/common/api/media_perception_private.idl:93: // By default, 1 channel means Grayscale, 2 channels meangs Grayscale + Alpha, Can the meaning of these values be different in any case? https://codereview.chromium.org/2791983004/diff/630001/extensions/common/api/... extensions/common/api/media_perception_private.idl:122: static void getState(StateCallback callback); nit: new line after methods https://codereview.chromium.org/2791983004/diff/630001/extensions/common/api/... extensions/common/api/media_perception_private.idl:127: static void setState(State state, StateCallback callback); So, what would be the meaning of calling setState({status: ERROR/TIMEOUT/RUNNING}). Is this intended to start/stop the mediaPerception daemon? Maybe it would be better to replace this with a clearer API, e.g. start(DeviceContext) stop()
mattfrazier@google.com changed reviewers: + mattfrazier@google.com
https://codereview.chromium.org/2791983004/diff/630001/extensions/common/api/... File extensions/common/api/media_perception_private.idl (right): https://codereview.chromium.org/2791983004/diff/630001/extensions/common/api/... extensions/common/api/media_perception_private.idl:127: static void setState(State state, StateCallback callback); On 2017/04/27 20:37:36, tbarzic wrote: > So, what would be the meaning of calling setState({status: > ERROR/TIMEOUT/RUNNING}). > Is this intended to start/stop the mediaPerception daemon? Maybe it would be > better to replace this with a clearer API, e.g. > start(DeviceContext) > stop() The only accestable setState values for State should be SUSPENDED and RUNNING, all other states are transitional and informational from getState. The transitional states should result in an error result from setState. The setState model retains more flexibility inthe API as the map gorws to include other state characteristics (eg runtime control) without exploding the top-level API.
rkc@google.com changed reviewers: + rkc@google.com
I'd like to echo Toni's thoughts here, this CL is way too big and needs to be split up into dependent CLs. Having a separate CL that implements the DBus changes + tests, another that implements the IDL + API + API tests sounds reasonable.
On 2017/05/02 22:07:25, rkc_google wrote: > I'd like to echo Toni's thoughts here, this CL is way too big and needs to be > split up into dependent CLs. > > Having a separate CL that implements the DBus changes + tests, another that > implements the IDL + API + API tests sounds reasonable. Sorry for the delay in getting back to your review Toni, I was out of office until yesterday. As far as splitting up the CL, I think that sounds very reasonable. I am in the midst of addressing the first round of comments on this change, and will separate out the D-Bus code into another CL afterwards. (Some draft comments may send with this reply, which may apply to a new patch set yet to be uploaded)
Thanks for the review! At least one open question still. https://codereview.chromium.org/2791983004/diff/630001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/media_perception_private/media_perception/runtest.js (right): https://codereview.chromium.org/2791983004/diff/630001/chrome/test/data/exten... chrome/test/data/extensions/api_test/media_perception_private/media_perception/runtest.js:7: let statusEnum = "STARTED" On 2017/04/27 20:37:35, tbarzic wrote: > I'd inline these. Done. https://codereview.chromium.org/2791983004/diff/630001/chrome/test/data/exten... chrome/test/data/extensions/api_test/media_perception_private/media_perception/runtest.js:12: state, chrome.test.callback(stateCallback)); On 2017/04/27 20:37:35, tbarzic wrote: > chrome.test.callbackPass instead of chrome.test.callback? Done. https://codereview.chromium.org/2791983004/diff/630001/chrome/test/data/exten... chrome/test/data/extensions/api_test/media_perception_private/media_perception/runtest.js:15: function stateCallback(response) { On 2017/04/27 20:37:35, tbarzic wrote: > can you inline this? Done. https://codereview.chromium.org/2791983004/diff/630001/chrome/test/data/exten... chrome/test/data/extensions/api_test/media_perception_private/media_perception/runtest.js:21: function(response) { On 2017/04/27 20:37:35, tbarzic wrote: > You can do this as: > chrome.test.listenOnce( > chrome.mediaPerceptionPrivate.onMediaPerceptions, > function(evt) { > chrome.test.assertEq(evt.timestamp, 1); > }); Done. https://codereview.chromium.org/2791983004/diff/630001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/media_perception_private/state/runtest.js (right): https://codereview.chromium.org/2791983004/diff/630001/chrome/test/data/exten... chrome/test/data/extensions/api_test/media_perception_private/state/runtest.js:5: let statusEnum = "SUSPENDED" On 2017/04/27 20:37:35, tbarzic wrote: > kStatusEnum Done. https://codereview.chromium.org/2791983004/diff/630001/chromeos/dbus/media_an... File chromeos/dbus/media_analytics_client.cc (right): https://codereview.chromium.org/2791983004/diff/630001/chromeos/dbus/media_an... chromeos/dbus/media_analytics_client.cc:30: const char kMediaPerceptionInterface[] = "org.chromium.MediaPerception"; On 2017/04/27 20:37:35, tbarzic wrote: > these should probably go to third_party/cros_system_api/dbus/service_constants.h > (note that this is pulled via DEPS from Chromium OS) Done. Moved here: https://chromium-review.googlesource.com/c/494806/ https://codereview.chromium.org/2791983004/diff/630001/chromeos/dbus/proto/me... File chromeos/dbus/proto/media_perception.proto (right): https://codereview.chromium.org/2791983004/diff/630001/chromeos/dbus/proto/me... chromeos/dbus/proto/media_perception.proto:7: // The output of a Drishti media perception graph. Implicitly tied to the On 2017/04/27 20:37:35, tbarzic wrote: > is this shared with Chrome OS? > If so, third_party/cros_system_api/dbus/ would probably be a better location for > the proto. > > note that third_party/cros_system_api/ is pulled via DEPS from Chrome OS - you'd > have to add the proto to //src/platform/system_api/dbus in Chrome OS source > tree, and update DEPS to pull in new version of the repo > (https://cs.chromium.org/chromium/src/DEPS?rcl=8cabc77f6204866cbcdc8d581e130f2...) This proto is not shared with Chrome OS. It is shared with google3 however, and exists here: https://cs.corp.google.com/piper///depot/google3/videoconf/gvc2/drishti/proto... As far I know, nothing in CrOS depends on this proto so if it is okay to leave it here, that is what I will do. https://codereview.chromium.org/2791983004/diff/630001/extensions/browser/api... File extensions/browser/api/media_perception_private/media_perception_api_manager.cc (right): https://codereview.chromium.org/2791983004/diff/630001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager.cc:14: namespace mpp = extensions::api::media_perception_private; On 2017/04/27 20:37:36, tbarzic wrote: > can you use more meaningful name here? Done. https://codereview.chromium.org/2791983004/diff/630001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager.cc:37: bbox_result->top_left.reset(new mpp::Point()); On 2017/04/27 20:37:36, tbarzic wrote: > suiggestion: > use base::MakeUnique to create unique_ptrs In this case, the unique_ptrs already exist, because they are member variables of the generated object (from idl) so I don't think base::MakeUnique is applicable? Example of other code using a similar pattern: https://cs.chromium.org/chromium/src/extensions/browser/api/webcam_private/we... https://codereview.chromium.org/2791983004/diff/630001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager.cc:218: api_state_callback_ = callback; On 2017/04/27 20:37:36, tbarzic wrote: > This won't work if there is another request before the current one finishes. > You can bind callback as an argument to the callback passed to > dbus_client->State: > > dbus_client->State(nullptr, 0, > base::Bind(&MediaPerceptionAPIManager::StateCallback, > base::Unretained(this), > callback)); > > Then have: > > void MediaPerceptionAPIManager::StateCallback(const APIStateCallback& callback, > bool succeeded, > const uint8_t* bytes, > size_t length) { > // Do stuff > } Done. https://codereview.chromium.org/2791983004/diff/630001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager.cc:232: dbus_client->StartMediaAnalytics(base::Bind( On 2017/04/27 20:37:36, tbarzic wrote: > doing this if state is anything else than STARTED seems wrong. Done. Although in this case it would be for SetState RUNNING. For API, only RUNNING and STARTED are allowed for SetState and I added that check. https://codereview.chromium.org/2791983004/diff/630001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager.cc:267: // TODO(lasoren): Consider propagating error up to the frontend. On 2017/04/27 20:37:36, tbarzic wrote: > you should invoke the API callback so the reference to the API function is > released (and the app doesn't hang) Done. https://codereview.chromium.org/2791983004/diff/630001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager.cc:288: if (state_proto.status() == mri::State::STARTED) { On 2017/04/27 20:37:35, tbarzic wrote: > This seems a little too magical :/ > > Does this mean that an app would have to poll GetState until it gets "STARTED" > state in order to receive media perception events? > > Can this handler be added when MediaPerceptionAPIManager is created, or maybe > when an app adds OnMediaPerception listener? The app wouldn't need to poll until it gets STARTED, SetState also triggers the StateCallback. STARTED is the immediate reply from the media analytics client after you set the state to RUNNING. I still think its less error prone to have the listener in the constructor so I moved it there. https://codereview.chromium.org/2791983004/diff/630001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager.cc:293: base::Unretained(this))); On 2017/04/27 20:37:36, tbarzic wrote: > Can we use weak ptr here instead of unretained. > > > Also, the handler should probably be unset when this is destructed. Handler unset when the class is destructed. What would the syntax of weak ptr be in this case? https://codereview.chromium.org/2791983004/diff/630001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager.cc:304: api_get_diagnostics_callback_.Run(false, std::move(diagnostics)); On 2017/04/27 20:37:35, tbarzic wrote: > can we pass in the callback as GetDiagnosticsCallback arg and make > GetDisagnosticsCalblack standalone function? Done. https://codereview.chromium.org/2791983004/diff/630001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager.cc:308: if (!diagnostics_proto.ParseFromArray(bytes, length)) { On 2017/04/27 20:37:36, tbarzic wrote: > I wonder if proto parsing should be done by dbus client (before invoking the > callback)? In my prior implementation the media_perception.proto was only accessible to the API manager (due to violate checkdeps rules) and because the proto was located in this directory. Now that I have moved the proto to the chromeos/dbus/ directory, I think I agree that the D-Bus client should do the proto parsing. Making this change. https://codereview.chromium.org/2791983004/diff/630001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager.cc:321: if (router && router->HasEventListener(mpp::OnMediaPerception::kEventName)) { On 2017/04/27 20:37:36, tbarzic wrote: > if (!router || !router->HasEventListener(...)) > return; Done. https://codereview.chromium.org/2791983004/diff/630001/extensions/common/api/... File extensions/common/api/media_perception_private.idl (right): https://codereview.chromium.org/2791983004/diff/630001/extensions/common/api/... extensions/common/api/media_perception_private.idl:7: [implemented_in = "extensions/browser/api/media_perception_private/media_perception_private_api.h"] On 2017/04/27 20:37:36, tbarzic wrote: > this is chromeos only, right? > can you add > platforms=("chromeos") Done. https://codereview.chromium.org/2791983004/diff/630001/extensions/common/api/... extensions/common/api/media_perception_private.idl:11: ERROR, // An error occurred that needs to be propagated to the frontend. On 2017/04/27 20:37:36, tbarzic wrote: > Can you put the comments above the values? > Also, I'd add a new line after each enum value (for readability). > Done. https://codereview.chromium.org/2791983004/diff/630001/extensions/common/api/... extensions/common/api/media_perception_private.idl:84: // A single framePerception or possibly array of framePerceptions. On 2017/04/27 20:37:36, tbarzic wrote: > The type indicates that this will always be an array. Done. https://codereview.chromium.org/2791983004/diff/630001/extensions/common/api/... extensions/common/api/media_perception_private.idl:91: // colorspace is defined in the same way as SimpleImage::ColorSpace. On 2017/04/27 20:37:36, tbarzic wrote: > What's SimpleImage? It's a class defined within google3, this entire dictionary will actually change because we want to use JPEG encoded images but haven't yet figured out how we want to represent them in idl. Adding a TODO for now. https://codereview.chromium.org/2791983004/diff/630001/extensions/common/api/... extensions/common/api/media_perception_private.idl:93: // By default, 1 channel means Grayscale, 2 channels meangs Grayscale + Alpha, On 2017/04/27 20:37:36, tbarzic wrote: > Can the meaning of these values be different in any case? See above comment, this dictionary is not final and will change. https://codereview.chromium.org/2791983004/diff/630001/extensions/common/api/... extensions/common/api/media_perception_private.idl:122: static void getState(StateCallback callback); On 2017/04/27 20:37:36, tbarzic wrote: > nit: new line after methods Done. https://codereview.chromium.org/2791983004/diff/630001/extensions/common/api/... extensions/common/api/media_perception_private.idl:127: static void setState(State state, StateCallback callback); On 2017/05/02 21:57:29, fraz wrote: > On 2017/04/27 20:37:36, tbarzic wrote: > > So, what would be the meaning of calling setState({status: > > ERROR/TIMEOUT/RUNNING}). > > Is this intended to start/stop the mediaPerception daemon? Maybe it would be > > better to replace this with a clearer API, e.g. > > start(DeviceContext) > > stop() > > The only accestable setState values for State should be SUSPENDED and RUNNING, > all other states are transitional and informational from getState. The > transitional states should result in an error result from setState. The setState > model retains more flexibility inthe API as the map gorws to include other state > characteristics (eg runtime control) without exploding the top-level API. Echoing what Matt said, we prefer our API to SetState and GetState generically so that we can easily extend the API to support different possible states for the media analytics process, which we suspect will get more complex over time. By using a generic SetState, we only need to modify the idl and media_perception proto, rather than adding a whole new API Function.
Description was changed from ========== mediaPerceptionPrivate API initial idl and impl. BUG=709180 ========== to ========== DBus MediaAnalyticsClient and media_perception pb. BUG=709180 ==========
Separated out the other files, this is now just the D-Bus client code. PTAL
Patchset #39 (id:750001) has been deleted
sorry for all the comments from the non dbus parts (that were made before you split those files out) https://codereview.chromium.org/2791983004/diff/630001/extensions/browser/api... File extensions/browser/api/media_perception_private/media_perception_api_manager.cc (right): https://codereview.chromium.org/2791983004/diff/630001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager.cc:37: bbox_result->top_left.reset(new mpp::Point()); On 2017/05/03 23:56:07, Luke Sorenson wrote: > On 2017/04/27 20:37:36, tbarzic wrote: > > suiggestion: > > use base::MakeUnique to create unique_ptrs > > In this case, the unique_ptrs already exist, because they are member variables > of the generated object (from idl) so I don't think base::MakeUnique is > applicable? > > Example of other code using a similar pattern: > https://cs.chromium.org/chromium/src/extensions/browser/api/webcam_private/we... yes, this works, but I think bbox_result->top_left = base::MakeUnique<mpp::Point>(); is easier to read. https://codereview.chromium.org/2791983004/diff/630001/extensions/common/api/... File extensions/common/api/media_perception_private.idl (right): https://codereview.chromium.org/2791983004/diff/630001/extensions/common/api/... extensions/common/api/media_perception_private.idl:127: static void setState(State state, StateCallback callback); On 2017/05/03 23:56:07, Luke Sorenson wrote: > On 2017/05/02 21:57:29, fraz wrote: > > On 2017/04/27 20:37:36, tbarzic wrote: > > > So, what would be the meaning of calling setState({status: > > > ERROR/TIMEOUT/RUNNING}). > > > Is this intended to start/stop the mediaPerception daemon? Maybe it would be > > > better to replace this with a clearer API, e.g. > > > start(DeviceContext) > > > stop() > > > > The only accestable setState values for State should be SUSPENDED and RUNNING, > > all other states are transitional and informational from getState. The > > transitional states should result in an error result from setState. The > setState > > model retains more flexibility inthe API as the map gorws to include other > state > > characteristics (eg runtime control) without exploding the top-level API. > > Echoing what Matt said, we prefer our API to SetState and GetState generically > so that we can easily extend the API to support different possible states for > the media analytics process, which we suspect will get more complex over time. > > By using a generic SetState, we only need to modify the idl and media_perception > proto, rather than adding a whole new API Function. Yes, having this generic setState is fine with me (unless you expect the set of settable states not to change - which seems not to be the case). Though, can you please document settable states here (or use different states enum). Also, what do you think about renaming this to requestState (logic being that depending on it's state the platform may refuse to change the state) - this is just a suggestion, feel free to disregard :) https://codereview.chromium.org/2791983004/diff/670001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/media_perception_private/media_perception/runtest.js (right): https://codereview.chromium.org/2791983004/diff/670001/chrome/test/data/exten... chrome/test/data/extensions/api_test/media_perception_private/media_perception/runtest.js:23: chrome.test.notifyPass(); you should not need this. https://codereview.chromium.org/2791983004/diff/690001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/media_perception_private/state/runtest.js (right): https://codereview.chromium.org/2791983004/diff/690001/chrome/test/data/exten... chrome/test/data/extensions/api_test/media_perception_private/state/runtest.js:5: let kStatusEnum = "RUNNING" nit: JS code should use single quotes https://codereview.chromium.org/2791983004/diff/690001/extensions/browser/api... File extensions/browser/api/media_perception_private/media_perception_api_manager.cc (right): https://codereview.chromium.org/2791983004/diff/690001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager.cc:253: if (state_proto.status() != mri::State::RUNNING && can we do this check in the API function? https://codereview.chromium.org/2791983004/diff/690001/extensions/browser/api... File extensions/browser/api/media_perception_private/media_perception_private_api.cc (right): https://codereview.chromium.org/2791983004/diff/690001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_private_api.cc:52: EXTENSION_FUNCTION_VALIDATE(params.get()); Check params->state is a settable one here and return errror response (so the extension sees an exception). https://codereview.chromium.org/2791983004/diff/690001/extensions/browser/api... File extensions/browser/api/media_perception_private/media_perception_private_api.h (right): https://codereview.chromium.org/2791983004/diff/690001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_private_api.h:15: class MediaPerceptionPrivateGetStateFunction : public AsyncExtensionFunction { I think AsyncExtenionFunction is deprecated. Can you use UIThreadExtensionFunction? https://codereview.chromium.org/2791983004/diff/690001/extensions/common/api/... File extensions/common/api/media_perception_private.idl (right): https://codereview.chromium.org/2791983004/diff/690001/extensions/common/api/... extensions/common/api/media_perception_private.idl:8: implemented_in = "extensions/browser/api/media_perception_private/media_perception_private_api.h"] I don't think you need this - this seems to be the default (expected) location https://codereview.chromium.org/2791983004/diff/690001/extensions/common/api/... extensions/common/api/media_perception_private.idl:12: // An error occurred that needs to be propagated to the frontend. I'd drop the "that needs to be propagated to the frontend". It's not clear what frontend is in this (exstension API) context. https://codereview.chromium.org/2791983004/diff/690001/extensions/common/api/... extensions/common/api/media_perception_private.idl:21: // Analysis process running but not recieving frames. Can you explain the difference between STARTED/RUNNING/SUSPENDED states a little better? https://codereview.chromium.org/2791983004/diff/690001/extensions/common/api/... extensions/common/api/media_perception_private.idl:37: DOMString? deviceContext; Is this allowed only with setState(RUNNING)? I.e. what expected behavior of setState({status: RUNNING, deviceContext:1}); setState({deviceContext: 2}); (would this switch the device context?) https://codereview.chromium.org/2791983004/diff/690001/extensions/common/api/... extensions/common/api/media_perception_private.idl:41: // x represents the horizontal distance from the top left corner of the drop "x represents" and "to the point" https://codereview.chromium.org/2791983004/diff/690001/extensions/common/api/... extensions/common/api/media_perception_private.idl:44: // y represents the vertical distance from the top left corner of the drop "y represents" and "to the point" https://codereview.chromium.org/2791983004/diff/690001/extensions/common/api/... extensions/common/api/media_perception_private.idl:46: double? y; This seems like a required value - can you go through the CL and make only truly optional properties optional? https://codereview.chromium.org/2791983004/diff/690001/extensions/common/api/... extensions/common/api/media_perception_private.idl:51: // If not set, the points are not normalized. "If not set, the points are not normalized" seems a little redundant :) https://codereview.chromium.org/2791983004/diff/690001/extensions/common/api/... extensions/common/api/media_perception_private.idl:67: long? id; why is this optional? https://codereview.chromium.org/2791983004/diff/690001/extensions/common/api/... extensions/common/api/media_perception_private.idl:94: // process. I'd comment on relationship to timestamps in framePerceptions objects. https://codereview.chromium.org/2791983004/diff/690001/extensions/common/api/... extensions/common/api/media_perception_private.idl:105: // colorspace is defined in the same way as SimpleImage::ColorSpace. I'd avoid referencing SimpleImage::ColorSpace here - I'd rather leave the comment blank. https://codereview.chromium.org/2791983004/diff/690001/extensions/common/api/... extensions/common/api/media_perception_private.idl:107: // By default, 1 channel means Grayscale, 2 channels meangs Grayscale + Alpha, So this is the number of channels? maybe rename it to channelCount? also, I'd reformat this slightly so meaning of particular channels are described as a list: " By default 1 channel indicates Grayscale 2 channels indicate Grayscale + Alpha 3 channels indicate RGB 4 channels indicate RGBA https://codereview.chromium.org/2791983004/diff/730001/chromeos/dbus/fake_med... File chromeos/dbus/fake_media_analytics_client.cc (right): https://codereview.chromium.org/2791983004/diff/730001/chromeos/dbus/fake_med... chromeos/dbus/fake_media_analytics_client.cc:37: current_state_ = state; can we check that the state is one of the settable ones. https://codereview.chromium.org/2791983004/diff/730001/chromeos/dbus/fake_ups... File chromeos/dbus/fake_upstart_client.h (right): https://codereview.chromium.org/2791983004/diff/730001/chromeos/dbus/fake_ups... chromeos/dbus/fake_upstart_client.h:25: remove line 25 (as these all seem to be UpstartClient overrides) https://codereview.chromium.org/2791983004/diff/730001/chromeos/dbus/media_an... File chromeos/dbus/media_analytics_client.cc (right): https://codereview.chromium.org/2791983004/diff/730001/chromeos/dbus/media_an... chromeos/dbus/media_analytics_client.cc:20: // The MediaAnalyticsCleint implementation used in production. nit:s/Cleint/Client https://codereview.chromium.org/2791983004/diff/730001/chromeos/dbus/media_an... chromeos/dbus/media_analytics_client.cc:29: media_perception_signal_handler_ = handler; DCHECK that the handler was not set before this? https://codereview.chromium.org/2791983004/diff/730001/chromeos/dbus/media_an... chromeos/dbus/media_analytics_client.cc:31: dbus_proxy_->ConnectToSignal( should this be done in init instead (or at least only when this is called the first time)? Doing something like: SetMediaPerceptionHandler() UnseMediaReceptionHandler(); SetMediaPerceptionHandler() would register 2 signal handlers, and each received signal would invoke the handler callback twice. https://codereview.chromium.org/2791983004/diff/730001/chromeos/dbus/media_an... chromeos/dbus/media_analytics_client.cc:47: if (state.has_status()) { // Check that a new state is requested. nit: put the comment above if https://codereview.chromium.org/2791983004/diff/730001/chromeos/dbus/media_an... chromeos/dbus/media_analytics_client.cc:104: if (!media_perception_signal_handler_.is_null()) { maybe do if (media_perception_signal_handler_.is_null()) return; before you start extracting the perception message https://codereview.chromium.org/2791983004/diff/730001/chromeos/dbus/media_an... chromeos/dbus/media_analytics_client.cc:123: callback.Run(false, state); return; (here and other places you run error callback) https://codereview.chromium.org/2791983004/diff/730001/chromeos/dbus/media_an... chromeos/dbus/media_analytics_client.cc:166: // For providing a pointer from an object of this class to bind to callbacks. I don't think this comment is needed. https://codereview.chromium.org/2791983004/diff/730001/chromeos/dbus/media_an... chromeos/dbus/media_analytics_client.cc:167: base::WeakPtrFactory<MediaAnalyticsClientImpl> weak_ptr_factory_; DISALLOW_COPY_AND_ASSIGN? https://codereview.chromium.org/2791983004/diff/730001/chromeos/dbus/media_an... File chromeos/dbus/media_analytics_client.h (right): https://codereview.chromium.org/2791983004/diff/730001/chromeos/dbus/media_an... chromeos/dbus/media_analytics_client.h:24: // Handler type for signal received from media analytics process. Contains a The "byte array with a serialized MediaPercetion proto" does not apply anymore :) https://codereview.chromium.org/2791983004/diff/730001/chromeos/dbus/media_an... chromeos/dbus/media_analytics_client.h:37: virtual void State(const mri::State& state, can we have separate methods for getting and setting state (even if the setter returns the final state)? https://codereview.chromium.org/2791983004/diff/730001/chromeos/dbus/proto/me... File chromeos/dbus/proto/media_perception.proto (right): https://codereview.chromium.org/2791983004/diff/730001/chromeos/dbus/proto/me... chromeos/dbus/proto/media_perception.proto:11: // exists in Chromium for the mediaPerceptionPrivate API implementation to is this still valid statement? I'd prefer to have a single copy of the proto :)
Thanks for the comments tbarzic! Addressed them often in the other broken out CLs, which are located at: https://codereview.chromium.org/2860203002/ https://codereview.chromium.org/2858353002/ Uploading new patch sets for all three now. https://codereview.chromium.org/2791983004/diff/630001/extensions/browser/api... File extensions/browser/api/media_perception_private/media_perception_api_manager.cc (right): https://codereview.chromium.org/2791983004/diff/630001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager.cc:37: bbox_result->top_left.reset(new mpp::Point()); On 2017/05/05 21:10:40, tbarzic wrote: > On 2017/05/03 23:56:07, Luke Sorenson wrote: > > On 2017/04/27 20:37:36, tbarzic wrote: > > > suiggestion: > > > use base::MakeUnique to create unique_ptrs > > > > In this case, the unique_ptrs already exist, because they are member variables > > of the generated object (from idl) so I don't think base::MakeUnique is > > applicable? > > > > Example of other code using a similar pattern: > > > https://cs.chromium.org/chromium/src/extensions/browser/api/webcam_private/we... > > yes, this works, but I think > > bbox_result->top_left = base::MakeUnique<mpp::Point>(); > > is easier to read. Seems strange to waste CPU (even if it is fairly negligible) to re-construct the entire std::unique_ptr object, when I can just replace the pointer with reset() :) Is there a style guide which specifies not to use reset()? https://codereview.chromium.org/2791983004/diff/630001/extensions/common/api/... File extensions/common/api/media_perception_private.idl (right): https://codereview.chromium.org/2791983004/diff/630001/extensions/common/api/... extensions/common/api/media_perception_private.idl:127: static void setState(State state, StateCallback callback); On 2017/05/05 21:10:40, tbarzic wrote: > On 2017/05/03 23:56:07, Luke Sorenson wrote: > > On 2017/05/02 21:57:29, fraz wrote: > > > On 2017/04/27 20:37:36, tbarzic wrote: > > > > So, what would be the meaning of calling setState({status: > > > > ERROR/TIMEOUT/RUNNING}). > > > > Is this intended to start/stop the mediaPerception daemon? Maybe it would > be > > > > better to replace this with a clearer API, e.g. > > > > start(DeviceContext) > > > > stop() > > > > > > The only accestable setState values for State should be SUSPENDED and > RUNNING, > > > all other states are transitional and informational from getState. The > > > transitional states should result in an error result from setState. The > > setState > > > model retains more flexibility inthe API as the map gorws to include other > > state > > > characteristics (eg runtime control) without exploding the top-level API. > > > > Echoing what Matt said, we prefer our API to SetState and GetState generically > > so that we can easily extend the API to support different possible states for > > the media analytics process, which we suspect will get more complex over time. > > > > By using a generic SetState, we only need to modify the idl and > media_perception > > proto, rather than adding a whole new API Function. > > Yes, having this generic setState is fine with me (unless you expect the set of > settable states not to change - which seems not to be the case). > Though, can you please document settable states here (or use different states > enum). > > Also, what do you think about renaming this to requestState (logic being that > depending on it's state the platform may refuse to change the state) - this is > just a suggestion, feel free to disregard :) I expect that the settable states will almost certainly grow over time. Added documentation to the idl in https://codereview.chromium.org/2860203002/ I can definitely see the name being requestState (something I discussed with team) but as with all APIs, its never a guarantee that a request will succeed and not return some error, so I think I will leave it as setState. https://codereview.chromium.org/2791983004/diff/670001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/media_perception_private/media_perception/runtest.js (right): https://codereview.chromium.org/2791983004/diff/670001/chrome/test/data/exten... chrome/test/data/extensions/api_test/media_perception_private/media_perception/runtest.js:23: chrome.test.notifyPass(); On 2017/05/05 21:10:41, tbarzic wrote: > you should not need this. Done. In https://codereview.chromium.org/2858353002/ https://codereview.chromium.org/2791983004/diff/690001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/media_perception_private/state/runtest.js (right): https://codereview.chromium.org/2791983004/diff/690001/chrome/test/data/exten... chrome/test/data/extensions/api_test/media_perception_private/state/runtest.js:5: let kStatusEnum = "RUNNING" On 2017/05/05 21:10:41, tbarzic wrote: > nit: JS code should use single quotes Done. In https://codereview.chromium.org/2858353002/ https://codereview.chromium.org/2791983004/diff/690001/extensions/browser/api... File extensions/browser/api/media_perception_private/media_perception_api_manager.cc (right): https://codereview.chromium.org/2791983004/diff/690001/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/05 21:10:41, tbarzic wrote: > can we do this check in the API function? Done. In https://codereview.chromium.org/2858353002/ https://codereview.chromium.org/2791983004/diff/690001/extensions/browser/api... File extensions/browser/api/media_perception_private/media_perception_private_api.cc (right): https://codereview.chromium.org/2791983004/diff/690001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_private_api.cc:52: EXTENSION_FUNCTION_VALIDATE(params.get()); On 2017/05/05 21:10:41, tbarzic wrote: > Check params->state is a settable one here and return errror response (so the > extension sees an exception). Done. In https://codereview.chromium.org/2858353002/ https://codereview.chromium.org/2791983004/diff/690001/extensions/browser/api... File extensions/browser/api/media_perception_private/media_perception_private_api.h (right): https://codereview.chromium.org/2791983004/diff/690001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_private_api.h:15: class MediaPerceptionPrivateGetStateFunction : public AsyncExtensionFunction { On 2017/05/05 21:10:41, tbarzic wrote: > I think AsyncExtenionFunction is deprecated. > Can you use UIThreadExtensionFunction? Done. In https://codereview.chromium.org/2860203002/ https://codereview.chromium.org/2791983004/diff/690001/extensions/common/api/... File extensions/common/api/media_perception_private.idl (right): https://codereview.chromium.org/2791983004/diff/690001/extensions/common/api/... extensions/common/api/media_perception_private.idl:8: implemented_in = "extensions/browser/api/media_perception_private/media_perception_private_api.h"] On 2017/05/05 21:10:41, tbarzic wrote: > I don't think you need this - this seems to be the default (expected) location Done. In https://codereview.chromium.org/2860203002/ https://codereview.chromium.org/2791983004/diff/690001/extensions/common/api/... extensions/common/api/media_perception_private.idl:12: // An error occurred that needs to be propagated to the frontend. On 2017/05/05 21:10:41, tbarzic wrote: > I'd drop the "that needs to be propagated to the frontend". It's not clear what > frontend is in this (exstension API) context. Done. In https://codereview.chromium.org/2860203002/ https://codereview.chromium.org/2791983004/diff/690001/extensions/common/api/... extensions/common/api/media_perception_private.idl:21: // Analysis process running but not recieving frames. On 2017/05/05 21:10:41, tbarzic wrote: > Can you explain the difference between STARTED/RUNNING/SUSPENDED states a little > better? Done. In https://codereview.chromium.org/2860203002/ https://codereview.chromium.org/2791983004/diff/690001/extensions/common/api/... extensions/common/api/media_perception_private.idl:37: DOMString? deviceContext; On 2017/05/05 21:10:41, tbarzic wrote: > Is this allowed only with setState(RUNNING)? > > I.e. what expected behavior of > setState({status: RUNNING, deviceContext:1}); > setState({deviceContext: 2}); > > (would this switch the device context?) Providing device context is only meaningful with setState(RUNNING), the value will be ignored by the media analytics process if provided with some other state. Added more clarifying comments: https://codereview.chromium.org/2860203002/ https://codereview.chromium.org/2791983004/diff/690001/extensions/common/api/... extensions/common/api/media_perception_private.idl:41: // x represents the horizontal distance from the top left corner of the On 2017/05/05 21:10:41, tbarzic wrote: > drop "x represents" and "to the point" Done. https://codereview.chromium.org/2791983004/diff/690001/extensions/common/api/... extensions/common/api/media_perception_private.idl:44: // y represents the vertical distance from the top left corner of the On 2017/05/05 21:10:41, tbarzic wrote: > drop "y represents" and "to the point" Done. https://codereview.chromium.org/2791983004/diff/690001/extensions/common/api/... extensions/common/api/media_perception_private.idl:46: double? y; On 2017/05/05 21:10:41, tbarzic wrote: > This seems like a required value - can you go through the CL and make only truly > optional properties optional? Because many of these Dictionary definitions are based on proto messages in media_perception.proto (and required values are now highly discouraged in protos), to me it makes sense for the values here to be optional as well. If we're converting from protos to these IDL classes, and for some reason, a value isn't provided in the proto message, I don't think it makes sense for the default value to appear in the JS Dictionary. https://codereview.chromium.org/2791983004/diff/690001/extensions/common/api/... extensions/common/api/media_perception_private.idl:51: // If not set, the points are not normalized. On 2017/05/05 21:10:41, tbarzic wrote: > "If not set, the points are not normalized" seems a little redundant :) Done :) In https://codereview.chromium.org/2860203002/ https://codereview.chromium.org/2791983004/diff/690001/extensions/common/api/... extensions/common/api/media_perception_private.idl:67: long? id; On 2017/05/05 21:10:41, tbarzic wrote: > why is this optional? See above explanation about decision to use only optional values in these dictionaries. For this field specifically, we actually don't use it at all currently, but plan to in the near future. It would be confusing if every entity has id = 0 because it was a non-optional value and we weren't setting it. https://codereview.chromium.org/2791983004/diff/690001/extensions/common/api/... extensions/common/api/media_perception_private.idl:94: // process. On 2017/05/05 21:10:41, tbarzic wrote: > I'd comment on relationship to timestamps in framePerceptions objects. Done. https://codereview.chromium.org/2791983004/diff/690001/extensions/common/api/... extensions/common/api/media_perception_private.idl:105: // colorspace is defined in the same way as SimpleImage::ColorSpace. On 2017/05/05 21:10:41, tbarzic wrote: > I'd avoid referencing SimpleImage::ColorSpace here - I'd rather leave the > comment blank. Acknowledged. https://codereview.chromium.org/2791983004/diff/690001/extensions/common/api/... extensions/common/api/media_perception_private.idl:107: // By default, 1 channel means Grayscale, 2 channels meangs Grayscale + Alpha, On 2017/05/05 21:10:41, tbarzic wrote: > So this is the number of channels? maybe rename it to channelCount? > > also, I'd reformat this slightly so meaning of particular channels are > described as a list: > " By default > 1 channel indicates Grayscale > 2 channels indicate Grayscale + Alpha > 3 channels indicate RGB > 4 channels indicate RGBA > > For now, since we not yet sure how we'll be representing these image frames, I've just removed those two fields (colorspace and channels). The TODO(lasoren) remains to change this Dictionary definition. See https://codereview.chromium.org/2860203002/ for current state of IDL. https://codereview.chromium.org/2791983004/diff/730001/chromeos/dbus/fake_med... File chromeos/dbus/fake_media_analytics_client.cc (right): https://codereview.chromium.org/2791983004/diff/730001/chromeos/dbus/fake_med... chromeos/dbus/fake_media_analytics_client.cc:37: current_state_ = state; On 2017/05/05 21:10:42, tbarzic wrote: > can we check that the state is one of the settable ones. Do we need to do that, if there is a check in API impl, and can verify that error in JS? https://codereview.chromium.org/2791983004/diff/730001/chromeos/dbus/fake_ups... File chromeos/dbus/fake_upstart_client.h (right): https://codereview.chromium.org/2791983004/diff/730001/chromeos/dbus/fake_ups... chromeos/dbus/fake_upstart_client.h:25: On 2017/05/05 21:10:42, tbarzic wrote: > remove line 25 (as these all seem to be UpstartClient overrides) Done. https://codereview.chromium.org/2791983004/diff/730001/chromeos/dbus/media_an... File chromeos/dbus/media_analytics_client.cc (right): https://codereview.chromium.org/2791983004/diff/730001/chromeos/dbus/media_an... chromeos/dbus/media_analytics_client.cc:20: // The MediaAnalyticsCleint implementation used in production. On 2017/05/05 21:10:42, tbarzic wrote: > nit:s/Cleint/Client Done. https://codereview.chromium.org/2791983004/diff/730001/chromeos/dbus/media_an... chromeos/dbus/media_analytics_client.cc:29: media_perception_signal_handler_ = handler; On 2017/05/05 21:10:42, tbarzic wrote: > DCHECK that the handler was not set before this? Done. https://codereview.chromium.org/2791983004/diff/730001/chromeos/dbus/media_an... chromeos/dbus/media_analytics_client.cc:31: dbus_proxy_->ConnectToSignal( On 2017/05/05 21:10:42, tbarzic wrote: > should this be done in init instead (or at least only when this is called the > first time)? > > Doing something like: > SetMediaPerceptionHandler() > UnseMediaReceptionHandler(); > SetMediaPerceptionHandler() > > would register 2 signal handlers, and each received signal would invoke the > handler callback twice. Done. https://codereview.chromium.org/2791983004/diff/730001/chromeos/dbus/media_an... chromeos/dbus/media_analytics_client.cc:47: if (state.has_status()) { // Check that a new state is requested. On 2017/05/05 21:10:42, tbarzic wrote: > nit: put the comment above if Done. https://codereview.chromium.org/2791983004/diff/730001/chromeos/dbus/media_an... chromeos/dbus/media_analytics_client.cc:104: if (!media_perception_signal_handler_.is_null()) { On 2017/05/05 21:10:42, tbarzic wrote: > maybe do > if (media_perception_signal_handler_.is_null()) > return; > before you start extracting the perception message Done. https://codereview.chromium.org/2791983004/diff/730001/chromeos/dbus/media_an... chromeos/dbus/media_analytics_client.cc:123: callback.Run(false, state); On 2017/05/05 21:10:42, tbarzic wrote: > return; > > (here and other places you run error callback) Done. https://codereview.chromium.org/2791983004/diff/730001/chromeos/dbus/media_an... chromeos/dbus/media_analytics_client.cc:166: // For providing a pointer from an object of this class to bind to callbacks. On 2017/05/05 21:10:42, tbarzic wrote: > I don't think this comment is needed. Done. https://codereview.chromium.org/2791983004/diff/730001/chromeos/dbus/media_an... chromeos/dbus/media_analytics_client.cc:167: base::WeakPtrFactory<MediaAnalyticsClientImpl> weak_ptr_factory_; On 2017/05/05 21:10:42, tbarzic wrote: > DISALLOW_COPY_AND_ASSIGN? Done. https://codereview.chromium.org/2791983004/diff/730001/chromeos/dbus/media_an... File chromeos/dbus/media_analytics_client.h (right): https://codereview.chromium.org/2791983004/diff/730001/chromeos/dbus/media_an... chromeos/dbus/media_analytics_client.h:24: // Handler type for signal received from media analytics process. Contains a On 2017/05/05 21:10:42, tbarzic wrote: > The "byte array with a serialized MediaPercetion proto" does not apply anymore > :) Done. https://codereview.chromium.org/2791983004/diff/730001/chromeos/dbus/media_an... chromeos/dbus/media_analytics_client.h:37: virtual void State(const mri::State& state, On 2017/05/05 21:10:42, tbarzic wrote: > can we have separate methods for getting and setting state (even if the setter > returns the final state)? From the perspective of the D-Bus comms, it's simpler for there to be a single org.chromium.MediaPerception.State service than to have our media analytics process initialize and provide services for both getState and setState. The MediaPerceptionAPIManager separates these two into distinct function for the JS frontend to make the interface clearer. https://codereview.chromium.org/2791983004/diff/730001/chromeos/dbus/proto/me... File chromeos/dbus/proto/media_perception.proto (right): https://codereview.chromium.org/2791983004/diff/730001/chromeos/dbus/proto/me... chromeos/dbus/proto/media_perception.proto:11: // exists in Chromium for the mediaPerceptionPrivate API implementation to On 2017/05/05 21:10:42, tbarzic wrote: > is this still valid statement? > > I'd prefer to have a single copy of the proto :) Edited the text a little. To be clear there is just one copy of this proto in the Chromium source. The reason I refer to this as a duplicate copy is because of the one that exists with and is defined as part of the media analytics process outside of Chrome.
lasoren@chromium.org changed reviewers: + stevenjb@chromium.org
owner lgtm
https://codereview.chromium.org/2791983004/diff/630001/extensions/browser/api... File extensions/browser/api/media_perception_private/media_perception_api_manager.cc (right): https://codereview.chromium.org/2791983004/diff/630001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager.cc:37: bbox_result->top_left.reset(new mpp::Point()); On 2017/05/08 19:06:06, Luke Sorenson wrote: > On 2017/05/05 21:10:40, tbarzic wrote: > > On 2017/05/03 23:56:07, Luke Sorenson wrote: > > > On 2017/04/27 20:37:36, tbarzic wrote: > > > > suiggestion: > > > > use base::MakeUnique to create unique_ptrs > > > > > > In this case, the unique_ptrs already exist, because they are member > variables > > > of the generated object (from idl) so I don't think base::MakeUnique is > > > applicable? > > > > > > Example of other code using a similar pattern: > > > > > > https://cs.chromium.org/chromium/src/extensions/browser/api/webcam_private/we... > > > > yes, this works, but I think > > > > bbox_result->top_left = base::MakeUnique<mpp::Point>(); > > > > is easier to read. > > Seems strange to waste CPU (even if it is fairly negligible) to re-construct the > entire std::unique_ptr object, when I can just replace the pointer with reset() > :) Is there a style guide which specifies not to use reset()? I don't think there's a particular syle guide for this - I'm fine keeping it as is. https://codereview.chromium.org/2791983004/diff/690001/extensions/common/api/... File extensions/common/api/media_perception_private.idl (right): https://codereview.chromium.org/2791983004/diff/690001/extensions/common/api/... extensions/common/api/media_perception_private.idl:67: long? id; On 2017/05/08 19:06:07, Luke Sorenson wrote: > On 2017/05/05 21:10:41, tbarzic wrote: > > why is this optional? > > See above explanation about decision to use only optional values in these > dictionaries. For this field specifically, we actually don't use it at all > currently, but plan to in the near future. It would be confusing if every entity > has id = 0 because it was a non-optional value and we weren't setting it. My idea was closer to dropping nonsensical messages, but it's OK if these are trully not certain to be defined. (Stronger API guaranties would probably make things easier for apps, but it's a private API, so it's probably not as important) https://codereview.chromium.org/2791983004/diff/730001/chromeos/dbus/fake_med... File chromeos/dbus/fake_media_analytics_client.cc (right): https://codereview.chromium.org/2791983004/diff/730001/chromeos/dbus/fake_med... chromeos/dbus/fake_media_analytics_client.cc:37: current_state_ = state; On 2017/05/08 19:06:07, Luke Sorenson wrote: > On 2017/05/05 21:10:42, tbarzic wrote: > > can we check that the state is one of the settable ones. > > Do we need to do that, if there is a check in API impl, and can verify that > error in JS? I'd add (at least) a DCHECK here - so it catches instances where (unintentionally) state is set to a wrong value. Plus. it's closer to the actual implementation. https://codereview.chromium.org/2791983004/diff/730001/chromeos/dbus/media_an... File chromeos/dbus/media_analytics_client.h (right): https://codereview.chromium.org/2791983004/diff/730001/chromeos/dbus/media_an... chromeos/dbus/media_analytics_client.h:37: virtual void State(const mri::State& state, On 2017/05/08 19:06:08, Luke Sorenson wrote: > On 2017/05/05 21:10:42, tbarzic wrote: > > can we have separate methods for getting and setting state (even if the setter > > returns the final state)? > > From the perspective of the D-Bus comms, it's simpler for there to be a single > org.chromium.MediaPerception.State service than to have our media analytics > process initialize and provide services for both getState and setState. The > MediaPerceptionAPIManager separates these two into distinct function for the JS > frontend to make the interface clearer. Wouldn't you need only one service that can provide both getState and setState method (instead of state method)? Even if DBus provides only a single State method, I think the API provided by client would be much cleaner with both a setter and a getter here (with different invocations to State in the implementation). https://codereview.chromium.org/2791983004/diff/730001/chromeos/dbus/media_an... chromeos/dbus/media_analytics_client.h:45: virtual void UnsetMediaPerceptionSignalHandler() = 0; suggestion: Clear/Reset instead of Unset https://codereview.chromium.org/2791983004/diff/730001/chromeos/dbus/proto/me... File chromeos/dbus/proto/media_perception.proto (right): https://codereview.chromium.org/2791983004/diff/730001/chromeos/dbus/proto/me... chromeos/dbus/proto/media_perception.proto:11: // exists in Chromium for the mediaPerceptionPrivate API implementation to On 2017/05/08 19:06:08, Luke Sorenson wrote: > On 2017/05/05 21:10:42, tbarzic wrote: > > is this still valid statement? > > > > I'd prefer to have a single copy of the proto :) > > Edited the text a little. To be clear there is just one copy of this proto in > the Chromium source. The reason I refer to this as a duplicate copy is because > of the one that exists with and is defined as part of the media analytics > process outside of Chrome. Oh. Can you please reword this a bit further. Someone reading this file only would get very confused (like I got). Maybe say that the proto has two copies - one being here - that have to be updated together (rather than saying the proto has a copy in Chromium codebase, even though this is that copy). also, are we OK with refering to "dristhi" in open source code? https://codereview.chromium.org/2791983004/diff/770001/chromeos/dbus/media_an... File chromeos/dbus/media_analytics_client.cc (right): https://codereview.chromium.org/2791983004/diff/770001/chromeos/dbus/media_an... chromeos/dbus/media_analytics_client.cc:70: // Connect to the MediaPerception proto signal. nit: drop "proto"
https://codereview.chromium.org/2791983004/diff/630001/extensions/browser/api... File extensions/browser/api/media_perception_private/media_perception_api_manager.cc (right): https://codereview.chromium.org/2791983004/diff/630001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager.cc:37: bbox_result->top_left.reset(new mpp::Point()); On 2017/05/08 19:06:06, Luke Sorenson wrote: > On 2017/05/05 21:10:40, tbarzic wrote: > > On 2017/05/03 23:56:07, Luke Sorenson wrote: > > > On 2017/04/27 20:37:36, tbarzic wrote: > > > > suiggestion: > > > > use base::MakeUnique to create unique_ptrs > > > > > > In this case, the unique_ptrs already exist, because they are member > variables > > > of the generated object (from idl) so I don't think base::MakeUnique is > > > applicable? > > > > > > Example of other code using a similar pattern: > > > > > > https://cs.chromium.org/chromium/src/extensions/browser/api/webcam_private/we... > > > > yes, this works, but I think > > > > bbox_result->top_left = base::MakeUnique<mpp::Point>(); > > > > is easier to read. > > Seems strange to waste CPU (even if it is fairly negligible) to re-construct the > entire std::unique_ptr object, when I can just replace the pointer with reset() > :) Is there a style guide which specifies not to use reset()? Look at the comment above base::MakeUnique. It specifically states that bare calls to new should be treated with scrutiny. Additionally, this does not take up any additional instructions - the compiler is smart enough to optimize this. In general when writing code, don't make assumptions on what will generate more instructions at a language level. Compilers now are very smart and will be able to see through things like this. When compilers do not do the right thing, we should be fixing the compilers, not writing our code to not trigger a bug in a particular compiler.
Thanks for the reviews. PTAL https://codereview.chromium.org/2791983004/diff/630001/extensions/browser/api... File extensions/browser/api/media_perception_private/media_perception_api_manager.cc (right): https://codereview.chromium.org/2791983004/diff/630001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_api_manager.cc:37: bbox_result->top_left.reset(new mpp::Point()); On 2017/05/08 23:34:09, rkc1 wrote: > On 2017/05/08 19:06:06, Luke Sorenson wrote: > > On 2017/05/05 21:10:40, tbarzic wrote: > > > On 2017/05/03 23:56:07, Luke Sorenson wrote: > > > > On 2017/04/27 20:37:36, tbarzic wrote: > > > > > suiggestion: > > > > > use base::MakeUnique to create unique_ptrs > > > > > > > > In this case, the unique_ptrs already exist, because they are member > > variables > > > > of the generated object (from idl) so I don't think base::MakeUnique is > > > > applicable? > > > > > > > > Example of other code using a similar pattern: > > > > > > > > > > https://cs.chromium.org/chromium/src/extensions/browser/api/webcam_private/we... > > > > > > yes, this works, but I think > > > > > > bbox_result->top_left = base::MakeUnique<mpp::Point>(); > > > > > > is easier to read. > > > > Seems strange to waste CPU (even if it is fairly negligible) to re-construct > the > > entire std::unique_ptr object, when I can just replace the pointer with > reset() > > :) Is there a style guide which specifies not to use reset()? > > Look at the comment above base::MakeUnique. It specifically states that bare > calls to new should be treated with scrutiny. > Additionally, this does not take up any additional instructions - the compiler > is smart enough to optimize this. > > In general when writing code, don't make assumptions on what will generate more > instructions at a language level. Compilers now are very smart and will be able > to see through things like this. When compilers do not do the right thing, we > should be fixing the compilers, not writing our code to not trigger a bug in a > particular compiler. Done. Changes made in https://codereview.chromium.org/2858353002/ (will appear in new patch set) https://codereview.chromium.org/2791983004/diff/730001/chromeos/dbus/fake_med... File chromeos/dbus/fake_media_analytics_client.cc (right): https://codereview.chromium.org/2791983004/diff/730001/chromeos/dbus/fake_med... chromeos/dbus/fake_media_analytics_client.cc:37: current_state_ = state; On 2017/05/08 23:04:55, tbarzic wrote: > On 2017/05/08 19:06:07, Luke Sorenson wrote: > > On 2017/05/05 21:10:42, tbarzic wrote: > > > can we check that the state is one of the settable ones. > > > > Do we need to do that, if there is a check in API impl, and can verify that > > error in JS? > > I'd add (at least) a DCHECK here - so it catches instances where > (unintentionally) state is set to a wrong value. Plus. it's closer to the actual > implementation. Good point. Done. https://codereview.chromium.org/2791983004/diff/730001/chromeos/dbus/media_an... File chromeos/dbus/media_analytics_client.h (right): https://codereview.chromium.org/2791983004/diff/730001/chromeos/dbus/media_an... chromeos/dbus/media_analytics_client.h:37: virtual void State(const mri::State& state, On 2017/05/08 23:04:55, tbarzic wrote: > On 2017/05/08 19:06:08, Luke Sorenson wrote: > > On 2017/05/05 21:10:42, tbarzic wrote: > > > can we have separate methods for getting and setting state (even if the > setter > > > returns the final state)? > > > > From the perspective of the D-Bus comms, it's simpler for there to be a single > > org.chromium.MediaPerception.State service than to have our media analytics > > process initialize and provide services for both getState and setState. The > > MediaPerceptionAPIManager separates these two into distinct function for the > JS > > frontend to make the interface clearer. > > Wouldn't you need only one service that can provide both getState and setState > method (instead of state method)? > > Even if DBus provides only a single State method, I think the API provided by > client would be much cleaner with both a setter and a getter here (with > different invocations to State in the implementation). Okay, I see what you're saying. Done. https://codereview.chromium.org/2791983004/diff/730001/chromeos/dbus/media_an... chromeos/dbus/media_analytics_client.h:45: virtual void UnsetMediaPerceptionSignalHandler() = 0; On 2017/05/08 23:04:55, tbarzic wrote: > suggestion: Clear/Reset instead of Unset Done. https://codereview.chromium.org/2791983004/diff/730001/chromeos/dbus/proto/me... File chromeos/dbus/proto/media_perception.proto (right): https://codereview.chromium.org/2791983004/diff/730001/chromeos/dbus/proto/me... chromeos/dbus/proto/media_perception.proto:11: // exists in Chromium for the mediaPerceptionPrivate API implementation to On 2017/05/08 23:04:55, tbarzic wrote: > On 2017/05/08 19:06:08, Luke Sorenson wrote: > > On 2017/05/05 21:10:42, tbarzic wrote: > > > is this still valid statement? > > > > > > I'd prefer to have a single copy of the proto :) > > > > Edited the text a little. To be clear there is just one copy of this proto in > > the Chromium source. The reason I refer to this as a duplicate copy is because > > of the one that exists with and is defined as part of the media analytics > > process outside of Chrome. > > Oh. > Can you please reword this a bit further. Someone reading this file only would > get very confused (like I got). > Maybe say that the proto has two copies - one being here - that have to be > updated together (rather than saying the proto has a copy in Chromium codebase, > even though this is that copy). > > also, are we OK with refering to "dristhi" in open source code? Done. Also removed references to "Drishti" since it's not necessary to have them. https://codereview.chromium.org/2791983004/diff/770001/chromeos/dbus/media_an... File chromeos/dbus/media_analytics_client.cc (right): https://codereview.chromium.org/2791983004/diff/770001/chromeos/dbus/media_an... chromeos/dbus/media_analytics_client.cc:70: // Connect to the MediaPerception proto signal. On 2017/05/08 23:04:55, tbarzic wrote: > nit: drop "proto" Done.
https://codereview.chromium.org/2791983004/diff/790001/chromeos/dbus/fake_med... File chromeos/dbus/fake_media_analytics_client.cc (right): https://codereview.chromium.org/2791983004/diff/790001/chromeos/dbus/fake_med... chromeos/dbus/fake_media_analytics_client.cc:18: // Override these initializations to do nothing. nit: no comment needed https://codereview.chromium.org/2791983004/diff/790001/chromeos/dbus/fake_med... chromeos/dbus/fake_media_analytics_client.cc:38: if (state.has_status()) { Status should be set here, right? https://codereview.chromium.org/2791983004/diff/790001/chromeos/dbus/fake_med... chromeos/dbus/fake_media_analytics_client.cc:53: FROM_HERE, base::Bind(&FakeMediaAnalyticsClient::OnMediaPerception, This seems unintuitive.. if you need the fired in tests, I'd add an explicit method to the fake for it https://codereview.chromium.org/2791983004/diff/790001/chromeos/dbus/fake_med... chromeos/dbus/fake_media_analytics_client.cc:67: diagnostics.add_perception_sample()->mutable_frame_perception()->set_frame_id( should frames returned here ahve unique ids? https://codereview.chromium.org/2791983004/diff/790001/chromeos/dbus/fake_med... File chromeos/dbus/fake_media_analytics_client.h (right): https://codereview.chromium.org/2791983004/diff/790001/chromeos/dbus/fake_med... chromeos/dbus/fake_media_analytics_client.h:20: // D-Bus communications to an external process. Drop the part about testing - this is used more widely than that (e.g. on Linux Chrome OS builds) https://codereview.chromium.org/2791983004/diff/790001/chromeos/dbus/fake_ups... File chromeos/dbus/fake_upstart_client.cc (right): https://codereview.chromium.org/2791983004/diff/790001/chromeos/dbus/fake_ups... chromeos/dbus/fake_upstart_client.cc:32: void FakeUpstartClient::StartMediaAnalytics(const UpstartCallback& callback) { it might be useful to tailor FakeMediaAnalytics client based on this - e.g. make all its methods fail until this is called. https://codereview.chromium.org/2791983004/diff/790001/chromeos/dbus/media_an... File chromeos/dbus/media_analytics_client.cc (right): https://codereview.chromium.org/2791983004/diff/790001/chromeos/dbus/media_an... chromeos/dbus/media_analytics_client.cc:59: LOG(WARNING) << "Attempting to SetState without status set."; should this case cause an error callback to be invoked? Or DCHECK might be fine. https://codereview.chromium.org/2791983004/diff/790001/chromeos/dbus/proto/me... File chromeos/dbus/proto/media_perception.proto (right): https://codereview.chromium.org/2791983004/diff/790001/chromeos/dbus/proto/me... chromeos/dbus/proto/media_perception.proto:11: // here (the other internal to Google) - that must be updated together. Maybe: This file has to kept in sync with changes in media analytics implementation (which is external to Chromium). Btw. how will the media perception implementation on cros side get updated, could this proto be updated in a similar fashion (and pulled in here via deps), at least eventually. I'm fine with keeping this here for now.
https://codereview.chromium.org/2791983004/diff/790001/chromeos/dbus/proto/me... File chromeos/dbus/proto/media_perception.proto (right): https://codereview.chromium.org/2791983004/diff/790001/chromeos/dbus/proto/me... chromeos/dbus/proto/media_perception.proto:11: // here (the other internal to Google) - that must be updated together. "...that must be updated together." This is not quite correct. This proto needs to be kept in sync with the proto used in the binary checked into Chrome OS. Can we change the comment to explicitly state that? Something like, "This .proto needs to be the compatible with the version used in the binary checked into the Chromebox for Meetings private overlay." would be more accurate and useful.
Fake client has custom method for firing media perception event. Addressing more comments. https://codereview.chromium.org/2791983004/diff/790001/chromeos/dbus/fake_med... File chromeos/dbus/fake_media_analytics_client.cc (right): https://codereview.chromium.org/2791983004/diff/790001/chromeos/dbus/fake_med... chromeos/dbus/fake_media_analytics_client.cc:18: // Override these initializations to do nothing. On 2017/05/09 20:28:40, tbarzic wrote: > nit: no comment needed Done. https://codereview.chromium.org/2791983004/diff/790001/chromeos/dbus/fake_med... chromeos/dbus/fake_media_analytics_client.cc:38: if (state.has_status()) { On 2017/05/09 20:28:40, tbarzic wrote: > Status should be set here, right? Done. https://codereview.chromium.org/2791983004/diff/790001/chromeos/dbus/fake_med... chromeos/dbus/fake_media_analytics_client.cc:53: FROM_HERE, base::Bind(&FakeMediaAnalyticsClient::OnMediaPerception, On 2017/05/09 20:28:40, tbarzic wrote: > This seems unintuitive.. if you need the fired in tests, I'd add an explicit > method to the fake for it Done. https://codereview.chromium.org/2791983004/diff/790001/chromeos/dbus/fake_med... chromeos/dbus/fake_media_analytics_client.cc:67: diagnostics.add_perception_sample()->mutable_frame_perception()->set_frame_id( On 2017/05/09 20:28:41, tbarzic wrote: > should frames returned here ahve unique ids? Only returning one fake PerceptionSample here, so there is only one Frame. Trusting DiagnosticsProtoToIdl unittest to verify that having multiple perception samples gets converted correctly. https://codereview.chromium.org/2791983004/diff/790001/chromeos/dbus/fake_med... File chromeos/dbus/fake_media_analytics_client.h (right): https://codereview.chromium.org/2791983004/diff/790001/chromeos/dbus/fake_med... chromeos/dbus/fake_media_analytics_client.h:20: // D-Bus communications to an external process. On 2017/05/09 20:28:41, tbarzic wrote: > Drop the part about testing - this is used more widely than that (e.g. on Linux > Chrome OS builds) Done. https://codereview.chromium.org/2791983004/diff/790001/chromeos/dbus/fake_ups... File chromeos/dbus/fake_upstart_client.cc (right): https://codereview.chromium.org/2791983004/diff/790001/chromeos/dbus/fake_ups... chromeos/dbus/fake_upstart_client.cc:32: void FakeUpstartClient::StartMediaAnalytics(const UpstartCallback& callback) { On 2017/05/09 20:28:41, tbarzic wrote: > it might be useful to tailor FakeMediaAnalytics client based on this - e.g. > make all its methods fail until this is called. Acknowledged. For now, going to keep them independent if that's okay. https://codereview.chromium.org/2791983004/diff/790001/chromeos/dbus/media_an... File chromeos/dbus/media_analytics_client.cc (right): https://codereview.chromium.org/2791983004/diff/790001/chromeos/dbus/media_an... chromeos/dbus/media_analytics_client.cc:59: LOG(WARNING) << "Attempting to SetState without status set."; On 2017/05/09 20:28:41, tbarzic wrote: > should this case cause an error callback to be invoked? > Or DCHECK might be fine. Done. https://codereview.chromium.org/2791983004/diff/790001/chromeos/dbus/proto/me... File chromeos/dbus/proto/media_perception.proto (right): https://codereview.chromium.org/2791983004/diff/790001/chromeos/dbus/proto/me... chromeos/dbus/proto/media_perception.proto:11: // here (the other internal to Google) - that must be updated together. On 2017/05/09 20:38:49, rkc1 wrote: > "...that must be updated together." > > This is not quite correct. This proto needs to be kept in sync with the proto > used in the binary checked into Chrome OS. Can we change the comment to > explicitly state that? > > Something like, > "This .proto needs to be the compatible with the version used in the binary > checked into the Chromebox for Meetings private overlay." would be more accurate > and useful. Done.
https://codereview.chromium.org/2791983004/diff/790001/chromeos/dbus/fake_med... File chromeos/dbus/fake_media_analytics_client.cc (right): https://codereview.chromium.org/2791983004/diff/790001/chromeos/dbus/fake_med... chromeos/dbus/fake_media_analytics_client.cc:38: if (state.has_status()) { On 2017/05/09 22:21:25, Luke Sorenson wrote: > On 2017/05/09 20:28:40, tbarzic wrote: > > Status should be set here, right? > > Done. sorry for not being precise enough... I think you should DCHECK state.has_status() https://codereview.chromium.org/2791983004/diff/790001/chromeos/dbus/fake_med... chromeos/dbus/fake_media_analytics_client.cc:67: diagnostics.add_perception_sample()->mutable_frame_perception()->set_frame_id( On 2017/05/09 22:21:24, Luke Sorenson wrote: > On 2017/05/09 20:28:41, tbarzic wrote: > > should frames returned here ahve unique ids? > > Only returning one fake PerceptionSample here, so there is only one Frame. > Trusting DiagnosticsProtoToIdl unittest to verify that having multiple > perception samples gets converted correctly. OK, though, my point was that this will always return the same frame id. I'd consider changing thay (e.g to have the frame set to an increasing index). Also, I'd make the returned frame value a little more representative (or maybe always return an empty value here). Ideally, in tests, there would be a way for a test to set the values to be returned here - that would be less fragile than having them rely on arbitrary values set here (and more flexible). Note that I'd be fine with tests overriding this implementations for their means, I'd just like to avoid having a test rely on values set here by default. https://codereview.chromium.org/2791983004/diff/790001/chromeos/dbus/fake_ups... File chromeos/dbus/fake_upstart_client.cc (right): https://codereview.chromium.org/2791983004/diff/790001/chromeos/dbus/fake_ups... chromeos/dbus/fake_upstart_client.cc:32: void FakeUpstartClient::StartMediaAnalytics(const UpstartCallback& callback) { On 2017/05/09 22:21:25, Luke Sorenson wrote: > On 2017/05/09 20:28:41, tbarzic wrote: > > it might be useful to tailor FakeMediaAnalytics client based on this - e.g. > > make all its methods fail until this is called. > > Acknowledged. For now, going to keep them independent if that's okay. I'd prefer it done now, given that it would match real device behavior more exactly; and to keep it consistent with the fake behavior for authpolicy client. https://codereview.chromium.org/2791983004/diff/810001/chromeos/dbus/fake_med... File chromeos/dbus/fake_media_analytics_client.h (right): https://codereview.chromium.org/2791983004/diff/810001/chromeos/dbus/fake_med... chromeos/dbus/fake_media_analytics_client.h:26: void FireMediaPerceptionEvent(); I'd move this after overrides, also I'd pass it the event to fire as an agrument https://codereview.chromium.org/2791983004/diff/810001/chromeos/dbus/media_an... File chromeos/dbus/media_analytics_client.cc (right): https://codereview.chromium.org/2791983004/diff/810001/chromeos/dbus/media_an... chromeos/dbus/media_analytics_client.cc:52: DCHECK(state.has_status()) << "Attempting to SetState without status set."; nit: new line after dcheck https://codereview.chromium.org/2791983004/diff/810001/chromeos/dbus/media_an... File chromeos/dbus/media_analytics_client.h (right): https://codereview.chromium.org/2791983004/diff/810001/chromeos/dbus/media_an... chromeos/dbus/media_analytics_client.h:37: virtual void SetState(const mri::State& state, document that state status has to be set.
https://codereview.chromium.org/2791983004/diff/790001/chromeos/dbus/fake_med... File chromeos/dbus/fake_media_analytics_client.cc (right): https://codereview.chromium.org/2791983004/diff/790001/chromeos/dbus/fake_med... chromeos/dbus/fake_media_analytics_client.cc:38: if (state.has_status()) { On 2017/05/09 22:21:25, Luke Sorenson wrote: > On 2017/05/09 20:28:40, tbarzic wrote: > > Status should be set here, right? > > Done. sorry for not being precise enough... I think you should DCHECK state.has_status() https://codereview.chromium.org/2791983004/diff/790001/chromeos/dbus/fake_med... chromeos/dbus/fake_media_analytics_client.cc:67: diagnostics.add_perception_sample()->mutable_frame_perception()->set_frame_id( On 2017/05/09 22:21:24, Luke Sorenson wrote: > On 2017/05/09 20:28:41, tbarzic wrote: > > should frames returned here ahve unique ids? > > Only returning one fake PerceptionSample here, so there is only one Frame. > Trusting DiagnosticsProtoToIdl unittest to verify that having multiple > perception samples gets converted correctly. OK, though, my point was that this will always return the same frame id. I'd consider changing thay (e.g to have the frame set to an increasing index). Also, I'd make the returned frame value a little more representative (or maybe always return an empty value here). Ideally, in tests, there would be a way for a test to set the values to be returned here - that would be less fragile than having them rely on arbitrary values set here (and more flexible). Note that I'd be fine with tests overriding this implementations for their means, I'd just like to avoid having a test rely on values set here by default. https://codereview.chromium.org/2791983004/diff/790001/chromeos/dbus/fake_ups... File chromeos/dbus/fake_upstart_client.cc (right): https://codereview.chromium.org/2791983004/diff/790001/chromeos/dbus/fake_ups... chromeos/dbus/fake_upstart_client.cc:32: void FakeUpstartClient::StartMediaAnalytics(const UpstartCallback& callback) { On 2017/05/09 22:21:25, Luke Sorenson wrote: > On 2017/05/09 20:28:41, tbarzic wrote: > > it might be useful to tailor FakeMediaAnalytics client based on this - e.g. > > make all its methods fail until this is called. > > Acknowledged. For now, going to keep them independent if that's okay. I'd prefer it done now, given that it would match real device behavior more exactly; and to keep it consistent with the fake behavior for authpolicy client. https://codereview.chromium.org/2791983004/diff/810001/chromeos/dbus/fake_med... File chromeos/dbus/fake_media_analytics_client.h (right): https://codereview.chromium.org/2791983004/diff/810001/chromeos/dbus/fake_med... chromeos/dbus/fake_media_analytics_client.h:26: void FireMediaPerceptionEvent(); I'd move this after overrides, also I'd pass it the event to fire as an agrument https://codereview.chromium.org/2791983004/diff/810001/chromeos/dbus/media_an... File chromeos/dbus/media_analytics_client.cc (right): https://codereview.chromium.org/2791983004/diff/810001/chromeos/dbus/media_an... chromeos/dbus/media_analytics_client.cc:52: DCHECK(state.has_status()) << "Attempting to SetState without status set."; nit: new line after dcheck https://codereview.chromium.org/2791983004/diff/810001/chromeos/dbus/media_an... File chromeos/dbus/media_analytics_client.h (right): https://codereview.chromium.org/2791983004/diff/810001/chromeos/dbus/media_an... chromeos/dbus/media_analytics_client.h:37: virtual void SetState(const mri::State& state, document that state status has to be set.
Thanks, addressed comments. PTAL (uploading new patch set) https://codereview.chromium.org/2791983004/diff/790001/chromeos/dbus/fake_med... File chromeos/dbus/fake_media_analytics_client.cc (right): https://codereview.chromium.org/2791983004/diff/790001/chromeos/dbus/fake_med... chromeos/dbus/fake_media_analytics_client.cc:38: if (state.has_status()) { On 2017/05/10 03:26:24, tbarzic wrote: > On 2017/05/09 22:21:25, Luke Sorenson wrote: > > On 2017/05/09 20:28:40, tbarzic wrote: > > > Status should be set here, right? > > > > Done. > > sorry for not being precise enough... > I think you should DCHECK state.has_status() Done. https://codereview.chromium.org/2791983004/diff/790001/chromeos/dbus/fake_med... chromeos/dbus/fake_media_analytics_client.cc:67: diagnostics.add_perception_sample()->mutable_frame_perception()->set_frame_id( On 2017/05/10 03:26:24, tbarzic wrote: > On 2017/05/09 22:21:24, Luke Sorenson wrote: > > On 2017/05/09 20:28:41, tbarzic wrote: > > > should frames returned here ahve unique ids? > > > > Only returning one fake PerceptionSample here, so there is only one Frame. > > Trusting DiagnosticsProtoToIdl unittest to verify that having multiple > > perception samples gets converted correctly. > > OK, though, my point was that this will always return the same frame id. > I'd consider changing thay (e.g to have the frame set to an increasing index). > Also, I'd make the returned frame value a little more representative (or maybe > always return an empty value here). > > Ideally, in tests, there would be a way for a test to set the values to be > returned here - that would be less fragile than having them rely on arbitrary > values set here (and more flexible). Note that I'd be fine with tests overriding > this implementations for their means, I'd just like to avoid having a test rely > on values set here by default. Done. https://codereview.chromium.org/2791983004/diff/790001/chromeos/dbus/fake_ups... File chromeos/dbus/fake_upstart_client.cc (right): https://codereview.chromium.org/2791983004/diff/790001/chromeos/dbus/fake_ups... chromeos/dbus/fake_upstart_client.cc:32: void FakeUpstartClient::StartMediaAnalytics(const UpstartCallback& callback) { On 2017/05/10 03:26:24, tbarzic wrote: > On 2017/05/09 22:21:25, Luke Sorenson wrote: > > On 2017/05/09 20:28:41, tbarzic wrote: > > > it might be useful to tailor FakeMediaAnalytics client based on this - e.g. > > > make all its methods fail until this is called. > > > > Acknowledged. For now, going to keep them independent if that's okay. > > I'd prefer it done now, given that it would match real device behavior more > exactly; and to keep it consistent with the fake behavior for authpolicy client. Done. https://codereview.chromium.org/2791983004/diff/810001/chromeos/dbus/fake_med... File chromeos/dbus/fake_media_analytics_client.h (right): https://codereview.chromium.org/2791983004/diff/810001/chromeos/dbus/fake_med... chromeos/dbus/fake_media_analytics_client.h:26: void FireMediaPerceptionEvent(); On 2017/05/10 03:26:25, tbarzic wrote: > I'd move this after overrides, also I'd pass it the event to fire as an agrument Moved below overrides. I also don't understand what you mean by pass the event to fire as an argument. Don't we want to test the code-path of adding a listener to MediaPerception events and then firing those events through the MediaPerceptionAPIManager? https://codereview.chromium.org/2791983004/diff/810001/chromeos/dbus/media_an... File chromeos/dbus/media_analytics_client.h (right): https://codereview.chromium.org/2791983004/diff/810001/chromeos/dbus/media_an... chromeos/dbus/media_analytics_client.h:37: virtual void SetState(const mri::State& state, On 2017/05/10 03:26:25, tbarzic wrote: > document that state status has to be set. Done.
https://codereview.chromium.org/2791983004/diff/810001/chromeos/dbus/fake_med... File chromeos/dbus/fake_media_analytics_client.h (right): https://codereview.chromium.org/2791983004/diff/810001/chromeos/dbus/fake_med... chromeos/dbus/fake_media_analytics_client.h:26: void FireMediaPerceptionEvent(); On 2017/05/10 16:38:22, Luke Sorenson wrote: > On 2017/05/10 03:26:25, tbarzic wrote: > > I'd move this after overrides, also I'd pass it the event to fire as an > agrument > > Moved below overrides. I also don't understand what you mean by pass the event > to fire as an argument. Don't we want to test the code-path of adding a listener > to MediaPerception events and then firing those events through the > MediaPerceptionAPIManager? I mean make this do something like: void FireMediaPerceptionEvent(mri::MediaPerception perception) { PostTask(base::Bind(&OnMediaPerception, weak_factory_.GetWeakPtr(), perception); } I'd like to avoid tests relying on arbitrary event values set here. https://codereview.chromium.org/2791983004/diff/850001/chromeos/dbus/fake_med... File chromeos/dbus/fake_media_analytics_client.cc (right): https://codereview.chromium.org/2791983004/diff/850001/chromeos/dbus/fake_med... chromeos/dbus/fake_media_analytics_client.cc:22: LOG(ERROR) << "Mocked media analytics process not running."; s/Mocked/Fake/ https://codereview.chromium.org/2791983004/diff/850001/chromeos/dbus/fake_med... File chromeos/dbus/fake_media_analytics_client.h (right): https://codereview.chromium.org/2791983004/diff/850001/chromeos/dbus/fake_med... chromeos/dbus/fake_media_analytics_client.h:45: void SetFrameId(int frame_id); I'd pass in mri::Diagnostics, rather than just frame ID.
Thanks! FakeMediaAnalyticsClient now accepts mri::MediaPerception and mri::Diagnostics. PTAL https://codereview.chromium.org/2791983004/diff/810001/chromeos/dbus/fake_med... File chromeos/dbus/fake_media_analytics_client.h (right): https://codereview.chromium.org/2791983004/diff/810001/chromeos/dbus/fake_med... chromeos/dbus/fake_media_analytics_client.h:26: void FireMediaPerceptionEvent(); On 2017/05/10 23:59:29, tbarzic wrote: > On 2017/05/10 16:38:22, Luke Sorenson wrote: > > On 2017/05/10 03:26:25, tbarzic wrote: > > > I'd move this after overrides, also I'd pass it the event to fire as an > > agrument > > > > Moved below overrides. I also don't understand what you mean by pass the event > > to fire as an argument. Don't we want to test the code-path of adding a > listener > > to MediaPerception events and then firing those events through the > > MediaPerceptionAPIManager? > > I mean make this do something like: > void FireMediaPerceptionEvent(mri::MediaPerception perception) { > PostTask(base::Bind(&OnMediaPerception, weak_factory_.GetWeakPtr(), > perception); > } > > I'd like to avoid tests relying on arbitrary event values set here. Done. https://codereview.chromium.org/2791983004/diff/850001/chromeos/dbus/fake_med... File chromeos/dbus/fake_media_analytics_client.cc (right): https://codereview.chromium.org/2791983004/diff/850001/chromeos/dbus/fake_med... chromeos/dbus/fake_media_analytics_client.cc:22: LOG(ERROR) << "Mocked media analytics process not running."; On 2017/05/10 23:59:29, tbarzic wrote: > s/Mocked/Fake/ Done. https://codereview.chromium.org/2791983004/diff/850001/chromeos/dbus/fake_med... File chromeos/dbus/fake_media_analytics_client.h (right): https://codereview.chromium.org/2791983004/diff/850001/chromeos/dbus/fake_med... chromeos/dbus/fake_media_analytics_client.h:45: void SetFrameId(int frame_id); On 2017/05/10 23:59:29, tbarzic wrote: > I'd pass in mri::Diagnostics, rather than just frame ID. Done.
Mostly nits left :) https://codereview.chromium.org/2791983004/diff/870001/chromeos/dbus/fake_med... File chromeos/dbus/fake_media_analytics_client.cc (right): https://codereview.chromium.org/2791983004/diff/870001/chromeos/dbus/fake_med... chromeos/dbus/fake_media_analytics_client.cc:23: LOG(ERROR) << "Fake media analytics process not running."; maybe return false here, so tests can assert the event was fired https://codereview.chromium.org/2791983004/diff/870001/chromeos/dbus/fake_med... chromeos/dbus/fake_media_analytics_client.cc:48: LOG(ERROR) << "Fake media analytics process not running."; I'd consider removing these logs https://codereview.chromium.org/2791983004/diff/870001/chromeos/dbus/fake_med... chromeos/dbus/fake_media_analytics_client.cc:97: if (media_perception_signal_handler_.is_null()) { nit: I prefer one line ifs without {} https://codereview.chromium.org/2791983004/diff/870001/chromeos/dbus/fake_med... File chromeos/dbus/fake_media_analytics_client.h (right): https://codereview.chromium.org/2791983004/diff/870001/chromeos/dbus/fake_med... chromeos/dbus/fake_media_analytics_client.h:28: nit: remove new lines between overrides of the same base class https://codereview.chromium.org/2791983004/diff/870001/chromeos/dbus/fake_med... chromeos/dbus/fake_media_analytics_client.h:39: // recieved by the app. Just: Fires a fake media perception event. https://codereview.chromium.org/2791983004/diff/870001/chromeos/dbus/fake_med... chromeos/dbus/fake_media_analytics_client.h:43: // to set the value being returned. nit: remove the second sentence. Maybe append ForTesting to this and FireMediaPerceptionEvent https://codereview.chromium.org/2791983004/diff/870001/chromeos/dbus/fake_med... chromeos/dbus/fake_media_analytics_client.h:46: // Make the media analytics process as launched or not running. I'm fine without this comment - process_running_ comment is enough. https://codereview.chromium.org/2791983004/diff/870001/chromeos/dbus/fake_med... chromeos/dbus/fake_media_analytics_client.h:52: // Simply echoes back the previously set state. nit: no "Simply" https://codereview.chromium.org/2791983004/diff/870001/chromeos/dbus/fake_med... chromeos/dbus/fake_media_analytics_client.h:55: // Returns a fake, nearly empty serialized Diagnostic proto. nit: Update comment in respect to SetDiagnostics comment. https://codereview.chromium.org/2791983004/diff/870001/chromeos/dbus/fake_med... chromeos/dbus/fake_media_analytics_client.h:58: // Returns a fake, nearly empty serialized MediaPerception proto. nit: Update the comment. https://codereview.chromium.org/2791983004/diff/870001/chromeos/dbus/fake_med... chromeos/dbus/fake_media_analytics_client.h:70: // Stores the launch state of the fake media analytics process. Whether the fake media analytics was started (for example by the fake upstart client) - If not set, all requests to this client will fail. https://codereview.chromium.org/2791983004/diff/870001/chromeos/dbus/media_an... File chromeos/dbus/media_analytics_client.cc (right): https://codereview.chromium.org/2791983004/diff/870001/chromeos/dbus/media_an... chromeos/dbus/media_analytics_client.cc:51: // Check that a new state is requested. no need for the comment here, the code itself is clear enough nit: add a new line after DCHECK; and move it before creating method_call https://codereview.chromium.org/2791983004/diff/870001/chromeos/dbus/media_an... chromeos/dbus/media_analytics_client.cc:104: } nit: new line here https://codereview.chromium.org/2791983004/diff/870001/chromeos/dbus/media_an... File chromeos/dbus/media_analytics_client.h (right): https://codereview.chromium.org/2791983004/diff/870001/chromeos/dbus/media_an... chromeos/dbus/media_analytics_client.h:34: // APIs for getting and setting the state of the media analytics process over Gets the media analytics process state. https://codereview.chromium.org/2791983004/diff/870001/chromeos/dbus/media_an... chromeos/dbus/media_analytics_client.h:37: // For setting state, state.status must be set. Sets the media analytics service state. |state.status| is expected to be set. (and new line before this line) https://codereview.chromium.org/2791983004/diff/870001/chromeos/dbus/upstart_... File chromeos/dbus/upstart_client.h (right): https://codereview.chromium.org/2791983004/diff/870001/chromeos/dbus/upstart_... chromeos/dbus/upstart_client.h:33: // Process management of a separate media analytics process. Comment each of the methods: Starts media analytics process. Stops media analytics process.
Addressed nits. PTAL https://codereview.chromium.org/2791983004/diff/870001/chromeos/dbus/fake_med... File chromeos/dbus/fake_media_analytics_client.cc (right): https://codereview.chromium.org/2791983004/diff/870001/chromeos/dbus/fake_med... chromeos/dbus/fake_media_analytics_client.cc:23: LOG(ERROR) << "Fake media analytics process not running."; On 2017/05/11 16:55:55, tbarzic wrote: > maybe return false here, so tests can assert the event was fired Done. https://codereview.chromium.org/2791983004/diff/870001/chromeos/dbus/fake_med... chromeos/dbus/fake_media_analytics_client.cc:48: LOG(ERROR) << "Fake media analytics process not running."; On 2017/05/11 16:55:55, tbarzic wrote: > I'd consider removing these logs Done. https://codereview.chromium.org/2791983004/diff/870001/chromeos/dbus/fake_med... chromeos/dbus/fake_media_analytics_client.cc:97: if (media_perception_signal_handler_.is_null()) { On 2017/05/11 16:55:55, tbarzic wrote: > nit: I prefer one line ifs without {} Done. https://codereview.chromium.org/2791983004/diff/870001/chromeos/dbus/fake_med... File chromeos/dbus/fake_media_analytics_client.h (right): https://codereview.chromium.org/2791983004/diff/870001/chromeos/dbus/fake_med... chromeos/dbus/fake_media_analytics_client.h:28: On 2017/05/11 16:55:55, tbarzic wrote: > nit: remove new lines between overrides of the same base class Done. https://codereview.chromium.org/2791983004/diff/870001/chromeos/dbus/fake_med... chromeos/dbus/fake_media_analytics_client.h:39: // recieved by the app. On 2017/05/11 16:55:55, tbarzic wrote: > Just: Fires a fake media perception event. Done. https://codereview.chromium.org/2791983004/diff/870001/chromeos/dbus/fake_med... chromeos/dbus/fake_media_analytics_client.h:43: // to set the value being returned. On 2017/05/11 16:55:55, tbarzic wrote: > nit: remove the second sentence. > > Maybe append ForTesting to this and FireMediaPerceptionEvent Done. https://codereview.chromium.org/2791983004/diff/870001/chromeos/dbus/fake_med... chromeos/dbus/fake_media_analytics_client.h:46: // Make the media analytics process as launched or not running. On 2017/05/11 16:55:55, tbarzic wrote: > I'm fine without this comment - process_running_ comment is enough. Done. https://codereview.chromium.org/2791983004/diff/870001/chromeos/dbus/fake_med... chromeos/dbus/fake_media_analytics_client.h:52: // Simply echoes back the previously set state. On 2017/05/11 16:55:55, tbarzic wrote: > nit: no "Simply" Done. https://codereview.chromium.org/2791983004/diff/870001/chromeos/dbus/fake_med... chromeos/dbus/fake_media_analytics_client.h:55: // Returns a fake, nearly empty serialized Diagnostic proto. On 2017/05/11 16:55:55, tbarzic wrote: > nit: Update comment in respect to SetDiagnostics comment. Done. https://codereview.chromium.org/2791983004/diff/870001/chromeos/dbus/fake_med... chromeos/dbus/fake_media_analytics_client.h:58: // Returns a fake, nearly empty serialized MediaPerception proto. On 2017/05/11 16:55:55, tbarzic wrote: > nit: Update the comment. Done. https://codereview.chromium.org/2791983004/diff/870001/chromeos/dbus/fake_med... chromeos/dbus/fake_media_analytics_client.h:70: // Stores the launch state of the fake media analytics process. On 2017/05/11 16:55:55, tbarzic wrote: > Whether the fake media analytics was started (for example by the fake upstart > client) - If not set, all requests to this client will fail. Done. https://codereview.chromium.org/2791983004/diff/870001/chromeos/dbus/media_an... File chromeos/dbus/media_analytics_client.cc (right): https://codereview.chromium.org/2791983004/diff/870001/chromeos/dbus/media_an... chromeos/dbus/media_analytics_client.cc:51: // Check that a new state is requested. On 2017/05/11 16:55:55, tbarzic wrote: > no need for the comment here, the code itself is clear enough > nit: add a new line after DCHECK; and move it before creating method_call Done. https://codereview.chromium.org/2791983004/diff/870001/chromeos/dbus/media_an... chromeos/dbus/media_analytics_client.cc:104: } On 2017/05/11 16:55:55, tbarzic wrote: > nit: new line here Done. https://codereview.chromium.org/2791983004/diff/870001/chromeos/dbus/media_an... File chromeos/dbus/media_analytics_client.h (right): https://codereview.chromium.org/2791983004/diff/870001/chromeos/dbus/media_an... chromeos/dbus/media_analytics_client.h:34: // APIs for getting and setting the state of the media analytics process over On 2017/05/11 16:55:55, tbarzic wrote: > Gets the media analytics process state. Done. https://codereview.chromium.org/2791983004/diff/870001/chromeos/dbus/media_an... chromeos/dbus/media_analytics_client.h:37: // For setting state, state.status must be set. On 2017/05/11 16:55:55, tbarzic wrote: > Sets the media analytics service state. |state.status| is expected to be set. > > (and new line before this line) Done. https://codereview.chromium.org/2791983004/diff/870001/chromeos/dbus/upstart_... File chromeos/dbus/upstart_client.h (right): https://codereview.chromium.org/2791983004/diff/870001/chromeos/dbus/upstart_... chromeos/dbus/upstart_client.h:33: // Process management of a separate media analytics process. On 2017/05/11 16:55:55, tbarzic wrote: > Comment each of the methods: > > Starts media analytics process. > > Stops media analytics process. Done.
lgtm https://codereview.chromium.org/2791983004/diff/890001/chromeos/dbus/fake_med... File chromeos/dbus/fake_media_analytics_client.cc (right): https://codereview.chromium.org/2791983004/diff/890001/chromeos/dbus/fake_med... chromeos/dbus/fake_media_analytics_client.cc:22: if (!process_running_) { nit: no {} https://codereview.chromium.org/2791983004/diff/890001/chromeos/dbus/fake_med... File chromeos/dbus/fake_media_analytics_client.h (right): https://codereview.chromium.org/2791983004/diff/890001/chromeos/dbus/fake_med... chromeos/dbus/fake_media_analytics_client.h:31: nit: no new line here https://codereview.chromium.org/2791983004/diff/890001/chromeos/dbus/fake_med... chromeos/dbus/fake_media_analytics_client.h:51: // Callback fires the Diagnostics proto provided in SetDiagnostics. Runs callback with ... https://codereview.chromium.org/2791983004/diff/890001/chromeos/dbus/fake_med... chromeos/dbus/fake_media_analytics_client.h:54: // Callback fires a MediaPerception proto provided in Runs callback with ... https://codereview.chromium.org/2791983004/diff/890001/chromeos/dbus/fake_med... chromeos/dbus/fake_media_analytics_client.h:61: // Stores a fake current state for the media analytics process. nit: drop "Stores" in these comments https://codereview.chromium.org/2791983004/diff/890001/chromeos/dbus/media_an... File chromeos/dbus/media_analytics_client.cc (right): https://codereview.chromium.org/2791983004/diff/890001/chromeos/dbus/media_an... chromeos/dbus/media_analytics_client.cc:102: LOG(WARNING) << "Received DetectionSignal but handler is null."; remove this warning log - the signal handler not being set seems like a real possibility https://codereview.chromium.org/2791983004/diff/890001/chromeos/dbus/media_an... File chromeos/dbus/media_analytics_client.h (right): https://codereview.chromium.org/2791983004/diff/890001/chromeos/dbus/media_an... chromeos/dbus/media_analytics_client.h:47: virtual void ClearMediaPerceptionSignalHandler() = 0; nit: add a comment that this resets the signal handler previously set by SetMediaPerceptionSignalHandler https://codereview.chromium.org/2791983004/diff/890001/chromeos/dbus/upstart_... File chromeos/dbus/upstart_client.h (right): https://codereview.chromium.org/2791983004/diff/890001/chromeos/dbus/upstart_... chromeos/dbus/upstart_client.h:33: // Starts the separate media analytics process. nit: it's unclear to what separate refers. I'd just remove it.
Addressed final comments. Thanks for the review! https://codereview.chromium.org/2791983004/diff/890001/chromeos/dbus/fake_med... File chromeos/dbus/fake_media_analytics_client.cc (right): https://codereview.chromium.org/2791983004/diff/890001/chromeos/dbus/fake_med... chromeos/dbus/fake_media_analytics_client.cc:22: if (!process_running_) { On 2017/05/11 18:49:57, tbarzic wrote: > nit: no {} Done. https://codereview.chromium.org/2791983004/diff/890001/chromeos/dbus/fake_med... File chromeos/dbus/fake_media_analytics_client.h (right): https://codereview.chromium.org/2791983004/diff/890001/chromeos/dbus/fake_med... chromeos/dbus/fake_media_analytics_client.h:31: On 2017/05/11 18:49:57, tbarzic wrote: > nit: no new line here Done. https://codereview.chromium.org/2791983004/diff/890001/chromeos/dbus/fake_med... chromeos/dbus/fake_media_analytics_client.h:51: // Callback fires the Diagnostics proto provided in SetDiagnostics. On 2017/05/11 18:49:57, tbarzic wrote: > Runs callback with ... Done. https://codereview.chromium.org/2791983004/diff/890001/chromeos/dbus/fake_med... chromeos/dbus/fake_media_analytics_client.h:54: // Callback fires a MediaPerception proto provided in On 2017/05/11 18:49:57, tbarzic wrote: > Runs callback with ... Done. https://codereview.chromium.org/2791983004/diff/890001/chromeos/dbus/fake_med... chromeos/dbus/fake_media_analytics_client.h:61: // Stores a fake current state for the media analytics process. On 2017/05/11 18:49:57, tbarzic wrote: > nit: drop "Stores" in these comments Done. https://codereview.chromium.org/2791983004/diff/890001/chromeos/dbus/media_an... File chromeos/dbus/media_analytics_client.cc (right): https://codereview.chromium.org/2791983004/diff/890001/chromeos/dbus/media_an... chromeos/dbus/media_analytics_client.cc:102: LOG(WARNING) << "Received DetectionSignal but handler is null."; On 2017/05/11 18:49:57, tbarzic wrote: > remove this warning log - the signal handler not being set seems like a real > possibility Done. https://codereview.chromium.org/2791983004/diff/890001/chromeos/dbus/media_an... File chromeos/dbus/media_analytics_client.h (right): https://codereview.chromium.org/2791983004/diff/890001/chromeos/dbus/media_an... chromeos/dbus/media_analytics_client.h:47: virtual void ClearMediaPerceptionSignalHandler() = 0; On 2017/05/11 18:49:57, tbarzic wrote: > nit: add a comment that this resets the signal handler previously set by > SetMediaPerceptionSignalHandler Done. https://codereview.chromium.org/2791983004/diff/890001/chromeos/dbus/upstart_... File chromeos/dbus/upstart_client.h (right): https://codereview.chromium.org/2791983004/diff/890001/chromeos/dbus/upstart_... chromeos/dbus/upstart_client.h:33: // Starts the separate media analytics process. On 2017/05/11 18:49:57, tbarzic wrote: > nit: it's unclear to what separate refers. I'd just remove it. Done.
The CQ bit was checked by lasoren@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bmayer@chromium.org, stevenjb@chromium.org, tbarzic@chromium.org Link to the patchset: https://codereview.chromium.org/2791983004/#ps930001 (title: "Rebased with origin/master")
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 tbarzic@chromium.org
On 2017/05/11 22:27:35, commit-bot: I haz the power wrote: > CQ is trying da patch. > > Follow status at: > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or... you have to pull in cros_system_api changes before landing this
Good catch, thanks. I'll wait until the service constants go in.
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, bmayer@chromium.org, tbarzic@chromium.org Link to the patchset: https://codereview.chromium.org/2791983004/#ps950001 (title: "Changing the type of a couple vars to const.")
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_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_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
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": 950001, "attempt_start_ts": 1494873952278180,
"parent_rev": "4bcb9954c02c7dd9c7cb2a0756e4a3002ec66e5d", "commit_rev":
"f933419fa6f3b7a34e6f035595b8f2dad44ce34c"}
Message was sent while issue was closed.
Description was changed from ========== DBus MediaAnalyticsClient and media_perception pb. BUG=709180 ========== to ========== DBus MediaAnalyticsClient and media_perception pb. BUG=709180 Review-Url: https://codereview.chromium.org/2791983004 Cr-Commit-Position: refs/heads/master@{#471869} Committed: https://chromium.googlesource.com/chromium/src/+/f933419fa6f3b7a34e6f035595b8... ==========
Message was sent while issue was closed.
Committed patchset #48 (id:950001) as https://chromium.googlesource.com/chromium/src/+/f933419fa6f3b7a34e6f035595b8... |
