Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2860203002:
MediaPerceptionPrivate IDL and skeleton. (Closed)
|
Created:
3 years, 7 months ago by Luke Sorenson Modified:
3 years, 7 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, asvitkine+watch_chromium.org, extensions-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionMediaPerceptionPrivate IDL and skeleton.
This private API is for process management of and communicating with a
media analytics process running outside of Chrome. The analytics process
uses computer vision technology to send metadata about image frames over
D-Bus, and MediaPerception events are fired on the frontend.
BUG=709180, 720495
Review-Url: https://codereview.chromium.org/2860203002
Cr-Commit-Position: refs/heads/master@{#473337}
Committed: https://chromium.googlesource.com/chromium/src/+/aab09b896d22f03ccc701cfb8c6186df508405d5
Patch Set 1 #Patch Set 2 : Addressing reviewer comments on dependant CL #Patch Set 3 : Addressing comments on IDL. #
Total comments: 28
Patch Set 4 : Addressing more comments. #
Total comments: 8
Patch Set 5 : crbug for API whitelist. #
Total comments: 4
Patch Set 6 : Updating media perception private idl for GetDiagnostics. #Patch Set 7 : Rename one field in IDL #
Total comments: 13
Patch Set 8 : Addressed reviewer comments. #Patch Set 9 : Remove changes to webcam_private.idl #
Total comments: 6
Patch Set 10 : Changes to IDL and commit description. #
Total comments: 8
Patch Set 11 : Addressed comments and rebased with origin/master #Patch Set 12 : Updating commit message. #
Total comments: 4
Patch Set 13 : Addressing final comments/ #Patch Set 14 : Removing error statuses from idl. #Patch Set 15 : Added depthInMeters to Entity dictionary. #
Total comments: 29
Patch Set 16 : Addressing Devlin's comments. #Patch Set 17 : Adding Status UNKNOWN to idl. #Patch Set 18 : I actually don't need UNKNOWN because STATUS_NONE exists by default. #Patch Set 19 : Addressed Devlin's comments. #
Total comments: 4
Patch Set 20 : Addressed final comments and rebased with master. #Patch Set 21 : Added permission set unittest fix. #Depends on Patchset: Dependent Patchsets: Messages
Total messages: 46 (14 generated)
Description was changed from ========== MediaPerceptionPrivate IDL and skeleton. BUG=709180 ========== to ========== MediaPerceptionPrivate IDL and skeleton. BUG=709180 ==========
lasoren@chromium.org changed reviewers: + bmayer@chromium.org, mattfrazier@chromium.org, rkc@chromium.org, tbarzic@chromium.org, weigua@chromium.org
lasoren@chromium.org changed reviewers: + sque@chromium.org
CL 2/3 with the IDL definition, permissions and API skeleton.
https://codereview.chromium.org/2860203002/diff/40001/extensions/common/api/_... File extensions/common/api/_permission_features.json (right): https://codereview.chromium.org/2860203002/diff/40001/extensions/common/api/_... extensions/common/api/_permission_features.json:255: // TODO(lasoren): Uncomment whitelist and make browser_tests work with whitelist. can you uncomment these and link a bug for whitelisting the app for the API (rather than listing the app name- see https://cs.chromium.org/chromium/src/chrome/common/extensions/api/_features.m...). https://codereview.chromium.org/2860203002/diff/40001/extensions/common/api/m... File extensions/common/api/media_perception_private.idl (right): https://codereview.chromium.org/2860203002/diff/40001/extensions/common/api/m... extensions/common/api/media_perception_private.idl:5: // Private API for communicating with and receiving real-time media perception Private API for receiving real-time media perception information. (the rest seems more like implementation detail) https://codereview.chromium.org/2860203002/diff/40001/extensions/common/api/m... extensions/common/api/media_perception_private.idl:8: nit: no new line https://codereview.chromium.org/2860203002/diff/40001/extensions/common/api/m... extensions/common/api/media_perception_private.idl:14: // Unable to reach media analysis process. Attempt to reach media analysis process failed. https://codereview.chromium.org/2860203002/diff/40001/extensions/common/api/m... extensions/common/api/media_perception_private.idl:29: // Analysis process is running and ready to be set to state RUNNING. How about "Media analytics process is running, but not receiving image frames"? And remove this sentence from the STARTED comment? Or am I misunderstanding what the states represent? https://codereview.chromium.org/2860203002/diff/40001/extensions/common/api/m... extensions/common/api/media_perception_private.idl:35: Status? status; So, if understand this correctly, status will always be set here? Maybe make it required, so the function call fails early in case someone tries to call it with only deviceContext set? https://codereview.chromium.org/2860203002/diff/40001/extensions/common/api/m... extensions/common/api/media_perception_private.idl:46: double? x; It feels a little weird that the API allows a point with an unset coordinate, but OK. Would the app chose a default value to handle this, just ignore the point, or ignore the whole message? https://codereview.chromium.org/2860203002/diff/40001/extensions/common/api/m... extensions/common/api/media_perception_private.idl:94: // The timestamp attached with when this data originated from the analysis nit: The time the media perception data was emitted by the analysis process. https://codereview.chromium.org/2860203002/diff/40001/extensions/common/api/m... extensions/common/api/media_perception_private.idl:104: // from the media analytics process. the comments keep switching between using analytics process and analysis process.. can you switch one of them to another. https://codereview.chromium.org/2860203002/diff/40001/extensions/common/api/m... extensions/common/api/media_perception_private.idl:124: // for diagnostics (when a user reports malfunction). "to be sent" seems like the app implementation detail (and is a little misleading in this context, given that the API received this). Maybe: A buffer of image frames and the associated video analytics infrormation that can be used to diagnose a malfunction. https://codereview.chromium.org/2860203002/diff/40001/extensions/common/api/m... extensions/common/api/media_perception_private.idl:141: // the state was set as desired. nit: s/Verifies that/Can be used to verify/ https://codereview.chromium.org/2860203002/diff/40001/extensions/common/api/m... extensions/common/api/media_perception_private.idl:146: // contains image frame data and associated detections to be logged. I think it's evident from other comments what the diagnostics dictionary contains :) https://codereview.chromium.org/2860203002/diff/40001/extensions/common/api/m... extensions/common/api/media_perception_private.idl:151: // Fired when the analysis process has passed back to Chrome the current "Fired when media perception information is received from media analytics process."?
Thanks for the review tbarzic@ PTAL https://codereview.chromium.org/2860203002/diff/40001/extensions/common/api/_... File extensions/common/api/_permission_features.json (right): https://codereview.chromium.org/2860203002/diff/40001/extensions/common/api/_... extensions/common/api/_permission_features.json:255: // TODO(lasoren): Uncomment whitelist and make browser_tests work with whitelist. On 2017/05/08 22:21:07, tbarzic wrote: > can you uncomment these and link a bug for whitelisting the app for the API > (rather than listing the app name- see > https://cs.chromium.org/chromium/src/chrome/common/extensions/api/_features.m...). Uncommented. So you're saying I should create a crbug for whitelisting hotrod for this private API, and then link that bug in the comments? https://codereview.chromium.org/2860203002/diff/40001/extensions/common/api/m... File extensions/common/api/media_perception_private.idl (right): https://codereview.chromium.org/2860203002/diff/40001/extensions/common/api/m... extensions/common/api/media_perception_private.idl:5: // Private API for communicating with and receiving real-time media perception On 2017/05/08 22:21:07, tbarzic wrote: > Private API for receiving real-time media perception information. > > (the rest seems more like implementation detail) Done. https://codereview.chromium.org/2860203002/diff/40001/extensions/common/api/m... extensions/common/api/media_perception_private.idl:8: On 2017/05/08 22:21:07, tbarzic wrote: > nit: no new line Done. https://codereview.chromium.org/2860203002/diff/40001/extensions/common/api/m... extensions/common/api/media_perception_private.idl:14: // Unable to reach media analysis process. On 2017/05/08 22:21:08, tbarzic wrote: > Attempt to reach media analysis process failed. Done. https://codereview.chromium.org/2860203002/diff/40001/extensions/common/api/m... extensions/common/api/media_perception_private.idl:29: // Analysis process is running and ready to be set to state RUNNING. On 2017/05/08 22:21:07, tbarzic wrote: > How about "Media analytics process is running, but not receiving image frames"? > And remove this sentence from the STARTED comment? > Or am I misunderstanding what the states represent? I added more clarifying comments to try to help make clear what each state represents. https://codereview.chromium.org/2860203002/diff/40001/extensions/common/api/m... extensions/common/api/media_perception_private.idl:35: Status? status; On 2017/05/08 22:21:07, tbarzic wrote: > So, if understand this correctly, status will always be set here? > Maybe make it required, so the function call fails early in case someone tries > to call it with only deviceContext set? Done. https://codereview.chromium.org/2860203002/diff/40001/extensions/common/api/m... extensions/common/api/media_perception_private.idl:46: double? x; On 2017/05/08 22:21:07, tbarzic wrote: > It feels a little weird that the API allows a point with an unset coordinate, > but OK. > Would the app chose a default value to handle this, just ignore the point, or > ignore the whole message? We will be careful to always check that a value exists where we expect it to. The app would most likely ignore the whole message in that case and log an error because it really shouldn't happen, by having an optional value here we can more easily identify when a member that really should be there is somehow not getting through. https://codereview.chromium.org/2860203002/diff/40001/extensions/common/api/m... extensions/common/api/media_perception_private.idl:94: // The timestamp attached with when this data originated from the analysis On 2017/05/08 22:21:07, tbarzic wrote: > nit: The time the media perception data was emitted by the analysis process. Done. https://codereview.chromium.org/2860203002/diff/40001/extensions/common/api/m... extensions/common/api/media_perception_private.idl:104: // from the media analytics process. On 2017/05/08 22:21:07, tbarzic wrote: > the comments keep switching between using analytics process and analysis > process.. can you switch one of them to another. Done. https://codereview.chromium.org/2860203002/diff/40001/extensions/common/api/m... extensions/common/api/media_perception_private.idl:124: // for diagnostics (when a user reports malfunction). On 2017/05/08 22:21:07, tbarzic wrote: > "to be sent" seems like the app implementation detail (and is a little > misleading in this context, given that the API received this). > > Maybe: > A buffer of image frames and the associated video analytics infrormation that > can be used to diagnose a malfunction. Done. https://codereview.chromium.org/2860203002/diff/40001/extensions/common/api/m... extensions/common/api/media_perception_private.idl:141: // the state was set as desired. On 2017/05/08 22:21:07, tbarzic wrote: > nit: s/Verifies that/Can be used to verify/ Done. https://codereview.chromium.org/2860203002/diff/40001/extensions/common/api/m... extensions/common/api/media_perception_private.idl:146: // contains image frame data and associated detections to be logged. On 2017/05/08 22:21:07, tbarzic wrote: > I think it's evident from other comments what the diagnostics dictionary > contains :) Done. https://codereview.chromium.org/2860203002/diff/40001/extensions/common/api/m... extensions/common/api/media_perception_private.idl:151: // Fired when the analysis process has passed back to Chrome the current On 2017/05/08 22:21:07, tbarzic wrote: > "Fired when media perception information is received from media analytics > process."? Done.
you should also add an owners file for extensions/broeser/api/media_perception_private/ https://codereview.chromium.org/2860203002/diff/40001/extensions/common/api/_... File extensions/common/api/_permission_features.json (right): https://codereview.chromium.org/2860203002/diff/40001/extensions/common/api/_... extensions/common/api/_permission_features.json:255: // TODO(lasoren): Uncomment whitelist and make browser_tests work with whitelist. On 2017/05/09 17:41:04, Luke Sorenson wrote: > On 2017/05/08 22:21:07, tbarzic wrote: > > can you uncomment these and link a bug for whitelisting the app for the API > > (rather than listing the app name- see > > > https://cs.chromium.org/chromium/src/chrome/common/extensions/api/_features.m...). > > Uncommented. So you're saying I should create a crbug for whitelisting hotrod > for this private API, and then link that bug in the comments? yes (and you can make the bug restrictview-google)- you can see some of the other links here as an example https://codereview.chromium.org/2860203002/diff/60001/extensions/browser/api/... File extensions/browser/api/media_perception_private/BUILD.gn (right): https://codereview.chromium.org/2860203002/diff/60001/extensions/browser/api/... extensions/browser/api/media_perception_private/BUILD.gn:7: source_set("media_perception_private") { you should also add an owners file for extensions/browser/api/media_perception_private https://codereview.chromium.org/2860203002/diff/60001/extensions/common/api/_... File extensions/common/api/_permission_features.json (right): https://codereview.chromium.org/2860203002/diff/60001/extensions/common/api/_... extensions/common/api/_permission_features.json:254: "extension_types": ["extension", "legacy_packaged_app", "platform_app"], restrict this to platform apps and add "platforms": ["chromeos"] "session_types": ["kiosk"] (assuming this is only intended to be used in kiosk) https://codereview.chromium.org/2860203002/diff/60001/extensions/common/api/m... File extensions/common/api/media_perception_private.idl (right): https://codereview.chromium.org/2860203002/diff/60001/extensions/common/api/m... extensions/common/api/media_perception_private.idl:40: // Optionally add device context to setState command for starting Optional $(ref:setState) parameter. Specifies the video device the media analytics process should open. Can be set only when |status| is set to <code>UNINITIALIZED</code> or <code>SUSPENDED</code>.
Thanks, addressed comments! PTAL (uploading new patch set now) https://codereview.chromium.org/2860203002/diff/40001/extensions/common/api/_... File extensions/common/api/_permission_features.json (right): https://codereview.chromium.org/2860203002/diff/40001/extensions/common/api/_... extensions/common/api/_permission_features.json:255: // TODO(lasoren): Uncomment whitelist and make browser_tests work with whitelist. On 2017/05/09 21:03:11, tbarzic wrote: > On 2017/05/09 17:41:04, Luke Sorenson wrote: > > On 2017/05/08 22:21:07, tbarzic wrote: > > > can you uncomment these and link a bug for whitelisting the app for the API > > > (rather than listing the app name- see > > > > > > https://cs.chromium.org/chromium/src/chrome/common/extensions/api/_features.m...). > > > > Uncommented. So you're saying I should create a crbug for whitelisting hotrod > > for this private API, and then link that bug in the comments? > > yes (and you can make the bug restrictview-google)- you can see some of the > other links here as an example Done. https://codereview.chromium.org/2860203002/diff/60001/extensions/browser/api/... File extensions/browser/api/media_perception_private/BUILD.gn (right): https://codereview.chromium.org/2860203002/diff/60001/extensions/browser/api/... extensions/browser/api/media_perception_private/BUILD.gn:7: source_set("media_perception_private") { On 2017/05/09 21:03:11, tbarzic wrote: > you should also add an owners file for > extensions/browser/api/media_perception_private Done. https://codereview.chromium.org/2860203002/diff/60001/extensions/common/api/_... File extensions/common/api/_permission_features.json (right): https://codereview.chromium.org/2860203002/diff/60001/extensions/common/api/_... extensions/common/api/_permission_features.json:254: "extension_types": ["extension", "legacy_packaged_app", "platform_app"], On 2017/05/09 21:03:11, tbarzic wrote: > restrict this to platform apps > and add > "platforms": ["chromeos"] > "session_types": ["kiosk"] (assuming this is only intended to be used in kiosk) Done on platforms. However, we want to be able to test and use this API outside of kiosk mode. We've built a test app (linked in the crbug) for fast prototyping using this API. We believe the test app is an extension, and not a platform app but not exactly sure what the difference is. https://codereview.chromium.org/2860203002/diff/60001/extensions/common/api/m... File extensions/common/api/media_perception_private.idl (right): https://codereview.chromium.org/2860203002/diff/60001/extensions/common/api/m... extensions/common/api/media_perception_private.idl:40: // Optionally add device context to setState command for starting On 2017/05/09 21:03:11, tbarzic wrote: > Optional $(ref:setState) parameter. Specifies the video device the media > analytics process should open. > Can be set only when |status| is set to <code>UNINITIALIZED</code> or > <code>SUSPENDED</code>. Done.
https://codereview.chromium.org/2860203002/diff/60001/extensions/common/api/_... File extensions/common/api/_permission_features.json (right): https://codereview.chromium.org/2860203002/diff/60001/extensions/common/api/_... extensions/common/api/_permission_features.json:254: "extension_types": ["extension", "legacy_packaged_app", "platform_app"], On 2017/05/10 17:33:26, Luke Sorenson wrote: > On 2017/05/09 21:03:11, tbarzic wrote: > > restrict this to platform apps > > and add > > "platforms": ["chromeos"] > > "session_types": ["kiosk"] (assuming this is only intended to be used in > kiosk) > > Done on platforms. > > However, we want to be able to test and use this API outside of kiosk mode. > We've built a test app (linked in the crbug) for fast prototyping using this > API. > > We believe the test app is an extension, and not a platform app but not exactly > sure what the difference is. Looking at the screen shot, it looks like a platform app (and even if it wasn't, it should be), so please restrict this to platform apps. https://codereview.chromium.org/2860203002/diff/80001/extensions/common/api/m... File extensions/common/api/media_perception_private.idl (right): https://codereview.chromium.org/2860203002/diff/80001/extensions/common/api/m... extensions/common/api/media_perception_private.idl:39: Status status; add a new line between different properties (here and elsewhere) https://codereview.chromium.org/2860203002/diff/80001/extensions/common/api/m... extensions/common/api/media_perception_private.idl:107: // TODO(lasoren): Change this interface based on the compressed images coming I'd remove TODOs from here. https://codereview.chromium.org/2860203002/diff/120001/extensions/browser/api... File extensions/browser/api/media_perception_private/OWNERS (right): https://codereview.chromium.org/2860203002/diff/120001/extensions/browser/api... extensions/browser/api/media_perception_private/OWNERS:1: lasoren@chromium.org Let's change this to: rkc@chromium.org tbarzic@chromium.org https://codereview.chromium.org/2860203002/diff/120001/extensions/common/api/... File extensions/common/api/media_perception_private.idl (right): https://codereview.chromium.org/2860203002/diff/120001/extensions/common/api/... extensions/common/api/media_perception_private.idl:23: // Note: STARTED is the initial reply to SetState RUNNING. How would the app know the state's changed to RUNNING? Should there be onStateChanged event? https://codereview.chromium.org/2860203002/diff/120001/extensions/common/api/... extensions/common/api/media_perception_private.idl:42: // Can be set only when |status| of the analytics process is currently |status| has to be RUNNING (as it refers to the status property of the state) - drop || https://codereview.chromium.org/2860203002/diff/120001/extensions/common/api/... File extensions/common/api/webcam_private.idl (left): https://codereview.chromium.org/2860203002/diff/120001/extensions/common/api/... extensions/common/api/webcam_private.idl:39: undo this
Addressed comments, PTAL (uploading new patch set now) https://codereview.chromium.org/2860203002/diff/60001/extensions/common/api/_... File extensions/common/api/_permission_features.json (right): https://codereview.chromium.org/2860203002/diff/60001/extensions/common/api/_... extensions/common/api/_permission_features.json:254: "extension_types": ["extension", "legacy_packaged_app", "platform_app"], On 2017/05/10 23:45:53, tbarzic wrote: > On 2017/05/10 17:33:26, Luke Sorenson wrote: > > On 2017/05/09 21:03:11, tbarzic wrote: > > > restrict this to platform apps > > > and add > > > "platforms": ["chromeos"] > > > "session_types": ["kiosk"] (assuming this is only intended to be used in > > kiosk) > > > > Done on platforms. > > > > However, we want to be able to test and use this API outside of kiosk mode. > > We've built a test app (linked in the crbug) for fast prototyping using this > > API. > > > > We believe the test app is an extension, and not a platform app but not > exactly > > sure what the difference is. > > Looking at the screen shot, it looks like a platform app (and even if it wasn't, > it should be), so please restrict this to platform apps. Done. https://codereview.chromium.org/2860203002/diff/80001/extensions/common/api/m... File extensions/common/api/media_perception_private.idl (right): https://codereview.chromium.org/2860203002/diff/80001/extensions/common/api/m... extensions/common/api/media_perception_private.idl:39: Status status; On 2017/05/10 23:45:53, tbarzic wrote: > add a new line between different properties (here and elsewhere) Done. https://codereview.chromium.org/2860203002/diff/80001/extensions/common/api/m... extensions/common/api/media_perception_private.idl:107: // TODO(lasoren): Change this interface based on the compressed images coming On 2017/05/10 23:45:53, tbarzic wrote: > I'd remove TODOs from here. This comment is on an older patch set. TODO no longer there on current patch set. https://codereview.chromium.org/2860203002/diff/120001/extensions/browser/api... File extensions/browser/api/media_perception_private/OWNERS (right): https://codereview.chromium.org/2860203002/diff/120001/extensions/browser/api... extensions/browser/api/media_perception_private/OWNERS:1: lasoren@chromium.org On 2017/05/10 23:45:53, tbarzic wrote: > Let's change this to: > mailto:rkc@chromium.org > mailto:tbarzic@chromium.org Done. https://codereview.chromium.org/2860203002/diff/120001/extensions/common/api/... File extensions/common/api/media_perception_private.idl (right): https://codereview.chromium.org/2860203002/diff/120001/extensions/common/api/... extensions/common/api/media_perception_private.idl:23: // Note: STARTED is the initial reply to SetState RUNNING. On 2017/05/10 23:45:53, tbarzic wrote: > How would the app know the state's changed to RUNNING? > Should there be onStateChanged event? When the process has changed to state RUNNING, onMediaPerception events should start coming through, so we have that signal. We want to add an onStateChanged event going forward, but decided it was not expressly necessary for the first implementation. https://codereview.chromium.org/2860203002/diff/120001/extensions/common/api/... extensions/common/api/media_perception_private.idl:42: // Can be set only when |status| of the analytics process is currently On 2017/05/10 23:45:53, tbarzic wrote: > |status| has to be RUNNING (as it refers to the status property of the state) - > drop || Done. Although I copied and pasted this comment exactly from your last set of comments :) https://codereview.chromium.org/2860203002/diff/120001/extensions/common/api/... File extensions/common/api/webcam_private.idl (left): https://codereview.chromium.org/2860203002/diff/120001/extensions/common/api/... extensions/common/api/webcam_private.idl:39: On 2017/05/10 23:45:53, tbarzic wrote: > undo this Done. Although, I don't see why its a problem to remove unnecessary white space from this other idl.
few more nits, but looks OK https://codereview.chromium.org/2860203002/diff/120001/extensions/common/api/... File extensions/common/api/media_perception_private.idl (right): https://codereview.chromium.org/2860203002/diff/120001/extensions/common/api/... extensions/common/api/media_perception_private.idl:42: // Can be set only when |status| of the analytics process is currently On 2017/05/11 16:18:22, Luke Sorenson wrote: > On 2017/05/10 23:45:53, tbarzic wrote: > > |status| has to be RUNNING (as it refers to the status property of the state) > - > > drop || > > Done. > > Although I copied and pasted this comment exactly from your last set of comments > :) s/when the desired status is running/while it's starting/ Or maybe rephrase this as something like "the media analytics process should analyze while running." https://codereview.chromium.org/2860203002/diff/120001/extensions/common/api/... File extensions/common/api/webcam_private.idl (left): https://codereview.chromium.org/2860203002/diff/120001/extensions/common/api/... extensions/common/api/webcam_private.idl:39: On 2017/05/11 16:18:22, Luke Sorenson wrote: > On 2017/05/10 23:45:53, tbarzic wrote: > > undo this > > Done. > > Although, I don't see why its a problem to remove unnecessary white space from > this other idl. mostly because it has nothing to do with this cl :) The change itself is fine, but should not be bundled with adding something completely unrelated. https://codereview.chromium.org/2860203002/diff/160001/extensions/common/api/... File extensions/common/api/media_perception_private.idl (right): https://codereview.chromium.org/2860203002/diff/160001/extensions/common/api/... extensions/common/api/media_perception_private.idl:106: // An array of framePerceptions, often just one. I'd remove "often just one" https://codereview.chromium.org/2860203002/diff/160001/extensions/common/api/... extensions/common/api/media_perception_private.idl:150: // |callback| : The current State of the system. nit: lower case State
https://codereview.chromium.org/2860203002/diff/160001/extensions/common/api/... File extensions/common/api/_permission_features.json (right): https://codereview.chromium.org/2860203002/diff/160001/extensions/common/api/... extensions/common/api/_permission_features.json:257: "E703483CEF33DEC18B4B6DD84B5C776FB9182BDB", // http://crbug.com/720495 can you also link this bug in the CL description Speaking of the cl description, could you add some more information there. e.g. briefly describe what the API does.
Description was changed from ========== MediaPerceptionPrivate IDL and skeleton. BUG=709180 ========== to ========== MediaPerceptionPrivate IDL and skeleton. This private API is for process management of and communicating with a media analytics process running outside of Chrome. The analytics process uses computer vision technology to send metadata about image frames over D-Bus, and MediaPerception events are fired on the frontend. BUG=709180 Whitelisting API bug: http://crbug.com/720495 ==========
Addressed comments. PTAL https://codereview.chromium.org/2860203002/diff/120001/extensions/common/api/... File extensions/common/api/media_perception_private.idl (right): https://codereview.chromium.org/2860203002/diff/120001/extensions/common/api/... extensions/common/api/media_perception_private.idl:23: // Note: STARTED is the initial reply to SetState RUNNING. On 2017/05/11 16:18:22, Luke Sorenson wrote: > On 2017/05/10 23:45:53, tbarzic wrote: > > How would the app know the state's changed to RUNNING? > > Should there be onStateChanged event? > > When the process has changed to state RUNNING, onMediaPerception events should > start coming through, so we have that signal. We want to add an onStateChanged > event going forward, but decided it was not expressly necessary for the first > implementation. Done. https://codereview.chromium.org/2860203002/diff/120001/extensions/common/api/... extensions/common/api/media_perception_private.idl:42: // Can be set only when |status| of the analytics process is currently On 2017/05/11 17:16:21, tbarzic wrote: > On 2017/05/11 16:18:22, Luke Sorenson wrote: > > On 2017/05/10 23:45:53, tbarzic wrote: > > > |status| has to be RUNNING (as it refers to the status property of the > state) > > - > > > drop || > > > > Done. > > > > Although I copied and pasted this comment exactly from your last set of > comments > > :) > > s/when the desired status is running/while it's starting/ > > Or maybe rephrase this as something like "the media analytics process should > analyze while running." Done. https://codereview.chromium.org/2860203002/diff/120001/extensions/common/api/... File extensions/common/api/webcam_private.idl (left): https://codereview.chromium.org/2860203002/diff/120001/extensions/common/api/... extensions/common/api/webcam_private.idl:39: On 2017/05/11 17:16:21, tbarzic wrote: > On 2017/05/11 16:18:22, Luke Sorenson wrote: > > On 2017/05/10 23:45:53, tbarzic wrote: > > > undo this > > > > Done. > > > > Although, I don't see why its a problem to remove unnecessary white space from > > this other idl. > > mostly because it has nothing to do with this cl :) > > The change itself is fine, but should not be bundled with adding something > completely unrelated. Acknowledged. https://codereview.chromium.org/2860203002/diff/160001/extensions/common/api/... File extensions/common/api/_permission_features.json (right): https://codereview.chromium.org/2860203002/diff/160001/extensions/common/api/... extensions/common/api/_permission_features.json:257: "E703483CEF33DEC18B4B6DD84B5C776FB9182BDB", // http://crbug.com/720495 On 2017/05/11 17:53:03, tbarzic wrote: > can you also link this bug in the CL description > > Speaking of the cl description, could you add some more information there. e.g. > briefly describe what the API does. > Done. https://codereview.chromium.org/2860203002/diff/160001/extensions/common/api/... File extensions/common/api/media_perception_private.idl (right): https://codereview.chromium.org/2860203002/diff/160001/extensions/common/api/... extensions/common/api/media_perception_private.idl:106: // An array of framePerceptions, often just one. On 2017/05/11 17:16:21, tbarzic wrote: > I'd remove "often just one" Done. https://codereview.chromium.org/2860203002/diff/160001/extensions/common/api/... extensions/common/api/media_perception_private.idl:150: // |callback| : The current State of the system. On 2017/05/11 17:16:21, tbarzic wrote: > nit: lower case State Done.
https://codereview.chromium.org/2860203002/diff/180001/extensions/browser/api... File extensions/browser/api/media_perception_private/BUILD.gn (right): https://codereview.chromium.org/2860203002/diff/180001/extensions/browser/api... extensions/browser/api/media_perception_private/BUILD.gn:15: "//chromeos:media_perception_proto", you don't need this here - can you move to the cl where it's actually needed. https://codereview.chromium.org/2860203002/diff/180001/extensions/common/api/... File extensions/common/api/_permission_features.json (right): https://codereview.chromium.org/2860203002/diff/180001/extensions/common/api/... extensions/common/api/_permission_features.json:254: "extension_types": ["platform_app"], So, let's restrict this to kiosk sessions for now ("session_types": ["kiosk"]), and then add whitelist for testing app separately. (and have that one restricted to dev channel, as, from your description, it seems it would not be used elsewhere) https://codereview.chromium.org/2860203002/diff/180001/extensions/common/api/... File extensions/common/api/media_perception_private.idl (right): https://codereview.chromium.org/2860203002/diff/180001/extensions/common/api/... extensions/common/api/media_perception_private.idl:24: // When the process has changed to state RUNNING, onMediaPerception events should nit: fix line overflows actually, I'd just remove this paragraph; it seems out of place
forgot to mention, bugs should be listed as: BUG=709180, 720495
Addressed comments and rebased with origin/master. PTAL https://codereview.chromium.org/2860203002/diff/180001/extensions/browser/api... File extensions/browser/api/media_perception_private/BUILD.gn (right): https://codereview.chromium.org/2860203002/diff/180001/extensions/browser/api... extensions/browser/api/media_perception_private/BUILD.gn:15: "//chromeos:media_perception_proto", On 2017/05/11 19:20:03, tbarzic wrote: > you don't need this here - can you move to the cl where it's actually needed. Done. https://codereview.chromium.org/2860203002/diff/180001/extensions/common/api/... File extensions/common/api/_permission_features.json (right): https://codereview.chromium.org/2860203002/diff/180001/extensions/common/api/... extensions/common/api/_permission_features.json:254: "extension_types": ["platform_app"], On 2017/05/11 19:20:03, tbarzic wrote: > So, let's restrict this to kiosk sessions for now ("session_types": ["kiosk"]), > and then add whitelist for testing app separately. > (and have that one restricted to dev channel, as, from your description, it > seems it would not be used elsewhere) Done. https://codereview.chromium.org/2860203002/diff/180001/extensions/common/api/... File extensions/common/api/media_perception_private.idl (right): https://codereview.chromium.org/2860203002/diff/180001/extensions/common/api/... extensions/common/api/media_perception_private.idl:24: // When the process has changed to state RUNNING, onMediaPerception events should On 2017/05/11 19:20:03, tbarzic wrote: > nit: fix line overflows > > actually, I'd just remove this paragraph; it seems out of place Done. Although again, this is just what I copied and pasted from your last review.
Description was changed from ========== MediaPerceptionPrivate IDL and skeleton. This private API is for process management of and communicating with a media analytics process running outside of Chrome. The analytics process uses computer vision technology to send metadata about image frames over D-Bus, and MediaPerception events are fired on the frontend. BUG=709180 Whitelisting API bug: http://crbug.com/720495 ========== to ========== MediaPerceptionPrivate IDL and skeleton. This private API is for process management of and communicating with a media analytics process running outside of Chrome. The analytics process uses computer vision technology to send metadata about image frames over D-Bus, and MediaPerception events are fired on the frontend. BUG=709180, 720495 ==========
also, you should run: python tools/metrics/histograms/update_extension_permission.py other than that, lgtm https://codereview.chromium.org/2860203002/diff/180001/extensions/common/api/... File extensions/common/api/media_perception_private.idl (right): https://codereview.chromium.org/2860203002/diff/180001/extensions/common/api/... extensions/common/api/media_perception_private.idl:24: // When the process has changed to state RUNNING, onMediaPerception events should On 2017/05/11 23:28:17, Luke Sorenson wrote: > On 2017/05/11 19:20:03, tbarzic wrote: > > nit: fix line overflows > > > > actually, I'd just remove this paragraph; it seems out of place > > Done. > > Although again, this is just what I copied and pasted from your last review. I'm pretty sure you did not :P You copy-pasted your response to my question about whether onStateChanged event is needed. https://codereview.chromium.org/2860203002/diff/220001/extensions/common/api/... File extensions/common/api/_permission_features.json (right): https://codereview.chromium.org/2860203002/diff/220001/extensions/common/api/... extensions/common/api/_permission_features.json:265: "81986D4F846CEDDDB962643FA501D1780DD441BB" // http://crbug.com/720495 nit: align // https://codereview.chromium.org/2860203002/diff/220001/extensions/common/api/... File extensions/common/api/media_perception_private.idl (right): https://codereview.chromium.org/2860203002/diff/220001/extensions/common/api/... extensions/common/api/media_perception_private.idl:154: // |state| : A dictionary with the desired new state. Settable states are nit: The only settable states
Done. Thanks for the review! https://codereview.chromium.org/2860203002/diff/180001/extensions/common/api/... File extensions/common/api/media_perception_private.idl (right): https://codereview.chromium.org/2860203002/diff/180001/extensions/common/api/... extensions/common/api/media_perception_private.idl:24: // When the process has changed to state RUNNING, onMediaPerception events should On 2017/05/11 23:42:50, tbarzic wrote: > On 2017/05/11 23:28:17, Luke Sorenson wrote: > > On 2017/05/11 19:20:03, tbarzic wrote: > > > nit: fix line overflows > > > > > > actually, I'd just remove this paragraph; it seems out of place > > > > Done. > > > > Although again, this is just what I copied and pasted from your last review. > > I'm pretty sure you did not :P > You copy-pasted your response to my question about whether onStateChanged event > is needed. You're right :o https://codereview.chromium.org/2860203002/diff/220001/extensions/common/api/... File extensions/common/api/_permission_features.json (right): https://codereview.chromium.org/2860203002/diff/220001/extensions/common/api/... extensions/common/api/_permission_features.json:265: "81986D4F846CEDDDB962643FA501D1780DD441BB" // http://crbug.com/720495 On 2017/05/11 23:42:50, tbarzic wrote: > nit: align // Done. https://codereview.chromium.org/2860203002/diff/220001/extensions/common/api/... File extensions/common/api/media_perception_private.idl (right): https://codereview.chromium.org/2860203002/diff/220001/extensions/common/api/... extensions/common/api/media_perception_private.idl:154: // |state| : A dictionary with the desired new state. Settable states are On 2017/05/11 23:42:50, tbarzic wrote: > nit: The only settable states Done.
lasoren@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
Adding Devlin for owner lgtm.
On 2017/05/15 20:04:24, Luke Sorenson wrote: > Adding Devlin for owner lgtm. Does this have a design doc at all, or has it gone through any kind of API review? I can't find anything on the linked bugs.
On 2017/05/17 14:46:22, Devlin (catching up) wrote: > On 2017/05/15 20:04:24, Luke Sorenson wrote: > > Adding Devlin for owner lgtm. > > Does this have a design doc at all, or has it gone through any kind of API > review? I can't find anything on the linked bugs. Yes, sorry I forgot to link it there. I linked docs in the first crbug. I also forwarded you the original email thread with the API proposal and security LGTM.
https://codereview.chromium.org/2860203002/diff/280001/extensions/browser/api... File extensions/browser/api/media_perception_private/BUILD.gn (right): https://codereview.chromium.org/2860203002/diff/280001/extensions/browser/api... extensions/browser/api/media_perception_private/BUILD.gn:1: # Copyright 2017 The Chromium Authors. All rights reserved. Is there a compelling reason to have this as a separate build file? Having separate files for each api has led to cyclical problems, since often an API will depend on extensions/browser/ and extensions/browser/ will depend on this API. https://codereview.chromium.org/2860203002/diff/280001/extensions/browser/api... File extensions/browser/api/media_perception_private/media_perception_private_api.h (right): https://codereview.chromium.org/2860203002/diff/280001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_private_api.h:11: namespace media_perception = extensions::api::media_perception_private; namespace aliases are disallowed in .h files. https://codereview.chromium.org/2860203002/diff/280001/extensions/common/api/... File extensions/common/api/media_perception_private.idl (right): https://codereview.chromium.org/2860203002/diff/280001/extensions/common/api/... extensions/common/api/media_perception_private.idl:12: // Analytics process running and media processing pipeline started, nit: use complete sentences in comments, so something like: "*The* analytics process *is* running and..." etc (throughout this file). https://codereview.chromium.org/2860203002/diff/280001/extensions/common/api/... extensions/common/api/media_perception_private.idl:43: double? x; When would these be null? https://codereview.chromium.org/2860203002/diff/280001/extensions/common/api/... extensions/common/api/media_perception_private.idl:67: long? id; Why would these be null? I'm not familiar enough with the implementation to know if the others are appropriately optional or not. https://codereview.chromium.org/2860203002/diff/280001/extensions/common/api/... extensions/common/api/media_perception_private.idl:93: Entity[]? entities; Usually prefer an empty list rather than an optional one. https://codereview.chromium.org/2860203002/diff/280001/extensions/common/api/... extensions/common/api/media_perception_private.idl:109: RGB, Is RGB an image format? https://codereview.chromium.org/2860203002/diff/280001/extensions/common/api/... extensions/common/api/media_perception_private.idl:146: // Get the status of the media perception process. Function comments should be descriptive, not imperative (i.e., use "Gets"). Same for below. https://codereview.chromium.org/2860203002/diff/280001/extensions/common/api/... extensions/common/api/media_perception_private.idl:153: // |callback| : The State of the system after setting it. Can be used to The callback isn't the state of the system; it's invoked with the state of the system. That said, I think we'd probably just set this to void and indicate failure via an error, rather than a different return result. Additionally, the callback can probably be optional.
Thanks Devlin! Addressed comments. PTAL https://codereview.chromium.org/2860203002/diff/280001/extensions/browser/api... File extensions/browser/api/media_perception_private/BUILD.gn (right): https://codereview.chromium.org/2860203002/diff/280001/extensions/browser/api... extensions/browser/api/media_perception_private/BUILD.gn:1: # Copyright 2017 The Chromium Authors. All rights reserved. On 2017/05/17 20:22:41, Devlin (catching up) wrote: > Is there a compelling reason to have this as a separate build file? Having > separate files for each api has led to cyclical problems, since often an API > will depend on extensions/browser/ and extensions/browser/ will depend on this > API. The reason I am using a separate build file is because this API is for chromeos only and all of the other APIs under is_chromeos in extensions/browser/api/BUILD.gn use their own build file. Not sure exactly why that is, but what do you suggest? https://codereview.chromium.org/2860203002/diff/280001/extensions/browser/api... File extensions/browser/api/media_perception_private/media_perception_private_api.h (right): https://codereview.chromium.org/2860203002/diff/280001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_private_api.h:11: namespace media_perception = extensions::api::media_perception_private; On 2017/05/17 20:22:41, Devlin (catching up) wrote: > namespace aliases are disallowed in .h files. Done. https://codereview.chromium.org/2860203002/diff/280001/extensions/common/api/... File extensions/common/api/media_perception_private.idl (right): https://codereview.chromium.org/2860203002/diff/280001/extensions/common/api/... extensions/common/api/media_perception_private.idl:12: // Analytics process running and media processing pipeline started, On 2017/05/17 20:22:42, Devlin (catching up) wrote: > nit: use complete sentences in comments, so something like: "*The* analytics > process *is* running and..." etc (throughout this file). Done. https://codereview.chromium.org/2860203002/diff/280001/extensions/common/api/... extensions/common/api/media_perception_private.idl:43: double? x; On 2017/05/17 20:22:42, Devlin (catching up) wrote: > When would these be null? Previously discussed the reason for these null values this with tbarzic, here's a copy and paste of a blanket explanation for the reasoning behind using optional values throughout: 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/2860203002/diff/280001/extensions/common/api/... extensions/common/api/media_perception_private.idl:67: long? id; On 2017/05/17 20:22:41, Devlin (catching up) wrote: > Why would these be null? > > I'm not familiar enough with the implementation to know if the others are > appropriately optional or not. See above explanation. https://codereview.chromium.org/2860203002/diff/280001/extensions/common/api/... extensions/common/api/media_perception_private.idl:93: Entity[]? entities; On 2017/05/17 20:22:41, Devlin (catching up) wrote: > Usually prefer an empty list rather than an optional one. Above explanation. Modeled after proto behavior. https://codereview.chromium.org/2860203002/diff/280001/extensions/common/api/... extensions/common/api/media_perception_private.idl:109: RGB, On 2017/05/17 20:22:41, Devlin (catching up) wrote: > Is RGB an image format? Based on media_perception.proto, but this could be changed to RAW? https://codereview.chromium.org/2860203002/diff/280001/extensions/common/api/... extensions/common/api/media_perception_private.idl:146: // Get the status of the media perception process. On 2017/05/17 20:22:42, Devlin (catching up) wrote: > Function comments should be descriptive, not imperative (i.e., use "Gets"). > Same for below. Done. https://codereview.chromium.org/2860203002/diff/280001/extensions/common/api/... extensions/common/api/media_perception_private.idl:153: // |callback| : The State of the system after setting it. Can be used to On 2017/05/17 20:22:42, Devlin (catching up) wrote: > The callback isn't the state of the system; it's invoked with the state of the > system. > > That said, I think we'd probably just set this to void and indicate failure via > an error, rather than a different return result. > > Additionally, the callback can probably be optional. We do indicate failure via an error, however there are cases where the callback state isn't always the same as the requested state, but it's not necessarily an error. We prefer to have the flexibility to return a different state than requested from the media analytics process back to the frontend.
lasoren@chromium.org changed reviewers: + isherman@chromium.org
Adding isherman@ for histograms files OWNER review.
https://codereview.chromium.org/2860203002/diff/280001/extensions/browser/api... File extensions/browser/api/media_perception_private/BUILD.gn (right): https://codereview.chromium.org/2860203002/diff/280001/extensions/browser/api... extensions/browser/api/media_perception_private/BUILD.gn:1: # Copyright 2017 The Chromium Authors. All rights reserved. On 2017/05/17 21:01:35, Luke Sorenson wrote: > On 2017/05/17 20:22:41, Devlin (catching up) wrote: > > Is there a compelling reason to have this as a separate build file? Having > > separate files for each api has led to cyclical problems, since often an API > > will depend on extensions/browser/ and extensions/browser/ will depend on this > > API. > > The reason I am using a separate build file is because this API is for chromeos > only and all of the other APIs under is_chromeos in > extensions/browser/api/BUILD.gn use their own build file. Not sure exactly why > that is, but what do you suggest? I'd suggest having these files directly included in a chromeos-specific block in extensions/browser/api/BUILD.gn. https://codereview.chromium.org/2860203002/diff/280001/extensions/common/api/... File extensions/common/api/media_perception_private.idl (right): https://codereview.chromium.org/2860203002/diff/280001/extensions/common/api/... extensions/common/api/media_perception_private.idl:43: double? x; On 2017/05/17 21:01:36, Luke Sorenson wrote: > On 2017/05/17 20:22:42, Devlin (catching up) wrote: > > When would these be null? > > Previously discussed the reason for these null values this with tbarzic, here's > a copy and paste of a blanket explanation for the reasoning behind using > optional values throughout: > > 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. That's a bit weird, but since this is a private API, I won't press the issue. https://codereview.chromium.org/2860203002/diff/280001/extensions/common/api/... extensions/common/api/media_perception_private.idl:109: RGB, On 2017/05/17 21:01:36, Luke Sorenson wrote: > On 2017/05/17 20:22:41, Devlin (catching up) wrote: > > Is RGB an image format? > > Based on media_perception.proto, but this could be changed to RAW? RAW makes a bit more sense to me. Comments would be helpful. :) https://codereview.chromium.org/2860203002/diff/280001/extensions/common/api/... extensions/common/api/media_perception_private.idl:153: // |callback| : The State of the system after setting it. Can be used to On 2017/05/17 21:01:36, Luke Sorenson wrote: > On 2017/05/17 20:22:42, Devlin (catching up) wrote: > > The callback isn't the state of the system; it's invoked with the state of the > > system. > > > > That said, I think we'd probably just set this to void and indicate failure > via > > an error, rather than a different return result. > > > > Additionally, the callback can probably be optional. > > We do indicate failure via an error, however there are cases where the callback > state isn't always the same as the requested state, but it's not necessarily an > error. We prefer to have the flexibility to return a different state than > requested from the media analytics process back to the frontend. When is it the case that calling setState() not setting the state is not an error?
enum changes LGTM, thanks
Thanks Devlin, addressed comments. PTAL Thanks Ilya for the LGTM! https://codereview.chromium.org/2860203002/diff/280001/extensions/browser/api... File extensions/browser/api/media_perception_private/BUILD.gn (right): https://codereview.chromium.org/2860203002/diff/280001/extensions/browser/api... extensions/browser/api/media_perception_private/BUILD.gn:1: # Copyright 2017 The Chromium Authors. All rights reserved. On 2017/05/18 21:21:24, Devlin (catching up) wrote: > On 2017/05/17 21:01:35, Luke Sorenson wrote: > > On 2017/05/17 20:22:41, Devlin (catching up) wrote: > > > Is there a compelling reason to have this as a separate build file? Having > > > separate files for each api has led to cyclical problems, since often an API > > > will depend on extensions/browser/ and extensions/browser/ will depend on > this > > > API. > > > > The reason I am using a separate build file is because this API is for > chromeos > > only and all of the other APIs under is_chromeos in > > extensions/browser/api/BUILD.gn use their own build file. Not sure exactly why > > that is, but what do you suggest? > > I'd suggest having these files directly included in a chromeos-specific block in > extensions/browser/api/BUILD.gn. Done. https://codereview.chromium.org/2860203002/diff/280001/extensions/common/api/... File extensions/common/api/media_perception_private.idl (right): https://codereview.chromium.org/2860203002/diff/280001/extensions/common/api/... extensions/common/api/media_perception_private.idl:43: double? x; On 2017/05/18 21:21:24, Devlin (catching up) wrote: > On 2017/05/17 21:01:36, Luke Sorenson wrote: > > On 2017/05/17 20:22:42, Devlin (catching up) wrote: > > > When would these be null? > > > > Previously discussed the reason for these null values this with tbarzic, > here's > > a copy and paste of a blanket explanation for the reasoning behind using > > optional values throughout: > > > > 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. > > That's a bit weird, but since this is a private API, I won't press the issue. Acknowledged. https://codereview.chromium.org/2860203002/diff/280001/extensions/common/api/... extensions/common/api/media_perception_private.idl:109: RGB, On 2017/05/18 21:21:24, Devlin (catching up) wrote: > On 2017/05/17 21:01:36, Luke Sorenson wrote: > > On 2017/05/17 20:22:41, Devlin (catching up) wrote: > > > Is RGB an image format? > > > > Based on media_perception.proto, but this could be changed to RAW? > > RAW makes a bit more sense to me. Comments would be helpful. :) Done. https://codereview.chromium.org/2860203002/diff/280001/extensions/common/api/... extensions/common/api/media_perception_private.idl:153: // |callback| : The State of the system after setting it. Can be used to On 2017/05/18 21:21:24, Devlin (catching up) wrote: > On 2017/05/17 21:01:36, Luke Sorenson wrote: > > On 2017/05/17 20:22:42, Devlin (catching up) wrote: > > > The callback isn't the state of the system; it's invoked with the state of > the > > > system. > > > > > > That said, I think we'd probably just set this to void and indicate failure > > via > > > an error, rather than a different return result. > > > > > > Additionally, the callback can probably be optional. > > > > We do indicate failure via an error, however there are cases where the > callback > > state isn't always the same as the requested state, but it's not necessarily > an > > error. We prefer to have the flexibility to return a different state than > > requested from the media analytics process back to the frontend. > > When is it the case that calling setState() not setting the state is not an > error? For simplicity, the app can only setState RUNNING and SUSPENDED, because we don't want the app to have to handle process management of the media analytics process. If the process is not launched yet and the app calls setState RUNNING, the reply will be { status: STARTED } which indicates that the process is launched but the media analytics pipeline hasn't started running. In the future, as our media analytics process potentially gets more complicated / adds features, there may be other cases where the return status may be different than the provided status in setState.
https://codereview.chromium.org/2860203002/diff/280001/extensions/common/api/... File extensions/common/api/media_perception_private.idl (right): https://codereview.chromium.org/2860203002/diff/280001/extensions/common/api/... extensions/common/api/media_perception_private.idl:153: // |callback| : The State of the system after setting it. Can be used to On 2017/05/18 21:56:33, Luke Sorenson wrote: > On 2017/05/18 21:21:24, Devlin (catching up) wrote: > > On 2017/05/17 21:01:36, Luke Sorenson wrote: > > > On 2017/05/17 20:22:42, Devlin (catching up) wrote: > > > > The callback isn't the state of the system; it's invoked with the state of > > the > > > > system. > > > > > > > > That said, I think we'd probably just set this to void and indicate > failure > > > via > > > > an error, rather than a different return result. > > > > > > > > Additionally, the callback can probably be optional. > > > > > > We do indicate failure via an error, however there are cases where the > > callback > > > state isn't always the same as the requested state, but it's not necessarily > > an > > > error. We prefer to have the flexibility to return a different state than > > > requested from the media analytics process back to the frontend. > > > > When is it the case that calling setState() not setting the state is not an > > error? > > For simplicity, the app can only setState RUNNING and SUSPENDED, because we > don't want the app to have to handle process management of the media analytics > process. If the process is not launched yet and the app calls setState RUNNING, > the reply will be { status: STARTED } which indicates that the process is > launched but the media analytics pipeline hasn't started running. > > In the future, as our media analytics process potentially gets more complicated > / adds features, there may be other cases where the return status may be > different than the provided status in setState. Is there a reason those cases shouldn't be surfaced through lastError? When would the behavior of the app be different based on that result? It seems like, from the app's perspective, if setState doesn't actually set the state, it's an error. Note that not all errors are fatal - so some errors can be handled, which it sounds like is more what we'd want to do here. It just seems like it's very convoluted to have to check both lastError *and* the returned state to find out if setState didn't set the state...
Thanks Devlin, replied to your comment https://codereview.chromium.org/2860203002/diff/280001/extensions/common/api/... File extensions/common/api/media_perception_private.idl (right): https://codereview.chromium.org/2860203002/diff/280001/extensions/common/api/... extensions/common/api/media_perception_private.idl:153: // |callback| : The State of the system after setting it. Can be used to On 2017/05/19 15:08:12, Devlin (catching up) wrote: > On 2017/05/18 21:56:33, Luke Sorenson wrote: > > On 2017/05/18 21:21:24, Devlin (catching up) wrote: > > > On 2017/05/17 21:01:36, Luke Sorenson wrote: > > > > On 2017/05/17 20:22:42, Devlin (catching up) wrote: > > > > > The callback isn't the state of the system; it's invoked with the state > of > > > the > > > > > system. > > > > > > > > > > That said, I think we'd probably just set this to void and indicate > > failure > > > > via > > > > > an error, rather than a different return result. > > > > > > > > > > Additionally, the callback can probably be optional. > > > > > > > > We do indicate failure via an error, however there are cases where the > > > callback > > > > state isn't always the same as the requested state, but it's not > necessarily > > > an > > > > error. We prefer to have the flexibility to return a different state than > > > > requested from the media analytics process back to the frontend. > > > > > > When is it the case that calling setState() not setting the state is not an > > > error? > > > > For simplicity, the app can only setState RUNNING and SUSPENDED, because we > > don't want the app to have to handle process management of the media analytics > > process. If the process is not launched yet and the app calls setState > RUNNING, > > the reply will be { status: STARTED } which indicates that the process is > > launched but the media analytics pipeline hasn't started running. > > > > In the future, as our media analytics process potentially gets more > complicated > > / adds features, there may be other cases where the return status may be > > different than the provided status in setState. > > Is there a reason those cases shouldn't be surfaced through lastError? When > would the behavior of the app be different based on that result? It seems like, > from the app's perspective, if setState doesn't actually set the state, it's an > error. Note that not all errors are fatal - so some errors can be handled, > which it sounds like is more what we'd want to do here. > > It just seems like it's very convoluted to have to check both lastError *and* > the returned state to find out if setState didn't set the state... So it's never the case currently that you need to check both lastError and the returned state (we only ever actually Respond with one). In general, the return state in setState will either be the requested state or some transitional state, in cases where the final state change to requested state may take more than a few seconds. In the future, we intend to add an onStateChanged event as well to notify the app about those changes, but because it wasn't expressly necessary for the M60 implementation, it's not part of this initial IDL.
lgtm https://codereview.chromium.org/2860203002/diff/280001/extensions/common/api/... File extensions/common/api/media_perception_private.idl (right): https://codereview.chromium.org/2860203002/diff/280001/extensions/common/api/... extensions/common/api/media_perception_private.idl:153: // |callback| : The State of the system after setting it. Can be used to On 2017/05/19 15:51:04, Luke Sorenson wrote: > On 2017/05/19 15:08:12, Devlin (catching up) wrote: > > On 2017/05/18 21:56:33, Luke Sorenson wrote: > > > On 2017/05/18 21:21:24, Devlin (catching up) wrote: > > > > On 2017/05/17 21:01:36, Luke Sorenson wrote: > > > > > On 2017/05/17 20:22:42, Devlin (catching up) wrote: > > > > > > The callback isn't the state of the system; it's invoked with the > state > > of > > > > the > > > > > > system. > > > > > > > > > > > > That said, I think we'd probably just set this to void and indicate > > > failure > > > > > via > > > > > > an error, rather than a different return result. > > > > > > > > > > > > Additionally, the callback can probably be optional. > > > > > > > > > > We do indicate failure via an error, however there are cases where the > > > > callback > > > > > state isn't always the same as the requested state, but it's not > > necessarily > > > > an > > > > > error. We prefer to have the flexibility to return a different state > than > > > > > requested from the media analytics process back to the frontend. > > > > > > > > When is it the case that calling setState() not setting the state is not > an > > > > error? > > > > > > For simplicity, the app can only setState RUNNING and SUSPENDED, because we > > > don't want the app to have to handle process management of the media > analytics > > > process. If the process is not launched yet and the app calls setState > > RUNNING, > > > the reply will be { status: STARTED } which indicates that the process is > > > launched but the media analytics pipeline hasn't started running. > > > > > > In the future, as our media analytics process potentially gets more > > complicated > > > / adds features, there may be other cases where the return status may be > > > different than the provided status in setState. > > > > Is there a reason those cases shouldn't be surfaced through lastError? When > > would the behavior of the app be different based on that result? It seems > like, > > from the app's perspective, if setState doesn't actually set the state, it's > an > > error. Note that not all errors are fatal - so some errors can be handled, > > which it sounds like is more what we'd want to do here. > > > > It just seems like it's very convoluted to have to check both lastError *and* > > the returned state to find out if setState didn't set the state... > > So it's never the case currently that you need to check both lastError and the > returned state (we only ever actually Respond with one). In general, the return > state in setState will either be the requested state or some transitional state, > in cases where the final state change to requested state may take more than a > few seconds. In the future, we intend to add an onStateChanged event as well to > notify the app about those changes, but because it wasn't expressly necessary > for the M60 implementation, it's not part of this initial IDL. Just because we only respond with one doesn't mean you only need to check one. :) If an app calls this: chrome.mediaPerceptionPrivate.setState(desiredState, (newState) => { // Here, if I only check last error, I might miss that newState != desiredState. If I only check // newState, I might miss the last error. I need to check both. }); But, again, I suppose it's not worth delving too deep into this. https://codereview.chromium.org/2860203002/diff/360001/extensions/browser/api... File extensions/browser/api/media_perception_private/media_perception_private_api.cc (right): https://codereview.chromium.org/2860203002/diff/360001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_private_api.cc:7: #include "extensions/browser/extension_function.h" nit: This is included in the header; it's not needed here. https://codereview.chromium.org/2860203002/diff/360001/extensions/browser/api... File extensions/browser/api/media_perception_private/media_perception_private_api.h (right): https://codereview.chromium.org/2860203002/diff/360001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_private_api.h:9: #include "extensions/common/api/media_perception_private.h" nit: needed?
Thanks for the lgtm Devlin! Addressed final comments. https://codereview.chromium.org/2860203002/diff/360001/extensions/browser/api... File extensions/browser/api/media_perception_private/media_perception_private_api.cc (right): https://codereview.chromium.org/2860203002/diff/360001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_private_api.cc:7: #include "extensions/browser/extension_function.h" On 2017/05/19 15:58:17, Devlin (catching up) wrote: > nit: This is included in the header; it's not needed here. Done. https://codereview.chromium.org/2860203002/diff/360001/extensions/browser/api... File extensions/browser/api/media_perception_private/media_perception_private_api.h (right): https://codereview.chromium.org/2860203002/diff/360001/extensions/browser/api... extensions/browser/api/media_perception_private/media_perception_private_api.h:9: #include "extensions/common/api/media_perception_private.h" On 2017/05/19 15:58:17, Devlin (catching up) wrote: > nit: needed? Done.
The CQ bit was checked by lasoren@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tbarzic@chromium.org, isherman@chromium.org, rdevlin.cronin@chromium.org Link to the patchset: https://codereview.chromium.org/2860203002/#ps380001 (title: "Addressed final comments and rebased with 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 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_...)
The CQ bit was checked by lasoren@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rdevlin.cronin@chromium.org, isherman@chromium.org, tbarzic@chromium.org Link to the patchset: https://codereview.chromium.org/2860203002/#ps400001 (title: "Added permission set unittest fix.")
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": 400001, "attempt_start_ts": 1495224442354790,
"parent_rev": "6fde4f2a798de7216e6e20057a227f431df761fd", "commit_rev":
"aab09b896d22f03ccc701cfb8c6186df508405d5"}
Message was sent while issue was closed.
Description was changed from ========== MediaPerceptionPrivate IDL and skeleton. This private API is for process management of and communicating with a media analytics process running outside of Chrome. The analytics process uses computer vision technology to send metadata about image frames over D-Bus, and MediaPerception events are fired on the frontend. BUG=709180, 720495 ========== to ========== MediaPerceptionPrivate IDL and skeleton. This private API is for process management of and communicating with a media analytics process running outside of Chrome. The analytics process uses computer vision technology to send metadata about image frames over D-Bus, and MediaPerception events are fired on the frontend. BUG=709180, 720495 Review-Url: https://codereview.chromium.org/2860203002 Cr-Commit-Position: refs/heads/master@{#473337} Committed: https://chromium.googlesource.com/chromium/src/+/aab09b896d22f03ccc701cfb8c61... ==========
Message was sent while issue was closed.
Committed patchset #21 (id:400001) as https://chromium.googlesource.com/chromium/src/+/aab09b896d22f03ccc701cfb8c61... |
