[CodeHealth] Use base::ElapsedTimer where applicable (#30918)
* Use `base::ElapsedTimer` where possible pt. 1 * Refactor `misc_metrics::UsageClock` to use `base::ElapsedTimer` * Address CodeHealth PR feedback
This commit is contained in:
@@ -6,30 +6,21 @@
|
||||
#include "brave/browser/misc_metrics/usage_clock.h"
|
||||
|
||||
#include "base/check.h"
|
||||
#include "base/time/tick_clock.h"
|
||||
#include "base/time/time.h"
|
||||
|
||||
namespace misc_metrics {
|
||||
|
||||
namespace {
|
||||
|
||||
const base::TickClock* g_tick_clock_for_testing = nullptr;
|
||||
|
||||
base::TimeTicks NowTicks() {
|
||||
return g_tick_clock_for_testing ? g_tick_clock_for_testing->NowTicks()
|
||||
: base::TimeTicks::Now();
|
||||
}
|
||||
|
||||
} // namespace
|
||||
|
||||
UsageClock::UsageClock() : current_usage_session_start_time_(NowTicks()) {
|
||||
UsageClock::UsageClock() {
|
||||
bool in_session = true;
|
||||
if (metrics::DesktopSessionDurationTracker::IsInitialized()) {
|
||||
auto* tracker = metrics::DesktopSessionDurationTracker::Get();
|
||||
tracker->AddObserver(this);
|
||||
if (!tracker->in_session()) {
|
||||
current_usage_session_start_time_ = base::TimeTicks();
|
||||
in_session = false;
|
||||
}
|
||||
}
|
||||
if (in_session) {
|
||||
current_session_elapsed_timer_ = base::ElapsedTimer();
|
||||
}
|
||||
}
|
||||
|
||||
UsageClock::~UsageClock() {
|
||||
@@ -40,21 +31,21 @@ UsageClock::~UsageClock() {
|
||||
|
||||
base::TimeDelta UsageClock::GetTotalUsageTime() const {
|
||||
base::TimeDelta elapsed_time_in_session = usage_time_in_completed_sessions_;
|
||||
if (IsInUse()) {
|
||||
elapsed_time_in_session += NowTicks() - current_usage_session_start_time_;
|
||||
if (current_session_elapsed_timer_) {
|
||||
elapsed_time_in_session += current_session_elapsed_timer_->Elapsed();
|
||||
}
|
||||
return elapsed_time_in_session;
|
||||
}
|
||||
|
||||
bool UsageClock::IsInUse() const {
|
||||
return !current_usage_session_start_time_.is_null();
|
||||
return current_session_elapsed_timer_.has_value();
|
||||
}
|
||||
|
||||
void UsageClock::OnSessionStarted(base::TimeTicks session_start) {
|
||||
// Ignore |session_start| because it doesn't come from the resource
|
||||
// coordinator clock.
|
||||
DCHECK(!IsInUse());
|
||||
current_usage_session_start_time_ = NowTicks();
|
||||
current_session_elapsed_timer_ = base::ElapsedTimer();
|
||||
}
|
||||
|
||||
void UsageClock::OnSessionEnded(base::TimeDelta session_length,
|
||||
@@ -63,16 +54,8 @@ void UsageClock::OnSessionEnded(base::TimeDelta session_length,
|
||||
// coordinator clock.
|
||||
DCHECK(IsInUse());
|
||||
usage_time_in_completed_sessions_ +=
|
||||
NowTicks() - current_usage_session_start_time_;
|
||||
current_usage_session_start_time_ = base::TimeTicks();
|
||||
}
|
||||
|
||||
void UsageClock::SetTickClockForTesting(const base::TickClock* tick_clock) {
|
||||
DCHECK(!g_tick_clock_for_testing);
|
||||
g_tick_clock_for_testing = tick_clock;
|
||||
if (IsInUse()) {
|
||||
current_usage_session_start_time_ = NowTicks();
|
||||
}
|
||||
current_session_elapsed_timer_->Elapsed();
|
||||
current_session_elapsed_timer_ = std::nullopt;
|
||||
}
|
||||
|
||||
} // namespace misc_metrics
|
||||
|
||||
@@ -6,7 +6,10 @@
|
||||
#ifndef BRAVE_BROWSER_MISC_METRICS_USAGE_CLOCK_H_
|
||||
#define BRAVE_BROWSER_MISC_METRICS_USAGE_CLOCK_H_
|
||||
|
||||
#include <optional>
|
||||
|
||||
#include "base/time/time.h"
|
||||
#include "base/timer/elapsed_timer.h"
|
||||
#include "chrome/browser/metrics/desktop_session_duration/desktop_session_duration_tracker.h"
|
||||
|
||||
namespace misc_metrics {
|
||||
@@ -33,8 +36,6 @@ class UsageClock : public metrics::DesktopSessionDurationTracker::Observer {
|
||||
// Returns true if Chrome is currently considered to be in use.
|
||||
bool IsInUse() const;
|
||||
|
||||
void SetTickClockForTesting(const base::TickClock* tick_clock);
|
||||
|
||||
private:
|
||||
// DesktopSessionDurationTracker::Observer:
|
||||
void OnSessionStarted(base::TimeTicks session_start) override;
|
||||
@@ -46,9 +47,9 @@ class UsageClock : public metrics::DesktopSessionDurationTracker::Observer {
|
||||
// time of Chrome.
|
||||
base::TimeDelta usage_time_in_completed_sessions_;
|
||||
|
||||
// The time at which the current session started, or a null TimeTicks if not
|
||||
// currently in a session.
|
||||
base::TimeTicks current_usage_session_start_time_;
|
||||
// Elapsed timer for the current session, or nullopt if not currently in a
|
||||
// session.
|
||||
std::optional<base::ElapsedTimer> current_session_elapsed_timer_;
|
||||
};
|
||||
|
||||
} // namespace misc_metrics
|
||||
|
||||
@@ -5,8 +5,6 @@
|
||||
|
||||
#include "brave/browser/misc_metrics/usage_clock.h"
|
||||
|
||||
#include "base/test/simple_test_clock.h"
|
||||
#include "base/test/simple_test_tick_clock.h"
|
||||
#include "base/test/task_environment.h"
|
||||
#include "chrome/browser/metrics/desktop_session_duration/desktop_session_duration_tracker.h"
|
||||
#include "testing/gtest/include/gtest/gtest.h"
|
||||
@@ -15,12 +13,10 @@ namespace misc_metrics {
|
||||
|
||||
TEST(ResourceCoordinatorUsageClock, UsageClock) {
|
||||
// Required to use DesktopSessionDurationTracker.
|
||||
base::test::TaskEnvironment task_environment;
|
||||
base::test::TaskEnvironment task_environment(
|
||||
base::test::TaskEnvironment::TimeSource::MOCK_TIME);
|
||||
|
||||
{
|
||||
base::SimpleTestTickClock tick_clock;
|
||||
tick_clock.Advance(base::Minutes(42));
|
||||
|
||||
metrics::DesktopSessionDurationTracker::Initialize();
|
||||
auto* tracker = metrics::DesktopSessionDurationTracker::Get();
|
||||
tracker->OnVisibilityChanged(true, base::TimeDelta());
|
||||
@@ -28,33 +24,32 @@ TEST(ResourceCoordinatorUsageClock, UsageClock) {
|
||||
EXPECT_TRUE(tracker->in_session());
|
||||
|
||||
UsageClock usage_clock;
|
||||
usage_clock.SetTickClockForTesting(&tick_clock);
|
||||
EXPECT_EQ(usage_clock.GetTotalUsageTime(), base::TimeDelta());
|
||||
EXPECT_TRUE(tracker->in_session());
|
||||
EXPECT_TRUE(usage_clock.IsInUse());
|
||||
|
||||
// Verify that time advances when Chrome is in use.
|
||||
tick_clock.Advance(base::Minutes(1));
|
||||
task_environment.FastForwardBy(base::Minutes(1));
|
||||
EXPECT_EQ(usage_clock.GetTotalUsageTime(), base::Minutes(1));
|
||||
tick_clock.Advance(base::Minutes(1));
|
||||
task_environment.FastForwardBy(base::Minutes(1));
|
||||
EXPECT_EQ(usage_clock.GetTotalUsageTime(), base::Minutes(2));
|
||||
|
||||
// Verify that time is updated when Chrome stops being used.
|
||||
tick_clock.Advance(base::Minutes(1));
|
||||
task_environment.FastForwardBy(base::Minutes(1));
|
||||
tracker->OnVisibilityChanged(false, base::TimeDelta());
|
||||
EXPECT_FALSE(tracker->in_session());
|
||||
EXPECT_FALSE(usage_clock.IsInUse());
|
||||
EXPECT_EQ(usage_clock.GetTotalUsageTime(), base::Minutes(3));
|
||||
|
||||
// Verify that time stays still when Chrome is not in use.
|
||||
tick_clock.Advance(base::Minutes(1));
|
||||
task_environment.FastForwardBy(base::Minutes(1));
|
||||
EXPECT_EQ(usage_clock.GetTotalUsageTime(), base::Minutes(3));
|
||||
|
||||
// Verify that time advances again when Chrome is in use.
|
||||
tracker->OnVisibilityChanged(true, base::TimeDelta());
|
||||
EXPECT_TRUE(tracker->in_session());
|
||||
EXPECT_TRUE(usage_clock.IsInUse());
|
||||
tick_clock.Advance(base::Minutes(1));
|
||||
task_environment.FastForwardBy(base::Minutes(1));
|
||||
EXPECT_EQ(usage_clock.GetTotalUsageTime(), base::Minutes(4));
|
||||
}
|
||||
|
||||
|
||||
@@ -17,6 +17,7 @@
|
||||
#include "base/metrics/histogram_macros.h"
|
||||
#include "base/notreached.h"
|
||||
#include "base/strings/string_util.h"
|
||||
#include "base/timer/elapsed_timer.h"
|
||||
#include "brave/browser/brave_browser_process.h"
|
||||
#include "brave/browser/brave_shields/ad_block_pref_service_factory.h"
|
||||
#include "brave/browser/brave_shields/brave_shields_web_contents_observer.h"
|
||||
@@ -76,7 +77,7 @@ class AdblockCnameResolveHostClient : public network::mojom::ResolveHostClient {
|
||||
private:
|
||||
mojo::Receiver<network::mojom::ResolveHostClient> receiver_{this};
|
||||
base::OnceCallback<void(std::optional<std::string>)> cb_;
|
||||
base::TimeTicks start_time_;
|
||||
base::ElapsedTimer elapsed_timer_;
|
||||
|
||||
public:
|
||||
AdblockCnameResolveHostClient(
|
||||
@@ -104,7 +105,7 @@ class AdblockCnameResolveHostClient : public network::mojom::ResolveHostClient {
|
||||
if (secure_dns_config.mode() == net::SecureDnsMode::kSecure)
|
||||
optional_parameters->source = net::HostResolverSource::DNS;
|
||||
|
||||
start_time_ = base::TimeTicks::Now();
|
||||
elapsed_timer_ = {};
|
||||
|
||||
if (g_testing_host_resolver) {
|
||||
g_testing_host_resolver->ResolveHost(
|
||||
@@ -116,7 +117,7 @@ class AdblockCnameResolveHostClient : public network::mojom::ResolveHostClient {
|
||||
auto* web_contents =
|
||||
content::WebContents::FromFrameTreeNodeId(ctx->frame_tree_node_id);
|
||||
if (!web_contents) {
|
||||
start_time_ = base::TimeTicks::Now();
|
||||
elapsed_timer_ = {};
|
||||
this->OnComplete(net::ERR_FAILED, net::ResolveErrorInfo(),
|
||||
net::AddressList(),
|
||||
net::HostResolverEndpointResults());
|
||||
@@ -147,7 +148,7 @@ class AdblockCnameResolveHostClient : public network::mojom::ResolveHostClient {
|
||||
const net::AddressList& resolved_addresses,
|
||||
const net::HostResolverEndpointResults& alternative_endpoints) override {
|
||||
UMA_HISTOGRAM_TIMES("Brave.ShieldsCNAMEBlocking.TotalResolutionTime",
|
||||
base::TimeTicks::Now() - start_time_);
|
||||
elapsed_timer_.Elapsed());
|
||||
if (result == net::OK) {
|
||||
DCHECK(!resolved_addresses.empty());
|
||||
std::move(cb_).Run(std::optional<std::string>(
|
||||
|
||||
@@ -143,7 +143,7 @@ void BraveProxyingURLLoaderFactory::InProgressRequest::UpdateRequestInfo() {
|
||||
|
||||
void BraveProxyingURLLoaderFactory::InProgressRequest::RestartInternal() {
|
||||
request_completed_ = false;
|
||||
start_time_ = base::TimeTicks::Now();
|
||||
elapsed_timer_ = {};
|
||||
|
||||
base::RepeatingCallback<void(int)> continuation =
|
||||
base::BindRepeating(&InProgressRequest::ContinueToBeforeSendHeaders,
|
||||
@@ -254,7 +254,7 @@ void BraveProxyingURLLoaderFactory::InProgressRequest::OnTransferSizeUpdated(
|
||||
void BraveProxyingURLLoaderFactory::InProgressRequest::OnComplete(
|
||||
const network::URLLoaderCompletionStatus& status) {
|
||||
UMA_HISTOGRAM_TIMES("Brave.ProxyingURLLoader.TotalRequestTime",
|
||||
base::TimeTicks::Now() - start_time_);
|
||||
elapsed_timer_.Elapsed());
|
||||
if (status.error_code != net::OK) {
|
||||
OnRequestError(status);
|
||||
return;
|
||||
|
||||
@@ -22,6 +22,7 @@
|
||||
#include "base/memory/ref_counted_delete_on_sequence.h"
|
||||
#include "base/memory/weak_ptr.h"
|
||||
#include "base/time/time.h"
|
||||
#include "base/timer/elapsed_timer.h"
|
||||
#include "brave/browser/net/resource_context_data.h"
|
||||
#include "brave/browser/net/url_context.h"
|
||||
#include "mojo/public/cpp/bindings/pending_receiver.h"
|
||||
@@ -114,7 +115,7 @@ class BraveProxyingURLLoaderFactory
|
||||
void OnRequestError(const network::URLLoaderCompletionStatus& status);
|
||||
void HandleBeforeRequestRedirect();
|
||||
|
||||
base::TimeTicks start_time_;
|
||||
base::ElapsedTimer elapsed_timer_;
|
||||
|
||||
// TODO(iefremov): Get rid of shared_ptr, we should clearly own the pointer.
|
||||
std::shared_ptr<brave::BraveRequestInfo> ctx_;
|
||||
|
||||
@@ -412,8 +412,7 @@ PageGraph::PageGraph(LocalFrame& local_frame)
|
||||
: Supplement<LocalFrame>(local_frame),
|
||||
frame_id_(GetFrameId(local_frame)),
|
||||
script_tracker_(this),
|
||||
request_tracker_(this),
|
||||
start_(base::TimeTicks::Now()) {
|
||||
request_tracker_(this) {
|
||||
CHECK(local_frame.IsLocalRoot());
|
||||
blink::Page* page = local_frame.GetPage();
|
||||
CHECK(page);
|
||||
@@ -1037,7 +1036,7 @@ void PageGraph::RegisterV8JSBuiltinCall(
|
||||
}
|
||||
|
||||
base::TimeTicks PageGraph::GetGraphStartTime() const {
|
||||
return start_;
|
||||
return elapsed_timer_.start_time();
|
||||
}
|
||||
|
||||
GraphItemId PageGraph::GetNextGraphItemId() {
|
||||
@@ -1174,7 +1173,7 @@ String PageGraph::ToGraphML() const {
|
||||
xmlNewTextChild(time_container_node, nullptr, BAD_CAST "start",
|
||||
BAD_CAST base::NumberToString(0).c_str());
|
||||
|
||||
const base::TimeDelta end_time = base::TimeTicks::Now() - start_;
|
||||
const auto end_time = elapsed_timer_.Elapsed();
|
||||
xmlNewTextChild(
|
||||
time_container_node, nullptr, BAD_CAST "end",
|
||||
BAD_CAST base::NumberToString(end_time.InMilliseconds()).c_str());
|
||||
@@ -1675,7 +1674,7 @@ void PageGraph::RegisterRequestStartForDocument(blink::DocumentLoader* loader,
|
||||
CHECK(frame);
|
||||
bool is_main_frame = frame->IsMainFrame();
|
||||
const FrameId frame_id = GetFrameId(*frame);
|
||||
const base::TimeDelta timestamp = base::TimeTicks::Now() - start_;
|
||||
const auto timestamp = elapsed_timer_.Elapsed();
|
||||
|
||||
VLOG(1) << "RegisterRequestStartForDocument) frame id: " << frame_id
|
||||
<< ", request id: " << request_id << ", url: " << url
|
||||
@@ -1714,7 +1713,7 @@ void PageGraph::RegisterRequestCompleteForDocument(
|
||||
<< ", frame id: " << frame_id
|
||||
<< ", encoded_data_length: " << encoded_data_length;
|
||||
|
||||
const base::TimeDelta timestamp = base::TimeTicks::Now() - start_;
|
||||
const auto timestamp = elapsed_timer_.Elapsed();
|
||||
request_tracker_.RegisterDocumentRequestComplete(
|
||||
request_id, frame_id, encoded_data_length, timestamp);
|
||||
}
|
||||
|
||||
@@ -14,6 +14,7 @@
|
||||
|
||||
#include "base/containers/span_or_size.h"
|
||||
#include "base/time/time.h"
|
||||
#include "base/timer/elapsed_timer.h"
|
||||
#include "brave/third_party/blink/renderer/core/brave_page_graph/blink_probe_types.h"
|
||||
#include "brave/third_party/blink/renderer/core/brave_page_graph/page_graph_context.h"
|
||||
#include "brave/third_party/blink/renderer/core/brave_page_graph/requests/request_tracker.h"
|
||||
@@ -461,8 +462,8 @@ class CORE_EXPORT PageGraph : public GarbageCollected<PageGraph>,
|
||||
// Data structure for keeping track of all the in-air requests that
|
||||
// have been made, but have not completed.
|
||||
RequestTracker request_tracker_;
|
||||
// Page Graph start time stamp.
|
||||
base::TimeTicks start_;
|
||||
// Page Graph elapsed time timer.
|
||||
base::ElapsedTimer elapsed_timer_;
|
||||
// Monotonically increasing counter, used so that we can replay the
|
||||
// the graph's construction if needed.
|
||||
GraphItemId id_counter_ = 0;
|
||||
|
||||
Reference in New Issue
Block a user