Add updater state for Mac.

Detect updater state for Mac (version, health) and attempt to understand
if Chrome was launched by an enterprise user (domain joined).

Review-Url: https://codereview.chromium.org/2862723002
Cr-Commit-Position: refs/heads/master@{#470622}
Committed: https://chromium.googlesource.com/chromium/src/+/a97d530f854c213b4b37b7e363fc341e2c44e773

[borisv, 2017-05-03 23:29:49]

The CQ bit was checked by borisv@google.com

[borisv, 2017-05-03 23:29:50]

The CQ bit was unchecked by borisv@google.com

[borisv, 2017-05-03 23:37:31]

The CQ bit was checked by borisv@google.com

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

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-03 23:38:17]

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

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

No L-G-T-M from a valid reviewer yet.
CQ run can only be started once the patch has received an L-G-T-M from a full
committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a
full super star committer.
Committers are members of the group "project-chromium-committers".
Note that this has nothing to do with OWNERS files.

[borisv, 2017-05-04 00:23:57]

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

[borisv, 2017-05-04 00:23:58]

Sorin, please take a look at the change. Note that I am dragging a system
library dependency (OpenDirectory.framework) and there is some objective-C code.
Let me know if this should be reviewed by the Mac colleagues too.

[Sorin Jianu, 2017-05-04 19:53:59]

Boris, please ask someone to do a review for the .mm code, for style as well as
correctness.

Thank you!

https://codereview.chromium.org/2862723002/diff/40001/components/update_clien...
File components/update_client/updater_state.h (right):

https://codereview.chromium.org/2862723002/diff/40001/components/update_clien...
components/update_client/updater_state.h:50: #if defined(OS_WIN)
can we avoid conditional compilation in this file? Can we ignore the is_machine
parameter on macOS?

https://codereview.chromium.org/2862723002/diff/40001/components/update_clien...
File components/update_client/updater_state_mac.mm (right):

https://codereview.chromium.org/2862723002/diff/40001/components/update_clien...
components/update_client/updater_state_mac.mm:6: #import
<OpenDirectory/OpenDirectory.h>
Usually, an empty line is inserted between 6 and 7.

