Change ACCELERATED_COMPOSITING blacklist to GPU_COMPOSITING.

Change ACCELERATED_COMPOSITING blacklist to GPU_COMPOSITING.

Remove any checks for ACCELERATED_COMPOSITING to determine if
compositing is available, since it always is. Instead rename
the blacklist entry to GPU_COMPOSITING and use it to prevent
GL-based compositing.

Depends on: https://codereview.chromium.org/238933009/

R=jbauman@chromium.org, piman@chromium.org, zmo@chromium.org
BUG=363772
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=264566

[danakj, 2014-04-16 17:07:57]

jbauman: please review
piman/zmo: OWNERS review
koz: please look at the */extensions/ changes

[Zhenyao Mo, 2014-04-16 17:19:18]

https://codereview.chromium.org/239083006/diff/20001/content/public/common/co...
File content/public/common/content_switches.h (right):

https://codereview.chromium.org/239083006/diff/20001/content/public/common/co...
content/public/common/content_switches.h:24: CONTENT_EXPORT extern const char
kBlacklistGpuCompositing[];
There is also kDisableAcceleratedCompositing you need to modify.

[danakj, 2014-04-16 17:20:34]

https://codereview.chromium.org/239083006/diff/20001/content/public/common/co...
File content/public/common/content_switches.h (right):

https://codereview.chromium.org/239083006/diff/20001/content/public/common/co...
content/public/common/content_switches.h:24: CONTENT_EXPORT extern const char
kBlacklistGpuCompositing[];
On 2014/04/16 17:19:18, Zhenyao Mo wrote:
> There is also kDisableAcceleratedCompositing you need to modify.

That should be by https://codereview.chromium.org/238933009/ which this depends
on? Or am I missing something else.

[Zhenyao Mo, 2014-04-16 17:21:36]

On 2014/04/16 17:20:34, danakj wrote:
>
https://codereview.chromium.org/239083006/diff/20001/content/public/common/co...
> File content/public/common/content_switches.h (right):
> 
>
https://codereview.chromium.org/239083006/diff/20001/content/public/common/co...
> content/public/common/content_switches.h:24: CONTENT_EXPORT extern const char
> kBlacklistGpuCompositing[];
> On 2014/04/16 17:19:18, Zhenyao Mo wrote:
> > There is also kDisableAcceleratedCompositing you need to modify.
> 
> That should be by https://codereview.chromium.org/238933009/ which this
depends
> on? Or am I missing something else.

I missed that one.  gpu/ LGTM then.

[piman, 2014-04-16 17:56:12]

https://codereview.chromium.org/239083006/diff/40001/content/public/common/co...
File content/public/common/content_switches.cc (right):

https://codereview.chromium.org/239083006/diff/40001/content/public/common/co...
content/public/common/content_switches.cc:43: "blacklist-gpu-compositing";
What's the use of that over --disable-gpu?

[danakj, 2014-04-16 18:07:43]

https://codereview.chromium.org/239083006/diff/40001/content/public/common/co...
File content/public/common/content_switches.cc (right):

https://codereview.chromium.org/239083006/diff/40001/content/public/common/co...
content/public/common/content_switches.cc:43: "blacklist-gpu-compositing";
On 2014/04/16 17:56:13, piman wrote:
> What's the use of that over --disable-gpu?

I don't know I was going to ask about this. It seemed like this was meant for
testing as if the card was blacklisted somehow. At first I removed it but I'm
not sure if people use it:

1. --disable-gpu causes GpuDataManagerImplPrivate::DisableHardwareAcceleration()
to happen in the constructor.
2. --disable-gpu then sets card_blacklisted_ = true
3. --disable-gpu then puts GPU_COMPOSITING into the blacklisted_features_

Later, GpuDataManagerImplPrivate::Initialize happens which goes to
InitializeImpl:
1. InitializeImpl calls UpdateGpuInfo which calls UpdateGpuInfoHelper
2. UpdateGpuInfoHelper gets a new |features| set from
gpu_blacklist_->MakeDecision
3. This |features| is given to UpdateBlacklistedFeatures 
4. The |features| replaces blacklisted_features_

At this point --disable-gpu has done nothing except mark card_blacklisted =
true.

5. If card_blacklisted_ or kBlacklistGpuCompositing, we add GPU_COMPOSITING back
to blacklisted_features.

So:
- Why does --disable-gpu bother doing anything except set card_blacklisted_ ?
- I don't think this flag is very useful, but then the same argument for
kBlacklistWebGL. Should I remove them both as another CL together?

[danakj, 2014-04-16 18:09:18]

