Add stub implementations for state keys and VPD

Add stub implementations for state keys and VPD

Stubbing out state keys and VPD (containing ActivateDate) is required
to test FRE on Linux.

BUG=703658
Review-Url: https://codereview.chromium.org/2774673004
Cr-Commit-Position: refs/heads/master@{#463656}
Committed: https://chromium.googlesource.com/chromium/src/+/9157ba4857d139cbdc161668e2ff6f3bce9ffbcc

[Thiemo Nagel, 2017-03-23 23:04:18]

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

[commit-bot: I haz the power, 2017-03-23 23:05:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Thiemo Nagel, 2017-03-23 23:10:25]

tnagel@chromium.org changed reviewers:
+ derat@chromium.org

[Thiemo Nagel, 2017-03-23 23:10:26]

Hi Dan,

could you please take a look at this CL to increase the testability of the FRE
code?

Thank you and kind regards,
Thiemo

[Daniel Erat, 2017-03-23 23:35:23]

https://codereview.chromium.org/2774673004/diff/1/chromeos/dbus/session_manag...
File chromeos/dbus/session_manager_client.cc (right):

https://codereview.chromium.org/2774673004/diff/1/chromeos/dbus/session_manag...
chromeos/dbus/session_manager_client.cc:79: std::vector<std::string>
ReadCreateStateKeys(const base::FilePath& path) {
please update this function's name to make it clear that it's only used by the
stub implementation.

https://codereview.chromium.org/2774673004/diff/1/chromeos/dbus/session_manag...
chromeos/dbus/session_manager_client.cc:100: std::vector<std::string>
g_state_keys;
i don't think that non-POD static variables are allowed:
https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables

https://codereview.chromium.org/2774673004/diff/1/chromeos/dbus/session_manag...
chromeos/dbus/session_manager_client.cc:103: // thread-safe as long as all
callers are on the same thread.
can you just fix the callback to not take a reference? alternately, can you
store the keys inside of the stub client instead of in a global?

https://codereview.chromium.org/2774673004/diff/1/chromeos/system/statistics_...
File chromeos/system/statistics_provider.cc (right):

https://codereview.chromium.org/2774673004/diff/1/chromeos/system/statistics_...
chromeos/system/statistics_provider.cc:476: LOG(ERROR) << "Error writing machine
info stub: "
nit: PLOG(ERROR) will include errno

https://codereview.chromium.org/2774673004/diff/1/chromeos/system/statistics_...
chromeos/system/statistics_provider.cc:488: LOG(ERROR) << "Error writing vpd
stub: " << vpd_path.value();
PLOG(ERROR) here as well

[commit-bot: I haz the power, 2017-03-23 23:50:30]

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

[commit-bot: I haz the power, 2017-03-23 23:50:31]

Dry run: This issue passed the CQ dry run.

[Thiemo Nagel, 2017-04-11 15:28:27]

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

[commit-bot: I haz the power, 2017-04-11 15:28:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Thiemo Nagel, 2017-04-11 15:29:32]

Sorry for the slow turnaround -- I've been out.  (And perf.)

Could you please take another look?

Thank you,
Thiemo

https://codereview.chromium.org/2774673004/diff/1/chromeos/dbus/session_manag...
File chromeos/dbus/session_manager_client.cc (right):

https://codereview.chromium.org/2774673004/diff/1/chromeos/dbus/session_manag...
chromeos/dbus/session_manager_client.cc:79: std::vector<std::string>
ReadCreateStateKeys(const base::FilePath& path) {
On 2017/03/23 23:35:23, Daniel Erat wrote:
> please update this function's name to make it clear that it's only used by the
> stub implementation.

Done.

(I really dislike the practice of keeping multiple classes in a single .cc file
which gives rise to such ambiguities.)

https://codereview.chromium.org/2774673004/diff/1/chromeos/dbus/session_manag...
chromeos/dbus/session_manager_client.cc:100: std::vector<std::string>
g_state_keys;
On 2017/03/23 23:35:23, Daniel Erat wrote:
> i don't think that non-POD static variables are allowed:

Sorry for that momentary lapse of reason.

https://codereview.chromium.org/2774673004/diff/1/chromeos/dbus/session_manag...
chromeos/dbus/session_manager_client.cc:103: // thread-safe as long as all
callers are on the same thread.
On 2017/03/23 23:35:23, Daniel Erat wrote:
> can you just fix the callback to not take a reference? alternately, can you
> store the keys inside of the stub client instead of in a global?

It's not even necessary to store them anywhere.  As the callback runs
synchronously, passing the reference of a stack variable is fine.  I just had
overlooked that in the first version of the CL.  :\

https://codereview.chromium.org/2774673004/diff/1/chromeos/system/statistics_...
File chromeos/system/statistics_provider.cc (right):

https://codereview.chromium.org/2774673004/diff/1/chromeos/system/statistics_...
chromeos/system/statistics_provider.cc:476: LOG(ERROR) << "Error writing machine
info stub: "
On 2017/03/23 23:35:23, Daniel Erat wrote:
> nit: PLOG(ERROR) will include errno

Done.

https://codereview.chromium.org/2774673004/diff/1/chromeos/system/statistics_...
chromeos/system/statistics_provider.cc:488: LOG(ERROR) << "Error writing vpd
stub: " << vpd_path.value();
On 2017/03/23 23:35:23, Daniel Erat wrote:
> PLOG(ERROR) here as well

Done.

[Daniel Erat, 2017-04-11 15:37:05]

lgtm

[Thiemo Nagel, 2017-04-11 15:58:53]

On 2017/04/11 15:37:05, Daniel Erat wrote:
> lgtm

Thank you!
Thiemo

[commit-bot: I haz the power, 2017-04-11 16:09:19]

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

[commit-bot: I haz the power, 2017-04-11 16:09:19]

Dry run: This issue passed the CQ dry run.

[Thiemo Nagel, 2017-04-11 16:44:05]

The CQ bit was checked by tnagel@chromium.org

[commit-bot: I haz the power, 2017-04-11 16:44:37]

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-04-11 16:56:23]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1491929045068810,
"parent_rev": "0dd660b9ba6e4699468c635874da841a25e135a7", "commit_rev":
"9157ba4857d139cbdc161668e2ff6f3bce9ffbcc"}

[commit-bot: I haz the power, 2017-04-11 16:57:21]

Description was changed from

==========
Add stub implementations for state keys and VPD

Stubbing out state keys and VPD (containing ActivateDate) is required
to test FRE on Linux.

BUG=703658
==========

to

==========
Add stub implementations for state keys and VPD

Stubbing out state keys and VPD (containing ActivateDate) is required
to test FRE on Linux.

BUG=703658

Review-Url: https://codereview.chromium.org/2774673004
Cr-Commit-Position: refs/heads/master@{#463656}
Committed:
https://chromium.googlesource.com/chromium/src/+/9157ba4857d139cbdc161668e2ff...
==========

[commit-bot: I haz the power, 2017-04-11 16:57:22]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/9157ba4857d139cbdc161668e2ff...