Prototype fix for version shear across updates.

Prototype fix for version shear across updates.

BUG=371087
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=271076

[grt (UTC plus 2), 2014-05-09 16:53:07]

https://codereview.chromium.org/274453010/diff/1/chrome/browser/first_run/upg...
File chrome/browser/first_run/upgrade_util_win.cc (right):

https://codereview.chromium.org/274453010/diff/1/chrome/browser/first_run/upg...
chrome/browser/first_run/upgrade_util_win.cc:127: 
nit: remove extra newline

https://codereview.chromium.org/274453010/diff/1/chrome/browser/first_run/upg...
chrome/browser/first_run/upgrade_util_win.cc:129: base::FilePath
current_exe_path(command_line.GetProgram().DirName());
i'm not sure about this. i recently fixed a bugaboo in the installer where the
current command line was being used similarly, and it turned out that due to
ambiguities in the way command lines are generated and parsed at the various
layers (cmd.exe, win32, base::CommandLine), base::CommandLine::GetProgram()
would return only the first "part" of program paths that contained spaces. the
fix was to use base::PathService::Get(base::FILE_EXE, ...) instead.

https://codereview.chromium.org/274453010/diff/1/chrome/browser/first_run/upg...
chrome/browser/first_run/upgrade_util_win.cc:263: DWORD returned_chars =
since returned_chars isn't really used, how about:
  if (!::GetMappedFileName(...)) {
    return false;
  }
  base::FilePath file_name(base::FilePath(mapped_file_name).BaseName());
  return base::FilePath::CompareEqualIgnoreCase(file_name.value(),
                                                installer::kChromeOldExe);

https://codereview.chromium.org/274453010/diff/1/chrome/browser/first_run/upg...
chrome/browser/first_run/upgrade_util_win.cc:267: MAX_PATH * 2);
arraysize(mapped_file_name)

[robertshield, 2014-05-15 16:49:34]

Thanks, PTAL!

https://codereview.chromium.org/274453010/diff/1/chrome/browser/first_run/upg...
File chrome/browser/first_run/upgrade_util_win.cc (right):

https://codereview.chromium.org/274453010/diff/1/chrome/browser/first_run/upg...
chrome/browser/first_run/upgrade_util_win.cc:127: 
On 2014/05/09 16:53:08, grt wrote:
> nit: remove extra newline

Done.

https://codereview.chromium.org/274453010/diff/1/chrome/browser/first_run/upg...
chrome/browser/first_run/upgrade_util_win.cc:129: base::FilePath
current_exe_path(command_line.GetProgram().DirName());
On 2014/05/09 16:53:08, grt wrote:
> i'm not sure about this. i recently fixed a bugaboo in the installer where the
> current command line was being used similarly, and it turned out that due to
> ambiguities in the way command lines are generated and parsed at the various
> layers (cmd.exe, win32, base::CommandLine), base::CommandLine::GetProgram()
> would return only the first "part" of program paths that contained spaces. the
> fix was to use base::PathService::Get(base::FILE_EXE, ...) instead.

Done.

https://codereview.chromium.org/274453010/diff/1/chrome/browser/first_run/upg...
chrome/browser/first_run/upgrade_util_win.cc:263: DWORD returned_chars =
On 2014/05/09 16:53:08, grt wrote:
> since returned_chars isn't really used, how about:
>   if (!::GetMappedFileName(...)) {
>     return false;
>   }
>   base::FilePath file_name(base::FilePath(mapped_file_name).BaseName());
>   return base::FilePath::CompareEqualIgnoreCase(file_name.value(),
>                                                 installer::kChromeOldExe);

Done.

https://codereview.chromium.org/274453010/diff/1/chrome/browser/first_run/upg...
chrome/browser/first_run/upgrade_util_win.cc:267: MAX_PATH * 2);
On 2014/05/09 16:53:08, grt wrote:
> arraysize(mapped_file_name)

Done.

[grt (UTC plus 2), 2014-05-15 17:47:38]

lgtm with two nits

https://codereview.chromium.org/274453010/diff/20001/chrome/browser/first_run...
File chrome/browser/first_run/upgrade_util_win.cc (right):

https://codereview.chromium.org/274453010/diff/20001/chrome/browser/first_run...
chrome/browser/first_run/upgrade_util_win.cc:133: CommandLine
chrome_exe_command_line = command_line;
consider:
  // Take care to relaunch chrome.exe rather than old_chrome.exe.
or something to explain why this matters. i can imagine a future reader being
confused as to why chrome.exe is being stripped off of |chrome_exe| and then
re-added.

https://codereview.chromium.org/274453010/diff/20001/chrome/browser/first_run...
chrome/browser/first_run/upgrade_util_win.cc:134: base::FilePath
current_exe_path(chrome_exe.DirName());
current_exe_path is a bit misleading. how about doing away with the local
variable?
  chrome_exe_command_line.SetProgram(
      chrome_exe.DirName().Append(installer::kChromeExe));

[robertshield, 2014-05-15 18:42:30]

Thanks!

https://codereview.chromium.org/274453010/diff/20001/chrome/browser/first_run...
File chrome/browser/first_run/upgrade_util_win.cc (right):

https://codereview.chromium.org/274453010/diff/20001/chrome/browser/first_run...
chrome/browser/first_run/upgrade_util_win.cc:133: CommandLine
chrome_exe_command_line = command_line;
On 2014/05/15 17:47:38, grt wrote:
> consider:
>   // Take care to relaunch chrome.exe rather than old_chrome.exe.
> or something to explain why this matters. i can imagine a future reader being
> confused as to why chrome.exe is being stripped off of |chrome_exe| and then
> re-added.

Done.

https://codereview.chromium.org/274453010/diff/20001/chrome/browser/first_run...
chrome/browser/first_run/upgrade_util_win.cc:134: base::FilePath
current_exe_path(chrome_exe.DirName());
On 2014/05/15 17:47:38, grt wrote:
> current_exe_path is a bit misleading. how about doing away with the local
> variable?
>   chrome_exe_command_line.SetProgram(
>       chrome_exe.DirName().Append(installer::kChromeExe));

Done.

[grt (UTC plus 2), 2014-05-15 18:44:42]

lgtm++

[robertshield, 2014-05-16 14:10:22]

The CQ bit was checked by robertshield@chromium.org

[commit-bot: I haz the power, 2014-05-16 14:10:39]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/robertshield@chromium.org/274453010/40001

[commit-bot: I haz the power, 2014-05-16 17:36:38]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-05-16 19:29:23]

Change committed as 271076