Support Chromium-branded builds and cleanup reference build usage.

Support Chromium-branded builds and cleanup reference build usage.

Implemented cmp's POC (http://codereview.chromium.org/7171005/) and enforced
the usage of UIPerfTest::UseReferenceBuild().


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

[jrt, 2011-06-16 22:46:37]

http://codereview.chromium.org/7185023/diff/1/chrome/test/perf/frame_rate/fra...
File chrome/test/perf/frame_rate/frame_rate_tests.cc (right):

http://codereview.chromium.org/7185023/diff/1/chrome/test/perf/frame_rate/fra...
chrome/test/perf/frame_rate/frame_rate_tests.cc:84: UITest::SetUp();
Is there a standard for calling inherited methods, i.e. should I use
FrameRateTest::{UseReferenceBuild,SetUp}()?

[jrt, 2011-06-16 22:53:47]

These tests also use a reference build, but access it in completely different
ways.

src/chrome/test/memory_test/memory_test.cc
Note: Reference build code exists, but not used.
src/chrome/test/url_fetch_test/url_fetch_test.cc
Note: Inherits UITest, not UIPerfTest...

[cmp, 2011-06-16 23:29:40]

Nice, some updates needed.  There may be a use of kBrowserProcessExecutableName
that we need to watch out for, too.

http://codereview.chromium.org/7185023/diff/1/chrome/test/page_cycler/page_cy...
File chrome/test/page_cycler/page_cycler_test.cc (right):

http://codereview.chromium.org/7185023/diff/1/chrome/test/page_cycler/page_cy...
chrome/test/page_cycler/page_cycler_test.cc:262:
UIPerfTest::UseReferenceBuild();
just UseReferenceBuild();

http://codereview.chromium.org/7185023/diff/1/chrome/test/perf/frame_rate/fra...
File chrome/test/perf/frame_rate/frame_rate_tests.cc (right):

http://codereview.chromium.org/7185023/diff/1/chrome/test/perf/frame_rate/fra...
chrome/test/perf/frame_rate/frame_rate_tests.cc:84: UITest::SetUp();
On 2011/06/16 22:46:37, Joe Tessler wrote:
> Is there a standard for calling inherited methods, i.e. should I use
> FrameRateTest::{UseReferenceBuild,SetUp}()?

I'd go with UseReferenceBuild and FrameRateTest::SetUp.

http://codereview.chromium.org/7185023/diff/1/chrome/test/ui/dromaeo_benchmar...
File chrome/test/ui/dromaeo_benchmark_uitest.cc (right):

http://codereview.chromium.org/7185023/diff/1/chrome/test/ui/dromaeo_benchmar...
chrome/test/ui/dromaeo_benchmark_uitest.cc:130: UIPerfTest::UseReferenceBuild();
just UseReferenceBuild();

http://codereview.chromium.org/7185023/diff/1/chrome/test/ui/sunspider_uitest.cc
File chrome/test/ui/sunspider_uitest.cc (right):

http://codereview.chromium.org/7185023/diff/1/chrome/test/ui/sunspider_uitest...
chrome/test/ui/sunspider_uitest.cc:125: UIPerfTest::UseReferenceBuild();
just UseReferenceBuild();

http://codereview.chromium.org/7185023/diff/1/chrome/test/ui/ui_test.cc
File chrome/test/ui/ui_test.cc (right):

http://codereview.chromium.org/7185023/diff/1/chrome/test/ui/ui_test.cc#newco...
chrome/test/ui/ui_test.cc:185: chrome::kBrowserProcessExecutablePathChromium);
I believe there's also a use of kBrowserProcessExecutableName that needs to be
replaced in the enable_chrome_branding_ false case with a use of
kBrowserProcessExecutableNameChromium.  Building and running startup_tests with
official branding on Mac should show you where that is, if it's really there at
all.

http://codereview.chromium.org/7185023/diff/1/chrome/test/ui/v8_benchmark_uit...
File chrome/test/ui/v8_benchmark_uitest.cc (right):

http://codereview.chromium.org/7185023/diff/1/chrome/test/ui/v8_benchmark_uit...
chrome/test/ui/v8_benchmark_uitest.cc:127: UIPerfTest::UseReferenceBuild();
just UseReferenceBuild();

[cmp, 2011-06-16 23:36:56]

On 2011/06/16 22:53:47, Joe Tessler wrote:
> These tests also use a reference build, but access it in completely different
> ways.
> 
> src/chrome/test/memory_test/memory_test.cc
> Note: Reference build code exists, but not used.
> src/chrome/test/url_fetch_test/url_fetch_test.cc
> Note: Inherits UITest, not UIPerfTest...

