cc: Add command line for frame production throttle

cc: Add command line for frame production throttle.

This CL decouples "disable gpu vsync" and "throttle frame production".

1. Add options of display-gpu-vsync
--disable-gpu-vsync=gpu :  Sets |disable_display_vsync| true.
--disable-gpu-vsync=beginframe : Sets |throttle_frame_production| false.
--disable-gpu-vsync : Same as using both flags above.

2. Rename |disable_gpu_vsync| variable to |disable_display_vsync|.

BUG=492732
R=brianderson@chromium.org, sky@chromium.org, danakj@chromium.org, jam@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/5899bd55e2174cb636c77d45d64827b76bda31b7
Cr-Commit-Position: refs/heads/master@{#338800}

[Jimmy Jo, 2015-06-13 13:26:10]

jincheol.jo@navercorp.com changed reviewers:
+ brianderson@chromium.org

[Jimmy Jo, 2015-06-13 13:45:30]

PTAL!

[chromium-reviews, 2015-06-13 18:01:39]

what does it mean to not throttle frames but still use vsync?
On Jun 13, 2015 6:45 AM, <jincheol.jo@navercorp.com> wrote:

> Reviewers: brianderson,
>
> Message:
> PTAL!
>
> Description:
> cc: Add command line for frame production throttle.
>
> This CL is implemented for the decoupling "disable gpu vsync" and "throttle
> frame production".
>
> BUG=492732
> CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
>
> Please review this at https://codereview.chromium.org/1177383005/
>
> Base URL: https://chromium.googlesource.com/chromium/src.git@master
>
> Affected files (+10, -1 lines):
>   M cc/base/switches.h
>   M cc/base/switches.cc
>   M cc/trees/layer_tree_settings.cc
>   M content/browser/renderer_host/render_process_host_impl.cc
>
>
> Index: cc/base/switches.cc
> diff --git a/cc/base/switches.cc b/cc/base/switches.cc
> index
>
b95ac111cc4d78e133e68d26a3aff72e42fdd0c4..675052dc2573157ef64a567219349dc7aeb9fcc3
> 100644
> --- a/cc/base/switches.cc
> +++ b/cc/base/switches.cc
> @@ -103,5 +103,9 @@ const char kCCLayerTreeTestNoTimeout[] =
> "cc-layer-tree-test-no-timeout";
>  // Makes pixel tests write their output instead of read it.
>  const char kCCRebaselinePixeltests[] = "cc-rebaseline-pixeltests";
>
> +// Disable throttle of frame production.
> +const char kDisableThrottleFrameProduction[] =
> +    "disable-frame-production-throttle";
> +
>  }  // namespace switches
>  }  // namespace cc
> Index: cc/base/switches.h
> diff --git a/cc/base/switches.h b/cc/base/switches.h
> index
>
2d29341acbcde5be5d3a60b3e6b66a33f13dd2c9..cf2c527b762a7abe56f6c54536f686b70bc1f3f7
> 100644
> --- a/cc/base/switches.h
> +++ b/cc/base/switches.h
> @@ -54,6 +54,7 @@ CC_EXPORT extern const char
> kUIShowReplicaScreenSpaceRects[];
>  CC_EXPORT extern const char kCCLayerTreeTestNoTimeout[];
>  CC_EXPORT extern const char kCCRebaselinePixeltests[];
>
> +CC_EXPORT extern const char kDisableThrottleFrameProduction[];
>  }  // namespace switches
>  }  // namespace cc
>
> Index: cc/trees/layer_tree_settings.cc
> diff --git a/cc/trees/layer_tree_settings.cc
> b/cc/trees/layer_tree_settings.cc
> index
>
80e356e0c3a80fa5e71602242a7c0df12a1a4d66..6fd9e8bc9de476f7a0567d58e0b4f68adce97c87
> 100644
> --- a/cc/trees/layer_tree_settings.cc
> +++ b/cc/trees/layer_tree_settings.cc
> @@ -10,6 +10,7 @@
>  #include "base/command_line.h"
>  #include "base/logging.h"
>  #include "base/strings/string_number_conversions.h"
> +#include "cc/base/switches.h"
>
>  namespace cc {
>
> @@ -81,6 +82,8 @@ LayerTreeSettings::LayerTreeSettings()
>  LayerTreeSettings::~LayerTreeSettings() {}
>
>  SchedulerSettings LayerTreeSettings::ToSchedulerSettings() const {
> +  base::CommandLine* cmd = base::CommandLine::ForCurrentProcess();
> +
>    SchedulerSettings scheduler_settings;
>    scheduler_settings.use_external_begin_frame_source =
>        use_external_begin_frame_source;
> @@ -94,7 +97,7 @@ SchedulerSettings
> LayerTreeSettings::ToSchedulerSettings() const {
>    scheduler_settings.using_synchronous_renderer_compositor =
>        using_synchronous_renderer_compositor;
>    scheduler_settings.throttle_frame_production =
> -      !renderer_settings.disable_gpu_vsync;
> +      !cmd->HasSwitch(switches::kDisableThrottleFrameProduction);
>    scheduler_settings.main_thread_should_always_be_low_latency = false;
>    scheduler_settings.background_frame_interval =
>        base::TimeDelta::FromSecondsD(1.0 / background_animation_rate);
> Index: content/browser/renderer_host/render_process_host_impl.cc
> diff --git a/content/browser/renderer_host/render_process_host_impl.cc
> b/content/browser/renderer_host/render_process_host_impl.cc
> index
>
db5ea5d7a4f71b2d7cb1e95f9a9fff93aee979b7..f1f8d06a29172b2271d11795305a209fd255af6e
> 100644
> --- a/content/browser/renderer_host/render_process_host_impl.cc
> +++ b/content/browser/renderer_host/render_process_host_impl.cc
> @@ -1357,6 +1357,7 @@ void
> RenderProcessHostImpl::PropagateBrowserCommandLineToRenderer(
>      cc::switches::kStrictLayerPropertyChangeChecking,
>      cc::switches::kTopControlsHideThreshold,
>      cc::switches::kTopControlsShowThreshold,
> +    cc::switches::kDisableThrottleFrameProduction,
>
>      scheduler::switches::kDisableBlinkScheduler,
>
>
>
>

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

[Jimmy Jo, 2015-06-14 08:38:08]

On 2015/06/13 18:01:39, chromium-reviews wrote:
> what does it mean to not throttle frames but still use vsync?
> On Jun 13, 2015 6:45 AM, <mailto:jincheol.jo@navercorp.com> wrote:
> 
> > Reviewers: brianderson,
> >
> > Message:
> > PTAL!
> >
> > Description:
> > cc: Add command line for frame production throttle.
> >
> > This CL is implemented for the decoupling "disable gpu vsync" and "throttle
> > frame production".
> >
> > BUG=492732
> > CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
> >
> > Please review this at https://codereview.chromium.org/1177383005/
> >
> > Base URL: https://chromium.googlesource.com/chromium/src.git@master
> >
> > Affected files (+10, -1 lines):
> >   M cc/base/switches.h
> >   M cc/base/switches.cc
> >   M cc/trees/layer_tree_settings.cc
> >   M content/browser/renderer_host/render_process_host_impl.cc
> >
> >
> > Index: cc/base/switches.cc
> > diff --git a/cc/base/switches.cc b/cc/base/switches.cc
> > index
> >
>
b95ac111cc4d78e133e68d26a3aff72e42fdd0c4..675052dc2573157ef64a567219349dc7aeb9fcc3
> > 100644
> > --- a/cc/base/switches.cc
> > +++ b/cc/base/switches.cc
> > @@ -103,5 +103,9 @@ const char kCCLayerTreeTestNoTimeout[] =
> > "cc-layer-tree-test-no-timeout";
> >  // Makes pixel tests write their output instead of read it.
> >  const char kCCRebaselinePixeltests[] = "cc-rebaseline-pixeltests";
> >
> > +// Disable throttle of frame production.
> > +const char kDisableThrottleFrameProduction[] =
> > +    "disable-frame-production-throttle";
> > +
> >  }  // namespace switches
> >  }  // namespace cc
> > Index: cc/base/switches.h
> > diff --git a/cc/base/switches.h b/cc/base/switches.h
> > index
> >
>
2d29341acbcde5be5d3a60b3e6b66a33f13dd2c9..cf2c527b762a7abe56f6c54536f686b70bc1f3f7
> > 100644
> > --- a/cc/base/switches.h
> > +++ b/cc/base/switches.h
> > @@ -54,6 +54,7 @@ CC_EXPORT extern const char
> > kUIShowReplicaScreenSpaceRects[];
> >  CC_EXPORT extern const char kCCLayerTreeTestNoTimeout[];
> >  CC_EXPORT extern const char kCCRebaselinePixeltests[];
> >
> > +CC_EXPORT extern const char kDisableThrottleFrameProduction[];
> >  }  // namespace switches
> >  }  // namespace cc
> >
> > Index: cc/trees/layer_tree_settings.cc
> > diff --git a/cc/trees/layer_tree_settings.cc
> > b/cc/trees/layer_tree_settings.cc
> > index
> >
>
80e356e0c3a80fa5e71602242a7c0df12a1a4d66..6fd9e8bc9de476f7a0567d58e0b4f68adce97c87
> > 100644
> > --- a/cc/trees/layer_tree_settings.cc
> > +++ b/cc/trees/layer_tree_settings.cc
> > @@ -10,6 +10,7 @@
> >  #include "base/command_line.h"
> >  #include "base/logging.h"
> >  #include "base/strings/string_number_conversions.h"
> > +#include "cc/base/switches.h"
> >
> >  namespace cc {
> >
> > @@ -81,6 +82,8 @@ LayerTreeSettings::LayerTreeSettings()
> >  LayerTreeSettings::~LayerTreeSettings() {}
> >
> >  SchedulerSettings LayerTreeSettings::ToSchedulerSettings() const {
> > +  base::CommandLine* cmd = base::CommandLine::ForCurrentProcess();
> > +
> >    SchedulerSettings scheduler_settings;
> >    scheduler_settings.use_external_begin_frame_source =
> >        use_external_begin_frame_source;
> > @@ -94,7 +97,7 @@ SchedulerSettings
> > LayerTreeSettings::ToSchedulerSettings() const {
> >    scheduler_settings.using_synchronous_renderer_compositor =
> >        using_synchronous_renderer_compositor;
> >    scheduler_settings.throttle_frame_production =
> > -      !renderer_settings.disable_gpu_vsync;
> > +      !cmd->HasSwitch(switches::kDisableThrottleFrameProduction);
> >    scheduler_settings.main_thread_should_always_be_low_latency = false;
> >    scheduler_settings.background_frame_interval =
> >        base::TimeDelta::FromSecondsD(1.0 / background_animation_rate);
> > Index: content/browser/renderer_host/render_process_host_impl.cc
> > diff --git a/content/browser/renderer_host/render_process_host_impl.cc
> > b/content/browser/renderer_host/render_process_host_impl.cc
> > index
> >
>
db5ea5d7a4f71b2d7cb1e95f9a9fff93aee979b7..f1f8d06a29172b2271d11795305a209fd255af6e
> > 100644
> > --- a/content/browser/renderer_host/render_process_host_impl.cc
> > +++ b/content/browser/renderer_host/render_process_host_impl.cc
> > @@ -1357,6 +1357,7 @@ void
> > RenderProcessHostImpl::PropagateBrowserCommandLineToRenderer(
> >      cc::switches::kStrictLayerPropertyChangeChecking,
> >      cc::switches::kTopControlsHideThreshold,
> >      cc::switches::kTopControlsShowThreshold,
> > +    cc::switches::kDisableThrottleFrameProduction,
> >
> >      scheduler::switches::kDisableBlinkScheduler,
> >
> >
> >
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

In my understanding, it doesn`t use vsync if not throttle frame.
Instead, it makes use of BackToBackBeginFrameSource.
This call beginFrame as soon as  remaining frames reaches zero.

[brianderson, 2015-06-15 17:56:51]

Copying comments from the bug here:

There are number of test frameworks and people who depend on
"--disable-gpu-vsync" being available, so we should make sure not to change the
meaning of that flag.

How about we have three flags:
--disable-display-vsync :  Sets |disable_display_vsync| false.
--disable-compositor-vsync : Sets |throttle_frame_production| false.
--disable-gpu-vsync : Disables both display and compositor vsync.

In the code, rename |disable_gpu_vsync| variable names to
|disable_display_vsync| so we avoid confusion with the command line flags names.

[brianderson, 2015-06-15 18:04:39]

Disabling frame production throttling without disabling vsync would cause the
compositor to tie the start of a frame to the swap ack of the previous frame
instead of waiting for a vsync-aligned BeginFrame.

This could improve the throughput of RAF+raster bound use-cases at the cost of
smoothness and latency.

[danakj, 2015-06-15 18:10:34]

On Mon, Jun 15, 2015 at 11:04 AM, <brianderson@chromium.org> wrote:

> Disabling frame production throttling without disabling vsync would cause
> the
> compositor to tie the start of a frame to the swap ack of the previous
> frame
> instead of waiting for a vsync-aligned BeginFrame.
>
> This could improve the throughput of RAF+raster bound use-cases at the
> cost of
> smoothness and latency.
>

So it's like --disable-backloading-beginframes or
--beginframe-immediately-inside-vsync something? It's still throttling and
it's still using vsync?

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

[brianderson, 2015-06-15 18:46:57]

On 2015/06/15 18:10:34, danakj wrote:
> On Mon, Jun 15, 2015 at 11:04 AM, <mailto:brianderson@chromium.org> wrote:
> 
> > Disabling frame production throttling without disabling vsync would cause
> > the
> > compositor to tie the start of a frame to the swap ack of the previous
> > frame
> > instead of waiting for a vsync-aligned BeginFrame.
> >
> > This could improve the throughput of RAF+raster bound use-cases at the
> > cost of
> > smoothness and latency.
> >
> 
> So it's like --disable-backloading-beginframes or
> --beginframe-immediately-inside-vsync something?

It's more like --beginframe-immediately-once-not-swap-throttled, though it would
be nice to have a shorter name. =)

> It's still throttling and it's still using vsync?
> 

I guess "--disable-compositor-vsync" would be a bad name. It's not using vsync
directly, but is tied to it indirectly via swap ack throttling from the Display.
"--disable-compositor-vsync" sounds like it would disable swap ack throttling of
the compositor too and let it submit more frames than are displayed.  However,
we can't remove swap ack throttling without some other form of back pressure
based on the compositor's gpu throughput, such as a "draw ack"; otherwise, we
could end up with a large queue of gpu work.

[danakj, 2015-06-15 19:16:17]

On Mon, Jun 15, 2015 at 11:46 AM, <brianderson@chromium.org> wrote:

> On 2015/06/15 18:10:34, danakj wrote:
>
>> On Mon, Jun 15, 2015 at 11:04 AM, <mailto:brianderson@chromium.org>
>> wrote:
>>
>
>  > Disabling frame production throttling without disabling vsync would
>> cause
>> > the
>> > compositor to tie the start of a frame to the swap ack of the previous
>> > frame
>> > instead of waiting for a vsync-aligned BeginFrame.
>> >
>> > This could improve the throughput of RAF+raster bound use-cases at the
>> > cost of
>> > smoothness and latency.
>> >
>>
>
>  So it's like --disable-backloading-beginframes or
>> --beginframe-immediately-inside-vsync something?
>>
>
> It's more like --beginframe-immediately-once-not-swap-throttled, though it
> would
> be nice to have a shorter name. =)


--disable-beginframe-delay?


>
>
>  It's still throttling and it's still using vsync?
>>
>
>
> I guess "--disable-compositor-vsync" would be a bad name. It's not using
> vsync
> directly, but is tied to it indirectly via swap ack throttling from the
> Display.
> "--disable-compositor-vsync" sounds like it would disable swap ack
> throttling of
> the compositor too and let it submit more frames than are displayed.
> However,
> we can't remove swap ack throttling without some other form of back
> pressure
> based on the compositor's gpu throughput, such as a "draw ack"; otherwise,
> we
> could end up with a large queue of gpu work.
>
>
>
>
>
> https://codereview.chromium.org/1177383005/
>

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

[Jimmy Jo, 2015-06-16 11:48:13]

In summary, there are two stuff. right?

1. add three command line flags
--disable-display-vsync :  Sets |disable_display_vsync| false.
--disable-compositor-vsync : Sets |throttle_frame_production| false.
--disable-gpu-vsync : Disables both display and compositor vsync.

2. rename |disable_display_vsync| variable.

Please make sure the naming for command line flags.

Thanks. Brian and Dana.

[Jimmy Jo, 2015-06-16 12:58:51]

Patchset #1 (id:1) has been deleted

[brianderson, 2015-06-16 16:54:58]

On 2015/06/16 11:48:13, Jimmy Jo wrote:
> In summary, there are two stuff. right?
> 
> 1. add three command line flags
> --disable-display-vsync :  Sets |disable_display_vsync| false.
> --disable-compositor-vsync : Sets |throttle_frame_production| false.
> --disable-gpu-vsync : Disables both display and compositor vsync.
> 
> 2. rename |disable_display_vsync| variable.
> 
> Please make sure the naming for command line flags.
> 
> Thanks. Brian and Dana.


Yep!

We're still trying to iron out a good name for "--disable-compositor-vsync".
"--disable-beginframe-delay" is getting close, but still a bit vague. What about
"--disable-beginframe-interval"? The BackToBackBeginFrameSource doesn't have an
interval, but the other sources do.

[Jimmy Jo, 2015-06-17 12:21:55]

On 2015/06/16 16:54:58, brianderson wrote:
> On 2015/06/16 11:48:13, Jimmy Jo wrote:
> > In summary, there are two stuff. right?
> > 
> > 1. add three command line flags
> > --disable-display-vsync :  Sets |disable_display_vsync| false.
> > --disable-compositor-vsync : Sets |throttle_frame_production| false.
> > --disable-gpu-vsync : Disables both display and compositor vsync.
> > 
> > 2. rename |disable_display_vsync| variable.
> > 
> > Please make sure the naming for command line flags.
> > 
> > Thanks. Brian and Dana.
> 
> 
> Yep!
> 
> We're still trying to iron out a good name for "--disable-compositor-vsync".
> "--disable-beginframe-delay" is getting close, but still a bit vague. What
about
> "--disable-beginframe-interval"? The BackToBackBeginFrameSource doesn't have
an
> interval, but the other sources do.

Does the BackToBackBFS have 16.666ms interval? 
https://code.google.com/p/chromium/codesearch#chromium/src/cc/scheduler/begin...

[brianderson, 2015-06-17 17:48:37]

On 2015/06/17 12:21:55, Jimmy Jo wrote:
> On 2015/06/16 16:54:58, brianderson wrote:
> > On 2015/06/16 11:48:13, Jimmy Jo wrote:
> > > In summary, there are two stuff. right?
> > > 
> > > 1. add three command line flags
> > > --disable-display-vsync :  Sets |disable_display_vsync| false.
> > > --disable-compositor-vsync : Sets |throttle_frame_production| false.
> > > --disable-gpu-vsync : Disables both display and compositor vsync.
> > > 
> > > 2. rename |disable_display_vsync| variable.
> > > 
> > > Please make sure the naming for command line flags.
> > > 
> > > Thanks. Brian and Dana.
> > 
> > 
> > Yep!
> > 
> > We're still trying to iron out a good name for "--disable-compositor-vsync".
> > "--disable-beginframe-delay" is getting close, but still a bit vague. What
> about
> > "--disable-beginframe-interval"? The BackToBackBeginFrameSource doesn't have
> an
> > interval, but the other sources do.
> 
> Does the BackToBackBFS have 16.666ms interval? 
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/scheduler/begin...

Conceptually, there's a begin_frame_interval and a vsync_interval. Usually they
are equal, except for the BackToBackBFS where the begin_frame_interval is
effectively 0 and is limited only by how fast we can draw.

[Jimmy Jo, 2015-06-18 01:42:29]

On 2015/06/17 17:48:37, brianderson wrote:
> On 2015/06/17 12:21:55, Jimmy Jo wrote:
> > On 2015/06/16 16:54:58, brianderson wrote:
> > > On 2015/06/16 11:48:13, Jimmy Jo wrote:
> > > > In summary, there are two stuff. right?
> > > > 
> > > > 1. add three command line flags
> > > > --disable-display-vsync :  Sets |disable_display_vsync| false.
> > > > --disable-compositor-vsync : Sets |throttle_frame_production| false.
> > > > --disable-gpu-vsync : Disables both display and compositor vsync.
> > > > 
> > > > 2. rename |disable_display_vsync| variable.
> > > > 
> > > > Please make sure the naming for command line flags.
> > > > 
> > > > Thanks. Brian and Dana.
> > > 
> > > 
> > > Yep!
> > > 
> > > We're still trying to iron out a good name for
"--disable-compositor-vsync".
> > > "--disable-beginframe-delay" is getting close, but still a bit vague. What
> > about
> > > "--disable-beginframe-interval"? The BackToBackBeginFrameSource doesn't
have
> > an
> > > interval, but the other sources do.
> > 
> > Does the BackToBackBFS have 16.666ms interval? 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/scheduler/begin...
> 
> Conceptually, there's a begin_frame_interval and a vsync_interval. Usually
they
> are equal, except for the BackToBackBFS where the begin_frame_interval is
> effectively 0 and is limited only by how fast we can draw.

I misunderstood. sorry about that.
The interval I mentioned is deadline.
I understood BackToBackBFS have 0 interval. 
https://code.google.com/p/chromium/codesearch#chromium/src/cc/scheduler/begin...

[Jimmy Jo, 2015-06-18 12:22:33]

On 2015/06/18 01:42:29, Jimmy Jo wrote:
> On 2015/06/17 17:48:37, brianderson wrote:
> > On 2015/06/17 12:21:55, Jimmy Jo wrote:
> > > On 2015/06/16 16:54:58, brianderson wrote:
> > > > On 2015/06/16 11:48:13, Jimmy Jo wrote:
> > > > > In summary, there are two stuff. right?
> > > > > 
> > > > > 1. add three command line flags
> > > > > --disable-display-vsync :  Sets |disable_display_vsync| false.
> > > > > --disable-compositor-vsync : Sets |throttle_frame_production| false.
> > > > > --disable-gpu-vsync : Disables both display and compositor vsync.
> > > > > 
> > > > > 2. rename |disable_display_vsync| variable.
> > > > > 
> > > > > Please make sure the naming for command line flags.
> > > > > 
> > > > > Thanks. Brian and Dana.
> > > > 
> > > > 
> > > > Yep!
> > > > 
> > > > We're still trying to iron out a good name for
> "--disable-compositor-vsync".
> > > > "--disable-beginframe-delay" is getting close, but still a bit vague.
What
> > > about
> > > > "--disable-beginframe-interval"? The BackToBackBeginFrameSource doesn't
> have
> > > an
> > > > interval, but the other sources do.
> > > 
> > > Does the BackToBackBFS have 16.666ms interval? 
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/scheduler/begin...
> > 
> > Conceptually, there's a begin_frame_interval and a vsync_interval. Usually
> they
> > are equal, except for the BackToBackBFS where the begin_frame_interval is
> > effectively 0 and is limited only by how fast we can draw.
> 
> I misunderstood. sorry about that.
> The interval I mentioned is deadline.
> I understood BackToBackBFS have 0 interval. 
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/scheduler/begin...

I think --disable-beginframe-delay is matched.

[Jimmy Jo, 2015-06-22 05:22:58]

On 2015/06/18 12:22:33, Jimmy Jo wrote:
> On 2015/06/18 01:42:29, Jimmy Jo wrote:
> > On 2015/06/17 17:48:37, brianderson wrote:
> > > On 2015/06/17 12:21:55, Jimmy Jo wrote:
> > > > On 2015/06/16 16:54:58, brianderson wrote:
> > > > > On 2015/06/16 11:48:13, Jimmy Jo wrote:
> > > > > > In summary, there are two stuff. right?
> > > > > > 
> > > > > > 1. add three command line flags
> > > > > > --disable-display-vsync :  Sets |disable_display_vsync| false.
> > > > > > --disable-compositor-vsync : Sets |throttle_frame_production| false.
> > > > > > --disable-gpu-vsync : Disables both display and compositor vsync.
> > > > > > 
> > > > > > 2. rename |disable_display_vsync| variable.
> > > > > > 
> > > > > > Please make sure the naming for command line flags.
> > > > > > 
> > > > > > Thanks. Brian and Dana.
> > > > > 
> > > > > 
> > > > > Yep!
> > > > > 
> > > > > We're still trying to iron out a good name for
> > "--disable-compositor-vsync".
> > > > > "--disable-beginframe-delay" is getting close, but still a bit vague.
> What
> > > > about
> > > > > "--disable-beginframe-interval"? The BackToBackBeginFrameSource
doesn't
> > have
> > > > an
> > > > > interval, but the other sources do.
> > > > 
> > > > Does the BackToBackBFS have 16.666ms interval? 
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/scheduler/begin...
> > > 
> > > Conceptually, there's a begin_frame_interval and a vsync_interval. Usually
> > they
> > > are equal, except for the BackToBackBFS where the begin_frame_interval is
> > > effectively 0 and is limited only by how fast we can draw.
> > 
> > I misunderstood. sorry about that.
> > The interval I mentioned is deadline.
> > I understood BackToBackBFS have 0 interval. 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/scheduler/begin...
> 
> I think --disable-beginframe-delay is matched.

Are you still naming this?

[Jimmy Jo, 2015-06-24 00:32:12]

On 2015/06/22 05:22:58, Jimmy Jo wrote:
> On 2015/06/18 12:22:33, Jimmy Jo wrote:
> > On 2015/06/18 01:42:29, Jimmy Jo wrote:
> > > On 2015/06/17 17:48:37, brianderson wrote:
> > > > On 2015/06/17 12:21:55, Jimmy Jo wrote:
> > > > > On 2015/06/16 16:54:58, brianderson wrote:
> > > > > > On 2015/06/16 11:48:13, Jimmy Jo wrote:
> > > > > > > In summary, there are two stuff. right?
> > > > > > > 
> > > > > > > 1. add three command line flags
> > > > > > > --disable-display-vsync :  Sets |disable_display_vsync| false.
> > > > > > > --disable-compositor-vsync : Sets |throttle_frame_production|
false.
> > > > > > > --disable-gpu-vsync : Disables both display and compositor vsync.
> > > > > > > 
> > > > > > > 2. rename |disable_display_vsync| variable.
> > > > > > > 
> > > > > > > Please make sure the naming for command line flags.
> > > > > > > 
> > > > > > > Thanks. Brian and Dana.
> > > > > > 
> > > > > > 
> > > > > > Yep!
> > > > > > 
> > > > > > We're still trying to iron out a good name for
> > > "--disable-compositor-vsync".
> > > > > > "--disable-beginframe-delay" is getting close, but still a bit
vague.
> > What
> > > > > about
> > > > > > "--disable-beginframe-interval"? The BackToBackBeginFrameSource
> doesn't
> > > have
> > > > > an
> > > > > > interval, but the other sources do.
> > > > > 
> > > > > Does the BackToBackBFS have 16.666ms interval? 
> > > > >
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/scheduler/begin...
> > > > 
> > > > Conceptually, there's a begin_frame_interval and a vsync_interval.
Usually
> > > they
> > > > are equal, except for the BackToBackBFS where the begin_frame_interval
is
> > > > effectively 0 and is limited only by how fast we can draw.
> > > 
> > > I misunderstood. sorry about that.
> > > The interval I mentioned is deadline.
> > > I understood BackToBackBFS have 0 interval. 
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/scheduler/begin...
> > 
> > I think --disable-beginframe-delay is matched.
> 
> Are you still naming this?

ping

[brianderson, 2015-06-24 01:14:41]

On 2015/06/24 00:32:12, Jimmy Jo wrote:
> On 2015/06/22 05:22:58, Jimmy Jo wrote:
> > On 2015/06/18 12:22:33, Jimmy Jo wrote:
> > > On 2015/06/18 01:42:29, Jimmy Jo wrote:
> > > > On 2015/06/17 17:48:37, brianderson wrote:
> > > > > On 2015/06/17 12:21:55, Jimmy Jo wrote:
> > > > > > On 2015/06/16 16:54:58, brianderson wrote:
> > > > > > > On 2015/06/16 11:48:13, Jimmy Jo wrote:
> > > > > > > > In summary, there are two stuff. right?
> > > > > > > > 
> > > > > > > > 1. add three command line flags
> > > > > > > > --disable-display-vsync :  Sets |disable_display_vsync| false.
> > > > > > > > --disable-compositor-vsync : Sets |throttle_frame_production|
> false.
> > > > > > > > --disable-gpu-vsync : Disables both display and compositor
vsync.
> > > > > > > > 
> > > > > > > > 2. rename |disable_display_vsync| variable.
> > > > > > > > 
> > > > > > > > Please make sure the naming for command line flags.
> > > > > > > > 
> > > > > > > > Thanks. Brian and Dana.
> > > > > > > 
> > > > > > > 
> > > > > > > Yep!
> > > > > > > 
> > > > > > > We're still trying to iron out a good name for
> > > > "--disable-compositor-vsync".
> > > > > > > "--disable-beginframe-delay" is getting close, but still a bit
> vague.
> > > What
> > > > > > about
> > > > > > > "--disable-beginframe-interval"? The BackToBackBeginFrameSource
> > doesn't
> > > > have
> > > > > > an
> > > > > > > interval, but the other sources do.
> > > > > > 
> > > > > > Does the BackToBackBFS have 16.666ms interval? 
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/scheduler/begin...
> > > > > 
> > > > > Conceptually, there's a begin_frame_interval and a vsync_interval.
> Usually
> > > > they
> > > > > are equal, except for the BackToBackBFS where the begin_frame_interval
> is
> > > > > effectively 0 and is limited only by how fast we can draw.
> > > > 
> > > > I misunderstood. sorry about that.
> > > > The interval I mentioned is deadline.
> > > > I understood BackToBackBFS have 0 interval. 
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/scheduler/begin...
> > > 
> > > I think --disable-beginframe-delay is matched.
> > 
> > Are you still naming this?
> 
> ping

[brianderson, 2015-06-24 01:16:45]

Please use "--disable-beginframe-interval".

+mithro and sunnyps in case they have an opinion regarding naming.

https://codereview.chromium.org/1177383005/diff/20001/cc/trees/layer_tree_set...
File cc/trees/layer_tree_settings.cc (right):

https://codereview.chromium.org/1177383005/diff/20001/cc/trees/layer_tree_set...
cc/trees/layer_tree_settings.cc:102:
(cmd->HasSwitch(::switches::kDisableGpuVsync)
We try to avoid checking the command line flags directly in cc code. Can you
move this to the same place we check for disable-display-vsync and assign it to
a new member variable of LayerTreeSettings?

https://codereview.chromium.org/1177383005/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/1177383005/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:1360:
cc::switches::kDisableDisplayVsync,
Please group this one with kDisableGpuVsync since it will affect the hardware
vsync.

https://codereview.chromium.org/1177383005/diff/20001/content/renderer/gpu/re...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/1177383005/diff/20001/content/renderer/gpu/re...
content/renderer/gpu/render_widget_compositor.cc:230: :
!cmd->HasSwitch(cc::switches::kDisableDisplayVsync));
Can this (and elsehwere) be a simple OR:

cmd->HasSwitch(switches::kDisableGpuVsync) ||
cmd->HasSwitch(cc::switches::kDisableDisplayVsync)

?

[Jimmy Jo, 2015-06-24 09:38:31]

Update patch.

https://codereview.chromium.org/1177383005/diff/20001/cc/trees/layer_tree_set...
File cc/trees/layer_tree_settings.cc (right):

https://codereview.chromium.org/1177383005/diff/20001/cc/trees/layer_tree_set...
cc/trees/layer_tree_settings.cc:102:
(cmd->HasSwitch(::switches::kDisableGpuVsync)
On 2015/06/24 01:16:45, brianderson wrote:
> We try to avoid checking the command line flags directly in cc code. Can you
> move this to the same place we check for disable-display-vsync and assign it
to
> a new member variable of LayerTreeSettings?

Done.

https://codereview.chromium.org/1177383005/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/1177383005/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:1360:
cc::switches::kDisableDisplayVsync,
On 2015/06/24 01:16:45, brianderson wrote:
> Please group this one with kDisableGpuVsync since it will affect the hardware
> vsync.

Done.

https://codereview.chromium.org/1177383005/diff/20001/content/renderer/gpu/re...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/1177383005/diff/20001/content/renderer/gpu/re...
content/renderer/gpu/render_widget_compositor.cc:230: :
!cmd->HasSwitch(cc::switches::kDisableDisplayVsync));
On 2015/06/24 01:16:45, brianderson wrote:
> Can this (and elsehwere) be a simple OR:
> 
> cmd->HasSwitch(switches::kDisableGpuVsync) ||
> cmd->HasSwitch(cc::switches::kDisableDisplayVsync)
> 
> ?

I misunderstood. When we turn on kDisableDisplayVsync,  |disable_display_vsync|
should be true. 
Done

[Jimmy Jo, 2015-06-24 09:46:47]

Patchset #2 (id:40001) has been deleted

[Jimmy Jo, 2015-06-26 01:23:46]

ping...

[brianderson, 2015-06-26 01:46:01]

lgtm, though you'll need an OWNERS review for the /content and /ui changes.

https://codereview.chromium.org/1177383005/diff/60001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/1177383005/diff/60001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:1231:
switches::kDisableDisplayVsync,
nit: To be consistent with the rest of the code use VSync instead of Vsync.

[Jimmy Jo, 2015-06-26 07:37:24]

jincheol.jo@navercorp.com changed reviewers:
+ jam@chromium.org, sky@chromium.org

[Jimmy Jo, 2015-06-26 07:38:06]

On 2015/06/26 01:46:01, brianderson wrote:
> lgtm, though you'll need an OWNERS review for the /content and /ui changes.
> 
>
https://codereview.chromium.org/1177383005/diff/60001/content/browser/rendere...
> File content/browser/renderer_host/render_process_host_impl.cc (right):
> 
>
https://codereview.chromium.org/1177383005/diff/60001/content/browser/rendere...
> content/browser/renderer_host/render_process_host_impl.cc:1231:
> switches::kDisableDisplayVsync,
> nit: To be consistent with the rest of the code use VSync instead of Vsync.

+jam for /conent
+sky for /ui

PTAL!!

[sky, 2015-06-29 02:20:28]

https://codereview.chromium.org/1177383005/diff/60001/ui/compositor/composito...
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/1177383005/diff/60001/ui/compositor/composito...
ui/compositor/compositor.cc:98:
(command_line->HasSwitch(switches::kDisableGpuVsync) ||
nit: run git cl format

https://codereview.chromium.org/1177383005/diff/60001/ui/gl/gl_switches.cc
File ui/gl/gl_switches.cc (right):

https://codereview.chromium.org/1177383005/diff/60001/ui/gl/gl_switches.cc#ne...
ui/gl/gl_switches.cc:35: //  for the decoupling "disable gpu vsync" and
"throttle frame production".
nit: too many spaces here.

[Jimmy Jo, 2015-06-29 02:37:50]

Thanks Sky.
PTAL again.

https://codereview.chromium.org/1177383005/diff/60001/ui/compositor/composito...
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/1177383005/diff/60001/ui/compositor/composito...
ui/compositor/compositor.cc:98:
(command_line->HasSwitch(switches::kDisableGpuVsync) ||
On 2015/06/29 02:20:27, sky wrote:
> nit: run git cl format

I already ran this script and there was no change.
I don`t know what is wrong.

https://codereview.chromium.org/1177383005/diff/60001/ui/gl/gl_switches.cc
File ui/gl/gl_switches.cc (right):

https://codereview.chromium.org/1177383005/diff/60001/ui/gl/gl_switches.cc#ne...
ui/gl/gl_switches.cc:35: //  for the decoupling "disable gpu vsync" and
"throttle frame production".
On 2015/06/29 02:20:27, sky wrote:
> nit: too many spaces here.

Done.

[sky, 2015-06-29 02:50:19]

Dana should really review all of this.

https://codereview.chromium.org/1177383005/diff/80001/ui/compositor/composito...
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/1177383005/diff/80001/ui/compositor/composito...
ui/compositor/compositor.cc:99:
command_line->HasSwitch(switches::kDisableDisplayVsync));
I would expect this line to indent one more space.

https://codereview.chromium.org/1177383005/diff/80001/ui/compositor/composito...
ui/compositor/compositor.cc:102:
command_line->HasSwitch(cc::switches::kDisableBeginFrameInterval));
And same here.

[Jimmy Jo, 2015-06-29 05:30:07]

Thanks. Sky.
I added space by your review.

PTAL again!!

https://codereview.chromium.org/1177383005/diff/80001/ui/compositor/composito...
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/1177383005/diff/80001/ui/compositor/composito...
ui/compositor/compositor.cc:99:
command_line->HasSwitch(switches::kDisableDisplayVsync));
On 2015/06/29 02:50:19, sky wrote:
> I would expect this line to indent one more space.

Done.

https://codereview.chromium.org/1177383005/diff/80001/ui/compositor/composito...
ui/compositor/compositor.cc:102:
command_line->HasSwitch(cc::switches::kDisableBeginFrameInterval));
On 2015/06/29 02:50:19, sky wrote:
> And same here.

