Prepare to abstract PersistedData by making it part of the configurator.

Prepare to abstract PersistedData by making it part of the configurator.

The next step is to make PersistedData an abstract class so that UpdateClient
can support globally persisted data and specific user profile data.

BUG=722942

[Minh X. Nguyen, 2017-05-09 01:06:48]

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

[mxnguyen, 2017-05-09 01:17:19]

The CQ bit was unchecked by mxnguyen@google.com

[mxnguyen, 2017-05-09 01:17:20]

The CQ bit was checked by mxnguyen@google.com

[mxnguyen, 2017-05-09 01:17:21]

The CQ bit was unchecked by mxnguyen@google.com

[mxnguyen, 2017-05-09 01:17:47]

The CQ bit was checked by mxnguyen@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-09 01:18:12]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-09 01:37:25]

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

[commit-bot: I haz the power, 2017-05-09 01:37:26]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Minh X. Nguyen, 2017-05-09 01:40:59]

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

[commit-bot: I haz the power, 2017-05-09 01:42:31]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-09 02:26:22]

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

[commit-bot: I haz the power, 2017-05-09 02:26:23]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Minh X. Nguyen, 2017-05-09 17:04:24]

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

[commit-bot: I haz the power, 2017-05-09 17:05:31]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-09 17:16:11]

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

[commit-bot: I haz the power, 2017-05-09 17:16:12]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[Minh X. Nguyen, 2017-05-09 17:25:04]

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

[commit-bot: I haz the power, 2017-05-09 17:26:11]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Minh X. Nguyen, 2017-05-09 17:53:13]

Description was changed from

==========
Prepare to abstract PersistedData by making it part of the configurator.

The next step is to make PersistedData an abstract class so that UpdateClient
can support globally persisted data and specific user profile data.

BUG=
==========

to

==========
Prepare to abstract PersistedData by making it part of the configurator.

The next step is to make PersistedData an abstract class so that UpdateClient
can support globally persisted data and specific user profile data.

BUG=
==========

[Minh X. Nguyen, 2017-05-09 17:53:13]

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

[Minh X. Nguyen, 2017-05-09 17:54:26]

This is the first step, I break it down into 2 patches so that it's easier to
review.

The next step is to make PersistedData an abstract class so that UpdateClient
can support globally persisted data and specific user profile data.

[commit-bot: I haz the power, 2017-05-09 19:32:22]

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

[commit-bot: I haz the power, 2017-05-09 19:32:23]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Minh X. Nguyen, 2017-05-09 20:00:35]

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

[commit-bot: I haz the power, 2017-05-09 20:01:53]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-10 02:04:25]

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

[commit-bot: I haz the power, 2017-05-10 02:04:26]

Dry run: This issue passed the CQ dry run.

[waffles, 2017-05-12 17:33:34]

https://codereview.chromium.org/2873533002/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/updater/chrome_update_client_config.h (right):

https://codereview.chromium.org/2873533002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/updater/chrome_update_client_config.h:49:
update_client::PersistedData* CreateMetadata() const override;
Can we return a unique_ptr instead?

https://codereview.chromium.org/2873533002/diff/60001/components/update_clien...
File components/update_client/update_checker.cc (right):

https://codereview.chromium.org/2873533002/diff/60001/components/update_clien...
components/update_client/update_checker.cc:188: : config_(config),
metadata_(config->CreateMetadata()) {}
This will be called once per update check - I think this is OK, but it's
probably important to document that PersistedData itself can't have any state
that is supposed to live across updatechecks.

[Sorin Jianu, 2017-05-12 18:17:22]

I will take a look after the current set of comments is addressed.

Thank you!

[Minh X. Nguyen, 2017-05-12 21:06:28]

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

[commit-bot: I haz the power, 2017-05-12 21:07:45]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-12 22:31:11]

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

[commit-bot: I haz the power, 2017-05-12 22:31:13]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Minh X. Nguyen, 2017-05-14 23:56:50]

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

[commit-bot: I haz the power, 2017-05-14 23:57:10]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Minh X. Nguyen, 2017-05-14 23:57:22]

https://codereview.chromium.org/2873533002/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/updater/chrome_update_client_config.h (right):

https://codereview.chromium.org/2873533002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/updater/chrome_update_client_config.h:49:
update_client::PersistedData* CreateMetadata() const override;
On 2017/05/12 17:33:34, waffles wrote:
> Can we return a unique_ptr instead?

Done.

https://codereview.chromium.org/2873533002/diff/60001/components/update_clien...
File components/update_client/update_checker.cc (right):

