Check env-version upon component load

Check env-version upon component load

In Traits::ComponentReady, it fetches env-version from a macro configuration. 
Then compare with min-env-version extracted from 
manifest for compatibility. If compatible, component name is 
saved in a member vector in g_browser_process->platform_part().

When it checks the compatibility of env-version (x.y) and
min-env-version (a.b):

If (x.y) and (a.b) are valid and x==a and y>=b, then its compatible; otherwise
incompatible.

code refactor:
1) reorder definition to be same sequence with declaration.
2) add new InstallCrOSComponent function for easy mocking.
3) more unit test.
4) remove dead code.
5) return void instead of bool in InstallCrOSComponent for consistency.
6) rename parameters for consistency
7) remove unnecessary DVLOG/VLOG and condense comments.

BUG=chromium:690521
TEST=1) unittest, 2) successfully install&load compatible component, 3) fail to load old incompatible component on HD when chrome is updated.

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

[xiaochu, 2017-05-26 00:41:57]

Description was changed from

==========
Check env-version upon component load

It fetches env-version from a macro configuration. It fetches
min-env-version by parsing file
/var/lib/imageloader/<name>/<version>/manifest.

Then it checks the compatibility of env-version (x.y) and
min-env-version (a.b):

If (x.y) and (a.b) are valid and x==a, then its compatible; otherwise
incompatible.

BUG=chromium:690521
TEST=manual
==========

to

==========
Check env-version upon component load

It fetches env-version from a macro configuration. It fetches
min-env-version by parsing file
/var/lib/imageloader/<name>/<version>/manifest.

Then it checks the compatibility of env-version (x.y) and
min-env-version (a.b):

If (x.y) and (a.b) are valid and x==a, then its compatible; otherwise
incompatible.

code refactor:
1) reorder definition to be same sequence with declaration.
2) add new InstallCrOSComponent function for unit test.
3) more unit test.
4) remove dead code.

BUG=chromium:690521
TEST=manual
==========

[xiaochu, 2017-05-26 00:43:02]

Description was changed from

==========
Check env-version upon component load

It fetches env-version from a macro configuration. It fetches
min-env-version by parsing file
/var/lib/imageloader/<name>/<version>/manifest.

Then it checks the compatibility of env-version (x.y) and
min-env-version (a.b):

If (x.y) and (a.b) are valid and x==a, then its compatible; otherwise
incompatible.

code refactor:
1) reorder definition to be same sequence with declaration.
2) add new InstallCrOSComponent function for unit test.
3) more unit test.
4) remove dead code.

BUG=chromium:690521
TEST=manual
==========

to

==========
Check env-version upon component load

It fetches env-version from a macro configuration. It fetches
min-env-version by parsing file
/var/lib/imageloader/<name>/<version>/manifest.

Then it checks the compatibility of env-version (x.y) and
min-env-version (a.b):

If (x.y) and (a.b) are valid and x==a, then its compatible; otherwise
incompatible.

code refactor:
1) reorder definition to be same sequence with declaration.
2) add new InstallCrOSComponent function for easy mocking.
3) more unit test.
4) remove dead code.

BUG=chromium:690521
TEST=manual
==========

[xiaochu, 2017-05-26 01:19:55]

Description was changed from

==========
Check env-version upon component load

It fetches env-version from a macro configuration. It fetches
min-env-version by parsing file
/var/lib/imageloader/<name>/<version>/manifest.

Then it checks the compatibility of env-version (x.y) and
min-env-version (a.b):

If (x.y) and (a.b) are valid and x==a, then its compatible; otherwise
incompatible.

code refactor:
1) reorder definition to be same sequence with declaration.
2) add new InstallCrOSComponent function for easy mocking.
3) more unit test.
4) remove dead code.

BUG=chromium:690521
TEST=manual
==========

to

==========
Check env-version upon component load

It fetches env-version from a macro configuration. It fetches
min-env-version by parsing file
/var/lib/imageloader/<name>/<version>/manifest.

Then it checks the compatibility of env-version (x.y) and
min-env-version (a.b):

If (x.y) and (a.b) are valid and x==a, then its compatible; otherwise
incompatible.

code refactor:
1) reorder definition to be same sequence with declaration.
2) add new InstallCrOSComponent function for easy mocking.
3) more unit test.
4) remove dead code.
5) return void instead of bool in InstallCrOSComponent for consistency.

BUG=chromium:690521
TEST=manual
==========

[xiaochu, 2017-05-26 01:23:30]

Description was changed from

==========
Check env-version upon component load

It fetches env-version from a macro configuration. It fetches
min-env-version by parsing file
/var/lib/imageloader/<name>/<version>/manifest.

Then it checks the compatibility of env-version (x.y) and
min-env-version (a.b):

If (x.y) and (a.b) are valid and x==a, then its compatible; otherwise
incompatible.

code refactor:
1) reorder definition to be same sequence with declaration.
2) add new InstallCrOSComponent function for easy mocking.
3) more unit test.
4) remove dead code.
5) return void instead of bool in InstallCrOSComponent for consistency.

BUG=chromium:690521
TEST=manual
==========

to

==========
Check env-version upon component load

In Traits::ComponentReady, it fetches env-version from a macro configuration.
Then compare with min-env-version extracted from manifest for compatibility. If
compatible, component name is saved in 
 a member vector g_browser_process->platform_part().

Then it checks the compatibility of env-version (x.y) and
min-env-version (a.b):

If (x.y) and (a.b) are valid and x==a, then its compatible; otherwise
incompatible.

code refactor:
1) reorder definition to be same sequence with declaration.
2) add new InstallCrOSComponent function for easy mocking.
3) more unit test.
4) remove dead code.
5) return void instead of bool in InstallCrOSComponent for consistency.

BUG=chromium:690521
TEST=manual
==========

[xiaochu, 2017-05-26 01:23:52]

Description was changed from

==========
Check env-version upon component load

In Traits::ComponentReady, it fetches env-version from a macro configuration.
Then compare with min-env-version extracted from manifest for compatibility. If
compatible, component name is saved in 
 a member vector g_browser_process->platform_part().

Then it checks the compatibility of env-version (x.y) and
min-env-version (a.b):

If (x.y) and (a.b) are valid and x==a, then its compatible; otherwise
incompatible.

code refactor:
1) reorder definition to be same sequence with declaration.
2) add new InstallCrOSComponent function for easy mocking.
3) more unit test.
4) remove dead code.
5) return void instead of bool in InstallCrOSComponent for consistency.