https://codereview.chromium.org/2862723002/diff/40001/components/update_clien...
components/update_client/updater_state_mac.mm:17: const base::FilePath::CharType
kKeystonePlist[] = FILE_PATH_LITERAL(
Namespace formatting: please include an empty line between 16 and 17.

https://codereview.chromium.org/2862723002/diff/40001/components/update_clien...
components/update_client/updater_state_mac.mm:23: // documentation for
tooll-free bridging.
toll

[Sorin Jianu, 2017-05-04 19:56:04]

Description was changed from

==========
Add updater state for Mac.


Merge branch 'master' of https://chromium.googlesource.com/chromium/src


Minor fixes on the comments: - Removed unnecessary comment in KeystoneGlue's
updateProfileCountsWithNumProfiles implementation - Fixed the spelling of
Keystone in ChromeBrowserMainPartsMac::PostProfileInit's implementation


Ensure that failure to load the profiles does not cause crash.

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

to

==========
Add updater state for Mac.


Merge branch 'master' of https://chromium.googlesource.com/chromium/src


Minor fixes on the comments: - Removed unnecessary comment in KeystoneGlue's
updateProfileCountsWithNumProfiles implementation - Fixed the spelling of
Keystone in ChromeBrowserMainPartsMac::PostProfileInit's implementation


Ensure that failure to load the profiles does not cause crash.

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

[Sorin Jianu, 2017-05-04 19:56:04]

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

[borisv, 2017-05-04 22:19:26]

borisv@google.com changed reviewers:
+ shrike@chromium.org

[borisv, 2017-05-04 22:19:27]

shrike@, please take a look from Mac point of view.

Sorin, thank you for the code review. I responded to the conditional compilation
suggestion and fixed the formatting/typos.

https://codereview.chromium.org/2862723002/diff/40001/components/update_clien...
File components/update_client/updater_state.h (right):

https://codereview.chromium.org/2862723002/diff/40001/components/update_clien...
components/update_client/updater_state.h:50: #if defined(OS_WIN)
On 2017/05/04 19:53:58, Sorin Jianu wrote:
> can we avoid conditional compilation in this file? Can we ignore the
is_machine
> parameter on macOS?

We have to user conditional anyway anyway for the  wchar_t* and char*
difference. I don't believe that Mac knows wchar_t at all. Alternatively, we can
drag an additional header to define it as base::FilePath::CharType. Or we can
try forward declaration of base::FilePath::CharType and then include that header
in updater_state_win.cc. 

A third option (my preference) is that we pull this method out of the class
definition at all. As this is a static method, it can very well be defined
locally for the _win and _mac implementation files.

https://codereview.chromium.org/2862723002/diff/40001/components/update_clien...
File components/update_client/updater_state_mac.mm (right):

https://codereview.chromium.org/2862723002/diff/40001/components/update_clien...
components/update_client/updater_state_mac.mm:6: #import
<OpenDirectory/OpenDirectory.h>
On 2017/05/04 19:53:59, Sorin Jianu wrote:
> Usually, an empty line is inserted between 6 and 7.

Done.

https://codereview.chromium.org/2862723002/diff/40001/components/update_clien...
components/update_client/updater_state_mac.mm:17: const base::FilePath::CharType
kKeystonePlist[] = FILE_PATH_LITERAL(
On 2017/05/04 19:53:59, Sorin Jianu wrote:
> Namespace formatting: please include an empty line between 16 and 17.

Done.

https://codereview.chromium.org/2862723002/diff/40001/components/update_clien...
components/update_client/updater_state_mac.mm:23: // documentation for
tooll-free bridging.
On 2017/05/04 19:53:59, Sorin Jianu wrote:
> toll

Done.

[Boris Vidolov, 2017-05-04 22:21:55]

borisv@chromium.org changed reviewers:
- borisv@google.com

[Boris Vidolov, 2017-05-04 22:21:55]

(cleaning up reviewers)

[Sorin Jianu, 2017-05-04 22:50:49]

Thank you Boris!

https://codereview.chromium.org/2862723002/diff/40001/components/update_clien...
File components/update_client/updater_state.h (right):

https://codereview.chromium.org/2862723002/diff/40001/components/update_clien...
components/update_client/updater_state.h:50: #if defined(OS_WIN)
On 2017/05/04 22:19:27, borisv wrote:
> On 2017/05/04 19:53:58, Sorin Jianu wrote:
> > can we avoid conditional compilation in this file? Can we ignore the
> is_machine
> > parameter on macOS?
> 
> We have to user conditional anyway anyway for the  wchar_t* and char*
> difference. I don't believe that Mac knows wchar_t at all. Alternatively, we
can
> drag an additional header to define it as base::FilePath::CharType. Or we can
> try forward declaration of base::FilePath::CharType and then include that
header
> in updater_state_win.cc. 
> 
> A third option (my preference) is that we pull this method out of the class
> definition at all. As this is a static method, it can very well be defined
> locally for the _win and _mac implementation files. 

wchar_t is a standard type.

Also, I can change the code to take a char literal, if that is better
accommodated, please tell me which one would you prefer.

[Boris Vidolov, 2017-05-05 16:43:28]

On 2017/05/04 22:50:49, Sorin Jianu wrote:
> Thank you Boris!
> 
>
https://codereview.chromium.org/2862723002/diff/40001/components/update_clien...
> File components/update_client/updater_state.h (right):
> 
>
https://codereview.chromium.org/2862723002/diff/40001/components/update_clien...
> components/update_client/updater_state.h:50: #if defined(OS_WIN)
> On 2017/05/04 22:19:27, borisv wrote:
> > On 2017/05/04 19:53:58, Sorin Jianu wrote:
> > > can we avoid conditional compilation in this file? Can we ignore the
> > is_machine
> > > parameter on macOS?
> > 
> > We have to user conditional anyway anyway for the  wchar_t* and char*
> > difference. I don't believe that Mac knows wchar_t at all. Alternatively, we
> can
> > drag an additional header to define it as base::FilePath::CharType. Or we
can
> > try forward declaration of base::FilePath::CharType and then include that
> header
> > in updater_state_win.cc. 
> > 
> > A third option (my preference) is that we pull this method out of the class
> > definition at all. As this is a static method, it can very well be defined
> > locally for the _win and _mac implementation files. 
> 
> wchar_t is a standard type.
> 
> Also, I can change the code to take a char literal, if that is better
> accommodated, please tell me which one would you prefer.

I removed the mac-specific declaration and placed it in the _mac.mm
implementation file.

[Boris Vidolov, 2017-05-05 16:44:07]

Minor updates to avoid conditional compilation in updater_state.h

PTAL

[Robert Sesek, 2017-05-05 21:53:55]

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

[Robert Sesek, 2017-05-05 21:53:56]

Some drive-by comments :)

https://codereview.chromium.org/2862723002/diff/80001/components/update_clien...
File components/update_client/updater_state_mac.mm (right):

https://codereview.chromium.org/2862723002/diff/80001/components/update_clien...
components/update_client/updater_state_mac.mm:27: T*
CopyUpdaterSettingsValue(NSString* value_name) {
Can this return a base::scoped_nsobject<T> to make ownership explicit, and
cleanup free?

https://codereview.chromium.org/2862723002/diff/80001/components/update_clien...
components/update_client/updater_state_mac.mm:45: @autoreleasepool {
Use base::mac::ScopedNSAutoreleasePool from
base/mac/scoped_nsautorelease_pool.h.

https://codereview.chromium.org/2862723002/diff/80001/components/update_clien...
components/update_client/updater_state_mac.mm:84: base::Version systemKeystone =
GetVersionFromPlist(system_bundle_plist);
naming: system_keystone

https://codereview.chromium.org/2862723002/diff/80001/components/update_clien...
components/update_client/updater_state_mac.mm:118: @autoreleasepool {
ScopedNSAutoreleasePool

https://codereview.chromium.org/2862723002/diff/80001/components/update_clien...
components/update_client/updater_state_mac.mm:165: NSString *attributeValue =
naming: attribute_value

https://codereview.chromium.org/2862723002/diff/80001/components/update_clien...
components/update_client/updater_state_mac.mm:165: NSString *attributeValue =
nit: * goes next to NSString

[shrike, 2017-05-05 23:13:45]

borisv@ - rsesek@ is an excellent person to review your code for Mac style (and
it looks like he already has taken a look), so I will bow out of reviewing.

[Boris Vidolov, 2017-05-06 00:31:15]

Thank you, Robert. I added the suggestions and also removed a possible object
leak if the retrieved data is of unexpected type.

PTAL

https://codereview.chromium.org/2862723002/diff/80001/components/update_clien...
File components/update_client/updater_state_mac.mm (right):

https://codereview.chromium.org/2862723002/diff/80001/components/update_clien...
components/update_client/updater_state_mac.mm:27: T*
CopyUpdaterSettingsValue(NSString* value_name) {
On 2017/05/05 21:53:56, Robert Sesek wrote:
> Can this return a base::scoped_nsobject<T> to make ownership explicit, and
> cleanup free?

Done.

https://codereview.chromium.org/2862723002/diff/80001/components/update_clien...
components/update_client/updater_state_mac.mm:45: @autoreleasepool {
On 2017/05/05 21:53:56, Robert Sesek wrote:
> Use base::mac::ScopedNSAutoreleasePool from
> base/mac/scoped_nsautorelease_pool.h.

Done.

https://codereview.chromium.org/2862723002/diff/80001/components/update_clien...
components/update_client/updater_state_mac.mm:84: base::Version systemKeystone =
GetVersionFromPlist(system_bundle_plist);
On 2017/05/05 21:53:55, Robert Sesek wrote:
> naming: system_keystone

Done.

https://codereview.chromium.org/2862723002/diff/80001/components/update_clien...
components/update_client/updater_state_mac.mm:118: @autoreleasepool {
On 2017/05/05 21:53:56, Robert Sesek wrote:
> ScopedNSAutoreleasePool

Done.

https://codereview.chromium.org/2862723002/diff/80001/components/update_clien...
components/update_client/updater_state_mac.mm:165: NSString *attributeValue =
On 2017/05/05 21:53:55, Robert Sesek wrote:
> nit: * goes next to NSString

Done.

https://codereview.chromium.org/2862723002/diff/80001/components/update_clien...
components/update_client/updater_state_mac.mm:165: NSString *attributeValue =
On 2017/05/05 21:53:55, Robert Sesek wrote:
> nit: * goes next to NSString

Done.

[Sorin Jianu, 2017-05-06 00:37:23]

lgtm

Thank you Boris!
Everything not .mm lgtm

[Robert Sesek, 2017-05-08 17:14:13]

LGTM. Please also edit the CL description to describe the change.

https://codereview.chromium.org/2862723002/diff/100001/components/update_clie...
File components/update_client/updater_state_mac.mm (right):

https://codereview.chromium.org/2862723002/diff/100001/components/update_clie...
components/update_client/updater_state_mac.mm:31: CFPropertyListRef plist =
Switch this to a base::ScopedCFTypeRef and then you can remove line 37.

[Boris Vidolov, 2017-05-08 18:16:50]

Description was changed from

==========
Add updater state for Mac.


Merge branch 'master' of https://chromium.googlesource.com/chromium/src


Minor fixes on the comments: - Removed unnecessary comment in KeystoneGlue's
updateProfileCountsWithNumProfiles implementation - Fixed the spelling of
Keystone in ChromeBrowserMainPartsMac::PostProfileInit's implementation


Ensure that failure to load the profiles does not cause crash.

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

to

==========
Detect updater state for Mac (version, health) and attempt to understand if
Chrome was launched by an enterprise user (domain joined).


Merge branch 'master' of https://chromium.googlesource.com/chromium/src


Minor fixes on the comments: - Removed unnecessary comment in KeystoneGlue's
updateProfileCountsWithNumProfiles implementation - Fixed the spelling of
Keystone in ChromeBrowserMainPartsMac::PostProfileInit's implementation


Ensure that failure to load the profiles does not cause crash.

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

[Boris Vidolov, 2017-05-08 18:18:07]

borisv@chromium.org changed reviewers:
- shrike@chromium.org

[Boris Vidolov, 2017-05-08 18:18:07]

Thank you, Robert. I changed the code to use ScopedCFTypeRef object - the whole
method is simpler now.

https://codereview.chromium.org/2862723002/diff/100001/components/update_clie...
File components/update_client/updater_state_mac.mm (right):

https://codereview.chromium.org/2862723002/diff/100001/components/update_clie...
components/update_client/updater_state_mac.mm:31: CFPropertyListRef plist =
On 2017/05/08 17:14:13, Robert Sesek wrote:
> Switch this to a base::ScopedCFTypeRef and then you can remove line 37.

Thanks, Robert. One more scoped pointer type that I was not aware of :). Good
suggestion - I threw away a few more lines of code that dealt with releasing and
checking for NULL.

[Robert Sesek, 2017-05-08 22:20:41]

LGTM. Please do clean up the CL description on Rietveld before landing, though.

[Boris Vidolov, 2017-05-08 23:30:23]

The CQ bit was checked by borisv@chromium.org

[Boris Vidolov, 2017-05-08 23:30:24]

The patchset sent to the CQ was uploaded after l-g-t-m from sorin@chromium.org,
rsesek@chromium.org
Link to the patchset: https://codereview.chromium.org/2862723002/#ps160001
(title: "Merge with master branch brought new name of a method.")

[Boris Vidolov, 2017-05-08 23:31:11]

Description was changed from

==========
Detect updater state for Mac (version, health) and attempt to understand if
Chrome was launched by an enterprise user (domain joined).


Merge branch 'master' of https://chromium.googlesource.com/chromium/src


Minor fixes on the comments: - Removed unnecessary comment in KeystoneGlue's
updateProfileCountsWithNumProfiles implementation - Fixed the spelling of
Keystone in ChromeBrowserMainPartsMac::PostProfileInit's implementation


Ensure that failure to load the profiles does not cause crash.

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

to

==========
Detect updater state for Mac (version, health) and attempt to understand if
Chrome was launched by an enterprise user (domain joined).
==========

[commit-bot: I haz the power, 2017-05-08 23:35:49]

CQ is trying da patch.

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

[Robert Sesek, 2017-05-09 00:10:10]

Description was changed from

==========
Detect updater state for Mac (version, health) and attempt to understand if
Chrome was launched by an enterprise user (domain joined).
==========

to

==========
Add updater state for Mac.

Detect updater state for Mac (version, health) and attempt to understand
if Chrome was launched by an enterprise user (domain joined).
==========

[commit-bot: I haz the power, 2017-05-09 00:45:54]

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

[commit-bot: I haz the power, 2017-05-09 00:45:55]

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_...)

[Boris Vidolov, 2017-05-09 04:11:44]

The CQ bit was checked by borisv@chromium.org

[commit-bot: I haz the power, 2017-05-09 04:12:27]

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 04:28:52]

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

[commit-bot: I haz the power, 2017-05-09 04:28:53]

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...)

[Boris Vidolov, 2017-05-09 19:26:18]

Description was changed from

==========
Add updater state for Mac.

Detect updater state for Mac (version, health) and attempt to understand
if Chrome was launched by an enterprise user (domain joined).
==========

to

==========
Detect updater state for Mac (version, health) and attempt to understand
if Chrome was launched by an enterprise user (domain joined).
==========

[Boris Vidolov, 2017-05-09 19:26:23]

The CQ bit was checked by borisv@chromium.org

[Boris Vidolov, 2017-05-09 19:26:23]

The patchset sent to the CQ was uploaded after l-g-t-m from sorin@chromium.org,
rsesek@chromium.org
Link to the patchset: https://codereview.chromium.org/2862723002/#ps180001
(title: "Exclude iOS from the updater state logic.")

[Robert Sesek, 2017-05-09 19:26:53]

Description was changed from

==========
Detect updater state for Mac (version, health) and attempt to understand
if Chrome was launched by an enterprise user (domain joined).
==========

to

==========
Add updater state for Mac.

Detect updater state for Mac (version, health) and attempt to understand
if Chrome was launched by an enterprise user (domain joined).
==========

[commit-bot: I haz the power, 2017-05-09 19:27:47]

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 20:17:02]

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

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

Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[Boris Vidolov, 2017-05-09 21:24:53]

The CQ bit was checked by borisv@chromium.org

[commit-bot: I haz the power, 2017-05-09 21:25:54]

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 01:00:45]

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

[commit-bot: I haz the power, 2017-05-10 01:00:47]

Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet;
builder either lacks capacity or does not exist (misspelled?))

