Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
413003:
Cleaning up Code. (Closed)
|
Created:
11 years, 1 month ago by Shriram Modified:
9 years, 7 months ago CC:
chromium-reviews Base URL:
http://src.chromium.org/svn/trunk/src/ Visibility:
Public. |
Description1. Changes program() to GetProgram() which returns program part
of command line string. Get Program()'s used for converting program in wstring type (present in the old chromium code) to FilePath.
2. Uses DCHECK_EQ instead of DCHECK. As per Peter Kasting's comment,
DCHECK_EQ used to make logic shorter.
BUG=none
TEST=none
committed as r46281
Patch Set 1 #Patch Set 2 : '' #Patch Set 3 : '' #Patch Set 4 : '' #Patch Set 5 : '' #
Total comments: 2
Patch Set 6 : '' #Patch Set 7 : '' #
Total comments: 3
Patch Set 8 : '' #Patch Set 9 : '' #Patch Set 10 : '' #Patch Set 11 : '' #
Total comments: 4
Patch Set 12 : '' #Patch Set 13 : '' #Patch Set 14 : '' #Patch Set 15 : '' #
Total comments: 4
Patch Set 16 : '' #
Total comments: 4
Patch Set 17 : '' #
Total comments: 3
Patch Set 18 : '' #Patch Set 19 : '' #Patch Set 20 : '' #
Total comments: 6
Patch Set 21 : '' #Patch Set 22 : '' #Patch Set 23 : '' #Patch Set 24 : '' #Patch Set 25 : '' #Patch Set 26 : '' #Patch Set 27 : '' #Messages
Total messages: 29 (0 generated)
patch set 3 doesn't apply cleanly against ToT (tip of the tree) @ r32820.
Ready for review
You need to follow the coding style guide. http://www.chromium.org/developers/coding-style http://codereview.chromium.org/413003/diff/6010/6012 File base/command_line.cc (right): http://codereview.chromium.org/413003/diff/6010/6012#newcode270 base/command_line.cc:270: std::wstring CommandLine::GetProgram() const { I don't understand why you renamed the function when you should be trying to remove it. http://codereview.chromium.org/413003/diff/6010/6018 File chrome/installer/setup/setup_main.cc (right): http://codereview.chromium.org/413003/diff/6010/6018#newcode114 chrome/installer/setup/setup_main.cc:114: if (!file_util::CreateNewTempDirectory(std::wstring(L"chrome_"),&(FilePath::FromWStringHack(temp_path)))) { std::wstring(L"chrome_") only works on Windows: const FilePath::StringType is not wstring on Unix.
Still a lot of style guide violations. http://codereview.chromium.org/413003/diff/13011/12003 File base/command_line.cc (right): http://codereview.chromium.org/413003/diff/13011/12003#newcode262 base/command_line.cc:262: nit: why are you adding newlines? Did the lint tool complain? http://codereview.chromium.org/413003/diff/13011/12004 File base/command_line_unittest.cc (right): http://codereview.chromium.org/413003/diff/13011/12004#newcode42 base/command_line_unittest.cc:42: EXPECT_EQ(FilePath(FILE_PATH_LITERAL("program")).value(), cl.GetProgram().value()); Does FILE_PATH_LITERAL("program") not work here? http://codereview.chromium.org/413003/diff/13011/12008 File chrome/browser/importer/firefox_profile_lock_unittest.cc (right): http://codereview.chromium.org/413003/diff/13011/12008#newcode49 chrome/browser/importer/firefox_profile_lock_unittest.cc:49: ASSERT_TRUE(file_util::CreateNewTempDirectory(FILE_PATH_LITERAL("firefox_profile"),&(FilePath::FromWStringHack(test_path)) )); nit: style (and in other places too)
You still haven't fixed the lint errors.
Before I fix the lint errors, I am trying to see if I can replace some other occurrences of std::wstring(L" and I tried to place the find results in http://google.pastebin.com/m41dce131 Now, my question is whether I should restrict to base/ for now as other ones seem to be missing header files let me know On Tue, Dec 8, 2009 at 1:51 PM, <thestig@chromium.org> wrote: > You still haven't fixed the lint errors. > > > > > http://codereview.chromium.org/413003 >
I don't see any more lint errors for the latest patch but upon gcl lint <Changelist name> , I do get the the following errors @ http://google.pastebin.com/m4b72ca82 On 12/8/09, Shriram Kunchanapalli <kshriram18@gmail.com> wrote: > Before I fix the lint errors, I am trying to see if I can replace some > other occurrences of std::wstring(L" > and I tried to place the find results in > http://google.pastebin.com/m41dce131 > > Now, my question is whether I should restrict to base/ for now as > other ones seem to be missing header files > let me know > > On Tue, Dec 8, 2009 at 1:51 PM, <thestig@chromium.org> wrote: > >> You still haven't fixed the lint errors. >> >> >> >> >> http://codereview.chromium.org/413003 >> >
You have passed the automated linter, but your style is still wrong. Please read the style guide. (I won't say it again) http://codereview.chromium.org/413003/diff/12037/13040 File base/file_util.h (left): http://codereview.chromium.org/413003/diff/12037/13040#oldcode287 base/file_util.h:287: std::wstring* new_temp_path); Someone else already fixed this in r32487. http://codereview.chromium.org/413003/diff/12037/13046 File chrome/browser/importer/firefox_profile_lock_unittest.cc (right): http://codereview.chromium.org/413003/diff/12037/13046#newcode51 chrome/browser/importer/firefox_profile_lock_unittest.cc:51: &(FilePath::FromWStringHack(test_path))) ); test_path is an out parameter, I don't think this works. But that's a moot point because someone else already fixed this. http://codereview.chromium.org/413003/diff/12037/13044 File chrome/browser/shell_integration_win.cc (right): http://codereview.chromium.org/413003/diff/12037/13044#newcode116 chrome/browser/shell_integration_win.cc:116: GetShortPathName((command_line.GetProgram()).ToWStringHack().c_str(), Extra parenthesis. http://codereview.chromium.org/413003/diff/12037/13043 File chrome/test/mini_installer_test/run_all_unittests.cc (right): http://codereview.chromium.org/413003/diff/12037/13043#newcode57 chrome/test/mini_installer_test/run_all_unittests.cc:57: (command_line.GetProgram().ToWStringHack()).c_str()); extra parenthesis
couple of lint errors as can be seen in the http://google.pastebin.com/m5dfa701 for build_time generated files like registered_dlls.h in chrome\installer\setup\uninstall.cc , can that be ignored? I mean it's possible to just have the directory listed there based on debug or release build On 12/9/09, thestig@chromium.org <thestig@chromium.org> wrote: > You have passed the automated linter, but your style is still wrong. Please > read > the style guide. (I won't say it again) > > > http://codereview.chromium.org/413003/diff/12037/13040 > File base/file_util.h (left): > > http://codereview.chromium.org/413003/diff/12037/13040#oldcode287 > base/file_util.h:287: std::wstring* new_temp_path); > Someone else already fixed this in r32487. > > http://codereview.chromium.org/413003/diff/12037/13046 > File chrome/browser/importer/firefox_profile_lock_unittest.cc (right): > > http://codereview.chromium.org/413003/diff/12037/13046#newcode51 > chrome/browser/importer/firefox_profile_lock_unittest.cc:51: > &(FilePath::FromWStringHack(test_path))) ); > test_path is an out parameter, I don't think this works. But that's a > moot point because someone else already fixed this. > > http://codereview.chromium.org/413003/diff/12037/13044 > File chrome/browser/shell_integration_win.cc (right): > > http://codereview.chromium.org/413003/diff/12037/13044#newcode116 > chrome/browser/shell_integration_win.cc:116: > GetShortPathName((command_line.GetProgram()).ToWStringHack().c_str(), > Extra parenthesis. > > http://codereview.chromium.org/413003/diff/12037/13043 > File chrome/test/mini_installer_test/run_all_unittests.cc (right): > > http://codereview.chromium.org/413003/diff/12037/13043#newcode57 > chrome/test/mini_installer_test/run_all_unittests.cc:57: > (command_line.GetProgram().ToWStringHack()).c_str()); > extra parenthesis > > http://codereview.chromium.org/413003 >
On 2009/12/10 04:31:12, Sam wrote: > couple of lint errors as can be seen in the http://google.pastebin.com/m5dfa701 > for build_time generated files like registered_dlls.h in > chrome\installer\setup\uninstall.cc , > can that be ignored? > I mean it's possible to just have the directory listed there based on > debug or release build Don't worry too much about lint errors not caused by your change.
http://codereview.chromium.org/413003/diff/23005/29023 File base/command_line.cc (right): http://codereview.chromium.org/413003/diff/23005/29023#newcode259 base/command_line.cc:259: std::wstring CommandLine::program() const { It's weird that you removed the other implementation of program() but not this one. http://codereview.chromium.org/413003/diff/23005/29023#newcode383 base/command_line.cc:383: other.GetProgram().empty()); Can you format it as: DCHECK(include_program ? other.GetProgram().empty() : other.GetProgram().empty()); http://codereview.chromium.org/413003/diff/23005/29024 File base/command_line_unittest.cc (right): http://codereview.chromium.org/413003/diff/23005/29024#newcode94 base/command_line_unittest.cc:94: EXPECT_EQ(0, cl.argv().size()); This needs to be 0U, otherwise you'll get a sign error. http://codereview.chromium.org/413003/diff/23005/29030 File chrome/installer/util/shell_util.cc (right): http://codereview.chromium.org/413003/diff/23005/29030#newcode84 chrome/installer/util/shell_util.cc:84: // TODO(my_user): use CommandLine API instead of constructing command lines. Please leave these alone since you don't know whose TODO list to put it on.
On 2009/12/15 08:37:26, Lei Zhang wrote: > http://codereview.chromium.org/413003/diff/23005/29023 > File base/command_line.cc (right): > > http://codereview.chromium.org/413003/diff/23005/29023#newcode259 > base/command_line.cc:259: std::wstring CommandLine::program() const { > It's weird that you removed the other implementation of program() but not this > one. Indeed. fixed now. > http://codereview.chromium.org/413003/diff/23005/29030 > File chrome/installer/util/shell_util.cc (right): > > http://codereview.chromium.org/413003/diff/23005/29030#newcode84 > chrome/installer/util/shell_util.cc:84: // TODO(my_user): use CommandLine API > instead of constructing command lines. > Please leave these alone since you don't know whose TODO list to put it on. I had to put that info. to fix the lint error. Fixed now. good for review
http://codereview.chromium.org/413003/diff/31001/31002 File base/command_line.cc (right): http://codereview.chromium.org/413003/diff/31001/31002#newcode296 base/command_line.cc:296: ) + value_string; Please revert this change. The lack of indentation looks wrong. http://codereview.chromium.org/413003/diff/31001/31002#newcode343 base/command_line.cc:343: other.GetProgram().empty()); Align other... with !other... http://codereview.chromium.org/413003/diff/31001/31002#newcode384 base/command_line.cc:384: other.GetProgram().empty()); Be consistent with the DCHECK() on line 342. http://codereview.chromium.org/413003/diff/31001/31003 File base/command_line_unittest.cc (right): http://codereview.chromium.org/413003/diff/31001/31003#newcode42 base/command_line_unittest.cc:42: EXPECT_EQ(FILE_PATH_LITERAL("foo"), cl.GetProgram().value()); Why did the expected value change here?
LGTM with formatting errors fixed. http://codereview.chromium.org/413003/diff/36001/37001 File base/command_line.cc (right): http://codereview.chromium.org/413003/diff/36001/37001#newcode296 base/command_line.cc:296: ) + value_string; You didn't put this back to where it was. http://codereview.chromium.org/413003/diff/36001/37001#newcode382 base/command_line.cc:382: DCHECK(include_program ?!other.GetProgram().empty() : nit: space between ? and !
Passed the try servers on patch set 20. However, it looks like you got rid of all calls to program(). Is that the case? If so, can't you just get rid of program()?
looks like program_ needs the program() call as per command_line.cc. maybe creating a FilePath variable instead of a wstring variable like program_ would help to remove the program() . not sure if that would be a hassle. On 2010/01/25 20:47:27, Lei Zhang wrote: > Passed the try servers on patch set 20. However, it looks like you got rid of > all calls to program(). Is that the case? If so, can't you just get rid of > program()?
Nit: Please update all files' copyrights to be "Copyright (c) 2010" http://codereview.chromium.org/413003/diff/43001/43002 File base/command_line.cc (right): http://codereview.chromium.org/413003/diff/43001/43002#newcode344 base/command_line.cc:344: DCHECK(include_program ? !other.GetProgram().empty() : Nit: Shorter: DCHECK_EQ(include_program, !other.GetProgram().empty()); (two places) http://codereview.chromium.org/413003/diff/43001/43009 File chrome/installer/util/shell_util.cc (right): http://codereview.chromium.org/413003/diff/43001/43009#newcode224 chrome/installer/util/shell_util.cc:224: Nit: Unneeded blank line http://codereview.chromium.org/413003/diff/43001/43009#newcode250 chrome/installer/util/shell_util.cc:250: DISALLOW_COPY_AND_ASSIGN(RegistryEntry); Nit: Blank line above this http://codereview.chromium.org/413003/diff/43001/43004 File chrome/test/mini_installer_test/run_all_unittests.cc (right): http://codereview.chromium.org/413003/diff/43001/43004#newcode10 chrome/test/mini_installer_test/run_all_unittests.cc:10: #include "chrome/test/mini_installer_test/chrome_mini_installer.h" Nit: Alphabetical order http://codereview.chromium.org/413003/diff/43001/43004#newcode57 chrome/test/mini_installer_test/run_all_unittests.cc:57: command_line.GetProgram().ToWStringHack().c_str()); Nit: I believe you can remove ToWStringHack() and use the %s formatting in printf(), no? Or are there encoding issues?
http://codereview.chromium.org/413003/diff/36001/37001 File base/command_line.cc (right): http://codereview.chromium.org/413003/diff/36001/37001#newcode296 base/command_line.cc:296: ) + value_string; On 2010/01/12 19:07:05, Lei Zhang wrote: > You didn't put this back to where it was. fixed it now. I kept messing with that line to get rid of a gcl lint error at the risk of a formatting error: upon doing gcl lint , I get base\command_line.cc:296: Extra space before ) [whitespace/parents] [2] I got rid of this error previously by removing whitespaces completely before the ) As you mentioned, due to the formatting error, I decided to put it back but not to where it was :( http://codereview.chromium.org/413003/diff/43001/43004 File chrome/test/mini_installer_test/run_all_unittests.cc (right): http://codereview.chromium.org/413003/diff/43001/43004#newcode57 chrome/test/mini_installer_test/run_all_unittests.cc:57: command_line.GetProgram().ToWStringHack().c_str()); Yeah encoding issues. I think we need the %ls. May be there's another way to remove the ToWStringHack
On 2010/03/13 10:48:36, Sam wrote ignore the comment regarding the File base/command_line.cc
On 2010/03/13 10:49:43, Sam wrote: > On 2010/03/13 10:48:36, Sam wrote > ignore the comment regarding the File base/command_line.cc Can you sync your tree? It's old enough that this patch no longer applies to ToT.
I think there is a typo at the BUG= description, the number doesn't point to a valid page. Could you add a TEST= line too?
LGTM, since the CL is pretty trivial now. Can you write a better description for your CL? When you do, I will commit http://codereview.chromium.org/1733010/show.
Done. Is that ok ? On 2010/04/22 21:19:24, Lei Zhang wrote:
On 2010/04/25 10:54:49, Sam wrote: > Done. Is that ok ? No, I don't think that's an accurate description of this CL. You referred to this as bug 413003 on IRC. FYI, this is not a bug, this is a code review.
On 2010/04/26 19:55:07, Lei Zhang wrote: Ok. As this's a codereview, isn't the BUG=none the a proper indicator of it? I shall make it more accurate in that case.
On 2010/04/27 03:18:40, Sam wrote: > On 2010/04/26 19:55:07, Lei Zhang wrote: > > Ok. As this's a codereview, isn't the BUG=none the a proper indicator of it? > I shall make it more accurate in that case. You should write in the description, what you are trying to fix. If you have the bug number, that could help figure out what you are trying to do here.
On 2010/04/27 03:56:10, tfarina wrote: I added some more details in the description, as what needs to be changed or fixed as per one of the comments on this issue. As this really isn't a bug, no bug number to include in the description.
On 2010/04/27 07:51:10, Sam wrote: > On 2010/04/27 03:56:10, tfarina wrote: > > I added some more details in the description, as what needs to be changed or > fixed as per one of the comments on this issue. > As this really isn't a bug, no bug number to include in the description. Alright, but IMO, just a *nit* can't be a justification for a patch.
may be this could be ignored , and this cl be merged/combined with a much larger cl On Tue, Apr 27, 2010 at 8:54 PM, <thiago.farina@gmail.com> wrote: > On 2010/04/27 07:51:10, Sam wrote: > >> On 2010/04/27 03:56:10, tfarina wrote: >> > > I added some more details in the description, as what needs to be changed >> or >> fixed as per one of the comments on this issue. >> As this really isn't a bug, no bug number to include in the description. >> > > Alright, but IMO, just a *nit* can't be a justification for a patch. > > > http://codereview.chromium.org/413003/show > |
