Add DIR_PROGRAM_FILES6432 to PathService to return "Program Files" on both 64-bit and 32-bit platfo…

Add DIR_PROGRAM_FILES6432 to PathService to return "Program Files" on both 64-bit and 32-bit platforms.

BUG=

Committed: https://crrev.com/16d2f1211e19b57fa1988896fb6eaea633cb3c1f
Cr-Commit-Position: refs/heads/master@{#320498}

[Will Harris, 2015-03-11 23:20:07]

wfh@chromium.org changed reviewers:
+ cpu@chromium.org

[Will Harris, 2015-03-11 23:21:01]

PTAL

[cpu_(ooo_6.6-7.5), 2015-03-12 00:38:15]

lgtm w/ nits

https://codereview.chromium.org/986373006/diff/1/base/base_paths_win.cc
File base/base_paths_win.cc (right):

https://codereview.chromium.org/986373006/diff/1/base/base_paths_win.cc#newco...
base/base_paths_win.cc:32: std::string programfiles_w6432;
I think the construct of line 31 is on the expensive side, can you check what
they do? consider using a lighter wrapper to GetEnvironmentVariable() which I
think exists.

https://codereview.chromium.org/986373006/diff/1/base/base_paths_win.h
File base/base_paths_win.h (right):

https://codereview.chromium.org/986373006/diff/1/base/base_paths_win.h#newcode26
base/base_paths_win.h:26: DIR_PROGRAM_FILES6432,  // c:\program files on both
32-bit and 64-bit.
the comment at the end of 24-26 can be 'see table above'

[Will Harris, 2015-03-12 01:18:36]

I changed the logic. previously it was broken on 32-bit/32-bit. I'd like to wait
until tomorrow when I can test on a 32-bit VM.

Also, I think ProgramW6432 only exists on Windows 7 and above, which makes this
CL sort of broken if that's true.  I'd like to test this too.

https://codereview.chromium.org/986373006/diff/1/base/base_paths_win.cc
File base/base_paths_win.cc (right):

https://codereview.chromium.org/986373006/diff/1/base/base_paths_win.cc#newco...
base/base_paths_win.cc:32: std::string programfiles_w6432;
On 2015/03/12 00:38:15, cpu wrote:
> I think the construct of line 31 is on the expensive side, can you check what
> they do? consider using a lighter wrapper to GetEnvironmentVariable() which I
> think exists.

I checked and Environment is very lightweight, the constructor is nothing - so
the only benefit of going directly to GetEnvironmentVariable would be to avoid
the wide->utf8->wide conversions.

There doesn't appear to be a lighter wrapper than using this.

https://codereview.chromium.org/986373006/diff/1/base/base_paths_win.h
File base/base_paths_win.h (right):

https://codereview.chromium.org/986373006/diff/1/base/base_paths_win.h#newcode26
base/base_paths_win.h:26: DIR_PROGRAM_FILES6432,  // c:\program files on both
32-bit and 64-bit.
On 2015/03/12 00:38:15, cpu wrote:
> the comment at the end of 24-26 can be 'see table above'

Done.

[grt (UTC plus 2), 2015-03-12 13:34:43]

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

[grt (UTC plus 2), 2015-03-12 13:34:44]

https://codereview.chromium.org/986373006/diff/20001/base/base_paths_win.cc
File base/base_paths_win.cc (right):

https://codereview.chromium.org/986373006/diff/20001/base/base_paths_win.cc#n...
base/base_paths_win.cc:31: scoped_ptr<base::Environment>
env(base::Environment::Create());
move these down into a block in the base::DIR_PROGRAM_FILES6432 case. there's no
reason all callers should pay the price of these.

https://codereview.chromium.org/986373006/diff/20001/base/base_paths_win.cc#n...
base/base_paths_win.cc:74: // 32-bit process running in WOW64 sets ProgramW6432
environment variable.
How about reducing the dependence on the variable by changing this to:

#if !defined(_WIN64)
    if (base::win::OSInfo::GetInstance()->wow64_status() ==
base::win::WOW64_ENABLED) {  // 32-bit running in WoW64.
      %ProgramW6432%
      break;
    }
#endif
    CSIDL_PROGRAM_FILES

they *should* amount to the same thing, but it seems safer to me to go to the
true source where possible.

https://codereview.chromium.org/986373006/diff/20001/base/base_paths_win.h
File base/base_paths_win.h (right):

https://codereview.chromium.org/986373006/diff/20001/base/base_paths_win.h#ne...
base/base_paths_win.h:20: // DIR_PROGRAM_FILES         1               2        
         1
(these are my comments for my own sanity)
== FOLDERID_ProgramFiles

https://codereview.chromium.org/986373006/diff/20001/base/base_paths_win.h#ne...
base/base_paths_win.h:21: // DIR_PROGRAM_FILESX86      1               2        
         2
== FOLDERID_ProgramFilesX86

https://codereview.chromium.org/986373006/diff/20001/base/base_paths_win.h#ne...
base/base_paths_win.h:22: // DIR_PROGRAM_FILES6432     1               1        
         1
#if defined(_WIN64)
FOLDERID_ProgramFilesX64
#else
if (WOW64_ENABLED)  // 32-on-64
  %ProgramW6432%
else  // valid for 32-on-32 and 64-on-64
  FOLDERID_ProgramFiles
#endif

[Will Harris, 2015-03-12 20:34:30]

Thanks for the review grt.  PTAL.

I actually went ahead and tested this on XP x64 and Vista x64 and it works on
both.  I think the MSDN page caveat about only supporting Win7 and above must
only apply for 64 processes running on 32bit (!!).

https://codereview.chromium.org/986373006/diff/20001/base/base_paths_win.cc
File base/base_paths_win.cc (right):

https://codereview.chromium.org/986373006/diff/20001/base/base_paths_win.cc#n...
base/base_paths_win.cc:31: scoped_ptr<base::Environment>
env(base::Environment::Create());
On 2015/03/12 13:34:44, grt wrote:
> move these down into a block in the base::DIR_PROGRAM_FILES6432 case. there's
no
> reason all callers should pay the price of these.

Done.

https://codereview.chromium.org/986373006/diff/20001/base/base_paths_win.cc#n...
base/base_paths_win.cc:74: // 32-bit process running in WOW64 sets ProgramW6432
environment variable.
On 2015/03/12 13:34:44, grt wrote:
> How about reducing the dependence on the variable by changing this to:
> 
> #if !defined(_WIN64)
>     if (base::win::OSInfo::GetInstance()->wow64_status() ==
> base::win::WOW64_ENABLED) {  // 32-bit running in WoW64.
>       %ProgramW6432%
>       break;
>     }
> #endif
>     CSIDL_PROGRAM_FILES
> 
> they *should* amount to the same thing, but it seems safer to me to go to the
> true source where possible.

Done.

[grt (UTC plus 2), 2015-03-13 13:01:32]

lgtm

[grt (UTC plus 2), 2015-03-13 13:02:56]

The CQ bit was checked by grt@chromium.org

[grt (UTC plus 2), 2015-03-13 13:02:57]

The patchset sent to the CQ was uploaded after l-g-t-m from cpu@chromium.org
Link to the patchset: https://codereview.chromium.org/986373006/#ps40001 (title:
"code review changes")

[commit-bot: I haz the power, 2015-03-13 13:03:08]

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

[commit-bot: I haz the power, 2015-03-13 14:34:54]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-03-13 14:35:31]

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