Display fetch and policy timestamps separately [chromium/src : main]
Sergiy Belozorov (Gerrit)
Sergiy Belozorov removed a vote from this change.
To view, visit change 3150410. To unsubscribe, or for help writing mail filters, visit settings.
Sergiy Belozorov (Gerrit)
Attention is currently required from: Roland Bock, Sergiy Belozorov.
Set Ready For Review
Roland Bock (Gerrit)
Attention is currently required from: Sergiy Belozorov.
7 comments:
Commit Message:
Patch Set #23, Line 9:
fixes the buFrom this description, I can't tell what the bug is/was.
Patchset:
Thanks for the CL!
Some comments while waiting on the previous one :-)
File chrome/browser/ui/webui/policy/policy_ui_browsertest.cc:
Patch Set #23, Line 538:
"(function findPolicyStatuses() {"Just curious: Why not use a raw string literal? Might be easier to read without the many `"`?
Patch Set #23, Line 550:
div:nth-childnth-child seems brittle to me (although admittedly, it might work for years).
How about looking for classes that start with "time-since-"?
Patch Set #23, Line 597:
base::SimpleTestClock* PolicyUIStatusTest::InitializeClockMock(Why not simply create the unique_ptr here? No static_cast, no non-const reference to unique_ptr.
std::unique_ptr<base::SimpleTestClock> PolicyUIStatusTest::CreateClockMock(
const base::Time& start) {
auto mock = std::make_unique<base::SimpleTestClock>();
raw_mock->Advance(start - raw_mock->Now());
return mock;
}Patch Set #23, Line 602:
AdvanceIsn't this the same as
raw_mock->SetNow(start)
?Patch Set #23, Line 625:
User policiesCan you add device policies, too?
To view, visit change 3150410. To unsubscribe, or for help writing mail filters, visit settings.
Sergiy Belozorov (Gerrit)
Attention is currently required from: Roland Bock.
6 comments:
Commit Message:
Patch Set #23, Line 9:
fixes the buFrom this description, I can't tell what the bug is/was.
Done
File chrome/browser/ui/webui/policy/policy_ui_browsertest.cc:
Patch Set #23, Line 538:
"(function findPolicyStatuses() {"Just curious: Why not use a raw string literal? Might be easier to read without the many `"`?
Tried to be consistent with existing code above. Done
Patch Set #23, Line 550:
div:nth-child
nth-child seems brittle to me (although admittedly, it might work for years). […]
Currently, the PolicyUIStatusTest::ReadStatusFor is universal and can read any row in the table in the status box as the value is always in the second column. This makes it easier to add other tests. Adding `time-since-` prefix makes it specific to rows for time. Given the poor coverage for this page, IMHO it's worth making this code a bit brittle, but make it easier to add tests.
Patch Set #23, Line 597:
base::SimpleTestClock* PolicyUIStatusTest::InitializeClockMock(
Why not simply create the unique_ptr here? No static_cast, no non-const reference to unique_ptr. […]
Replaced with a different system after addressing comments in the base CL.
Patch Set #23, Line 602:
Advance
Isn't this the same as […]
Didn't notice that method. Thanks, done
Patch Set #23, Line 625:
User policiesCan you add device policies, too?
Done
To view, visit change 3150410. To unsubscribe, or for help writing mail filters, visit settings.
Sergiy Belozorov (Gerrit)
Attention is currently required from: Roland Bock.
Sergiy Belozorov uploaded patch set #43 to this change.
Display fetch and policy timestamps separately
When the user attempts to fetch policy without an active network
connection, we used to still update the "Last fetched" timestamp. This
CL fixes this by splitting the timestamp into "Last fetch attempted"
and "Last policy timestamp". The former is always updated, the latter
is only updated when we have actually fetched an updated policy.
Bug: b:217860627
Change-Id: I97416021b92964deee8d5ebc430d2ac97f1da0e6
---
M chrome/browser/ash/login/test/embedded_policy_test_server_mixin.cc
M chrome/browser/ash/login/test/embedded_policy_test_server_mixin.h
M chrome/browser/ui/webui/policy/policy_ui.cc
M chrome/browser/ui/webui/policy/policy_ui_browsertest.cc
M chrome/browser/ui/webui/policy/policy_ui_handler.cc
M components/policy/core/browser/webui/machine_level_user_cloud_policy_status_provider.cc
M components/policy/core/browser/webui/policy_status_provider.cc
M components/policy/core/browser/webui/policy_status_provider.h
M components/policy/core/common/policy_scheduler.cc
M components/policy/core/common/policy_scheduler.h
M components/policy/resources/webui/policy.html
M components/policy/resources/webui/policy_base.js
M components/policy_strings.grdp
A components/policy_strings_grdp/IDS_POLICY_LABEL_TIME_SINCE_LAST_FETCH_ATTEMPT.png.sha1
A components/policy_strings_grdp/IDS_POLICY_LABEL_TIME_SINCE_LAST_REFRESH.png.sha1
M ios/chrome/browser/ui/webui/policy/policy_ui.mm
16 files changed, 282 insertions(+), 28 deletions(-)
To view, visit change 3150410. To unsubscribe, or for help writing mail filters, visit settings.
Sergiy Belozorov (Gerrit)
Attention is currently required from: Roland Bock.
Roland Bock (Gerrit)
Attention is currently required from: Sergiy Belozorov.
Patch set 46:Code-Review +1
1 comment:
Patchset:
LGTM, but I wonder if it would make sense to split the CL into two:
1) setup the test machinery with a minimal test of the old behavior
2) change the behaviorI could imagine that being easier to review.
To view, visit change 3150410. To unsubscribe, or for help writing mail filters, visit settings.
Sergiy Belozorov (Gerrit)
1 comment:
Patchset:
LGTM, but I wonder if it would make sense to split the CL into two: […]
Good idea. Split off test changes into https://crrev.com/c/3474382. Added you as a reviewer there too.
To view, visit change 3150410. To unsubscribe, or for help writing mail filters, visit settings.
Sergiy Belozorov (Gerrit)
Attention is currently required from: Gauthier Ambard, Nico Weber.
Sergiy Belozorov would like Gauthier Ambard and Nico Weber to review this change.
Display fetch and policy timestamps separately
When the user attempts to fetch policy without an active network
connection, we used to still update the "Last fetched" timestamp. This
CL fixes this by splitting the timestamp into "Last fetch attempted"
and "Last policy timestamp". The former is always updated, the latter
is only updated when we have actually fetched an updated policy.
Bug: b:217860627
Change-Id: I97416021b92964deee8d5ebc430d2ac97f1da0e6
---
M chrome/browser/ui/webui/policy/policy_ui.cc
M chrome/browser/ui/webui/policy/policy_ui_browsertest.cc
M chrome/browser/ui/webui/policy/policy_ui_handler.cc
M components/policy/core/browser/webui/machine_level_user_cloud_policy_status_provider.cc
M components/policy/core/browser/webui/policy_status_provider.cc
M components/policy/core/browser/webui/policy_status_provider.h
M components/policy/core/common/policy_scheduler.cc
M components/policy/core/common/policy_scheduler.h
M components/policy/resources/webui/policy.html
M components/policy/resources/webui/policy_base.js
M components/policy_strings.grdp
A components/policy_strings_grdp/IDS_POLICY_LABEL_TIME_SINCE_LAST_FETCH_ATTEMPT.png.sha1
A components/policy_strings_grdp/IDS_POLICY_LABEL_TIME_SINCE_LAST_REFRESH.png.sha1
M ios/chrome/browser/ui/webui/policy/policy_ui.mm
14 files changed, 117 insertions(+), 26 deletions(-)
Sergiy Belozorov (Gerrit)
Attention is currently required from: Gauthier Ambard, Nico Weber.
1 comment:
Patchset:
Gauthier, PTAL at policy_ui.mm
Nico, PTAL at everything else
To view, visit change 3150410. To unsubscribe, or for help writing mail filters, visit settings.
Gauthier Ambard (Gerrit)
Attention is currently required from: Sergiy Belozorov, Nico Weber.
Patch set 49:Code-Review +1
1 comment:
Patchset:
ios lgtm
To view, visit change 3150410. To unsubscribe, or for help writing mail filters, visit settings.
Nico Weber (Gerrit)
Attention is currently required from: Sergiy Belozorov.
2 comments:
Patchset:
Please don't link to buganizer bugs in cl descs, and especially in comments. This is an open-source project with contributors from many places, and contributors from all but one place can't access buganizer.
File chrome/browser/ui/webui/policy/policy_ui_handler.cc:
Patch Set #49, Line 489:
// TODO(b/217860627): Add timeSinceLastFetchAttempt for LaCrOS.Let's not link to buganizer bugs in comments.
To view, visit change 3150410. To unsubscribe, or for help writing mail filters, visit settings.
Nico Weber (Gerrit)
Attention is currently required from: Sergiy Belozorov.
1 comment:
Patchset:
(Instead, file a crbug that describes the problem, and link to that)
To view, visit change 3150410. To unsubscribe, or for help writing mail filters, visit settings.
Sergiy Belozorov (Gerrit)
Attention is currently required from: Sergiy Belozorov.
Sergiy Belozorov uploaded patch set #50 to this change.
Display fetch and policy timestamps separately
When the user attempts to fetch policy without an active network
connection, we used to still update the "Last fetched" timestamp. This
CL fixes this by splitting the timestamp into "Last fetch attempted"
and "Last policy timestamp". The former is always updated, the latter
is only updated when we have actually fetched an updated policy.
Bug: 1217542
Change-Id: I97416021b92964deee8d5ebc430d2ac97f1da0e6
---
M chrome/browser/ui/webui/policy/policy_ui.cc
M chrome/browser/ui/webui/policy/policy_ui_browsertest.cc
M chrome/browser/ui/webui/policy/policy_ui_handler.cc
M components/policy/core/browser/webui/machine_level_user_cloud_policy_status_provider.cc
M components/policy/core/browser/webui/policy_status_provider.cc
M components/policy/core/browser/webui/policy_status_provider.h
M components/policy/core/common/policy_scheduler.cc
M components/policy/core/common/policy_scheduler.h
M components/policy/resources/webui/policy.html
M components/policy/resources/webui/policy_base.js
M components/policy_strings.grdp
A components/policy_strings_grdp/IDS_POLICY_LABEL_TIME_SINCE_LAST_FETCH_ATTEMPT.png.sha1
A components/policy_strings_grdp/IDS_POLICY_LABEL_TIME_SINCE_LAST_REFRESH.png.sha1
M ios/chrome/browser/ui/webui/policy/policy_ui.mm
14 files changed, 117 insertions(+), 26 deletions(-)
To view, visit change 3150410. To unsubscribe, or for help writing mail filters, visit settings.
Nico Weber (Gerrit)
Attention is currently required from: Sergiy Belozorov.
Patch set 50:Code-Review +1
1 comment:
Patchset:
Other than the bug ref in that one comment, this makes sense to me.
To view, visit change 3150410. To unsubscribe, or for help writing mail filters, visit settings.
Sergiy Belozorov (Gerrit)
Patch set 51:Commit-Queue +1
1 comment:
File chrome/browser/ui/webui/policy/policy_ui_handler.cc:
Patch Set #49, Line 489:
// TODO(b/217860627): Add timeSinceLastFetchAttempt for LaCrOS.Let's not link to buganizer bugs in comments.
Done
To view, visit change 3150410. To unsubscribe, or for help writing mail filters, visit settings.
Sergiy Belozorov (Gerrit)
Attention is currently required from: Sergiy Belozorov.
Patch set 53:Commit-Queue +2
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
50 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: chrome/browser/ui/webui/policy/policy_ui_handler.cc
Insertions: 1, Deletions: 1.
The diff is too large to show. Please review the diff.
```
Display fetch and policy timestamps separately
When the user attempts to fetch policy without an active network
connection, we used to still update the "Last fetched" timestamp. This
CL fixes this by splitting the timestamp into "Last fetch attempted"
and "Last policy timestamp". The former is always updated, the latter
is only updated when we have actually fetched an updated policy.
Bug: 1217542
Change-Id: I97416021b92964deee8d5ebc430d2ac97f1da0e6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3150410
Reviewed-by: Roland Bock <rb...@google.com>
Reviewed-by: Gauthier Ambard <gam...@chromium.org>
Reviewed-by: Nico Weber <tha...@chromium.org>
Commit-Queue: Sergiy Belozorov <ser...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#976674}
---
M chrome/browser/ui/webui/policy/policy_ui.cc
M chrome/browser/ui/webui/policy/policy_ui_browsertest.cc
M chrome/browser/ui/webui/policy/policy_ui_handler.cc
M components/policy/core/browser/webui/machine_level_user_cloud_policy_status_provider.cc
M components/policy/core/browser/webui/policy_status_provider.cc
M components/policy/core/browser/webui/policy_status_provider.h
M components/policy/core/common/policy_scheduler.cc
M components/policy/core/common/policy_scheduler.h
M components/policy/resources/webui/policy.html
M components/policy/resources/webui/policy_base.js
M components/policy_strings.grdp
A components/policy_strings_grdp/IDS_POLICY_LABEL_TIME_SINCE_LAST_FETCH_ATTEMPT.png.sha1
A components/policy_strings_grdp/IDS_POLICY_LABEL_TIME_SINCE_LAST_REFRESH.png.sha1
M ios/chrome/browser/ui/webui/policy/policy_ui.mm
14 files changed, 123 insertions(+), 26 deletions(-)
Brandon Tolsch (Gerrit)
Brandon Tolsch has created a revert of this change.
To view, visit change 3150410. To unsubscribe, or for help writing mail filters, visit settings.