cc: Trigger GPU rasterization based on a per-layer hint

cc: Trigger GPU rasterization based on a per-layer hint

This adds a GPU rasterization hint to PictureLayer and
PictureLayerImpl.

This also replaces the existing --enable-gpu-rasterization
and --disable-gpu-rasterization flags with a new
--rasterization-site flag that can be given values
'cpu', 'gpu', and 'hybrid'.

When running with "--rasterization-site=hybrid", only
layers with a GPU rasterization hint are rasterized on
the GPU.

BUG=329722

[ajuma, 2014-03-04 21:11:25]

The Blink-side CL that actually sets GPU rasterization hints (based on
will-change) is http://crrev.com/183103007.

[ernstm, 2014-03-04 21:53:51]

We should use the opportunity to clean-up the naming of our flags with this
patch. For example, we now have an enum GpuRasterizationState that has
SoftwareRasterization as an entry. And we have an entry HybridGpuRasterization
that corresponds to the --enable-gpu-rasterization command line flag, etc. I
think that is a bit confusing.

I also don't think that software rasterization is a good term in general. It's
all software that is executed on hardware. I'd prefer to distinguish between CPU
and GPU rasterization.

Maybe we can have three modes
  CpuRasterization
  HybridRasterization
  GpuRasterization
and have three corresponding command line flags
  --cpu-rasterization
  --hybrid-rasterization
  --gpu-rasterization
that set the mode. --cpu-rasterization is the default, but that might change at
some point in the future.

What do you think?

[ajuma, 2014-03-04 22:05:14]

On 2014/03/04 21:53:51, ernstm wrote:
> We should use the opportunity to clean-up the naming of our flags with this
> patch. For example, we now have an enum GpuRasterizationState that has
> SoftwareRasterization as an entry. And we have an entry HybridGpuRasterization
> that corresponds to the --enable-gpu-rasterization command line flag, etc. I
> think that is a bit confusing.
> 
> I also don't think that software rasterization is a good term in general. It's
> all software that is executed on hardware. I'd prefer to distinguish between
CPU
> and GPU rasterization.
> 
> Maybe we can have three modes
>   CpuRasterization
>   HybridRasterization
>   GpuRasterization
> and have three corresponding command line flags
>   --cpu-rasterization
>   --hybrid-rasterization
>   --gpu-rasterization
> that set the mode. --cpu-rasterization is the default, but that might change
at
> some point in the future.
> 
> What do you think?

I like this. One concern is that it isn't obvious which flag wins when, say,
both cpu and gpu rasterization command line flags are specified (unlike when we
have --enable-gpu-rasterization and --disable-gpu-rasterization, where --disable
wins by convention). Presumably we'd still want cpu to take precedence over both
flags, and we'd want gpu to take precedence over hybrid.

[vmpstr, 2014-03-04 22:07:42]

On 2014/03/04 22:05:14, ajuma wrote:
> On 2014/03/04 21:53:51, ernstm wrote:
> > We should use the opportunity to clean-up the naming of our flags with this
> > patch. For example, we now have an enum GpuRasterizationState that has
> > SoftwareRasterization as an entry. And we have an entry
HybridGpuRasterization
> > that corresponds to the --enable-gpu-rasterization command line flag, etc. I
> > think that is a bit confusing.
> > 
> > I also don't think that software rasterization is a good term in general.
It's
> > all software that is executed on hardware. I'd prefer to distinguish between
> CPU
> > and GPU rasterization.
> > 
> > Maybe we can have three modes
> >   CpuRasterization
> >   HybridRasterization
> >   GpuRasterization
> > and have three corresponding command line flags
> >   --cpu-rasterization
> >   --hybrid-rasterization
> >   --gpu-rasterization
> > that set the mode. --cpu-rasterization is the default, but that might change
> at
> > some point in the future.
> > 
> > What do you think?
> 
> I like this. One concern is that it isn't obvious which flag wins when, say,
> both cpu and gpu rasterization command line flags are specified (unlike when
we
> have --enable-gpu-rasterization and --disable-gpu-rasterization, where
--disable
> wins by convention). Presumably we'd still want cpu to take precedence over
both
> flags, and we'd want gpu to take precedence over hybrid.