On 2014/04/16 18:07:43, danakj wrote:
>
https://codereview.chromium.org/239083006/diff/40001/content/public/common/co...
> File content/public/common/content_switches.cc (right):
> 
>
https://codereview.chromium.org/239083006/diff/40001/content/public/common/co...
> content/public/common/content_switches.cc:43: "blacklist-gpu-compositing";
> On 2014/04/16 17:56:13, piman wrote:
> > What's the use of that over --disable-gpu?
> 
> I don't know I was going to ask about this. It seemed like this was meant for
> testing as if the card was blacklisted somehow. At first I removed it but I'm
> not sure if people use it:
> 
> 1. --disable-gpu causes
GpuDataManagerImplPrivate::DisableHardwareAcceleration()
> to happen in the constructor.
> 2. --disable-gpu then sets card_blacklisted_ = true
> 3. --disable-gpu then puts GPU_COMPOSITING into the blacklisted_features_
> 
> Later, GpuDataManagerImplPrivate::Initialize happens which goes to
> InitializeImpl:
> 1. InitializeImpl calls UpdateGpuInfo which calls UpdateGpuInfoHelper
> 2. UpdateGpuInfoHelper gets a new |features| set from
> gpu_blacklist_->MakeDecision
> 3. This |features| is given to UpdateBlacklistedFeatures 
> 4. The |features| replaces blacklisted_features_
> 
> At this point --disable-gpu has done nothing except mark card_blacklisted =
> true.
> 
> 5. If card_blacklisted_ or kBlacklistGpuCompositing, we add GPU_COMPOSITING
back
> to blacklisted_features.
> 
> So:
> - Why does --disable-gpu bother doing anything except set card_blacklisted_ ?
> - I don't think this flag is very useful, but then the same argument for
> kBlacklistWebGL. Should I remove them both as another CL together?

I guess 1 difference is that --disable-gpu still has an effect if
kIgnoreGpuBlacklist or kUseGpuInTests is used. And the blacklist-gpu-compositing
flag would not?

[piman, 2014-04-16 18:10:49]

LGTM, certainly don't want to block this... but we should revisit those flags.

[Zhenyao Mo, 2014-04-16 18:22:20]

On 2014/04/16 18:07:43, danakj wrote:
>
https://codereview.chromium.org/239083006/diff/40001/content/public/common/co...
> File content/public/common/content_switches.cc (right):
> 
>
https://codereview.chromium.org/239083006/diff/40001/content/public/common/co...
> content/public/common/content_switches.cc:43: "blacklist-gpu-compositing";
> On 2014/04/16 17:56:13, piman wrote:
> > What's the use of that over --disable-gpu?
> 
> I don't know I was going to ask about this. It seemed like this was meant for
> testing as if the card was blacklisted somehow. At first I removed it but I'm
> not sure if people use it:
> 
> 1. --disable-gpu causes
GpuDataManagerImplPrivate::DisableHardwareAcceleration()
> to happen in the constructor.
> 2. --disable-gpu then sets card_blacklisted_ = true
> 3. --disable-gpu then puts GPU_COMPOSITING into the blacklisted_features_
> 
> Later, GpuDataManagerImplPrivate::Initialize happens which goes to
> InitializeImpl:
> 1. InitializeImpl calls UpdateGpuInfo which calls UpdateGpuInfoHelper
> 2. UpdateGpuInfoHelper gets a new |features| set from
> gpu_blacklist_->MakeDecision
> 3. This |features| is given to UpdateBlacklistedFeatures 
> 4. The |features| replaces blacklisted_features_
> 
> At this point --disable-gpu has done nothing except mark card_blacklisted =
> true.
> 
> 5. If card_blacklisted_ or kBlacklistGpuCompositing, we add GPU_COMPOSITING
back
> to blacklisted_features.
> 
> So:
> - Why does --disable-gpu bother doing anything except set card_blacklisted_ ?
> - I don't think this flag is very useful, but then the same argument for
> kBlacklistWebGL. Should I remove them both as another CL together?

jbauman should be able answer this question.  There was definitely a purpose,
which may not hold to be true now.

[jbauman, 2014-04-16 20:31:46]

On 2014/04/16 18:22:20, Zhenyao Mo wrote:
> On 2014/04/16 18:07:43, danakj wrote:
> >
>
https://codereview.chromium.org/239083006/diff/40001/content/public/common/co...
> > File content/public/common/content_switches.cc (right):
> > 
> >
>
https://codereview.chromium.org/239083006/diff/40001/content/public/common/co...
> > content/public/common/content_switches.cc:43: "blacklist-gpu-compositing";
> > On 2014/04/16 17:56:13, piman wrote:
> > > What's the use of that over --disable-gpu?
> > 
> > I don't know I was going to ask about this. It seemed like this was meant
for
> > testing as if the card was blacklisted somehow. At first I removed it but
I'm
> > not sure if people use it:
> > 
> > 1. --disable-gpu causes
> GpuDataManagerImplPrivate::DisableHardwareAcceleration()
> > to happen in the constructor.
> > 2. --disable-gpu then sets card_blacklisted_ = true
> > 3. --disable-gpu then puts GPU_COMPOSITING into the blacklisted_features_
> > 
> > Later, GpuDataManagerImplPrivate::Initialize happens which goes to
> > InitializeImpl:
> > 1. InitializeImpl calls UpdateGpuInfo which calls UpdateGpuInfoHelper
> > 2. UpdateGpuInfoHelper gets a new |features| set from
> > gpu_blacklist_->MakeDecision
> > 3. This |features| is given to UpdateBlacklistedFeatures 
> > 4. The |features| replaces blacklisted_features_
> > 
> > At this point --disable-gpu has done nothing except mark card_blacklisted =
> > true.
> > 
> > 5. If card_blacklisted_ or kBlacklistGpuCompositing, we add GPU_COMPOSITING
> back
> > to blacklisted_features.
> > 
> > So:
> > - Why does --disable-gpu bother doing anything except set card_blacklisted_
?
> > - I don't think this flag is very useful, but then the same argument for
> > kBlacklistWebGL. Should I remove them both as another CL together?
> 
> jbauman should be able answer this question.  There was definitely a purpose,
> which may not hold to be true now.

