Move CC switches to cc/switches.h.

Move CC switches to cc/switches.h.

This moves these switches to their appropriate place, and sets them all
directly from command-line flags there.  I deleted the WebCompositor
setter methods, but setters are still needed in CCSettings for unit
tests.

BUG=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=162921

[aelias_OOO_until_Jul13, 2012-10-18 02:34:35]

This is some straightforward switch reshuffling.  Note that we'll need to add
cc/ to DEPS in chrome/ and content/ before this can land, which according to
James is already planned.

[aelias_OOO_until_Jul13, 2012-10-18 02:38:47]

I went ahead and added the DEPS entries to this patch.

[jamesr, 2012-10-18 02:44:32]

I'm afraid this will break chromeos - they use different command line flags to
control the same CCSettings in different contexts.  I.e. currently
--enable-ui-per-tile-painting is set and --enable-per-tile-painting is not.

https://codereview.chromium.org/11186039/diff/3001/ui/compositor/compositor.cc
File ui/compositor/compositor.cc (left):

https://codereview.chromium.org/11186039/diff/3001/ui/compositor/compositor.c...
ui/compositor/compositor.cc:186:
command_line->HasSwitch(switches::kUIEnablePerTilePainting));
notice that this is checking switches::kUIEnablePartialSwap /
switches::kUIEnablePerTilePainting - not the same thing as
switches::kEnablePartialSwap / switches::kEnablePerTilePainting

[piman, 2012-10-18 03:05:32]

https://codereview.chromium.org/11186039/diff/3001/cc/settings.h
File cc/settings.h (right):

https://codereview.chromium.org/11186039/diff/3001/cc/settings.h#newcode12
cc/settings.h:12: // setting.
Please read this :)

[aelias_OOO_until_Jul13, 2012-10-18 03:18:04]

https://codereview.chromium.org/11186039/diff/3001/cc/settings.h
File cc/settings.h (right):

https://codereview.chromium.org/11186039/diff/3001/cc/settings.h#newcode12
cc/settings.h:12: // setting.
On 2012/10/18 03:05:33, piman wrote:
> Please read this :)

Based on this, I think this class should not exist at all.  We can't predict
what kind of setting we may wish to use in one compositor but not the other.

[piman, 2012-10-18 03:22:01]

https://codereview.chromium.org/11186039/diff/3001/cc/settings.h
File cc/settings.h (right):

https://codereview.chromium.org/11186039/diff/3001/cc/settings.h#newcode12
cc/settings.h:12: // setting.
On 2012/10/18 03:18:04, aelias wrote:
> On 2012/10/18 03:05:33, piman wrote:
> > Please read this :)
> 
> Based on this, I think this class should not exist at all.  We can't predict
> what kind of setting we may wish to use in one compositor but not the other.

I actually agree with that - I hate globals :)

In practice, having a per-process global seems enough today for many settings,
but not a cross-process global.

[aelias_OOO_until_Jul13, 2012-10-18 03:47:14]

OK, anyway, this patch is an improvement on the status quo.

I fixed ChromeOS by reintroducing the setters and having it call cc::Settings
directly.  To avoid Enne having to touch ui/ during the big rename, I renamed
CCSettings to Settings.

[piman, 2012-10-18 04:13:46]

https://codereview.chromium.org/11186039/diff/8001/content/public/common/cont...
File content/public/common/content_switches.h (left):

https://codereview.chromium.org/11186039/diff/8001/content/public/common/cont...
content/public/common/content_switches.h:112: extern const char
kEnablePartialSwap[];
You'll have to update chrome/browser/chromeos/login/login_utils.cc where it
whitelists the flags for the guest session on Chrome OS

[aelias_OOO_until_Jul13, 2012-10-18 04:35:32]

On 2012/10/18 04:13:46, piman wrote:
>
https://codereview.chromium.org/11186039/diff/8001/content/public/common/cont...
> File content/public/common/content_switches.h (left):
> 
>
https://codereview.chromium.org/11186039/diff/8001/content/public/common/cont...
> content/public/common/content_switches.h:112: extern const char
> kEnablePartialSwap[];
> You'll have to update chrome/browser/chromeos/login/login_utils.cc where it
> whitelists the flags for the guest session on Chrome OS

OK, done.  I also took the opportunity to add kEnablePinchInCompositor to the
whitelist, since you guys will want that when you support pinch.

[piman, 2012-10-18 04:46:43]

LGTM for my parts. I find the Settings globals rather clunky as is (initialize
with the value of --enable-partial-swap then override with the value of
--ui-enable-partial-swap), but I don't have a good way otherwise so I'll live.

[jamesr, 2012-10-18 05:52:37]

lgtm

de-staticifying these is on the list but a little ways down there

[sky, 2012-10-18 17:16:26]

LGTM

https://codereview.chromium.org/11186039/diff/24/content/DEPS
File content/DEPS (right):

https://codereview.chromium.org/11186039/diff/24/content/DEPS#newcode6
content/DEPS:6: "+cc",
nit: keep sorted.

https://codereview.chromium.org/11186039/diff/24/content/public/common/conten...
File content/public/common/content_switches.cc (right):

https://codereview.chromium.org/11186039/diff/24/content/public/common/conten...
content/public/common/content_switches.cc:714: const char
kDisableThreadedCompositing[]     = "disable-threaded-compositing";
Make position match that of header.

[tfarina, 2012-10-18 17:31:33]

http://codereview.chromium.org/11186039/diff/24/ui/DEPS
File ui/DEPS (right):

http://codereview.chromium.org/11186039/diff/24/ui/DEPS#newcode2
ui/DEPS:2: "+cc",
hum? these is going to introduce a circular dependency!

cc/ depends on ui/, now ui/ depends on cc/

Please, can you avoid this?