BUG=chromium:690521
TEST=manual
==========

to

==========
Check env-version upon component load

In Traits::ComponentReady, it fetches env-version from a macro configuration.
Then compare with min-env-version extracted from 
manifest for compatibility. If compatible, component name is 
saved in a member vector g_browser_process->platform_part().

Then it checks the compatibility of env-version (x.y) and
min-env-version (a.b):

If (x.y) and (a.b) are valid and x==a, then its compatible; otherwise
incompatible.

code refactor:
1) reorder definition to be same sequence with declaration.
2) add new InstallCrOSComponent function for easy mocking.
3) more unit test.
4) remove dead code.
5) return void instead of bool in InstallCrOSComponent for consistency.

BUG=chromium:690521
TEST=manual
==========

[xiaochu, 2017-05-26 01:24:04]

Description was changed from

==========
Check env-version upon component load

In Traits::ComponentReady, it fetches env-version from a macro configuration.
Then compare with min-env-version extracted from 
manifest for compatibility. If compatible, component name is 
saved in a member vector g_browser_process->platform_part().

Then it checks the compatibility of env-version (x.y) and
min-env-version (a.b):

If (x.y) and (a.b) are valid and x==a, then its compatible; otherwise
incompatible.

code refactor:
1) reorder definition to be same sequence with declaration.
2) add new InstallCrOSComponent function for easy mocking.
3) more unit test.
4) remove dead code.
5) return void instead of bool in InstallCrOSComponent for consistency.

BUG=chromium:690521
TEST=manual
==========

to

==========
Check env-version upon component load

In Traits::ComponentReady, it fetches env-version from a macro configuration. 
Then compare with min-env-version extracted from 
manifest for compatibility. If compatible, component name is 
saved in a member vector g_browser_process->platform_part().

Then it checks the compatibility of env-version (x.y) and
min-env-version (a.b):

If (x.y) and (a.b) are valid and x==a, then its compatible; otherwise
incompatible.

code refactor:
1) reorder definition to be same sequence with declaration.
2) add new InstallCrOSComponent function for easy mocking.
3) more unit test.
4) remove dead code.
5) return void instead of bool in InstallCrOSComponent for consistency.

BUG=chromium:690521
TEST=manual
==========

[xiaochu, 2017-05-26 01:24:26]

Description was changed from

==========
Check env-version upon component load

In Traits::ComponentReady, it fetches env-version from a macro configuration. 
Then compare with min-env-version extracted from 
manifest for compatibility. If compatible, component name is 
saved in a member vector g_browser_process->platform_part().

Then it checks the compatibility of env-version (x.y) and
min-env-version (a.b):

If (x.y) and (a.b) are valid and x==a, then its compatible; otherwise
incompatible.

code refactor:
1) reorder definition to be same sequence with declaration.
2) add new InstallCrOSComponent function for easy mocking.
3) more unit test.
4) remove dead code.
5) return void instead of bool in InstallCrOSComponent for consistency.

BUG=chromium:690521
TEST=manual
==========

to

==========
Check env-version upon component load

In Traits::ComponentReady, it fetches env-version from a macro configuration. 
Then compare with min-env-version extracted from 
manifest for compatibility. If compatible, component name is 
saved in a member vector g_browser_process->platform_part().

When it checks the compatibility of env-version (x.y) and
min-env-version (a.b):

If (x.y) and (a.b) are valid and x==a, then its compatible; otherwise
incompatible.

code refactor:
1) reorder definition to be same sequence with declaration.
2) add new InstallCrOSComponent function for easy mocking.
3) more unit test.
4) remove dead code.
5) return void instead of bool in InstallCrOSComponent for consistency.

BUG=chromium:690521
TEST=manual
==========

[xiaochu, 2017-05-26 01:24:56]

Description was changed from

==========
Check env-version upon component load

In Traits::ComponentReady, it fetches env-version from a macro configuration. 
Then compare with min-env-version extracted from 
manifest for compatibility. If compatible, component name is 
saved in a member vector g_browser_process->platform_part().

When it checks the compatibility of env-version (x.y) and
min-env-version (a.b):

If (x.y) and (a.b) are valid and x==a, then its compatible; otherwise
incompatible.

code refactor:
1) reorder definition to be same sequence with declaration.
2) add new InstallCrOSComponent function for easy mocking.
3) more unit test.
4) remove dead code.
5) return void instead of bool in InstallCrOSComponent for consistency.

BUG=chromium:690521
TEST=manual
==========

to

==========
Check env-version upon component load

In Traits::ComponentReady, it fetches env-version from a macro configuration. 
Then compare with min-env-version extracted from 
manifest for compatibility. If compatible, component name is 
saved in a member vector in g_browser_process->platform_part().

When it checks the compatibility of env-version (x.y) and
min-env-version (a.b):

If (x.y) and (a.b) are valid and x==a, then its compatible; otherwise
incompatible.

code refactor:
1) reorder definition to be same sequence with declaration.
2) add new InstallCrOSComponent function for easy mocking.
3) more unit test.
4) remove dead code.
5) return void instead of bool in InstallCrOSComponent for consistency.

BUG=chromium:690521
TEST=manual
==========

[xiaochu, 2017-05-26 16:28:30]

Description was changed from

==========
Check env-version upon component load

In Traits::ComponentReady, it fetches env-version from a macro configuration. 
Then compare with min-env-version extracted from 
manifest for compatibility. If compatible, component name is 
saved in a member vector in g_browser_process->platform_part().

When it checks the compatibility of env-version (x.y) and
min-env-version (a.b):

If (x.y) and (a.b) are valid and x==a, then its compatible; otherwise
incompatible.

code refactor:
1) reorder definition to be same sequence with declaration.
2) add new InstallCrOSComponent function for easy mocking.
3) more unit test.
4) remove dead code.
5) return void instead of bool in InstallCrOSComponent for consistency.

BUG=chromium:690521
TEST=manual
==========

to

==========
Check env-version upon component load

In Traits::ComponentReady, it fetches env-version from a macro configuration. 
Then compare with min-env-version extracted from 
manifest for compatibility. If compatible, component name is 
saved in a member vector in g_browser_process->platform_part().

When it checks the compatibility of env-version (x.y) and
min-env-version (a.b):

If (x.y) and (a.b) are valid and x==a, then its compatible; otherwise
incompatible.

