Disable first run for the New Tab Cold perf test....

Disable first run for the New Tab Cold perf test.

BUG=17668
TEST=Run the test


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

[arv (Not doing code reviews), 2009-07-24 18:21:55]

I had to refactor the code since the NewTabHTMLSource was in the anonymous
namespace.

[darin (slow to review), 2009-07-24 19:29:06]

LGTM

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

http://codereview.chromium.org/160108/diff/9/1012#newcode59
Line 59: NewTabHTMLSource::set_first_run(false);
please add a comment here with a TODO(arv) referencing the bug.  that way others
will understand why this is here.  it should be noted that this is a temporary
hack to help keep this test meaningful (i.e., able to catch other regressions)
while we deal with the present regression.

[arv (Not doing code reviews), 2009-07-24 19:34:30]

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

http://codereview.chromium.org/160108/diff/9/1012#newcode59
Line 59: NewTabHTMLSource::set_first_run(false);
On 2009/07/24 19:29:06, darin wrote:
> please add a comment here with a TODO(arv) referencing the bug.  that way
others
> will understand why this is here.  it should be noted that this is a temporary
> hack to help keep this test meaningful (i.e., able to catch other regressions)
> while we deal with the present regression.

Added a comment

This is however not a temporary hack. I explain why this is needed in the
comment:

    // We need to ensure that we are not going into first run mode since first
    // run has a couple of animations which would hide any real performance
    // regressions.

[darin (slow to review), 2009-07-24 19:43:59]

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

http://codereview.chromium.org/160108/diff/9/1012#newcode59
Line 59: NewTabHTMLSource::set_first_run(false);
So the cold start test is not a test of first run?  I'm confused...

[arv (Not doing code reviews), 2009-07-24 20:28:19]

The problem is how the timing is done. The timer measures how long it
took until there are no more redraws. Since there is an animation on
startup when the first run is enabled the page will keep redrawing
until the animation is done.

On Fri, Jul 24, 2009 at 12:43, <darin@chromium.org> wrote:
>
> http://codereview.chromium.org/160108/diff/9/1012
> File chrome/test/startup/feature_startup_test.cc (right):
>
> http://codereview.chromium.org/160108/diff/9/1012#newcode59
> Line 59: NewTabHTMLSource::set_first_run(false);
> So the cold start test is not a test of first run? =C2=A0I'm confused...
>
> http://codereview.chromium.org/160108
>



--=20
erik

[darin (slow to review), 2009-07-24 21:05:11]

I see....  anyways, my LGTM still stands :)-Darin


On Fri, Jul 24, 2009 at 1:27 PM, Erik Arvidsson <arv@chromium.org> wrote:

> The problem is how the timing is done. The timer measures how long it
> took until there are no more redraws. Since there is an animation on
> startup when the first run is enabled the page will keep redrawing
> until the animation is done.
>
> On Fri, Jul 24, 2009 at 12:43, <darin@chromium.org> wrote:
> >
> > http://codereview.chromium.org/160108/diff/9/1012
> > File chrome/test/startup/feature_startup_test.cc (right):
> >
> > http://codereview.chromium.org/160108/diff/9/1012#newcode59
> > Line 59: NewTabHTMLSource::set_first_run(false);
> > So the cold start test is not a test of first run?  I'm confused...
> >
> > http://codereview.chromium.org/160108
> >
>
>
>
> --
> erik
>

[arv (Not doing code reviews), 2009-07-25 21:31:30]

This doesn't work. I am getting link errors because I assume that the test code
does not pull in all the browser code but it communicates with the browser
through the automation. I will need to extend the automation tool to expose this
instead.

[darin (slow to review), 2009-07-26 00:30:11]

Perhaps this could be done via a command line switch?
 --disable-first-run-ui or some such?  It might be useful in other contexts.

-Darin



On Sat, Jul 25, 2009 at 2:31 PM, <arv@chromium.org> wrote:

> This doesn't work. I am getting link errors because I assume that the
> test code does not pull in all the browser code but it communicates with
> the browser through the automation. I will need to extend the automation
> tool to expose this instead.
>
>
> http://codereview.chromium.org/160108
>

