Add fixing of MSI DisplayVersion to Chrome installs/updates.

Add fixing of MSI DisplayVersion to Chrome installs/updates.

BUG=67348

[robertshield, 2014-11-26 20:21:38]

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

[robertshield, 2014-11-26 20:21:39]

MSI tomfoolery. Please take a look.

[grt (UTC plus 2), 2014-11-27 19:42:04]

i can't imagine doing this any better. mainly some nits.

https://codereview.chromium.org/749133003/diff/40001/chrome/installer/setup/i...
File chrome/installer/setup/install_worker.cc (right):

https://codereview.chromium.org/749133003/diff/40001/chrome/installer/setup/i...
chrome/installer/setup/install_worker.cc:97: // http://crbug.com/111058 to be
fixed.
please add to the doc comment that this is a format string and what the
substituted value must be.

https://codereview.chromium.org/749133003/diff/40001/chrome/installer/setup/i...
chrome/installer/setup/install_worker.cc:706: base::string16* product_id) {
why the out param for this? why not return the product id or an empty string on
failure?

https://codereview.chromium.org/749133003/diff/40001/chrome/installer/setup/i...
chrome/installer/setup/install_worker.cc:713: while (value_iter.Valid()) {
nit:
  for (; value_iter.Valid(); ++value_iter) {
and remove line 721

https://codereview.chromium.org/749133003/diff/40001/chrome/installer/setup/i...
chrome/installer/setup/install_worker.cc:717: *product_id +=
value_name.substr(arraysize(kMSIProductIdPrefix) - 1);
why append?

https://codereview.chromium.org/749133003/diff/40001/chrome/installer/setup/i...
chrome/installer/setup/install_worker.cc:734: base::string16
ConvertGUIDToMSIUserDataFormat(const base::string16& guid) {
please keep the definition order here in the .cc file in sync with the
declaration order in the .h file. in other words, either put this at the bottom
of the file here or at the top in the header.

https://codereview.chromium.org/749133003/diff/40001/chrome/installer/setup/i...
chrome/installer/setup/install_worker.cc:741: base::WStringTokenizer t(guid,
L"-");
t -> tokenizer ("Function names, variable names, and filenames should be
descriptive; eschew abbreviation."
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#General_Namin...).

https://codereview.chromium.org/749133003/diff/40001/chrome/installer/setup/i...
chrome/installer/setup/install_worker.cc:765: if (token_count == 5)
nit: more shorter?
  return (token_count == 5) ? msi_formatted_guid : base::string();

https://codereview.chromium.org/749133003/diff/40001/chrome/installer/setup/i...
chrome/installer/setup/install_worker.cc:905:
install_list->AddCreateRegKeyWorkItem(reg_root, uninstall_reg_path,
this key is owned by Windows, no? if so, it isn't Chrome's responsibility to
create it. if it's not there (imagine it gets moved in the future), then
Chrome's installer probably shouldn't create it, don't you think? or am i
misunderstanding?

https://codereview.chromium.org/749133003/diff/40001/chrome/installer/setup/i...
chrome/installer/setup/install_worker.cc:910:
base::ASCIIToWide(new_version.GetString()), true);
please use base::ASCIIToUTF16 here and on line 931 (see http://crbug.com/23581
for reference).

https://codereview.chromium.org/749133003/diff/40001/chrome/installer/setup/i...
chrome/installer/setup/install_worker.cc:910:
base::ASCIIToWide(new_version.GetString()), true);
should failures here not also be ignored?

https://codereview.chromium.org/749133003/diff/40001/chrome/installer/setup/i...
chrome/installer/setup/install_worker.cc:934: VLOG(1) << "Failed to convert MSI
user data id.";
suggestion: LOG(DFATAL) and include msi_product_id in the log message so we can
see what the input was.

[robertshield, 2014-11-27 22:25:58]

Thanks, addressed all comments except for the creation of the uninstall registry
key. Will check back when I've got the MSI build running locally to test.

https://codereview.chromium.org/749133003/diff/40001/chrome/installer/setup/i...
File chrome/installer/setup/install_worker.cc (right):

https://codereview.chromium.org/749133003/diff/40001/chrome/installer/setup/i...
chrome/installer/setup/install_worker.cc:97: // http://crbug.com/111058 to be
fixed.
On 2014/11/27 19:42:04, grt wrote:
> please add to the doc comment that this is a format string and what the
> substituted value must be.

Done.

https://codereview.chromium.org/749133003/diff/40001/chrome/installer/setup/i...
chrome/installer/setup/install_worker.cc:706: base::string16* product_id) {
On 2014/11/27 19:42:04, grt wrote:
> why the out param for this? why not return the product id or an empty string
on
> failure?

The result of pruning an earlier API and not pruning hard enough. Now fixed.

https://codereview.chromium.org/749133003/diff/40001/chrome/installer/setup/i...
chrome/installer/setup/install_worker.cc:713: while (value_iter.Valid()) {
On 2014/11/27 19:42:04, grt wrote:
> nit:
>   for (; value_iter.Valid(); ++value_iter) {
> and remove line 721

Done.

https://codereview.chromium.org/749133003/diff/40001/chrome/installer/setup/i...
chrome/installer/setup/install_worker.cc:717: *product_id +=
value_name.substr(arraysize(kMSIProductIdPrefix) - 1);
On 2014/11/27 19:42:04, grt wrote:
> why append?

Done.

https://codereview.chromium.org/749133003/diff/40001/chrome/installer/setup/i...
chrome/installer/setup/install_worker.cc:734: base::string16
ConvertGUIDToMSIUserDataFormat(const base::string16& guid) {
On 2014/11/27 19:42:04, grt wrote:
> please keep the definition order here in the .cc file in sync with the
> declaration order in the .h file. in other words, either put this at the
bottom
> of the file here or at the top in the header.

Done.

https://codereview.chromium.org/749133003/diff/40001/chrome/installer/setup/i...
chrome/installer/setup/install_worker.cc:741: base::WStringTokenizer t(guid,
L"-");
On 2014/11/27 19:42:04, grt wrote:
> t -> tokenizer ("Function names, variable names, and filenames should be
> descriptive; eschew abbreviation."
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#General_Namin...).

Done.

https://codereview.chromium.org/749133003/diff/40001/chrome/installer/setup/i...
chrome/installer/setup/install_worker.cc:765: if (token_count == 5)
On 2014/11/27 19:42:04, grt wrote:
> nit: more shorter?
>   return (token_count == 5) ? msi_formatted_guid : base::string();

Done.

https://codereview.chromium.org/749133003/diff/40001/chrome/installer/setup/i...
chrome/installer/setup/install_worker.cc:905:
install_list->AddCreateRegKeyWorkItem(reg_root, uninstall_reg_path,
On 2014/11/27 19:42:04, grt wrote:
> this key is owned by Windows, no? if so, it isn't Chrome's responsibility to
> create it. if it's not there (imagine it gets moved in the future), then
> Chrome's installer probably shouldn't create it, don't you think? or am i
> misunderstanding?

Hrmm, that's not unreasonable: the key is owned by the MSI machinery, I was
creating it here because I wasn't sure if it would be present before the custom
action that runs the .exe installer was run. Conversely it's at least vaguely
possible that creating it here is harmful. Let me postpone my answer here until
I complete the end-to-end test w/ building a full MSI.

https://codereview.chromium.org/749133003/diff/40001/chrome/installer/setup/i...
chrome/installer/setup/install_worker.cc:910:
base::ASCIIToWide(new_version.GetString()), true);
On 2014/11/27 19:42:04, grt wrote:
> please use base::ASCIIToUTF16 here and on line 931 (see http://crbug.com/23581
> for reference).

Ok. 
<grump>
If ASCIIToWide is deprecated, it should be marked as such.
</grump>

https://codereview.chromium.org/749133003/diff/40001/chrome/installer/setup/i...
chrome/installer/setup/install_worker.cc:910:
base::ASCIIToWide(new_version.GetString()), true);
On 2014/11/27 19:42:04, grt wrote:
> should failures here not also be ignored?

Done.

https://codereview.chromium.org/749133003/diff/40001/chrome/installer/setup/i...
chrome/installer/setup/install_worker.cc:934: VLOG(1) << "Failed to convert MSI
user data id.";
On 2014/11/27 19:42:04, grt wrote:
> suggestion: LOG(DFATAL) and include msi_product_id in the log message so we
can
> see what the input was.

I had picked VLOG since we always run the installer with verbose-logging in the
MSI and I'm not sure about wanting to DCHECK from a child-process of an MSI
custom action which doesn't run on the interactive desktop iirc.

Let me know if you'd rather I LOG(ERROR). 

Adding the extra logging details in any case.

[grt (UTC plus 2), 2014-12-01 16:19:30]

https://codereview.chromium.org/749133003/diff/40001/chrome/installer/setup/i...
File chrome/installer/setup/install_worker.cc (right):

https://codereview.chromium.org/749133003/diff/40001/chrome/installer/setup/i...
chrome/installer/setup/install_worker.cc:910:
base::ASCIIToWide(new_version.GetString()), true);
On 2014/11/27 22:25:57, robertshield wrote:
> On 2014/11/27 19:42:04, grt wrote:
> > please use base::ASCIIToUTF16 here and on line 931 (see
http://crbug.com/23581
> > for reference).
> 
> Ok. 
> <grump>
> If ASCIIToWide is deprecated, it should be marked as such.
> </grump>

Agreed.

https://codereview.chromium.org/749133003/diff/40001/chrome/installer/setup/i...
chrome/installer/setup/install_worker.cc:934: VLOG(1) << "Failed to convert MSI
user data id.";
On 2014/11/27 22:25:57, robertshield wrote:
> On 2014/11/27 19:42:04, grt wrote:
> > suggestion: LOG(DFATAL) and include msi_product_id in the log message so we
> can
> > see what the input was.
> 
> I had picked VLOG since we always run the installer with verbose-logging in
the
> MSI

This new code runs on all updates following an MSI install, so it isn't
restricted to only running in the context of a Windows Installer-driven MSI
install.

> and I'm not sure about wanting to DCHECK from a child-process of an MSI
> custom action which doesn't run on the interactive 
desktop iirc.

We'll never ship an MSI with DCHECK on, so this will never CHECK when run from
an MSI. DFATAL will mean it'll be a DCHECK in debug builds (which you'll be
doing by hand) and an ERROR in release builds.

> Let me know if you'd rather I LOG(ERROR). 

I still thing LOG(DFATAL) is a good blend: CHECK in debug, ERROR otherwise.

> Adding the extra logging details in any case.

Thanks.

https://codereview.chromium.org/749133003/diff/60001/chrome/installer/setup/i...
File chrome/installer/setup/install_worker.cc (right):

https://codereview.chromium.org/749133003/diff/60001/chrome/installer/setup/i...
chrome/installer/setup/install_worker.cc:716: if (StartsWith(value_name,
kMSIProductIdPrefix, false))
uber-nit: "false /* !case_sensitive */))"

[grt (UTC plus 2), 2016-10-14 14:19:56]

Would you believe that bcwhite did this in r353459? AWESOME! Closing out this
old issue.

[grt (UTC plus 2), 2016-10-14 14:20:02]

Description was changed from

==========
Add fixing of MSI DisplayVersion to Chrome installs/updates.

BUG=67348
==========

to

==========
Add fixing of MSI DisplayVersion to Chrome installs/updates.

BUG=67348
==========

[robertshield, 2016-10-14 14:22:20]

On 2016/10/14 14:19:56, grt (no reviews Oct 17-21) wrote:
> Would you believe that bcwhite did this in r353459? AWESOME! Closing out this
> old issue.

Huzzah!