Rename CommandLine::GetCommandLineString().

Rename CommandLine::GetCommandLineString().
Fix string hackery in net/tools/dump_cache/dump_cache.cc
Fix const casts in chrome/installer/util/product.cc and base/process_util_win.cc.

BUG=73195
TEST=none

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

[msw, 2011-07-15 20:27:30]

PTAL, thanks.

[Evan Martin, 2011-07-15 20:31:32]

Wait a sec, wasn't the plan to remove this?

http://codereview.chromium.org/7386002/diff/2001/chrome/browser/about_flags_u...
File chrome/browser/about_flags_unittest.cc (left):

http://codereview.chromium.org/7386002/diff/2001/chrome/browser/about_flags_u...
chrome/browser/about_flags_unittest.cc:254:
command_line.GetSwitchValueASCII(kSwitch1));
Not that it matters, but I think the reasoning for this type of code is that
when the EXPECT fails, it prints the input values, while in your new code it'll
just print "false".

[msw, 2011-07-15 20:42:54]

Reducing uses and ultimately removing the string generation might be good.
I can't commit to that work immediately, but wanted to address the renaming
TODO.

http://codereview.chromium.org/7386002/diff/2001/chrome/browser/about_flags_u...
File chrome/browser/about_flags_unittest.cc (left):

http://codereview.chromium.org/7386002/diff/2001/chrome/browser/about_flags_u...
chrome/browser/about_flags_unittest.cc:254:
command_line.GetSwitchValueASCII(kSwitch1));
On 2011/07/15 20:31:32, Evan Martin wrote:
> Not that it matters, but I think the reasoning for this type of code is that
> when the EXPECT fails, it prints the input values, while in your new code
it'll
> just print "false".

Good to know, I can revert that if you'd like.

[Evan Martin, 2011-07-15 21:05:16]

I guess since you've already written the patch it's fine, but I worry that
callers will start thinking they should use this function; if I were to rename
it, I'd rename it GetCommandLineStringDoNotUse() or something.  :(

http://codereview.chromium.org/7386002/diff/2001/chrome/browser/web_applicati...
File chrome/browser/web_applications/web_app.cc (left):

http://codereview.chromium.org/7386002/diff/2001/chrome/browser/web_applicati...
chrome/browser/web_applications/web_app.cc:336: // fact.
This TODO still exists, then.

http://codereview.chromium.org/7386002/diff/2001/chrome/installer/util/produc...
File chrome/installer/util/product.cc (right):

http://codereview.chromium.org/7386002/diff/2001/chrome/installer/util/produc...
chrome/installer/util/product.cc:86: // we don't care.
This feels pretty dangerous to me; for example, the PLOG() below will do the
wrong thing depending on whether we're reusing a member or not.  Sigh.

[Evan Martin, 2011-07-15 21:09:54]

and LGTM

On Fri, Jul 15, 2011 at 2:05 PM,  <evan@chromium.org> wrote:
> I guess since you've already written the patch it's fine, but I worry that
> callers will start thinking they should use this function; if I were to
> rename
> it, I'd rename it GetCommandLineStringDoNotUse() or something.  :(
>
>
>
http://codereview.chromium.org/7386002/diff/2001/chrome/browser/web_applicati...
> File chrome/browser/web_applications/web_app.cc (left):
>
>
http://codereview.chromium.org/7386002/diff/2001/chrome/browser/web_applicati...
> chrome/browser/web_applications/web_app.cc:336: // fact.
> This TODO still exists, then.
>
>
http://codereview.chromium.org/7386002/diff/2001/chrome/installer/util/produc...
> File chrome/installer/util/product.cc (right):
>
>
http://codereview.chromium.org/7386002/diff/2001/chrome/installer/util/produc...
> chrome/installer/util/product.cc:86: // we don't care.
> This feels pretty dangerous to me; for example, the PLOG() below will do
> the wrong thing depending on whether we're reusing a member or not.
> Sigh.
>
> http://codereview.chromium.org/7386002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To post to this group, send email to native-client-reviews@googlegroups.com.
To unsubscribe from this group, send email to
native-client-reviews+unsubscribe@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/native-client-reviews?hl=en.

[Evan Martin, 2011-07-15 21:09:55]

and LGTM

