Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2383763002:
Report ARC status asynchronously via DeviceStatusCollector (Closed)
DescriptionReport ARC status asynchronously via DeviceStatusCollector
Add field to SessionStatusReportRequest proto and add code to query
ARC for status blob and DroidGuardInfo. Code is still hidden behind a flag,
and will be controlled by a user policy that will be created in a follow-up
CL. Browser tests will also come with the policy CL.
BUG=b/31084348
Committed: https://crrev.com/629db8c2706566b27b4dccb4142157e6041d046b
Cr-Commit-Position: refs/heads/master@{#423561}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Restructure AndroidStatusFetcher #
Total comments: 8
Patch Set 3 : const, include, auto, formatting #
Total comments: 4
Patch Set 4 : Renaming Kiosk-related functions #Patch Set 5 : Overlong line shortened #
Total comments: 12
Patch Set 6 : Nits addressed #
Messages
Total messages: 34 (20 generated)
phweiss@chromium.org changed reviewers: + ljusten@chromium.org, tnagel@chromium.org
Hi, ptal. Functionality is disabled until I create the controlling policy in a follow-up CL.
Lutz, could you maybe take a look first since you've worked on this very recently?
https://codereview.chromium.org/2383763002/diff/1/chrome/browser/chromeos/pol... File chrome/browser/chromeos/policy/device_status_collector.cc (right): https://codereview.chromium.org/2383763002/diff/1/chrome/browser/chromeos/pol... chrome/browser/chromeos/policy/device_status_collector.cc:346: if (android_status_fetcher.is_null()) { A more elegant approach would be to make android_status_fetcher asynchronous, i.e. have it take a response callback, so that it matches instance->GetStatus, and make the default implementation of android_status_fetcher (if no fetcher is passed into the constructor of DeviceStatusCollector) to be the arc code below. The PostTaskAndReplyWithResult and the OnAndroidInfoPairReceived could be moved to the unit test this way. It would make this class very clean and move details like the pair<string, string> method to the test. https://codereview.chromium.org/2383763002/diff/1/chrome/browser/chromeos/pol... chrome/browser/chromeos/policy/device_status_collector.cc:395: void OnAndroidInfoPairReceived(std::pair<mojo::String, mojo::String> status) { Nit: Is it possible to change the signature of mojo::String to const mojo::String& to prevent copies?
ptal https://codereview.chromium.org/2383763002/diff/1/chrome/browser/chromeos/pol... File chrome/browser/chromeos/policy/device_status_collector.cc (right): https://codereview.chromium.org/2383763002/diff/1/chrome/browser/chromeos/pol... chrome/browser/chromeos/policy/device_status_collector.cc:346: if (android_status_fetcher.is_null()) { On 2016/10/04 08:56:45, ljusten wrote: > A more elegant approach would be to make android_status_fetcher asynchronous, > i.e. have it take a response callback, so that it matches instance->GetStatus, > and make the default implementation of android_status_fetcher (if no fetcher is > passed into the constructor of DeviceStatusCollector) to be the arc code below. > The PostTaskAndReplyWithResult and the OnAndroidInfoPairReceived could be moved > to the unit test this way. It would make this class very clean and move details > like the pair<string, string> method to the test. Done. https://codereview.chromium.org/2383763002/diff/1/chrome/browser/chromeos/pol... chrome/browser/chromeos/policy/device_status_collector.cc:395: void OnAndroidInfoPairReceived(std::pair<mojo::String, mojo::String> status) { On 2016/10/04 08:56:45, ljusten wrote: > Nit: Is it possible to change the signature of mojo::String to const > mojo::String& to prevent copies? No, mojo can connect C++ and Java, so their interfaces do not have such things.
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...
Looks good to me with one nit (const&) and the question about status orthogonality. I'm not a committer, though, so I can't give you my L-G-T-M. https://codereview.chromium.org/2383763002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/device_status_collector.cc (right): https://codereview.chromium.org/2383763002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/device_status_collector.cc:1130: if (report_session_status_) { I'm wondering whether account status is orthogonal to android status, i.e. reporting either status should be allowed independently. Before, there was only one session status, so account status was -the- session status, but now we have 2 entries, so maybe we should allow android status without account status? I guess that would be a question for bartfab@. https://codereview.chromium.org/2383763002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/device_status_collector.h (right): https://codereview.chromium.org/2383763002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/device_status_collector.h:74: using AndroidStatusReceiver = Whoa, double callback all the way!! https://codereview.chromium.org/2383763002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/device_status_collector.h:78: using AndroidStatusFetcher = base::Callback<bool(AndroidStatusReceiver)>; Can you use const AndroidStatusReceiver & to prevent copies? https://codereview.chromium.org/2383763002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/device_status_collector_browsertest.cc (right): https://codereview.chromium.org/2383763002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/device_status_collector_browsertest.cc:220: policy::DeviceStatusCollector::AndroidStatusReceiver receiver, Can you use const AndroidStatusReceiver&?
phweiss@chromium.org changed reviewers: + bartfab@chromium.org - tnagel@chromium.org
ptal https://codereview.chromium.org/2383763002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/device_status_collector.cc (right): https://codereview.chromium.org/2383763002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/device_status_collector.cc:1130: if (report_session_status_) { On 2016/10/05 16:10:42, ljusten wrote: > I'm wondering whether account status is orthogonal to android status, i.e. > reporting either status should be allowed independently. Before, there was only > one session status, so account status was -the- session status, but now we have > 2 entries, so maybe we should allow android status without account status? I > guess that would be a question for bartfab@. Good point, I just looked at the policy for report_session_status, and it is only about Kiosk. So I'll separate that. https://codereview.chromium.org/2383763002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/device_status_collector.h (right): https://codereview.chromium.org/2383763002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/device_status_collector.h:74: using AndroidStatusReceiver = On 2016/10/05 16:10:42, ljusten wrote: > Whoa, double callback all the way!! Acknowledged. https://codereview.chromium.org/2383763002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/device_status_collector.h:78: using AndroidStatusFetcher = base::Callback<bool(AndroidStatusReceiver)>; On 2016/10/05 16:10:42, ljusten wrote: > Can you use const AndroidStatusReceiver & to prevent copies? Done. https://codereview.chromium.org/2383763002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/device_status_collector_browsertest.cc (right): https://codereview.chromium.org/2383763002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/device_status_collector_browsertest.cc:220: policy::DeviceStatusCollector::AndroidStatusReceiver receiver, On 2016/10/05 16:10:42, ljusten wrote: > Can you use const AndroidStatusReceiver&? Done.
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.
Some renamings and comment fix. https://codereview.chromium.org/2383763002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/device_status_collector.cc (right): https://codereview.chromium.org/2383763002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/policy/device_status_collector.cc:1123: void DeviceStatusCollector::GetSessionStatus( While you're at it, the code that calls this method has a comment saying 'Gather device status (might queue some async queries)'. This should be 'session status', not 'device status'. https://codereview.chromium.org/2383763002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/policy/device_status_collector.cc:1128: if (report_session_status_) We should rename a few things then to prevent confusion. I'd suggest report_session_status_ -> report_kiosk_session_status_ and GetAccountStatus -> GetKioskSessionStatus
https://codereview.chromium.org/2383763002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/device_status_collector.cc (right): https://codereview.chromium.org/2383763002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/policy/device_status_collector.cc:1123: void DeviceStatusCollector::GetSessionStatus( On 2016/10/05 18:19:49, ljusten wrote: > While you're at it, the code that calls this method has a comment saying 'Gather > device status (might queue some async queries)'. This should be 'session > status', not 'device status'. Done. https://codereview.chromium.org/2383763002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/policy/device_status_collector.cc:1128: if (report_session_status_) On 2016/10/05 18:19:49, ljusten wrote: > We should rename a few things then to prevent confusion. I'd suggest > report_session_status_ -> report_kiosk_session_status_ and GetAccountStatus -> > GetKioskSessionStatus Done.
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: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
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.
https://codereview.chromium.org/2383763002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/device_status_collector.cc (right): https://codereview.chromium.org/2383763002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/device_status_collector.cc:24: #include "base/memory/ref_counted.h" Nit: This should be included by the header now. https://codereview.chromium.org/2383763002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/device_status_collector.cc:224: if (arc_service == nullptr) Nit, here and below: Alternatively, shorter and idiomatic: if (!arc_service) https://codereview.chromium.org/2383763002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/device_status_collector.cc:395: em::AndroidStatus* android_status = Nit: Const pointer. https://codereview.chromium.org/2383763002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/device_status_collector.h (right): https://codereview.chromium.org/2383763002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/device_status_collector.h:97: const AndroidStatusFetcher& android_status_fetcher_); Nit: s/fetcher_/fetcher/ https://codereview.chromium.org/2383763002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/device_status_collector.h:198: scoped_refptr<GetStatusState> state); // Queues async queries! Nit 1: #include "base/memory/ref_counted.h" Nit 2: Pass the scoped_refptr by const reference. https://codereview.chromium.org/2383763002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/device_status_collector_browsertest.cc (right): https://codereview.chromium.org/2383763002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/device_status_collector_browsertest.cc:231: FROM_HERE, Bind(&CallAndroidStatusReceiver, receiver, "", "")); 1: Nit: #include "base/location.h" 2: Nit: s/Bind/base::Bind/ 3: Are you planning to add a test that actually returns some non-empty data?
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/2383763002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/device_status_collector.cc (right): https://codereview.chromium.org/2383763002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/device_status_collector.cc:24: #include "base/memory/ref_counted.h" On 2016/10/06 15:43:26, bartfab (slow) wrote: > Nit: This should be included by the header now. Done. https://codereview.chromium.org/2383763002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/device_status_collector.cc:224: if (arc_service == nullptr) On 2016/10/06 15:43:26, bartfab (slow) wrote: > Nit, here and below: Alternatively, shorter and idiomatic: if (!arc_service) Done. https://codereview.chromium.org/2383763002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/device_status_collector.cc:395: em::AndroidStatus* android_status = On 2016/10/06 15:43:26, bartfab (slow) wrote: > Nit: Const pointer. Done. https://codereview.chromium.org/2383763002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/device_status_collector.h (right): https://codereview.chromium.org/2383763002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/device_status_collector.h:97: const AndroidStatusFetcher& android_status_fetcher_); On 2016/10/06 15:43:26, bartfab (slow) wrote: > Nit: s/fetcher_/fetcher/ Done. https://codereview.chromium.org/2383763002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/device_status_collector.h:198: scoped_refptr<GetStatusState> state); // Queues async queries! On 2016/10/06 15:43:26, bartfab (slow) wrote: > Nit 1: #include "base/memory/ref_counted.h" > Nit 2: Pass the scoped_refptr by const reference. Done. https://codereview.chromium.org/2383763002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/device_status_collector_browsertest.cc (right): https://codereview.chromium.org/2383763002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/device_status_collector_browsertest.cc:231: FROM_HERE, Bind(&CallAndroidStatusReceiver, receiver, "", "")); On 2016/10/06 15:43:26, bartfab (slow) wrote: > 1: Nit: #include "base/location.h" > 2: Nit: s/Bind/base::Bind/ > 3: Are you planning to add a test that actually returns some non-empty data? 1: Done. 2: Done. 3: Yes, another function GetFakeAndroidStatus will be added for that in the follow-up CL.
lgtm
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 #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Report ARC status asynchronously via DeviceStatusCollector Add field to SessionStatusReportRequest proto and add code to query ARC for status blob and DroidGuardInfo. Code is still hidden behind a flag, and will be controlled by a user policy that will be created in a follow-up CL. Browser tests will also come with the policy CL. BUG=b/31084348 ========== to ========== Report ARC status asynchronously via DeviceStatusCollector Add field to SessionStatusReportRequest proto and add code to query ARC for status blob and DroidGuardInfo. Code is still hidden behind a flag, and will be controlled by a user policy that will be created in a follow-up CL. Browser tests will also come with the policy CL. BUG=b/31084348 Committed: https://crrev.com/629db8c2706566b27b4dccb4142157e6041d046b Cr-Commit-Position: refs/heads/master@{#423561} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/629db8c2706566b27b4dccb4142157e6041d046b Cr-Commit-Position: refs/heads/master@{#423561} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
