Pull g_browser_process out of PopularSites.

Pull g_browser_process out of PopularSites.

BUG=603026
Committed: https://crrev.com/9a20152b17517d19aaa4086cc1c72c4ace1e49e0
Cr-Commit-Position: refs/heads/master@{#390913}

[sfiera, 2016-04-29 14:23:48]

sfiera@chromium.org changed reviewers:
+ bauerb@chromium.org

[Bernhard Bauer, 2016-04-29 14:26:00]

Code LGTM, but description nit: Pull out g_browser_process of where?

[Bernhard Bauer, 2016-04-29 14:35:46]

https://codereview.chromium.org/1930413002/diff/20001/chrome/browser/android/...
File chrome/browser/android/ntp/most_visited_sites.h (right):

https://codereview.chromium.org/1930413002/diff/20001/chrome/browser/android/...
chrome/browser/android/ntp/most_visited_sites.h:90:
variations::VariationsService* variations_service);
Oops, forgot: This doesn't need to be explicit anymore now.

[sfiera, 2016-04-29 14:43:01]

Description was changed from

==========
Pull out g_browser_process.

BUG=603026
==========

to

==========
Pull g_browser_process out of PopularSites.

BUG=603026
==========

[sfiera, 2016-04-29 14:43:45]

The CQ bit was checked by sfiera@chromium.org

