Skip to content

GitLab

T
tangled
Commit fa9be289 authored by Tarun Bansal's avatar Tarun Bansal Committed by Commit Bot
Browse files
Options

Revert "Include anchor element metrics that differ from one from document URL" 

This reverts commit 290d578e.

Reason for revert: Crashes renderer

Original change's description:
> Include anchor element metrics that differ from one from document URL
> 
> Currently, the render process sends the statistics of
> only the anchor elements that are in the viewport to
> the browser. This CL changes it to also include the
> anchor elements whose target URL is one digit different
> from the document URL.
> 
> Note that there is still a limit of 40 on the count of anchor
> elements that are sent from the render to the browser process.
> 
> Change-Id: I4083e3605095a44700cc2178615713ede6e0dde2
> Bug: 850624
> TBR: ryansturm@chromium.org
> Reviewed-on: https://chromium-review.googlesource.com/c/1329848
> Commit-Queue: Tarun Bansal <tbansal@chromium.org>
> Reviewed-by: Tarun Bansal <tbansal@chromium.org>
> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#607328}

TBR=kinuko@chromium.org,tbansal@chromium.org,ryansturm@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 850624,905224
Change-Id: I8fe41adb0434d410eb6fd2244e525c05037585f2
Reviewed-on: https://chromium-review.googlesource.com/c/1335664Reviewed-by: default avatarTarun Bansal <tbansal@chromium.org>
Commit-Queue: Tarun Bansal <tbansal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#608105}
parent 19f9a541
Show whitespace changes
Inline Side-by-side
Showing with 9 additions and 65 deletions
chrome/browser/navigation_predictor/navigation_predictor_browsertest.cc
View file @ fa9be289
...@@ -464,27 +464,3 @@ IN_PROC_BROWSER_TEST_P(NavigationPredictorBrowserTest, ...@@ -464,27 +464,3 @@ IN_PROC_BROWSER_TEST_P(NavigationPredictorBrowserTest,
"AnchorElementMetrics.Clicked.OnNonDSE.SameHost", 0, 1); "AnchorElementMetrics.Clicked.OnNonDSE.SameHost", 0, 1);
} }
} }
// Tests that the browser only receives anchor elements that are in the
// viewport, and from anchor elements whose target differ from document URL
// by one digit.
IN_PROC_BROWSER_TEST_P(NavigationPredictorBrowserTest,
ViewportOnlyAndUrlIncrementByOne) {
base::HistogramTester histogram_tester;
const GURL& url = GetTestURL("/long_page_with_anchors-1.html");
ui_test_utils::NavigateToURL(browser(), url);
base::RunLoop().RunUntilIdle();
if (base::FeatureList::IsEnabled(
blink::features::kRecordAnchorMetricsVisible)) {
histogram_tester.ExpectUniqueSample(
"AnchorElementMetrics.Visible.NumberOfAnchorElements", 2, 1);
// Same document anchor element should be removed after merge.
histogram_tester.ExpectUniqueSample(
"AnchorElementMetrics.Visible.NumberOfAnchorElementsAfterMerge", 2, 1);
} else {
histogram_tester.ExpectTotalCount(
"AnchorElementMetrics.Visible.NumberOfAnchorElements", 0);
}
}
chrome/test/data/navigation_predictor/long_page_with_anchors-1.html deleted 100644 → 0
View file @ 19f9a541
<html>
<head>
</head>
<body>
<a id="google" href="https://google.com">Google</a>
<img src="large_image_takes_all_viewport.gif" height="4200" width="4200">
<a id="example2" href="https://example2.com">Below viewport</a>
<a id="example3" href="long_page_with_anchors-2.html">Below viewport differ by one</a>
</body>
</html>
\ No newline at end of file
third_party/blink/renderer/core/html/anchor_element_metrics.cc
View file @ fa9be289
...@@ -141,8 +141,7 @@ const int AnchorElementMetrics::kMaxAnchorElementMetricsSize = 40; ...@@ -141,8 +141,7 @@ const int AnchorElementMetrics::kMaxAnchorElementMetricsSize = 40;
// static // static
base::Optional<AnchorElementMetrics> AnchorElementMetrics::Create( base::Optional<AnchorElementMetrics> AnchorElementMetrics::Create(
const HTMLAnchorElement* anchor_element, const HTMLAnchorElement* anchor_element) {
bool is_url_incremented_by_one) {
LocalFrame* local_frame = anchor_element->GetDocument().GetFrame(); LocalFrame* local_frame = anchor_element->GetDocument().GetFrame();
LayoutObject* layout_object = anchor_element->GetLayoutObject(); LayoutObject* layout_object = anchor_element->GetLayoutObject();
if (!local_frame || !layout_object) if (!local_frame || !layout_object)
...@@ -154,11 +153,7 @@ base::Optional<AnchorElementMetrics> AnchorElementMetrics::Create( ...@@ -154,11 +153,7 @@ base::Optional<AnchorElementMetrics> AnchorElementMetrics::Create(
return base::nullopt; return base::nullopt;
IntRect viewport = root_frame_view->LayoutViewport()->VisibleContentRect(); IntRect viewport = root_frame_view->LayoutViewport()->VisibleContentRect();
if (viewport.Size().IsEmpty())
// Do not ignore anchor elements for which |is_url_incremented_by_one| is
// true since there is typically high click probability for such anchor
// elements.
if (viewport.Size().IsEmpty() && !is_url_incremented_by_one)
return base::nullopt; return base::nullopt;
// Use the viewport size to normalize anchor element metrics. // Use the viewport size to normalize anchor element metrics.
...@@ -210,7 +205,7 @@ base::Optional<AnchorElementMetrics> AnchorElementMetrics::Create( ...@@ -210,7 +205,7 @@ base::Optional<AnchorElementMetrics> AnchorElementMetrics::Create(
ratio_distance_top_to_visible_top, ratio_distance_center_to_visible_top, ratio_distance_top_to_visible_top, ratio_distance_center_to_visible_top,
ratio_distance_root_top, ratio_distance_root_bottom, ratio_root_height, ratio_distance_root_top, ratio_distance_root_bottom, ratio_root_height,
IsInIFrame(*anchor_element), ContainsImage(*anchor_element), IsInIFrame(*anchor_element), ContainsImage(*anchor_element),
IsSameHost(*anchor_element), is_url_incremented_by_one); IsSameHost(*anchor_element), IsUrlIncrementedByOne(*anchor_element));
} }
// static // static
...@@ -224,8 +219,7 @@ AnchorElementMetrics::MaybeReportClickedMetricsOnClick( ...@@ -224,8 +219,7 @@ AnchorElementMetrics::MaybeReportClickedMetricsOnClick(
return base::nullopt; return base::nullopt;
} }
bool is_url_incremented_by_one = IsUrlIncrementedByOne(*anchor_element); auto anchor_metrics = Create(anchor_element);
auto anchor_metrics = Create(anchor_element, is_url_incremented_by_one);
if (anchor_metrics.has_value()) { if (anchor_metrics.has_value()) {
anchor_metrics.value().RecordMetricsOnClick(); anchor_metrics.value().RecordMetricsOnClick();
...@@ -255,22 +249,14 @@ void AnchorElementMetrics::MaybeReportViewportMetricsOnLoad( ...@@ -255,22 +249,14 @@ void AnchorElementMetrics::MaybeReportViewportMetricsOnLoad(
for (const auto& member_element : sender->GetAnchorElements()) { for (const auto& member_element : sender->GetAnchorElements()) {
const HTMLAnchorElement& anchor_element = *member_element; const HTMLAnchorElement& anchor_element = *member_element;
if (!anchor_element.Href().ProtocolIsInHTTPFamily())
continue;
bool is_url_incremented_by_one = IsUrlIncrementedByOne(anchor_element);
// We ignore anchor elements that are not in the visual viewport. // We ignore anchor elements that are not in the visual viewport.
// Do not ignore anchor elements for which |is_url_incremented_by_one| is if (!anchor_element.Href().ProtocolIsInHTTPFamily() ||
// true since there is typically high click probability for such anchor anchor_element.VisibleBoundsInVisualViewport().IsEmpty()) {
// elements.
if (anchor_element.VisibleBoundsInVisualViewport().IsEmpty() &&
!is_url_incremented_by_one) {
continue; continue;
} }
base::Optional<AnchorElementMetrics> anchor_metric = base::Optional<AnchorElementMetrics> anchor_metric =
Create(&anchor_element, is_url_incremented_by_one); Create(&anchor_element);
if (!anchor_metric.has_value()) if (!anchor_metric.has_value())
continue; continue;
......
third_party/blink/renderer/core/html/anchor_element_metrics.h
View file @ fa9be289
...@@ -55,16 +55,8 @@ class CORE_EXPORT AnchorElementMetrics { ...@@ -55,16 +55,8 @@ class CORE_EXPORT AnchorElementMetrics {
// browser on page load. // browser on page load.
static const int kMaxAnchorElementMetricsSize; static const int kMaxAnchorElementMetricsSize;
// Extract features of the anchor element. |is_url_incremented_by_one| is // Extract features of the anchor element.
// true if |anchor_element| links to a URL which is one number different static base::Optional<AnchorElementMetrics> Create(const HTMLAnchorElement*);
// from the document's URL. Based on the value of the anchor element URL,
// and document URL, |is_url_incremented_by_one| may be true or false.
// Examples for different values of |is_url_incremented_by_one|:
// example.com/page9/cat5, example.com/page10/cat5 => true
// example.com/page9/cat5, example.com/page10/cat10 => false
static base::Optional<AnchorElementMetrics> Create(
const HTMLAnchorElement* anchor_element,
bool is_url_incremented_by_one);
// Returns the mojom struct used to send metrics to the browser process. // Returns the mojom struct used to send metrics to the browser process.
mojom::blink::AnchorElementMetricsPtr CreateMetricsPtr() const; mojom::blink::AnchorElementMetricsPtr CreateMetricsPtr() const;
......
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