Add flag to disable impl-side painting

Add flag to disable impl-side painting

To check whether impl-side painting is enabled, two command line flags are checked, with the disabling flag taking precedence over the one that enables it.

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

[gone, 2013-02-05 02:20:09]

Hey, does this approach make sense?  Seems like something similar was done for
the threaded compositing flags.

[danakj, 2013-02-05 02:23:57]

https://chromiumcodereview.appspot.com/12221005/diff/3001/cc/util.h
File cc/util.h (right):

https://chromiumcodereview.appspot.com/12221005/diff/3001/cc/util.h#newcode19
cc/util.h:19: bool IsImplSidePaintingEnabled(const CommandLine* cl);
Please don't read command line flags from in cc/. Use the values in
LayerTreeSettings instead.

Command line flags are per-process, but you may have multiple compositors in a
single process with different settings.

If you need a helper function for things in webkit/cb/ then it should live in
webkit/cb/

[Vangelis Kokkevis, 2013-02-05 17:37:11]

I think it would be best to follow the pattern we use for
force-compositing-mode. about:flags can have a dropdown menu with three options
(default, enabled, disabled), we get two flags (--enable and --disable like you
have in this CL) and put the logic in a central place, preferably here:

https://code.google.com/p/chromium/codesearch#chrome/src/content/browser/gpu/...

You can then call that method here:

https://code.google.com/p/chromium/codesearch#chrome/src/chrome/browser/chrom...

and pass it down to the render_view as a webpref . From that point on, like Dana
suggested, it can go through the LayerTreeSettings . 

Let me know if that doesn't make sense. 

If we do that, we'll be able to use the same flags for turning on
impl-side-painting on the desktop too.

[gone, 2013-02-06 00:42:16]

On 2013/02/05 17:37:11, Vangelis Kokkevis wrote:
> I think it would be best to follow the pattern we use for
> force-compositing-mode. about:flags can have a dropdown menu with three
options
> (default, enabled, disabled), we get two flags (--enable and --disable like
you
> have in this CL) and put the logic in a central place, preferably here:
> 
>
https://code.google.com/p/chromium/codesearch#chrome/src/content/browser/gpu/...
> 
> You can then call that method here:
> 
>
https://code.google.com/p/chromium/codesearch#chrome/src/chrome/browser/chrom...
> 
> and pass it down to the render_view as a webpref . From that point on, like
Dana
> suggested, it can go through the LayerTreeSettings . 
> 
> Let me know if that doesn't make sense. 
> 
> If we do that, we'll be able to use the same flags for turning on
> impl-side-painting on the desktop too.

I've piped the flag down from the browser to the renderer using
webkit_glue::WebPreferences, but I'm missing the connection from there to the
LayerTreeSettings, which appears to be reading the command line throughout the
constructor.  Is there a path I'm not seeing that doesn't involve reading the
command line in some way?

[danakj, 2013-02-06 00:46:06]

On Tue, Feb 5, 2013 at 7:42 PM, <dfalcantara@chromium.org> wrote:

> I've piped the flag down from the browser to the renderer using
> webkit_glue::WebPreferences, but I'm missing the connection from there to
> the
> LayerTreeSettings, which appears to be reading the command line throughout
> the
> constructor.  Is there a path I'm not seeing that doesn't involve reading
> the
> command line in some way?
>

webkit::WebPreferences (in webkit/) sets its values on WebKit::WebSettings
(in WebKit/Source/WebKit/chromium)
WebViewImpl (in WebKit/Source/WebKit/chromium) sets values on
WebLayerTreeView::Settings (in WebKit/Source/WebKit/chromium)
WebLayerTreeViewImpl (in webkit/) copies values from
WebLayerTreeView::Settings (in WebKit/Source/WebKit/chromium) to
LayerTreeSettings (in cc/)

[gone, 2013-02-06 22:27:51]

On 2013/02/06 00:46:06, danakj wrote:
> On Tue, Feb 5, 2013 at 7:42 PM, <mailto:dfalcantara@chromium.org> wrote:
> 
> > I've piped the flag down from the browser to the renderer using
> > webkit_glue::WebPreferences, but I'm missing the connection from there to
> > the
> > LayerTreeSettings, which appears to be reading the command line throughout
> > the
> > constructor.  Is there a path I'm not seeing that doesn't involve reading
> > the
> > command line in some way?
> >
> 
> webkit::WebPreferences (in webkit/) sets its values on WebKit::WebSettings
> (in WebKit/Source/WebKit/chromium)
> WebViewImpl (in WebKit/Source/WebKit/chromium) sets values on
> WebLayerTreeView::Settings (in WebKit/Source/WebKit/chromium)
> WebLayerTreeViewImpl (in webkit/) copies values from
> WebLayerTreeView::Settings (in WebKit/Source/WebKit/chromium) to
> LayerTreeSettings (in cc/)

