Fix shortcut tests and remove legacy shortcut code.

Fix shortcut tests and remove legacy shortcut code for the Default-user
Quick Launch shortcut.

InstallShortcutTest.CreateAllShortcutsSystemLevel has been broken since http://crrev.com/164849 ...

The test expectations had not been adjusted in this CL as setup_unittests.exe
doesn't run on the waterfall yet (issue #153829) and the breakage had gone
unnoticed.

Expectations have now been adjusted such that we no longer expect a system-level
(Default-user) Quick Launch shortcut, but a per-user Quick Launch shortcut for
the admin running the install as the product code's logic has been doing for over
a year...

Also remove the code cleaning up the legacy Default-user Quick Launch shortcut as
this cleanup code has been running for 21 months now and leaving this shortcut
behind for a few users is a no-op (i.e. the Default-user shortcut will get copied
to new Windows profiles and Active Setup will kick in a few seconds later to try
to install the same shortcut which will already exist... the UX result will be
the same: a single Chrome Quick Launch shortcut will be installed).

BUG=329239, 153829
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=288393

[gab, 2014-08-01 19:53:16]

Sir, fixed some tests, cleaned up some code, more to come :-)!

Cheers!
Gab

[grt (UTC plus 2), 2014-08-04 18:27:08]

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

https://codereview.chromium.org/432273005/diff/1/base/base_paths_win.cc#newco...
base/base_paths_win.cc:150: // For the current user, grab the APPDATA directory
directly from the
nit: move this comment up above the "if" and nix the braces. previously it had
to be in the body since this was an "else" clause, but that's no loner the case.

https://codereview.chromium.org/432273005/diff/1/base/base_paths_win.cc#newco...
base/base_paths_win.cc:161: .AppendASCII("Internet Explorer")
is this how git cl format does things? shouldn't this be four-space indent from
the start of the line above?

https://codereview.chromium.org/432273005/diff/1/chrome/installer/setup/insta...
File chrome/installer/setup/install.cc (left):

