Refactoring the debug creation of BeginFrameArgs objects to be in unittest code only.

Refactoring the debug creation of BeginFrameArgs objects to be in unittest code only.

Adding an output_test_common.xx file which will also gain more testing features
for BeginFrameArgs soon.

BUG=371223
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=269474

[mithro-old, 2014-05-08 01:59:55]

Hi Brian,

This patch is the first in splitting the BeginFrameArgs debug changes from the
scheduler frame source refactor.

Will very shortly send out another patch on top of this one which adds in the
AsValue() support + TRACE_EVENT support.

Tim

[brianderson, 2014-05-08 02:09:31]

Thanks for splitting this out and thanks for improving the testing
infrastructure! Overall looks good.

Please open a bug to reference for your testing-related changes.

https://codereview.chromium.org/275543004/diff/20001/cc/test/output_test_comm...
File cc/test/output_test_common.cc (right):

https://codereview.chromium.org/275543004/diff/20001/cc/test/output_test_comm...
cc/test/output_test_common.cc:28: BeginFrameArgs BeginFrameArgsCreateQuick(int64
frame_time,
What value does this add over BeginFrameArgs::Create?

[mithro-old, 2014-05-08 02:18:23]

On 2014/05/08 02:09:31, brianderson wrote:
> Thanks for splitting this out and thanks for improving the testing
> infrastructure! Overall looks good.
> 
> Please open a bug to reference for your testing-related changes.

Done -- https://code.google.com/p/chromium/issues/detail?id=371223

> 
>
https://codereview.chromium.org/275543004/diff/20001/cc/test/output_test_comm...
> File cc/test/output_test_common.cc (right):
> 
>
https://codereview.chromium.org/275543004/diff/20001/cc/test/output_test_comm...
> cc/test/output_test_common.cc:28: BeginFrameArgs
BeginFrameArgsCreateQuick(int64
> frame_time,
> What value does this add over BeginFrameArgs::Create?

BeginFrameArgsCreateQuick(1, 2, 3); 
  verses
BeginFrameArgs::Create(base::TimeTicks::FromInternalValue(1),
base::TimeTicks::FromInternalValue(2), base::TimeDelta::FromInternalValue(3));

[brianderson, 2014-05-08 02:35:27]

Btw, why did you chose output_test_common rather than
begin_frame_args_test_common?

Is the intent of the "Quick" version to only be used in tests? Should we add
ForTesting to the name to have presubmit make sure it isn't used outside of
tests? Although, I guess the link would fail if a non-test file tried to use it.

https://codereview.chromium.org/275543004/diff/20001/cc/test/output_test_comm...
File cc/test/output_test_common.h (right):

https://codereview.chromium.org/275543004/diff/20001/cc/test/output_test_comm...
cc/test/output_test_common.h:16: BeginFrameArgs
BeginFrameArgsCreateForTesting();
Now that these aren't methods within a class:
CreateBeginFrameArgsForTesting()?
CreateExpiredBeginFrameArgsForTesting()?
CreateBeginFrameArgsQuick()?

https://codereview.chromium.org/275543004/diff/20001/cc/test/output_test_comm...
cc/test/output_test_common.h:18: BeginFrameArgs BeginFrameArgsCreateQuick(int64
frame_time,
Instead of "Quick", maybe "Raw" or "FromInternal"?

[mithro-old, 2014-05-08 03:26:46]

On 2014/05/08 02:35:27, brianderson wrote:
> Btw, why did you chose output_test_common rather than
> begin_frame_args_test_common?

The convention I saw as XXXX_test_common where XXX was a directory under cc/
--------------------------------------------------------------------
-rw-r----- 1 tansell eng 11653 Apr 29 09:20 cc/test/animation_test_common.cc
-rw-r----- 1 tansell eng  6232 Apr 29 09:20 cc/test/animation_test_common.h
-rw-r----- 1 tansell eng  5685 May  2 10:49 cc/test/layer_test_common.cc
-rw-r----- 1 tansell eng  4494 Apr 29 09:20 cc/test/layer_test_common.h
-rw-r----- 1 tansell eng  1326 May  8 13:15 cc/test/output_test_common.cc
-rw-r----- 1 tansell eng   737 May  8 13:15 cc/test/output_test_common.h
-rw-r----- 1 tansell eng  9791 May  6 08:19 cc/test/render_pass_test_common.cc
-rw-r----- 1 tansell eng   859 Oct  1  2013 cc/test/render_pass_test_common.h
-rw-r----- 1 tansell eng   413 May  8 11:32 cc/test/scheduler_test_common.cc
-rw-r----- 1 tansell eng  1546 May  8 11:32 cc/test/scheduler_test_common.h
-rw-r----- 1 tansell eng  1236 May  7 04:52 cc/test/skia_common.cc
-rw-r----- 1 tansell eng   809 Mar 13 10:59 cc/test/skia_common.h
-rw-r----- 1 tansell eng  5835 Apr 29 09:20 cc/test/tiled_layer_test_common.cc
-rw-r----- 1 tansell eng  5292 Apr 29 09:20 cc/test/tiled_layer_test_common.h
--------------------------------------------------------------------

> 
> Is the intent of the "Quick" version to only be used in tests? Should we add
> ForTesting to the name to have presubmit make sure it isn't used outside of
> tests? Although, I guess the link would fail if a non-test file tried to use
it.

These files are *not* included in cc.gyp, only cc_tests.gyp so if used outside
cc_unittest compile/linking will fail.

>
https://codereview.chromium.org/275543004/diff/20001/cc/test/output_test_comm...
> File cc/test/output_test_common.h (right):
> 
>
https://codereview.chromium.org/275543004/diff/20001/cc/test/output_test_comm...
> cc/test/output_test_common.h:16: BeginFrameArgs
> BeginFrameArgsCreateForTesting();
> Now that these aren't methods within a class:
> CreateBeginFrameArgsForTesting()?
> CreateExpiredBeginFrameArgsForTesting()?
> CreateBeginFrameArgsQuick()?

I was pondering if the Testing part should be the first part of the name; IE
Something like TestCreateBeginFrameArgs() - but that doesn't read right?

Maybe;
  CreateSimpleBeginFrameArgsForTesting()
  CreateExpiredBeginFrameArgsForTesting()
  CreateBeginFrameArgsForTesting()

> 
>
https://codereview.chromium.org/275543004/diff/20001/cc/test/output_test_comm...
> cc/test/output_test_common.h:18: BeginFrameArgs
BeginFrameArgsCreateQuick(int64
> frame_time,
> Instead of "Quick", maybe "Raw" or "FromInternal"?

If we rename the current CreateBeginFrameArgsForTesting method, I think it would
make sense to just use CreateBeginFrameArgsForTesting name instead?

[brianderson, 2014-05-08 21:28:46]

The names you picked sgtm.

[danakj, 2014-05-08 21:37:48]

https://codereview.chromium.org/275543004/diff/40001/cc/test/output_test_comm...
File cc/test/output_test_common.h (right):

https://codereview.chromium.org/275543004/diff/40001/cc/test/output_test_comm...
cc/test/output_test_common.h:5: #ifndef CC_TEST_OUTPUT_TEST_COMMON_H_
I'd prefer begin_frame_args_common.h for this, it's coincidental that we ended
up breaking cc into directories in similar way that test filers were broken up.
Smaller more focused files would be good, we don't need to group everything that
tests things in output/ together.

[mithro-old, 2014-05-08 21:41:15]

https://codereview.chromium.org/275543004/diff/40001/cc/test/output_test_comm...
File cc/test/output_test_common.h (right):

https://codereview.chromium.org/275543004/diff/40001/cc/test/output_test_comm...
cc/test/output_test_common.h:5: #ifndef CC_TEST_OUTPUT_TEST_COMMON_H_
On 2014/05/08 21:37:48, danakj wrote:
> I'd prefer begin_frame_args_common.h for this, it's coincidental that we ended
> up breaking cc into directories in similar way that test filers were broken
up.
> Smaller more focused files would be good, we don't need to group everything
that
> tests things in output/ together.

Is there any reason not to use cc/debug/begin_frames_args.h or is
cc/debug/begin_frames_args_common.h better?

[danakj, 2014-05-08 21:46:06]

https://codereview.chromium.org/275543004/diff/40001/cc/test/output_test_comm...
File cc/test/output_test_common.h (right):

https://codereview.chromium.org/275543004/diff/40001/cc/test/output_test_comm...
cc/test/output_test_common.h:5: #ifndef CC_TEST_OUTPUT_TEST_COMMON_H_
On 2014/05/08 21:41:16, mithro wrote:
> On 2014/05/08 21:37:48, danakj wrote:
> > I'd prefer begin_frame_args_common.h for this, it's coincidental that we
ended
> > up breaking cc into directories in similar way that test filers were broken
> up.
> > Smaller more focused files would be good, we don't need to group everything
> that
> > tests things in output/ together.
> 
> Is there any reason not to use cc/debug/begin_frames_args.h or is
> cc/debug/begin_frames_args_common.h better?
> 

I assume you mean cc/test/ not cc/debug/ ?

I'd use begin_frame_args.h if this file defined a class called BeginFrameArgs.
Since it doesn't we should clarify that. _common is a "common" way to do that :)

[danakj, 2014-05-08 21:47:30]

On Thu, May 8, 2014 at 5:46 PM, <danakj@chromium.org> wrote:

>
> https://codereview.chromium.org/275543004/diff/40001/cc/
> test/output_test_common.h
> File cc/test/output_test_common.h (right):
>
> https://codereview.chromium.org/275543004/diff/40001/cc/
> test/output_test_common.h#newcode5
> cc/test/output_test_common.h:5: #ifndef CC_TEST_OUTPUT_TEST_COMMON_H_
> On 2014/05/08 21:41:16, mithro wrote:
>
>> On 2014/05/08 21:37:48, danakj wrote:
>> > I'd prefer begin_frame_args_common.h for this, it's coincidental
>>
> that we ended
>
>> > up breaking cc into directories in similar way that test filers were
>>
> broken
>
>> up.
>> > Smaller more focused files would be good, we don't need to group
>>
> everything
>
>> that
>> > tests things in output/ together.
>>
>
>  Is there any reason not to use cc/debug/begin_frames_args.h or is
>> cc/debug/begin_frames_args_common.h better?
>>
>
>
> I assume you mean cc/test/ not cc/debug/ ?
>
> I'd use begin_frame_args.h if this file defined a class called
> BeginFrameArgs. Since it doesn't we should clarify that. _common is a
> "common" way to do that :)
>

Or _test_common, I guess, sorry. Not that picky about the suffix.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mithro-old, 2014-05-08 21:47:42]

On 2014/05/08 21:46:06, danakj wrote:
>
https://codereview.chromium.org/275543004/diff/40001/cc/test/output_test_comm...
> File cc/test/output_test_common.h (right):
> 
>
https://codereview.chromium.org/275543004/diff/40001/cc/test/output_test_comm...
> cc/test/output_test_common.h:5: #ifndef CC_TEST_OUTPUT_TEST_COMMON_H_
> On 2014/05/08 21:41:16, mithro wrote:
> > On 2014/05/08 21:37:48, danakj wrote:
> > > I'd prefer begin_frame_args_common.h for this, it's coincidental that we
> ended
> > > up breaking cc into directories in similar way that test filers were
broken
> > up.
> > > Smaller more focused files would be good, we don't need to group
everything
> > that
> > > tests things in output/ together.
> > 
> > Is there any reason not to use cc/debug/begin_frames_args.h or is
> > cc/debug/begin_frames_args_common.h better?
> > 
> 
> I assume you mean cc/test/ not cc/debug/ ?
> 
> I'd use begin_frame_args.h if this file defined a class called BeginFrameArgs.
> Since it doesn't we should clarify that. _common is a "common" way to do that
:)

Well these methods where original in cc/output/begin_frame_args.h and now in
cc/debug/XXXX.h

But happy to go with whenever.

Brian what is your preference?

[mithro-old, 2014-05-08 22:08:26]

> Brian what is your preference?

Brian's preference seems to be cc/test/begin_frame_args_test.{h,cc}

I think that works, Dana, happy with that?

[danakj, 2014-05-08 22:09:19]

On Thu, May 8, 2014 at 6:08 PM, <mithro@mithis.com> wrote:

> Brian what is your preference?
>>
>
> Brian's preference seems to be cc/test/begin_frame_args_test.{h,cc}
>
> I think that works, Dana, happy with that?
>

Yup sgtm.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mithro-old, 2014-05-08 22:22:31]

All done!

PTAL

[brianderson, 2014-05-08 23:40:18]

lgtm

[mithro-old, 2014-05-08 23:45:26]

The CQ bit was checked by mithro@mithis.com

[commit-bot: I haz the power, 2014-05-08 23:53:07]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mithro@mithis.com/275543004/100001

[commit-bot: I haz the power, 2014-05-09 04:04:24]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/18...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-05-09 04:12:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-09 04:12:22]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)

[mithro-old, 2014-05-09 06:04:30]

The CQ bit was checked by mithro@mithis.com

[commit-bot: I haz the power, 2014-05-09 06:09:55]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mithro@mithis.com/275543004/120001

[commit-bot: I haz the power, 2014-05-10 01:50:41]

Change committed as 269474