(drive-by)

I don't know if there's really any precedent, but is it possible to have
something like --rasterization_mode="cpu" or ="hybrid" or ="gpu"

I don't really know if we can or do have string arguments

[alokp, 2014-03-04 22:16:45]

> (drive-by)
> 
> I don't know if there's really any precedent, but is it possible to have
> something like --rasterization_mode="cpu" or ="hybrid" or ="gpu"
> 
> I don't really know if we can or do have string arguments

+1 if we support string arguments. I would prefer --rasterization-site or
something like that though. Mode is too generic.

[jamesr, 2014-03-04 22:20:59]

We can (and do) have switches with different string values. 
witches::kTabCaptureUpscaleQuality is one example that's wired up in
about:flags.

[ajuma, 2014-03-05 00:26:13]

On 2014/03/04 22:16:45, Alok Priyadarshi wrote:
> > (drive-by)
> > 
> > I don't know if there's really any precedent, but is it possible to have
> > something like --rasterization_mode="cpu" or ="hybrid" or ="gpu"
> > 
> > I don't really know if we can or do have string arguments
> 
> +1 if we support string arguments. I would prefer --rasterization-site or
> something like that though. Mode is too generic.

--rasterization-site=cpu or =hybrid or =gpu seems reasonable to me. Any
objections?

[ernstm, 2014-03-05 00:36:00]

On 2014/03/05 00:26:13, ajuma wrote:
> On 2014/03/04 22:16:45, Alok Priyadarshi wrote:
> > > (drive-by)
> > > 
> > > I don't know if there's really any precedent, but is it possible to have
> > > something like --rasterization_mode="cpu" or ="hybrid" or ="gpu"
> > > 
> > > I don't really know if we can or do have string arguments
> > 
> > +1 if we support string arguments. I would prefer --rasterization-site or
> > something like that though. Mode is too generic.
> 
> --rasterization-site=cpu or =hybrid or =gpu seems reasonable to me. Any
> objections?

I like it. You should check that telemetry supports setting string arguments.

[ajuma, 2014-03-05 19:41:48]

PTAL. This now uses "--rasterization-site", with values 'cpu', 'gpu' and
'hybrid'.

> You should check that telemetry supports setting string arguments.

I verified locally (using gpu_rasterization.py) that string arguments specified
in AppendExtraBrowserArgs do get correctly set.

[Vangelis Kokkevis, 2014-03-05 19:52:07]

https://codereview.chromium.org/181063022/diff/90001/content/renderer/gpu/ren...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/181063022/diff/90001/content/renderer/gpu/ren...
content/renderer/gpu/render_widget_compositor.cc:130: if
(cc::switches::IsGpuRasterizationEnabled())
Should we also check if impl-side-painting is enabled and if not revert to
CpuRasterization, to avoid confusion from incompatible flags?

[ajuma, 2014-03-05 20:37:02]

https://codereview.chromium.org/181063022/diff/90001/content/renderer/gpu/ren...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/181063022/diff/90001/content/renderer/gpu/ren...
content/renderer/gpu/render_widget_compositor.cc:130: if
(cc::switches::IsGpuRasterizationEnabled())
On 2014/03/05 19:52:08, Vangelis Kokkevis wrote:
> Should we also check if impl-side-painting is enabled and if not revert to
> CpuRasterization, to avoid confusion from incompatible flags? 

Good idea. I've added this check to IsGpuRasterizationEnabled and to
IsHybridRasterizationEnabled.

[ajuma, 2014-03-06 15:27:13]

enne, PTAL.

[enne (OOO), 2014-03-06 21:28:45]

Looks good in general.  A few small comments:

https://codereview.chromium.org/181063022/diff/60001/cc/base/switches.h
File cc/base/switches.h (right):

https://codereview.chromium.org/181063022/diff/60001/cc/base/switches.h#newco...
cc/base/switches.h:29: CC_EXPORT extern const char
kEnableUniversalGPURasterization[];
Sorry to airdrop in and bikeshed, but kEnableGPURasterization and
kEnableUniversalGPURasterization sound like they're doing different things.