Tunneled through that connection, thanks!  Is there something similar that can
be done for web_content_layer_impl.cc and web_image_layer_impl.cc?  I don't see
either of those having access to the LayerTreeSettings.

[gone, 2013-02-13 00:45:24]

On 2013/02/06 22:27:51, dfalcantara wrote:
> On 2013/02/06 00:46:06, danakj wrote:
> > On Tue, Feb 5, 2013 at 7:42 PM, <mailto:dfalcantara@chromium.org> wrote:
> > 
> > > I've piped the flag down from the browser to the renderer using
> > > webkit_glue::WebPreferences, but I'm missing the connection from there to
> > > the
> > > LayerTreeSettings, which appears to be reading the command line throughout
> > > the
> > > constructor.  Is there a path I'm not seeing that doesn't involve reading
> > > the
> > > command line in some way?
> > >
> > 
> > webkit::WebPreferences (in webkit/) sets its values on WebKit::WebSettings
> > (in WebKit/Source/WebKit/chromium)
> > WebViewImpl (in WebKit/Source/WebKit/chromium) sets values on
> > WebLayerTreeView::Settings (in WebKit/Source/WebKit/chromium)
> > WebLayerTreeViewImpl (in webkit/) copies values from
> > WebLayerTreeView::Settings (in WebKit/Source/WebKit/chromium) to
> > LayerTreeSettings (in cc/)
> 
> Tunneled through that connection, thanks!  Is there something similar that can
> be done for web_content_layer_impl.cc and web_image_layer_impl.cc?  I don't
see
> either of those having access to the LayerTreeSettings.

Ping on this; going to be picking it up tomorrow.

[Vangelis Kokkevis, 2013-02-13 01:58:01]

On 2013/02/13 00:45:24, dfalcantara wrote:
> On 2013/02/06 22:27:51, dfalcantara wrote:
> > On 2013/02/06 00:46:06, danakj wrote:
> > > On Tue, Feb 5, 2013 at 7:42 PM, <mailto:dfalcantara@chromium.org> wrote:
> > > 
> > > > I've piped the flag down from the browser to the renderer using
> > > > webkit_glue::WebPreferences, but I'm missing the connection from there
to
> > > > the
> > > > LayerTreeSettings, which appears to be reading the command line
throughout
> > > > the
> > > > constructor.  Is there a path I'm not seeing that doesn't involve
reading
> > > > the
> > > > command line in some way?
> > > >
> > > 
> > > webkit::WebPreferences (in webkit/) sets its values on WebKit::WebSettings
> > > (in WebKit/Source/WebKit/chromium)
> > > WebViewImpl (in WebKit/Source/WebKit/chromium) sets values on
> > > WebLayerTreeView::Settings (in WebKit/Source/WebKit/chromium)
> > > WebLayerTreeViewImpl (in webkit/) copies values from
> > > WebLayerTreeView::Settings (in WebKit/Source/WebKit/chromium) to
> > > LayerTreeSettings (in cc/)
> > 
> > Tunneled through that connection, thanks!  Is there something similar that
can
> > be done for web_content_layer_impl.cc and web_image_layer_impl.cc?  I don't
> see
> > either of those having access to the LayerTreeSettings.
> 
> Ping on this; going to be picking it up tomorrow.

Did you upload a new patch?

[gone, 2013-02-13 02:00:58]