https://codereview.chromium.org/2873533002/diff/60001/components/update_clien...
components/update_client/update_checker.cc:188: : config_(config),
metadata_(config->CreateMetadata()) {}
On 2017/05/12 17:33:34, waffles wrote:
> This will be called once per update check - I think this is OK, but it's
> probably important to document that PersistedData itself can't have any state
> that is supposed to live across updatechecks.

As discussed offline, it's better to store PersistedData in UpdateEngine.

[commit-bot: I haz the power, 2017-05-15 00:04:21]

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

[commit-bot: I haz the power, 2017-05-15 00:04:22]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[waffles, 2017-05-15 17:52:20]

lgtm

https://codereview.chromium.org/2873533002/diff/100001/components/update_clie...
File components/update_client/update_engine.cc (right):

https://codereview.chromium.org/2873533002/diff/100001/components/update_clie...
components/update_client/update_engine.cc:61:
metadata_(std::move(config->CreateMetadata())),
remove std::move. (moving a temporary object prevents copy elision)

[Minh X. Nguyen, 2017-05-15 18:57:11]

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

[commit-bot: I haz the power, 2017-05-15 18:58:12]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-15 19:14:54]

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

[commit-bot: I haz the power, 2017-05-15 19:14:55]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)

[Minh X. Nguyen, 2017-05-15 19:36:35]

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

[commit-bot: I haz the power, 2017-05-15 19:37:23]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-15 21:54:27]

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

[commit-bot: I haz the power, 2017-05-15 21:54:28]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Minh X. Nguyen, 2017-05-15 21:58:49]

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

[commit-bot: I haz the power, 2017-05-15 21:59:38]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Sorin Jianu, 2017-05-15 22:19:56]

lgtm

Thank you!

[Sorin Jianu, 2017-05-15 22:19:58]

lgtm

Thank you!

[commit-bot: I haz the power, 2017-05-15 23:34:45]

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

[commit-bot: I haz the power, 2017-05-15 23:34:46]

Dry run: This issue passed the CQ dry run.

[Sorin Jianu, 2017-05-16 00:00:50]

The CQ bit was checked by sorin@chromium.org

[Sorin Jianu, 2017-05-16 00:00:50]

The patchset sent to the CQ was uploaded after l-g-t-m from waffles@chromium.org
Link to the patchset: https://codereview.chromium.org/2873533002/#ps140001
(title: "Fix ios compile error.")

[commit-bot: I haz the power, 2017-05-16 00:01:46]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-16 00:20:06]

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

[commit-bot: I haz the power, 2017-05-16 00:20:08]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Minh X. Nguyen, 2017-05-16 02:03:33]

mxnguyen@chromium.org changed reviewers:
+ asargent@chromium.org, cbentzel@chromium.org

[cbentzel, 2017-05-16 18:13:18]

Was there a specific reason I was added as a reviewer on this CL? Not that I see
a problem, just not sure if you were looking for my feedback on something in
particular.

[mxnguyen, 2017-05-16 18:18:02]

On 2017/05/16 18:13:18, cbentzel wrote:
> Was there a specific reason I was added as a reviewer on this CL? Not that I
see
> a problem, just not sure if you were looking for my feedback on something in
> particular.

I made a change in
ios/chrome/browser/component_updater/ios_component_updater_configurator.cc, thus
someone from ios/chrome/browswer owners would have to approve my change. Your
name just happens to appear first in the owner list. Can you suggest someone who
can review my patch? Thanks.

[Minh X. Nguyen, 2017-05-16 18:35:34]

Description was changed from

==========
Prepare to abstract PersistedData by making it part of the configurator.

The next step is to make PersistedData an abstract class so that UpdateClient
can support globally persisted data and specific user profile data.

BUG=
==========

to

==========
Prepare to abstract PersistedData by making it part of the configurator.

The next step is to make PersistedData an abstract class so that UpdateClient
can support globally persisted data and specific user profile data.

BUG=722942
==========

[Sorin Jianu, 2017-05-16 18:41:21]

sorin@chromium.org changed reviewers:
+ rdcronin@google.com, sdefresne@chromium.org
- asargent@chromium.org, cbentzel@chromium.org

[Sorin Jianu, 2017-05-16 18:43:55]

sorin@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org
- rdcronin@google.com

[Sorin Jianu, 2017-05-16 18:43:55]

Sylvain, could you please review and approve the changes under the ios path?
thank you!

Devlin, could you please review and approve the changes under the
chrome/browser/extensions/ path? thank you!

Moving Chris to CC list.

Thank you so much!

[Devlin, 2017-05-16 23:03:09]

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

https://codereview.chromium.org/2873533002/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/updater/chrome_update_client_config.cc:5: #include
"chrome/browser/extensions/updater/chrome_update_client_config.h"
I think git cl format messed this up - please put a newline after this.