code refactor:
1) reorder definition to be same sequence with declaration.
2) add new InstallCrOSComponent function for easy mocking.
3) more unit test.
4) remove dead code.
5) return void instead of bool in InstallCrOSComponent for consistency.

BUG=chromium:690521
TEST=1) unittest, 2) successfully install&load compatible component, 3) fail to
load old incompatible component on HD when chrome is updated.
==========

[xiaochu, 2017-05-26 16:33:10]

Description was changed from

==========
Check env-version upon component load

In Traits::ComponentReady, it fetches env-version from a macro configuration. 
Then compare with min-env-version extracted from 
manifest for compatibility. If compatible, component name is 
saved in a member vector in g_browser_process->platform_part().

When it checks the compatibility of env-version (x.y) and
min-env-version (a.b):

If (x.y) and (a.b) are valid and x==a, then its compatible; otherwise
incompatible.

code refactor:
1) reorder definition to be same sequence with declaration.
2) add new InstallCrOSComponent function for easy mocking.
3) more unit test.
4) remove dead code.
5) return void instead of bool in InstallCrOSComponent for consistency.

BUG=chromium:690521
TEST=1) unittest, 2) successfully install&load compatible component, 3) fail to
load old incompatible component on HD when chrome is updated.
==========

to

==========
Check env-version upon component load

In Traits::ComponentReady, it fetches env-version from a macro configuration. 
Then compare with min-env-version extracted from 
manifest for compatibility. If compatible, component name is 
saved in a member vector in g_browser_process->platform_part().

When it checks the compatibility of env-version (x.y) and
min-env-version (a.b):

If (x.y) and (a.b) are valid and x==a, then its compatible; otherwise
incompatible.

code refactor:
1) reorder definition to be same sequence with declaration.
2) add new InstallCrOSComponent function for easy mocking.
3) more unit test.
4) remove dead code.
5) return void instead of bool in InstallCrOSComponent for consistency.
6) rename parameters for consistency

BUG=chromium:690521
TEST=1) unittest, 2) successfully install&load compatible component, 3) fail to
load old incompatible component on HD when chrome is updated.
==========

[xiaochu, 2017-05-26 16:42:23]

Description was changed from

==========
Check env-version upon component load

In Traits::ComponentReady, it fetches env-version from a macro configuration. 
Then compare with min-env-version extracted from 
manifest for compatibility. If compatible, component name is 
saved in a member vector in g_browser_process->platform_part().

When it checks the compatibility of env-version (x.y) and
min-env-version (a.b):

If (x.y) and (a.b) are valid and x==a, then its compatible; otherwise
incompatible.

code refactor:
1) reorder definition to be same sequence with declaration.
2) add new InstallCrOSComponent function for easy mocking.
3) more unit test.
4) remove dead code.
5) return void instead of bool in InstallCrOSComponent for consistency.
6) rename parameters for consistency

BUG=chromium:690521
TEST=1) unittest, 2) successfully install&load compatible component, 3) fail to
load old incompatible component on HD when chrome is updated.
==========

to

==========
Check env-version upon component load

In Traits::ComponentReady, it fetches env-version from a macro configuration. 
Then compare with min-env-version extracted from 
manifest for compatibility. If compatible, component name is 
saved in a member vector in g_browser_process->platform_part().

When it checks the compatibility of env-version (x.y) and
min-env-version (a.b):

If (x.y) and (a.b) are valid and x==a, then its compatible; otherwise
incompatible.

code refactor:
1) reorder definition to be same sequence with declaration.
2) add new InstallCrOSComponent function for easy mocking.
3) more unit test.
4) remove dead code.
5) return void instead of bool in InstallCrOSComponent for consistency.
6) rename parameters for consistency
7) remove unnecessary DVLOG/VLOG and condense comments.

BUG=chromium:690521
TEST=1) unittest, 2) successfully install&load compatible component, 3) fail to
load old incompatible component on HD when chrome is updated.
==========

[xiaochu, 2017-05-26 16:46:11]

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

[xiaochu, 2017-05-26 16:46:12]

Can you please take a look?

[waffles, 2017-05-26 17:28:26]

https://codereview.chromium.org/2911483002/diff/260001/chrome/browser/compone...
File chrome/browser/component_updater/cros_component_installer.cc (right):

https://codereview.chromium.org/2911483002/diff/260001/chrome/browser/compone...
chrome/browser/component_updater/cros_component_installer.cc:161: }
empty line between function definitions

https://codereview.chromium.org/2911483002/diff/260001/chrome/browser/compone...
chrome/browser/component_updater/cros_component_installer.cc:165: // env_version
format: [0-9]+.[0-9]+
Use base::Version, then you can get rid of IsValidEnvVersion.

https://codereview.chromium.org/2911483002/diff/260001/chrome/browser/compone...
chrome/browser/component_updater/cros_component_installer.cc:173: if
(env_version_major == min_env_version_major)
You also need to check that min_env_version_minor <= env_version_minor.

https://codereview.chromium.org/2911483002/diff/260001/chrome/browser/compone...
chrome/browser/component_updater/cros_component_installer.cc:184: }
Recommend you place this definition near the other InstallCrOSComponent.

https://codereview.chromium.org/2911483002/diff/260001/chrome/browser/compone...
chrome/browser/component_updater/cros_component_installer.cc:193: if
(result.empty()) {
Why is this block necessary?

https://codereview.chromium.org/2911483002/diff/260001/chrome/browser/compone...
chrome/browser/component_updater/cros_component_installer.cc:197:
base::PostTask(FROM_HERE, base::Bind(load_callback, result));
Maybe just PostTask(FROM_HERE, base::Bind(load_callback, call_status !=
chromeos::DBUS_METHOD_CALL_SUCCESS ? "" : result))?

https://codereview.chromium.org/2911483002/diff/260001/chrome/browser/compone...
File chrome/browser/component_updater/cros_component_installer.h (right):

https://codereview.chromium.org/2911483002/diff/260001/chrome/browser/compone...
chrome/browser/component_updater/cros_component_installer.h:20:
"1913a5e0a6cad30b6f03e176177e0d7ed62c5d6700a9c66da556d7c3f5d6a47e"}}}};
Define this in the .cc, since that's the only place you need it.

https://codereview.chromium.org/2911483002/diff/260001/chrome/browser/compone...
chrome/browser/component_updater/cros_component_installer.h:94: //   
component_updater:CrOSComponent::LoadCrOSComponent(name, load_callback);
This API is confusing to me; if you always want to call LoadCrOSComponent
anyways, why doesn't InstallCrOSComponent do it for you?