On 2013/02/13 01:58:01, Vangelis Kokkevis wrote:
> On 2013/02/13 00:45:24, dfalcantara wrote:
> > On 2013/02/06 22:27:51, dfalcantara wrote:
> > > On 2013/02/06 00:46:06, danakj wrote:
> > > > On Tue, Feb 5, 2013 at 7:42 PM, <mailto:dfalcantara@chromium.org> wrote:
> > > > 
> > > > > I've piped the flag down from the browser to the renderer using
> > > > > webkit_glue::WebPreferences, but I'm missing the connection from there
> to
> > > > > the
> > > > > LayerTreeSettings, which appears to be reading the command line
> throughout
> > > > > the
> > > > > constructor.  Is there a path I'm not seeing that doesn't involve
> reading
> > > > > the
> > > > > command line in some way?
> > > > >
> > > > 
> > > > webkit::WebPreferences (in webkit/) sets its values on
WebKit::WebSettings
> > > > (in WebKit/Source/WebKit/chromium)
> > > > WebViewImpl (in WebKit/Source/WebKit/chromium) sets values on
> > > > WebLayerTreeView::Settings (in WebKit/Source/WebKit/chromium)
> > > > WebLayerTreeViewImpl (in webkit/) copies values from
> > > > WebLayerTreeView::Settings (in WebKit/Source/WebKit/chromium) to
> > > > LayerTreeSettings (in cc/)
> > > 
> > > Tunneled through that connection, thanks!  Is there something similar that
> can
> > > be done for web_content_layer_impl.cc and web_image_layer_impl.cc?  I
don't
> > see
> > > either of those having access to the LayerTreeSettings.
> > 
> > Ping on this; going to be picking it up tomorrow.
> 
> Did you upload a new patch?

Was looking for an answer to the question in #6; I don't know how to get around
the command line requirement for the other two cc files.

[vangelis, 2013-02-13 08:01:35]

> Was looking for an answer to the question in #6; I don't know how to get
around
> the command line requirement for the other two cc files.

Oh, I see. That's a tricky one given that the checks happen in the webkit glue
layer. I'm afraid I cannot think of any easy solutions here.  Dana?

[jamesr, 2013-02-13 08:46:04]

On 2013/02/05 17:37:11, Vangelis Kokkevis wrote:
> I think it would be best to follow the pattern we use for
> force-compositing-mode. about:flags can have a dropdown menu with three
options
> (default, enabled, disabled), we get two flags (--enable and --disable like
you
> have in this CL) and put the logic in a central place, preferably here:
> 
>
https://code.google.com/p/chromium/codesearch#chrome/src/content/browser/gpu/...
> 
> You can then call that method here:
> 
>
https://code.google.com/p/chromium/codesearch#chrome/src/chrome/browser/chrom...
> 
> and pass it down to the render_view as a webpref . From that point on, like
Dana
> suggested, it can go through the LayerTreeSettings . 

I don't think treating this as a webpref is possible since we need to know the
value in order to initialize a WebContentLayerImpl, and at the time we're
instantiating a WebContentLayerImpl we do not know which view (and thus which
webprefs, which are associated with views) should apply.

I'd go back to the switch version but put the logic for testing the switches and
figuring out what the enabled-ness of the feature is based on the combination of
enabled/disable flags in one place.

[gone, 2013-02-13 21:45:08]

On 2013/02/13 08:46:04, jamesr wrote:
> I'd go back to the switch version but put the logic for testing the switches
and
> figuring out what the enabled-ness of the feature is based on the combination
of
> enabled/disable flags in one place.

