Issue 8341: Add the default search engine metric. #4771

iefremov · 2020-02-27T17:24:40Z

brave/brave-browser#8341

Submitter Checklist:

Test Plan:

Reviewer Checklist:

New files have MPL-2.0 license header.
Request a security/privacy review as needed.
Adequate test coverage exists to prevent regressions
Verify test plan is specified in PR before merging to source

After-merge Checklist:

The associated issue milestone is set to the smallest version that the
changes has landed on.
All relevant documentation has been updated.

iefremov · 2020-03-03T17:07:34Z

@bridiver @simonhong ready for review

simonhong · 2020-03-04T08:50:48Z

browser/search_engines/search_engine_provider_service_factory.cc

@@ -47,7 +48,7 @@ KeyedService* InitializeSearchEngineProviderServiceIfNeeded(Profile* profile) {
    return new PrivateWindowSearchEngineProviderService(profile);
  }

-  return nullptr;
+  return new DefaultSearchEngineProviderService(profile);


Hmm, using |profile| could make confusing.
So far, SearchEngineProviderService and its subclasses are instantiated with otr profile because they are only used for otr profile. and all implementation assumes it's otr profile.
How about introducing new service instead of subclassing SearchEngineProviderService?

Yeah, that's a good point, I was thinking about it, but decided that observing a single data point doesn't worth creating a whole new service :) If you feel like writing a separate one is more clear, I will do that

actually, passing normal profile doesn't cause any side effect because you're only using original_template_url_service_. Or, how about adding some comments?

Hmm, I think this service factory is not a proper place to get default search engine metric because this service was intended only for handling default search engine of non-normal profile.
but you only want normal profile's default search engine changing.
If we don't have yet, I think general-purpose service seems fine for logging metric if there is no good point for. WDYT?

Or just using different service with this factory. is it strange?

I think I'll just go for separate service&factory to reduce confusion

simonhong · 2020-03-04T08:51:29Z

chromium_src/components/search_engines/template_url_prepopulate_data.cc