On Fri, Jul 15, 2011 at 2:05 PM,  <evan@chromium.org> wrote:
> I guess since you've already written the patch it's fine, but I worry that
> callers will start thinking they should use this function; if I were to
> rename
> it, I'd rename it GetCommandLineStringDoNotUse() or something.  :(
>
>
>
http://codereview.chromium.org/7386002/diff/2001/chrome/browser/web_applicati...
> File chrome/browser/web_applications/web_app.cc (left):
>
>
http://codereview.chromium.org/7386002/diff/2001/chrome/browser/web_applicati...
> chrome/browser/web_applications/web_app.cc:336: // fact.
> This TODO still exists, then.
>
>
http://codereview.chromium.org/7386002/diff/2001/chrome/installer/util/produc...
> File chrome/installer/util/product.cc (right):
>
>
http://codereview.chromium.org/7386002/diff/2001/chrome/installer/util/produc...
> chrome/installer/util/product.cc:86: // we don't care.
> This feels pretty dangerous to me; for example, the PLOG() below will do
> the wrong thing depending on whether we're reusing a member or not.
> Sigh.
>
> http://codereview.chromium.org/7386002/
>

[msw, 2011-07-18 20:59:32]

Please take another quick look, as I tried to fixup the const casting in
chrome/installer/util/product.cc and base/process_util_win.cc.

http://codereview.chromium.org/7386002/diff/2001/chrome/browser/about_flags_u...
File chrome/browser/about_flags_unittest.cc (left):

http://codereview.chromium.org/7386002/diff/2001/chrome/browser/about_flags_u...
chrome/browser/about_flags_unittest.cc:254:
command_line.GetSwitchValueASCII(kSwitch1));
On 2011/07/15 20:42:55, msw wrote:
> On 2011/07/15 20:31:32, Evan Martin wrote:
> > Not that it matters, but I think the reasoning for this type of code is that
> > when the EXPECT fails, it prints the input values, while in your new code
> it'll
> > just print "false".
> 
> Good to know, I can revert that if you'd like.

Reverted.

http://codereview.chromium.org/7386002/diff/2001/chrome/browser/web_applicati...
File chrome/browser/web_applications/web_app.cc (left):

http://codereview.chromium.org/7386002/diff/2001/chrome/browser/web_applicati...
chrome/browser/web_applications/web_app.cc:336: // fact.
On 2011/07/15 21:05:16, Evan Martin wrote:
> This TODO still exists, then.

Done.

http://codereview.chromium.org/7386002/diff/2001/chrome/installer/util/produc...
File chrome/installer/util/product.cc (right):

http://codereview.chromium.org/7386002/diff/2001/chrome/installer/util/produc...
chrome/installer/util/product.cc:86: // we don't care.
On 2011/07/15 21:05:16, Evan Martin wrote:
> This feels pretty dangerous to me; for example, the PLOG() below will do the
> wrong thing depending on whether we're reusing a member or not.  Sigh.

Yeah, this isn't a good practice. I changed this and base/process_util_win.cc to
follow the pattern used in sandbox/src/Wow64.cc; please consider.

[Evan Martin, 2011-07-18 21:15:57]

OK

[rvargas (doing something else), 2011-07-18 21:33:27]

drive by

http://codereview.chromium.org/7386002/diff/11001/net/tools/dump_cache/dump_c...
File net/tools/dump_cache/dump_cache.cc (right):