https://codereview.chromium.org/432273005/diff/1/chrome/installer/setup/insta...
chrome/installer/setup/install.cc:252: void CleanupLegacyShortcuts(const
installer::InstallerState& installer_state,
this is still run on a surprising number of updates from old versions. i don't
think removing it altogether is the right thing. can some cleanups take place
while still leaving a function to remove old legacy shortcuts?

[gab, 2014-08-04 18:57:08]

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

https://codereview.chromium.org/432273005/diff/1/base/base_paths_win.cc#newco...
base/base_paths_win.cc:150: // For the current user, grab the APPDATA directory
directly from the
On 2014/08/04 18:27:07, grt (no reviews Aug 9 - 27) wrote:
> nit: move this comment up above the "if" and nix the braces. previously it had
> to be in the body since this was an "else" clause, but that's no loner the
case.

Actually I'll just remove this comment, it's no longer relevant without the
all-users case.

https://codereview.chromium.org/432273005/diff/1/base/base_paths_win.cc#newco...
base/base_paths_win.cc:161: .AppendASCII("Internet Explorer")
On 2014/08/04 18:27:08, grt (no reviews Aug 9 - 27) wrote:
> is this how git cl format does things? shouldn't this be four-space indent
from
> the start of the line above?

Yes it is, I like it as-is personally.

https://codereview.chromium.org/432273005/diff/1/chrome/installer/setup/insta...
File chrome/installer/setup/install.cc (left):

https://codereview.chromium.org/432273005/diff/1/chrome/installer/setup/insta...
chrome/installer/setup/install.cc:252: void CleanupLegacyShortcuts(const
installer::InstallerState& installer_state,
On 2014/08/04 18:27:08, grt (no reviews Aug 9 - 27) wrote:
> this is still run on a surprising number of updates from old versions. 

Why do you think this is the case? Google Update data?

> i don't
> think removing it altogether is the right thing. can some cleanups take place
> while still leaving a function to remove old legacy shortcuts?

We typically leave migration code in Chrome for 2-3 milestones; this code has
been here for 14 milestones I think it's more than time to remove it...

(the side-effect of someone not running this code is only that they may be left
with (1) an uninstall link in the Start Menu and (2) a Quick Launch link in the
Default user profile which doesn't matter, it's just that we wanted to go away
from using the Default user because Active Setup is better)

[grt (UTC plus 2), 2014-08-04 19:05:52]

https://codereview.chromium.org/432273005/diff/1/chrome/installer/setup/insta...
File chrome/installer/setup/install.cc (left):

https://codereview.chromium.org/432273005/diff/1/chrome/installer/setup/insta...
chrome/installer/setup/install.cc:252: void CleanupLegacyShortcuts(const
installer::InstallerState& installer_state,
On 2014/08/04 18:57:08, gab wrote:
> We typically leave migration code in Chrome for 2-3 milestones;

If there is a policy for this (I'm not aware of one), it should be made based on
data. What's the justification for 2-3 milestones? What's the cutoff guideline
that we use for choosing 2-3 over 3-4 or 15-20?

[gab, 2014-08-04 20:22:51]

https://codereview.chromium.org/432273005/diff/1/chrome/installer/setup/insta...
File chrome/installer/setup/install.cc (left):

https://codereview.chromium.org/432273005/diff/1/chrome/installer/setup/insta...
chrome/installer/setup/install.cc:252: void CleanupLegacyShortcuts(const
installer::InstallerState& installer_state,
On 2014/08/04 19:05:51, grt (no reviews Aug 9 - 27) wrote:
> On 2014/08/04 18:57:08, gab wrote:
> > We typically leave migration code in Chrome for 2-3 milestones;
> 
> If there is a policy for this (I'm not aware of one), 

This is what we do in prefs when deprecating prefs (deprecate them, leaving only
clearing code with a TODO to remove it altogether 3 milestones later).

> it should be made based on
> data. What's the justification for 2-3 milestones? What's the cutoff guideline
> that we use for choosing 2-3 over 3-4 or 15-20?

I don't know what it's based on, just the trend I've seen around Chrome and have
picked up myself (w/ many reviews lgtm'ed by other Chromites where I have a
TODO(gab): Remove this in MXX -- where XX = trunk + 3).

[gab, 2014-08-05 20:35:18]

Re-added some code to focus this CL on fixing the test and removing the
deprecated Default-user Quick Launch shortcut (which I think there is no doubt
we can remove as leaving it behind on a few machine has no UX impact beyond
having an extra tiny file on disk -- for new Windows profileswho get the Default
profile on such a machine, Active Setup will simply update that shortcut a few
seconds later rather than truly install it...)

[gab, 2014-08-05 20:43:27]

Mark, PTAL at code cleanup in base :-)!

[Mark Mentovai, 2014-08-05 20:58:24]

Please choose another base OWNER for this, I’m doing two code yellow/brown/aquas
right now.

[gab, 2014-08-05 21:52:24]

On 2014/08/05 20:58:24, Mark Mentovai wrote:
> Please choose another base OWNER for this, I’m doing two code
yellow/brown/aquas
> right now.

@willchan: PTAL at code cleanup in base :-)!

[willchan no longer on Chromium, 2014-08-05 21:54:29]

Shit, I guess I lose since I'm only involved in one code yellow. Dammit
Mark!


On Tue, Aug 5, 2014 at 2:52 PM, <gab@chromium.org> wrote:

> On 2014/08/05 20:58:24, Mark Mentovai wrote:
>
>> Please choose another base OWNER for this, I'm doing two code
>>
> yellow/brown/aquas
>
>> right now.
>>
>
> @willchan: PTAL at code cleanup in base :-)!
>
> https://codereview.chromium.org/432273005/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[willchan no longer on Chromium, 2014-08-05 22:51:47]

I looked through the bugs and changelist description but found no explanation of
what was broken in the shortcut tests and how it's getting fixed, nor why it's
OK to remove the legacy code. Can you update the changelist description with
those details?

[gab, 2014-08-06 14:09:02]

On 2014/08/05 22:51:47, willchan wrote:
> I looked through the bugs and changelist description but found no explanation
of
> what was broken in the shortcut tests and how it's getting fixed, nor why it's
> OK to remove the legacy code. Can you update the changelist description with
> those details?

Done. PTAL.

[grt (UTC plus 2), 2014-08-06 14:44:38]

lgtm. thanks for the cleanup.

is there any documentation (chromium.org?) that should be updated as well?

[grt (UTC plus 2), 2014-08-06 15:49:57]

OMG! test_installer caught a regression!

[0806/083701:ERROR:shell_util.cc(1361)] Cannot resolve shortcut at
C:\Users\chrome-bot\AppData\Roaming\Microsoft\Internet Explorer\Quick
Launch\User Pinned\TaskBar\Windows Media Player.lnk
[0806/083701:ERROR:product.cc(126)] Failed to Open or Write MSI valueto client
state key. error: 2
[0806/083701:ERROR:product.cc(126)] Failed to Open or Write MSI valueto client
state key. error: 2
[0806/083701:ERROR:product.cc(126)] Failed to Open or Write MSI valueto client
state key. error: 2
.[0806/083708:FATAL:shell_util.cc(1478)] Check failed: level == CURRENT_USER (2
vs. 1)
Traceback (most recent call last):
  File "uninstall_chrome.py", line 68, in <module>
    sys.exit(main())
  File "uninstall_chrome.py", line 63, in main
    'status %d.' % exit_status)
Exception: Could not uninstall Chrome. The installer exited with status
-2147483645.
E[0806/083710:FATAL:shell_util.cc(1478)] Check failed: level == CURRENT_USER (2
vs. 1)
Traceback (most recent call last):
  File "uninstall_chrome.py", line 68, in <module>
    sys.exit(main())
  File "uninstall_chrome.py", line 63, in main
    'status %d.' % exit_status)
Exception: Could not uninstall Chrome. The installer exited with status
-2147483645.
E
======================================================================
ERROR: ChromeSystemLevel: clean -> install_chrome_system ->
chrome_system_installed_not_inuse -> launch_chrome_system ->
chrome_system_installed_inuse -> quit_chrome_system ->
chrome_system_installed_not_inuse -> uninstall_chrome_system -> clean

----------------------------------------------------------------------
Traceback (most recent call last):
  File
"E:\b\build\slave\win\build\src\chrome\test\mini_installer\test_installer.py",
line 90, in runTest
    RunCommand(self._config.actions[action], self._variable_expander)
  File
"E:\b\build\slave\win\build\src\chrome\test\mini_installer\test_installer.py",
line 144, in RunCommand
    expanded_command, exit_status))