https://codereview.chromium.org/2911483002/diff/260001/chrome/browser/compone...
chrome/browser/component_updater/cros_component_installer.h:100: //
For some reason I didn't notice this comment before. Can we remove it or move it
out of the class definition? The top of this file is a decent place to put usage
information.

https://codereview.chromium.org/2911483002/diff/260001/chrome/browser/compone...
File chrome/browser/component_updater/cros_component_installer_unittest.cc
(right):

https://codereview.chromium.org/2911483002/diff/260001/chrome/browser/compone...
chrome/browser/component_updater/cros_component_installer_unittest.cc:77:
TEST_F(CrOSComponentInstallerTest, ComponentReadyIsCompatible) {
This appears to be failing.

[xiaochu, 2017-05-26 19:44:02]

https://codereview.chromium.org/2911483002/diff/260001/chrome/browser/compone...
File chrome/browser/component_updater/cros_component_installer.cc (right):

https://codereview.chromium.org/2911483002/diff/260001/chrome/browser/compone...
chrome/browser/component_updater/cros_component_installer.cc:161: }
On 2017/05/26 17:28:26, waffles wrote:
> empty line between function definitions

Done.

https://codereview.chromium.org/2911483002/diff/260001/chrome/browser/compone...
chrome/browser/component_updater/cros_component_installer.cc:165: // env_version
format: [0-9]+.[0-9]+
On 2017/05/26 17:28:26, waffles wrote:
> Use base::Version, then you can get rid of IsValidEnvVersion.

Done.

https://codereview.chromium.org/2911483002/diff/260001/chrome/browser/compone...
chrome/browser/component_updater/cros_component_installer.cc:173: if
(env_version_major == min_env_version_major)
On 2017/05/26 17:28:26, waffles wrote:
> You also need to check that min_env_version_minor <= env_version_minor.

Done.

https://codereview.chromium.org/2911483002/diff/260001/chrome/browser/compone...
chrome/browser/component_updater/cros_component_installer.cc:184: }
On 2017/05/26 17:28:26, waffles wrote:
> Recommend you place this definition near the other InstallCrOSComponent.

Done.

I reordered all functions so caller and callee are close together.

https://codereview.chromium.org/2911483002/diff/260001/chrome/browser/compone...
chrome/browser/component_updater/cros_component_installer.cc:193: if
(result.empty()) {
On 2017/05/26 17:28:26, waffles wrote:
> Why is this block necessary?

Done.

https://codereview.chromium.org/2911483002/diff/260001/chrome/browser/compone...
chrome/browser/component_updater/cros_component_installer.cc:197:
base::PostTask(FROM_HERE, base::Bind(load_callback, result));
On 2017/05/26 17:28:26, waffles wrote:
> Maybe just PostTask(FROM_HERE, base::Bind(load_callback, call_status !=
> chromeos::DBUS_METHOD_CALL_SUCCESS ? "" : result))?

Done.

https://codereview.chromium.org/2911483002/diff/260001/chrome/browser/compone...
File chrome/browser/component_updater/cros_component_installer.h (right):

https://codereview.chromium.org/2911483002/diff/260001/chrome/browser/compone...
chrome/browser/component_updater/cros_component_installer.h:20:
"1913a5e0a6cad30b6f03e176177e0d7ed62c5d6700a9c66da556d7c3f5d6a47e"}}}};
On 2017/05/26 17:28:26, waffles wrote:
> Define this in the .cc, since that's the only place you need it.

Done.

https://codereview.chromium.org/2911483002/diff/260001/chrome/browser/compone...
chrome/browser/component_updater/cros_component_installer.h:94: //   
component_updater:CrOSComponent::LoadCrOSComponent(name, load_callback);
On 2017/05/26 17:28:26, waffles wrote:
> This API is confusing to me; if you always want to call LoadCrOSComponent
> anyways, why doesn't InstallCrOSComponent do it for you?

It was originally designed the way you are speaking. But component developers
have concern about network latency (update check, I know it's small) and
availability. So I de-coupled them and give developers more control over what
they are doing...

https://codereview.chromium.org/2911483002/diff/260001/chrome/browser/compone...
chrome/browser/component_updater/cros_component_installer.h:100: //
On 2017/05/26 17:28:26, waffles wrote:
> For some reason I didn't notice this comment before. Can we remove it or move
it
> out of the class definition? The top of this file is a decent place to put
usage
> information.

Done.

https://codereview.chromium.org/2911483002/diff/260001/chrome/browser/compone...
File chrome/browser/component_updater/cros_component_installer_unittest.cc
(right):

https://codereview.chromium.org/2911483002/diff/260001/chrome/browser/compone...
chrome/browser/component_updater/cros_component_installer_unittest.cc:77:
TEST_F(CrOSComponentInstallerTest, ComponentReadyIsCompatible) {
On 2017/05/26 17:28:26, waffles wrote:
> This appears to be failing.

Done.

I forgot to update the env_version number.

[xiaochu, 2017-05-26 20:34:59]

Description was changed from

==========
Check env-version upon component load

In Traits::ComponentReady, it fetches env-version from a macro configuration. 
Then compare with min-env-version extracted from 
manifest for compatibility. If compatible, component name is 
saved in a member vector in g_browser_process->platform_part().

When it checks the compatibility of env-version (x.y) and
min-env-version (a.b):

If (x.y) and (a.b) are valid and x==a, then its compatible; otherwise
incompatible.

code refactor:
1) reorder definition to be same sequence with declaration.
2) add new InstallCrOSComponent function for easy mocking.
3) more unit test.
4) remove dead code.
5) return void instead of bool in InstallCrOSComponent for consistency.
6) rename parameters for consistency
7) remove unnecessary DVLOG/VLOG and condense comments.

BUG=chromium:690521
TEST=1) unittest, 2) successfully install&load compatible component, 3) fail to
load old incompatible component on HD when chrome is updated.
==========

to

==========
Check env-version upon component load

In Traits::ComponentReady, it fetches env-version from a macro configuration. 
Then compare with min-env-version extracted from 
manifest for compatibility. If compatible, component name is 
saved in a member vector in g_browser_process->platform_part().

When it checks the compatibility of env-version (x.y) and
min-env-version (a.b):

If (x.y) and (a.b) are valid and x==a and y>=b, then its compatible; otherwise
incompatible.

