Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(276)

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.

Description

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/+/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. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+676 lines, -0 lines) Patch
M chromeos/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 4 chunks +13 lines, -0 lines 0 comments Download
M chromeos/dbus/dbus_clients_browser.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 2 chunks +2 lines, -0 lines 0 comments Download
M chromeos/dbus/dbus_clients_browser.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 3 chunks +8 lines, -0 lines 0 comments Download
M chromeos/dbus/dbus_thread_manager.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 3 chunks +3 lines, -0 lines 0 comments Download
M chromeos/dbus/dbus_thread_manager.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 3 chunks +12 lines, -0 lines 0 comments Download
A chromeos/dbus/fake_media_analytics_client.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 1 chunk +77 lines, -0 lines 0 comments Download
A chromeos/dbus/fake_media_analytics_client.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 1 chunk +100 lines, -0 lines 0 comments Download
M chromeos/dbus/fake_upstart_client.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 1 chunk +2 lines, -0 lines 0 comments Download
M chromeos/dbus/fake_upstart_client.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 2 chunks +20 lines, -0 lines 0 comments Download
A chromeos/dbus/media_analytics_client.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 1 chunk +68 lines, -0 lines 0 comments Download
A chromeos/dbus/media_analytics_client.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 1 chunk +198 lines, -0 lines 0 comments Download
A chromeos/dbus/proto/media_perception.proto View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 1 chunk +122 lines, -0 lines 0 comments Download
M chromeos/dbus/upstart_client.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 3 chunks +9 lines, -0 lines 0 comments Download
M chromeos/dbus/upstart_client.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 3 chunks +42 lines, -0 lines 0 comments Download

Dependent Patchsets:

Messages

