Skip to content

Commit

Permalink
Fix metrics for when suggester is not allowed.
Browse files Browse the repository at this point in the history 
Currently the metrics depend on whether emoji suggester is not allowed
for both personal info and emoji suggester. This would create the wrong
metrics. Added Parameterised Testing so there is a total of 44 new
tests.

Other changes:
In order to unit test this for all AssistiveTypes related to
PersonalInfo, I had to allow the unit tests to set the personal data in
personal suggester. This was done by adding an optional constructor
argument to AssistiveSuggester.

Created a helper function to map AssistiveType -> Feature since the
feature is what is ultimately used to record some metrics.

Refactored some code in other functions that was doing a manual mapping
to use this new helper function.

Added tests for both "NotAlowed" and "Disabled" histogram entries.

Bug: b/228136553
Change-Id: Ifb0ae66154fe2cd65504ce5e52c9a624a834a0c8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3568930
Reviewed-by: Darren Shen <shend@chromium.org>
Reviewed-by: Curtis McMullan <curtismcmullan@chromium.org>
Commit-Queue: jhtin <jhtin@chromium.org>
Cr-Commit-Position: refs/heads/main@{#989322}
  • Loading branch information
jhtin authored and Chromium LUCI CQ committed Apr 6, 2022
1 parent 7b98ccf commit 5e6a60c
Show file tree
Hide file tree
Showing 4 changed files with 334 additions and 61 deletions.
85 changes: 64 additions & 21 deletions chrome/browser/ash/input_method/assistive_suggester.cc
View file
Expand Up @@ -20,6 +20,7 @@
#include "chrome/browser/ui/browser_finder.h"
#include "chrome/browser/ui/browser_window.h"
#include "chrome/common/pref_names.h"
#include "components/autofill/core/browser/personal_data_manager.h"
#include "components/exo/wm_helper.h"
#include "components/prefs/pref_service.h"
#include "components/prefs/scoped_user_pref_update.h"
Expand Down Expand Up @@ -167,9 +168,12 @@ void RecordMultiWordTextInputState(PrefService* pref_service,
AssistiveSuggester::AssistiveSuggester(
SuggestionHandlerInterface* suggestion_handler,
Profile* profile,
std::unique_ptr<AssistiveSuggesterSwitch> suggester_switch)
std::unique_ptr<AssistiveSuggesterSwitch> suggester_switch,
autofill::PersonalDataManager* personal_data_manager_for_testing)
: profile_(profile),
personal_info_suggester_(suggestion_handler, profile),
personal_info_suggester_(suggestion_handler,
profile,
personal_data_manager_for_testing),
emoji_suggester_(suggestion_handler, profile),
multi_word_suggester_(suggestion_handler, profile),
suggester_switch_(std::move(suggester_switch)) {
Expand Down Expand Up @@ -272,26 +276,66 @@ DisabledReason AssistiveSuggester::GetDisabledReasonForMultiWord(
return DisabledReason::kNone;
}

bool AssistiveSuggester::IsActionEnabled(AssistiveType action) {
switch (action) {
AssistiveSuggester::AssistiveFeature
AssistiveSuggester::GetAssistiveFeatureForType(AssistiveType type) {
switch (type) {
case AssistiveType::kPersonalEmail:
case AssistiveType::kPersonalAddress:
case AssistiveType::kPersonalPhoneNumber:
case AssistiveType::kPersonalName:
case AssistiveType::kPersonalNumber:
case AssistiveType::kPersonalFirstName:
case AssistiveType::kPersonalLastName:
// TODO: Use value from settings when crbug/1068457 is done.
return IsAssistPersonalInfoEnabled();
return AssistiveFeature::kPersonalInfoSuggestion;
case AssistiveType::kEmoji:
return IsEmojiSuggestAdditionEnabled();
return AssistiveFeature::kEmojiSuggestion;
case AssistiveType::kMultiWordCompletion:
case AssistiveType::kMultiWordPrediction:
return AssistiveFeature::kMultiWordSuggestion;
default:
// We should only handle Personal Info, Emoji, and Multiword related
// assistive types.
//
// Any assistive types outside of this should not be processed in this
// class, hence we shall DCHECK here if that ever occurs.
LOG(DFATAL) << "Unexpected AssistiveType value: "
<< static_cast<int>(type);
return AssistiveFeature::kUnknown;
}
}

bool AssistiveSuggester::IsAssistiveTypeEnabled(AssistiveType type) {
switch (GetAssistiveFeatureForType(type)) {
case AssistiveFeature::kPersonalInfoSuggestion:
// TODO: Use value from settings when crbug/1068457 is done.
return IsAssistPersonalInfoEnabled();
case AssistiveFeature::kEmojiSuggestion:
return IsEmojiSuggestAdditionEnabled();
case AssistiveFeature::kMultiWordSuggestion:
return IsMultiWordSuggestEnabled();
default:
break;
LOG(DFATAL) << "Unexpected AssistiveType value: "
<< static_cast<int>(type);
return false;
}
}

bool AssistiveSuggester::IsAssistiveTypeAllowedInBrowserContext(
AssistiveType type) {
// TODO(b/222218270): Replace all legacy suggester_switch->IsXAllowed() calls
// with GetEnabledSuggestions.
switch (GetAssistiveFeatureForType(type)) {
case AssistiveFeature::kPersonalInfoSuggestion:
return suggester_switch_->IsPersonalInfoSuggestionAllowed();
case AssistiveFeature::kEmojiSuggestion:
return suggester_switch_->IsEmojiSuggestionAllowed();
case AssistiveFeature::kMultiWordSuggestion:
return suggester_switch_->IsMultiWordSuggestionAllowed();
default:
LOG(DFATAL) << "Unexpected AssistiveType value: "
<< static_cast<int>(type);
return false;
}
return false;
}

void AssistiveSuggester::OnFocus(int context_id) {
Expand Down Expand Up @@ -380,13 +424,13 @@ void AssistiveSuggester::RecordTextInputStateMetrics() {
}
}

void AssistiveSuggester::RecordAssistiveMatchMetricsForAction(
AssistiveType action) {
RecordAssistiveMatch(action);
if (!IsActionEnabled(action)) {
RecordAssistiveDisabled(action);
} else if (!suggester_switch_->IsEmojiSuggestionAllowed()) {
RecordAssistiveNotAllowed(action);
void AssistiveSuggester::RecordAssistiveMatchMetricsForAssistiveType(
AssistiveType type) {
RecordAssistiveMatch(type);
if (!IsAssistiveTypeEnabled(type)) {
RecordAssistiveDisabled(type);
} else if (!IsAssistiveTypeAllowedInBrowserContext(type)) {
RecordAssistiveNotAllowed(type);
}
}

Expand All @@ -400,15 +444,14 @@ void AssistiveSuggester::RecordAssistiveMatchMetrics(const std::u16string& text,
std::u16string text_before_cursor =
text.substr(start_pos, cursor_pos - start_pos);
// Personal info suggestion match
AssistiveType action =
ProposePersonalInfoAssistiveAction(text_before_cursor);
if (action != AssistiveType::kGenericAction) {
RecordAssistiveMatchMetricsForAction(action);
AssistiveType type = ProposePersonalInfoAssistiveAction(text_before_cursor);
if (type != AssistiveType::kGenericAction) {
RecordAssistiveMatchMetricsForAssistiveType(type);
RecordAssistiveDisabledReasonForPersonalInfo(
GetDisabledReasonForPersonalInfo());
// Emoji suggestion match
} else if (emoji_suggester_.ShouldShowSuggestion(text_before_cursor)) {
RecordAssistiveMatchMetricsForAction(AssistiveType::kEmoji);
RecordAssistiveMatchMetricsForAssistiveType(AssistiveType::kEmoji);
base::RecordAction(
base::UserMetricsAction("InputMethod.Assistive.EmojiSuggested"));
RecordAssistiveDisabledReasonForEmoji(GetDisabledReasonForEmoji());
Expand Down
27 changes: 19 additions & 8 deletions chrome/browser/ash/input_method/assistive_suggester.h
View file
Expand Up @@ -19,6 +19,7 @@
#include "chrome/browser/ash/input_method/suggestion_enums.h"
#include "chrome/browser/ash/input_method/suggestion_handler_interface.h"
#include "chrome/browser/ash/input_method/suggestions_source.h"
#include "components/autofill/core/browser/personal_data_manager.h"

namespace ash {
namespace input_method {
Expand All @@ -27,16 +28,21 @@ namespace input_method {
// dismiss the suggestion according to the user action.
class AssistiveSuggester : public SuggestionsSource {
public:
enum AssistiveFeature {
// Features handled by assistive suggester.
enum class AssistiveFeature {
kUnknown, // Includes features not handled by assistive suggester.
kEmojiSuggestion,
kMultiWordSuggestion,
kPersonalInfoSuggestion,
};

AssistiveSuggester(
SuggestionHandlerInterface* suggestion_handler,
Profile* profile,
std::unique_ptr<AssistiveSuggesterSwitch> suggester_switch);
// personal_data_manager is only used for testing to override the default
// autofill data for PersonalInfoSuggester.
AssistiveSuggester(SuggestionHandlerInterface* suggestion_handler,
Profile* profile,
std::unique_ptr<AssistiveSuggesterSwitch> suggester_switch,
autofill::PersonalDataManager*
personal_data_manager_for_testing = nullptr);

~AssistiveSuggester() override;

Expand Down Expand Up @@ -73,7 +79,8 @@ class AssistiveSuggester : public SuggestionsSource {
void OnExternalSuggestionsUpdated(
const std::vector<ime::TextSuggestion>& suggestions);

// Accepts the suggestion at a given index if a suggester is currently active.
// Accepts the suggestion at a given index if a suggester is currently
// active.
void AcceptSuggestion(size_t index);

// Check if suggestion is being shown.
Expand Down Expand Up @@ -117,7 +124,7 @@ class AssistiveSuggester : public SuggestionsSource {
int cursor_pos,
int anchor_pos);

void RecordAssistiveMatchMetricsForAction(AssistiveType action);
void RecordAssistiveMatchMetricsForAssistiveType(AssistiveType type);

// Only the first applicable reason in DisabledReason enum is returned.
DisabledReason GetDisabledReasonForEmoji();
Expand All @@ -129,7 +136,11 @@ class AssistiveSuggester : public SuggestionsSource {
DisabledReason GetDisabledReasonForMultiWord(
const AssistiveSuggesterSwitch::EnabledSuggestions& enabled_suggestions);

bool IsActionEnabled(AssistiveType action);
AssistiveFeature GetAssistiveFeatureForType(AssistiveType type);

bool IsAssistiveTypeEnabled(AssistiveType type);

bool IsAssistiveTypeAllowedInBrowserContext(AssistiveType type);

bool WithinGrammarFragment(int cursor_pos, int anchor_pos);

Expand Down

0 comments on commit 5e6a60c

Please sign in to comment.