[Robert Sesek, 2017-05-10 01:18:23]

The CQ bit was checked by rsesek@chromium.org

[Robert Sesek, 2017-05-10 01:18:39]

The CQ bit was unchecked by rsesek@chromium.org

[Boris Vidolov, 2017-05-10 16:11:33]

The CQ bit was checked by borisv@chromium.org

[Boris Vidolov, 2017-05-10 16:12:12]

Description was changed from

==========
Add updater state for Mac.

Detect updater state for Mac (version, health) and attempt to understand
if Chrome was launched by an enterprise user (domain joined).
==========

to

==========
Detect updater state for Mac (version, health) and attempt to understand
if Chrome was launched by an enterprise user (domain joined).
==========

[commit-bot: I haz the power, 2017-05-10 16:12:39]

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 17:11:59]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1494432693044990,
"parent_rev": "011afda700511a9c30c7256cda8d5bc2e4b62fe8", "commit_rev":
"a97d530f854c213b4b37b7e363fc341e2c44e773"}

[commit-bot: I haz the power, 2017-05-10 17:12:07]

Description was changed from

==========
Detect updater state for Mac (version, health) and attempt to understand
if Chrome was launched by an enterprise user (domain joined).
==========

to

==========
Detect updater state for Mac (version, health) and attempt to understand
if Chrome was launched by an enterprise user (domain joined).

Review-Url: https://codereview.chromium.org/2862723002
Cr-Commit-Position: refs/heads/master@{#470622}
Committed:
https://chromium.googlesource.com/chromium/src/+/a97d530f854c213b4b37b7e363fc...
==========

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

Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/a97d530f854c213b4b37b7e363fc...