cc: Small BeginFrameArgs cleanup.

cc: Small BeginFrameArgs cleanup.

 * Rename BeginFrameArgs::CreateTyped to BeginFrameArgs::Create and remove
   other versions.
    * The other constructors where not used anywhere.
    * The non-typed version was accidentally being used to create a SYNCHRONOUS
      BeginFrameArgs.

 * Fixed testing methods to understand type information.

 * Create method for converting the type enum to string.

BUG=346230
Committed: https://crrev.com/bfa0ec0ee961313f82c90a760dd36d18d9ffa0d4
Cr-Commit-Position: refs/heads/master@{#305218}

[mithro-old, 2014-11-19 06:33:39]

mithro@mithis.com changed reviewers:
+ brianderson@chromium.org, simonhong@chromium.org, skyostil@chromium.org

[mithro-old, 2014-11-19 06:34:35]

Hi,

Just a bunch of small clean ups to the BeginFrameArgs class.

Tim

[simonhong, 2014-11-19 13:48:52]

LGTM if compiler error is fixed (ui/compositor/compositor.cc)
I think you missed.

Quick question about style guide.
Which is better way if all parameters are not fit into one line?
a) CallMethod(a,b
                       c,d) or
b) CallMethod(a,
                        b,
                        c,
                        d) ?

[simonhong, 2014-11-19 13:51:14]

On 2014/11/19 13:48:52, simonhong wrote:
> LGTM if compiler error is fixed (ui/compositor/compositor.cc)
> I think you missed.
> 
> Quick question about style guide.
> Which is better way if all parameters are not fit into one line?
> a) CallMethod(a,b
>                        c,d) or
> b) CallMethod(a,
>                         b,
>                         c,
>                         d) ?

Hmm, my previous comment looks ugly.
Case a) locates only c, d parameters in the next line.
Case b) locates all parameters in a different line.

[mithro-old, 2014-11-19 14:08:48]

Whatever "git cl format" does is considered right.

Tim 'mithro' Ansell
On 20 Nov 2014 00:51, <simonhong@chromium.org> wrote:

> On 2014/11/19 13:48:52, simonhong wrote:
>
>> LGTM if compiler error is fixed (ui/compositor/compositor.cc)
>> I think you missed.
>>
>
>  Quick question about style guide.
>> Which is better way if all parameters are not fit into one line?
>> a) CallMethod(a,b
>>                         c,d) or
>> b) CallMethod(a,
>>                          b,
>>                          c,
>>                          d) ?
>>
>
> Hmm, my previous comment looks ugly.
> Case a) locates only c, d parameters in the next line.
> Case b) locates all parameters in a different line.
>
> https://codereview.chromium.org/742683002/
>

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

[danakj, 2014-11-19 15:36:49]

On 2014/11/19 14:08:48, mithro wrote:
> Whatever "git cl format" does is considered right.

+1, and it changed recently, so it'll be different than you're expecting for a
while..

> 
> Tim 'mithro' Ansell
> On 20 Nov 2014 00:51, <mailto:simonhong@chromium.org> wrote:
> 
> > On 2014/11/19 13:48:52, simonhong wrote:
> >
> >> LGTM if compiler error is fixed (ui/compositor/compositor.cc)
> >> I think you missed.
> >>
> >
> >  Quick question about style guide.
> >> Which is better way if all parameters are not fit into one line?
> >> a) CallMethod(a,b
> >>                         c,d) or
> >> b) CallMethod(a,
> >>                          b,
> >>                          c,
> >>                          d) ?
> >>
> >
> > Hmm, my previous comment looks ugly.
> > Case a) locates only c, d parameters in the next line.
> > Case b) locates all parameters in a different line.
> >
> > https://codereview.chromium.org/742683002/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[brianderson, 2014-11-19 20:24:19]

lgtm

[mithro-old, 2014-11-20 03:49:21]

mithro@mithis.com changed reviewers:
+ aelias@chromium.org, cpu@chromium.org, danakj@chromium.org,
darin@chromium.org, jam@chromium.org, piman@chromium.org

[mithro-old, 2014-11-20 03:49:21]

