Skip to content

Commit

Permalink
Revert of Enforce lowercase switches when calling CommandLine::HasSwi…
Browse files Browse the repository at this point in the history
…tch(const char*). (patchset #5 id:80001 of https://codereview.chromium.org/1046363002/)

Reason for revert:
This causes test fails on Android:
https://build.chromium.org/p/chromium.linux/builders/Android%20Tests%20%28dbg%29/builds/27294

The test uses upper case with HasSwitch():
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/android/javatests/src/org/chromium/content/browser/ContentCommandLineTest.java&q=testJavaNativeTransition&sq=package:chromium&type=cs&l=96

Original issue's description:
> Enforce lowercase switches when calling CommandLine::HasSwitch.
>
> At the moment, all compile-time switches are lowercase. By enforcing
> this, we can skip converting it to lowercase on Windows, which saves
> one string allocation per call.
>
> On a profile with 2 extensions, HasSwitch is called ~12k times during
> startup. In an ideal situation (no paging/cache pressure), the
> string allocation under Windows takes ~137ns on an Xeon E5-2690 @
> 2.9Ghz. So this should shave off at least 1.6ms off a typical startup
> with this hardware. For context,
> Startup.BrowserMessageLoopStartTimeFromMainEntry is typically
> 280-300ms on the same hardware, so we should get a ~0.5% improvement.
>
> BUG=472383
>
> Committed: https://crrev.com/f58961749a980032241fe6c3fc829ac2e6652030
> Cr-Commit-Position: refs/heads/master@{#325576}

TBR=tapted@chromium.org,brettw@chromium.org,jackhou@chromium.org
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=472383

Review URL: https://codereview.chromium.org/1091993002

Cr-Commit-Position: refs/heads/master@{#325610}
  • Loading branch information
yoichio authored and Commit bot committed Apr 17, 2015
1 parent ff73923 commit 817cf38
Show file tree
Hide file tree
Showing 3 changed files with 7 additions and 8 deletions.
3 changes: 1 addition & 2 deletions base/command_line.cc
Original file line number Diff line number Diff line change
Expand Up @@ -263,8 +263,7 @@ void CommandLine::SetProgram(const FilePath& program) {
}

bool CommandLine::HasSwitch(const std::string& switch_string) const {
DCHECK_EQ(StringToLowerASCII(switch_string), switch_string);
return switches_.find(switch_string) != switches_.end();
return switches_.find(LowerASCIIOnWindows(switch_string)) != switches_.end();
}

bool CommandLine::HasSwitch(const char string_constant[]) const {
Expand Down
2 changes: 1 addition & 1 deletion base/command_line.h
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ class BASE_EXPORT CommandLine {
void SetProgram(const FilePath& program);

// Returns true if this command line contains the given switch.
// Switch names should only be lowercase.
// (Switch names are case-insensitive).
// The second override provides an optimized version to avoid inlining the
// codegen for the string allocation.
bool HasSwitch(const std::string& switch_string) const;
Expand Down
10 changes: 5 additions & 5 deletions base/command_line_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -60,13 +60,12 @@ TEST(CommandLineTest, CommandLineConstructor) {
cl.GetProgram().value());

EXPECT_TRUE(cl.HasSwitch("foo"));
#if defined(OS_WIN)
EXPECT_TRUE(cl.HasSwitch("bar"));
#else
EXPECT_FALSE(cl.HasSwitch("bar"));
#endif
EXPECT_TRUE(cl.HasSwitch("bAr"));
EXPECT_TRUE(cl.HasSwitch("baz"));
EXPECT_TRUE(cl.HasSwitch("spaetzle"));
#if defined(OS_WIN)
EXPECT_TRUE(cl.HasSwitch("SPAETZLE"));
#endif
EXPECT_TRUE(cl.HasSwitch("other-switches"));
EXPECT_TRUE(cl.HasSwitch("input-translation"));

Expand Down Expand Up @@ -129,6 +128,7 @@ TEST(CommandLineTest, CommandLineFromString) {
EXPECT_TRUE(cl.HasSwitch("bar"));
EXPECT_TRUE(cl.HasSwitch("baz"));
EXPECT_TRUE(cl.HasSwitch("spaetzle"));
EXPECT_TRUE(cl.HasSwitch("SPAETZLE"));
EXPECT_TRUE(cl.HasSwitch("other-switches"));
EXPECT_TRUE(cl.HasSwitch("input-translation"));
EXPECT_TRUE(cl.HasSwitch("quotes"));
Expand Down

0 comments on commit 817cf38

Please sign in to comment.