I think you meant to add this to ui/compositor/DEPS instead.

[tfarina, 2012-10-18 17:32:10]

http://codereview.chromium.org/11186039/diff/24/ui/DEPS
File ui/DEPS (right):

http://codereview.chromium.org/11186039/diff/24/ui/DEPS#newcode2
ui/DEPS:2: "+cc",
On 2012/10/18 17:31:33, tfarina wrote:
> hum? these is going to introduce a circular dependency!
> 
sorry, typed too fast. s/these/this

[sky, 2012-10-18 17:38:27]

http://codereview.chromium.org/11186039/diff/24/ui/DEPS
File ui/DEPS (right):

http://codereview.chromium.org/11186039/diff/24/ui/DEPS#newcode2
ui/DEPS:2: "+cc",
On 2012/10/18 17:32:10, tfarina wrote:
> On 2012/10/18 17:31:33, tfarina wrote:
> > hum? these is going to introduce a circular dependency!
> > 
> sorry, typed too fast. s/these/this
> 
> 

Good point. That needs to be straightened out.

[jamesr, 2012-10-18 18:22:40]

On 2012/10/18 17:38:27, sky wrote:
> http://codereview.chromium.org/11186039/diff/24/ui/DEPS
> File ui/DEPS (right):
> 
> http://codereview.chromium.org/11186039/diff/24/ui/DEPS#newcode2
> ui/DEPS:2: "+cc",
> On 2012/10/18 17:32:10, tfarina wrote:
> > On 2012/10/18 17:31:33, tfarina wrote:
> > > hum? these is going to introduce a circular dependency!
> > > 
> > sorry, typed too fast. s/these/this
> > 
> > 
> 
> Good point. That needs to be straightened out.

cc/ is using ui/gfx/ types, but shouldn't be touching anything else in ui/.  I
think the tree we want is:

ui/ depends on cc/
ui/ depends on ui/gfx
cc/ depends on ui/gfx

assuming that ui/gfx's geometry types do not depend on anything else in ui/

[aelias_OOO_until_Jul13, 2012-10-18 18:55:12]

cc was only depending on ui/gfx so if I understand the DEPS semantics correctly,
there should already have been a non-circular dependency ui/gfx/ -> cc/ -> ui/. 
But I added ui/gfx/ to ui/DEPS as James suggested.

[jamesr, 2012-10-18 19:25:46]

http://codereview.chromium.org/11186039/diff/10002/ui/DEPS
File ui/DEPS (right):

http://codereview.chromium.org/11186039/diff/10002/ui/DEPS#newcode13
ui/DEPS:13: "+ui/gfx",
this isn't what - meant. ui/DEPS is the list of paths that code in ui/ is
allowed to include. it doesn't make sense to list ui/gfx in here, code in ui/
can obviously include other code in ui/

cc/DEPS already lists ui/gfx (but not ui)

[tfarina, 2012-10-18 19:27:43]

On 2012/10/18 19:25:46, jamesr wrote:
> http://codereview.chromium.org/11186039/diff/10002/ui/DEPS
> File ui/DEPS (right):
> 
> http://codereview.chromium.org/11186039/diff/10002/ui/DEPS#newcode13
> ui/DEPS:13: "+ui/gfx",
> this isn't what - meant. ui/DEPS is the list of paths that code in ui/ is
> allowed to include. it doesn't make sense to list ui/gfx in here, code in ui/
> can obviously include other code in ui/
> 
> cc/DEPS already lists ui/gfx (but not ui)

My comment still stands, why this can't be in ui/compositor/DEPS instead?
cc/settings.h is only necessary for compositor.cc

When other places of ui/ needs to depend on cc/, then we can evaluate.

[jamesr, 2012-10-18 19:29:54]

On 2012/10/18 19:27:43, tfarina wrote:
> On 2012/10/18 19:25:46, jamesr wrote:
> > http://codereview.chromium.org/11186039/diff/10002/ui/DEPS
> > File ui/DEPS (right):
> > 
> > http://codereview.chromium.org/11186039/diff/10002/ui/DEPS#newcode13
> > ui/DEPS:13: "+ui/gfx",
> > this isn't what - meant. ui/DEPS is the list of paths that code in ui/ is
> > allowed to include. it doesn't make sense to list ui/gfx in here, code in
ui/
> > can obviously include other code in ui/
> > 
> > cc/DEPS already lists ui/gfx (but not ui)
> 
> My comment still stands, why this can't be in ui/compositor/DEPS instead?
> cc/settings.h is only necessary for compositor.cc
> 
> When other places of ui/ needs to depend on cc/, then we can evaluate.

This is a very good point - I agree with making the DEPS rule more specific.

[aelias_OOO_until_Jul13, 2012-10-18 21:54:00]

OK, I moved the DEPS rule to ui/compositor instead and fixed some other build
nits.  Landing.

[commit-bot: I haz the power, 2012-10-18 21:54:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aelias@chromium.org/11186039/14033

[commit-bot: I haz the power, 2012-10-18 23:14:34]

Sorry for I got bad news for ya.
Compile failed with a clobber build.
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[aelias_OOO_until_Jul13, 2012-10-18 23:28:20]

Windows bot doesn't like the direct dependency on both CC and webkit_glue in
ui/compositor so I reverted that part of my patch.  It seems this kind of change
may have to wait until CC is DLLized.

[commit-bot: I haz the power, 2012-10-18 23:28:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aelias@chromium.org/11186039/5007

[commit-bot: I haz the power, 2012-10-19 01:23:00]

Retried try job too often for step(s) interactive_ui_tests

[commit-bot: I haz the power, 2012-10-19 01:26:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aelias@chromium.org/11186039/5007

[commit-bot: I haz the power, 2012-10-19 03:39:41]

Change committed as 162921