Can you update both to use UIPerfTest and UseReferenceBuild()?

[darin (slow to review), 2011-06-17 16:20:11]

Looks good to me too.  I agree with Chase's feedback.

[jrt, 2011-06-17 19:50:46]

http://codereview.chromium.org/7185023/diff/6001/chrome/test/chrome_process_u...
File chrome/test/chrome_process_util.h (right):

http://codereview.chromium.org/7185023/diff/6001/chrome/test/chrome_process_u...
chrome/test/chrome_process_util.h:19: ChromeProcessList
GetRunningChromiumProcesses(base::ProcessId browser_pid);
Will every call to GetRunningChromeProcess break on a Chromium-branded build? If
so, my idea is to add the GetRunningChromiumProcess function.

However, there are many places this method is called without access to
enable_chrome_branding_...

http://codereview.chromium.org/7185023/diff/6001/chrome/test/memory_test/memo...
File chrome/test/memory_test/memory_test.cc (right):

http://codereview.chromium.org/7185023/diff/6001/chrome/test/memory_test/memo...
chrome/test/memory_test/memory_test.cc:505:
GENERAL_MIX_MEMORY_TESTS(TwelveTabTest, 12);
This test did not previously use a reference build, although the code existed.
Should I add a reference build test like so?

http://codereview.chromium.org/7185023/diff/6001/chrome/test/url_fetch_test/u...
File chrome/test/url_fetch_test/url_fetch_test.cc (right):

http://codereview.chromium.org/7185023/diff/6001/chrome/test/url_fetch_test/u...
chrome/test/url_fetch_test/url_fetch_test.cc:93: // passed on the command line,
eg.
Why does this test use command line arguments?

[jrt, 2011-06-20 19:14:01]

I decided to use a command-line switch instead of a member variable for the
branding flag. Note: there are two more uses of
chrome::kBrowserProcessExecutablePath in the testing sub-directory:
src/chrome/test/startup/startup_test.cc
src/chrome/test/in_process_browser_test.cc

[jrt, 2011-06-22 18:43:46]

On 2011/06/20 19:14:01, jrt wrote:
> I decided to use a command-line switch instead of a member variable for the
> branding flag. Note: there are two more uses of
> chrome::kBrowserProcessExecutablePath in the testing sub-directory:
> src/chrome/test/startup/startup_test.cc
> src/chrome/test/in_process_browser_test.cc
> src/chrome/test/webdriver/automation.cc

These cases do not use a reference build, therefore no change is required. I
just tested the changes on a Chrome-branded Mac build and everything worked as
expected. Ready for final review.

[darin (slow to review), 2011-06-22 22:11:28]

LGTM

[cmp, 2011-06-22 22:41:58]

lgtm with nits

http://codereview.chromium.org/7185023/diff/11038/chrome/test/chrome_process_...
File chrome/test/chrome_process_util.cc (right):

http://codereview.chromium.org/7185023/diff/11038/chrome/test/chrome_process_...
chrome/test/chrome_process_util.cc:1: // Copyright (c) 2010 The Chromium
Authors. All rights reserved.
2011

http://codereview.chromium.org/7185023/diff/11038/chrome/test/chrome_process_...
File chrome/test/chrome_process_util.h (right):

http://codereview.chromium.org/7185023/diff/11038/chrome/test/chrome_process_...
chrome/test/chrome_process_util.h:16: // Returns the executable name of the
current chrome browser process.
chrome -> Chrome

http://codereview.chromium.org/7185023/diff/11038/chrome/test/test_switches.cc
File chrome/test/test_switches.cc (right):

http://codereview.chromium.org/7185023/diff/11038/chrome/test/test_switches.c...
chrome/test/test_switches.cc:1: // Copyright (c) 2010 The Chromium Authors. All
rights reserved.
2011

http://codereview.chromium.org/7185023/diff/11038/chrome/test/test_switches.h
File chrome/test/test_switches.h (right):

http://codereview.chromium.org/7185023/diff/11038/chrome/test/test_switches.h...
chrome/test/test_switches.h:1: // Copyright (c) 2010 The Chromium Authors. All
rights reserved.
2011

http://codereview.chromium.org/7185023/diff/11038/chrome/test/ui/ui_test.h
File chrome/test/ui/ui_test.h (right):

http://codereview.chromium.org/7185023/diff/11038/chrome/test/ui/ui_test.h#ne...
chrome/test/ui/ui_test.h:169: // Gets the executable file path of the chrome
browser process.
chrome -> Chrome

[commit-bot: I haz the power, 2011-06-23 00:34:57]

Change committed as 90141