code refactor:
1) reorder definition to be same sequence with declaration.
2) add new InstallCrOSComponent function for easy mocking.
3) more unit test.
4) remove dead code.
5) return void instead of bool in InstallCrOSComponent for consistency.
6) rename parameters for consistency
7) remove unnecessary DVLOG/VLOG and condense comments.

BUG=chromium:690521
TEST=1) unittest, 2) successfully install&load compatible component, 3) fail to
load old incompatible component on HD when chrome is updated.
==========

[xiaochu, 2017-05-27 01:01:58]

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

[commit-bot: I haz the power, 2017-05-27 01:02:11]

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-27 01:13:46]

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

[commit-bot: I haz the power, 2017-05-27 01:13:47]

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

[xiaochu, 2017-05-27 02:25:04]

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

[commit-bot: I haz the power, 2017-05-27 02:25:16]

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-27 02:57:07]

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

[commit-bot: I haz the power, 2017-05-27 02:57:07]

Dry run: This issue passed the CQ dry run.

[xiaochu, 2017-05-30 21:23:28]

On 2017/05/26 19:44:02, xiaochu wrote:
>
https://codereview.chromium.org/2911483002/diff/260001/chrome/browser/compone...
> File chrome/browser/component_updater/cros_component_installer.cc (right):
> 
>
https://codereview.chromium.org/2911483002/diff/260001/chrome/browser/compone...
> chrome/browser/component_updater/cros_component_installer.cc:161: }
> On 2017/05/26 17:28:26, waffles wrote:
> > empty line between function definitions
> 
> Done.
> 
>
https://codereview.chromium.org/2911483002/diff/260001/chrome/browser/compone...
> chrome/browser/component_updater/cros_component_installer.cc:165: //
env_version
> format: [0-9]+.[0-9]+
> On 2017/05/26 17:28:26, waffles wrote:
> > Use base::Version, then you can get rid of IsValidEnvVersion.
> 
> Done.
> 
>
https://codereview.chromium.org/2911483002/diff/260001/chrome/browser/compone...
> chrome/browser/component_updater/cros_component_installer.cc:173: if
> (env_version_major == min_env_version_major)
> On 2017/05/26 17:28:26, waffles wrote:
> > You also need to check that min_env_version_minor <= env_version_minor.
> 
> Done.
> 
>
https://codereview.chromium.org/2911483002/diff/260001/chrome/browser/compone...
> chrome/browser/component_updater/cros_component_installer.cc:184: }
> On 2017/05/26 17:28:26, waffles wrote:
> > Recommend you place this definition near the other InstallCrOSComponent.
> 
> Done.
> 
> I reordered all functions so caller and callee are close together.
> 
>
https://codereview.chromium.org/2911483002/diff/260001/chrome/browser/compone...
> chrome/browser/component_updater/cros_component_installer.cc:193: if
> (result.empty()) {
> On 2017/05/26 17:28:26, waffles wrote:
> > Why is this block necessary?
> 
> Done.
> 
>
https://codereview.chromium.org/2911483002/diff/260001/chrome/browser/compone...
> chrome/browser/component_updater/cros_component_installer.cc:197:
> base::PostTask(FROM_HERE, base::Bind(load_callback, result));
> On 2017/05/26 17:28:26, waffles wrote:
> > Maybe just PostTask(FROM_HERE, base::Bind(load_callback, call_status !=
> > chromeos::DBUS_METHOD_CALL_SUCCESS ? "" : result))?
> 
> Done.
> 
>
https://codereview.chromium.org/2911483002/diff/260001/chrome/browser/compone...
> File chrome/browser/component_updater/cros_component_installer.h (right):
> 
>
https://codereview.chromium.org/2911483002/diff/260001/chrome/browser/compone...
> chrome/browser/component_updater/cros_component_installer.h:20:
> "1913a5e0a6cad30b6f03e176177e0d7ed62c5d6700a9c66da556d7c3f5d6a47e"}}}};
> On 2017/05/26 17:28:26, waffles wrote:
> > Define this in the .cc, since that's the only place you need it.
> 
> Done.
> 
>
https://codereview.chromium.org/2911483002/diff/260001/chrome/browser/compone...
> chrome/browser/component_updater/cros_component_installer.h:94: //   
> component_updater:CrOSComponent::LoadCrOSComponent(name, load_callback);
> On 2017/05/26 17:28:26, waffles wrote:
> > This API is confusing to me; if you always want to call LoadCrOSComponent
> > anyways, why doesn't InstallCrOSComponent do it for you?
> 
> It was originally designed the way you are speaking. But component developers
> have concern about network latency (update check, I know it's small) and
> availability. So I de-coupled them and give developers more control over what
> they are doing...
> 
>
https://codereview.chromium.org/2911483002/diff/260001/chrome/browser/compone...
> chrome/browser/component_updater/cros_component_installer.h:100: //
> On 2017/05/26 17:28:26, waffles wrote:
> > For some reason I didn't notice this comment before. Can we remove it or
move
> it
> > out of the class definition? The top of this file is a decent place to put
> usage
> > information.
> 
> Done.
> 
>
https://codereview.chromium.org/2911483002/diff/260001/chrome/browser/compone...
> File chrome/browser/component_updater/cros_component_installer_unittest.cc
> (right):
> 
>
https://codereview.chromium.org/2911483002/diff/260001/chrome/browser/compone...
> chrome/browser/component_updater/cros_component_installer_unittest.cc:77:
> TEST_F(CrOSComponentInstallerTest, ComponentReadyIsCompatible) {
> On 2017/05/26 17:28:26, waffles wrote:
> > This appears to be failing.
> 
> Done.
> 
> I forgot to update the env_version number.

Can you please take a look?

[waffles, 2017-05-30 22:12:17]

https://codereview.chromium.org/2911483002/diff/540001/chrome/browser/compone...
File chrome/browser/component_updater/cros_component_installer.cc (right):

https://codereview.chromium.org/2911483002/diff/540001/chrome/browser/compone...
chrome/browser/component_updater/cros_component_installer.cc:151:
env_version.components()[1] >= min_env_version.components()[1]) {
You can replace the == 2 check with .IsValid and the env[1] >= min_env[1] check
with env > min_env.

https://codereview.chromium.org/2911483002/diff/540001/chrome/browser/compone...
chrome/browser/component_updater/cros_component_installer.cc:154: return false;
IMHO it's better to just return the value of the expression, even though it's a
multi-line expression.

https://codereview.chromium.org/2911483002/diff/540001/chrome/browser/compone...
File chrome/browser/component_updater/cros_component_installer.h (right):

https://codereview.chromium.org/2911483002/diff/540001/chrome/browser/compone...
chrome/browser/component_updater/cros_component_installer.h:33: //   
component_updater:CrOSComponent::LoadCrOSComponent(name, load_callback);
Sorry, I'm still confused about this API. We have Install and Load. What exactly
is the difference between these two?

Am I right that:

Install: Go get the latest thing from the network.
Load: Go get the latest thing without the network.

Is there any reason to not just have:
Load: Go get the latest thing without the network (if available), else get the
latest thing from the network?

https://codereview.chromium.org/2911483002/diff/540001/chrome/browser/compone...
chrome/browser/component_updater/cros_component_installer.h:117: public:
Group all public members together. Same for private.

[xiaochu, 2017-05-31 05:06:08]

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

[commit-bot: I haz the power, 2017-05-31 05:06:28]

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-31 05:51:20]

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

[commit-bot: I haz the power, 2017-05-31 05:51:21]

Dry run: This issue passed the CQ dry run.

[xiaochu, 2017-05-31 15:40:11]

Thanks! I left only Load API and fixed the order and logic of the functions
accordingly to make them clear.

https://codereview.chromium.org/2911483002/diff/540001/chrome/browser/compone...
File chrome/browser/component_updater/cros_component_installer.cc (right):

https://codereview.chromium.org/2911483002/diff/540001/chrome/browser/compone...
chrome/browser/component_updater/cros_component_installer.cc:151:
env_version.components()[1] >= min_env_version.components()[1]) {
On 2017/05/30 22:12:17, waffles wrote:
> You can replace the == 2 check with .IsValid and the env[1] >= min_env[1]
check
> with env > min_env.

Done.

https://codereview.chromium.org/2911483002/diff/540001/chrome/browser/compone...
chrome/browser/component_updater/cros_component_installer.cc:154: return false;
On 2017/05/30 22:12:17, waffles wrote:
> IMHO it's better to just return the value of the expression, even though it's
a
> multi-line expression.

Done.

https://codereview.chromium.org/2911483002/diff/540001/chrome/browser/compone...
File chrome/browser/component_updater/cros_component_installer.h (right):

https://codereview.chromium.org/2911483002/diff/540001/chrome/browser/compone...
chrome/browser/component_updater/cros_component_installer.h:33: //   
component_updater:CrOSComponent::LoadCrOSComponent(name, load_callback);
On 2017/05/30 22:12:17, waffles wrote:
> Sorry, I'm still confused about this API. We have Install and Load. What
exactly
> is the difference between these two?
> 
> Am I right that:
> 
> Install: Go get the latest thing from the network.
> Load: Go get the latest thing without the network.
> 
> Is there any reason to not just have:
> Load: Go get the latest thing without the network (if available), else get the
> latest thing from the network?

Done.

This makes sense. Thanks!

https://codereview.chromium.org/2911483002/diff/540001/chrome/browser/compone...
chrome/browser/component_updater/cros_component_installer.h:117: public:
On 2017/05/30 22:12:17, waffles wrote:
> Group all public members together. Same for private.

Done.

[xiaochu, 2017-05-31 23:22:20]

xiaochu@chromium.org changed reviewers:
+ ketakid@chromium.org

[xiaochu, 2017-05-31 23:22:43]

xiaochu@chromium.org changed reviewers:
+ adlr@chromium.org

[xiaochu, 2017-05-31 23:45:06]

xiaochu@chromium.org changed reviewers:
- adlr@chromium.org, ketakid@chromium.org

[xiaochu, 2017-05-31 23:45:07]

Can you please take a look?

[waffles, 2017-06-01 00:08:14]

https://codereview.chromium.org/2911483002/diff/700001/chrome/browser/compone...
File chrome/browser/component_updater/cros_component_installer.cc (right):

https://codereview.chromium.org/2911483002/diff/700001/chrome/browser/compone...
chrome/browser/component_updater/cros_component_installer.cc:155:
InstallComponent(name, load_callback);
Seems simpler to just call the three-argument version here and get rid of the
two-argument version.

https://codereview.chromium.org/2911483002/diff/700001/chrome/browser/compone...
chrome/browser/component_updater/cros_component_installer.cc:218: if
(g_browser_process->platform_part()->IsCompatibleCrOSComponent(name)) {
This check seems redundant with the check the caller performs. You could DCHECK
it if you want.

https://codereview.chromium.org/2911483002/diff/700001/chrome/browser/compone...
File chrome/browser/component_updater/cros_component_installer.h (right):

https://codereview.chromium.org/2911483002/diff/700001/chrome/browser/compone...
chrome/browser/component_updater/cros_component_installer.h:23: //    if
(mount_point.empty) {
empty()?

https://codereview.chromium.org/2911483002/diff/700001/chrome/browser/compone...
chrome/browser/component_updater/cros_component_installer.h:87: class
CrOSComponent {
This class has only static members, can we avoid using a class? Move all private
methods into an anonymous namespace in the .cc file, and move the public method
into a named namespace.

[xiaochu, 2017-06-01 02:57:29]

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

[commit-bot: I haz the power, 2017-06-01 02:58:00]

Dry run: CQ is trying da patch.

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

[xiaochu, 2017-06-01 02:58:10]

Thanks! I have some questions asked inline.

https://codereview.chromium.org/2911483002/diff/700001/chrome/browser/compone...
File chrome/browser/component_updater/cros_component_installer.cc (right):

https://codereview.chromium.org/2911483002/diff/700001/chrome/browser/compone...
chrome/browser/component_updater/cros_component_installer.cc:155:
InstallComponent(name, load_callback);
On 2017/06/01 00:08:14, waffles wrote:
> Seems simpler to just call the three-argument version here and get rid of the
> two-argument version.

Done.

https://codereview.chromium.org/2911483002/diff/700001/chrome/browser/compone...
chrome/browser/component_updater/cros_component_installer.cc:218: if
(g_browser_process->platform_part()->IsCompatibleCrOSComponent(name)) {
On 2017/06/01 00:08:14, waffles wrote:
> This check seems redundant with the check the caller performs. You could
DCHECK
> it if you want.

I am checking this since otherwise it might load an incompatible component?

https://codereview.chromium.org/2911483002/diff/700001/chrome/browser/compone...
File chrome/browser/component_updater/cros_component_installer.h (right):

https://codereview.chromium.org/2911483002/diff/700001/chrome/browser/compone...
chrome/browser/component_updater/cros_component_installer.h:23: //    if
(mount_point.empty) {
On 2017/06/01 00:08:14, waffles wrote:
> empty()?

Done.

https://codereview.chromium.org/2911483002/diff/700001/chrome/browser/compone...
chrome/browser/component_updater/cros_component_installer.h:87: class
CrOSComponent {
On 2017/06/01 00:08:14, waffles wrote:
> This class has only static members, can we avoid using a class? Move all
private
> methods into an anonymous namespace in the .cc file, and move the public
method
> into a named namespace.

We are adding this class in order to make it friend for OnDemandUpdater
(https://cs.chromium.org/chromium/src/components/component_updater/component_u...).


Please advise if this needs to change.

[commit-bot: I haz the power, 2017-06-01 04:17:32]

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

[commit-bot: I haz the power, 2017-06-01 04:17:33]

Dry run: This issue passed the CQ dry run.

[xiaochu, 2017-06-01 16:26:50]

Missed one in last reply.

https://codereview.chromium.org/2911483002/diff/700001/chrome/browser/compone...
File chrome/browser/component_updater/cros_component_installer.cc (right):

https://codereview.chromium.org/2911483002/diff/700001/chrome/browser/compone...
chrome/browser/component_updater/cros_component_installer.cc:218: if
(g_browser_process->platform_part()->IsCompatibleCrOSComponent(name)) {
On 2017/06/01 02:58:09, xiaochu wrote:
> On 2017/06/01 00:08:14, waffles wrote:
> > This check seems redundant with the check the caller performs. You could
> DCHECK
> > it if you want.
> 
> I am checking this since otherwise it might load an incompatible component? 

Done.
Sorry about last message. I get it now.

[xiaochu, 2017-06-01 16:27:01]

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

[commit-bot: I haz the power, 2017-06-01 16:27:34]

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

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

[commit-bot: I haz the power, 2017-06-01 17:20:57]

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

[waffles, 2017-06-01 22:42:41]

https://codereview.chromium.org/2911483002/diff/700001/chrome/browser/compone...
File chrome/browser/component_updater/cros_component_installer.h (right):

https://codereview.chromium.org/2911483002/diff/700001/chrome/browser/compone...
chrome/browser/component_updater/cros_component_installer.h:87: class
CrOSComponent {
On 2017/06/01 02:58:10, xiaochu wrote:
> On 2017/06/01 00:08:14, waffles wrote:
> > This class has only static members, can we avoid using a class? Move all
> private
> > methods into an anonymous namespace in the .cc file, and move the public
> method
> > into a named namespace.
> 
> We are adding this class in order to make it friend for OnDemandUpdater
>
(https://cs.chromium.org/chromium/src/components/component_updater/component_u...).
> 
> 
> Please advise if this needs to change.

Ah! I forgot about that. Can we just friend the public method (or create a
public adapter method in another .h), then? Let me know if this will complicate
your testing too much - it's just unfortunate to expose all of these
implementation details in the header. At minimum, some of the private methods
could be moved out of the class.

https://codereview.chromium.org/2911483002/diff/740001/chrome/browser/compone...
File chrome/browser/component_updater/cros_component_installer.h (right):

https://codereview.chromium.org/2911483002/diff/740001/chrome/browser/compone...
chrome/browser/component_updater/cros_component_installer.h:98: // Install a
component with a dedicated ComponentUpdateServie instance.
ComponentUpdateServie→ComponentUpdateService

https://codereview.chromium.org/2911483002/diff/740001/chrome/browser/compone...
chrome/browser/component_updater/cros_component_installer.h:112: // It call
LoadComponentInternal to load the installed component.
call→calls

https://codereview.chromium.org/2911483002/diff/740001/chrome/browser/compone...
chrome/browser/component_updater/cros_component_installer.h:121: // It returns
load result.
The function returns void, maybe elaborate on what you mean here?

[xiaochu, 2017-06-02 03:54:40]

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

[commit-bot: I haz the power, 2017-06-02 03:54:55]

Dry run: CQ is trying da patch.

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

[xiaochu, 2017-06-02 04:42:15]

Thanks! One question is inlined.

https://codereview.chromium.org/2911483002/diff/700001/chrome/browser/compone...
File chrome/browser/component_updater/cros_component_installer.h (right):

https://codereview.chromium.org/2911483002/diff/700001/chrome/browser/compone...
chrome/browser/component_updater/cros_component_installer.h:87: class
CrOSComponent {
On 2017/06/01 22:42:41, waffles wrote:
> On 2017/06/01 02:58:10, xiaochu wrote:
> > On 2017/06/01 00:08:14, waffles wrote:
> > > This class has only static members, can we avoid using a class? Move all
> > private
> > > methods into an anonymous namespace in the .cc file, and move the public
> > method
> > > into a named namespace.
> > 
> > We are adding this class in order to make it friend for OnDemandUpdater
> >
>
(https://cs.chromium.org/chromium/src/components/component_updater/component_u...).
> > 
> > 
> > Please advise if this needs to change.
> 
> Ah! I forgot about that. Can we just friend the public method (or create a
> public adapter method in another .h), then? Let me know if this will
complicate
> your testing too much - it's just unfortunate to expose all of these
> implementation details in the header. At minimum, some of the private methods
> could be moved out of the class.

Done.

There are 3 functions left in header:
LoadComponent - public API
RegisterResult - which calls onDemandUpdate
InstallComponent - it accepts a CUS object (mocked) so my unittest could work.

Unfotunately, public API is not what calls OnDemandUpdate so caller has to be
exposed (if not CrOSComponent class name). I can remove that particular unittest
and hide InstallComponent as well... Please advise.

https://codereview.chromium.org/2911483002/diff/740001/chrome/browser/compone...
File chrome/browser/component_updater/cros_component_installer.h (right):

https://codereview.chromium.org/2911483002/diff/740001/chrome/browser/compone...
chrome/browser/component_updater/cros_component_installer.h:98: // Install a
component with a dedicated ComponentUpdateServie instance.
On 2017/06/01 22:42:41, waffles wrote:
> ComponentUpdateServie→ComponentUpdateService

Done.

https://codereview.chromium.org/2911483002/diff/740001/chrome/browser/compone...
chrome/browser/component_updater/cros_component_installer.h:112: // It call
LoadComponentInternal to load the installed component.
On 2017/06/01 22:42:41, waffles wrote:
> call→calls

Done.

https://codereview.chromium.org/2911483002/diff/740001/chrome/browser/compone...
chrome/browser/component_updater/cros_component_installer.h:121: // It returns
load result.
On 2017/06/01 22:42:41, waffles wrote:
> The function returns void, maybe elaborate on what you mean here?

Done.

[commit-bot: I haz the power, 2017-06-02 05:07:38]

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

[commit-bot: I haz the power, 2017-06-02 05:07:39]

Dry run: This issue passed the CQ dry run.

[xiaochu, 2017-06-05 18:24:00]

Can you please take a look? thanks!

[waffles, 2017-06-05 18:43:44]

lgtm

https://codereview.chromium.org/2911483002/diff/760001/chrome/browser/compone...
File chrome/browser/component_updater/cros_component_installer.cc (right):

https://codereview.chromium.org/2911483002/diff/760001/chrome/browser/compone...
chrome/browser/component_updater/cros_component_installer.cc:147: env_version >=
min_env_version;
I thought you still needed to check that env_version[0] == min_env_version[0].

https://codereview.chromium.org/2911483002/diff/760001/chrome/browser/compone...
chrome/browser/component_updater/cros_component_installer.cc:153: 
Remove empty line.

[xiaochu, 2017-06-05 19:09:29]

On 2017/06/05 18:43:44, waffles wrote:
> lgtm
> 
>
https://codereview.chromium.org/2911483002/diff/760001/chrome/browser/compone...
> File chrome/browser/component_updater/cros_component_installer.cc (right):
> 
>
https://codereview.chromium.org/2911483002/diff/760001/chrome/browser/compone...
> chrome/browser/component_updater/cros_component_installer.cc:147: env_version
>=
> min_env_version;
> I thought you still needed to check that env_version[0] == min_env_version[0].
> 
>
https://codereview.chromium.org/2911483002/diff/760001/chrome/browser/compone...
> chrome/browser/component_updater/cros_component_installer.cc:153: 
> Remove empty line.

Thanks!

[xiaochu, 2017-06-05 19:09:32]

The CQ bit was checked by xiaochu@chromium.org

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

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:27:17]

The CQ bit was unchecked by waffles@chromium.org

[waffles, 2017-06-05 19:28:08]

On 2017/06/05 19:09:58, commit-bot: I haz the power wrote:
> CQ is trying da patch.
> 
> Follow status at:
>
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

Sorry, make sure you address the two comments before you commit.

[xiaochu, 2017-06-05 19:34:58]

I apologize for that. My phone hides the detailed comments from me...

https://codereview.chromium.org/2911483002/diff/760001/chrome/browser/compone...
File chrome/browser/component_updater/cros_component_installer.cc (right):

https://codereview.chromium.org/2911483002/diff/760001/chrome/browser/compone...
chrome/browser/component_updater/cros_component_installer.cc:147: env_version >=
min_env_version;
On 2017/06/05 18:43:44, waffles wrote:
> I thought you still needed to check that env_version[0] == min_env_version[0].

Done.

https://codereview.chromium.org/2911483002/diff/760001/chrome/browser/compone...
chrome/browser/component_updater/cros_component_installer.cc:153: 
On 2017/06/05 18:43:44, waffles wrote:
> Remove empty line.

Done.

[xiaochu, 2017-06-05 19:49:52]

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

[commit-bot: I haz the power, 2017-06-05 19:50:22]

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 23:05:00]

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

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

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[xiaochu, 2017-06-05 23:13:58]

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

[commit-bot: I haz the power, 2017-06-05 23:14:40]

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-06 03:19:24]

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

[commit-bot: I haz the power, 2017-06-06 03:19:25]

Dry run: This issue passed the CQ dry run.

[xiaochu, 2017-06-06 16:13:39]

The CQ bit was checked by xiaochu@chromium.org

[xiaochu, 2017-06-06 16:13:39]

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/2911483002/#ps780001
(title: "address comments from waffles")

[commit-bot: I haz the power, 2017-06-06 16:13:57]

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-06 16:18:33]

CQ is committing da patch.

Bot data: {"patchset_id": 780001, "attempt_start_ts": 1496765619048260,
"parent_rev": "a7103a79f3d2aacc2854c21ede4bf442c80afae8", "commit_rev":
"fd9d71980637bf89994f61476acd7760da72612d"}

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

Description was changed from

==========
Check env-version upon component load

In Traits::ComponentReady, it fetches env-version from a macro configuration. 
Then compare with min-env-version extracted from 
manifest for compatibility. If compatible, component name is 
saved in a member vector in g_browser_process->platform_part().

When it checks the compatibility of env-version (x.y) and
min-env-version (a.b):

If (x.y) and (a.b) are valid and x==a and y>=b, then its compatible; otherwise
incompatible.

code refactor:
1) reorder definition to be same sequence with declaration.
2) add new InstallCrOSComponent function for easy mocking.
3) more unit test.
4) remove dead code.
5) return void instead of bool in InstallCrOSComponent for consistency.
6) rename parameters for consistency
7) remove unnecessary DVLOG/VLOG and condense comments.

BUG=chromium:690521
TEST=1) unittest, 2) successfully install&load compatible component, 3) fail to
load old incompatible component on HD when chrome is updated.
==========

to

==========
Check env-version upon component load

In Traits::ComponentReady, it fetches env-version from a macro configuration. 
Then compare with min-env-version extracted from 
manifest for compatibility. If compatible, component name is 
saved in a member vector in g_browser_process->platform_part().

When it checks the compatibility of env-version (x.y) and
min-env-version (a.b):

If (x.y) and (a.b) are valid and x==a and y>=b, then its compatible; otherwise
incompatible.

code refactor:
1) reorder definition to be same sequence with declaration.
2) add new InstallCrOSComponent function for easy mocking.
3) more unit test.
4) remove dead code.
5) return void instead of bool in InstallCrOSComponent for consistency.
6) rename parameters for consistency
7) remove unnecessary DVLOG/VLOG and condense comments.

BUG=chromium:690521
TEST=1) unittest, 2) successfully install&load compatible component, 3) fail to
load old incompatible component on HD when chrome is updated.

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

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

Committed patchset #41 (id:780001) as
https://chromium.googlesource.com/chromium/src/+/fd9d71980637bf89994f61476acd...