Commit b8881ace authored by Findit's avatar Findit

Revert "[SH-Blink] Move Link Generation Error Logging"

This reverts commit c754406a.

Reason for revert:

Findit (https://goo.gl/kROfz5) identified CL at revision 830817 as the
culprit for failures in the build cycles as shown on:
https://analysis.chromium.org/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3ItbWVyRAsSDVdmU3VzcGVjdGVkQ0wiMWNocm9taXVtL2M3NTQ0MDZhY2JjZjgwOWU1OGRjMWFlMmMwZTVkYzIzZDk2MDMzYTYM

Sample Failed Build: https://ci.chromium.org/b/8862701172936116704

Sample Failed Step: compile

Original change's description:
> [SH-Blink] Move Link Generation Error Logging
> 
> By consolidating the error histogram logging (and import the shared
> component), we'll be able to make use of the new UKM logging functions
> in a follow-up CL and provide it with the error causes.
> 
> Added histogram validation steps in unit tests before moving the
> logging logic.
> 
> Bug: 1148807
> Change-Id: I63fd8ca1652d885705c0a17f6131e207ce2e6da3
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2551793
> Commit-Queue: Sebastien Lalancette <seblalancette@chromium.org>
> Reviewed-by: Kentaro Hara <haraken@chromium.org>
> Reviewed-by: David Bokan <bokan@chromium.org>
> Reviewed-by: Gayane Petrosyan <gayane@chromium.org>
> Reviewed-by: sebsg <sebsg@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#830817}


Change-Id: I27cc3e78742d57aba7fb9b23f595cbd6e12cf6c9
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 1148807
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2559585
Cr-Commit-Position: refs/heads/master@{#830820}
parent 58287cdf
......@@ -13,7 +13,6 @@ namespace shared_highlighting {
// These values are persisted to logs. Entries should not be renumbered and
// numeric values should never be reused.
// The type of errors that can happen during link generation.
// Update corresponding |LinkGenerationError| in enums.xml.
enum class LinkGenerationError {
kIncorrectSelector = 0,
kNoRange = 1,
......
......@@ -263,7 +263,6 @@ component("core") {
deps = [
"//build:chromeos_buildflags",
"//components/paint_preview/common",
"//components/shared_highlighting/core/common",
"//gpu/config",
"//mojo/public/cpp/bindings:bindings",
"//mojo/public/cpp/system",
......@@ -1621,7 +1620,6 @@ source_set("unit_tests") {
":unit_test_support",
"//build:chromeos_buildflags",
"//components/paint_preview/common:common",
"//components/shared_highlighting/core/common",
"//components/ukm:test_support",
"//mojo/public/cpp/system",
"//mojo/public/cpp/test_support:test_utils",
......
include_rules = [
"+components/shared_highlighting/core/common"
]
specific_include_rules = {
"scrolling_test.cc": [
"+third_party/blink/renderer/core/exported/web_plugin_container_impl.h"
......
......@@ -6,7 +6,6 @@
#include "base/metrics/histogram_macros.h"
#include "base/time/default_tick_clock.h"
#include "components/shared_highlighting/core/common/shared_highlighting_metrics.h"
#include "third_party/blink/public/common/browser_interface_broker_proxy.h"
#include "third_party/blink/public/platform/interface_registry.h"
#include "third_party/blink/renderer/core/editing/ephemeral_range.h"
......@@ -17,8 +16,6 @@
#include "third_party/blink/renderer/core/page/scrolling/text_fragment_finder.h"
#include "third_party/blink/renderer/platform/text/text_boundaries.h"
using LinkGenerationError = shared_highlighting::LinkGenerationError;
namespace blink {
namespace {
......@@ -267,7 +264,6 @@ void TextFragmentSelectorGenerator::GenerateSelector(
generation_start_time_ = base::DefaultTickClock::GetInstance()->NowTicks();
pending_generate_selector_callback_ = std::move(callback);
state_ = kNeedsNewCandidate;
error_.reset();
step_ = kExact;
max_available_prefix_ = "";
max_available_suffix_ = "";
......@@ -349,9 +345,10 @@ void TextFragmentSelectorGenerator::DidFindMatch(
}
void TextFragmentSelectorGenerator::NoMatchFound() {
state_ = kFailure;
error_ = LinkGenerationError::kIncorrectSelector;
ResolveSelectorState();
UMA_HISTOGRAM_ENUMERATION("SharedHighlights.LinkGenerated.Error",
LinkGenerationError::kIncorrectSelector);
NotifySelectorReady(
TextFragmentSelector(TextFragmentSelector::SelectorType::kInvalid));
}
void TextFragmentSelectorGenerator::NotifySelectorReady(
......@@ -382,9 +379,6 @@ void TextFragmentSelectorGenerator::NotifySelectorReady(
UMA_HISTOGRAM_TIMES("SharedHighlights.LinkGenerated.Error.TimeToGenerate",
base::DefaultTickClock::GetInstance()->NowTicks() -
generation_start_time_);
shared_highlighting::LogLinkGenerationErrorReason(
error_.has_value() ? error_.value() : LinkGenerationError::kUnknown);
}
std::move(pending_generate_selector_callback_).Run(selector.ToString());
......@@ -418,8 +412,9 @@ void TextFragmentSelectorGenerator::GenerateExactSelector() {
String selected_text = PlainText(ephemeral_range).StripWhiteSpace();
if (selected_text.IsEmpty()) {
UMA_HISTOGRAM_ENUMERATION("SharedHighlights.LinkGenerated.Error",
LinkGenerationError::kEmptySelection);
state_ = kFailure;
error_ = LinkGenerationError::kEmptySelection;
return;
}
......@@ -471,8 +466,9 @@ void TextFragmentSelectorGenerator::ExtendRangeSelector() {
// If from middle till end of selection there is no word break, then we
// cannot use it for range end.
if (mid_point == selection_length) {
UMA_HISTOGRAM_ENUMERATION("SharedHighlights.LinkGenerated.Error",
LinkGenerationError::kNoRange);
state_ = kFailure;
error_ = LinkGenerationError::kNoRange;
return;
}
......@@ -512,8 +508,9 @@ void TextFragmentSelectorGenerator::ExtendContext() {
// Give up if context is already too long.
if (num_prefix_words_ == kMaxContextWords ||
num_prefix_words_ == kMaxContextWords) {
UMA_HISTOGRAM_ENUMERATION("SharedHighlights.LinkGenerated.Error",
LinkGenerationError::kContextLimitReached);
state_ = kFailure;
error_ = LinkGenerationError::kContextLimitReached;
return;
}
......@@ -525,8 +522,9 @@ void TextFragmentSelectorGenerator::ExtendContext() {
}
if (max_available_prefix_.IsEmpty() && max_available_suffix_.IsEmpty()) {
UMA_HISTOGRAM_ENUMERATION("SharedHighlights.LinkGenerated.Error",
LinkGenerationError::kNoContext);
state_ = kFailure;
error_ = LinkGenerationError::kNoContext;
return;
}
......@@ -535,8 +533,9 @@ void TextFragmentSelectorGenerator::ExtendContext() {
// Give up if we were unable to get new prefix and suffix.
if (prefix == selector_->Prefix() && suffix == selector_->Suffix()) {
UMA_HISTOGRAM_ENUMERATION("SharedHighlights.LinkGenerated.Error",
LinkGenerationError::kContextExhausted);
state_ = kFailure;
error_ = LinkGenerationError::kContextExhausted;
return;
}
selector_ = std::make_unique<TextFragmentSelector>(
......
......@@ -5,8 +5,6 @@
#ifndef THIRD_PARTY_BLINK_RENDERER_CORE_PAGE_SCROLLING_TEXT_FRAGMENT_SELECTOR_GENERATOR_H_
#define THIRD_PARTY_BLINK_RENDERER_CORE_PAGE_SCROLLING_TEXT_FRAGMENT_SELECTOR_GENERATOR_H_
#include "base/optional.h"
#include "components/shared_highlighting/core/common/shared_highlighting_metrics.h"
#include "third_party/blink/public/mojom/link_to_text/link_to_text.mojom-blink.h"
#include "third_party/blink/renderer/core/editing/forward.h"
#include "third_party/blink/renderer/core/page/scrolling/text_fragment_finder.h"
......@@ -33,6 +31,27 @@ class CORE_EXPORT TextFragmentSelectorGenerator final
public TextFragmentFinder::Client,
public blink::mojom::blink::TextFragmentSelectorProducer {
public:
// Update corresponding |LinkGenerationError| in enums.xml.
enum class LinkGenerationError {
kIncorrectSelector,
kNoRange,
kNoContext,
kContextExhausted,
kContextLimitReached,
kEmptySelection,
// Recorded from browser/java side when tab or its content becomes
// unavailable. Added here to keep in sync with the enums.xml values.
kTabHidden,
kOmniboxNavigation,
kTabCrash,
kUnknown,
kIFrame,
kMaxValue = kIFrame
};
explicit TextFragmentSelectorGenerator() = default;
void BindTextFragmentSelectorProducer(
......@@ -127,8 +146,6 @@ class CORE_EXPORT TextFragmentSelectorGenerator final
GenerationStep step_ = kExact;
SelectorState state_ = kNeedsNewCandidate;
base::Optional<shared_highlighting::LinkGenerationError> error_;
// Fields used for keeping track of context.
// Strings available for gradually forming prefix and suffix.
......
......@@ -717,7 +717,6 @@ _CONFIG = [
'paths': ['third_party/blink/renderer/core/page/scrolling'],
'allowed': [
'cc::ScrollbarLayerBase',
'shared_highlighting::.+',
],
},
{
......
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