Done.

[danakj, 2015-06-29 18:06:19]

https://codereview.chromium.org/1177383005/diff/60001/ui/compositor/composito...
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/1177383005/diff/60001/ui/compositor/composito...
ui/compositor/compositor.cc:98:
(command_line->HasSwitch(switches::kDisableGpuVsync) ||
On 2015/06/29 02:37:49, Jimmy Jo wrote:
> On 2015/06/29 02:20:27, sky wrote:
> > nit: run git cl format
> 
> I already ran this script and there was no change.
> I don`t know what is wrong.

File a bug against clang-format in b/ then please.

[danakj, 2015-06-29 18:07:39]

https://codereview.chromium.org/1177383005/diff/60001/ui/compositor/composito...
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/1177383005/diff/60001/ui/compositor/composito...
ui/compositor/compositor.cc:98:
(command_line->HasSwitch(switches::kDisableGpuVsync) ||
On 2015/06/29 18:06:19, danakj OOO back june 28 wrote:
> On 2015/06/29 02:37:49, Jimmy Jo wrote:
> > On 2015/06/29 02:20:27, sky wrote:
> > > nit: run git cl format
> > 
> > I already ran this script and there was no change.
> > I don`t know what is wrong.
> 
> File a bug against clang-format in b/ then please.

or, sorry, in crbug.com/, there's no template for clangformat bugs it looks
like. But you should cc djasper@google.com on it.

[Jimmy Jo, 2015-06-30 02:11:25]

jincheol.jo@navercorp.com changed reviewers:
+ piman@chromium.org

[Jimmy Jo, 2015-06-30 02:13:37]

On 2015/06/29 18:07:39, danakj OOO back june 28 wrote:
>
https://codereview.chromium.org/1177383005/diff/60001/ui/compositor/composito...
> File ui/compositor/compositor.cc (right):
> 
>
https://codereview.chromium.org/1177383005/diff/60001/ui/compositor/composito...
> ui/compositor/compositor.cc:98:
> (command_line->HasSwitch(switches::kDisableGpuVsync) ||
> On 2015/06/29 18:06:19, danakj OOO back june 28 wrote:
> > On 2015/06/29 02:37:49, Jimmy Jo wrote:
> > > On 2015/06/29 02:20:27, sky wrote:
> > > > nit: run git cl format
> > > 
> > > I already ran this script and there was no change.
> > > I don`t know what is wrong.
> > 
> > File a bug against clang-format in b/ then please.
> 
> or, sorry, in crbug.com/, there's no template for clangformat bugs it looks
> like. But you should cc mailto:djasper@google.com on it.

@Sky. I added space . PTAL again
@Dana, I will make a crbug for clangformat bug.
@Jam PTAL !!

[Jimmy Jo, 2015-06-30 02:14:00]

jincheol.jo@navercorp.com changed reviewers:
- piman@chromium.org

[Jimmy Jo, 2015-06-30 11:53:53]

@Sky, Dana : I am really sorry.  I confuse "git cl format cc" and "git cl
format".
So I ran "git cl format" and then it worked very well.

[Jimmy Jo, 2015-07-01 01:32:54]

On 2015/06/30 11:53:53, Jimmy Jo wrote:
> @Sky, Dana : I am really sorry.  I confuse "git cl format cc" and "git cl
> format".
> So I ran "git cl format" and then it worked very well.

ping...

[danakj, 2015-07-01 19:47:24]

https://codereview.chromium.org/1177383005/diff/120001/cc/base/switches.cc
File cc/base/switches.cc (right):

https://codereview.chromium.org/1177383005/diff/120001/cc/base/switches.cc#ne...
cc/base/switches.cc:106: // Disable frame production throttling.
Can you expand this comment? What is a beginframe interval? What throttling is
this disabling?

https://codereview.chromium.org/1177383005/diff/120001/cc/trees/layer_tree_se...
File cc/trees/layer_tree_settings.cc (right):

https://codereview.chromium.org/1177383005/diff/120001/cc/trees/layer_tree_se...
cc/trees/layer_tree_settings.cc:79: throttle_frame_production(true) {
can you make this name more closely match the flag?
wait_for_beginframe_interval?

https://codereview.chromium.org/1177383005/diff/120001/cc/trees/layer_tree_se...
cc/trees/layer_tree_settings.cc:97: 
remove whitespace

https://codereview.chromium.org/1177383005/diff/120001/cc/trees/layer_tree_se...
cc/trees/layer_tree_settings.cc:99: 
remove whitespace

https://codereview.chromium.org/1177383005/diff/120001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/1177383005/diff/120001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:1231:
switches::kDisableDisplayVsync,
sort this

https://codereview.chromium.org/1177383005/diff/120001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:1361:
cc::switches::kDisableBeginFrameInterval,
sort this

https://codereview.chromium.org/1177383005/diff/120001/content/renderer/gpu/r...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/1177383005/diff/120001/content/renderer/gpu/r...
content/renderer/gpu/render_widget_compositor.cc:227:
settings.renderer_settings.disable_display_vsync =
wow is this confusing.

we have 2 flags that you can set either one to do one thing.
we have 2 overlapping/different flags that must both be off to do another thing.

brian, is this really the best we can do?

[danakj, 2015-07-01 19:52:24]

You need to add switches you want to use in the renderer process to
chrome/browser/chromeos/login/chrome_restart_request.cc as well

[brianderson, 2015-07-01 20:51:57]

https://codereview.chromium.org/1177383005/diff/120001/content/renderer/gpu/r...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/1177383005/diff/120001/content/renderer/gpu/r...
content/renderer/gpu/render_widget_compositor.cc:227:
settings.renderer_settings.disable_display_vsync =
On 2015/07/01 19:47:23, danakj OOO back july 6 wrote:
> wow is this confusing.
> 
> we have 2 flags that you can set either one to do one thing.
> we have 2 overlapping/different flags that must both be off to do another
thing.
> 
> brian, is this really the best we can do?

From a code readability perspective, I agree it's pretty bad. But from a user's
perspective I think it makes sense.

Most users just want to use --disable-gpu-vsync to disable both (like they can
at ToT today). Very few users will care about disabling one or the other
individually.

Would it help code readability if "throttle_frame_production" became
"disable_begin_frame_interval" so we don't have the double negation?

[Jimmy Jo, 2015-07-01 23:08:40]

On 2015/07/01 19:52:24, danakj OOO back july 6 wrote:
> You need to add switches you want to use in the renderer process to
> chrome/browser/chromeos/login/chrome_restart_request.cc as well
Thanks dana.
I wonder why there is no kDisableGpuVsync.

[Jimmy Jo, 2015-07-02 08:38:48]

On 2015/07/01 23:08:40, Jimmy Jo wrote:
> On 2015/07/01 19:52:24, danakj OOO back july 6 wrote:
> > You need to add switches you want to use in the renderer process to
> > chrome/browser/chromeos/login/chrome_restart_request.cc as well
> Thanks dana.
> I wonder why there is no kDisableGpuVsync.

Should kDisableGpuVsync with both flags be added in there?

[danakj, 2015-07-03 00:06:49]

what if we have 1 flag, like--disable-vsync, and you can optionally pass it
an argument, like --disable-vsync=beginframe or something, if you only want
to disable one part.
or --disable-vsync=gpu etc. and the default is "all?"

On Thu, Jul 2, 2015 at 1:38 AM, <jincheol.jo@navercorp.com> wrote:

> On 2015/07/01 23:08:40, Jimmy Jo wrote:
>
>> On 2015/07/01 19:52:24, danakj OOO back july 6 wrote:
>> > You need to add switches you want to use in the renderer process to
>> > chrome/browser/chromeos/login/chrome_restart_request.cc as well
>> Thanks dana.
>> I wonder why there is no kDisableGpuVsync.
>>
>
> Should kDisableGpuVsync with both flags be added in there?
>
> https://codereview.chromium.org/1177383005/
>

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

[danakj, 2015-07-03 00:07:31]

On Thu, Jul 2, 2015 at 1:38 AM, <jincheol.jo@navercorp.com> wrote:

> On 2015/07/01 23:08:40, Jimmy Jo wrote:
>
>> On 2015/07/01 19:52:24, danakj OOO back july 6 wrote:
>> > You need to add switches you want to use in the renderer process to
>> > chrome/browser/chromeos/login/chrome_restart_request.cc as well
>> Thanks dana.
>> I wonder why there is no kDisableGpuVsync.
>>
>
> Should kDisableGpuVsync with both flags be added in there?
>

All flags passed to the renderer should be there. It's a whitelist for
flags when running in incognito. So unless you want different behaviour in
incognito mode (we don't), they should be htere.


>
> https://codereview.chromium.org/1177383005/
>

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

[brianderson, 2015-07-06 23:47:38]

On 2015/07/03 00:06:49, danakj wrote:
> what if we have 1 flag, like--disable-vsync, and you can optionally pass it
> an argument, like --disable-vsync=beginframe or something, if you only want
> to disable one part.
> or --disable-vsync=gpu etc. and the default is "all?"

That sounds like an excellent suggestion. Jimmy, wdyt?

(Sorry to keep going back and forth like this btw.)

> 
> On Thu, Jul 2, 2015 at 1:38 AM, <mailto:jincheol.jo@navercorp.com> wrote:
> 
> > On 2015/07/01 23:08:40, Jimmy Jo wrote:
> >
> >> On 2015/07/01 19:52:24, danakj OOO back july 6 wrote:
> >> > You need to add switches you want to use in the renderer process to
> >> > chrome/browser/chromeos/login/chrome_restart_request.cc as well
> >> Thanks dana.
> >> I wonder why there is no kDisableGpuVsync.
> >>
> >
> > Should kDisableGpuVsync with both flags be added in there?
> >
> > https://codereview.chromium.org/1177383005/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[Jimmy Jo, 2015-07-07 02:59:11]

On 2015/07/06 23:47:38, brianderson wrote:
> On 2015/07/03 00:06:49, danakj wrote:
> > what if we have 1 flag, like--disable-vsync, and you can optionally pass it
> > an argument, like --disable-vsync=beginframe or something, if you only want
> > to disable one part.
> > or --disable-vsync=gpu etc. and the default is "all?"
> 
> That sounds like an excellent suggestion. Jimmy, wdyt?
> 
> (Sorry to keep going back and forth like this btw.)
> 
No problem. Brian.
I will give a shot again.
Thanks Brian and Dana.

[jam, 2015-07-08 19:34:14]

content lgtm

[Jimmy Jo, 2015-07-12 07:39:11]

On 2015/07/03 00:06:49, danakj wrote:
> what if we have 1 flag, like--disable-vsync, and you can optionally pass it
> an argument, like --disable-vsync=beginframe or something, if you only want
> to disable one part.
> or --disable-vsync=gpu etc. and the default is "all?"
> 

PTAL!!
I added some options "gpu" and "beginframe" and
removed new two flags --disable-display-vsync and --disable-beginframe-interval.
Also default is empty string.
Because previous cmd string is same and it seems better to me.
We can use  --display-gpu-vsync continue.

[Jimmy Jo, 2015-07-12 11:24:35]

Patchset #7 (id:160001) has been deleted

[Jimmy Jo, 2015-07-14 00:53:44]

On 2015/07/12 07:39:11, Jimmy Jo wrote:
> On 2015/07/03 00:06:49, danakj wrote:
> > what if we have 1 flag, like--disable-vsync, and you can optionally pass it
> > an argument, like --disable-vsync=beginframe or something, if you only want
> > to disable one part.
> > or --disable-vsync=gpu etc. and the default is "all?"
> > 
> 
> PTAL!!
> I added some options "gpu" and "beginframe" and
> removed new two flags --disable-display-vsync and
--disable-beginframe-interval.
> Also default is empty string.
> Because previous cmd string is same and it seems better to me.
> We can use  --display-gpu-vsync continue.

ping!!

[brianderson, 2015-07-14 01:48:26]

A couple nits, but still lgtm.

https://codereview.chromium.org/1177383005/diff/180001/ui/compositor/composit...
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/1177383005/diff/180001/ui/compositor/composit...
ui/compositor/compositor.cc:99: settings.renderer_settings.disable_display_vsync
=
Might be easier to read this way:

if (displayVsyncStr == "gpu") {
  settings.renderer_settings.disable_display_vsync = true;
} else if (displayVsyncStr == "beginframe") {
  settings.wait_for_beginframe_interval = false;
} else {
  settings.renderer_settings.disable_display_vsync = true;
  settings.wait_for_beginframe_interval = false;
}

https://codereview.chromium.org/1177383005/diff/180001/ui/gl/gl_switches.cc
File ui/gl/gl_switches.cc (right):

https://codereview.chromium.org/1177383005/diff/180001/ui/gl/gl_switches.cc#n...
ui/gl/gl_switches.cc:32: // We can select below options:
// We can select from the options below:

[danakj, 2015-07-14 19:43:42]

LGTM % brianderson.

thanks

https://codereview.chromium.org/1177383005/diff/180001/content/renderer/gpu/r...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/1177383005/diff/180001/content/renderer/gpu/r...
content/renderer/gpu/render_widget_compositor.cc:231: std::string
displayVsyncStr =
display_vsync_string

https://codereview.chromium.org/1177383005/diff/180001/ui/compositor/composit...
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/1177383005/diff/180001/ui/compositor/composit...
ui/compositor/compositor.cc:97: std::string displayVsyncStr =
display_vsync_string

[Jimmy Jo, 2015-07-15 00:47:27]

Thanks Brian and Dana.
I modified some nits you mentioned.

Should I wait SKY`s stamp?

https://codereview.chromium.org/1177383005/diff/180001/content/renderer/gpu/r...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/1177383005/diff/180001/content/renderer/gpu/r...
content/renderer/gpu/render_widget_compositor.cc:231: std::string
displayVsyncStr =
On 2015/07/14 19:43:42, danakj wrote:
> display_vsync_string

Done.

https://codereview.chromium.org/1177383005/diff/180001/ui/compositor/composit...
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/1177383005/diff/180001/ui/compositor/composit...
ui/compositor/compositor.cc:97: std::string displayVsyncStr =
On 2015/07/14 19:43:42, danakj wrote:
> display_vsync_string

Done.

https://codereview.chromium.org/1177383005/diff/180001/ui/compositor/composit...
ui/compositor/compositor.cc:99: settings.renderer_settings.disable_display_vsync
=
On 2015/07/14 01:48:25, brianderson wrote:
> Might be easier to read this way:
> 
> if (displayVsyncStr == "gpu") {
>   settings.renderer_settings.disable_display_vsync = true;
> } else if (displayVsyncStr == "beginframe") {
>   settings.wait_for_beginframe_interval = false;
> } else {
>   settings.renderer_settings.disable_display_vsync = true;
>   settings.wait_for_beginframe_interval = false;
> }

Done.

https://codereview.chromium.org/1177383005/diff/180001/ui/gl/gl_switches.cc
File ui/gl/gl_switches.cc (right):

https://codereview.chromium.org/1177383005/diff/180001/ui/gl/gl_switches.cc#n...
ui/gl/gl_switches.cc:32: // We can select below options:
On 2015/07/14 01:48:25, brianderson wrote:
> // We can select from the options below:

Done.

[brianderson, 2015-07-15 01:00:12]

On 2015/07/15 00:47:27, Jimmy Jo wrote:
> Thanks Brian and Dana.
> I modified some nits you mentioned.
> 
> Should I wait SKY`s stamp?

Latest patch looks good to land - go ahead and hit the commit box.

The changes since @jam's review are trivial, so I don't think we need to wait.

Thanks for working this out!

> 
>
https://codereview.chromium.org/1177383005/diff/180001/content/renderer/gpu/r...
> File content/renderer/gpu/render_widget_compositor.cc (right):
> 
>
https://codereview.chromium.org/1177383005/diff/180001/content/renderer/gpu/r...
> content/renderer/gpu/render_widget_compositor.cc:231: std::string
> displayVsyncStr =
> On 2015/07/14 19:43:42, danakj wrote:
> > display_vsync_string
> 
> Done.
> 
>
https://codereview.chromium.org/1177383005/diff/180001/ui/compositor/composit...
> File ui/compositor/compositor.cc (right):
> 
>
https://codereview.chromium.org/1177383005/diff/180001/ui/compositor/composit...
> ui/compositor/compositor.cc:97: std::string displayVsyncStr =
> On 2015/07/14 19:43:42, danakj wrote:
> > display_vsync_string
> 
> Done.
> 
>
https://codereview.chromium.org/1177383005/diff/180001/ui/compositor/composit...
> ui/compositor/compositor.cc:99:
settings.renderer_settings.disable_display_vsync
> =
> On 2015/07/14 01:48:25, brianderson wrote:
> > Might be easier to read this way:
> > 
> > if (displayVsyncStr == "gpu") {
> >   settings.renderer_settings.disable_display_vsync = true;
> > } else if (displayVsyncStr == "beginframe") {
> >   settings.wait_for_beginframe_interval = false;
> > } else {
> >   settings.renderer_settings.disable_display_vsync = true;
> >   settings.wait_for_beginframe_interval = false;
> > }
> 
> Done.
> 
> https://codereview.chromium.org/1177383005/diff/180001/ui/gl/gl_switches.cc
> File ui/gl/gl_switches.cc (right):
> 
>
https://codereview.chromium.org/1177383005/diff/180001/ui/gl/gl_switches.cc#n...
> ui/gl/gl_switches.cc:32: // We can select below options:
> On 2015/07/14 01:48:25, brianderson wrote:
> > // We can select from the options below:
> 
> Done.

[brianderson, 2015-07-15 01:00:38]

lgtm, btw.

[Jimmy Jo, 2015-07-15 01:03:07]

The CQ bit was checked by jincheol.jo@navercorp.com

[Jimmy Jo, 2015-07-15 01:03:07]

The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org,
danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/1177383005/#ps200001
(title: "hope to be final patch")

[commit-bot: I haz the power, 2015-07-15 01:03:34]

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

[commit-bot: I haz the power, 2015-07-15 02:06:57]

Committed patchset #8 (id:200001)

[commit-bot: I haz the power, 2015-07-15 02:08:05]

Patchset 8 (id:??) landed as
https://crrev.com/5899bd55e2174cb636c77d45d64827b76bda31b7
Cr-Commit-Position: refs/heads/master@{#338800}

[Jimmy Jo, 2015-07-15 02:15:18]

On 2015/07/15 02:08:05, commit-bot: I haz the power wrote:
> Patchset 8 (id:??) landed as
> https://crrev.com/5899bd55e2174cb636c77d45d64827b76bda31b7
> Cr-Commit-Position: refs/heads/master@{#338800}

Brian, Dana and Chrome reviewers. Thanks for detailed review.
I learned many points while we were reviewing this patches.
Thank you again.
Have a nice day. :-)