+  SearchEngineType type = GetEngineType_ChromiumImpl(url);
+  if (type == SEARCH_ENGINE_OTHER) {
+    for (const auto& entry : brave_engines_map) {
+      const auto& engine = entry.second;


This causes compile failure - maybe you want const auto*?

compiled successfully on my machine :-/

iefremov · 2020-03-05T08:26:41Z

@simonhong ptal

iefremov · 2020-03-05T08:27:35Z

@bridiver pls check the removed patch. We don't need it because:

It is not needed for anything
It spoils the existing chromium histogram values
without it I can reuse the enum for my P3A purposes

simonhong · 2020-03-05T08:33:16Z

browser/search_engines/search_engine_tracker.cc

+SearchEngineTrackerFactory::~SearchEngineTrackerFactory() {}
+
+KeyedService* SearchEngineTrackerFactory::BuildServiceInstanceFor(
+    content::BrowserContext* context) const {


How about return nullptr if |context| is non normal profile?
I think this only tracks for normal profile?

It already works like this, as per BrowserContextKeyedServiceFactory::GetBrowserContextToUse

Oh, it's default behavior! :)

simonhong · 2020-03-05T08:37:06Z

please add issue url in the description.

simonhong · 2020-03-05T08:40:40Z

browser/search_engines/search_engine_tracker.cc

+    const SearchTermsData& search_terms =
+        template_url_service_->search_terms_data();
+    const GURL& url = template_url->GenerateSearchURL(search_terms);
+    if (url != default_search_url_) {


Just out of curiosity, this callback is called in url == default_search_url_ case?

I think yes, because other details (e.g. shortcut or anything else) could be changed, and in this case the callback fires as well

iefremov · 2020-03-05T08:51:00Z

please add issue url in the description.

added, I hope this will not close it ...

simonhong

LGTM

bsclifton · 2020-03-19T21:36:24Z

@bridiver can you please review 😄 I'll try to check out soon too!

bridiver

patch change looks good

Issue 8341: Add the default search engine metric.

GeetaSarvadnya · 2020-03-25T07:28:08Z

Edit: Only Question 2 is implemented in this PR. @iefremov logged a follow-up issue for Question1 brave/brave-browser#8854

GeetaSarvadnya · 2020-03-25T08:31:20Z

Verification PASSED with Windows 10 x64 using

Brave	1.8.36 Chromium: 81.0.4044.69 (Official Build) nightly (64-bit)
Revision	6813546031a4bc83f717a2ef7cd4ac6ec1199132-refs/branch-heads/4044@{#776}
OS	Windows 10 OS Version 1803 (Build 17134.1006)

Verified the description from the issue P3A: Ask for search volume and default search engine brave-browser#8341 (comment)
Verified only question 2 - see Issue 8341: Add the default search engine metric. #4771 (comment)

Question 2 - metric Brave.Search.DefaultEngine

Clean profile:

Verified that default search engine response value is displayed as 1 for Google
Verified that search engine response value is displayed as 2 when DDG custom search engine is set in brave://settings/search
Verified that search engine response value is displayed as 3 when Startpage custom search engine is set in brave://settings/search
Verified that search engine response value is displayed as 4 when Bing custom search engine is set in brave://settings/search
Verified that search engine response value is displayed as 5 when Qwant custom search engine is set in brave://settings/search
Verified that default search engine response value is displayed as 1 when there is no custom search engine set in the welcome screen
Verified that search engine response value is displayed as 2 when DDG custom search engine is set in the welcome screen
Verified that search engine response value is displayed as 3 when Startpage custom search engine is set in the welcome screen
Verified that search engine response value is displayed as 4 when Bing custom search engine is set in the welcome screen
Verified that search engine response value is displayed as 5 when Qwant custom search engine is set in the welcome screen

Upgraded profile:

Install 1.6.42 and upgrade to 1.8.36 and ensure that default search engine response value is displayed as 1 for Google
Install 1.6.42 and set the default search engine to DDG and upgrade to 1.8.36 and ensured that the search engine response value is displayed as 2
Install 1.6.42 and set the default search engine to Startpage and upgrade to 1.8.36 and ensured that the search engine response value is displayed as 3
Install 1.6.42 and set the default search engine to Bing and upgrade to 1.8.36 and ensured that the search engine response value is displayed as 4
Install 1.6.42 and set the default search engine to Qwant and upgrade to 1.8.36 and ensured that the search engine response value is displayed as 5
Install 1.6.42 select default search as DDG in welcome screen and upgrade to 1.8.36 and ensure that default search engine response value is displayed as 2
Install 1.6.42 select default search as Startpage in welcome screen and upgrade to 1.8.36 and ensure that default search engine response value is displayed as 3
Install 1.6.42 select default search as Bing in welcome screen and upgrade to 1.8.36 and ensure that default search engine response value is displayed as 4
Install 1.6.42 select default search as Qwant in welcome screen and upgrade to 1.8.36 and ensure that default search engine response value is displayed as 5

iefremov requested a review from bridiver as a code owner February 27, 2020 17:24

iefremov mentioned this pull request Feb 27, 2020

P3A: Ask for search volume and default search engine brave/brave-browser#8341

Closed

iefremov changed the title ~~Issue 8341: Add the default search engine metric.~~ [WIP] Issue 8341: Add the default search engine metric. Feb 27, 2020

iefremov force-pushed the ie_search_p3a branch from 328ee50 to cdf07e5 Compare March 3, 2020 17:04

iefremov changed the title ~~[WIP] Issue 8341: Add the default search engine metric.~~ Issue 8341: Add the default search engine metric. Mar 3, 2020

iefremov requested review from simonhong and bsclifton March 3, 2020 17:06

simonhong reviewed Mar 4, 2020

View reviewed changes

iefremov force-pushed the ie_search_p3a branch 2 times, most recently from 37af5ca to 8e535ee Compare March 5, 2020 08:26

simonhong reviewed Mar 5, 2020

View reviewed changes

simonhong approved these changes Mar 5, 2020

View reviewed changes

iefremov self-assigned this Mar 17, 2020

bridiver approved these changes Mar 20, 2020

View reviewed changes

Ivan Efremov added 2 commits March 23, 2020 12:46

Issue 8341: Add the default search engine metric.

574115d

Issue 8341: Add missing browsertest.

5293ba2

iefremov force-pushed the ie_search_p3a branch from 5f791dd to 5293ba2 Compare March 23, 2020 05:46

iefremov merged commit 3ecf1ba into master Mar 23, 2020

iefremov deleted the ie_search_p3a branch March 23, 2020 10:04

bsclifton pushed a commit that referenced this pull request Mar 24, 2020

Merge pull request #4771 from brave/ie_search_p3a

fe3a893

Issue 8341: Add the default search engine metric.

bsclifton mentioned this pull request Mar 24, 2020

Issue 8341: Add the default search engine metric. (uplift to 1.7.x) #5051

Merged

4 tasks

kjozwiak added this to the 1.8.x - Nightly milestone Mar 25, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue 8341: Add the default search engine metric. #4771

Issue 8341: Add the default search engine metric. #4771

iefremov commented Feb 27, 2020 •

edited by simonhong

Loading

iefremov commented Mar 3, 2020

simonhong Mar 4, 2020 •

edited

Loading

iefremov Mar 4, 2020

simonhong Mar 4, 2020

simonhong Mar 4, 2020

simonhong Mar 4, 2020

iefremov Mar 4, 2020

iefremov Mar 5, 2020

simonhong Mar 4, 2020

iefremov Mar 4, 2020

iefremov Mar 5, 2020

iefremov commented Mar 5, 2020

iefremov commented Mar 5, 2020

simonhong Mar 5, 2020

iefremov Mar 5, 2020

simonhong Mar 5, 2020

simonhong commented Mar 5, 2020

simonhong Mar 5, 2020

iefremov Mar 5, 2020

iefremov commented Mar 5, 2020

simonhong left a comment

bsclifton commented Mar 19, 2020

bridiver left a comment

GeetaSarvadnya commented Mar 25, 2020 •

edited

Loading

GeetaSarvadnya commented Mar 25, 2020 •

edited

Loading

Issue 8341: Add the default search engine metric. #4771

Issue 8341: Add the default search engine metric. #4771

Conversation

iefremov commented Feb 27, 2020 • edited by simonhong Loading

Submitter Checklist:

Test Plan:

Reviewer Checklist:

After-merge Checklist:

iefremov commented Mar 3, 2020

simonhong Mar 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iefremov commented Mar 5, 2020

iefremov commented Mar 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonhong commented Mar 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iefremov commented Mar 5, 2020

simonhong left a comment

Choose a reason for hiding this comment

bsclifton commented Mar 19, 2020

bridiver left a comment

Choose a reason for hiding this comment

GeetaSarvadnya commented Mar 25, 2020 • edited Loading

GeetaSarvadnya commented Mar 25, 2020 • edited Loading

iefremov commented Feb 27, 2020 •

edited by simonhong

Loading

simonhong Mar 4, 2020 •

edited

Loading

GeetaSarvadnya commented Mar 25, 2020 •

edited

Loading

GeetaSarvadnya commented Mar 25, 2020 •

edited

Loading