[commit-bot: I haz the power, 2016-04-29 14:44:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1930413002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1930413002/20001

[sfiera, 2016-04-29 14:44:06]

The CQ bit was unchecked by sfiera@chromium.org

[sfiera, 2016-04-29 14:45:34]

The CQ bit was checked by sfiera@chromium.org

[sfiera, 2016-04-29 14:45:34]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org
Link to the patchset: https://codereview.chromium.org/1930413002/#ps40001
(title: " ")

[commit-bot: I haz the power, 2016-04-29 14:45:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1930413002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1930413002/40001

[Marc Treib, 2016-04-29 14:46:33]

treib@chromium.org changed reviewers:
+ treib@chromium.org

[Marc Treib, 2016-04-29 14:46:34]

https://codereview.chromium.org/1930413002/diff/20001/chrome/browser/android/...
File chrome/browser/android/ntp/most_visited_sites.h (left):

https://codereview.chromium.org/1930413002/diff/20001/chrome/browser/android/...
chrome/browser/android/ntp/most_visited_sites.h:37: class Profile;
Profile one is actually still required for now.

https://codereview.chromium.org/1930413002/diff/20001/chrome/browser/android/...
File chrome/browser/android/ntp/popular_sites.cc (right):

https://codereview.chromium.org/1930413002/diff/20001/chrome/browser/android/...
chrome/browser/android/ntp/popular_sites.cc:91: // Get the country that the
experiment is running under
Not really accurate - it's just the country that the VariationsService thinks
it's in.

[Marc Treib, 2016-04-29 14:47:10]

On 2016/04/29 14:46:34, Marc Treib wrote:
>
https://codereview.chromium.org/1930413002/diff/20001/chrome/browser/android/...
> File chrome/browser/android/ntp/most_visited_sites.h (left):
> 
>
https://codereview.chromium.org/1930413002/diff/20001/chrome/browser/android/...
> chrome/browser/android/ntp/most_visited_sites.h:37: class Profile;
> Profile one is actually still required for now.
> 
>
https://codereview.chromium.org/1930413002/diff/20001/chrome/browser/android/...
> File chrome/browser/android/ntp/popular_sites.cc (right):
> 
>
https://codereview.chromium.org/1930413002/diff/20001/chrome/browser/android/...
> chrome/browser/android/ntp/popular_sites.cc:91: // Get the country that the
> experiment is running under
> Not really accurate - it's just the country that the VariationsService thinks
> it's in.

(No need to abort the commit for these)

[sfiera, 2016-04-29 14:50:55]

https://codereview.chromium.org/1930413002/diff/20001/chrome/browser/android/...
File chrome/browser/android/ntp/most_visited_sites.h (left):

https://codereview.chromium.org/1930413002/diff/20001/chrome/browser/android/...
chrome/browser/android/ntp/most_visited_sites.h:37: class Profile;
On 2016/04/29 14:46:34, Marc Treib wrote:
> Profile one is actually still required for now.

Oh yeah, I guess we must be picking it up from SUS.

In any case, CL is already out to you that will actually get rid of Profile from
this file.

https://codereview.chromium.org/1930413002/diff/20001/chrome/browser/android/...
File chrome/browser/android/ntp/most_visited_sites.h (right):

https://codereview.chromium.org/1930413002/diff/20001/chrome/browser/android/...
chrome/browser/android/ntp/most_visited_sites.h:90:
variations::VariationsService* variations_service);
On 2016/04/29 14:35:45, Bernhard Bauer wrote:
> Oops, forgot: This doesn't need to be explicit anymore now.

Done.

[Alexei Svitkine (slow), 2016-04-29 14:56:08]

asvitkine@chromium.org changed reviewers:
+ asvitkine@chromium.org

[Alexei Svitkine (slow), 2016-04-29 14:56:09]

https://codereview.chromium.org/1930413002/diff/40001/chrome/browser/android/...
File chrome/browser/android/ntp/popular_sites.h (right):

https://codereview.chromium.org/1930413002/diff/40001/chrome/browser/android/...
chrome/browser/android/ntp/popular_sites.h:66: variations::VariationsService*
variations_service,
Have you considered just plumbing the country code from variations service
instead of the full service?

[Marc Treib, 2016-04-29 14:59:19]

https://codereview.chromium.org/1930413002/diff/40001/chrome/browser/android/...
File chrome/browser/android/ntp/popular_sites.h (right):

https://codereview.chromium.org/1930413002/diff/40001/chrome/browser/android/...
chrome/browser/android/ntp/popular_sites.h:66: variations::VariationsService*
variations_service,
On 2016/04/29 14:56:09, Alexei Svitkine wrote:
> Have you considered just plumbing the country code from variations service
> instead of the full service?

Can the country code ever change during runtime? If not, then this seems like a
good idea.

[Alexei Svitkine (slow), 2016-04-29 15:03:02]

https://codereview.chromium.org/1930413002/diff/40001/chrome/browser/android/...
File chrome/browser/android/ntp/popular_sites.h (right):

https://codereview.chromium.org/1930413002/diff/40001/chrome/browser/android/...
chrome/browser/android/ntp/popular_sites.h:66: variations::VariationsService*
variations_service,
On 2016/04/29 14:59:19, Marc Treib wrote:
> On 2016/04/29 14:56:09, Alexei Svitkine wrote:
> > Have you considered just plumbing the country code from variations service
> > instead of the full service?
> 
> Can the country code ever change during runtime? If not, then this seems like
a
> good idea.

It can change when a new variations seed is fetched *if* it's initially empty.
But otherwise, it gets cached for the current Chrome version, so since the
version doesn't change during runtime, it shouldn't change.

[sfiera, 2016-04-29 15:10:42]

https://codereview.chromium.org/1930413002/diff/40001/chrome/browser/android/...
File chrome/browser/android/ntp/popular_sites.h (right):

https://codereview.chromium.org/1930413002/diff/40001/chrome/browser/android/...
chrome/browser/android/ntp/popular_sites.h:66: variations::VariationsService*
variations_service,
On 2016/04/29 14:59:19, Marc Treib wrote:
> On 2016/04/29 14:56:09, Alexei Svitkine wrote:
> > Have you considered just plumbing the country code from variations service
> > instead of the full service?
> 
> Can the country code ever change during runtime? If not, then this seems like
a
> good idea.

I wouldn't really like to have variations_country and override_country as
separate string arguments to the constructor.

I think my favored option would be to have a separate function
"GetPopularSitesCountryCode(profile)", so that we can take a single country code
and use it unconditionally. That function can stay outside components and depend
on chrome to make it convenient.

[Marc Treib, 2016-04-29 15:15:01]

https://codereview.chromium.org/1930413002/diff/40001/chrome/browser/android/...
File chrome/browser/android/ntp/popular_sites.h (right):

https://codereview.chromium.org/1930413002/diff/40001/chrome/browser/android/...
chrome/browser/android/ntp/popular_sites.h:66: variations::VariationsService*
variations_service,
On 2016/04/29 15:10:42, sfiera wrote:
> On 2016/04/29 14:59:19, Marc Treib wrote:
> > On 2016/04/29 14:56:09, Alexei Svitkine wrote:
> > > Have you considered just plumbing the country code from variations service
> > > instead of the full service?
> > 
> > Can the country code ever change during runtime? If not, then this seems
like
> a
> > good idea.
> 
> I wouldn't really like to have variations_country and override_country as
> separate string arguments to the constructor.
> 
> I think my favored option would be to have a separate function
> "GetPopularSitesCountryCode(profile)", so that we can take a single country
code
> and use it unconditionally. That function can stay outside components and
depend
> on chrome to make it convenient.

SGTM.
If we ever want to support iOS, we'll need two implementations of that function,
but that's probably unavoidable anyway.

[commit-bot: I haz the power, 2016-04-29 15:31:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-29 15:31:52]

Failed to apply patch for chrome/browser/android/ntp/most_visited_sites.cc:
While running git apply --index -3 -p1;
  error: patch failed: chrome/browser/android/ntp/most_visited_sites.cc:177
  Falling back to three-way merge...
  Applied patch to 'chrome/browser/android/ntp/most_visited_sites.cc' with
conflicts.
  U chrome/browser/android/ntp/most_visited_sites.cc

Patch:       chrome/browser/android/ntp/most_visited_sites.cc
Index: chrome/browser/android/ntp/most_visited_sites.cc
diff --git a/chrome/browser/android/ntp/most_visited_sites.cc
b/chrome/browser/android/ntp/most_visited_sites.cc
index
53c83aff0c94d778e2371e809faeac154bed3612..fb350c950ca7a55b99df42d773b55127b38e2e24
100644
--- a/chrome/browser/android/ntp/most_visited_sites.cc
+++ b/chrome/browser/android/ntp/most_visited_sites.cc
@@ -177,12 +177,15 @@ MostVisitedSites::Suggestion::Suggestion(Suggestion&&) =
default;
 MostVisitedSites::Suggestion&
 MostVisitedSites::Suggestion::operator=(Suggestion&&) = default;
 
-MostVisitedSites::MostVisitedSites(Profile* profile)
-    : profile_(profile), top_sites_(TopSitesFactory::GetForProfile(profile)),
+MostVisitedSites::MostVisitedSites(Profile* profile,
+    variations::VariationsService* variations_service)
+    : profile_(profile),
+      variations_service_(variations_service),
+      top_sites_(TopSitesFactory::GetForProfile(profile)),
       suggestions_service_(SuggestionsServiceFactory::GetForProfile(profile_)),
-      observer_(nullptr), num_sites_(0),
-      received_most_visited_sites_(false), received_popular_sites_(false),
-      recorded_uma_(false), scoped_observer_(this), weak_ptr_factory_(this) {
+      observer_(nullptr), num_sites_(0), received_most_visited_sites_(false),
+      received_popular_sites_(false), recorded_uma_(false),
+      scoped_observer_(this), weak_ptr_factory_(this) {
   // Register the thumbnails debugging page.
   content::URLDataSource::Add(profile_, new ThumbnailListSource(profile_));
 
@@ -207,6 +210,7 @@ void MostVisitedSites::SetMostVisitedURLsObserver(
     popular_sites_.reset(new PopularSites(
         profile_->GetPrefs(),
         TemplateURLServiceFactory::GetForProfile(profile_),
+        variations_service_,
         profile_->GetRequestContext(),
         GetPopularSitesCountry(),
         GetPopularSitesVersion(),

[Marc Treib, 2016-04-29 15:35:30]

On 2016/04/29 15:31:52, commit-bot: I haz the power wrote:
> Failed to apply patch for chrome/browser/android/ntp/most_visited_sites.cc:
> While running git apply --index -3 -p1;
>   error: patch failed: chrome/browser/android/ntp/most_visited_sites.cc:177
>   Falling back to three-way merge...
>   Applied patch to 'chrome/browser/android/ntp/most_visited_sites.cc' with
> conflicts.
>   U chrome/browser/android/ntp/most_visited_sites.cc
> 
> Patch:       chrome/browser/android/ntp/most_visited_sites.cc
> Index: chrome/browser/android/ntp/most_visited_sites.cc
> diff --git a/chrome/browser/android/ntp/most_visited_sites.cc
> b/chrome/browser/android/ntp/most_visited_sites.cc
> index
>
53c83aff0c94d778e2371e809faeac154bed3612..fb350c950ca7a55b99df42d773b55127b38e2e24
> 100644
> --- a/chrome/browser/android/ntp/most_visited_sites.cc
> +++ b/chrome/browser/android/ntp/most_visited_sites.cc
> @@ -177,12 +177,15 @@ MostVisitedSites::Suggestion::Suggestion(Suggestion&&) =
> default;
>  MostVisitedSites::Suggestion&
>  MostVisitedSites::Suggestion::operator=(Suggestion&&) = default;
>  
> -MostVisitedSites::MostVisitedSites(Profile* profile)
> -    : profile_(profile), top_sites_(TopSitesFactory::GetForProfile(profile)),
> +MostVisitedSites::MostVisitedSites(Profile* profile,
> +    variations::VariationsService* variations_service)
> +    : profile_(profile),
> +      variations_service_(variations_service),
> +      top_sites_(TopSitesFactory::GetForProfile(profile)),
>       
suggestions_service_(SuggestionsServiceFactory::GetForProfile(profile_)),
> -      observer_(nullptr), num_sites_(0),
> -      received_most_visited_sites_(false), received_popular_sites_(false),
> -      recorded_uma_(false), scoped_observer_(this), weak_ptr_factory_(this) {
> +      observer_(nullptr), num_sites_(0), received_most_visited_sites_(false),
> +      received_popular_sites_(false), recorded_uma_(false),
> +      scoped_observer_(this), weak_ptr_factory_(this) {
>    // Register the thumbnails debugging page.
>    content::URLDataSource::Add(profile_, new ThumbnailListSource(profile_));
>  
> @@ -207,6 +210,7 @@ void MostVisitedSites::SetMostVisitedURLsObserver(
>      popular_sites_.reset(new PopularSites(
>          profile_->GetPrefs(),
>          TemplateURLServiceFactory::GetForProfile(profile_),
> +        variations_service_,
>          profile_->GetRequestContext(),
>          GetPopularSitesCountry(),
>          GetPopularSitesVersion(),

Haha, my change (https://codereview.chromium.org/1934583002/) landed first, so
you'll have to rebase ;P

[sfiera, 2016-05-02 10:12:29]

The CQ bit was checked by sfiera@chromium.org

[sfiera, 2016-05-02 10:12:29]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org
Link to the patchset: https://codereview.chromium.org/1930413002/#ps60001
(title: " ")

[commit-bot: I haz the power, 2016-05-02 10:12:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1930413002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1930413002/60001

[commit-bot: I haz the power, 2016-05-02 10:48:07]

Description was changed from

==========
Pull g_browser_process out of PopularSites.

BUG=603026
==========

to

==========
Pull g_browser_process out of PopularSites.

BUG=603026
==========

[commit-bot: I haz the power, 2016-05-02 10:48:08]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-05-02 10:49:31]

Description was changed from

==========
Pull g_browser_process out of PopularSites.

BUG=603026
==========

to

==========
Pull g_browser_process out of PopularSites.

BUG=603026

Committed: https://crrev.com/9a20152b17517d19aaa4086cc1c72c4ace1e49e0
Cr-Commit-Position: refs/heads/master@{#390913}
==========

[commit-bot: I haz the power, 2016-05-02 10:49:32]

Patchset 4 (id:??) landed as
https://crrev.com/9a20152b17517d19aaa4086cc1c72c4ace1e49e0
Cr-Commit-Position: refs/heads/master@{#390913}