Can this be kEnableGPURasterization and kForceGPURasterization, to make it
clearer that one is a "may" and the other is a "must", but they affect the same
feature?

https://codereview.chromium.org/181063022/diff/60001/cc/layers/picture_layer_...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/181063022/diff/60001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:874: if (tilings_)
When can this be NULL?

[ajuma, 2014-03-06 21:44:27]

https://codereview.chromium.org/181063022/diff/60001/cc/base/switches.h
File cc/base/switches.h (right):

https://codereview.chromium.org/181063022/diff/60001/cc/base/switches.h#newco...
cc/base/switches.h:29: CC_EXPORT extern const char
kEnableUniversalGPURasterization[];
On 2014/03/06 21:28:46, enne wrote:
> Sorry to airdrop in and bikeshed, but kEnableGPURasterization and
> kEnableUniversalGPURasterization sound like they're doing different things.
> 
> Can this be kEnableGPURasterization and kForceGPURasterization, to make it
> clearer that one is a "may" and the other is a "must", but they affect the
same
> feature?

Please see the latest patch set, where these have both been removed and instead
we have a single kRasterizationSite flag.

https://codereview.chromium.org/181063022/diff/60001/cc/layers/picture_layer_...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/181063022/diff/60001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:874: if (tilings_)
On 2014/03/06 21:28:46, enne wrote:
> When can this be NULL?

One situation is during the initial commit. During this initial PushProperties,
if the gpu rasterization hint gets set, we wind up here with tilings_ == NULL.

[enne (OOO), 2014-03-06 21:54:04]

lgtm

[enne (OOO), 2014-03-06 21:54:07]

lgtm

[ajuma, 2014-03-06 21:57:18]

+piman for content/ and chrome/browser/chromeos/login OWNERS.

[piman, 2014-03-06 22:13:52]

https://codereview.chromium.org/181063022/diff/150001/cc/base/switches.cc
File cc/base/switches.cc (right):

https://codereview.chromium.org/181063022/diff/150001/cc/base/switches.cc#new...
cc/base/switches.cc:178: return false;
I think all these flags and helper functions should be in content/

Having them in cc/ suggests they apply to both the browser compositor and the
renderer compositor, whereas they only apply to the latter.

They're also not accurate since gpu rasterization is only enabled if
--enable-bleeding-edge-rendering-fast-paths is set as well.

The temptation is high to use them inside of cc/ and cause issues because of
mismatches.

My suggestion is to move the flags to content_flags.cc, and the helper functions
to somewhere on content/renderer (e.g. maybe bools on RenderThreadImpl), and
have them accurately reflect the full policy..

[ernstm, 2014-03-06 22:47:25]

https://codereview.chromium.org/181063022/diff/150001/cc/base/switches.cc
File cc/base/switches.cc (right):

https://codereview.chromium.org/181063022/diff/150001/cc/base/switches.cc#new...
cc/base/switches.cc:178: return false;
> They're also not accurate since gpu rasterization is only enabled if
> --enable-bleeding-edge-rendering-fast-paths is set as well

GPU rasterization can be enabled without setting
--enable-bleeding-edge-rendering-fast-paths. As I understand it,
--enable-bleeding-edge-rendering-fast-paths is just a shortcut to enable a
collection of features with a single flag.

[ernstm, 2014-03-06 22:57:50]

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

https://codereview.chromium.org/181063022/diff/150001/content/renderer/gpu/re...
content/renderer/gpu/render_widget_compositor.cc:131:
cmd->HasSwitch(switches::kEnableBleedingEdgeRenderingFastPaths))
--enable-bleeding-edge-rendering-fast-paths should enable hybrid rasterization
not gpu rasterization.

[piman, 2014-03-06 22:58:59]

On 2014/03/06 22:47:25, ernstm wrote:
> https://codereview.chromium.org/181063022/diff/150001/cc/base/switches.cc
> File cc/base/switches.cc (right):
> 
>
https://codereview.chromium.org/181063022/diff/150001/cc/base/switches.cc#new...
> cc/base/switches.cc:178: return false;
> > They're also not accurate since gpu rasterization is only enabled if
> > --enable-bleeding-edge-rendering-fast-paths is set as well
> 
> GPU rasterization can be enabled without setting
> --enable-bleeding-edge-rendering-fast-paths. As I understand it,
> --enable-bleeding-edge-rendering-fast-paths is just a shortcut to enable a
> collection of features with a single flag.

