Commit d1c3e687 authored by mrefaat's avatar mrefaat Committed by Commit Bot

Record time spent on browsing data removal.

I added task_started time to RemovalTask structure, this was dropped
when this class was ported from content.

Bug: 796231, 759229
Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: I19f44ef3a72685b1fbe398e2915b8fd842307541
Reviewed-on: https://chromium-review.googlesource.com/1164571Reviewed-by: default avatarEugene But <eugenebut@chromium.org>
Commit-Queue: Mohammad Refaat <mrefaat@chromium.org>
Cr-Commit-Position: refs/heads/master@{#581704}
parent f170144c
...@@ -64,6 +64,7 @@ class BrowsingDataRemoverImpl : public BrowsingDataRemover { ...@@ -64,6 +64,7 @@ class BrowsingDataRemoverImpl : public BrowsingDataRemover {
base::Time delete_end; base::Time delete_end;
BrowsingDataRemoveMask mask; BrowsingDataRemoveMask mask;
base::OnceClosure callback; base::OnceClosure callback;
base::Time task_started;
}; };
// Setter for |is_removing_|; DCHECKs that we can only start removing if we're // Setter for |is_removing_|; DCHECKs that we can only start removing if we're
......
...@@ -276,7 +276,8 @@ void BrowsingDataRemoverImpl::Remove(browsing_data::TimePeriod time_period, ...@@ -276,7 +276,8 @@ void BrowsingDataRemoverImpl::Remove(browsing_data::TimePeriod time_period,
void BrowsingDataRemoverImpl::RunNextTask() { void BrowsingDataRemoverImpl::RunNextTask() {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
DCHECK(!removal_queue_.empty()); DCHECK(!removal_queue_.empty());
const RemovalTask& removal_task = removal_queue_.front(); RemovalTask& removal_task = removal_queue_.front();
removal_task.task_started = base::Time::Now();
RemoveImpl(removal_task.delete_begin, removal_task.delete_end, RemoveImpl(removal_task.delete_begin, removal_task.delete_end,
removal_task.mask); removal_task.mask);
} }
...@@ -674,6 +675,22 @@ void BrowsingDataRemoverImpl::NotifyRemovalComplete() { ...@@ -674,6 +675,22 @@ void BrowsingDataRemoverImpl::NotifyRemovalComplete() {
{ {
RemovalTask task = std::move(removal_queue_.front()); RemovalTask task = std::move(removal_queue_.front());
// Only log clear browsing data on regular browsing mode. In OTR mode, only
// few types of data are cleared and the rest is handled by deleting the
// browser state, so logging in these cases will render the histogram not
// useful.
if (!browser_state_->IsOffTheRecord()) {
base::TimeDelta delta = base::Time::Now() - task.task_started;
bool is_deletion_start_earliest = task.delete_begin.is_null();
bool is_deletion_end_now = task.delete_end.is_max();
if (is_deletion_start_earliest && is_deletion_end_now) {
UMA_HISTOGRAM_MEDIUM_TIMES(
"History.ClearBrowsingData.Duration.FullDeletion", delta);
} else {
UMA_HISTOGRAM_MEDIUM_TIMES(
"History.ClearBrowsingData.Duration.PartialDeletion", delta);
}
}
removal_queue_.pop(); removal_queue_.pop();
// Schedule the task to be executed soon. This ensure that the IsRemoving() // Schedule the task to be executed soon. This ensure that the IsRemoving()
......
...@@ -13,6 +13,7 @@ ...@@ -13,6 +13,7 @@
#include "base/run_loop.h" #include "base/run_loop.h"
#include "base/scoped_observer.h" #include "base/scoped_observer.h"
#import "base/test/ios/wait_util.h" #import "base/test/ios/wait_util.h"
#include "base/test/metrics/histogram_tester.h"
#include "components/open_from_clipboard/clipboard_recent_content.h" #include "components/open_from_clipboard/clipboard_recent_content.h"
#include "components/open_from_clipboard/fake_clipboard_recent_content.h" #include "components/open_from_clipboard/fake_clipboard_recent_content.h"
#include "ios/chrome/browser/browser_state/test_chrome_browser_state.h" #include "ios/chrome/browser/browser_state/test_chrome_browser_state.h"
...@@ -26,6 +27,9 @@ ...@@ -26,6 +27,9 @@
#error "This file requires ARC support." #error "This file requires ARC support."
#endif #endif
using base::test::ios::WaitUntilConditionOrTimeout;
using base::test::ios::kWaitForActionTimeout;
namespace { namespace {
// Flags passed when calling Remove(). Clear as much data as possible, avoiding // Flags passed when calling Remove(). Clear as much data as possible, avoiding
...@@ -45,6 +49,11 @@ constexpr BrowsingDataRemoveMask kRemoveMask = ...@@ -45,6 +49,11 @@ constexpr BrowsingDataRemoveMask kRemoveMask =
BrowsingDataRemoveMask::REMOVE_VISITED_LINKS | BrowsingDataRemoveMask::REMOVE_VISITED_LINKS |
BrowsingDataRemoveMask::REMOVE_LAST_USER_ACCOUNT; BrowsingDataRemoveMask::REMOVE_LAST_USER_ACCOUNT;
const char kFullDeletionHistogram[] =
"History.ClearBrowsingData.Duration.FullDeletion";
const char kPartialDeletionHistogram[] =
"History.ClearBrowsingData.Duration.PartialDeletion";
// Observer used to validate that BrowsingDataRemoverImpl notifies its // Observer used to validate that BrowsingDataRemoverImpl notifies its
// observers. // observers.
class TestBrowsingDataRemoverObserver : public BrowsingDataRemoverObserver { class TestBrowsingDataRemoverObserver : public BrowsingDataRemoverObserver {
...@@ -114,12 +123,11 @@ TEST_F(BrowsingDataRemoverImplTest, InvokesObservers) { ...@@ -114,12 +123,11 @@ TEST_F(BrowsingDataRemoverImplTest, InvokesObservers) {
kRemoveMask, base::DoNothing()); kRemoveMask, base::DoNothing());
TestBrowsingDataRemoverObserver* observer_ptr = &observer; TestBrowsingDataRemoverObserver* observer_ptr = &observer;
EXPECT_TRUE(base::test::ios::WaitUntilConditionOrTimeout( EXPECT_TRUE(WaitUntilConditionOrTimeout(kWaitForActionTimeout, ^{
base::test::ios::kWaitForActionTimeout, ^{ // Spin the RunLoop as WaitUntilConditionOrTimeout doesn't.
// Spin the RunLoop as WaitUntilConditionOrTimeout doesn't. base::RunLoop().RunUntilIdle();
base::RunLoop().RunUntilIdle(); return observer_ptr->last_remove_mask() == kRemoveMask;
return observer_ptr->last_remove_mask() == kRemoveMask; }));
}));
} }
// Tests that BrowsingDataRemoverImpl::Remove() can be called multiple times. // Tests that BrowsingDataRemoverImpl::Remove() can be called multiple times.
...@@ -134,12 +142,55 @@ TEST_F(BrowsingDataRemoverImplTest, SerializeRemovals) { ...@@ -134,12 +142,55 @@ TEST_F(BrowsingDataRemoverImplTest, SerializeRemovals) {
--remaining_calls; --remaining_calls;
})); }));
EXPECT_TRUE(base::test::ios::WaitUntilConditionOrTimeout( EXPECT_TRUE(WaitUntilConditionOrTimeout(kWaitForActionTimeout, ^{
base::test::ios::kWaitForActionTimeout, ^{ // Spin the RunLoop as WaitUntilConditionOrTimeout doesn't.
// Spin the RunLoop as WaitUntilConditionOrTimeout doesn't. base::RunLoop().RunUntilIdle();
base::RunLoop().RunUntilIdle(); return remaining_calls == 0;
return remaining_calls == 0; }));
})); }
// Tests that BrowsingDataRemoverImpl::Remove() Logs the duration to the correct
// histogram for full deletion.
TEST_F(BrowsingDataRemoverImplTest, LogDurationForFullDeletion) {
base::HistogramTester histogram_tester;
__block int remaining_calls = 1;
histogram_tester.ExpectTotalCount(kFullDeletionHistogram, 0);
histogram_tester.ExpectTotalCount(kPartialDeletionHistogram, 0);
browsing_data_remover_.Remove(browsing_data::TimePeriod::ALL_TIME,
kRemoveMask, base::BindOnce(^{
--remaining_calls;
}));
EXPECT_TRUE(WaitUntilConditionOrTimeout(kWaitForActionTimeout, ^{
// Spin the RunLoop as WaitUntilConditionOrTimeout doesn't.
base::RunLoop().RunUntilIdle();
return remaining_calls == 0;
}));
histogram_tester.ExpectTotalCount(kFullDeletionHistogram, 1);
histogram_tester.ExpectTotalCount(kPartialDeletionHistogram, 0);
}
// Tests that BrowsingDataRemoverImpl::Remove() Logs the duration to the correct
// histogram for partial deletion.
TEST_F(BrowsingDataRemoverImplTest, LogDurationForPartialDeletion) {
base::HistogramTester histogram_tester;
__block int remaining_calls = 1;
histogram_tester.ExpectTotalCount(kFullDeletionHistogram, 0);
histogram_tester.ExpectTotalCount(kPartialDeletionHistogram, 0);
browsing_data_remover_.Remove(browsing_data::TimePeriod::LAST_HOUR,
kRemoveMask, base::BindOnce(^{
--remaining_calls;
}));
EXPECT_TRUE(WaitUntilConditionOrTimeout(kWaitForActionTimeout, ^{
// Spin the RunLoop as WaitUntilConditionOrTimeout doesn't.
base::RunLoop().RunUntilIdle();
return remaining_calls == 0;
}));
histogram_tester.ExpectTotalCount(kFullDeletionHistogram, 0);
histogram_tester.ExpectTotalCount(kPartialDeletionHistogram, 1);
} }
// Tests that BrowsingDataRemoverImpl::Remove() can finish performing its // Tests that BrowsingDataRemoverImpl::Remove() can finish performing its
...@@ -154,10 +205,9 @@ TEST_F(BrowsingDataRemoverImplTest, PerformAfterBrowserStateDestruction) { ...@@ -154,10 +205,9 @@ TEST_F(BrowsingDataRemoverImplTest, PerformAfterBrowserStateDestruction) {
// Simulate destruction of BrowserState. // Simulate destruction of BrowserState.
browsing_data_remover_.Shutdown(); browsing_data_remover_.Shutdown();
EXPECT_TRUE(base::test::ios::WaitUntilConditionOrTimeout( EXPECT_TRUE(WaitUntilConditionOrTimeout(kWaitForActionTimeout, ^{
base::test::ios::kWaitForActionTimeout, ^{ // Spin the RunLoop as WaitUntilConditionOrTimeout doesn't.
// Spin the RunLoop as WaitUntilConditionOrTimeout doesn't. base::RunLoop().RunUntilIdle();
base::RunLoop().RunUntilIdle(); return remaining_calls == 0;
return remaining_calls == 0; }));
}));
} }
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment