Add a comment on PRODUCT_STRING_PATH usage in chrome_elf.

Add a comment on PRODUCT_STRING_PATH usage in chrome_elf.

BUG=577820
Committed: https://crrev.com/7bb3b72511b381a990f2e04c11541d16ede459c5
Cr-Commit-Position: refs/heads/master@{#371446}

[gab, 2016-01-25 18:50:52]

Patchset #1 (id:1) has been deleted

[gab, 2016-01-25 18:52:00]

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

[gab, 2016-01-25 18:52:00]

Robert PTAL, adding explicit comment per discussion @ http://crbug.com/577820#c3

[robertshield, 2016-01-25 22:21:03]

https://codereview.chromium.org/1634793002/diff/20001/chrome_elf/chrome_elf_c...
File chrome_elf/chrome_elf_constants.cc (right):

https://codereview.chromium.org/1634793002/diff/20001/chrome_elf/chrome_elf_c...
chrome_elf/chrome_elf_constants.cc:8: // Using PRODUCT_STRING_PATH is wrong as
it makes Google Chrome and Canary
Suggest "Using PRODUCT_STRING_PATH will make Google Chrome and Canary
configurations collide. This was deemed to be the best option due to ELF's
inability to depend on BrowserDistribution. http://crbug.com/577820".

It's only wrong if there's a better overall option, which it doesn't look like
there is in this case.

[gab, 2016-01-26 02:14:56]

https://codereview.chromium.org/1634793002/diff/20001/chrome_elf/chrome_elf_c...
File chrome_elf/chrome_elf_constants.cc (right):

https://codereview.chromium.org/1634793002/diff/20001/chrome_elf/chrome_elf_c...
chrome_elf/chrome_elf_constants.cc:8: // Using PRODUCT_STRING_PATH is wrong as
it makes Google Chrome and Canary
On 2016/01/25 22:21:03, robertshield wrote:
> Suggest "Using PRODUCT_STRING_PATH will make Google Chrome and Canary
> configurations collide. This was deemed to be the best option due to ELF's
> inability to depend on BrowserDistribution. http://crbug.com/577820%22.

Done.

> 
> It's only wrong if there's a better overall option, which it doesn't look like
> there is in this case.

The "better" option is to turn the portion of installer_util which is not chrome
specific into a component, but that is far from done, so yea no clean short term
solution (short of duplicating more code to figure out SxS in ELF).

Given I assume the BL's config is fairly stable and would only be used to
rollback all channels in case of emergency it's probably okay.

[robertshield, 2016-01-26 03:41:35]

LGTM

On 2016/01/26 02:14:56, gab wrote:
>
https://codereview.chromium.org/1634793002/diff/20001/chrome_elf/chrome_elf_c...
> File chrome_elf/chrome_elf_constants.cc (right):
> 
>
https://codereview.chromium.org/1634793002/diff/20001/chrome_elf/chrome_elf_c...
> chrome_elf/chrome_elf_constants.cc:8: // Using PRODUCT_STRING_PATH is wrong as
> it makes Google Chrome and Canary
> On 2016/01/25 22:21:03, robertshield wrote:
> > Suggest "Using PRODUCT_STRING_PATH will make Google Chrome and Canary
> > configurations collide. This was deemed to be the best option due to ELF's
> > inability to depend on BrowserDistribution. http://crbug.com/577820%22.
> 
> Done.
> 
> > 
> > It's only wrong if there's a better overall option, which it doesn't look
like
> > there is in this case.
> 
> The "better" option is to turn the portion of installer_util which is not
chrome
> specific into a component, but that is far from done, so yea no clean short
term
> solution (short of duplicating more code to figure out SxS in ELF).
> 
> Given I assume the BL's config is fairly stable and would only be used to
> rollback all channels in case of emergency it's probably okay.

That assumption is mostly correct. The path here is also used to control the
"successful startup beacon", which can disable the blacklist if set incorrectly
(possible if both channels frequently raced at startup and one was encountering
problems). This is mostly fine as the blacklist is really just a best effort
stability thing anyway, and it's not terribly robust against things that really
want to find their way in.

[gab, 2016-01-26 03:47:29]

The CQ bit was checked by gab@chromium.org

[commit-bot: I haz the power, 2016-01-26 03:50:31]

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

[commit-bot: I haz the power, 2016-01-26 03:58:18]

Committed patchset #2 (id:40001)

[commit-bot: I haz the power, 2016-01-26 03:59:43]

Description was changed from

==========
Add a comment on PRODUCT_STRING_PATH usage in chrome_elf.

BUG=577820
==========

to

==========
Add a comment on PRODUCT_STRING_PATH usage in chrome_elf.

BUG=577820

Committed: https://crrev.com/7bb3b72511b381a990f2e04c11541d16ede459c5
Cr-Commit-Position: refs/heads/master@{#371446}
==========

[commit-bot: I haz the power, 2016-01-26 03:59:44]

Patchset 2 (id:??) landed as
https://crrev.com/7bb3b72511b381a990f2e04c11541d16ede459c5
Cr-Commit-Position: refs/heads/master@{#371446}