Oh, yeah, it's a || not a &&. Regardless, the point still stands :)

[ajuma, 2014-03-06 23:03:15]

On 2014/03/06 22:57:50, ernstm wrote:
>
https://codereview.chromium.org/181063022/diff/150001/content/renderer/gpu/re...
> File content/renderer/gpu/render_widget_compositor.cc (right):
> 
>
https://codereview.chromium.org/181063022/diff/150001/content/renderer/gpu/re...
> content/renderer/gpu/render_widget_compositor.cc:131:
> cmd->HasSwitch(switches::kEnableBleedingEdgeRenderingFastPaths))
> --enable-bleeding-edge-rendering-fast-paths should enable hybrid rasterization
> not gpu rasterization.

I was planning on switching this to hybrid after the Blink CL for setting a
rasterization hint on WebLayers based on will-change lands (it can't land till
after this CL lands), so that we don't have an intermediate state where
--enable-bleeding-edge-rendering-fast-paths doesn't trigger any gpu
rasterization at all.

[ernstm, 2014-03-06 23:15:28]

> I was planning on switching this to hybrid after the Blink CL for setting a
> rasterization hint on WebLayers based on will-change lands (it can't land till
> after this CL lands), so that we don't have an intermediate state where
> --enable-bleeding-edge-rendering-fast-paths doesn't trigger any gpu
> rasterization at all.

Sounds good.

[ernstm, 2014-03-06 23:17:22]

On 2014/03/06 23:15:28, ernstm wrote:
> > I was planning on switching this to hybrid after the Blink CL for setting a
> > rasterization hint on WebLayers based on will-change lands (it can't land
till
> > after this CL lands), so that we don't have an intermediate state where
> > --enable-bleeding-edge-rendering-fast-paths doesn't trigger any gpu
> > rasterization at all.
> 
> Sounds good.

and LGTM % piman's comments.

[ajuma, 2014-03-07 00:26:40]

On 2014/03/06 22:13:52, piman wrote:
> https://codereview.chromium.org/181063022/diff/150001/cc/base/switches.cc
> File cc/base/switches.cc (right):
> 
>
https://codereview.chromium.org/181063022/diff/150001/cc/base/switches.cc#new...
> cc/base/switches.cc:178: return false;
> I think all these flags and helper functions should be in content/
> 
> Having them in cc/ suggests they apply to both the browser compositor and the
> renderer compositor, whereas they only apply to the latter.
> 
> They're also not accurate since gpu rasterization is only enabled if
> --enable-bleeding-edge-rendering-fast-paths is set as well.
> 
> The temptation is high to use them inside of cc/ and cause issues because of
> mismatches.
> 
> My suggestion is to move the flags to content_flags.cc, and the helper
functions
> to somewhere on content/renderer (e.g. maybe bools on RenderThreadImpl), and
> have them accurately reflect the full policy..

The IsImplSidePaintingEnabled() helper function is also accessed outside
content/renderer (it's accessed in content/browser and in
webkit/renderer/compositor_bindings). Suggestions for an appropriate place to
put it (and the rest of the helpers, assuming they should all end up in the same
place)?

[piman, 2014-03-07 00:46:24]

On 2014/03/07 00:26:40, ajuma wrote:
> On 2014/03/06 22:13:52, piman wrote:
> > https://codereview.chromium.org/181063022/diff/150001/cc/base/switches.cc
> > File cc/base/switches.cc (right):
> > 
> >
>
https://codereview.chromium.org/181063022/diff/150001/cc/base/switches.cc#new...
> > cc/base/switches.cc:178: return false;
> > I think all these flags and helper functions should be in content/
> > 
> > Having them in cc/ suggests they apply to both the browser compositor and
the
> > renderer compositor, whereas they only apply to the latter.
> > 
> > They're also not accurate since gpu rasterization is only enabled if
> > --enable-bleeding-edge-rendering-fast-paths is set as well.
> > 
> > The temptation is high to use them inside of cc/ and cause issues because of
> > mismatches.
> > 
> > My suggestion is to move the flags to content_flags.cc, and the helper
> functions
> > to somewhere on content/renderer (e.g. maybe bools on RenderThreadImpl), and
> > have them accurately reflect the full policy..
> 
> The IsImplSidePaintingEnabled() helper function is also accessed outside
> content/renderer (it's accessed in content/browser and in
> webkit/renderer/compositor_bindings). Suggestions for an appropriate place to
> put it (and the rest of the helpers, assuming they should all end up in the
same
> place)?

