Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2312873002:
Change Chrome OS status reporting interface to ARC++ (Closed)
|
Created:
4 years, 3 months ago by phweiss Modified:
4 years, 3 months ago CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, yusukes+watch_chromium.org, viettrungluu+watch_chromium.org, hidehiko+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, lhchavez+watch_chromium.org, oshima+watch_chromium.org, darin (slow to review), davemoore+watch_chromium.org, qsr+mojo_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionChange Chrome OS status reporting interface to ARC++
Add two functions to enterprise_reporting.mojom without implementation
to unblock Android-side development. Chrome side implementation is blocked
on crbug.com/639372.
BUG=b/31084348
Committed: https://crrev.com/8f7ef8183f4aa81f9deec07f4d0c7398d715070c
Cr-Commit-Position: refs/heads/master@{#417537}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Make call synchronous, one function removed #
Total comments: 2
Patch Set 3 : CamelCase -> snake_case #Messages
Total messages: 27 (16 generated)
The CQ bit was checked by phweiss@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
phweiss@chromium.org changed reviewers: + dcheng@chromium.org, lhchavez@chromium.org
Hi, ptal
https://codereview.chromium.org/2312873002/diff/1/components/arc/common/enter... File components/arc/common/enterprise_reporting.mojom (right): https://codereview.chromium.org/2312873002/diff/1/components/arc/common/enter... components/arc/common/enterprise_reporting.mojom:28: [MinVersion=1] OnStatusRetrieved@1(string status, string droidGuardInfo); Is this supposed to be the asynchronous response to GetStatus() below? Can this be called unrequested? Otherwise, it's probably better to change the signature of GetStatus() to better reflect the caller/response pattern: [MinVersion=1] GetStatus@1() => (string status, string droidGuardInfo); (and remove this function).
Is the implementation in a followup CL? Is it possible to merge it with this CL? it's hard to do a security review with a mojo stub. https://codereview.chromium.org/2312873002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_enterprise_reporting_service.cc (right): https://codereview.chromium.org/2312873002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_enterprise_reporting_service.cc:49: const mojo::String& droidGuardInfo) { Nit: C++ naming convention is droid_guard_info
The CQ bit was checked by phweiss@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Luis, thanks for pointing that out, it indeed did not need to be asynchronous. I replaced the functions as you suggested. Daniel, since one function is gone now, no implementation is needed on Chrome OS side, and the Android side needs this CL landed before they can do more. I could add you for a security review when I write the CL that actually uses GetStatus()? You could also base your security review on the design doc linked in the bug?
hidehiko@chromium.org changed reviewers: + hidehiko@chromium.org
FYI. https://codereview.chromium.org/2312873002/diff/20001/components/arc/common/e... File components/arc/common/enterprise_reporting.mojom (right): https://codereview.chromium.org/2312873002/diff/20001/components/arc/common/e... components/arc/common/enterprise_reporting.mojom:35: [MinVersion=1] GetStatus@1() => (string status, string droidGuardInfo); Drive-by. snake_case please, for param names.
The CQ bit was checked by phweiss@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2312873002/diff/20001/components/arc/common/e... File components/arc/common/enterprise_reporting.mojom (right): https://codereview.chromium.org/2312873002/diff/20001/components/arc/common/e... components/arc/common/enterprise_reporting.mojom:35: [MinVersion=1] GetStatus@1() => (string status, string droidGuardInfo); On 2016/09/08 16:31:22, hidehiko wrote: > Drive-by. snake_case please, for param names. Thanks for the catch. Had it already fixed once, but then copy-pasted the old version while restructuring.
lgtm (but I don't have OWNERS rights over that file)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
mojom lgtm, since the interface change now only requires an implementation on the ARC++ side.
The CQ bit was checked by phweiss@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Change Chrome OS status reporting interface to ARC++ Add two functions to enterprise_reporting.mojom without implementation to unblock Android-side development. Chrome side implementation is blocked on crbug.com/639372. BUG=b/31084348 ========== to ========== Change Chrome OS status reporting interface to ARC++ Add two functions to enterprise_reporting.mojom without implementation to unblock Android-side development. Chrome side implementation is blocked on crbug.com/639372. BUG=b/31084348 Committed: https://crrev.com/8f7ef8183f4aa81f9deec07f4d0c7398d715070c Cr-Commit-Position: refs/heads/master@{#417537} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/8f7ef8183f4aa81f9deec07f4d0c7398d715070c Cr-Commit-Position: refs/heads/master@{#417537} |