https://codereview.chromium.org/2873533002/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/updater/chrome_update_client_config.cc:126: return
base::MakeUnique<update_client::PersistedData>(GetPrefService());
nitty nit: include-what-you-use: #include "base/memory/ptr_util.h"

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

https://codereview.chromium.org/2873533002/diff/140001/components/update_clie...
components/update_client/configurator.h:130: virtual PrefService*
GetPrefService() const = 0;
A quick codesearch indicates that this is only used in
components/update_client/update_engine.cc where we created the persisted data. 
Since that's abstracted out, can we remove this?  Or is this actually used
elsewhere that's not showing up?

[sdefresne, 2017-05-17 09:57:39]

ios/ lgtm

[Minh X. Nguyen, 2017-06-01 18:26:22]

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

[commit-bot: I haz the power, 2017-06-01 18:27:05]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-01 18:35:50]

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

[commit-bot: I haz the power, 2017-06-01 18:35:52]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Minh X. Nguyen, 2017-06-01 20:32:34]

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

[commit-bot: I haz the power, 2017-06-01 20:32:56]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-01 20:32:58]

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

[commit-bot: I haz the power, 2017-06-01 20:32:59]

Dry run: CQ cannot get SignCLA result. Please try later.

[Minh X. Nguyen, 2017-06-01 20:36:44]

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

[commit-bot: I haz the power, 2017-06-01 20:37:18]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Minh X. Nguyen, 2017-06-01 21:04:19]

Sorry for the late response, I was OOO for the last 2 weeks.

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

https://codereview.chromium.org/2873533002/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/updater/chrome_update_client_config.cc:5: #include
"chrome/browser/extensions/updater/chrome_update_client_config.h"
On 2017/05/16 23:03:09, Devlin wrote:
> I think git cl format messed this up - please put a newline after this.

Done.

https://codereview.chromium.org/2873533002/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/updater/chrome_update_client_config.cc:126: return
base::MakeUnique<update_client::PersistedData>(GetPrefService());
On 2017/05/16 23:03:09, Devlin wrote:
> nitty nit: include-what-you-use: #include "base/memory/ptr_util.h"

Done.

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

https://codereview.chromium.org/2873533002/diff/140001/components/update_clie...
components/update_client/configurator.h:130: virtual PrefService*
GetPrefService() const = 0;
On 2017/05/16 23:03:09, Devlin wrote:
> A quick codesearch indicates that this is only used in
> components/update_client/update_engine.cc where we created the persisted data.

> Since that's abstracted out, can we remove this?  Or is this actually used
> elsewhere that's not showing up?

Done.

[commit-bot: I haz the power, 2017-06-01 22:41:19]

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

[commit-bot: I haz the power, 2017-06-01 22:41:20]

Dry run: This issue passed the CQ dry run.

[Devlin, 2017-06-05 16:13:38]

lgtm!

https://codereview.chromium.org/2873533002/diff/180001/components/update_clie...
File components/update_client/test_configurator.h (right):

https://codereview.chromium.org/2873533002/diff/180001/components/update_clie...
components/update_client/test_configurator.h:113: PrefService* pref_;
nit: maybe s/pref_/pref_service_?  A pref is something returned by the pref
service.

[Minh X. Nguyen, 2017-06-05 18:16:28]

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

[Minh X. Nguyen, 2017-06-05 18:16:38]

https://codereview.chromium.org/2873533002/diff/180001/components/update_clie...
File components/update_client/test_configurator.h (right):

https://codereview.chromium.org/2873533002/diff/180001/components/update_clie...
components/update_client/test_configurator.h:113: PrefService* pref_;
On 2017/06/05 16:13:38, Devlin wrote:
> nit: maybe s/pref_/pref_service_?  A pref is something returned by the pref
> service.

Done.

[commit-bot: I haz the power, 2017-06-05 18:16:45]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-05 19:33:58]

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

[commit-bot: I haz the power, 2017-06-05 19:33:59]

Dry run: This issue passed the CQ dry run.

[Minh X. Nguyen, 2017-06-05 19:45:53]

The CQ bit was checked by mxnguyen@chromium.org

[Minh X. Nguyen, 2017-06-05 19:45:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
waffles@chromium.org, sorin@chromium.org, sdefresne@chromium.org,
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/2873533002/#ps200001
(title: "Rename pref_ to pref_service_.")

[commit-bot: I haz the power, 2017-06-05 19:46:35]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[waffles, 2017-06-05 19:48:15]

The CQ bit was unchecked by waffles@chromium.org

[sdefresne, 2017-06-15 13:29:03]

mxnguyen: ping?