Test app launcher registration in test_installer rather than InstallationValidator.

Test app launcher registration in test_installer rather than InstallationValidator.

BUG=474398
Committed: https://crrev.com/b9e58ee85ea09c4abd142c3020ac1ca9101d9d0c
Cr-Commit-Position: refs/heads/master@{#324249}

[grt (UTC plus 2), 2015-04-07 18:32:16]

grt@chromium.org changed reviewers:
+ gab@chromium.org

[grt (UTC plus 2), 2015-04-07 18:32:35]

PTAL

[grt (UTC plus 2), 2015-04-08 12:47:30]

On 2015/04/07 18:32:35, grt wrote:
> PTAL

Ping! This is blocking a bot. Please check the CQ box if it looks good. I'm
happy to address minor comments in a follow-up cl. Thannks.

[gab, 2015-04-08 14:28:29]

https://codereview.chromium.org/1069003002/diff/1/chrome/test/mini_installer/...
File chrome/test/mini_installer/config/chrome_system_installed.prop (right):

https://codereview.chromium.org/1069003002/diff/1/chrome/test/mini_installer/...
chrome/test/mini_installer/config/chrome_system_installed.prop:29: "condition":
"$SUPPORTS_SXS",
This is testing only under SxS while the old code was testing only when NOT
under SxS?

Why is the app launcher even SxS dependent?

https://codereview.chromium.org/1069003002/diff/1/chrome/test/mini_installer/...
File chrome/test/mini_installer/verifier.py (right):

https://codereview.chromium.org/1069003002/diff/1/chrome/test/mini_installer/...
chrome/test/mini_installer/verifier.py:52: return eval(condition,
{'__builtins__': {'False': False, 'True': True}})
This change looks like it makes sense to me, but I'm curious why this is needed
in this CL? i.e. if it's required, how did similar statement to the ones added
in this CL pass before?

[grt (UTC plus 2), 2015-04-08 14:35:40]

Thanks.

https://codereview.chromium.org/1069003002/diff/1/chrome/test/mini_installer/...
File chrome/test/mini_installer/config/chrome_system_installed.prop (right):

https://codereview.chromium.org/1069003002/diff/1/chrome/test/mini_installer/...
chrome/test/mini_installer/config/chrome_system_installed.prop:29: "condition":
"$SUPPORTS_SXS",
On 2015/04/08 14:28:29, gab wrote:
> This is testing only under SxS while the old code was testing only when NOT
> under SxS?

Not quite. This is testing non-SxS Chrome when SxS is supported (i.e., this is
Google Chrome rather than Chromium). The chrome_canary_*.prop files are for SxS
Chrome.

> Why is the app launcher even SxS dependent?

Neither Chromium (where SxS is not supported) nor SxS Chrome (canary) track
usage of the app launcher by this appguid.

https://codereview.chromium.org/1069003002/diff/1/chrome/test/mini_installer/...
File chrome/test/mini_installer/verifier.py (right):

https://codereview.chromium.org/1069003002/diff/1/chrome/test/mini_installer/...
chrome/test/mini_installer/verifier.py:52: return eval(condition,
{'__builtins__': {'False': False, 'True': True}})
On 2015/04/08 14:28:29, gab wrote:
> This change looks like it makes sense to me, but I'm curious why this is
needed
> in this CL? i.e. if it's required, how did similar statement to the ones added
> in this CL pass before?

Prior to this change, $SUPPORTS_SXS wasn't evaluated in a condition in this
codepath's context. This is now consistent with the evaluation in
test_installer.py. It should have been updated a few revs ago when the other was
changed, but was missed since it wasn't needed at the time.

[gab, 2015-04-08 15:00:58]

https://codereview.chromium.org/1069003002/diff/1/chrome/test/mini_installer/...
File chrome/test/mini_installer/config/chrome_system_installed.prop (right):

https://codereview.chromium.org/1069003002/diff/1/chrome/test/mini_installer/...
chrome/test/mini_installer/config/chrome_system_installed.prop:29: "condition":
"$SUPPORTS_SXS",
On 2015/04/08 14:35:40, grt wrote:
> On 2015/04/08 14:28:29, gab wrote:
> > This is testing only under SxS while the old code was testing only when NOT
> > under SxS?
> 
> Not quite. This is testing non-SxS Chrome when SxS is supported (i.e., this is
> Google Chrome rather than Chromium). The chrome_canary_*.prop files are for
SxS
> Chrome.
> 
> > Why is the app launcher even SxS dependent?
> 
> Neither Chromium (where SxS is not supported) nor SxS Chrome (canary) track
> usage of the app launcher by this appguid.

I see, so this has nothing to do with SxS really... Can we introduce something
like $SUPPORTS_APP_LAUNCHER or even more generic $IS_GOOGLE_CHROME? Feels like
relying on $SUPPORTS_SXS here is just a hack depending on a correlation between
two mostly independent logical elements.

[grt (UTC plus 2), 2015-04-08 15:11:10]

https://codereview.chromium.org/1069003002/diff/1/chrome/test/mini_installer/...
File chrome/test/mini_installer/config/chrome_system_installed.prop (right):

https://codereview.chromium.org/1069003002/diff/1/chrome/test/mini_installer/...
chrome/test/mini_installer/config/chrome_system_installed.prop:29: "condition":
"$SUPPORTS_SXS",
On 2015/04/08 15:00:58, gab wrote:
> On 2015/04/08 14:35:40, grt wrote:
> > On 2015/04/08 14:28:29, gab wrote:
> > > This is testing only under SxS while the old code was testing only when
NOT
> > > under SxS?
> > 
> > Not quite. This is testing non-SxS Chrome when SxS is supported (i.e., this
is
> > Google Chrome rather than Chromium). The chrome_canary_*.prop files are for
> SxS
> > Chrome.
> > 
> > > Why is the app launcher even SxS dependent?
> > 
> > Neither Chromium (where SxS is not supported) nor SxS Chrome (canary) track
> > usage of the app launcher by this appguid.
> 
> I see, so this has nothing to do with SxS really... Can we introduce something
> like $SUPPORTS_APP_LAUNCHER or even more generic $IS_GOOGLE_CHROME? Feels like
> relying on $SUPPORTS_SXS here is just a hack depending on a correlation
between
> two mostly independent logical elements.

Good point. I've made it conditional on Google Chrome, testing by name. Whaddya
think?

[gab, 2015-04-08 15:17:16]

lgtm, would be nice to be able to add comments in those property files eh..!

[grt (UTC plus 2), 2015-04-08 15:18:56]

On 2015/04/08 15:17:16, gab wrote:
> lgtm, would be nice to be able to add comments in those property files eh..!

no kidding. i've thought of adding a comment field to the json format for this.
probably worth doing. it could be emitted in failure logs, possibly making
failure diagnosis easier. i'll leave that for another day. :-)

thanks for the review. now get back to speed! :-)

[grt (UTC plus 2), 2015-04-08 15:56:29]

The CQ bit was checked by grt@chromium.org

[grt (UTC plus 2), 2015-04-08 15:56:29]

The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org
Link to the patchset: https://codereview.chromium.org/1069003002/#ps40001
(title: "oh, python")

[commit-bot: I haz the power, 2015-04-08 15:57:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1069003002/40001

[commit-bot: I haz the power, 2015-04-08 17:35:30]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-04-08 17:36:21]

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