lgtm

It was originally designed to make it easy to manually enable swiftshader, but
now that we have --disable-gpu we can probably get rid of it.

[Marijn Kruisselbrink, 2014-04-16 20:39:13]

https://codereview.chromium.org/239083006/diff/40001/extensions/common/manife...
File extensions/common/manifest_handlers/requirements_info.cc (left):

https://codereview.chromium.org/239083006/diff/40001/extensions/common/manife...
extensions/common/manifest_handlers/requirements_info.cc:123: } else if (feature
== "css3d") {
Removing this if entirely makes this a backwards incompatible change. Extensions
that specify the css3d requirement would now always give an error (since now the
else below would be reached). I think the right thing to do would be to silently
ignore (but not reject) the css3d feature? (at least I understand that now what
the css3d feature specified will always be available)

[danakj, 2014-04-16 20:46:57]

https://codereview.chromium.org/239083006/diff/40001/extensions/common/manife...
File extensions/common/manifest_handlers/requirements_info.cc (left):

https://codereview.chromium.org/239083006/diff/40001/extensions/common/manife...
extensions/common/manifest_handlers/requirements_info.cc:123: } else if (feature
== "css3d") {
On 2014/04/16 20:39:14, Marijn Kruisselbrink wrote:
> Removing this if entirely makes this a backwards incompatible change.
Extensions
> that specify the css3d requirement would now always give an error (since now
the
> else below would be reached). I think the right thing to do would be to
silently
> ignore (but not reject) the css3d feature? (at least I understand that now
what
> the css3d feature specified will always be available)

oh ok that was what I was attempting to do yes. i guess i messed that up here.

[Marijn Kruisselbrink, 2014-04-16 20:52:06]

*/extensions/ lgtm

[danakj, 2014-04-16 20:54:10]

https://codereview.chromium.org/239083006/diff/40001/extensions/common/manife...
File extensions/common/manifest_handlers/requirements_info.cc (left):

https://codereview.chromium.org/239083006/diff/40001/extensions/common/manife...
extensions/common/manifest_handlers/requirements_info.cc:123: } else if (feature
== "css3d") {
On 2014/04/16 20:46:57, danakj wrote:
> On 2014/04/16 20:39:14, Marijn Kruisselbrink wrote:
> > Removing this if entirely makes this a backwards incompatible change.
> Extensions
> > that specify the css3d requirement would now always give an error (since now
> the
> > else below would be reached). I think the right thing to do would be to
> silently
> > ignore (but not reject) the css3d feature? (at least I understand that now
> what
> > the css3d feature specified will always be available)
> 
> oh ok that was what I was attempting to do yes. i guess i messed that up here.


I'm trying to understand what the right thing to do here is. I don't see "css3d"
or even "3D" in the manifest_constants.cc file. Would branching and doing
nothing here be the right thing? Should I leave this code alone and just change
the RequirementsChecker?

[danakj, 2014-04-16 20:59:41]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-04-16 21:00:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/239083006/120001

[commit-bot: I haz the power, 2014-04-16 21:29:38]

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

[commit-bot: I haz the power, 2014-04-16 21:29:39]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_clang_dbg

[danakj, 2014-04-16 22:36:44]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-04-16 22:37:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/239083006/120001

[commit-bot: I haz the power, 2014-04-16 22:39:35]

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

[commit-bot: I haz the power, 2014-04-16 22:39:36]

Try jobs failed on following builders:
  tryserver.chromium on mac_chromium_compile_dbg

[danakj, 2014-04-16 23:44:38]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-04-16 23:45:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/239083006/100002

[commit-bot: I haz the power, 2014-04-17 00:57:15]

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

[commit-bot: I haz the power, 2014-04-17 00:57:15]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel

[danakj, 2014-04-17 14:43:04]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-04-17 14:44:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/239083006/160001

[commit-bot: I haz the power, 2014-04-17 17:38:50]

Change committed as 264566