Exception: Command python uninstall_chrome.py --chrome-long-name="Chromium"
--system-level returned non-zero exit status 1

======================================================================
ERROR: ChromeSystemLevel: clean -> install_chrome_system ->
chrome_system_installed_not_inuse -> launch_chrome_system ->
chrome_system_installed_inuse -> quit_chrome_system ->
chrome_system_installed_not_inuse -> uninstall_chrome_system -> clean

----------------------------------------------------------------------
Traceback (most recent call last):
  File
"E:\b\build\slave\win\build\src\chrome\test\mini_installer\test_installer.py",
line 102, in tearDown
    RunCleanCommand(True, self._variable_expander)
  File
"E:\b\build\slave\win\build\src\chrome\test\mini_installer\test_installer.py",
line 165, in RunCleanCommand
    RunCommand(' && '.join(commands), variable_expander)
  File
"E:\b\build\slave\win\build\src\chrome\test\mini_installer\test_installer.py",
line 144, in RunCommand
    expanded_command, exit_status))
Exception: Command python uninstall_chrome.py --chrome-long-name="Chromium"
--no-error-if-absent   && python uninstall_chrome.py
--chrome-long-name="Chromium" --no-error-if-absent --system-level  returned
non-zero exit status 1

----------------------------------------------------------------------
Ran 2 tests in 21.867s

FAILED (errors=2)

unfortunately the bot is b0rked now since it can't uninstall the version with
the DCHECK. :-( I'll manually remove it.

[willchan no longer on Chromium, 2014-08-06 20:41:54]

Thanks, LGTM mod whatever test failure grt@ is referencing :) I don't know
what's going on there and defer to grt@.

[gab, 2014-08-07 12:37:29]

On 2014/08/06 14:44:38, grt (no reviews Aug 9 - 27) wrote:
> lgtm. thanks for the cleanup.
> 
> is there any documentation (chromium.org?) that should be updated as well?

Looks like http://www.chromium.org/developers/installer#shortcuts is already
up-to-date (i.e. that's how critical it is to get setup_unittests on the
waterfall -- the documentation was more up-to-date than the tests...!)

Looking at the test_installer regression.

[gab, 2014-08-07 13:02:26]

Fixed installer regression (glad the installer tests were in in time!). PTAL.

(we should probably add the same kind of DCHECK in GetShortcutPath for other
user-level-only locations, e.g., taskbar pins -- or we should change
GetShortcutPath to return true/false based on existence of path, WDYT?)

Cheers,
Gab

[grt (UTC plus 2), 2014-08-08 13:46:48]

On 2014/08/07 13:02:26, gab wrote:
> Fixed installer regression (glad the installer tests were in in time!). PTAL.
> 
> (we should probably add the same kind of DCHECK in GetShortcutPath for other
> user-level-only locations, e.g., taskbar pins -- or we should change
> GetShortcutPath to return true/false based on existence of path, WDYT?)
> 
> Cheers,
> Gab

Lgtm. Additional DCHECKs sounds like a good idea. Let's discuss after vacations.

[gab, 2014-08-08 14:02:27]

The CQ bit was checked by gab@chromium.org

[commit-bot: I haz the power, 2014-08-08 14:04:37]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/gab@chromium.org/432273005/80001

[commit-bot: I haz the power, 2014-08-08 15:29:09]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/44371)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-08 18:17:59]

Change committed as 288393