Test startup of new tab page with a complex theme....

Test startup of new tab page with a complex theme.

BUG= http://crbug.com/18767
TEST= none


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

[Miranda Callahan, 2009-08-07 22:55:37]

Nicolas, we had discussed making sure this gets picked up by the bots and
displayed as a separate line in the New Tab Cold graph; is this sufficient to
make this happen, or do I need to do something else?  Thanks!

[Nicolas Sylvain, 2009-08-07 23:12:14]

http://codereview.chromium.org/165168/diff/9/1010
File chrome/test/startup/feature_startup_test.cc (right):

http://codereview.chromium.org/165168/diff/9/1010#newcode60
Line 60: int profile_type) {
align with const above?

http://codereview.chromium.org/165168/diff/9/1010#newcode124
Line 124: TEST_F(NewTabUIStartupTest, ComplexTheme) {
I had in mind that you would be creating a StartupTest, not a New Tab Ui tests.
If you really want a New Tab Ui tests (which  makes sense, even though we should
be testing the StartupTest too) then should can't use warm. I thin it used to be
way too flaky. It's why the one just above is actually disabled.

You should also call it "tab_theme_warn" or something that matches more the
other name.

http://codereview.chromium.org/165168/diff/9/1010#newcode127
Line 127: UITest::COMPLEX_THEME);
You should also check startup_test.cc

[Miranda Callahan, 2009-08-09 20:34:34]

All right -- made the test cold, and added a test to startup_test.cc that tests
startup without a new tab page.  I also moved ComputeTypicalUserDataSource into
ui_test.cc, so both startup test classes could use it.

In my tests so far, I'm seeing a difference of anywhere between 200 and 700 ms
between NewTabUIStartupTest.PerfCold and NewTabUIStartupTest.ComplexTheme.

Note: I think that the profile I'm using adds nothing additional to the startup
besides the complex theme, but I would appreciate someone more expert briefly
looking through the profile I used (http://codereview.chromium.org/164193) to
make sure there's nothing in there that would add time to the run.

http://codereview.chromium.org/165168/diff/9/1010
File chrome/test/startup/feature_startup_test.cc (right):

http://codereview.chromium.org/165168/diff/9/1010#newcode60
Line 60: int profile_type) {
On 2009/08/07 23:12:14, Nicolas Sylvain wrote:
> align with const above?

Done.

http://codereview.chromium.org/165168/diff/9/1010#newcode124
Line 124: TEST_F(NewTabUIStartupTest, ComplexTheme) {
On 2009/08/07 23:12:14, Nicolas Sylvain wrote:
> I had in mind that you would be creating a StartupTest, not a New Tab Ui
tests.
> If you really want a New Tab Ui tests (which  makes sense, even though we
should
> be testing the StartupTest too) then should can't use warm. I thin it used to
be
> way too flaky. It's why the one just above is actually disabled.
> 
> You should also call it "tab_theme_warn" or something that matches more the
> other name.
> 
> 

Ok, that makes sense -- changed it to cold, changed the name.

http://codereview.chromium.org/165168/diff/9/1010#newcode127
Line 127: UITest::COMPLEX_THEME);
On 2009/08/07 23:12:14, Nicolas Sylvain wrote:
> You should also check startup_test.cc

Done.

[Nicolas Sylvain, 2009-08-10 16:42:48]

http://codereview.chromium.org/165168/diff/1024/26
File chrome/test/startup/feature_startup_test.cc (right):

http://codereview.chromium.org/165168/diff/1024/26#newcode31
Line 31: else
why not "else if COMPLEX_THEME ... else NOTREACHED()" ?

And I personally prefer to put { } for the if blocks if it's more than 1 line.
This is a personal preference, I don't think it's in the guide. Feel free to
ignore.

http://codereview.chromium.org/165168/diff/1024/28
File chrome/test/startup/startup_test.cc (right):

http://codereview.chromium.org/165168/diff/1024/28#newcode164
Line 164: TEST_F(StartupFileTest, PerfColdComplexTheme) {
For this one I'm not sure what "File" means here. It seems to be something for
gears. You should use StartupTest instead of StartupFileTest. Cold here is also
pretty noisy and we usually ignore it. In this case Warm is the one that we care
about. 
You also need to change "t" for "t-theme", otherwise it will overlap with the
other line.

http://codereview.chromium.org/165168/diff/1024/29
File chrome/test/ui/ui_test.cc (right):

http://codereview.chromium.org/165168/diff/1024/29#newcode972
Line 972: if (profile_type == UITest::DEFAULT_THEME)
This code seems to be there twice. Is there a way we can use the same function
at both places?

[Miranda Callahan, 2009-08-10 17:12:43]

http://codereview.chromium.org/165168/diff/1024/26
File chrome/test/startup/feature_startup_test.cc (right):

http://codereview.chromium.org/165168/diff/1024/26#newcode31
Line 31: else
On 2009/08/10 16:42:48, Nicolas Sylvain wrote:
> why not "else if COMPLEX_THEME ... else NOTREACHED()" ?
> 
> And I personally prefer to put { } for the if blocks if it's more than 1 line.
> This is a personal preference, I don't think it's in the guide. Feel free to
> ignore.

Yes, the brackets look better, especially with three cases -- done.

http://codereview.chromium.org/165168/diff/1024/28
File chrome/test/startup/startup_test.cc (right):

http://codereview.chromium.org/165168/diff/1024/28#newcode164
Line 164: TEST_F(StartupFileTest, PerfColdComplexTheme) {
On 2009/08/10 16:42:48, Nicolas Sylvain wrote:
> For this one I'm not sure what "File" means here. It seems to be something for
> gears. You should use StartupTest instead of StartupFileTest. Cold here is
also
> pretty noisy and we usually ignore it. In this case Warm is the one that we
care
> about. 
> You also need to change "t" for "t-theme", otherwise it will overlap with the
> other line.

Done.

http://codereview.chromium.org/165168/diff/1024/29
File chrome/test/ui/ui_test.cc (right):

http://codereview.chromium.org/165168/diff/1024/29#newcode972
Line 972: if (profile_type == UITest::DEFAULT_THEME)
On 2009/08/10 16:42:48, Nicolas Sylvain wrote:
> This code seems to be there twice. Is there a way we can use the same function
> at both places?

D'oh -- my bad, I moved this function to ui_test.cc so I could call it from both
startup tests.  I have removed it from feature_startup_test.cc.  Thanks for
catching this.

[Nicolas Sylvain, 2009-08-10 17:41:36]

LGTM!