Re-commit: Add new PathService paths for Windows' All Users Desktop and Quick Launch folders.

Add new PathService paths for Windows' All Users Desktop and Quick Launch folders.

Re-commit, reverted in http://crrev.com/157680 (original commit in http://crrev.com/157667).

This allows usage of PathService to cache the shortcut install paths and more importantly to mock them in shortcut tests!

Also move chrome::DIR_USER_DESKTOP to base::DIR_USER_DESKTOP; this is really where it belongs. In fact it is only in chrome_paths.h because it used to be called DIR_DEFAULT_DOWNLOAD and cpu@ renamed it to DIR_USER_DESKTOP in http://crrev.com/1753 (early days!) after that it started to be used all over the place as the Desktop path. Finally bringing it to base_paths.h, beside DIR_START_MENU and friends, is the right thing to do imo.

TBR=brettw@chromium.org
BUG=148539
TEST=Quick Launch shortcut installed in the right place on XP (both Default and current user)
Desktop shortcuts installed in the right place (both All Users and per-user installs).

installer_util_unittests.exe --gtest_filter=ShellUtilShortcutTest*
unit_tests.exe --gtest_filter=ProfileShortcutManagerTest*
base_unittests --gtest_filter=PathServiceTest*

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=157779

[gab, 2012-09-20 03:12:07]

Fixed XP tests, Default user's quick launch folder doesn't exist by default on
XP... PTAL.

http://codereview.chromium.org/10964007/diff/2001/base/path_service_unittest.cc
File base/path_service_unittest.cc (right):

http://codereview.chromium.org/10964007/diff/2001/base/path_service_unittest....
base/path_service_unittest.cc:46: path.value().find(L"Default User") !=
string16::npos);
Let me know what you think of adding the "find" to the extra validation here
(and on the OS_LINUX exception above).

My thinking is that since we can't do the full validation, we can at least do a
little bit more than assert !path.empty().

At the same time if we assert .find() we can probably remove the path.empty()
check... I only kept it as I don't like to rely on implementation and I'm
paranoically thinking that one day someone could decide that you must not call
path.value() if path.empty()... (i.e. that you could reset a FilePath without
reseting its internal state... but that's probably pushing it so feel free to
tell me I'm crazy!).

[grt (UTC plus 2), 2012-09-20 03:44:07]

http://codereview.chromium.org/10964007/diff/2001/base/path_service_unittest.cc
File base/path_service_unittest.cc (right):

http://codereview.chromium.org/10964007/diff/2001/base/path_service_unittest....
base/path_service_unittest.cc:39: path.value().find(L"Desktop") !=
string16::npos);
IMO, this looks like a test failure waiting to happen.  Is it always called
"Desktop" on all machines all the time (not just the bot, but also every
developer's workstation)?

http://codereview.chromium.org/10964007/diff/2001/base/path_service_unittest....
base/path_service_unittest.cc:40: #elif defined(OS_WIN)
the following is more consistent with lines 33 and 34:
#endif
#if defined(OS_WIN)

http://codereview.chromium.org/10964007/diff/2001/base/path_service_unittest....
base/path_service_unittest.cc:44: base::win::GetVersion() <=
base::win::VERSION_XP) {
maybe this needs to be < base::win::VERSION_VISTA since VERSION_SERVER_2003 is
more like XP than Vista and covers XP Pro x64?

http://codereview.chromium.org/10964007/diff/2001/base/path_service_unittest....
base/path_service_unittest.cc:46: path.value().find(L"Default User") !=
string16::npos);
On 2012/09/20 03:12:07, gab wrote:
> Let me know what you think of adding the "find" to the extra validation here
> (and on the OS_LINUX exception above).
> 
> My thinking is that since we can't do the full validation, we can at least do
a
> little bit more than assert !path.empty().

Since pulling in Userenv.lib shouldn't be a problem for this test binary, what
do you think of calling GetDefaultUserProfileDirectory and confirm that it's a
prefix of |path|?  This avoids hardcoding "Default User".

> At the same time if we assert .find() we can probably remove the path.empty()
> check... I only kept it as I don't like to rely on implementation and I'm
> paranoically thinking that one day someone could decide that you must not call
> path.value() if path.empty()...

chrome will explode if that ever becomes the case.

> (i.e. that you could reset a FilePath without
> reseting its internal state... but that's probably pushing it so feel free to
> tell me I'm crazy!).

[gab, 2012-09-20 04:15:35]

Done.

http://codereview.chromium.org/10964007/diff/2001/base/path_service_unittest.cc
File base/path_service_unittest.cc (right):

http://codereview.chromium.org/10964007/diff/2001/base/path_service_unittest....
base/path_service_unittest.cc:39: path.value().find(L"Desktop") !=
string16::npos);
On 2012/09/20 03:44:07, grt wrote:
> IMO, this looks like a test failure waiting to happen.  Is it always called
> "Desktop" on all machines all the time (not just the bot, but also every
> developer's workstation)?

Right, probably too dangerous, reverted.

http://codereview.chromium.org/10964007/diff/2001/base/path_service_unittest....
base/path_service_unittest.cc:40: #elif defined(OS_WIN)
On 2012/09/20 03:44:07, grt wrote:
> the following is more consistent with lines 33 and 34:
> #endif
> #if defined(OS_WIN)

I debated doing that, thanks for making my life/decision easy, done :)!

http://codereview.chromium.org/10964007/diff/2001/base/path_service_unittest....
base/path_service_unittest.cc:44: base::win::GetVersion() <=
base::win::VERSION_XP) {
On 2012/09/20 03:44:07, grt wrote:
> maybe this needs to be < base::win::VERSION_VISTA since VERSION_SERVER_2003 is
> more like XP than Vista and covers XP Pro x64?

Done.

[grt (UTC plus 2), 2012-09-20 04:33:42]

lgtm w/ a nit.

http://codereview.chromium.org/10964007/diff/8001/base/path_service_unittest.cc
File base/path_service_unittest.cc (right):

http://codereview.chromium.org/10964007/diff/8001/base/path_service_unittest....
base/path_service_unittest.cc:52: DWORD size = MAX_PATH;
MAX_PATH -> arraysize(default_profile_path)

[gab, 2012-09-20 12:30:32]

Thanks,

brettw to TBR since this is basically the same as the already LGTMed
https://chromiumcodereview.appspot.com/10910209.

Gab

http://codereview.chromium.org/10964007/diff/8001/base/path_service_unittest.cc
File base/path_service_unittest.cc (right):

http://codereview.chromium.org/10964007/diff/8001/base/path_service_unittest....
base/path_service_unittest.cc:52: DWORD size = MAX_PATH;
On 2012/09/20 04:33:42, grt wrote:
> MAX_PATH -> arraysize(default_profile_path)

Done.

[commit-bot: I haz the power, 2012-09-20 12:30:57]

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

[commit-bot: I haz the power, 2012-09-20 12:42:11]

Sorry for I got bad news for ya.
Compile failed with a clobber build.
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-09-20 14:05:54]

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

[commit-bot: I haz the power, 2012-09-20 16:24:54]

Change committed as 157779