Total messages: 60 (23 generated)
mattfrazier
I believe you included webcamprivate_idl by mistake (just a whitespace change) (Also I'm just figuring ...
3 years, 8 months ago (2017-04-05 11:55:50 UTC) #3
Luke Sorenson
On 2017/04/05 11:55:50, mattfrazier wrote: > I believe you included webcamprivate_idl by mistake (just a ...
3 years, 8 months ago (2017-04-05 14:17:57 UTC) #4
bmayer
Just a few nits otherwise LGTM. Will let Simon hit the LGTM after he reviews ...
3 years, 8 months ago (2017-04-13 17:42:00 UTC) #8
Luke Sorenson
Thanks for the review bmayer@, addressed your comments https://codereview.chromium.org/2791983004/diff/250001/extensions/browser/api/media_perception_private/media_perception_api_manager.cc File extensions/browser/api/media_perception_private/media_perception_api_manager.cc (right): https://codereview.chromium.org/2791983004/diff/250001/extensions/browser/api/media_perception_private/media_perception_api_manager.cc#newcode15 extensions/browser/api/media_perception_private/media_perception_api_manager.cc:15: // ...
3 years, 8 months ago (2017-04-13 17:58:21 UTC) #9
Simon Que
Have you seen this? go/new-chrome-api There's some reviews to go through when adding an API.
3 years, 8 months ago (2017-04-13 19:16:19 UTC) #10
tbarzic
I think it would be nice if you could separate dbus client code into a ...
3 years, 7 months ago (2017-04-27 20:37:36 UTC) #15
fraz
https://codereview.chromium.org/2791983004/diff/630001/extensions/common/api/media_perception_private.idl File extensions/common/api/media_perception_private.idl (right): https://codereview.chromium.org/2791983004/diff/630001/extensions/common/api/media_perception_private.idl#newcode127 extensions/common/api/media_perception_private.idl:127: static void setState(State state, StateCallback callback); On 2017/04/27 20:37:36, ...
3 years, 7 months ago (2017-05-02 21:57:29 UTC) #17
rkc1
I'd like to echo Toni's thoughts here, this CL is way too big and needs ...
3 years, 7 months ago (2017-05-02 22:07:25 UTC) #19
Luke Sorenson
On 2017/05/02 22:07:25, rkc_google wrote: > I'd like to echo Toni's thoughts here, this CL ...
3 years, 7 months ago (2017-05-03 08:48:20 UTC) #20
Luke Sorenson
Thanks for the review! At least one open question still. https://codereview.chromium.org/2791983004/diff/630001/chrome/test/data/extensions/api_test/media_perception_private/media_perception/runtest.js 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/extensions/api_test/media_perception_private/media_perception/runtest.js#newcode7 ...
3 years, 7 months ago (2017-05-03 23:56:07 UTC) #21
Luke Sorenson
Separated out the other files, this is now just the D-Bus client code. PTAL
3 years, 7 months ago (2017-05-04 19:56:29 UTC) #23
tbarzic
sorry for all the comments from the non dbus parts (that were made before you ...
3 years, 7 months ago (2017-05-05 21:10:42 UTC) #25
Luke Sorenson
Thanks for the comments tbarzic! Addressed them often in the other broken out CLs, which ...
3 years, 7 months ago (2017-05-08 19:06:08 UTC) #26
Luke Sorenson
3 years, 7 months ago (2017-05-08 21:47:02 UTC) #28
stevenjb
owner lgtm
3 years, 7 months ago (2017-05-08 23:01:41 UTC) #29
tbarzic
https://codereview.chromium.org/2791983004/diff/630001/extensions/browser/api/media_perception_private/media_perception_api_manager.cc File extensions/browser/api/media_perception_private/media_perception_api_manager.cc (right): https://codereview.chromium.org/2791983004/diff/630001/extensions/browser/api/media_perception_private/media_perception_api_manager.cc#newcode37 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: > ...
3 years, 7 months ago (2017-05-08 23:04:56 UTC) #30
rkc1
https://codereview.chromium.org/2791983004/diff/630001/extensions/browser/api/media_perception_private/media_perception_api_manager.cc File extensions/browser/api/media_perception_private/media_perception_api_manager.cc (right): https://codereview.chromium.org/2791983004/diff/630001/extensions/browser/api/media_perception_private/media_perception_api_manager.cc#newcode37 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: > ...
3 years, 7 months ago (2017-05-08 23:34:09 UTC) #31
Luke Sorenson
Thanks for the reviews. PTAL https://codereview.chromium.org/2791983004/diff/630001/extensions/browser/api/media_perception_private/media_perception_api_manager.cc File extensions/browser/api/media_perception_private/media_perception_api_manager.cc (right): https://codereview.chromium.org/2791983004/diff/630001/extensions/browser/api/media_perception_private/media_perception_api_manager.cc#newcode37 extensions/browser/api/media_perception_private/media_perception_api_manager.cc:37: bbox_result->top_left.reset(new mpp::Point()); On 2017/05/08 ...
3 years, 7 months ago (2017-05-09 17:39:45 UTC) #32
tbarzic
https://codereview.chromium.org/2791983004/diff/790001/chromeos/dbus/fake_media_analytics_client.cc File chromeos/dbus/fake_media_analytics_client.cc (right): https://codereview.chromium.org/2791983004/diff/790001/chromeos/dbus/fake_media_analytics_client.cc#newcode18 chromeos/dbus/fake_media_analytics_client.cc:18: // Override these initializations to do nothing. nit: no ...
3 years, 7 months ago (2017-05-09 20:28:41 UTC) #33
rkc1
https://codereview.chromium.org/2791983004/diff/790001/chromeos/dbus/proto/media_perception.proto File chromeos/dbus/proto/media_perception.proto (right): https://codereview.chromium.org/2791983004/diff/790001/chromeos/dbus/proto/media_perception.proto#newcode11 chromeos/dbus/proto/media_perception.proto:11: // here (the other internal to Google) - that ...
3 years, 7 months ago (2017-05-09 20:38:49 UTC) #34
Luke Sorenson
Fake client has custom method for firing media perception event. Addressing more comments. https://codereview.chromium.org/2791983004/diff/790001/chromeos/dbus/fake_media_analytics_client.cc File ...
3 years, 7 months ago (2017-05-09 22:21:25 UTC) #35
tbarzic
https://codereview.chromium.org/2791983004/diff/790001/chromeos/dbus/fake_media_analytics_client.cc File chromeos/dbus/fake_media_analytics_client.cc (right): https://codereview.chromium.org/2791983004/diff/790001/chromeos/dbus/fake_media_analytics_client.cc#newcode38 chromeos/dbus/fake_media_analytics_client.cc:38: if (state.has_status()) { On 2017/05/09 22:21:25, Luke Sorenson wrote: ...
3 years, 7 months ago (2017-05-10 03:26:24 UTC) #36
tbarzic
https://codereview.chromium.org/2791983004/diff/790001/chromeos/dbus/fake_media_analytics_client.cc File chromeos/dbus/fake_media_analytics_client.cc (right): https://codereview.chromium.org/2791983004/diff/790001/chromeos/dbus/fake_media_analytics_client.cc#newcode38 chromeos/dbus/fake_media_analytics_client.cc:38: if (state.has_status()) { On 2017/05/09 22:21:25, Luke Sorenson wrote: ...
3 years, 7 months ago (2017-05-10 03:26:25 UTC) #37
Luke Sorenson
Thanks, addressed comments. PTAL (uploading new patch set) https://codereview.chromium.org/2791983004/diff/790001/chromeos/dbus/fake_media_analytics_client.cc File chromeos/dbus/fake_media_analytics_client.cc (right): https://codereview.chromium.org/2791983004/diff/790001/chromeos/dbus/fake_media_analytics_client.cc#newcode38 chromeos/dbus/fake_media_analytics_client.cc:38: if ...
3 years, 7 months ago (2017-05-10 16:38:22 UTC) #38
tbarzic
https://codereview.chromium.org/2791983004/diff/810001/chromeos/dbus/fake_media_analytics_client.h File chromeos/dbus/fake_media_analytics_client.h (right): https://codereview.chromium.org/2791983004/diff/810001/chromeos/dbus/fake_media_analytics_client.h#newcode26 chromeos/dbus/fake_media_analytics_client.h:26: void FireMediaPerceptionEvent(); On 2017/05/10 16:38:22, Luke Sorenson wrote: > ...
3 years, 7 months ago (2017-05-10 23:59:30 UTC) #39
Luke Sorenson
Thanks! FakeMediaAnalyticsClient now accepts mri::MediaPerception and mri::Diagnostics. PTAL https://codereview.chromium.org/2791983004/diff/810001/chromeos/dbus/fake_media_analytics_client.h File chromeos/dbus/fake_media_analytics_client.h (right): https://codereview.chromium.org/2791983004/diff/810001/chromeos/dbus/fake_media_analytics_client.h#newcode26 chromeos/dbus/fake_media_analytics_client.h:26: void ...
3 years, 7 months ago (2017-05-11 15:50:29 UTC) #40
tbarzic
Mostly nits left :) https://codereview.chromium.org/2791983004/diff/870001/chromeos/dbus/fake_media_analytics_client.cc File chromeos/dbus/fake_media_analytics_client.cc (right): https://codereview.chromium.org/2791983004/diff/870001/chromeos/dbus/fake_media_analytics_client.cc#newcode23 chromeos/dbus/fake_media_analytics_client.cc:23: LOG(ERROR) << "Fake media analytics ...
3 years, 7 months ago (2017-05-11 16:55:56 UTC) #41
Luke Sorenson
Addressed nits. PTAL https://codereview.chromium.org/2791983004/diff/870001/chromeos/dbus/fake_media_analytics_client.cc File chromeos/dbus/fake_media_analytics_client.cc (right): https://codereview.chromium.org/2791983004/diff/870001/chromeos/dbus/fake_media_analytics_client.cc#newcode23 chromeos/dbus/fake_media_analytics_client.cc:23: LOG(ERROR) << "Fake media analytics process ...
3 years, 7 months ago (2017-05-11 18:16:48 UTC) #42
tbarzic
lgtm https://codereview.chromium.org/2791983004/diff/890001/chromeos/dbus/fake_media_analytics_client.cc File chromeos/dbus/fake_media_analytics_client.cc (right): https://codereview.chromium.org/2791983004/diff/890001/chromeos/dbus/fake_media_analytics_client.cc#newcode22 chromeos/dbus/fake_media_analytics_client.cc:22: if (!process_running_) { nit: no {} https://codereview.chromium.org/2791983004/diff/890001/chromeos/dbus/fake_media_analytics_client.h File ...
3 years, 7 months ago (2017-05-11 18:49:57 UTC) #43
Luke Sorenson
Addressed final comments. Thanks for the review! https://codereview.chromium.org/2791983004/diff/890001/chromeos/dbus/fake_media_analytics_client.cc File chromeos/dbus/fake_media_analytics_client.cc (right): https://codereview.chromium.org/2791983004/diff/890001/chromeos/dbus/fake_media_analytics_client.cc#newcode22 chromeos/dbus/fake_media_analytics_client.cc:22: if (!process_running_) ...
3 years, 7 months ago (2017-05-11 22:01:56 UTC) #44
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2791983004/930001
3 years, 7 months ago (2017-05-11 22:27:35 UTC) #47
tbarzic
On 2017/05/11 22:27:35, commit-bot: I haz the power wrote: > CQ is trying da patch. ...
3 years, 7 months ago (2017-05-11 22:29:49 UTC) #49
Luke Sorenson
Good catch, thanks. I'll wait until the service constants go in.
3 years, 7 months ago (2017-05-11 22:35:22 UTC) #50
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2791983004/950001
3 years, 7 months ago (2017-05-15 14:15:29 UTC) #53
commit-bot: I haz the power
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_chromeos_ozone_rel_ng/builds/384420) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, ...
3 years, 7 months ago (2017-05-15 14:30:00 UTC) #55
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2791983004/950001
3 years, 7 months ago (2017-05-15 18:46:26 UTC) #57
commit-bot: I haz the power
3 years, 7 months ago (2017-05-15 19:37:58 UTC) #60
Message was sent while issue was closed.
Committed patchset #48 (id:950001) as
https://chromium.googlesource.com/chromium/src/+/f933419fa6f3b7a34e6f035595b8...

Powered by Google App Engine
This is Rietveld 408576698