http://codereview.chromium.org/7386002/diff/11001/net/tools/dump_cache/dump_c...
net/tools/dump_cache/dump_cache.cc:74: int LaunchSlave(CommandLine command_line,
You should not modify the provided command_line (current process')

[rvargas (doing something else), 2011-07-18 21:36:33]

http://codereview.chromium.org/7386002/diff/11001/net/tools/dump_cache/dump_c...
File net/tools/dump_cache/dump_cache.cc (right):

http://codereview.chromium.org/7386002/diff/11001/net/tools/dump_cache/dump_c...
net/tools/dump_cache/dump_cache.cc:74: int LaunchSlave(CommandLine command_line,
On 2011/07/18 21:33:27, rvargas wrote:
> You should not modify the provided command_line (current process')

nevermind

[grt (UTC plus 2), 2011-07-19 03:19:34]

Drive-by comments about the installer_util change.

http://codereview.chromium.org/7386002/diff/11001/chrome/installer/util/produ...
File chrome/installer/util/product.cc (right):

http://codereview.chromium.org/7386002/diff/11001/chrome/installer/util/produ...
chrome/installer/util/product.cc:84: // arguments).  Since we're not using the
cmd variable beyond this point
The "Since we're not using..." comment isn't relevant anymore, is it?  If this
is the case, please remove it.

http://codereview.chromium.org/7386002/diff/11001/chrome/installer/util/produ...
chrome/installer/util/product.cc:86: scoped_ptr_malloc<wchar_t>
writable_command_line_string(
Why not:
std::wstring writable_command_line_string(cmd.GetCommandLineString());
followed by &writable_command_line_string[0] in the call to CreateProcess?  I
find this more readable and it should be more efficient since it makes use of
the string's knowledge of its length (no scan for the terminator).

http://codereview.chromium.org/7386002/diff/11001/chrome/installer/util/produ...
chrome/installer/util/product.cc:92: PLOG(ERROR) << "Failed to launch: " <<
writable_command_line_string.get();
How about either cmd.GetCommandLineString() (to log the whole command line) or
cmd.GetProgram().value() (to log only the program) here to avoid the ambiguity
of what CreateProcess may or may not do to the string?

[msw, 2011-07-19 07:14:26]

Thanks for the drive-by! PTAL.

http://codereview.chromium.org/7386002/diff/11001/chrome/installer/util/produ...
File chrome/installer/util/product.cc (right):

http://codereview.chromium.org/7386002/diff/11001/chrome/installer/util/produ...
chrome/installer/util/product.cc:84: // arguments).  Since we're not using the
cmd variable beyond this point
On 2011/07/19 03:19:34, grt wrote:
> The "Since we're not using..." comment isn't relevant anymore, is it?  If this
> is the case, please remove it.

Done.

http://codereview.chromium.org/7386002/diff/11001/chrome/installer/util/produ...
chrome/installer/util/product.cc:86: scoped_ptr_malloc<wchar_t>
writable_command_line_string(
On 2011/07/19 03:19:34, grt wrote:
> Why not:
> std::wstring writable_command_line_string(cmd.GetCommandLineString());
> followed by &writable_command_line_string[0] in the call to CreateProcess?  I
> find this more readable and it should be more efficient since it makes use of
> the string's knowledge of its length (no scan for the terminator).

Done.

http://codereview.chromium.org/7386002/diff/11001/chrome/installer/util/produ...
chrome/installer/util/product.cc:92: PLOG(ERROR) << "Failed to launch: " <<
writable_command_line_string.get();
On 2011/07/19 03:19:34, grt wrote:
> How about either cmd.GetCommandLineString() (to log the whole command line) or
> cmd.GetProgram().value() (to log only the program) here to avoid the ambiguity
> of what CreateProcess may or may not do to the string?

Done.

[grt (UTC plus 2), 2011-07-19 12:48:08]

installer/util/product.cc changes LGTM.  Thanks.

On 2011/07/19 07:14:26, msw wrote:
> Thanks for the drive-by! PTAL.
> 
>
http://codereview.chromium.org/7386002/diff/11001/chrome/installer/util/produ...
> File chrome/installer/util/product.cc (right):
> 
>
http://codereview.chromium.org/7386002/diff/11001/chrome/installer/util/produ...
> chrome/installer/util/product.cc:84: // arguments).  Since we're not using the
> cmd variable beyond this point
> On 2011/07/19 03:19:34, grt wrote:
> > The "Since we're not using..." comment isn't relevant anymore, is it?  If
this
> > is the case, please remove it.
> 
> Done.
> 
>
http://codereview.chromium.org/7386002/diff/11001/chrome/installer/util/produ...
> chrome/installer/util/product.cc:86: scoped_ptr_malloc<wchar_t>
> writable_command_line_string(
> On 2011/07/19 03:19:34, grt wrote:
> > Why not:
> > std::wstring writable_command_line_string(cmd.GetCommandLineString());
> > followed by &writable_command_line_string[0] in the call to CreateProcess? 
I
> > find this more readable and it should be more efficient since it makes use
of
> > the string's knowledge of its length (no scan for the terminator).
> 
> Done.
> 
>
http://codereview.chromium.org/7386002/diff/11001/chrome/installer/util/produ...
> chrome/installer/util/product.cc:92: PLOG(ERROR) << "Failed to launch: " <<
> writable_command_line_string.get();
> On 2011/07/19 03:19:34, grt wrote:
> > How about either cmd.GetCommandLineString() (to log the whole command line)
or
> > cmd.GetProgram().value() (to log only the program) here to avoid the
ambiguity
> > of what CreateProcess may or may not do to the string?
> 
> Done.

[Evan Martin, 2011-07-19 14:35:05]

http://codereview.chromium.org/7386002/diff/11001/chrome/installer/util/produ...
File chrome/installer/util/product.cc (right):

http://codereview.chromium.org/7386002/diff/11001/chrome/installer/util/produ...
chrome/installer/util/product.cc:86: scoped_ptr_malloc<wchar_t>
writable_command_line_string(
Oh yeah, should have remembered this earlier:
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/base/string_util.h&ex...