[NTP Popular Sites] Store country/version in prefs

[NTP Popular Sites] Store country/version in prefs
...and show on chrome://popular-sites-internals

BUG=599844
Committed: https://crrev.com/f38cc25bf2d6fa2407d9b3fc7cbdc16836b5ee12
Cr-Commit-Position: refs/heads/master@{#385753}

[Marc Treib, 2016-04-01 15:07:10]

The CQ bit was checked by treib@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-01 15:07:30]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1851803002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1851803002/1

[Marc Treib, 2016-04-01 15:10:53]

treib@chromium.org changed reviewers:
+ battre@chromium.org, bauerb@chromium.org

[Marc Treib, 2016-04-01 15:10:53]

battre@chromium.org: Please review changes in c/b/net.

bauerb@chromium.org: Please review changes in everything else :)

[battre, 2016-04-01 15:17:33]

c/b/n LGTM

[Bernhard Bauer, 2016-04-01 15:45:11]

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

https://codereview.chromium.org/1851803002/diff/1/chrome/browser/android/ntp/...
chrome/browser/android/ntp/popular_sites.cc:258: if
(FileDownloader::IsSuccess(result)) {
I might be more straight-forward to switch on the result value instead of having
this check and another one in line 261.

[commit-bot: I haz the power, 2016-04-01 15:48:34]

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

[commit-bot: I haz the power, 2016-04-01 15:48:35]

Dry run: This issue passed the CQ dry run.

[Bernhard Bauer, 2016-04-01 15:49:47]

Some more comments:

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

https://codereview.chromium.org/1851803002/diff/1/chrome/browser/android/ntp/...
chrome/browser/android/ntp/popular_sites.cc:266: country_ =
prefs->GetString(kPopularSitesCountryPref);
Why do we have to do this?

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

https://codereview.chromium.org/1851803002/diff/1/chrome/browser/android/ntp/...
chrome/browser/android/ntp/popular_sites.h:75: // well-defined values after
FinishedCallback has been called!
That also applies to sites(), right?

https://codereview.chromium.org/1851803002/diff/1/chrome/browser/net/file_dow...
File chrome/browser/net/file_downloader.h (right):

https://codereview.chromium.org/1851803002/diff/1/chrome/browser/net/file_dow...
chrome/browser/net/file_downloader.h:28: enum Result { DOWNLOADED, EXISTS,
FAILED };
Add comments what these mean?

[Marc Treib, 2016-04-01 16:33:28]

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

https://codereview.chromium.org/1851803002/diff/1/chrome/browser/android/ntp/...
chrome/browser/android/ntp/popular_sites.cc:258: if
(FileDownloader::IsSuccess(result)) {
On 2016/04/01 15:45:11, Bernhard Bauer wrote:
> I might be more straight-forward to switch on the result value instead of
having
> this check and another one in line 261.

Eh... sure. Done.

https://codereview.chromium.org/1851803002/diff/1/chrome/browser/android/ntp/...
chrome/browser/android/ntp/popular_sites.cc:266: country_ =
prefs->GetString(kPopularSitesCountryPref);
On 2016/04/01 15:49:47, Bernhard Bauer wrote:
> Why do we have to do this?

Added a comment: If we loaded an existing file, then its country/version might
not match what |country_| and |version_| say, so load the correct values.
(In that case, we should probably have re-downloaded, see the TODO I added
elsewhere. But I'm leaving that for another CL.)

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

https://codereview.chromium.org/1851803002/diff/1/chrome/browser/android/ntp/...
chrome/browser/android/ntp/popular_sites.h:75: // well-defined values after
FinishedCallback has been called!
On 2016/04/01 15:49:47, Bernhard Bauer wrote:
> That also applies to sites(), right?

sites() will just be empty. These might actually have inaccurate values.

https://codereview.chromium.org/1851803002/diff/1/chrome/browser/net/file_dow...
File chrome/browser/net/file_downloader.h (right):

https://codereview.chromium.org/1851803002/diff/1/chrome/browser/net/file_dow...
chrome/browser/net/file_downloader.h:28: enum Result { DOWNLOADED, EXISTS,
FAILED };
On 2016/04/01 15:49:47, Bernhard Bauer wrote:
> Add comments what these mean?

Done.

[Bernhard Bauer, 2016-04-01 17:40:48]

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

https://codereview.chromium.org/1851803002/diff/1/chrome/browser/android/ntp/...
chrome/browser/android/ntp/popular_sites.cc:266: country_ =
prefs->GetString(kPopularSitesCountryPref);
On 2016/04/01 16:33:28, Marc Treib wrote:
> On 2016/04/01 15:49:47, Bernhard Bauer wrote:
> > Why do we have to do this?
> 
> Added a comment: If we loaded an existing file, then its country/version might
> not match what |country_| and |version_| say, so load the correct values.
> (In that case, we should probably have re-downloaded, see the TODO I added
> elsewhere. But I'm leaving that for another CL.)

Oof. So, |country_| and |version_| contain the values for the last downloaded
file, unless we're currently trying to download a new one, in which case they
contain the new ones? I'm starting to see what you mean with the other comment,
but this just seems confusing.

Thinking about it, don't we have the correct values in prefs anyway? Could we
have the accessors return those values, rename the members to
|pending_country_|/|pending_version_| and clear them when we're not trying to
download a file? Or am I missing something?

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

https://codereview.chromium.org/1851803002/diff/20001/chrome/browser/android/...
chrome/browser/android/ntp/popular_sites.cc:1: // Copyright 2015 The Chromium
Authors. All rights reserved.
Is there a reason you're updating the copyright to last year? 😃

[Marc Treib, 2016-04-06 11:39:39]

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

https://codereview.chromium.org/1851803002/diff/1/chrome/browser/android/ntp/...
chrome/browser/android/ntp/popular_sites.cc:266: country_ =
prefs->GetString(kPopularSitesCountryPref);
On 2016/04/01 17:40:48, Bernhard Bauer wrote:
> On 2016/04/01 16:33:28, Marc Treib wrote:
> > On 2016/04/01 15:49:47, Bernhard Bauer wrote:
> > > Why do we have to do this?
> > 
> > Added a comment: If we loaded an existing file, then its country/version
might
> > not match what |country_| and |version_| say, so load the correct values.
> > (In that case, we should probably have re-downloaded, see the TODO I added
> > elsewhere. But I'm leaving that for another CL.)
> 
> Oof. So, |country_| and |version_| contain the values for the last downloaded
> file, unless we're currently trying to download a new one, in which case they
> contain the new ones? I'm starting to see what you mean with the other
comment,
> but this just seems confusing.
> 
> Thinking about it, don't we have the correct values in prefs anyway? Could we
> have the accessors return those values, rename the members to
> |pending_country_|/|pending_version_| and clear them when we're not trying to
> download a file? Or am I missing something?

Done.

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

https://codereview.chromium.org/1851803002/diff/20001/chrome/browser/android/...
chrome/browser/android/ntp/popular_sites.cc:1: // Copyright 2015 The Chromium
Authors. All rights reserved.
On 2016/04/01 17:40:48, Bernhard Bauer wrote:
> Is there a reason you're updating the copyright to last year? 😃

Yup: The file was created in 2015 :)  The "2013" must have been a bad
copy&paste. The corresponding header already says "2015".

[Bernhard Bauer, 2016-04-06 15:41:55]

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

https://codereview.chromium.org/1851803002/diff/20001/chrome/browser/android/...
chrome/browser/android/ntp/popular_sites.cc:1: // Copyright 2015 The Chromium
Authors. All rights reserved.
On 2016/04/06 11:39:38, Marc Treib wrote:
> On 2016/04/01 17:40:48, Bernhard Bauer wrote:
> > Is there a reason you're updating the copyright to last year? 😃
> 
> Yup: The file was created in 2015 :)  The "2013" must have been a bad
> copy&paste. The corresponding header already says "2015".

FWIW, the response I have received in the past from The Powers That Be about
changes like this was a quite literal "who cares". We don't update copyrights
that e.g. use the old style (with a "(c)") either.

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

https://codereview.chromium.org/1851803002/diff/40001/chrome/browser/android/...
chrome/browser/android/ntp/popular_sites.h:75: std::string country() const;
Nit: Make these GetCountry() and GetVersion(), as they are somewhat more
complicated than just a plain accessor?

https://codereview.chromium.org/1851803002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/popular_sites_internals.html (right):

https://codereview.chromium.org/1851803002/diff/40001/chrome/browser/resource...
chrome/browser/resources/popular_sites_internals.html:56: <td class="value"
id="country"></td>
Is there a reason we can't just use jstemplate to set this value?

[Marc Treib, 2016-04-06 16:37:11]

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

https://codereview.chromium.org/1851803002/diff/40001/chrome/browser/android/...
chrome/browser/android/ntp/popular_sites.h:75: std::string country() const;
On 2016/04/06 15:41:54, Bernhard Bauer wrote:
> Nit: Make these GetCountry() and GetVersion(), as they are somewhat more
> complicated than just a plain accessor?

Done. (I had to rename the global GetVersion (to GetVersionToUse) to avoid a
name collision.)

https://codereview.chromium.org/1851803002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/popular_sites_internals.html (right):

https://codereview.chromium.org/1851803002/diff/40001/chrome/browser/resource...
chrome/browser/resources/popular_sites_internals.html:56: <td class="value"
id="country"></td>
On 2016/04/06 15:41:54, Bernhard Bauer wrote:
> Is there a reason we can't just use jstemplate to set this value?

Err.. ignorance? :)
Done.

[Marc Treib, 2016-04-07 12:05:46]

Ping! :)

[Bernhard Bauer, 2016-04-07 12:24:29]

Sorry! I had to leave yesterday before I could get to this, and then I forgot
about it this morning.

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

https://codereview.chromium.org/1851803002/diff/60001/chrome/browser/android/...
chrome/browser/android/ntp/popular_sites.cc:118: GURL GetPopularSitesURL(const
std::string& country,
This now always gets called with |pending_country_| and |pending_version_| --
would it make sense to make it a member function?

https://codereview.chromium.org/1851803002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/popular_sites_internals.html (right):

https://codereview.chromium.org/1851803002/diff/60001/chrome/browser/resource...
chrome/browser/resources/popular_sites_internals.html:51: <div id="info"
class="section">
This ID is already in use.

https://codereview.chromium.org/1851803002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/popular_sites_internals.js (right):

https://codereview.chromium.org/1851803002/diff/60001/chrome/browser/resource...
chrome/browser/resources/popular_sites_internals.js:36: jstProcess(context,
$('info'));
You could just process the parent of these nodes.

[Marc Treib, 2016-04-07 13:38:39]

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

https://codereview.chromium.org/1851803002/diff/60001/chrome/browser/android/...
chrome/browser/android/ntp/popular_sites.cc:118: GURL GetPopularSitesURL(const
std::string& country,
On 2016/04/07 12:24:28, Bernhard Bauer wrote:
> This now always gets called with |pending_country_| and |pending_version_| --
> would it make sense to make it a member function?

Done.

https://codereview.chromium.org/1851803002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/popular_sites_internals.html (right):

https://codereview.chromium.org/1851803002/diff/60001/chrome/browser/resource...
chrome/browser/resources/popular_sites_internals.html:51: <div id="info"
class="section">
On 2016/04/07 12:24:28, Bernhard Bauer wrote:
> This ID is already in use.

D'oh.. good catch! How did this not create any problems?!
Anyway, this ID (also "sites" below) isn't needed anymore, so I removed it.

https://codereview.chromium.org/1851803002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/popular_sites_internals.js (right):

https://codereview.chromium.org/1851803002/diff/60001/chrome/browser/resource...
chrome/browser/resources/popular_sites_internals.js:36: jstProcess(context,
$('info'));
On 2016/04/07 12:24:28, Bernhard Bauer wrote:
> You could just process the parent of these nodes.

Done.

[Bernhard Bauer, 2016-04-07 13:46:12]

LGTM!

https://codereview.chromium.org/1851803002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/popular_sites_internals.html (right):

https://codereview.chromium.org/1851803002/diff/60001/chrome/browser/resource...
chrome/browser/resources/popular_sites_internals.html:51: <div id="info"
class="section">
On 2016/04/07 13:38:39, Marc Treib wrote:
> On 2016/04/07 12:24:28, Bernhard Bauer wrote:
> > This ID is already in use.
> 
> D'oh.. good catch! How did this not create any problems?!

Browsers will happily render documents with duplicate IDs. getElementById() will
return the first element (in tree order) with the ID
[https://dom.spec.whatwg.org/#dom-nonelementparentnode-getelementbyid], which is
the parent node, but that one can be processed just as well.

[Marc Treib, 2016-04-07 13:50:40]

The CQ bit was checked by treib@chromium.org

[Marc Treib, 2016-04-07 13:50:41]

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

[commit-bot: I haz the power, 2016-04-07 13:50:57]

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

[commit-bot: I haz the power, 2016-04-07 14:44:24]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-04-07 14:46:39]

Description was changed from

==========
[NTP Popular Sites] Store country/version in prefs
...and show on chrome://popular-sites-internals

BUG=599844
==========

to

==========
[NTP Popular Sites] Store country/version in prefs
...and show on chrome://popular-sites-internals

BUG=599844

Committed: https://crrev.com/f38cc25bf2d6fa2407d9b3fc7cbdc16836b5ee12
Cr-Commit-Position: refs/heads/master@{#385753}
==========

[commit-bot: I haz the power, 2016-04-07 14:46:40]

Patchset 5 (id:??) landed as
https://crrev.com/f38cc25bf2d6fa2407d9b3fc7cbdc16836b5ee12
Cr-Commit-Position: refs/heads/master@{#385753}