Any opinions on where this function should go?  Vangelis suggested earlier that
it should go into compositor_util.h, but that lives in content/public/browser/*
and seems to be insulated from the webkit/compositor_bindings files.

[jamesr, 2013-02-13 21:59:29]

On 2013/02/13 21:45:08, dfalcantara wrote:
> On 2013/02/13 08:46:04, jamesr wrote:
> > I'd go back to the switch version but put the logic for testing the switches
> and
> > figuring out what the enabled-ness of the feature is based on the
combination
> of
> > enabled/disable flags in one place.
> 
> Any opinions on where this function should go?  Vangelis suggested earlier
that
> it should go into compositor_util.h, but that lives in
content/public/browser/*
> and seems to be insulated from the webkit/compositor_bindings files.

The switches are in cc/ so i'd put a static function somewhere in cc/ that
provides the correct answer.

[gone, 2013-02-13 22:04:34]

On 2013/02/13 21:59:29, jamesr wrote:
> On 2013/02/13 21:45:08, dfalcantara wrote:
> > On 2013/02/13 08:46:04, jamesr wrote:
> > > I'd go back to the switch version but put the logic for testing the
switches
> > and
> > > figuring out what the enabled-ness of the feature is based on the
> combination
> > of
> > > enabled/disable flags in one place.
> > 
> > Any opinions on where this function should go?  Vangelis suggested earlier
> that
> > it should go into compositor_util.h, but that lives in
> content/public/browser/*
> > and seems to be insulated from the webkit/compositor_bindings files.
> 
> The switches are in cc/ so i'd put a static function somewhere in cc/ that
> provides the correct answer.

Alright, I'll go back to patch set 1 and clean it up.

[gone, 2013-02-13 22:05:42]

On 2013/02/13 22:04:34, dfalcantara wrote:
> On 2013/02/13 21:59:29, jamesr wrote:
> > On 2013/02/13 21:45:08, dfalcantara wrote:
> > > On 2013/02/13 08:46:04, jamesr wrote:
> > > > I'd go back to the switch version but put the logic for testing the
> switches
> > > and
> > > > figuring out what the enabled-ness of the feature is based on the
> > combination
> > > of
> > > > enabled/disable flags in one place.
> > > 
> > > Any opinions on where this function should go?  Vangelis suggested earlier
> > that
> > > it should go into compositor_util.h, but that lives in
> > content/public/browser/*
> > > and seems to be insulated from the webkit/compositor_bindings files.
> > 
> > The switches are in cc/ so i'd put a static function somewhere in cc/ that
> > provides the correct answer.
> 
> Alright, I'll go back to patch set 1 and clean it up.

Er, unless danakj objects to moving them back.

[danakj, 2013-02-13 22:06:57]

On 2013/02/13 22:05:42, dfalcantara wrote:
> On 2013/02/13 22:04:34, dfalcantara wrote:
> > On 2013/02/13 21:59:29, jamesr wrote:
> > > On 2013/02/13 21:45:08, dfalcantara wrote:
> > > > On 2013/02/13 08:46:04, jamesr wrote:
> > > > > I'd go back to the switch version but put the logic for testing the
> > switches
> > > > and
> > > > > figuring out what the enabled-ness of the feature is based on the
> > > combination
> > > > of
> > > > > enabled/disable flags in one place.
> > > > 
> > > > Any opinions on where this function should go?  Vangelis suggested
earlier
> > > that
> > > > it should go into compositor_util.h, but that lives in
> > > content/public/browser/*
> > > > and seems to be insulated from the webkit/compositor_bindings files.
> > > 
> > > The switches are in cc/ so i'd put a static function somewhere in cc/ that
> > > provides the correct answer.
> > 
> > Alright, I'll go back to patch set 1 and clean it up.
> 
> Er, unless danakj objects to moving them back.

I think putting a static function in cc/switches.h that has no side effects is
fine. I just didn't want to have us reading cmd line flags and then setting
things based on them from inside cc/. That should happen at the
RenderWidgetCompositor level.

[gone, 2013-02-20 00:16:07]

I took another pass and moved the function to switches.h/cc.  Tried to make the
function live entirely in switches.h, but I got unused function errors during
compilation.

[danakj, 2013-02-20 00:17:47]

https://chromiumcodereview.appspot.com/12221005/diff/17001/cc/switches.h
File cc/switches.h (right):

https://chromiumcodereview.appspot.com/12221005/diff/17001/cc/switches.h#newc...
cc/switches.h:10: #include "base/command_line.h"
This should move to the .cc then.

[gone, 2013-02-20 00:28:21]

https://chromiumcodereview.appspot.com/12221005/diff/17001/cc/switches.h
File cc/switches.h (right):

https://chromiumcodereview.appspot.com/12221005/diff/17001/cc/switches.h#newc...
cc/switches.h:10: #include "base/command_line.h"
On 2013/02/20 00:17:48, danakj wrote:
> This should move to the .cc then.

Done.

[gone, 2013-02-21 18:59:35]

Adding more folks from OWNERS files.

[Ben Goodger (Google), 2013-02-22 00:48:52]

lgtm

[gone, 2013-02-22 21:09:43]

Ping?

[jamesr, 2013-02-22 21:11:11]

On 2013/02/22 21:09:43, dfalcantara wrote:
> Ping?

There are 6 people on the review line.  ben@ has OWNERS on everything, so I'm
guessing you want specific people to look at specific pieces.  Please say who
you are pinging and what parts of the patch you want them to look at.

[gone, 2013-02-22 21:12:14]

On 2013/02/22 21:11:11, jamesr wrote:
> On 2013/02/22 21:09:43, dfalcantara wrote:
> > Ping?
> 
> There are 6 people on the review line.  ben@ has OWNERS on everything, so I'm
> guessing you want specific people to look at specific pieces.  Please say who
> you are pinging and what parts of the patch you want them to look at.

Oh, didn't realize the src/ directory had a catch-all OWNERS file.  Will go
ahead and go through the queue.  Thanks!

[commit-bot: I haz the power, 2013-02-22 21:12:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dfalcantara@chromium.org/12221005/29001

[commit-bot: I haz the power, 2013-02-23 00:31:37]

Change committed as 184224