Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10826188:
Sharing shell_util_unittest code to verify shortcuts (Closed)
DescriptionSharing shell_util_unittest code to verify shortcuts for creation of profile shortcuts for Windows. profile_manager_unittest needs to be able to verify shortcuts created as well.
BUG=NONE
TEST=installer_util_unittests --gtest_filter=ShellUtilTest*
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=151091
Patch Set 1 #
Total comments: 29
Patch Set 2 : #
Total comments: 20
Patch Set 3 : #
Total comments: 4
Patch Set 4 : #Patch Set 5 : #
Total comments: 6
Patch Set 6 : #
Total comments: 14
Patch Set 7 : #
Total comments: 10
Patch Set 8 : #Patch Set 9 : #
Total comments: 2
Patch Set 10 : #
Messages
Total messages: 21 (0 generated)
http://codereview.chromium.org/10826188/diff/1/chrome/installer/util/shell_ut... File chrome/installer/util/shell_util.h (right): http://codereview.chromium.org/10826188/diff/1/chrome/installer/util/shell_ut... chrome/installer/util/shell_util.h:446: // |icon_index| in master prefs I got presubmit warnings about using wide strings, are they OK to use here?
@robertshield: Please see @ comment for you below :)! ---------- I'm sure there is a good reason to expose this, but please clarify that reason in this CL's description :). Also please add BUG=NONE TEST=NONE Thanks! Gab http://codereview.chromium.org/10826188/diff/1/chrome/installer/util/shell_ut... File chrome/installer/util/shell_util.cc (right): http://codereview.chromium.org/10826188/diff/1/chrome/installer/util/shell_ut... chrome/installer/util/shell_util.cc:1687: base::win::ScopedComPtr<IShellLink> i_shell_link; #include "base/win/scoped_comptr.h" http://codereview.chromium.org/10826188/diff/1/chrome/installer/util/shell_ut... chrome/installer/util/shell_util.cc:1688: base::win::ScopedComPtr<IPersistFile> i_persist_file; IPersistFile is said to come from ObjIdl.h by MSDN: http://msdn.microsoft.com/en-us/library/windows/desktop/ms687223(v=vs.85).aspx @robertshield: is including windows.h and shlobj.h sufficient here? not clear from documentation... http://codereview.chromium.org/10826188/diff/1/chrome/installer/util/shell_ut... chrome/installer/util/shell_util.cc:1691: bool failed = FAILED(i_shell_link.CreateInstance(CLSID_ShellLink, NULL, The value of this variable is never used except in the if block. Remove this variable and change this (and all of the similar blocks below) to: if (FAILED(i_shell_link....)) return false; (as per the style in http://goo.gl/n5NuS) Also, please remove logging, this adds extra strings to chrome.dll and setup.exe (and we try to avoid bloating those already big targets -- furthermore we never get to see those logs in the wild -- you can ASSERT_TRUE() on the result of this in tests and if the assert fails a developer can debug where it failed manually). http://codereview.chromium.org/10826188/diff/1/chrome/installer/util/shell_ut... File chrome/installer/util/shell_util.h (right): http://codereview.chromium.org/10826188/diff/1/chrome/installer/util/shell_ut... chrome/installer/util/shell_util.h:442: // Verify that a shortcut exists with expected information (for testing) s/with/with the/ Remove "(for testing)". Add '.' at the end. http://codereview.chromium.org/10826188/diff/1/chrome/installer/util/shell_ut... chrome/installer/util/shell_util.h:446: // |icon_index| in master prefs On 2012/08/07 17:16:52, Halli wrote: > I got presubmit warnings about using wide strings, are they OK to use here? No, replace all std::wstring by string16. http://codereview.chromium.org/10826188/diff/1/chrome/installer/util/shell_ut... chrome/installer/util/shell_util.h:446: // |icon_index| in master prefs |icon_index| The icon's index in the exe. http://codereview.chromium.org/10826188/diff/1/chrome/installer/util/shell_ut... chrome/installer/util/shell_util.h:448: const std::wstring& shortcut, ident to match variable above http://codereview.chromium.org/10826188/diff/1/chrome/installer/util/shell_ut... File chrome/installer/util/shell_util_unittest.cc (right): http://codereview.chromium.org/10826188/diff/1/chrome/installer/util/shell_ut... chrome/installer/util/shell_util_unittest.cc:7: #include <shlobj.h> I don't think any of these include are needed anymore as well. http://codereview.chromium.org/10826188/diff/1/chrome/installer/util/shell_ut... chrome/installer/util/shell_util_unittest.cc:16: #include "base/string16.h" optional: Not your fault, but while you're at it cleaning includes... this file includes string16.h and not <string> which is the correct thing to do, but also means all std::wstring should be changed to string16 as per the Chromium style, if you can clean this up with this CL that'd be greatly appreciated :)! http://codereview.chromium.org/10826188/diff/1/chrome/installer/util/shell_ut... chrome/installer/util/shell_util_unittest.cc:18: #include "base/win/scoped_comptr.h" Remove this include. http://codereview.chromium.org/10826188/diff/1/chrome/installer/util/shell_ut... chrome/installer/util/shell_util_unittest.cc:26: namespace { nit: Please add a blank line here (as per the Chromium style). http://codereview.chromium.org/10826188/diff/1/chrome/installer/util/shell_ut... chrome/installer/util/shell_util_unittest.cc:38: }; nit: Blank line here as well. http://codereview.chromium.org/10826188/diff/1/chrome/installer/util/shell_ut... chrome/installer/util/shell_util_unittest.cc:90: shortcut_path.value(), ident variable to same level as the one above it (same comment for all other changes below)
http://codereview.chromium.org/10826188/diff/1/chrome/installer/util/shell_ut... File chrome/installer/util/shell_util.cc (right): http://codereview.chromium.org/10826188/diff/1/chrome/installer/util/shell_ut... chrome/installer/util/shell_util.cc:1687: base::win::ScopedComPtr<IShellLink> i_shell_link; On 2012/08/07 19:18:25, gab wrote: > #include "base/win/scoped_comptr.h" This is included already. http://codereview.chromium.org/10826188/diff/1/chrome/installer/util/shell_ut... chrome/installer/util/shell_util.cc:1691: bool failed = FAILED(i_shell_link.CreateInstance(CLSID_ShellLink, NULL, On 2012/08/07 19:18:25, gab wrote: > The value of this variable is never used except in the if block. Remove this > variable and change this (and all of the similar blocks below) to: > > if (FAILED(i_shell_link....)) > return false; > > (as per the style in http://goo.gl/n5NuS) > > Also, please remove logging, this adds extra strings to chrome.dll and setup.exe > (and we try to avoid bloating those already big targets -- furthermore we never > get to see those logs in the wild -- you can ASSERT_TRUE() on the result of this > in tests and if the assert fails a developer can debug where it failed > manually). Done. http://codereview.chromium.org/10826188/diff/1/chrome/installer/util/shell_ut... File chrome/installer/util/shell_util.h (right): http://codereview.chromium.org/10826188/diff/1/chrome/installer/util/shell_ut... chrome/installer/util/shell_util.h:442: // Verify that a shortcut exists with expected information (for testing) On 2012/08/07 19:18:25, gab wrote: > s/with/with the/ > > Remove "(for testing)". > > Add '.' at the end. Done. http://codereview.chromium.org/10826188/diff/1/chrome/installer/util/shell_ut... chrome/installer/util/shell_util.h:446: // |icon_index| in master prefs On 2012/08/07 19:18:25, gab wrote: > |icon_index| The icon's index in the exe. Done. http://codereview.chromium.org/10826188/diff/1/chrome/installer/util/shell_ut... chrome/installer/util/shell_util.h:446: // |icon_index| in master prefs On 2012/08/07 19:18:25, gab wrote: > On 2012/08/07 17:16:52, Halli wrote: > > I got presubmit warnings about using wide strings, are they OK to use here? > > No, replace all std::wstring by string16. Done. http://codereview.chromium.org/10826188/diff/1/chrome/installer/util/shell_ut... chrome/installer/util/shell_util.h:448: const std::wstring& shortcut, On 2012/08/07 19:18:25, gab wrote: > ident to match variable above Done. http://codereview.chromium.org/10826188/diff/1/chrome/installer/util/shell_ut... File chrome/installer/util/shell_util_unittest.cc (right): http://codereview.chromium.org/10826188/diff/1/chrome/installer/util/shell_ut... chrome/installer/util/shell_util_unittest.cc:7: #include <shlobj.h> On 2012/08/07 19:18:25, gab wrote: > I don't think any of these include are needed anymore as well. shlobj.h is used for IsUserAnAdmin() Done. http://codereview.chromium.org/10826188/diff/1/chrome/installer/util/shell_ut... chrome/installer/util/shell_util_unittest.cc:16: #include "base/string16.h" On 2012/08/07 19:18:25, gab wrote: > optional: Not your fault, but while you're at it cleaning includes... this file > includes string16.h and not <string> which is the correct thing to do, but also > means all std::wstring should be changed to string16 as per the Chromium style, > if you can clean this up with this CL that'd be greatly appreciated :)! Done. http://codereview.chromium.org/10826188/diff/1/chrome/installer/util/shell_ut... chrome/installer/util/shell_util_unittest.cc:18: #include "base/win/scoped_comptr.h" On 2012/08/07 19:18:25, gab wrote: > Remove this include. Done. http://codereview.chromium.org/10826188/diff/1/chrome/installer/util/shell_ut... chrome/installer/util/shell_util_unittest.cc:26: namespace { On 2012/08/07 19:18:25, gab wrote: > nit: Please add a blank line here (as per the Chromium style). Done. http://codereview.chromium.org/10826188/diff/1/chrome/installer/util/shell_ut... chrome/installer/util/shell_util_unittest.cc:38: }; On 2012/08/07 19:18:25, gab wrote: > nit: Blank line here as well. Done. http://codereview.chromium.org/10826188/diff/1/chrome/installer/util/shell_ut... chrome/installer/util/shell_util_unittest.cc:90: shortcut_path.value(), On 2012/08/07 19:18:25, gab wrote: > ident variable to same level as the one above it (same comment for all other > changes below) Done.
More comments, thanks for the string16 fixes :)! Cheers, Gab http://codereview.chromium.org/10826188/diff/1/chrome/installer/util/shell_ut... File chrome/installer/util/shell_util.cc (right): http://codereview.chromium.org/10826188/diff/1/chrome/installer/util/shell_ut... chrome/installer/util/shell_util.cc:1687: base::win::ScopedComPtr<IShellLink> i_shell_link; On 2012/08/07 20:52:16, Halli wrote: > On 2012/08/07 19:18:25, gab wrote: > > #include "base/win/scoped_comptr.h" > This is included already. No, scoped_ptr.h is not scoped_comptr.h, it might be implicitly included, but we have an include-what-you-use policy in Chromium/Google C++ style-guide. http://codereview.chromium.org/10826188/diff/6001/chrome/installer/util/shell... File chrome/installer/util/shell_util.cc (right): http://codereview.chromium.org/10826188/diff/6001/chrome/installer/util/shell... chrome/installer/util/shell_util.cc:1736: // Failed to get IShellLink Remove these "Failed *" comments (here and below), the code is explicit enough. http://codereview.chromium.org/10826188/diff/6001/chrome/installer/util/shell... chrome/installer/util/shell_util.cc:1738: CLSCTX_INPROC_SERVER))) nit: indentation http://codereview.chromium.org/10826188/diff/6001/chrome/installer/util/shell... chrome/installer/util/shell_util.cc:1754: if (((::GetLongPathName(exe_path.c_str(), long_path, MAX_PATH) == 0) || In the conditionals all the way from here to the bottom of this function there a bunch of unnecessary extra parentheses. Please remove unnecessary parentheses. http://codereview.chromium.org/10826188/diff/6001/chrome/installer/util/shell... chrome/installer/util/shell_util.cc:1755: (::GetShortPathName(exe_path.c_str(), short_path, MAX_PATH) == 0))) nit: indentation http://codereview.chromium.org/10826188/diff/6001/chrome/installer/util/shell... chrome/installer/util/shell_util.cc:1762: SLGP_UNCPRIORITY))) || nit: indentation http://codereview.chromium.org/10826188/diff/6001/chrome/installer/util/shell... chrome/installer/util/shell_util.cc:1763: ((FilePath(file_path) != FilePath(long_path)) && nit: indentation http://codereview.chromium.org/10826188/diff/6001/chrome/installer/util/shell... chrome/installer/util/shell_util.cc:1777: if (((FAILED(i_shell_link->GetIconLocation(icon_path, MAX_PATH, Thinking more about it (and chatting with robert) since this is used in test code we likely want to know what went wrong, but as mentioned before we can't add a bunch of logs to shell_util.cc. I think your best bet is to have an enum return type that states the status of things and then EXPECT_EQ(VERIFY_SHORTCUT_SUCCESS, ...) in tests. You probably don't want an enum for each individual interface failure. I would put all the interface getters in a single if at the top and then do individual checks for the actual properties being validated. http://codereview.chromium.org/10826188/diff/6001/chrome/installer/util/shell... File chrome/installer/util/shell_util.h (right): http://codereview.chromium.org/10826188/diff/6001/chrome/installer/util/shell... chrome/installer/util/shell_util.h:445: // Verify that a shortcut exists with expected information. add "the" after "with" http://codereview.chromium.org/10826188/diff/6001/chrome/installer/util/shell... chrome/installer/util/shell_util.h:449: // |icon_index| in the exe Please make the definition of |icon_index| "The icon's index in the exe.", i.e. the variable name shouldn't be considered part of the description sentence :).
One more comment below! Also, I forgot to say this in my last comment: when you sync between patch sets, it's best practice to upload a patch set with only the sync changes and then upload another patchset with your actual changes (otherwise it's hard for the reviewerer to tell what *you* actually changed in the diff). Thanks! Gab http://codereview.chromium.org/10826188/diff/1/chrome/installer/util/shell_ut... File chrome/installer/util/shell_util.cc (right): http://codereview.chromium.org/10826188/diff/1/chrome/installer/util/shell_ut... chrome/installer/util/shell_util.cc:1688: base::win::ScopedComPtr<IPersistFile> i_persist_file; On 2012/08/07 19:18:25, gab wrote: > IPersistFile is said to come from ObjIdl.h by MSDN: > http://msdn.microsoft.com/en-us/library/windows/desktop/ms687223%28v=vs.85%29... > > @robertshield: is including windows.h and shlobj.h sufficient here? not clear > from documentation... Discussed with robert offline, although we like include-what-you-use, Windows headers are messed up and it's fine in this case to include shlobj.h (already included) and not add Objidl.h.
http://codereview.chromium.org/10826188/diff/1/chrome/installer/util/shell_ut... File chrome/installer/util/shell_util.cc (right): http://codereview.chromium.org/10826188/diff/1/chrome/installer/util/shell_ut... chrome/installer/util/shell_util.cc:1687: base::win::ScopedComPtr<IShellLink> i_shell_link; Line 34 (: On 2012/08/08 16:24:26, gab wrote: > On 2012/08/07 20:52:16, Halli wrote: > > On 2012/08/07 19:18:25, gab wrote: > > > #include "base/win/scoped_comptr.h" > > This is included already. > > No, scoped_ptr.h is not scoped_comptr.h, it might be implicitly included, but we > have an include-what-you-use policy in Chromium/Google C++ style-guide. http://codereview.chromium.org/10826188/diff/6001/chrome/installer/util/shell... File chrome/installer/util/shell_util.cc (right): http://codereview.chromium.org/10826188/diff/6001/chrome/installer/util/shell... chrome/installer/util/shell_util.cc:1736: // Failed to get IShellLink On 2012/08/08 16:24:27, gab wrote: > Remove these "Failed *" comments (here and below), the code is explicit enough. Done. http://codereview.chromium.org/10826188/diff/6001/chrome/installer/util/shell... chrome/installer/util/shell_util.cc:1738: CLSCTX_INPROC_SERVER))) On 2012/08/08 16:24:27, gab wrote: > nit: indentation Done. http://codereview.chromium.org/10826188/diff/6001/chrome/installer/util/shell... chrome/installer/util/shell_util.cc:1754: if (((::GetLongPathName(exe_path.c_str(), long_path, MAX_PATH) == 0) || On 2012/08/08 16:24:27, gab wrote: > In the conditionals all the way from here to the bottom of this function there a > bunch of unnecessary extra parentheses. > Please remove unnecessary parentheses. Done. http://codereview.chromium.org/10826188/diff/6001/chrome/installer/util/shell... chrome/installer/util/shell_util.cc:1755: (::GetShortPathName(exe_path.c_str(), short_path, MAX_PATH) == 0))) On 2012/08/08 16:24:27, gab wrote: > nit: indentation Done. http://codereview.chromium.org/10826188/diff/6001/chrome/installer/util/shell... chrome/installer/util/shell_util.cc:1762: SLGP_UNCPRIORITY))) || On 2012/08/08 16:24:27, gab wrote: > nit: indentation Done. http://codereview.chromium.org/10826188/diff/6001/chrome/installer/util/shell... chrome/installer/util/shell_util.cc:1763: ((FilePath(file_path) != FilePath(long_path)) && On 2012/08/08 16:24:27, gab wrote: > nit: indentation Done. http://codereview.chromium.org/10826188/diff/6001/chrome/installer/util/shell... chrome/installer/util/shell_util.cc:1777: if (((FAILED(i_shell_link->GetIconLocation(icon_path, MAX_PATH, How is this different from returning true or false since you don't want a different enum for each failure? On 2012/08/08 16:24:27, gab wrote: > Thinking more about it (and chatting with robert) since this is used in test > code we likely want to know what went wrong, but as mentioned before we can't > add a bunch of logs to shell_util.cc. > > I think your best bet is to have an enum return type that states the status of > things and then EXPECT_EQ(VERIFY_SHORTCUT_SUCCESS, ...) in tests. > > You probably don't want an enum for each individual interface failure. I would > put all the interface getters in a single if at the top and then do individual > checks for the actual properties being validated. http://codereview.chromium.org/10826188/diff/6001/chrome/installer/util/shell... File chrome/installer/util/shell_util.h (right): http://codereview.chromium.org/10826188/diff/6001/chrome/installer/util/shell... chrome/installer/util/shell_util.h:445: // Verify that a shortcut exists with expected information. On 2012/08/08 16:24:27, gab wrote: > add "the" after "with" Done. http://codereview.chromium.org/10826188/diff/6001/chrome/installer/util/shell... chrome/installer/util/shell_util.h:449: // |icon_index| in the exe On 2012/08/08 16:24:27, gab wrote: > Please make the definition of |icon_index| "The icon's index in the exe.", i.e. > the variable name shouldn't be considered part of the description sentence :). Done.
[robertshield -> cc] Comment on main comment. http://codereview.chromium.org/10826188/diff/6001/chrome/installer/util/shell... File chrome/installer/util/shell_util.cc (right): http://codereview.chromium.org/10826188/diff/6001/chrome/installer/util/shell... chrome/installer/util/shell_util.cc:1777: if (((FAILED(i_shell_link->GetIconLocation(icon_path, MAX_PATH, I don't want a different one for the getters (i.e. i_shell_link->Get*), etc., but then I do want a different one for comparison failures. That's why I suggested grouping the getters at the top in a single if (i.e. ifs with the same outcome should be grouped) and then just do the comparisons in independent ifs returning various failure possibilities. On 2012/08/08 18:59:13, Halli wrote: > How is this different from returning true or false since you don't want a > different enum for each failure? > On 2012/08/08 16:24:27, gab wrote: > > Thinking more about it (and chatting with robert) since this is used in test > > code we likely want to know what went wrong, but as mentioned before we can't > > add a bunch of logs to shell_util.cc. > > > > I think your best bet is to have an enum return type that states the status of > > things and then EXPECT_EQ(VERIFY_SHORTCUT_SUCCESS, ...) in tests. > > > > You probably don't want an enum for each individual interface failure. I would > > put all the interface getters in a single if at the top and then do individual > > checks for the actual properties being validated. > http://codereview.chromium.org/10826188/diff/7/chrome/installer/util/shell_ut... File chrome/installer/util/shell_util.cc (right): http://codereview.chromium.org/10826188/diff/7/chrome/installer/util/shell_ut... chrome/installer/util/shell_util.cc:1756: // File path did not match exe path nit: remove this comment as well http://codereview.chromium.org/10826188/diff/7/chrome/installer/util/shell_ut... chrome/installer/util/shell_util.cc:1758: SLGP_UNCPRIORITY)) || FilePath(file_path) != FilePath(long_path) && align SLGP_UNCPRIORITY with file_path above and put the next condition on the next line http://codereview.chromium.org/10826188/diff/7/chrome/installer/util/shell_ut... File chrome/installer/util/shell_util.h (right): http://codereview.chromium.org/10826188/diff/7/chrome/installer/util/shell_ut... chrome/installer/util/shell_util.h:446: // |exe_path| shortcut's exe nit: Please all of these descriptions complete sentences, e.g. "The shorcut's exe."
FYI, ref: lint error in shell_util.cc, in general reitveld lint sucks at end-of-file new line detection. In general I would avoid putting your change at the end of the file (i.e. putting it closer to the other shortcut code probably makes more sense anyways). If moving the code however please make one patch set upload for the move and one for the code changes (otherwise the diff is the whole thing and I can't see what code actually changed (as opposed to just moved)). Thanks! Gab
http://codereview.chromium.org/10826188/diff/6001/chrome/installer/util/shell... File chrome/installer/util/shell_util.cc (right): http://codereview.chromium.org/10826188/diff/6001/chrome/installer/util/shell... chrome/installer/util/shell_util.cc:1777: if (((FAILED(i_shell_link->GetIconLocation(icon_path, MAX_PATH, Hopefully this is what you meant (: On 2012/08/08 19:17:08, gab wrote: > I don't want a different one for the getters (i.e. i_shell_link->Get*), etc., > but then I do want a different one for comparison failures. > > That's why I suggested grouping the getters at the top in a single if (i.e. ifs > with the same outcome should be grouped) and then just do the comparisons in > independent ifs returning various failure possibilities. > > On 2012/08/08 18:59:13, Halli wrote: > > How is this different from returning true or false since you don't want a > > different enum for each failure? > > On 2012/08/08 16:24:27, gab wrote: > > > Thinking more about it (and chatting with robert) since this is used in test > > > code we likely want to know what went wrong, but as mentioned before we > can't > > > add a bunch of logs to shell_util.cc. > > > > > > I think your best bet is to have an enum return type that states the status > of > > > things and then EXPECT_EQ(VERIFY_SHORTCUT_SUCCESS, ...) in tests. > > > > > > You probably don't want an enum for each individual interface failure. I > would > > > put all the interface getters in a single if at the top and then do > individual > > > checks for the actual properties being validated. > > > http://codereview.chromium.org/10826188/diff/7/chrome/installer/util/shell_ut... File chrome/installer/util/shell_util.h (right): http://codereview.chromium.org/10826188/diff/7/chrome/installer/util/shell_ut... chrome/installer/util/shell_util.h:446: // |exe_path| shortcut's exe On 2012/08/08 19:17:08, gab wrote: > nit: Please all of these descriptions complete sentences, > e.g. "The shorcut's exe." Done.
See comments below. Cheers! Gab http://codereview.chromium.org/10826188/diff/12004/chrome/installer/util/shel... File chrome/installer/util/shell_util.cc (right): http://codereview.chromium.org/10826188/diff/12004/chrome/installer/util/shel... chrome/installer/util/shell_util.cc:1690: if (FAILED(i_shell_link->GetPath(file_path, MAX_PATH, NULL, It's the opposite we want. i.e. the things that should never fail (CreateInstance, Load shortcut, Get*) should all be grouped in one and return something like VERIFY_SHORTCUT_FAILURE_UNEXPECTED. Comparisons however should be split and report on which comparison failed through the enum if any. http://codereview.chromium.org/10826188/diff/12004/chrome/installer/util/shel... File chrome/installer/util/shell_util.h (right): http://codereview.chromium.org/10826188/diff/12004/chrome/installer/util/shel... chrome/installer/util/shell_util.h:459: static VerifyShortcuts VerifyChromeShortcut(const string16& exe_path, Also move this to align it with its new position in shell_util.cc. http://codereview.chromium.org/10826188/diff/12004/chrome/installer/util/shel... File chrome/installer/util/shell_util_unittest.cc (right): http://codereview.chromium.org/10826188/diff/12004/chrome/installer/util/shel... chrome/installer/util/shell_util_unittest.cc:63: shortcut_path.value(), description, 0)); align this with variables in the VerifyChromeShorcut() call (here and below).
http://codereview.chromium.org/10826188/diff/12004/chrome/installer/util/shel... File chrome/installer/util/shell_util.cc (right): http://codereview.chromium.org/10826188/diff/12004/chrome/installer/util/shel... chrome/installer/util/shell_util.cc:1690: if (FAILED(i_shell_link->GetPath(file_path, MAX_PATH, NULL, On 2012/08/08 20:41:14, gab wrote: > It's the opposite we want. > > i.e. the things that should never fail (CreateInstance, Load shortcut, Get*) > should all be grouped in one and return something like > VERIFY_SHORTCUT_FAILURE_UNEXPECTED. Comparisons however should be split and > report on which comparison failed through the enum if any. Done.
looks great! a couple more comments and you're good to go :) Looking at your new description I feel like you are only exposing this for another set of tests (in which case it doesn't belong in shell_util.cc)? Or will you also need this in the actual profile code? Cheers, Gab http://codereview.chromium.org/10826188/diff/12004/chrome/installer/util/shel... File chrome/installer/util/shell_util.h (right): http://codereview.chromium.org/10826188/diff/12004/chrome/installer/util/shel... chrome/installer/util/shell_util.h:459: static VerifyShortcuts VerifyChromeShortcut(const string16& exe_path, On 2012/08/08 20:41:14, gab wrote: > Also move this to align it with its new position in shell_util.cc. Ping. http://codereview.chromium.org/10826188/diff/2004/chrome/installer/util/shell... File chrome/installer/util/shell_util.cc (right): http://codereview.chromium.org/10826188/diff/2004/chrome/installer/util/shell... chrome/installer/util/shell_util.cc:1665: nit: Remove this line to have all declarations grouped in one block. http://codereview.chromium.org/10826188/diff/2004/chrome/installer/util/shell... chrome/installer/util/shell_util.cc:1673: // Get pointer to the IShellLink interface Change this comment to something like "Get the shortcut's properties." http://codereview.chromium.org/10826188/diff/2004/chrome/installer/util/shell... chrome/installer/util/shell_util.cc:1681: SLGP_UNCPRIORITY)) || nit: align with |file_path| http://codereview.chromium.org/10826188/diff/2004/chrome/installer/util/shell... chrome/installer/util/shell_util.cc:1687: if (FilePath(file_path) != FilePath(long_path) && Change && to || http://codereview.chromium.org/10826188/diff/2004/chrome/installer/util/shell... chrome/installer/util/shell_util.cc:1688: FilePath(file_path) != FilePath(short_path)) nit: indent one more space http://codereview.chromium.org/10826188/diff/2004/chrome/installer/util/shell... chrome/installer/util/shell_util.cc:1688: FilePath(file_path) != FilePath(short_path)) nit: as-per chromium style, since this conditional spans more than one line, surround the return statement in curly braces. http://codereview.chromium.org/10826188/diff/2004/chrome/installer/util/shell... chrome/installer/util/shell_util.cc:1688: FilePath(file_path) != FilePath(short_path)) You are creating a FilePath(file_path) twice, extract that in a constant variable right above this if.
On 2012/08/09 02:52:31, gab wrote: > looks great! a couple more comments and you're good to go :) > > Looking at your new description I feel like you are only exposing this for > another set of tests (in which case it doesn't belong in shell_util.cc)? Or will > you also need this in the actual profile code? > > Cheers, > Gab > > http://codereview.chromium.org/10826188/diff/12004/chrome/installer/util/shel... > File chrome/installer/util/shell_util.h (right): > > http://codereview.chromium.org/10826188/diff/12004/chrome/installer/util/shel... > chrome/installer/util/shell_util.h:459: static VerifyShortcuts > VerifyChromeShortcut(const string16& exe_path, > On 2012/08/08 20:41:14, gab wrote: > > Also move this to align it with its new position in shell_util.cc. > > Ping. > > http://codereview.chromium.org/10826188/diff/2004/chrome/installer/util/shell... > File chrome/installer/util/shell_util.cc (right): > > http://codereview.chromium.org/10826188/diff/2004/chrome/installer/util/shell... > chrome/installer/util/shell_util.cc:1665: > nit: Remove this line to have all declarations grouped in one block. > > http://codereview.chromium.org/10826188/diff/2004/chrome/installer/util/shell... > chrome/installer/util/shell_util.cc:1673: // Get pointer to the IShellLink > interface > Change this comment to something like "Get the shortcut's properties." > > http://codereview.chromium.org/10826188/diff/2004/chrome/installer/util/shell... > chrome/installer/util/shell_util.cc:1681: SLGP_UNCPRIORITY)) || > nit: align with |file_path| > > http://codereview.chromium.org/10826188/diff/2004/chrome/installer/util/shell... > chrome/installer/util/shell_util.cc:1687: if (FilePath(file_path) != > FilePath(long_path) && > Change && to || > > http://codereview.chromium.org/10826188/diff/2004/chrome/installer/util/shell... > chrome/installer/util/shell_util.cc:1688: FilePath(file_path) != > FilePath(short_path)) > nit: indent one more space > > http://codereview.chromium.org/10826188/diff/2004/chrome/installer/util/shell... > chrome/installer/util/shell_util.cc:1688: FilePath(file_path) != > FilePath(short_path)) > nit: as-per chromium style, since this conditional spans more than one line, > surround the return statement in curly braces. > > http://codereview.chromium.org/10826188/diff/2004/chrome/installer/util/shell... > chrome/installer/util/shell_util.cc:1688: FilePath(file_path) != > FilePath(short_path)) > You are creating a FilePath(file_path) twice, extract that in a constant > variable right above this if. Oh also, instead of TEST=NONE, you probably at least want TEST=installer_util_unittests --gtest_filter=ShellUtilTest*
Right now we are just using it for testing profile shortcuts. Since the original function was in the anonymous namespace in shell_util_unittest we would have had to copy code to use it. We haven't determined if we're going to use this function in profile's code yet. http://codereview.chromium.org/10826188/diff/12004/chrome/installer/util/shel... File chrome/installer/util/shell_util.h (right): http://codereview.chromium.org/10826188/diff/12004/chrome/installer/util/shel... chrome/installer/util/shell_util.h:459: static VerifyShortcuts VerifyChromeShortcut(const string16& exe_path, On 2012/08/09 02:52:32, gab wrote: > On 2012/08/08 20:41:14, gab wrote: > > Also move this to align it with its new position in shell_util.cc. > > Ping. Done. http://codereview.chromium.org/10826188/diff/2004/chrome/installer/util/shell... File chrome/installer/util/shell_util.cc (right): http://codereview.chromium.org/10826188/diff/2004/chrome/installer/util/shell... chrome/installer/util/shell_util.cc:1665: On 2012/08/09 02:52:32, gab wrote: > nit: Remove this line to have all declarations grouped in one block. Done. http://codereview.chromium.org/10826188/diff/2004/chrome/installer/util/shell... chrome/installer/util/shell_util.cc:1673: // Get pointer to the IShellLink interface On 2012/08/09 02:52:32, gab wrote: > Change this comment to something like "Get the shortcut's properties." Done. http://codereview.chromium.org/10826188/diff/2004/chrome/installer/util/shell... chrome/installer/util/shell_util.cc:1681: SLGP_UNCPRIORITY)) || On 2012/08/09 02:52:32, gab wrote: > nit: align with |file_path| Done. http://codereview.chromium.org/10826188/diff/2004/chrome/installer/util/shell... chrome/installer/util/shell_util.cc:1687: if (FilePath(file_path) != FilePath(long_path) && Original code uses && Isn't && correct? I think this On 2012/08/09 02:52:32, gab wrote: > Change && to || http://codereview.chromium.org/10826188/diff/2004/chrome/installer/util/shell... chrome/installer/util/shell_util.cc:1688: FilePath(file_path) != FilePath(short_path)) On 2012/08/09 02:52:32, gab wrote: > nit: indent one more space Done. http://codereview.chromium.org/10826188/diff/2004/chrome/installer/util/shell... chrome/installer/util/shell_util.cc:1688: FilePath(file_path) != FilePath(short_path)) On 2012/08/09 02:52:32, gab wrote: > You are creating a FilePath(file_path) twice, extract that in a constant > variable right above this if. Done. http://codereview.chromium.org/10826188/diff/2004/chrome/installer/util/shell... chrome/installer/util/shell_util.cc:1688: FilePath(file_path) != FilePath(short_path)) Line is less than 80 chars with the constant from below (: On 2012/08/09 02:52:32, gab wrote: > nit: as-per chromium style, since this conditional spans more than one line, > surround the return statement in curly braces.
Ok, given this is only for test: ideally it would be in a test_support target. Me and Robert don't know that it's worth it to create a support target for this single function though... you'll have to make sure to add installer_util as a link dependency to your profile tests. Also, I just realized that I put that comment at the bottom of the previous stack of comments so you probably missed it, but: "instead of TEST=NONE in the description, you probably at least want TEST=installer_util_unittests --gtest_filter=ShellUtilTest*" Looks great! Couple more things and you're good to go :)! Cheers, Gab http://codereview.chromium.org/10826188/diff/7008/chrome/installer/util/shell... File chrome/installer/util/shell_util.cc (right): http://codereview.chromium.org/10826188/diff/7008/chrome/installer/util/shell... chrome/installer/util/shell_util.cc:1687: nit: no empty line between variable and if (i.e. since this variable is only used in that if I feel it visually belongs to the same "section" of code). http://codereview.chromium.org/10826188/diff/7008/chrome/installer/util/shell... File chrome/installer/util/shell_util.h (right): http://codereview.chromium.org/10826188/diff/7008/chrome/installer/util/shell... chrome/installer/util/shell_util.h:38: enum VerifyShortcuts { nit: Rename this to VerifyShortcutStatus to be more explicit about this type. http://codereview.chromium.org/10826188/diff/7008/chrome/installer/util/shell... chrome/installer/util/shell_util.h:39: VERIFY_SHORTCUT_SUCCESS, nit: Set this first enum to 0 explicitly as per chromium-style (e.g. as in shell_util.cc:55) http://codereview.chromium.org/10826188/diff/7008/chrome/installer/util/shell... chrome/installer/util/shell_util.h:425: // |shortcut| The path to shortcut. +"the" after "to" http://codereview.chromium.org/10826188/diff/7008/chrome/installer/util/shell... File chrome/installer/util/shell_util_unittest.cc (right): http://codereview.chromium.org/10826188/diff/7008/chrome/installer/util/shell... chrome/installer/util/shell_util_unittest.cc:90: shortcut_path.value(), description, 1)); Align shortcut_path.value() with other VerifyChromeShortcut arguments (here and for all calls below). Alternatively, put all parameters on a newline, indented 4 spaces; whichever takes less lines :).
http://codereview.chromium.org/10826188/diff/7008/chrome/installer/util/shell... File chrome/installer/util/shell_util.cc (right): http://codereview.chromium.org/10826188/diff/7008/chrome/installer/util/shell... chrome/installer/util/shell_util.cc:1687: On 2012/08/09 19:25:57, gab wrote: > nit: no empty line between variable and if (i.e. since this variable is only > used in that if I feel it visually belongs to the same "section" of code). Done. http://codereview.chromium.org/10826188/diff/7008/chrome/installer/util/shell... File chrome/installer/util/shell_util.h (right): http://codereview.chromium.org/10826188/diff/7008/chrome/installer/util/shell... chrome/installer/util/shell_util.h:38: enum VerifyShortcuts { On 2012/08/09 19:25:57, gab wrote: > nit: Rename this to VerifyShortcutStatus to be more explicit about this type. Done. http://codereview.chromium.org/10826188/diff/7008/chrome/installer/util/shell... chrome/installer/util/shell_util.h:39: VERIFY_SHORTCUT_SUCCESS, On 2012/08/09 19:25:57, gab wrote: > nit: Set this first enum to 0 explicitly as per chromium-style (e.g. as in > shell_util.cc:55) Done. http://codereview.chromium.org/10826188/diff/7008/chrome/installer/util/shell... chrome/installer/util/shell_util.h:425: // |shortcut| The path to shortcut. On 2012/08/09 19:25:57, gab wrote: > +"the" after "to" Done. http://codereview.chromium.org/10826188/diff/7008/chrome/installer/util/shell... File chrome/installer/util/shell_util_unittest.cc (right): http://codereview.chromium.org/10826188/diff/7008/chrome/installer/util/shell... chrome/installer/util/shell_util_unittest.cc:90: shortcut_path.value(), description, 1)); On 2012/08/09 19:25:57, gab wrote: > Align shortcut_path.value() with other VerifyChromeShortcut arguments (here and > for all calls below). > > Alternatively, put all parameters on a newline, indented 4 spaces; whichever > takes less lines :). Done.
LGTM (with a single nit below), thanks for baring with me! Cheers, Gab http://codereview.chromium.org/10826188/diff/4012/chrome/installer/util/shell... File chrome/installer/util/shell_util.h (right): http://codereview.chromium.org/10826188/diff/4012/chrome/installer/util/shell... chrome/installer/util/shell_util.h:43: VERIFY_SHORTCUT_FAILURE_ICON_INDEX nit: add ',' after last item as well (as per chromium-style, this makes the diffs smaller when adding a new line...)
http://codereview.chromium.org/10826188/diff/4012/chrome/installer/util/shell... File chrome/installer/util/shell_util.h (right): http://codereview.chromium.org/10826188/diff/4012/chrome/installer/util/shell... chrome/installer/util/shell_util.h:43: VERIFY_SHORTCUT_FAILURE_ICON_INDEX On 2012/08/10 13:46:00, gab wrote: > nit: add ',' after last item as well (as per chromium-style, this makes the > diffs smaller when adding a new line...) Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hallielaine@chromium.org/10826188/11004
Change committed as 151091 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
