1266846 - Move prefs and safe mode checks for Layers&WebGL into GetFeatureStatus

Status: RESOLVED INVALID

(Core :: Graphics: Layers, defect)

Description

[Benoit Girard (:BenWa)]

This will organize the logic, make GetFeatureStatus more accurate and make it easier to assign a FailureID.

Comment 1

[Benoit Girard (:BenWa)]

Attachment: MozReview Request: Bug 1266846 - Move prefs and safe mode checks for Layers&WebGL into GetFeatureStatus. r=milan

Review commit: https://reviewboard.mozilla.org/r/48555/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/48555/

Comment 2

[Benoit Girard (:BenWa)]

This is going to break a lot of tests that test a windows/*nix only feature on all platforms:
    status = gfxInfo.getFeatureStatus(Ci.nsIGfxInfo.FEATURE_DIRECT3D_9_LAYERS);
    do_check_eq(status, Ci.nsIGfxInfo.FEATURE_STATUS_OK);

Comment 3

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8744458 [details]
MozReview Request: Bug 1266846 - Move prefs and safe mode checks for Layers&WebGL into GetFeatureStatus. r=milan

https://reviewboard.mozilla.org/r/48555/#review45549

::: dom/canvas/WebGLContext.cpp
(Diff revision 1)
>      // increment the generation number - Do this early because later
>      // in CreateOffscreenGL(), "default" objects are created that will
>      // pick up the old generation.
>      ++mGeneration;
>  
> -    bool disabled = gfxPrefs::WebGLDisabled();

I think we may have changed the logic with this edit, where webgl.disabled used to win over webgl.force-enabled, and now that's reversed - webgl.force-enabled wins over webgl.disabled.  Can you double check?

::: dom/canvas/WebGLContext.cpp:853
(Diff revision 1)
>      // Alright, now let's start trying.
>      bool forceEnabled = gfxPrefs::WebGLForceEnabled();
>      ScopedGfxFeatureReporter reporter("WebGL", forceEnabled);
>  
>      MOZ_ASSERT(!gl);
>      if (!CreateAndInitGL(forceEnabled)) {

From what I can tell, with this change webgl.force-enabled preference wins over webgl.disabled preference?  Before, this change, webgl.disabled wins over webgl.force-enabled.  Can you double check?

::: gfx/thebes/gfxPlatform.cpp
(Diff revision 1)
>  
>  bool
>  gfxPlatform::ShouldUseLayersAcceleration()
>  {
> -  const char *acceleratedEnv = PR_GetEnv("MOZ_ACCELERATED");
> +
> -  if (gfxPrefs::LayersAccelerationDisabled() ||

I don't understand this change - why do we want ShouldUseLayersAcceleration() to return true when layers.acceleration.disabled preference is set, or we're in safe mode?  Are we convinced all the places that call gfxPlatform::ShouldUseLayersAcceleration also check GfxInfo?

::: widget/GfxInfoBase.cpp:983
(Diff revision 1)
>    {
>      aFailureId = "FEATURE_FAILURE_CANT_RESOLVE_ADAPTER";
>      return NS_OK;
>    }
>  
> +  bool acceleratedLayers =

May actually be more readable if we don't create "acceleratedLayers" and "isWebGL" variables, but do a switch and immedately run the tests/set status/return as we do below.  The code is a bit out of order this way, we set these "are we asking about layer acceleration" and "are we asking about WebGL features", then we do the platform specific quick exit, then we use those variables we set.  It will probably read better if things are grouped.

::: widget/GfxInfoBase.cpp:1012
(Diff revision 1)
> +#endif
> +#ifndef ANDROID
> +      || aFeature == nsIGfxInfo::FEATURE_STAGEFRIGHT
> +#endif
> +#ifndef XP_WIN
> +      || aFeature == nsIGfxInfo::FEATURE_DIRECT2D

Already 13 lines above?

::: widget/GfxInfoBase.cpp:1016
(Diff revision 1)
> +#ifndef XP_WIN
> +      || aFeature == nsIGfxInfo::FEATURE_DIRECT2D
> +#endif
> +      ) {
> +    *aStatus = nsIGfxInfo::FEATURE_BLOCKED_OS_VERSION;
> +    aFailureId = "FEATURE_FAILURE_NOT_SUPPORTED";

I'd be interested to see if we ever hit this code.  If we don't, maybe not a bad idea to put an assert/gfxCriticalError?

Comment 4

[Benoit Girard (:BenWa)]

(In reply to Milan Sreckovic [:milan] from comment #3)
> I think we may have changed the logic with this edit, where webgl.disabled
> used to win over webgl.force-enabled, and now that's reversed -
> webgl.force-enabled wins over webgl.disabled.  Can you double check?

You're correct. Is this something that's important to maintain? I don't see a use case where having a particular behavior when conflicting preferences are set is important.

I don't really see a good way to preserve the old behavior.

> ::: gfx/thebes/gfxPlatform.cpp
> (Diff revision 1)
> >  
> >  bool
> >  gfxPlatform::ShouldUseLayersAcceleration()
> >  {
> > -  const char *acceleratedEnv = PR_GetEnv("MOZ_ACCELERATED");
> > +
> > -  if (gfxPrefs::LayersAccelerationDisabled() ||
> 
> I don't understand this change - why do we want
> ShouldUseLayersAcceleration() to return true when
> layers.acceleration.disabled preference is set, or we're in safe mode?  Are
> we convinced all the places that call
> gfxPlatform::ShouldUseLayersAcceleration also check GfxInfo?

My logic behind this change was that this is really just 'ShouldMaybeUseLayersAcceleration'. I think ideally this should go away all together. IMO it would be best if GetFeatureStatus held all the static checks and after you tried to init a feature than gfxConfig would know the run-time results. It's already the case that this check is incomplete since we don't check the blocklist.

I'm not convinced that all places also check GfxInfo. I checked and this method has a complex callgraph. This change is certainly risky. We could leave it here to minimize the risk of regressions at the cost of duplicating code that may fall out of sync. Long term it would be best to organize this code as much as possible.

> ::: widget/GfxInfoBase.cpp:1016
> (Diff revision 1)
> > +#ifndef XP_WIN
> > +      || aFeature == nsIGfxInfo::FEATURE_DIRECT2D
> > +#endif
> > +      ) {
> > +    *aStatus = nsIGfxInfo::FEATURE_BLOCKED_OS_VERSION;
> > +    aFailureId = "FEATURE_FAILURE_NOT_SUPPORTED";
> 
> I'd be interested to see if we ever hit this code.  If we don't, maybe not a
> bad idea to put an assert/gfxCriticalError?

We do in our tests at the very least. With some future changes I'd like to just query all the features we have and use this to report features that are not supported rather than having each call site maintain the list of unsupported features.

Comment 5

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Benoit Girard (:BenWa) from comment #4)
> 
> My logic behind this change was that this is really just
> 'ShouldMaybeUseLayersAcceleration'. I think ideally this should go away all
> together. IMO it would be best if GetFeatureStatus held all the static
> checks and after you tried to init a feature than gfxConfig would know the
> run-time results. It's already the case that this check is incomplete since
> we don't check the blocklist.
> 
> I'm not convinced that all places also check GfxInfo. I checked and this
> method has a complex callgraph. This change is certainly risky. We could
> leave it here to minimize the risk of regressions at the cost of duplicating
> code that may fall out of sync. Long term it would be best to organize this
> code as much as possible.

Let's double up the code, leave this in, and open another bug to sort out exactly what we want to happen, while not blocking this particular change.

Comment 6

[Benoit Girard (:BenWa)]

I misunderstood gfxConfig. This data belongs there, not here.