So... the long answer is...
1- compositor_bindings shouldn't use IsImplSidePaintingEnabled(), instead
content/ should set a global flag for compositor_bindings that tells it what
type of layer to create, based on the value of IsImplSidePaintingEnabled().
2- IsImplSidePaintingEnabled() should probably be renamed
IsImplSidePaintingEnabledForRenderer() and live in content/common if it's also
used by the browser. Or better yet, it should be in content/browser
(content/browser/gpu/compositor_util.cc), and for the renderer have a bool on
RenderThreadImpl
3- the other flags should be a bool on RenderThreadImpl (if not in
WebPreferences, actually)
99- we should stop using flags to communicate between the browser and the
renderer but that's definitely for another CL.

[ajuma, 2014-03-07 19:23:53]

PTAL.

> So... the long answer is...
> 1- compositor_bindings shouldn't use IsImplSidePaintingEnabled(), instead
> content/ should set a global flag for compositor_bindings that tells it what
> type of layer to create, based on the value of IsImplSidePaintingEnabled().
> 2- IsImplSidePaintingEnabled() should probably be renamed
> IsImplSidePaintingEnabledForRenderer() and live in content/common if it's also
> used by the browser. Or better yet, it should be in content/browser
> (content/browser/gpu/compositor_util.cc), and for the renderer have a bool on
> RenderThreadImpl
> 3- the other flags should be a bool on RenderThreadImpl (if not in
> WebPreferences, actually)

I've moved IsImplSidePaintingEnabled to content/browser/gpu/compositor_util.cc,
and I've added a bool to RenderThreadImpl. For the other flags, I've also added
bools to RenderThreadImpl. For compositor_bindings, I've added a static
WebLayerImpl::SetImplSidePaintingEnabled that gets called by
RenderWidgetCompositor.

[piman, 2014-03-07 19:41:03]

LGTM+a few nits. Thanks for the cleanup!

https://codereview.chromium.org/181063022/diff/220001/content/browser/gpu/com...
File content/browser/gpu/compositor_util.h (right):

https://codereview.chromium.org/181063022/diff/220001/content/browser/gpu/com...
content/browser/gpu/compositor_util.h:22: // Returns ture if impl-side painting
is on (via flags, or platform default).
nit: s/ture/true/

Can you add a note saying this only affects the renderer?

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

https://codereview.chromium.org/181063022/diff/220001/content/renderer/gpu/re...
content/renderer/gpu/render_widget_compositor.cc:131:
webkit::WebLayerImpl::SetImplSidePaintingEnabled(settings.impl_side_painting);
nit: I'd rather do that in RendetThreadImpl::Init.
This kinda suggests we could change our mind between different instantiations of
the compositor, but we really can't.

https://codereview.chromium.org/181063022/diff/220001/webkit/renderer/composi...
File webkit/renderer/compositor_bindings/web_layer_impl.cc (right):

https://codereview.chromium.org/181063022/diff/220001/webkit/renderer/composi...
webkit/renderer/compositor_bindings/web_layer_impl.cc:51:
LAZY_INSTANCE_INITIALIZER;
nit: at this point, a simple bool might suffice.

[ajuma, 2014-03-07 20:21:10]

enne, does this still look good to you?

https://codereview.chromium.org/181063022/diff/220001/content/browser/gpu/com...
File content/browser/gpu/compositor_util.h (right):

https://codereview.chromium.org/181063022/diff/220001/content/browser/gpu/com...
content/browser/gpu/compositor_util.h:22: // Returns ture if impl-side painting
is on (via flags, or platform default).
On 2014/03/07 19:41:04, piman wrote:
> nit: s/ture/true/
> 
> Can you add a note saying this only affects the renderer?