Looks like I need a couple more LGTMs from OWNERs for areas outside cc that are
using BeginFrameArgs.

Any of piman, cpu, jam or darin should cover all the remaining files under
content + ui directories.

Otherwise the combination of danakj (for ui/), skyostil (for some files under
content/) and aelias (for other files under content/) should also work.

[aelias_OOO_until_Jul13, 2014-11-20 04:30:28]

content/browser/*android* lgtm

[Sami, 2014-11-20 11:14:04]

content/browser/android & content/renderer/scheduler lgtm.

[danakj, 2014-11-20 16:06:22]

ui/compositor LGTM

[piman, 2014-11-20 21:08:15]

lgtm

[mithro-old, 2014-11-21 03:09:12]

mithro@mithis.com changed reviewers:
+ picksi@chromium.org

[mithro-old, 2014-11-21 03:09:13]

Hi Simon,

These comments actually belong on this CL
(https://codereview.chromium.org/742683002/) not on
https://codereview.chromium.org/735723005 (which depends on this CL).

Tim 'mithro' Ansell

---

https://codereview.chromium.org/735723005/diff/120001/cc/output/begin_frame_a...
File cc/output/begin_frame_args.cc (right):

https://codereview.chromium.org/735723005/diff/120001/cc/output/begin_frame_a...
cc/output/begin_frame_args.cc:13: switch (type) {
On 2014/11/20 14:04:20, picksi1 wrote:
> should this be a const function?

Static functions can't be const.

https://codereview.chromium.org/735723005/diff/120001/cc/output/begin_frame_a...
cc/output/begin_frame_args.cc:24: return "???";
On 2014/11/20 14:04:20, picksi1 wrote:
> If this return value is Logged somewhere (in the trace events, I guess) it
might
> be helpful to explicitly say "UNKNOWN BEGIN FRAME TYPE" - it would also be
> really nice to add the numeric value of the unknown type...

This follows the approach in cc/scheduler/scheduler_state_machine.cc

This function needs to return a "const char*" so there is no way to include the
numeric value without leaking memory.

https://codereview.chromium.org/735723005/diff/120001/cc/output/begin_frame_a...
cc/output/begin_frame_args.cc:24: return "???";
On 2014/11/20 16:03:02, danakj wrote:
> On 2014/11/20 14:04:20, picksi1 wrote:
> > If this return value is Logged somewhere (in the trace events, I guess) it
> might
> > be helpful to explicitly say "UNKNOWN BEGIN FRAME TYPE" - it would also be
> > really nice to add the numeric value of the unknown type...
> 
> The NOTREACHED implies we never get to this line. We don't need to do anything
> sane here.

NOTREACHED() doesn't fail where DCHECK is not enabled, so we still need to
return something.

https://codereview.chromium.org/735723005/diff/120001/cc/scheduler/begin_fram...
File cc/scheduler/begin_frame_source_unittest.cc (right):

https://codereview.chromium.org/735723005/diff/120001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source_unittest.cc:47:
(source).TestOnBeginFrame(new_args);                              \
On 2014/11/20 14:04:20, picksi1 wrote:
> ASSERT_FALSE(old_args == new_args) ?

Done.

[mithro-old, 2014-11-21 03:10:25]

The CQ bit was checked by mithro@mithis.com

[commit-bot: I haz the power, 2014-11-21 03:12:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/742683002/100001

[commit-bot: I haz the power, 2014-11-21 03:34:45]

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

[commit-bot: I haz the power, 2014-11-21 03:34:47]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[mithro-old, 2014-11-21 07:41:15]

The CQ bit was checked by mithro@mithis.com

[commit-bot: I haz the power, 2014-11-21 07:42:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/742683002/120001

[mithro-old, 2014-11-21 14:00:10]

The CQ bit was checked by mithro@mithis.com

[commit-bot: I haz the power, 2014-11-21 14:00:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/742683002/160001

[commit-bot: I haz the power, 2014-11-21 15:13:57]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2014-11-21 15:14:38]

Patchset 9 (id:??) landed as
https://crrev.com/bfa0ec0ee961313f82c90a760dd36d18d9ffa0d4
Cr-Commit-Position: refs/heads/master@{#305218}