Add module for counting date-last-roll-call and persisting those counts

Add module for counting date-last-roll-call and persisting those counts
in the user-data-dir.

BUG=590291
Committed: https://crrev.com/d2d9a3375e853ba101d16a5cc8d91d6c2db44f2d
Cr-Commit-Position: refs/heads/master@{#386271}

[waffles, 2016-04-07 02:37:29]

waffles@chromium.org changed reviewers:
+ sorin@google.com

[waffles, 2016-04-07 02:37:29]

PTAL; if LGTY, will then expand reviewers to include ios and extension owners.

Otherwise, it's been a while since I landed code in Chromium, so I'm a little
rusty and appreciate the feedback.

[Sorin Jianu, 2016-04-07 04:03:18]

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

[Sorin Jianu, 2016-04-07 04:03:19]

lgtm

Thank you. The code is in good shape, all below is minor nits that you could
consider and eventually fix after this lands.

I will be in the promo committee for the entire day tomorrow. You could ping me
as needed or land this as is.

https://codereview.chromium.org/1861383004/diff/1/chrome/browser/component_up...
File chrome/browser/component_updater/chrome_component_updater_configurator.cc
(right):

https://codereview.chromium.org/1861383004/diff/1/chrome/browser/component_up...
chrome/browser/component_updater/chrome_component_updater_configurator.cc:181:
CHECK(PathService::Get(chrome::DIR_USER_DATA, &result));
Do we need to CHECK? Can we be fault tolerant? I don't have an opinion, I am
just asking the question.

https://codereview.chromium.org/1861383004/diff/1/components/update_client.gypi
File components/update_client.gypi (right):

https://codereview.chromium.org/1861383004/diff/1/components/update_client.gy...
components/update_client.gypi:37: 'update_client/component_metadata.cc',
Just curious, why do we call this "metadata"?

https://codereview.chromium.org/1861383004/diff/1/components/update_client/co...
File components/update_client/component_metadata.cc (right):

https://codereview.chromium.org/1861383004/diff/1/components/update_client/co...
components/update_client/component_metadata.cc:36:
scoped_refptr<ComponentMetadata>(this),
is the cast needed, here and below?

https://codereview.chromium.org/1861383004/diff/1/components/update_client/co...
components/update_client/component_metadata.cc:61: if (!root.get() ||
!root->IsType(base::Value::TYPE_DICTIONARY)) {
Can 61-66 be written using ?

matadata_ = scoped_ptr<base::DictionaryValue>(root && root->IsType() ? ...

Alternatively, can we use the idiomatic if (a && a->) which is easier to read
instead of doing De Morgan in my head?

https://codereview.chromium.org/1861383004/diff/1/components/update_client/co...
components/update_client/component_metadata.cc:79: int dlrc;
not initialized

https://codereview.chromium.org/1861383004/diff/1/components/update_client/co...
components/update_client/component_metadata.cc:89: if (!ready_)  // Trying to
write before we've ever read it: drop the update.
Will this cause any issue? Is it ok to silently fail the call?
Maybe have this function return bool, so that the caller knows the call failed.

https://codereview.chromium.org/1861383004/diff/1/components/update_client/co...
File components/update_client/component_metadata.h (right):

https://codereview.chromium.org/1861383004/diff/1/components/update_client/co...
components/update_client/component_metadata.h:48: // Records the
DateLastRollCall for the specified |ids|. |datenum| must be a
assert in the implementation?

https://codereview.chromium.org/1861383004/diff/1/components/update_client/co...
components/update_client/component_metadata.h:55: virtual ~ComponentMetadata();
This class has no virtual function, why the virtual dtor?

https://codereview.chromium.org/1861383004/diff/1/components/update_client/co...
components/update_client/component_metadata.h:67: const base::ThreadChecker
thread_checker_;
do we want the thread_checker_ to be declared as the first data member, so that
is the first to be initialized, and last to be destroyed?

https://codereview.chromium.org/1861383004/diff/1/components/update_client/co...
File components/update_client/component_metadata_unittest.cc (right):

https://codereview.chromium.org/1861383004/diff/1/components/update_client/co...
components/update_client/component_metadata_unittest.cc:29:
base::MessageLoopForIO used_for_threadtaskrunnerhandle_;
Do we need base::MessageLoopForIO? I think we've been using this for tests that
use the network.

https://codereview.chromium.org/1861383004/diff/1/components/update_client/co...
components/update_client/component_metadata_unittest.cc:35: task_runner_ =
base::ThreadTaskRunnerHandle::Get();
why not directly initialize in the ctor?

https://codereview.chromium.org/1861383004/diff/1/components/update_client/up...
File components/update_client/update_checker.cc (right):

https://codereview.chromium.org/1861383004/diff/1/components/update_client/up...
components/update_client/update_checker.cc:99: explicit UpdateCheckerImpl(
explicit not needed.

[waffles, 2016-04-07 17:04:44]

Thanks!

https://codereview.chromium.org/1861383004/diff/1/chrome/browser/component_up...
File chrome/browser/component_updater/chrome_component_updater_configurator.cc
(right):

https://codereview.chromium.org/1861383004/diff/1/chrome/browser/component_up...
chrome/browser/component_updater/chrome_component_updater_configurator.cc:181:
CHECK(PathService::Get(chrome::DIR_USER_DATA, &result));
On 2016/04/07 04:03:19, Sorin Jianu wrote:
> Do we need to CHECK? Can we be fault tolerant? I don't have an opinion, I am
> just asking the question.

Done. In practice it won't matter because several component installers do this
same CHECK, but you make a good point.

https://codereview.chromium.org/1861383004/diff/1/components/update_client.gypi
File components/update_client.gypi (right):

https://codereview.chromium.org/1861383004/diff/1/components/update_client.gy...
components/update_client.gypi:37: 'update_client/component_metadata.cc',
On 2016/04/07 04:03:19, Sorin Jianu wrote:
> Just curious, why do we call this "metadata"?

I've renamed it to persisted_data.*. component_metadata is a poor name.

https://codereview.chromium.org/1861383004/diff/1/components/update_client/co...
File components/update_client/component_metadata.cc (right):

https://codereview.chromium.org/1861383004/diff/1/components/update_client/co...
components/update_client/component_metadata.cc:36:
scoped_refptr<ComponentMetadata>(this),
On 2016/04/07 04:03:19, Sorin Jianu wrote:
> is the cast needed, here and below?

Not sure about this one, let's discuss offline. There are several places in the
this directory where we follow this pattern, so if they can be cleaned up, let's
take it in a second CL, but keep consistency here.

https://codereview.chromium.org/1861383004/diff/1/components/update_client/co...
components/update_client/component_metadata.cc:61: if (!root.get() ||
!root->IsType(base::Value::TYPE_DICTIONARY)) {
On 2016/04/07 04:03:19, Sorin Jianu wrote:
> Can 61-66 be written using ?
> 
> matadata_ = scoped_ptr<base::DictionaryValue>(root && root->IsType() ? ...
> 
> Alternatively, can we use the idiomatic if (a && a->) which is easier to read
> instead of doing De Morgan in my head?

Done.

https://codereview.chromium.org/1861383004/diff/1/components/update_client/co...
components/update_client/component_metadata.cc:79: int dlrc;
On 2016/04/07 04:03:19, Sorin Jianu wrote:
> not initialized

Done.

https://codereview.chromium.org/1861383004/diff/1/components/update_client/co...
components/update_client/component_metadata.cc:89: if (!ready_)  // Trying to
write before we've ever read it: drop the update.
On 2016/04/07 04:03:19, Sorin Jianu wrote:
> Will this cause any issue? Is it ok to silently fail the call?
> Maybe have this function return bool, so that the caller knows the call
failed.

At this time, it's not clear to me what the caller can do differently based on
that information (we don't want to abandon the updatecheck just because of a
failure here), so I'm inclined to punt this. I've expanded the comment to call
out this behavior.

https://codereview.chromium.org/1861383004/diff/1/components/update_client/co...
File components/update_client/component_metadata.h (right):

https://codereview.chromium.org/1861383004/diff/1/components/update_client/co...
components/update_client/component_metadata.h:48: // Records the
DateLastRollCall for the specified |ids|. |datenum| must be a
On 2016/04/07 04:03:19, Sorin Jianu wrote:
> assert in the implementation?

Done.

https://codereview.chromium.org/1861383004/diff/1/components/update_client/co...
components/update_client/component_metadata.h:55: virtual ~ComponentMetadata();
On 2016/04/07 04:03:19, Sorin Jianu wrote:
> This class has no virtual function, why the virtual dtor?

Done.

https://codereview.chromium.org/1861383004/diff/1/components/update_client/co...
components/update_client/component_metadata.h:67: const base::ThreadChecker
thread_checker_;
On 2016/04/07 04:03:19, Sorin Jianu wrote:
> do we want the thread_checker_ to be declared as the first data member, so
that
> is the first to be initialized, and last to be destroyed?

Done.

https://codereview.chromium.org/1861383004/diff/1/components/update_client/co...
File components/update_client/component_metadata_unittest.cc (right):

https://codereview.chromium.org/1861383004/diff/1/components/update_client/co...
components/update_client/component_metadata_unittest.cc:29:
base::MessageLoopForIO used_for_threadtaskrunnerhandle_;
On 2016/04/07 04:03:19, Sorin Jianu wrote:
> Do we need base::MessageLoopForIO? I think we've been using this for tests
that
> use the network.

Done.

https://codereview.chromium.org/1861383004/diff/1/components/update_client/co...
components/update_client/component_metadata_unittest.cc:35: task_runner_ =
base::ThreadTaskRunnerHandle::Get();
On 2016/04/07 04:03:19, Sorin Jianu wrote:
> why not directly initialize in the ctor?

Done. (Removed it as a class member, put it in the tests directly.)

https://codereview.chromium.org/1861383004/diff/1/components/update_client/up...
File components/update_client/update_checker.cc (right):

https://codereview.chromium.org/1861383004/diff/1/components/update_client/up...
components/update_client/update_checker.cc:99: explicit UpdateCheckerImpl(
On 2016/04/07 04:03:19, Sorin Jianu wrote:
> explicit not needed.

Done.

[waffles, 2016-04-07 17:10:55]

waffles@chromium.org changed reviewers:
+ asargent@chromium.org, rohitrao@chromium.org
- sorin@google.com

[waffles, 2016-04-07 17:10:56]

asargent, rohitrao: Please take a look; hoping to make the branch cut. The
ultimate effect of this change is that a *_metadata.json file will be created in
the DIR_USER_DATA if and only if your configurator is actually being used in
conjunction with update_client.

[asargent_no_longer_on_chrome, 2016-04-07 17:53:49]

One overall question: is there a reason you're using separate files for this
data instead of just writing it into a section of the already existing
Preferences file? If you used components/prefs/pref_service.h and friends you'd
then get a lot of the implementation details of "a serialized DictionaryValue"
for free. 

Assuming you really do need separate files, one naming nit: the names
"component_metadata.json" and "extensions_metadata.json" aren't descriptive of
the fact that the data they contain has to do with update checks; it would be
nice to indicate that in the names somehow.

[waffles, 2016-04-07 17:59:19]

On 2016/04/07 17:53:49, Antony Sargent wrote:
> One overall question: is there a reason you're using separate files for this
> data instead of just writing it into a section of the already existing
> Preferences file? If you used components/prefs/pref_service.h and friends
you'd
> then get a lot of the implementation details of "a serialized DictionaryValue"
> for free. 
> 
> Assuming you really do need separate files, one naming nit: the names
> "component_metadata.json" and "extensions_metadata.json" aren't descriptive of
> the fact that the data they contain has to do with update checks; it would be
> nice to indicate that in the names somehow.

My understanding is that prefs are per-profile. If that's correct, then we can't
use them, because components are installed cross-profile. Happy to adjust the
names. component|extension_update_metadata sound better to you?

[asargent_no_longer_on_chrome, 2016-04-07 18:18:46]

For install-wide preferences that aren't user specific, there's the
"<UserDataDir>/Local State" file. You could have the configurators pass a
PrefService* instead of a FilePath, and then the chrome configurators would just
use g_browser_process->local_state() to get at the Local State prefs. 

Doing it this way would have nice benefits for eventually moving extension
updates to the common updater code, because presumably the last-roll-call data
for extension updates would be stored per-user-profile, so the configurator for
extension updates could pass a PrefService that is in the per-profile
Preferences file.

[Sorin Jianu, 2016-04-07 18:27:35]

lgtm

I will look at the last patch soon.

https://codereview.chromium.org/1861383004/diff/1/components/update_client/co...
File components/update_client/component_metadata.cc (right):

https://codereview.chromium.org/1861383004/diff/1/components/update_client/co...
components/update_client/component_metadata.cc:36:
scoped_refptr<ComponentMetadata>(this),
On 2016/04/07 17:04:43, waffles wrote:
> On 2016/04/07 04:03:19, Sorin Jianu wrote:
> > is the cast needed, here and below?
> 
> Not sure about this one, let's discuss offline. There are several places in
the
> this directory where we follow this pattern, so if they can be cleaned up,
let's
> take it in a second CL, but keep consistency here.

If the code compiles without the function-style cast, then I suggest removing
it. C++ is a statically typed language, and in most cases, if the code compiles,
it will have the correct meaning.

https://codereview.chromium.org/1861383004/diff/1/components/update_client/co...
components/update_client/component_metadata.cc:89: if (!ready_)  // Trying to
write before we've ever read it: drop the update.
On 2016/04/07 17:04:43, waffles wrote:
> On 2016/04/07 04:03:19, Sorin Jianu wrote:
> > Will this cause any issue? Is it ok to silently fail the call?
> > Maybe have this function return bool, so that the caller knows the call
> failed.
> 
> At this time, it's not clear to me what the caller can do differently based on
> that information (we don't want to abandon the updatecheck just because of a
> failure here), so I'm inclined to punt this. I've expanded the comment to call
> out this behavior.

The caller could assert, if it wants to enforce some correctness issue. Don't
call the function before initialization is complete. Also, since the
initialization is asynchronous, we might consider a callback be called, so that
a caller can synchronize its execution and not call this function too soon, if
the error in this call is unrecoverable, as far as the caller is concerned. TBD
in a future CL.

[Sorin Jianu, 2016-04-07 18:53:50]

lgtm

https://codereview.chromium.org/1861383004/diff/80001/components/update_clien...
File components/update_client/configurator.h (right):

https://codereview.chromium.org/1861383004/diff/80001/components/update_clien...
components/update_client/configurator.h:117: virtual base::FilePath
GetMetadataPath() const = 0;
maybe rename to GetPersistedDataPath?

https://codereview.chromium.org/1861383004/diff/80001/components/update_clien...
File components/update_client/persisted_data.cc (right):

https://codereview.chromium.org/1861383004/diff/80001/components/update_clien...
components/update_client/persisted_data.cc:62: ?
scoped_ptr<base::DictionaryValue>(
could do the ?: as an argument to scoped_ptr<base::DictionaryValue>.

[waffles, 2016-04-07 22:45:17]

waffles@chromium.org changed reviewers:
+ bauerb@chromium.org, pam@chromium.org

[waffles, 2016-04-07 22:45:18]

OK, refactored to use PrefService.

PrefService is not ref-counted and so I have concerns about how this will play
out for extensions during browser shutdown - asargent and I have discussed the
various implications and decided not to implement this feature for extensions at
this time - hence the extension configurator will return NULL.

I am similarly unsure of which PrefService / lifetimes to use on iOS, so the
feature is disabled for iOS as well. rohitrao: I just need your approval for the
"return NULL".

I believe we're OK during browser shutdown for the component_updater, because
browser_process_impl will release the component updater reference before tearing
down the path (and all threads/blocking pool should have surrendered any
callbacks holding references to update_client members by then).

pam, bauerb: I need your approvals for the DEPS changes and the new Register
call.

https://codereview.chromium.org/1861383004/diff/80001/components/update_clien...
File components/update_client/configurator.h (right):

https://codereview.chromium.org/1861383004/diff/80001/components/update_clien...
components/update_client/configurator.h:117: virtual base::FilePath
GetMetadataPath() const = 0;
On 2016/04/07 18:53:50, Sorin Jianu wrote:
> maybe rename to GetPersistedDataPath?

Moot.

https://codereview.chromium.org/1861383004/diff/80001/components/update_clien...
File components/update_client/persisted_data.cc (right):

https://codereview.chromium.org/1861383004/diff/80001/components/update_clien...
components/update_client/persisted_data.cc:62: ?
scoped_ptr<base::DictionaryValue>(
On 2016/04/07 18:53:50, Sorin Jianu wrote:
> could do the ?: as an argument to scoped_ptr<base::DictionaryValue>.

Good point, but moot.

[asargent_no_longer_on_chrome, 2016-04-07 23:07:25]

extensions parts lgtm

https://codereview.chromium.org/1861383004/diff/120001/chrome/browser/extensi...
File chrome/browser/extensions/updater/chrome_update_client_config.cc (right):

https://codereview.chromium.org/1861383004/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/updater/chrome_update_client_config.cc:14: #include
"components/prefs/pref_service.h"
do you still need these added includes?

https://codereview.chromium.org/1861383004/diff/120001/chrome/browser/extensi...
File chrome/browser/extensions/updater/chrome_update_client_config.h (right):

https://codereview.chromium.org/1861383004/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/updater/chrome_update_client_config.h:16: class
PrefService;
You should already get this from the
extensions/browser/updater/update_client_config.h include which gets it from
components/update_client/configurator.h, but it isn't that big a deal either way
(note that we don't forward declare other classes like GURL).

https://codereview.chromium.org/1861383004/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/updater/chrome_update_client_config.h:20: }
do you still need this FilePath declaration?

[waffles, 2016-04-07 23:22:24]

https://codereview.chromium.org/1861383004/diff/120001/chrome/browser/extensi...
File chrome/browser/extensions/updater/chrome_update_client_config.cc (right):

https://codereview.chromium.org/1861383004/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/updater/chrome_update_client_config.cc:14: #include
"components/prefs/pref_service.h"
On 2016/04/07 23:07:25, Antony Sargent wrote:
> do you still need these added includes?

Done.

https://codereview.chromium.org/1861383004/diff/120001/chrome/browser/extensi...
File chrome/browser/extensions/updater/chrome_update_client_config.h (right):

https://codereview.chromium.org/1861383004/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/updater/chrome_update_client_config.h:16: class
PrefService;
On 2016/04/07 23:07:25, Antony Sargent wrote:
> You should already get this from the
> extensions/browser/updater/update_client_config.h include which gets it from
> components/update_client/configurator.h, but it isn't that big a deal either
way
> (note that we don't forward declare other classes like GURL). 
Done.

https://codereview.chromium.org/1861383004/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/updater/chrome_update_client_config.h:20: }
On 2016/04/07 23:07:25, Antony Sargent wrote:
> do you still need this FilePath declaration?

Done.

[Bernhard Bauer, 2016-04-08 09:06:10]

A couple of comments, but they're mostly nits.

What I'm wondering about the most is, why is PersistedData refcounted? If
whoever constructs it needs to pass in a raw pointer to a PrefService that is
guaranteed to outlive it anyway, what's the advantage we get from refcounting?
See also my comment in
https://codereview.chromium.org/1861383004/diff/160001/components/update_clie...:
What you are building is essentially the model where there is a single owner for
an object, and that owner guarantees aliveness for all other places that access
it -- except you are building that with refcounting instead of regular
ownership.

https://codereview.chromium.org/1861383004/diff/160001/chrome/browser/extensi...
File chrome/browser/extensions/updater/chrome_update_client_config.cc (right):

https://codereview.chromium.org/1861383004/diff/160001/chrome/browser/extensi...
chrome/browser/extensions/updater/chrome_update_client_config.cc:105: return
NULL;
nullptr ☺

https://codereview.chromium.org/1861383004/diff/160001/components/update_clie...
File components/update_client/configurator.h (right):

https://codereview.chromium.org/1861383004/diff/160001/components/update_clie...
components/update_client/configurator.h:119: // Returning NULL is safe and will
disable any functionality that requires
Nit: Now that the NULL #define is deprecated, I would use "null" for the
conceptual null value (which we implement in code with `nullptr`).

https://codereview.chromium.org/1861383004/diff/160001/components/update_clie...
File components/update_client/persisted_data.cc (right):

https://codereview.chromium.org/1861383004/diff/160001/components/update_clie...
components/update_client/persisted_data.cc:19: const char
kPersistedDataPreference[] = "updateclientdata";
FWIW, the anonymous namespace isn't strictly necessary when all you have in
there are constants, as those get internal linkage automatically.

https://codereview.chromium.org/1861383004/diff/160001/components/update_clie...
components/update_client/persisted_data.cc:36: return -2;
Make this a constant?

https://codereview.chromium.org/1861383004/diff/160001/components/update_clie...
components/update_client/persisted_data.cc:40: // We assume ids do not contain
'.' characters.
DCHECK that?

https://codereview.chromium.org/1861383004/diff/160001/components/update_clie...
File components/update_client/persisted_data.h (right):

https://codereview.chromium.org/1861383004/diff/160001/components/update_clie...
components/update_client/persisted_data.h:32: // The prefService must outlive
the entire update_client.
Nit: |prefService| in pipes?

https://codereview.chromium.org/1861383004/diff/160001/components/update_clie...
components/update_client/persisted_data.h:36: // is no record of the app.
Can you explain what the value means otherwise?

https://codereview.chromium.org/1861383004/diff/160001/components/update_clie...
components/update_client/persisted_data.h:37: int DateLastRollCall(const
std::string& id) const;
Nit: GetDateLastRollCall(), so it doesn't sound like "date" is a verb here?

https://codereview.chromium.org/1861383004/diff/160001/components/update_clie...
components/update_client/persisted_data.h:40: // non-negative integer: calls
with a negative |datenum| are simply ignored.
Is there a reason you ignore negative values rather than DCHECKing? I.e., can
this really happen in the absence of bugs?

https://codereview.chromium.org/1861383004/diff/160001/components/update_clie...
components/update_client/persisted_data.h:57: void
RegisterPersistedDataPreferences(PrefRegistrySimple* registry);
Make this a static method on PersistedData?

https://codereview.chromium.org/1861383004/diff/160001/components/update_clie...
File components/update_client/persisted_data_unittest.cc (right):

https://codereview.chromium.org/1861383004/diff/160001/components/update_clie...
components/update_client/persisted_data_unittest.cc:14: class PersistedDataTest
: public testing::Test {
You don't need to add an empty test fixture. Just use the TEST macro instead of
TEST_F, which creates a test without a fixture.

https://codereview.chromium.org/1861383004/diff/160001/components/update_clie...
components/update_client/persisted_data_unittest.cc:30:
scoped_ptr<TestingPrefServiceSimple> pref(new TestingPrefServiceSimple());
Nit: scoped_ptr<> is deprecated now! 😃 Use std::unique_ptr<> instead.

https://codereview.chromium.org/1861383004/diff/160001/components/update_clie...
File components/update_client/test_configurator.cc (right):

https://codereview.chromium.org/1861383004/diff/160001/components/update_clie...
components/update_client/test_configurator.cc:158: return NULL;
nullptr 😃

https://codereview.chromium.org/1861383004/diff/160001/components/update_clie...
File components/update_client/update_checker.cc (right):

https://codereview.chromium.org/1861383004/diff/160001/components/update_clie...
components/update_client/update_checker.cc:118: const
scoped_refptr<PersistedData>& metadata_;
Uh, this is a bit... unconventional. Why not just take a regular reference to
the PersistedData?

https://codereview.chromium.org/1861383004/diff/160001/components/update_clie...
components/update_client/update_checker.cc:154: ids_checked->push_back(crx->id);
Nit: Braces are optional for one-line bodies (and not used elsewhere, e.g. line
149).

https://codereview.chromium.org/1861383004/diff/160001/components/update_clie...
components/update_client/update_checker.cc:162: base::Unretained(this),
base::Passed(&ids_checked)));
Since base::Passed is now implemented in terms of std::move, you could try just
using the std::vector<> directly here, and base::Passed should make sure that
its passed to the function instead of copied.

https://codereview.chromium.org/1861383004/diff/160001/components/update_clie...
components/update_client/update_checker.cc:167:
scoped_ptr<std::vector<std::string>> ids_checked,
std::unique_ptr

https://codereview.chromium.org/1861383004/diff/160001/components/update_clie...
components/update_client/update_checker.cc:177: if (daynum !=
UpdateResponse::kNoDaystart) {
No braces.

https://codereview.chromium.org/1861383004/diff/160001/ios/chrome/browser/com...
File ios/chrome/browser/component_updater/ios_component_updater_configurator.cc
(right):

https://codereview.chromium.org/1861383004/diff/160001/ios/chrome/browser/com...
ios/chrome/browser/component_updater/ios_component_updater_configurator.cc:157:
return NULL;
nullptr 😃

[Bernhard Bauer, 2016-04-08 09:10:42]

https://codereview.chromium.org/1861383004/diff/160001/components/update_clie...
File components/update_client/persisted_data.h (right):

https://codereview.chromium.org/1861383004/diff/160001/components/update_clie...
components/update_client/persisted_data.h:33: PersistedData(PrefService*
prefService);
Also, underscore_style for variables.

[waffles, 2016-04-08 19:11:01]

PTAL. Removed the refcounting. Thanks!

https://codereview.chromium.org/1861383004/diff/160001/chrome/browser/extensi...
File chrome/browser/extensions/updater/chrome_update_client_config.cc (right):

https://codereview.chromium.org/1861383004/diff/160001/chrome/browser/extensi...
chrome/browser/extensions/updater/chrome_update_client_config.cc:105: return
NULL;
On 2016/04/08 09:06:09, Bernhard Bauer wrote:
> nullptr ☺

Done.

https://codereview.chromium.org/1861383004/diff/160001/components/update_clie...
File components/update_client/configurator.h (right):

https://codereview.chromium.org/1861383004/diff/160001/components/update_clie...
components/update_client/configurator.h:119: // Returning NULL is safe and will
disable any functionality that requires
On 2016/04/08 09:06:09, Bernhard Bauer wrote:
> Nit: Now that the NULL #define is deprecated, I would use "null" for the
> conceptual null value (which we implement in code with `nullptr`).

Done.

https://codereview.chromium.org/1861383004/diff/160001/components/update_clie...
File components/update_client/persisted_data.cc (right):

https://codereview.chromium.org/1861383004/diff/160001/components/update_clie...
components/update_client/persisted_data.cc:19: const char
kPersistedDataPreference[] = "updateclientdata";
On 2016/04/08 09:06:09, Bernhard Bauer wrote:
> FWIW, the anonymous namespace isn't strictly necessary when all you have in
> there are constants, as those get internal linkage automatically. 

Done.

https://codereview.chromium.org/1861383004/diff/160001/components/update_clie...
components/update_client/persisted_data.cc:36: return -2;
On 2016/04/08 09:06:09, Bernhard Bauer wrote:
> Make this a constant?

Done.

https://codereview.chromium.org/1861383004/diff/160001/components/update_clie...
components/update_client/persisted_data.cc:40: // We assume ids do not contain
'.' characters.
On 2016/04/08 09:06:09, Bernhard Bauer wrote:
> DCHECK that?

Done.

https://codereview.chromium.org/1861383004/diff/160001/components/update_clie...
File components/update_client/persisted_data.h (right):

https://codereview.chromium.org/1861383004/diff/160001/components/update_clie...
components/update_client/persisted_data.h:32: // The prefService must outlive
the entire update_client.
On 2016/04/08 09:06:09, Bernhard Bauer wrote:
> Nit: |prefService| in pipes?

Done.

https://codereview.chromium.org/1861383004/diff/160001/components/update_clie...
components/update_client/persisted_data.h:33: PersistedData(PrefService*
prefService);
On 2016/04/08 09:10:41, Bernhard Bauer wrote:
> Also, underscore_style for variables.

Done.

https://codereview.chromium.org/1861383004/diff/160001/components/update_clie...
components/update_client/persisted_data.h:36: // is no record of the app.
On 2016/04/08 09:06:10, Bernhard Bauer wrote:
> Can you explain what the value means otherwise?

Done.

https://codereview.chromium.org/1861383004/diff/160001/components/update_clie...
components/update_client/persisted_data.h:37: int DateLastRollCall(const
std::string& id) const;
On 2016/04/08 09:06:09, Bernhard Bauer wrote:
> Nit: GetDateLastRollCall(), so it doesn't sound like "date" is a verb here?

Done. (Too much Go lately, I guess.)

https://codereview.chromium.org/1861383004/diff/160001/components/update_clie...
components/update_client/persisted_data.h:40: // non-negative integer: calls
with a negative |datenum| are simply ignored.
On 2016/04/08 09:06:10, Bernhard Bauer wrote:
> Is there a reason you ignore negative values rather than DCHECKing? I.e., can
> this really happen in the absence of bugs?

The value ultimately comes from the network - so we could either require the
parser to branch to not call this if it sees a negative number and then DCHECK
here, or just have the parsing code treat it as an opaque int and have the
branch here. Seemed better to me to handle it here, since this module is really
responsible for interpreting the meaning of these values.

https://codereview.chromium.org/1861383004/diff/160001/components/update_clie...
components/update_client/persisted_data.h:57: void
RegisterPersistedDataPreferences(PrefRegistrySimple* registry);
On 2016/04/08 09:06:10, Bernhard Bauer wrote:
> Make this a static method on PersistedData?

Done.

https://codereview.chromium.org/1861383004/diff/160001/components/update_clie...
File components/update_client/persisted_data_unittest.cc (right):

https://codereview.chromium.org/1861383004/diff/160001/components/update_clie...
components/update_client/persisted_data_unittest.cc:14: class PersistedDataTest
: public testing::Test {
On 2016/04/08 09:06:10, Bernhard Bauer wrote:
> You don't need to add an empty test fixture. Just use the TEST macro instead
of
> TEST_F, which creates a test without a fixture.

Done.

https://codereview.chromium.org/1861383004/diff/160001/components/update_clie...
components/update_client/persisted_data_unittest.cc:30:
scoped_ptr<TestingPrefServiceSimple> pref(new TestingPrefServiceSimple());
On 2016/04/08 09:06:10, Bernhard Bauer wrote:
> Nit: scoped_ptr<> is deprecated now! 😃 Use std::unique_ptr<> instead.

Done.

https://codereview.chromium.org/1861383004/diff/160001/components/update_clie...
File components/update_client/test_configurator.cc (right):

https://codereview.chromium.org/1861383004/diff/160001/components/update_clie...
components/update_client/test_configurator.cc:158: return NULL;
On 2016/04/08 09:06:10, Bernhard Bauer wrote:
> nullptr 😃

Done.

https://codereview.chromium.org/1861383004/diff/160001/components/update_clie...
File components/update_client/update_checker.cc (right):

https://codereview.chromium.org/1861383004/diff/160001/components/update_clie...
components/update_client/update_checker.cc:118: const
scoped_refptr<PersistedData>& metadata_;
On 2016/04/08 09:06:10, Bernhard Bauer wrote:
> Uh, this is a bit... unconventional. Why not just take a regular reference to
> the PersistedData?

Done. (I don't think I originally meant to have that & in there.)

https://codereview.chromium.org/1861383004/diff/160001/components/update_clie...
components/update_client/update_checker.cc:154: ids_checked->push_back(crx->id);
On 2016/04/08 09:06:10, Bernhard Bauer wrote:
> Nit: Braces are optional for one-line bodies (and not used elsewhere, e.g.
line
> 149).

Done.

https://codereview.chromium.org/1861383004/diff/160001/components/update_clie...
components/update_client/update_checker.cc:162: base::Unretained(this),
base::Passed(&ids_checked)));
On 2016/04/08 09:06:10, Bernhard Bauer wrote:
> Since base::Passed is now implemented in terms of std::move, you could try
just
> using the std::vector<> directly here, and base::Passed should make sure that
> its passed to the function instead of copied.

Hm, not sure I'm understand what you're suggesting. The std::vector will be an
lvalue, so we can't use base::Passed, right?

https://codereview.chromium.org/1861383004/diff/160001/components/update_clie...
components/update_client/update_checker.cc:167:
scoped_ptr<std::vector<std::string>> ids_checked,
On 2016/04/08 09:06:10, Bernhard Bauer wrote:
> std::unique_ptr

Done.

https://codereview.chromium.org/1861383004/diff/160001/components/update_clie...
components/update_client/update_checker.cc:177: if (daynum !=
UpdateResponse::kNoDaystart) {
On 2016/04/08 09:06:10, Bernhard Bauer wrote:
> No braces.

Done.

https://codereview.chromium.org/1861383004/diff/160001/ios/chrome/browser/com...
File ios/chrome/browser/component_updater/ios_component_updater_configurator.cc
(right):

https://codereview.chromium.org/1861383004/diff/160001/ios/chrome/browser/com...
ios/chrome/browser/component_updater/ios_component_updater_configurator.cc:157:
return NULL;
On 2016/04/08 09:06:10, Bernhard Bauer wrote:
> nullptr 😃

Done.

[rohitrao (ping after 24h), 2016-04-08 20:22:58]

ios/ changes LGTM.  We don't need to persist this data on iOS?

[waffles, 2016-04-08 20:29:05]

Thanks Rohit!

On 2016/04/08 20:22:58, rohitrao wrote:
> ios/ changes LGTM.  We don't need to persist this data on iOS?

Not at this time. I'll work on a follow-up CL that handles iOS.

[Bernhard Bauer, 2016-04-08 21:54:07]

LGTM, thanks!

https://codereview.chromium.org/1861383004/diff/160001/components/update_clie...
File components/update_client/update_checker.cc (right):

https://codereview.chromium.org/1861383004/diff/160001/components/update_clie...
components/update_client/update_checker.cc:162: base::Unretained(this),
base::Passed(&ids_checked)));
On 2016/04/08 19:11:01, waffles wrote:
> On 2016/04/08 09:06:10, Bernhard Bauer wrote:
> > Since base::Passed is now implemented in terms of std::move, you could try
> just
> > using the std::vector<> directly here, and base::Passed should make sure
that
> > its passed to the function instead of copied.
> 
> Hm, not sure I'm understand what you're suggesting. The std::vector will be an
> lvalue, so we can't use base::Passed, right?

No, base::Passed has a form where it takes a pointer, and then calls std::move
on the value pointed to, which allows it to make it an rvalue. Basically, that
is the same trick that binding a scoped_ptr to a callback (which isn't copyable
at all), so it should also work for not copying a std::vector. But this is
definitely okay too – no need for you to become the guinea pig for me here :-D

https://codereview.chromium.org/1861383004/diff/240001/components/update_clie...
File components/update_client/persisted_data.cc (right):

https://codereview.chromium.org/1861383004/diff/240001/components/update_clie...
components/update_client/persisted_data.cc:37: DCHECK(std::string::npos ==
id.find('.'));
Nit: Use DCHECK_EQ for nicer error messages in case of a failure (keeping the
expected value first).

[waffles, 2016-04-08 22:00:49]

https://codereview.chromium.org/1861383004/diff/240001/components/update_clie...
File components/update_client/persisted_data.cc (right):

https://codereview.chromium.org/1861383004/diff/240001/components/update_clie...
components/update_client/persisted_data.cc:37: DCHECK(std::string::npos ==
id.find('.'));
On 2016/04/08 21:54:07, Bernhard Bauer wrote:
> Nit: Use DCHECK_EQ for nicer error messages in case of a failure (keeping the
> expected value first).

Done.

[waffles, 2016-04-08 22:00:59]

The CQ bit was checked by waffles@chromium.org

[waffles, 2016-04-08 22:01:00]

The patchset sent to the CQ was uploaded after l-g-t-m from sorin@chromium.org,
asargent@chromium.org, bauerb@chromium.org, rohitrao@chromium.org
Link to the patchset: https://codereview.chromium.org/1861383004/#ps260001
(title: "Final comments")

[commit-bot: I haz the power, 2016-04-08 22:01:55]

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

[commit-bot: I haz the power, 2016-04-08 23:13:40]

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

[commit-bot: I haz the power, 2016-04-08 23:13:43]

Failed to apply patch for components/update_client/update_checker_unittest.cc:
While running git apply --index -3 -p1;
  error: patch failed: components/update_client/update_checker_unittest.cc:158
  Falling back to three-way merge...
  Applied patch to 'components/update_client/update_checker_unittest.cc' with
conflicts.
  U components/update_client/update_checker_unittest.cc

Patch:       components/update_client/update_checker_unittest.cc
Index: components/update_client/update_checker_unittest.cc
diff --git a/components/update_client/update_checker_unittest.cc
b/components/update_client/update_checker_unittest.cc
index
334899611f8323b22e910f0645b5037572e22c1b..9b6a7fcf855a87001a8bb5157e68b66224ee14ca
100644
--- a/components/update_client/update_checker_unittest.cc
+++ b/components/update_client/update_checker_unittest.cc
@@ -12,7 +12,9 @@
 #include "base/run_loop.h"
 #include "base/thread_task_runner_handle.h"
 #include "base/version.h"
+#include "components/prefs/testing_pref_service.h"
 #include "components/update_client/crx_update_item.h"
+#include "components/update_client/persisted_data.h"
 #include "components/update_client/test_configurator.h"
 #include "components/update_client/update_checker.h"
 #include "components/update_client/url_request_post_interceptor.h"
@@ -59,6 +61,8 @@ class UpdateCheckerTest : public testing::Test {
   CrxUpdateItem BuildCrxUpdateItem();
 
   scoped_refptr<TestConfigurator> config_;
+  std::unique_ptr<TestingPrefServiceSimple> pref_;
+  std::unique_ptr<PersistedData> metadata_;
 
   scoped_ptr<UpdateChecker> update_checker_;
 
@@ -84,6 +88,9 @@ UpdateCheckerTest::~UpdateCheckerTest() {
 void UpdateCheckerTest::SetUp() {
   config_ = new TestConfigurator(base::ThreadTaskRunnerHandle::Get(),
                                  base::ThreadTaskRunnerHandle::Get());
+  pref_.reset(new TestingPrefServiceSimple());
+  PersistedData::RegisterPrefs(pref_->registry());
+  metadata_.reset(new PersistedData(pref_.get()));
   interceptor_factory_.reset(
       new InterceptorFactory(base::ThreadTaskRunnerHandle::Get()));
   post_interceptor_ = interceptor_factory_->CreateInterceptor();
@@ -158,7 +165,7 @@ TEST_F(UpdateCheckerTest, UpdateCheckSuccess) {
   EXPECT_TRUE(post_interceptor_->ExpectRequest(
       new PartialMatch("updatecheck"), test_file("updatecheck_reply_1.xml")));
 
-  update_checker_ = UpdateChecker::Create(config_);
+  update_checker_ = UpdateChecker::Create(config_, *metadata_);
 
   CrxUpdateItem item(BuildCrxUpdateItem());
   std::vector<CrxUpdateItem*> items_to_check;
@@ -186,7 +193,7 @@ TEST_F(UpdateCheckerTest, UpdateCheckSuccess) {
       string::npos,
       post_interceptor_->GetRequests()[0].find(
           "app appid=\"jebgalgnebhfojomionfpkfelancnnkf\" version=\"0.9\" "
-          "brand=\"TEST\"><updatecheck />"
+          "brand=\"TEST\"><updatecheck /><ping rd=\"-2\" />"
           "<packages><package fp=\"fp1\"/></packages></app>"));
 
   EXPECT_NE(string::npos,
@@ -205,7 +212,7 @@ TEST_F(UpdateCheckerTest, UpdateCheckSuccessNoBrand) {
       new PartialMatch("updatecheck"), test_file("updatecheck_reply_1.xml")));
 
   config_->SetBrand("TOOLONG");   // Sets an invalid brand code.
-  update_checker_ = UpdateChecker::Create(config_);
+  update_checker_ = UpdateChecker::Create(config_, *metadata_);
 
   CrxUpdateItem item(BuildCrxUpdateItem());
   std::vector<CrxUpdateItem*> items_to_check;
@@ -221,7 +228,8 @@ TEST_F(UpdateCheckerTest, UpdateCheckSuccessNoBrand) {
       string::npos,
       post_interceptor_->GetRequests()[0].find(
           "app appid=\"jebgalgnebhfojomionfpkfelancnnkf\" version=\"0.9\">"
-          "<updatecheck /><packages><package fp=\"fp1\"/></packages></app>"));
+          "<updatecheck /><ping rd=\"-2\" /><packages><package fp=\"fp1\"/>"
+          "</packages></app>"));
 }
 
 // Simulates a 403 server response error.
@@ -229,7 +237,7 @@ TEST_F(UpdateCheckerTest, UpdateCheckError) {
   EXPECT_TRUE(
       post_interceptor_->ExpectRequest(new PartialMatch("updatecheck"), 403));
 
-  update_checker_ = UpdateChecker::Create(config_);
+  update_checker_ = UpdateChecker::Create(config_, *metadata_);
 
   CrxUpdateItem item(BuildCrxUpdateItem());
   std::vector<CrxUpdateItem*> items_to_check;
@@ -255,7 +263,7 @@ TEST_F(UpdateCheckerTest, UpdateCheckDownloadPreference) {
 
   config_->SetDownloadPreference(string("cacheable"));
 
-  update_checker_ = UpdateChecker::Create(config_);
+  update_checker_ = UpdateChecker::Create(config_, *metadata_);
 
   CrxUpdateItem item(BuildCrxUpdateItem());
   std::vector<CrxUpdateItem*> items_to_check;
@@ -281,7 +289,7 @@ TEST_F(UpdateCheckerTest, UpdateCheckCupError) {
       new PartialMatch("updatecheck"), test_file("updatecheck_reply_1.xml")));
 
   config_->SetUseCupSigning(true);
-  update_checker_ = UpdateChecker::Create(config_);
+  update_checker_ = UpdateChecker::Create(config_, *metadata_);
 
   CrxUpdateItem item(BuildCrxUpdateItem());
   std::vector<CrxUpdateItem*> items_to_check;
@@ -303,7 +311,7 @@ TEST_F(UpdateCheckerTest, UpdateCheckCupError) {
       string::npos,
       post_interceptor_->GetRequests()[0].find(
           "app appid=\"jebgalgnebhfojomionfpkfelancnnkf\" version=\"0.9\" "
-          "brand=\"TEST\"><updatecheck />"
+          "brand=\"TEST\"><updatecheck /><ping rd=\"-2\" />"
           "<packages><package fp=\"fp1\"/></packages></app>"));
 
   // Expect an error since the response is not trusted.
@@ -316,7 +324,7 @@ TEST_F(UpdateCheckerTest, UpdateCheckCupError) {
 TEST_F(UpdateCheckerTest, UpdateCheckRequiresEncryptionError) {
   config_->SetUpdateCheckUrl(GURL("http:\\foo\bar"));
 
-  update_checker_ = UpdateChecker::Create(config_);
+  update_checker_ = UpdateChecker::Create(config_, *metadata_);
 
   CrxUpdateItem item(BuildCrxUpdateItem());
   item.component.requires_network_encryption = true;
@@ -332,4 +340,41 @@ TEST_F(UpdateCheckerTest,
UpdateCheckRequiresEncryptionError) {
   EXPECT_EQ(0u, results_.list.size());
 }
 
+// Tests that the PersistedData will get correctly update and reserialize
+// the elapsed_days value.
+TEST_F(UpdateCheckerTest, UpdateCheckDateLastRollCall) {
+  EXPECT_TRUE(post_interceptor_->ExpectRequest(
+      new PartialMatch("updatecheck"), test_file("updatecheck_reply_4.xml")));
+  EXPECT_TRUE(post_interceptor_->ExpectRequest(
+      new PartialMatch("updatecheck"), test_file("updatecheck_reply_4.xml")));
+
+  update_checker_ = UpdateChecker::Create(config_, *metadata_);
+
+  CrxUpdateItem item(BuildCrxUpdateItem());
+  std::vector<CrxUpdateItem*> items_to_check;
+  items_to_check.push_back(&item);
+
+  // Do two update-checks.
+  update_checker_->CheckForUpdates(
+      items_to_check, "extra=\"params\"",
+      base::Bind(&UpdateCheckerTest::UpdateCheckComplete,
+                 base::Unretained(this)));
+  RunThreads();
+  update_checker_ = UpdateChecker::Create(config_, *metadata_);
+  update_checker_->CheckForUpdates(
+      items_to_check, "extra=\"params\"",
+      base::Bind(&UpdateCheckerTest::UpdateCheckComplete,
+                 base::Unretained(this)));
+  RunThreads();
+
+  EXPECT_EQ(2, post_interceptor_->GetHitCount())
+      << post_interceptor_->GetRequestsAsString();
+  ASSERT_EQ(2, post_interceptor_->GetCount())
+      << post_interceptor_->GetRequestsAsString();
+  EXPECT_NE(string::npos,
+            post_interceptor_->GetRequests()[0].find("<ping rd=\"-2\" />"));
+  EXPECT_NE(string::npos,
+            post_interceptor_->GetRequests()[1].find("<ping rd=\"3383\" />"));
+}
+
 }  // namespace update_client

[commit-bot: I haz the power, 2016-04-08 23:13:52]

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

[waffles, 2016-04-08 23:25:42]

The CQ bit was checked by waffles@chromium.org

[waffles, 2016-04-08 23:25:42]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
sorin@chromium.org, asargent@chromium.org, rohitrao@chromium.org
Link to the patchset: https://codereview.chromium.org/1861383004/#ps280001
(title: "Rebase")

[commit-bot: I haz the power, 2016-04-08 23:25:59]

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

[commit-bot: I haz the power, 2016-04-08 23:32:49]

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

[commit-bot: I haz the power, 2016-04-08 23:32:51]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[waffles, 2016-04-09 00:18:43]

The CQ bit was checked by waffles@chromium.org

[waffles, 2016-04-09 00:18:44]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
sorin@chromium.org, asargent@chromium.org, rohitrao@chromium.org
Link to the patchset: https://codereview.chromium.org/1861383004/#ps300001
(title: "Fix merge-added test")

[commit-bot: I haz the power, 2016-04-09 00:19:39]

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

[waffles, 2016-04-09 00:42:13]

The CQ bit was unchecked by waffles@chromium.org

[waffles, 2016-04-09 01:04:55]

The CQ bit was checked by waffles@chromium.org

[waffles, 2016-04-09 01:04:56]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
sorin@chromium.org, asargent@chromium.org, rohitrao@chromium.org
Link to the patchset: https://codereview.chromium.org/1861383004/#ps320001
(title: "Another test fix.")

[commit-bot: I haz the power, 2016-04-09 01:05:13]

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

[commit-bot: I haz the power, 2016-04-09 02:00:06]

Committed patchset #17 (id:320001)

[commit-bot: I haz the power, 2016-04-09 02:01:44]

Description was changed from

==========
Add module for counting date-last-roll-call and persisting those counts
in the user-data-dir.

BUG=590291
==========

to

==========
Add module for counting date-last-roll-call and persisting those counts
in the user-data-dir.

BUG=590291

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

[commit-bot: I haz the power, 2016-04-09 02:01:46]

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