Done.

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

https://codereview.chromium.org/181063022/diff/220001/content/renderer/gpu/re...
content/renderer/gpu/render_widget_compositor.cc:131:
webkit::WebLayerImpl::SetImplSidePaintingEnabled(settings.impl_side_painting);
On 2014/03/07 19:41:04, piman wrote:
> nit: I'd rather do that in RendetThreadImpl::Init.
> This kinda suggests we could change our mind between different instantiations
of
> the compositor, but we really can't.

Done.

https://codereview.chromium.org/181063022/diff/220001/webkit/renderer/composi...
File webkit/renderer/compositor_bindings/web_layer_impl.cc (right):

https://codereview.chromium.org/181063022/diff/220001/webkit/renderer/composi...
webkit/renderer/compositor_bindings/web_layer_impl.cc:51:
LAZY_INSTANCE_INITIALIZER;
On 2014/03/07 19:41:04, piman wrote:
> nit: at this point, a simple bool might suffice.

Done.

[nduca, 2014-03-07 23:17:11]

lgtm

i'd like the flag changes to be separated out from plumbing please.

i'd also like to have the flag changes done with urgency as it helps deal with
the folks needing to use our bleeding edge in b43.

I personally question the correctness of --rasterization-site=xyz approach ...
i'd prefer we use the same approach we've done before:
--disable-gpu-rasterization
--enable-gpu-rasterzation
--force-gpu-rasterization

why aren't we doing that?

[nduca, 2014-03-07 23:17:31]

er, not lgtm

[ajuma, 2014-03-07 23:26:54]

On 2014/03/07 23:17:11, nduca wrote:
> lgtm
> 
> i'd like the flag changes to be separated out from plumbing please.
> 
> i'd also like to have the flag changes done with urgency as it helps deal with
> the folks needing to use our bleeding edge in b43.
> 
> I personally question the correctness of --rasterization-site=xyz approach ...
> i'd prefer we use the same approach we've done before:
> --disable-gpu-rasterization
> --enable-gpu-rasterzation
> --force-gpu-rasterization
> 
> why aren't we doing that?

This was roughly the approach of the initial patch set (except that
--force-gpu-rasterization was --enable-universal-gpu-rasterization), but see the
discussion in the earlier comments (2 thru 5) for how we got to
rasterization-site.  The tldr is that there was a desire to use the terms "cpu",
"gpu", and "hybrid", and doing this with three flags leads to ambiguity when
more than one flag is specified.

I'm happy to switch back if that's what's preferred.

[ajuma, 2014-03-07 23:29:31]

On 2014/03/07 23:26:54, ajuma wrote:
> On 2014/03/07 23:17:11, nduca wrote:
> > lgtm
> > 
> > i'd like the flag changes to be separated out from plumbing please.
> > 
> > i'd also like to have the flag changes done with urgency as it helps deal
with
> > the folks needing to use our bleeding edge in b43.
> > 
> > I personally question the correctness of --rasterization-site=xyz approach
...
> > i'd prefer we use the same approach we've done before:
> > --disable-gpu-rasterization
> > --enable-gpu-rasterzation
> > --force-gpu-rasterization
> > 
> > why aren't we doing that?
> 
> This was roughly the approach of the initial patch set (except that
> --force-gpu-rasterization was --enable-universal-gpu-rasterization), but see
the
> discussion in the earlier comments (2 thru 5) for how we got to
> rasterization-site.  The tldr is that there was a desire to use the terms
"cpu",
> "gpu", and "hybrid", and doing this with three flags leads to ambiguity when
> more than one flag is specified.
> 
> I'm happy to switch back if that's what's preferred.

(Also note that the flag changes provide no additional functionality without the
plumbing changes, so it's not clear what we win by prioritizing them over the
plumbing.)

[ajuma, 2014-03-08 01:18:58]

Given Nat's comments, I'm going to close this CL and put the flag changes and
the plumbing in separate CLs.

The CL for flag changes is: https://codereview.chromium.org/189883009/