[arv (Not doing code reviews), 2009-07-28 01:10:08]

Hi Darin,

After some adventures in FirstRun land I ended up going with a command line flag
after all.

Please take another (quick) look.

[darin (slow to review), 2009-07-28 03:13:38]

http://codereview.chromium.org/160108/diff/56/1062
File chrome/browser/dom_ui/new_tab_ui.cc (right):

http://codereview.chromium.org/160108/diff/56/1062#newcode212
Line 212: static bool first_run();
if a method is named using "unix_hacker" notation, then it should be implemented
inline.  given that this function is no longer as cheap as a normal member
variable access, i think it is probably appropriate to rename it to CamelCase. 
however, i think it would be even better to keep first_run() as is, and instead
call set_first_run(!NewTabUI::FirstRunDisabled()) at some preset time during
startup.

http://codereview.chromium.org/160108/diff/56/1063
File chrome/browser/dom_ui/new_tab_ui.h (right):

http://codereview.chromium.org/160108/diff/56/1063#newcode35
Line 35: static bool FirstRunDisabled();
style nit: add a new line above the "private:"

http://codereview.chromium.org/160108/diff/56/1065
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/160108/diff/56/1065#newcode495
Line 495: const wchar_t kDisableNewTabFirstRun[]        =
L"disable-new-tab-first-run";
the indentation of the "=" in this file is all confused.  once upon a time, all
of the "="'s lined up in a column, but now the file has become an inconsistent
mess.  sigh... not saying you should do anything about it in this CL :-/

http://codereview.chromium.org/160108/diff/56/1061
File chrome/test/startup/feature_startup_test.cc (right):

http://codereview.chromium.org/160108/diff/56/1061#newcode59
Line 59: launch_arguments_.AppendSwitch(switches::kDisableNewTabFirstRun);
I still think it would be good to put a comment here explaining why we aren't
interested in exercising first run perf.

[arv (Not doing code reviews), 2009-07-28 16:57:03]

http://codereview.chromium.org/160108/diff/56/1062
File chrome/browser/dom_ui/new_tab_ui.cc (right):

http://codereview.chromium.org/160108/diff/56/1062#newcode212
Line 212: static bool first_run();
On 2009/07/28 03:13:38, darin wrote:
> if a method is named using "unix_hacker" notation, then it should be
implemented
> inline.  given that this function is no longer as cheap as a normal member
> variable access, i think it is probably appropriate to rename it to CamelCase.

> however, i think it would be even better to keep first_run() as is, and
instead
> call set_first_run(!NewTabUI::FirstRunDisabled()) at some preset time during
> startup.

Done.

And now I know the rule. It has been confusing me for a while.

http://codereview.chromium.org/160108/diff/56/1063
File chrome/browser/dom_ui/new_tab_ui.h (right):

http://codereview.chromium.org/160108/diff/56/1063#newcode35
Line 35: static bool FirstRunDisabled();
On 2009/07/28 03:13:38, darin wrote:
> style nit: add a new line above the "private:"

Done.

http://codereview.chromium.org/160108/diff/56/1065
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/160108/diff/56/1065#newcode495
Line 495: const wchar_t kDisableNewTabFirstRun[]        =
L"disable-new-tab-first-run";
On 2009/07/28 03:13:38, darin wrote:
> the indentation of the "=" in this file is all confused.  once upon a time,
all
> of the "="'s lined up in a column, but now the file has become an inconsistent
> mess.  sigh... not saying you should do anything about it in this CL :-/

I cleaned up a few neighboring ones.

http://codereview.chromium.org/160108/diff/56/1061
File chrome/test/startup/feature_startup_test.cc (right):

http://codereview.chromium.org/160108/diff/56/1061#newcode59
Line 59: launch_arguments_.AppendSwitch(switches::kDisableNewTabFirstRun);
On 2009/07/28 03:13:38, darin wrote:
> I still think it would be good to put a comment here explaining why we aren't
> interested in exercising first run perf.

Done.

[darin (slow to review), 2009-07-28 17:16:04]

LGTM!