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.

This allows usage of PathService to cache the 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.

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=157667

[gab, 2012-09-13 23:16:50]

This looks like a big CL, but it really isn't!!

It's adding 3 path enums to base/base_paths_win.h and moving DIR_USER_DESKTOP
from chrome_paths.h to base_paths.h (see description for details).

All the rest is just cleanup (i.e.
  1) introduce base_paths_posix.h to respect general style of base_paths and not
#ifdef OS_POSIX in base_paths.h
  2) #include "base/base_paths*.h" where appropriate in the code.
)

This might seem like overkill, but the point is I didn't want to put
DIR_COMMON_DESKTOP, etc. in chrome_paths, but also felt it belonged by
DIR_USER_DESKTOP.
When moving DIR_USER_DESKTOP to base_paths I was confused by the overall
structure so I fixed it to prevent further confusion for the next developer to
read it.

@grt: OWNER approval for chrome/installer/*
@brettw: OWNER approval for base/* and chrome/*
@jochen: OWNER rubberstamp for include fixes in chrome/shell/*
@phajdan.jr@chromium.org: OWNER rubberstamp for include fixes in
net/test/remote_test_server.cc
@satorux: OWNER rubberstamp for include fixes in
webkit/chromeos/fileapi/cros_mount_point_provider.cc

http://codereview.chromium.org/10910209/diff/2001/base/base_paths.cc
File base/base_paths.cc (right):

http://codereview.chromium.org/10910209/diff/2001/base/base_paths.cc#newcode5
base/base_paths.cc:5: #include "base/base_paths.h"
All of the base_paths*.cc header listed
#include "base/base_paths*.h" at the top as if they were that header's
implementation, but this is not correct and it makes even less sense for headers
that include multiple base/base_paths*.h files (i.e. which one should be at top
then...?!)...

http://codereview.chromium.org/10910209/diff/2001/base/base_paths.h
File base/base_paths.h (left):

http://codereview.chromium.org/10910209/diff/2001/base/base_paths.h#oldcode37
base/base_paths.h:37: #if defined(OS_POSIX)
Moved all of the OS specific paths to their respective base_paths_OS.h file.

http://codereview.chromium.org/10910209/diff/2001/base/base_paths.h
File base/base_paths.h (right):

http://codereview.chromium.org/10910209/diff/2001/base/base_paths.h#newcode37
base/base_paths.h:37: DIR_USER_DESKTOP,  // Directory that correspond to the
current user's desktop.
Moved from chrome/common/chrome_paths.h as it belongs more here and I didn't
want to add other similar paths to chrome_paths (ref: additions to
base_paths_win.h).

[gab, 2012-09-13 23:22:30]

@hallielaine, rlp: FYI, this makes ProfileShortcutManagerTests (and
ShellUtilShortcutTests) finally create shortcuts in a mock Desktop (without
needing to change the tests other than setting up the mock directory at the
beginning :)).

[satorux1, 2012-09-13 23:32:32]

webkit/chromeos/fileapi/cros_mount_point_provider.cc LGTM

http://codereview.chromium.org/10910209/diff/1007/webkit/chromeos/fileapi/cro...
File webkit/chromeos/fileapi/cros_mount_point_provider.cc (right):

http://codereview.chromium.org/10910209/diff/1007/webkit/chromeos/fileapi/cro...
webkit/chromeos/fileapi/cros_mount_point_provider.cc:7: #include
"base/base_paths_posix.h"
just curious but why did it become necessary?

[gab, 2012-09-13 23:40:12]

@satorux: see below.

http://codereview.chromium.org/10910209/diff/1007/webkit/chromeos/fileapi/cro...
File webkit/chromeos/fileapi/cros_mount_point_provider.cc (right):

http://codereview.chromium.org/10910209/diff/1007/webkit/chromeos/fileapi/cro...
webkit/chromeos/fileapi/cros_mount_point_provider.cc:7: #include
"base/base_paths_posix.h"
On 2012/09/13 23:32:32, satorux1 wrote:
> just curious but why did it become necessary?

There should already have been an include to "base/base_paths.h", but this
currently compiled somehow as it was likely transitively included from another
header. Now that DIR_HOME (used below) moved to base/base_paths_posix.h, the
include is required (it may still transitively compile, but I didn't try, I'm a
big fan of "include-what-you-use").

[satorux1, 2012-09-13 23:41:40]

http://codereview.chromium.org/10910209/diff/1007/webkit/chromeos/fileapi/cro...
File webkit/chromeos/fileapi/cros_mount_point_provider.cc (right):

http://codereview.chromium.org/10910209/diff/1007/webkit/chromeos/fileapi/cro...
webkit/chromeos/fileapi/cros_mount_point_provider.cc:7: #include
"base/base_paths_posix.h"
On 2012/09/13 23:40:12, gab wrote:
> On 2012/09/13 23:32:32, satorux1 wrote:
> > just curious but why did it become necessary?
> 
> There should already have been an include to "base/base_paths.h", but this
> currently compiled somehow as it was likely transitively included from another
> header. Now that DIR_HOME (used below) moved to base/base_paths_posix.h, the
> include is required (it may still transitively compile, but I didn't try, I'm
a
> big fan of "include-what-you-use").

Thank you for the explanation! "include-what-you-use" is great.

[grt (UTC plus 2), 2012-09-14 15:35:16]

https://chromiumcodereview.appspot.com/10910209/diff/5016/base/base.gypi
File base/base.gypi (right):

https://chromiumcodereview.appspot.com/10910209/diff/5016/base/base.gypi#newc...
base/base.gypi:65: 'base_paths_posix.h',
move after .cc file to preserve order

https://chromiumcodereview.appspot.com/10910209/diff/5016/base/base_paths.h
File base/base_paths.h (left):

https://chromiumcodereview.appspot.com/10910209/diff/5016/base/base_paths.h#o...
base/base_paths.h:37: #if defined(OS_POSIX)
these paths are valid for all POSIX-like systems, whereas base_paths_posix.cc is
only for non-OSX and non-android systems.  by moving these two into
base_paths_posix.h, there's now a discontinuity in that base_paths_posix.h is
used on many platforms but base_paths_posix.cc is used on fewer.  i think this
is confusing.

https://chromiumcodereview.appspot.com/10910209/diff/5016/base/base_paths.h
File base/base_paths.h (right):

https://chromiumcodereview.appspot.com/10910209/diff/5016/base/base_paths.h#n...
base/base_paths.h:20: 
#if defined(OS_POSIX)
#include "base/base_paths_posix.h"
#endif

Platform-specific base_paths headers are an implementation detail.  The world
should include only base_paths.h.

https://chromiumcodereview.appspot.com/10910209/diff/5016/base/base_paths.h#n...
base/base_paths.h:37: DIR_USER_DESKTOP,  // Directory that correspond to the
current user's desktop.
"The current user's desktop."  (the above would need to be "...corresponds..."
to be grammatically correct, but it's overly wordy so i'd go with something more
concise.)

https://chromiumcodereview.appspot.com/10910209/diff/5016/base/base_paths_and...
File base/base_paths_android.h (right):

https://chromiumcodereview.appspot.com/10910209/diff/5016/base/base_paths_and...
base/base_paths_android.h:18: DIR_ANDROID_EXTERNAL_STORAGE,  // Android external
storage directory.
nice catch

[gab, 2012-09-14 18:10:06]

@grt: done, see comments below.

Also removed attempts at including the includes where unnecessary, will do in a
separate CL as discussed.

http://codereview.chromium.org/10910209/diff/5016/base/base.gypi
File base/base.gypi (right):

http://codereview.chromium.org/10910209/diff/5016/base/base.gypi#newcode65
base/base.gypi:65: 'base_paths_posix.h',
On 2012/09/14 15:35:16, grt wrote:
> move after .cc file to preserve order

N/A

http://codereview.chromium.org/10910209/diff/5016/base/base_paths.h
File base/base_paths.h (left):

http://codereview.chromium.org/10910209/diff/5016/base/base_paths.h#oldcode37
base/base_paths.h:37: #if defined(OS_POSIX)
On 2012/09/14 15:35:16, grt wrote:
> these paths are valid for all POSIX-like systems, whereas base_paths_posix.cc
is
> only for non-OSX and non-android systems.  by moving these two into
> base_paths_posix.h, there's now a discontinuity in that base_paths_posix.h is
> used on many platforms but base_paths_posix.cc is used on fewer.  i think this
> is confusing.

I agree; following the current convention in path_provider.cc that
PathProviderPosix is the default provider for POSIX OSes, but can be overridden
(as it is for Mac and Android), I renamed base_paths_posix.cc to
base_paths_default_posix.cc to make it clear that this is the default path
provider, not the absolute one, for POSIX.

http://codereview.chromium.org/10910209/diff/5016/base/base_paths.h
File base/base_paths.h (right):

http://codereview.chromium.org/10910209/diff/5016/base/base_paths.h#newcode20
base/base_paths.h:20: 
On 2012/09/14 15:35:16, grt wrote:
> #if defined(OS_POSIX)
> #include "base/base_paths_posix.h"
> #endif
> 
> Platform-specific base_paths headers are an implementation detail.  The world
> should include only base_paths.h.

Done.

http://codereview.chromium.org/10910209/diff/5016/base/base_paths.h#newcode37
base/base_paths.h:37: DIR_USER_DESKTOP,  // Directory that correspond to the
current user's desktop.
On 2012/09/14 15:35:16, grt wrote:
> "The current user's desktop."  (the above would need to be "...corresponds..."
> to be grammatically correct, but it's overly wordy so i'd go with something
more
> concise.)

Done.

http://codereview.chromium.org/10910209/diff/5016/base/base_paths_android.h
File base/base_paths_android.h (right):

http://codereview.chromium.org/10910209/diff/5016/base/base_paths_android.h#n...
base/base_paths_android.h:18: DIR_ANDROID_EXTERNAL_STORAGE,  // Android external
storage directory.
On 2012/09/14 15:35:16, grt wrote:
> nice catch

:)

[gab, 2012-09-14 18:27:24]

-jochen, satorux, phajdan.jr: I decided not to do the "include-what-you-use"
fixup as part of this CL to keep this change to a minimal number of
files/directories. I'll follow up in a subsequent CL. Thanks!

@brettw: Moving you to CC while I'm waiting for grt's approval, this will likely
sit inactive for a couple of days as we get through perf (and as we don't need
this in M23).

@grt: back to you when time allows :)!

[grt (UTC plus 2), 2012-09-14 18:44:57]

On 2012/09/14 18:27:24, gab wrote:
> @grt: back to you when time allows :)!

Thanks for your understanding.

[brettw, 2012-09-14 22:10:05]

http://codereview.chromium.org/10910209/diff/5027/base/base_paths_default_pos...
File base/base_paths_default_posix.cc (right):

http://codereview.chromium.org/10910209/diff/5027/base/base_paths_default_pos...
base/base_paths_default_posix.cc:5: // Defines base::PathProviderPosix which is
the default path provider for paths
Isn't this just used on Liunx then? I think base_paths_linux would be more
clear.

http://codereview.chromium.org/10910209/diff/5027/base/base_paths_mac.mm
File base/base_paths_mac.mm (right):

http://codereview.chromium.org/10910209/diff/5027/base/base_paths_mac.mm#newc...
base/base_paths_mac.mm:97: case base::DIR_CACHE:
I'd use {} here to be more in-line with the surrounding code (I realize this
whole switch is a bit inconsistent).

http://codereview.chromium.org/10910209/diff/5027/base/base_paths_posix.h
File base/base_paths_posix.h (right):

http://codereview.chromium.org/10910209/diff/5027/base/base_paths_posix.h#new...
base/base_paths_posix.h:5: #ifndef BASE_BASE_PATHS_POSIX_H__
Only single underscore at the end of these.

[gab, 2012-09-16 03:13:51]

nits adressed, see comment below on why base_paths_default_posix.cc.

Thanks,
Gab

http://codereview.chromium.org/10910209/diff/5027/base/base_paths_default_pos...
File base/base_paths_default_posix.cc (right):

http://codereview.chromium.org/10910209/diff/5027/base/base_paths_default_pos...
base/base_paths_default_posix.cc:5: // Defines base::PathProviderPosix which is
the default path provider for paths
On 2012/09/14 22:10:06, brettw wrote:
> Isn't this just used on Liunx then? I think base_paths_linux would be more
> clear.

I was saying the same thing, but grt@ didn't agree. Technically if you look at
the definition of OS_POSIX in build/build_config.h it's more than just linux
even when stripped from Mac and Android:
#if defined(OS_MACOSX) || defined(OS_LINUX) || defined(OS_FREEBSD) ||     \
    defined(OS_OPENBSD) || defined(OS_SOLARIS) || defined(OS_ANDROID) ||  \
    defined(OS_NACL)
#define OS_POSIX 1
#endif

However if you look at build\filename_rules.gypi:34
(http://code.google.com/searchframe#OAMlx_jo-ck/src/build/filename_rules.gypi&...):
_linux files are not excluded on *BSD OSes (although this is incorrect according
to TODO on line above so we should probably not worsen the situation; not to
mention that *BSD doesn't include SOLARIS...).

Overall, this file is really meant to be _posix imo, but it's overwritten by two
of the POSIX OSes we support, hence why I like the _default_posix.cc name to
make it obvious it's posix, but is overriden in some scenarios (maybe we could
even wrap the whole file in #if !defined(OS_ANROID) && !defined(OS_MACOSX) as
it's useless to compile it under those two OSes...

http://codereview.chromium.org/10910209/diff/5027/base/base_paths_mac.mm
File base/base_paths_mac.mm (right):

http://codereview.chromium.org/10910209/diff/5027/base/base_paths_mac.mm#newc...
base/base_paths_mac.mm:97: case base::DIR_CACHE:
On 2012/09/14 22:10:06, brettw wrote:
> I'd use {} here to be more in-line with the surrounding code (I realize this
> whole switch is a bit inconsistent).

Done (and made the file uniformly respect this syntax).

http://codereview.chromium.org/10910209/diff/5027/base/base_paths_posix.h
File base/base_paths_posix.h (right):

http://codereview.chromium.org/10910209/diff/5027/base/base_paths_posix.h#new...
base/base_paths_posix.h:5: #ifndef BASE_BASE_PATHS_POSIX_H__
On 2012/09/14 22:10:06, brettw wrote:
> Only single underscore at the end of these.

Done.

[brettw, 2012-09-16 04:25:00]

I'd be OK calling the file base_paths_posix.cc as well. default_posix doesn't
match anything else we have and I find it confusing.

[gab, 2012-09-16 15:16:02]

On 2012/09/16 04:25:00, brettw wrote:
> I'd be OK calling the file base_paths_posix.cc as well. default_posix doesn't
> match anything else we have and I find it confusing.

That's the original name which I changed in patch set 13 after grt's comment.

See full discussion at:
https://chromiumcodereview.appspot.com/10910209/diff/5016/base/base_paths.h#o...

I'm personally fine with keeping base_paths_posix.cc and making it clear (with
the comment I already added a the top of the file) that this is only the
default, but can overriden (and has been for Mac and Anroid).

[brettw, 2012-09-17 22:30:17]

grt: do you have any further comments? I think base_paths_default_posix is
weird. Personally I'd prefer _posix or _linux.

[grt (UTC plus 2), 2012-09-18 00:54:56]

On 2012/09/17 22:30:17, brettw wrote:
> grt: do you have any further comments? I think base_paths_default_posix is
> weird. Personally I'd prefer _posix or _linux.

_posix with documentation explaining sgtm.  i'll review the rest of the CL in a
day or so.  thanks for your patience, gab.

[grt (UTC plus 2), 2012-09-19 16:44:46]

looks good.  just a smattering of small comments.

https://chromiumcodereview.appspot.com/10910209/diff/14108/base/base_paths.cc
File base/base_paths.cc (left):

https://chromiumcodereview.appspot.com/10910209/diff/14108/base/base_paths.cc...
base/base_paths.cc:6: 
please put this newline back

https://chromiumcodereview.appspot.com/10910209/diff/14108/base/base_paths_de...
File base/base_paths_default_posix.cc (right):

https://chromiumcodereview.appspot.com/10910209/diff/14108/base/base_paths_de...
base/base_paths_default_posix.cc:5: // Defines base::PathProviderPosix which is
the default path provider for paths
"which is the " -> ", "
" for paths" -> ""

https://chromiumcodereview.appspot.com/10910209/diff/14108/base/base_paths_de...
base/base_paths_default_posix.cc:7: // all POSIX OSes, but Mac and Android).
"all POSIX OSes, but" -> "all but"

https://chromiumcodereview.appspot.com/10910209/diff/14108/base/base_paths_ma...
File base/base_paths_mac.mm (right):

https://chromiumcodereview.appspot.com/10910209/diff/14108/base/base_paths_ma...
base/base_paths_mac.mm:6: // OS_MAXOSX in base/path_service.cc.
"OS_MAXOSX" -> "Mac" for consistency with base_paths_posix.cc.

https://chromiumcodereview.appspot.com/10910209/diff/14108/base/base_paths_ma...
base/base_paths_mac.mm:56: case base::FILE_EXE: {
i think it's more consistent with Chromium style to omit the braces when not
needed.

https://chromiumcodereview.appspot.com/10910209/diff/14108/base/base_paths_wi...
File base/base_paths_win.cc (right):

https://chromiumcodereview.appspot.com/10910209/diff/14108/base/base_paths_wi...
base/base_paths_win.cc:25: // As per MSDN (http://goo.gl/U8Wcp) passing -1 for
|hToken| indicates the
re goo.gl: i'm not a fan of shortened urls here since it adds a dependency on
the shortener.  please use the msdn url directly here and elsewhere.

https://chromiumcodereview.appspot.com/10910209/diff/14108/base/base_paths_wi...
base/base_paths_win.cc:172: case base::DIR_USER_DESKTOP: {
nit: only use braces when necessary

https://chromiumcodereview.appspot.com/10910209/diff/14108/chrome/installer/u...
File chrome/installer/util/shell_util.cc (right):

https://chromiumcodereview.appspot.com/10910209/diff/14108/chrome/installer/u...
chrome/installer/util/shell_util.cc:1101: int dir_enum = system_level ?
base::DIR_COMMON_DESKTOP :
dir_enum -> key or dir_key for consistency with PathService::Get

https://chromiumcodereview.appspot.com/10910209/diff/14108/chrome/installer/u...
chrome/installer/util/shell_util.cc:1107: int dir_enum = system_level ?
base::DIR_DEFAULT_USER_QUICK_LAUNCH :
same comment here

https://chromiumcodereview.appspot.com/10910209/diff/14108/chrome/installer/u...
File chrome/installer/util/shell_util_unittest.cc (right):

https://chromiumcodereview.appspot.com/10910209/diff/14108/chrome/installer/u...
chrome/installer/util/shell_util_unittest.cc:28: virtual void SetUp() {
OVERRIDE

https://chromiumcodereview.appspot.com/10910209/diff/14108/chrome/installer/u...
chrome/installer/util/shell_util_unittest.cc:35:
EXPECT_TRUE(PathService::Override(base::DIR_USER_DESKTOP,
why not ASSERT_TRUE?  should the test continue if these calls to Override fail?

[gab, 2012-09-19 18:22:58]

Done + 1 comment on comment.

https://chromiumcodereview.appspot.com/10910209/diff/14108/base/base_paths.cc
File base/base_paths.cc (left):

https://chromiumcodereview.appspot.com/10910209/diff/14108/base/base_paths.cc...
base/base_paths.cc:6: 
On 2012/09/19 16:44:46, grt wrote:
> please put this newline back

Done.

https://chromiumcodereview.appspot.com/10910209/diff/14108/base/base_paths_de...
File base/base_paths_default_posix.cc (right):

https://chromiumcodereview.appspot.com/10910209/diff/14108/base/base_paths_de...
base/base_paths_default_posix.cc:5: // Defines base::PathProviderPosix which is
the default path provider for paths
On 2012/09/19 16:44:46, grt wrote:
> "which is the " -> ", "
> " for paths" -> ""

Done.

https://chromiumcodereview.appspot.com/10910209/diff/14108/base/base_paths_de...
base/base_paths_default_posix.cc:7: // all POSIX OSes, but Mac and Android).
On 2012/09/19 16:44:46, grt wrote:
> "all POSIX OSes, but" -> "all but"

Done.

https://chromiumcodereview.appspot.com/10910209/diff/14108/base/base_paths_ma...
File base/base_paths_mac.mm (right):

https://chromiumcodereview.appspot.com/10910209/diff/14108/base/base_paths_ma...
base/base_paths_mac.mm:6: // OS_MAXOSX in base/path_service.cc.
On 2012/09/19 16:44:46, grt wrote:
> "OS_MAXOSX" -> "Mac" for consistency with base_paths_posix.cc.

Done.

https://chromiumcodereview.appspot.com/10910209/diff/14108/base/base_paths_ma...
base/base_paths_mac.mm:56: case base::FILE_EXE: {
On 2012/09/19 16:44:46, grt wrote:
> i think it's more consistent with Chromium style to omit the braces when not
> needed.

Ok removed it for all but the two big ones (with #ifdefs) as I feel it's easier
to for the reader to be able to do bracket matching in the text editor to see
how far the handling of each big case goes...

(this seems to be the rule already used in base_paths_win.cc.

https://chromiumcodereview.appspot.com/10910209/diff/14108/base/base_paths_wi...
File base/base_paths_win.cc (right):

https://chromiumcodereview.appspot.com/10910209/diff/14108/base/base_paths_wi...
base/base_paths_win.cc:25: // As per MSDN (http://goo.gl/U8Wcp) passing -1 for
|hToken| indicates the
On 2012/09/19 16:44:46, grt wrote:
> re goo.gl: i'm not a fan of shortened urls here since it adds a dependency on
> the shortener.  please use the msdn url directly here and elsewhere.

Done.

https://chromiumcodereview.appspot.com/10910209/diff/14108/base/base_paths_wi...
base/base_paths_win.cc:172: case base::DIR_USER_DESKTOP: {
On 2012/09/19 16:44:46, grt wrote:
> nit: only use braces when necessary

Done.

https://chromiumcodereview.appspot.com/10910209/diff/14108/chrome/installer/u...
File chrome/installer/util/shell_util.cc (right):

https://chromiumcodereview.appspot.com/10910209/diff/14108/chrome/installer/u...
chrome/installer/util/shell_util.cc:1101: int dir_enum = system_level ?
base::DIR_COMMON_DESKTOP :
On 2012/09/19 16:44:46, grt wrote:
> dir_enum -> key or dir_key for consistency with PathService::Get

Done.

https://chromiumcodereview.appspot.com/10910209/diff/14108/chrome/installer/u...
chrome/installer/util/shell_util.cc:1107: int dir_enum = system_level ?
base::DIR_DEFAULT_USER_QUICK_LAUNCH :
On 2012/09/19 16:44:46, grt wrote:
> same comment here

Done.

https://chromiumcodereview.appspot.com/10910209/diff/14108/chrome/installer/u...
File chrome/installer/util/shell_util_unittest.cc (right):

https://chromiumcodereview.appspot.com/10910209/diff/14108/chrome/installer/u...
chrome/installer/util/shell_util_unittest.cc:28: virtual void SetUp() {
On 2012/09/19 16:44:46, grt wrote:
> OVERRIDE

Done (and also done in profile_shorcut_manager_unittest_win.cc).

https://chromiumcodereview.appspot.com/10910209/diff/14108/chrome/installer/u...
chrome/installer/util/shell_util_unittest.cc:35:
EXPECT_TRUE(PathService::Override(base::DIR_USER_DESKTOP,
On 2012/09/19 16:44:46, grt wrote:
> why not ASSERT_TRUE?  should the test continue if these calls to Override
fail?

Well the test as a whole can still pass if Override fails (i.e. by creating and
testing shortcuts on the real Desktop).

The thinking was that the test would fail (as there would be an EXPECT error at
the end), but the developer investigating would know the rest of the test passed
(i.e. not other errors) so that the problem is the Override (which is likely a
problem external to this test).

[grt (UTC plus 2), 2012-09-19 19:12:06]

lgtm w/ the EXPECT -> ASSERT change discussed offline.

https://chromiumcodereview.appspot.com/10910209/diff/14108/base/base_paths_ma...
File base/base_paths_mac.mm (right):

https://chromiumcodereview.appspot.com/10910209/diff/14108/base/base_paths_ma...
base/base_paths_mac.mm:56: case base::FILE_EXE: {
On 2012/09/19 18:22:58, gab wrote:
> On 2012/09/19 16:44:46, grt wrote:
> > i think it's more consistent with Chromium style to omit the braces when not
> > needed.
> 
> Ok removed it for all but the two big ones (with #ifdefs) as I feel it's
easier
> to for the reader to be able to do bracket matching in the text editor to see
> how far the handling of each big case goes...

i don't understand this logic; handling goes until the next "case" statement. 
DIR_APP_DATA needs the braces, DIR_SOURCE_ROOT doesn't.  i'm okay if you leave
it this way, but it seems arbitrary to me, whereas "braces when required" is
simple.

[gab, 2012-09-19 19:44:25]

Done.

@brettw: over to you for OWNER approval.

Thanks,
Gab

https://chromiumcodereview.appspot.com/10910209/diff/14108/base/base_paths_ma...
File base/base_paths_mac.mm (right):

https://chromiumcodereview.appspot.com/10910209/diff/14108/base/base_paths_ma...
base/base_paths_mac.mm:56: case base::FILE_EXE: {
On 2012/09/19 19:12:06, grt wrote:
> On 2012/09/19 18:22:58, gab wrote:
> > On 2012/09/19 16:44:46, grt wrote:
> > > i think it's more consistent with Chromium style to omit the braces when
not
> > > needed.
> > 
> > Ok removed it for all but the two big ones (with #ifdefs) as I feel it's
> easier
> > to for the reader to be able to do bracket matching in the text editor to
see
> > how far the handling of each big case goes...
> 
> i don't understand this logic; handling goes until the next "case" statement. 
> DIR_APP_DATA needs the braces, DIR_SOURCE_ROOT doesn't.  i'm okay if you leave
> it this way, but it seems arbitrary to me, whereas "braces when required" is
> simple.

You're right, better to go by the simple rule, done.

[brettw, 2012-09-19 20:53:29]

lgtm

[commit-bot: I haz the power, 2012-09-19 21:14:55]

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

[commit-bot: I haz the power, 2012-09-19 21:15:03]

Presubmit check for 10910209-17007 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit Warnings **
Found lines longer than 80 characters (first 5 shown).
  base/base_paths_win.cc, line 42, 121 chars

Presubmit checks took 2.4s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[M-A Ruel, 2012-09-19 21:30:29]

http://codereview.chromium.org/10910209/diff/17007/base/base_paths_win.cc
File base/base_paths_win.cc (right):

http://codereview.chromium.org/10910209/diff/17007/base/base_paths_win.cc#new...
base/base_paths_win.cc:26: //
http://msdn.microsoft.com/en-us/library/windows/desktop/bb762181(v=vs.85).aspx
// http://msdn.microsoft.com/library/windows/desktop/bb762181.aspx
works well.

http://codereview.chromium.org/10910209/diff/17007/base/base_paths_win.cc#new...
base/base_paths_win.cc:41: //
http://stackoverflow.com/questions/76080/how-do-you-reliably-get-the-quick-la...
http://stackoverflow.com/questions/76080/how-do-you-
works without problem.

http://codereview.chromium.org/10910209/diff/17007/base/base_paths_win.cc#new...
base/base_paths_win.cc:42: //
http://blogs.technet.com/b/heyscriptingguy/archive/2005/09/01/how-can-i-add-a...
That's technet, eh.

Just split it on two lines. The hard limit is 120 chars.

[gab, 2012-09-19 21:38:30]

Thanks for the tips M-A. Tweaked the links as described in comments below.

http://codereview.chromium.org/10910209/diff/17007/base/base_paths_win.cc
File base/base_paths_win.cc (right):

http://codereview.chromium.org/10910209/diff/17007/base/base_paths_win.cc#new...
base/base_paths_win.cc:26: //
http://msdn.microsoft.com/en-us/library/windows/desktop/bb762181(v=vs.85).aspx
On 2012/09/19 21:30:29, Marc-Antoine Ruel wrote:
> // http://msdn.microsoft.com/library/windows/desktop/bb762181.aspx
> works well.

Done.

http://codereview.chromium.org/10910209/diff/17007/base/base_paths_win.cc#new...
base/base_paths_win.cc:41: //
http://stackoverflow.com/questions/76080/how-do-you-reliably-get-the-quick-la...
On 2012/09/19 21:30:29, Marc-Antoine Ruel wrote:
> http://stackoverflow.com/questions/76080/how-do-you-
> works without problem.

Sweet, didn't know you could do this to stack overflow links :)!

Changed to
http://stackoverflow.com/questions/76080/how-do-you-reliably-get-the-quick- to
give as much context as possible while respecting 80 chars.

http://codereview.chromium.org/10910209/diff/17007/base/base_paths_win.cc#new...
base/base_paths_win.cc:42: //
http://blogs.technet.com/b/heyscriptingguy/archive/2005/09/01/how-can-i-add-a...
On 2012/09/19 21:30:29, Marc-Antoine Ruel wrote:
> That's technet, eh.
> 
> Just split it on two lines. The hard limit is 120 chars.

Found the original link that got me there
(http://www.microsoft.com/technet/scriptcenter/resources/qanda/sept05/hey0901....)
which is shorter than the technet archive link.

[commit-bot: I haz the power, 2012-09-19 21:39:26]

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

[commit-bot: I haz the power, 2012-09-20 00:13:42]

Change committed as 157667