Commit 03a25075 authored by Aran Gilman's avatar Aran Gilman Committed by Commit Bot

Add UKMs for Reader Mode.

Note that the tests currently cover only one metric,
ReaderModeReceivedDistillability. The other still needs to have a test
added in a follow-up CL.

See go/chrome-desktop-reader-mode-dd#heading=h.5rfn5g2dude for the UKM
collection review.

Bug: 992074
Change-Id: I30b31bca6d7d3e3310c984022118a56d3b0a68b4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1881497Reviewed-by: default avatarElly Fong-Jones <ellyjones@chromium.org>
Reviewed-by: default avatarSteven Holte <holte@chromium.org>
Commit-Queue: Aran Gilman <gilmanmh@google.com>
Cr-Commit-Position: refs/heads/master@{#748788}
parent fa3ca921
...@@ -10,6 +10,7 @@ ...@@ -10,6 +10,7 @@
#include "base/optional.h" #include "base/optional.h"
#include "base/run_loop.h" #include "base/run_loop.h"
#include "base/threading/thread_task_runner_handle.h" #include "base/threading/thread_task_runner_handle.h"
#include "build/build_config.h"
#include "chrome/browser/ui/browser.h" #include "chrome/browser/ui/browser.h"
#include "chrome/browser/ui/tabs/tab_strip_model.h" #include "chrome/browser/ui/tabs/tab_strip_model.h"
#include "chrome/common/chrome_switches.h" #include "chrome/common/chrome_switches.h"
...@@ -23,14 +24,23 @@ ...@@ -23,14 +24,23 @@
#include "net/test/embedded_test_server/embedded_test_server.h" #include "net/test/embedded_test_server/embedded_test_server.h"
#include "testing/gmock/include/gmock/gmock.h" #include "testing/gmock/include/gmock/gmock.h"
#if defined(OS_CHROMEOS) || defined(OS_LINUX) || defined(OS_MACOSX) || \
defined(OS_WIN)
#include "components/ukm/test_ukm_recorder.h"
#include "testing/gmock/include/gmock/gmock.h"
#endif
namespace dom_distiller { namespace dom_distiller {
namespace { namespace {
using ::testing::_; using ::testing::_;
using ::testing::AllOf; using ::testing::AllOf;
using ::testing::Field; using ::testing::Field;
using ::testing::InvokeWithoutArgs;
using ::testing::Not; using ::testing::Not;
using ::testing::Optional; using ::testing::Optional;
using ::testing::Pointee;
using ::testing::SizeIs;
const char kSimpleArticlePath[] = "/dom_distiller/simple_article.html"; const char kSimpleArticlePath[] = "/dom_distiller/simple_article.html";
const char kSimpleArticleIFramePath[] = const char kSimpleArticleIFramePath[] =
...@@ -259,4 +269,39 @@ IN_PROC_BROWSER_TEST_F(DistillablePageUtilsBrowserTestAllArticles, ...@@ -259,4 +269,39 @@ IN_PROC_BROWSER_TEST_F(DistillablePageUtilsBrowserTestAllArticles,
Optional(AllOf(IsDistillable(), IsLast(), Not(IsMobileFriendly())))); Optional(AllOf(IsDistillable(), IsLast(), Not(IsMobileFriendly()))));
} }
#if defined(OS_CHROMEOS) || defined(OS_LINUX) || defined(OS_MACOSX) || \
defined(OS_WIN)
IN_PROC_BROWSER_TEST_F(DistillablePageUtilsBrowserTestAllArticles,
RecordPageIsDistillableOnArticleLoad) {
ON_CALL(holder_, OnResult(IsLast()))
.WillByDefault(InvokeWithoutArgs(this, &TestOption::QuitSoon));
ukm::TestAutoSetUkmRecorder ukm_recorder;
NavigateAndWait(kSimpleArticlePath, base::TimeDelta());
std::vector<const ukm::mojom::UkmEntry*> distillability_entries =
ukm_recorder.GetEntriesByName("ReaderModeReceivedDistillability");
ASSERT_THAT(distillability_entries, SizeIs(1));
EXPECT_THAT(ukm_recorder.GetEntryMetric(distillability_entries.front(),
"IsPageDistillable"),
Pointee(true));
}
IN_PROC_BROWSER_TEST_F(DistillablePageUtilsBrowserTestAllArticles,
RecordPageIsNotDistillableOnNonArticleLoad) {
ON_CALL(holder_, OnResult(IsLast()))
.WillByDefault(InvokeWithoutArgs(this, &TestOption::QuitSoon));
ukm::TestAutoSetUkmRecorder ukm_recorder;
NavigateAndWait(kNonArticlePath, base::TimeDelta());
std::vector<const ukm::mojom::UkmEntry*> distillability_entries =
ukm_recorder.GetEntriesByName("ReaderModeReceivedDistillability");
ASSERT_THAT(distillability_entries, SizeIs(1));
EXPECT_THAT(ukm_recorder.GetEntryMetric(distillability_entries.front(),
"IsPageDistillable"),
Pointee(false));
}
#endif // OS_CHROMEOS || OS_LINUX || OS_MACOS || OS_WIN
} // namespace dom_distiller } // namespace dom_distiller
...@@ -11,8 +11,11 @@ ...@@ -11,8 +11,11 @@
#include "components/dom_distiller/core/uma_helper.h" #include "components/dom_distiller/core/uma_helper.h"
#include "components/dom_distiller/core/url_utils.h" #include "components/dom_distiller/core/url_utils.h"
#include "components/prefs/pref_service.h" #include "components/prefs/pref_service.h"
#include "components/ukm/content/source_url_recorder.h"
#include "content/public/browser/navigation_handle.h" #include "content/public/browser/navigation_handle.h"
#include "content/public/browser/web_contents.h" #include "content/public/browser/web_contents.h"
#include "services/metrics/public/cpp/ukm_builders.h"
#include "services/metrics/public/cpp/ukm_recorder.h"
#include "ui/base/l10n/l10n_util.h" #include "ui/base/l10n/l10n_util.h"
using dom_distiller::url_utils::IsDistilledPage; using dom_distiller::url_utils::IsDistilledPage;
...@@ -108,9 +111,24 @@ void ReaderModeIconView::OnExecuting( ...@@ -108,9 +111,24 @@ void ReaderModeIconView::OnExecuting(
dom_distiller::UMAHelper::RecordReaderModeEntry( dom_distiller::UMAHelper::RecordReaderModeEntry(
dom_distiller::UMAHelper::ReaderModeEntryPoint::kOmniboxIcon); dom_distiller::UMAHelper::ReaderModeEntryPoint::kOmniboxIcon);
} }
content::WebContents* contents = GetWebContents();
if (!contents || IsDistilledPage(contents->GetLastCommittedURL()))
return;
ukm::SourceId source_id = ukm::GetSourceIdForWebContentsDocument(contents);
ukm::builders::ReaderModeActivated(source_id)
.SetActivatedViaOmnibox(true)
.Record(ukm::UkmRecorder::Get());
} }
void ReaderModeIconView::OnResult( void ReaderModeIconView::OnResult(
const dom_distiller::DistillabilityResult& result) { const dom_distiller::DistillabilityResult& result) {
content::WebContents* contents = GetWebContents();
if (contents && result.is_last) {
ukm::SourceId source_id = ukm::GetSourceIdForWebContentsDocument(contents);
ukm::builders::ReaderModeReceivedDistillability(source_id)
.SetIsPageDistillable(result.is_distillable)
.Record(ukm::UkmRecorder::Get());
}
Update(); Update();
} }
...@@ -42,11 +42,11 @@ class ReaderModeIconView : public PageActionIconView, ...@@ -42,11 +42,11 @@ class ReaderModeIconView : public PageActionIconView,
const gfx::VectorIcon& GetVectorIcon() const override; const gfx::VectorIcon& GetVectorIcon() const override;
base::string16 GetTextForTooltipAndAccessibleName() const override; base::string16 GetTextForTooltipAndAccessibleName() const override;
const char* GetClassName() const override; const char* GetClassName() const override;
void OnExecuting(PageActionIconView::ExecuteSource execute_source) override;
// GetBubble() is required by PageActionIconView; however, the icon // GetBubble() is required by PageActionIconView; however, the icon
// intentionally does not display a bubble when activated. // intentionally does not display a bubble when activated.
views::BubbleDialogDelegateView* GetBubble() const override; views::BubbleDialogDelegateView* GetBubble() const override;
void OnExecuting(PageActionIconView::ExecuteSource execute_source) override;
void OnResult(const dom_distiller::DistillabilityResult& result) override; void OnResult(const dom_distiller::DistillabilityResult& result) override;
......
...@@ -1327,7 +1327,6 @@ if (!is_android) { ...@@ -1327,7 +1327,6 @@ if (!is_android) {
"../browser/ui/views/device_chooser_browsertest.cc", "../browser/ui/views/device_chooser_browsertest.cc",
"../browser/ui/views/hats/hats_browsertest.cc", "../browser/ui/views/hats/hats_browsertest.cc",
"../browser/ui/views/intent_picker_bubble_view_browsertest.cc", "../browser/ui/views/intent_picker_bubble_view_browsertest.cc",
"../browser/ui/views/reader_mode/reader_mode_icon_view_browsertest.cc",
"../browser/ui/views/try_chrome_dialog_win/try_chrome_dialog_browsertest.cc", "../browser/ui/views/try_chrome_dialog_win/try_chrome_dialog_browsertest.cc",
"../browser/ui/views/web_apps/web_app_frame_toolbar_browsertest.cc", "../browser/ui/views/web_apps/web_app_frame_toolbar_browsertest.cc",
"../browser/ui/views/web_apps/web_app_minimal_ui_test.cc", "../browser/ui/views/web_apps/web_app_minimal_ui_test.cc",
......
...@@ -8786,6 +8786,36 @@ be describing additional metrics about the same event. ...@@ -8786,6 +8786,36 @@ be describing additional metrics about the same event.
</metric> </metric>
</event> </event>
<event name="ReaderModeActivated" singular="True">
<owner>gilmanmh@google.com</owner>
<owner>kjbooker@google.com</owner>
<owner>dmazzoni@chromium.org</owner>
<summary>
Recorded after a user activates Reader Mode for a given page.
</summary>
<metric name="ActivatedViaOmnibox" enum="Boolean">
<summary>
Whether the user activated Reader Mode via the omnibox badge.
</summary>
</metric>
</event>
<event name="ReaderModeReceivedDistillability">
<owner>gilmanmh@google.com</owner>
<owner>kjbooker@google.com</owner>
<owner>dmazzoni@chromium.org</owner>
<summary>
Recorded when the Reader Mode badge receives a result from the
distillability service.
</summary>
<metric name="IsPageDistillable" enum="Boolean">
<summary>
Whether the distillability service determined the page to be distillable,
i.e. likely to render well in Reader Mode.
</summary>
</metric>
</event>
<event name="RendererSchedulerTask"> <event name="RendererSchedulerTask">
<owner>altimin@chromium.org</owner> <owner>altimin@chromium.org</owner>
<summary> <summary>
......
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