Moving SwiftShader from component to bundled library

chrome/browser/policy/policy_browsertest.cc

Index: chrome/browser/policy/policy_browsertest.cc
diff --git a/chrome/browser/policy/policy_browsertest.cc b/chrome/browser/policy/policy_browsertest.cc
index 1c77345bbea40745a727ff99c9c251e938af09a7..8679dfa55046421aa3b7de2636c3529e9ac85554 100644
--- a/chrome/browser/policy/policy_browsertest.cc
+++ b/chrome/browser/policy/policy_browsertest.cc
@@ -4078,7 +4078,8 @@ IN_PROC_BROWSER_TEST_F(HardwareAccelerationModePolicyTest,
                        HardwareAccelerationDisabled) {
   // Verifies that hardware acceleration can be disabled with policy.
   EXPECT_FALSE(
-      content::GpuDataManager::GetInstance()->GpuAccessAllowed(nullptr));
+      content::GpuDataManager::GetInstance()->GpuAccessAllowed(nullptr) &&

[emaxx, 2017/03/02 16:06:29]

I'm not entirely following this. Is this changed because GpuAccessAllowed()
returns true even when SwiftShader is going to be used instead of the real GPU?
If that's the case, could it be possible to change the test to verify that GPU
is _actually_ not allowed? Without relying on this implicit interplay between
GpuAccessAllowed() and ShouldUseSwiftShader().

[sugoi1, 2017/03/02 16:23:16]

That's indeed the first line of GpuAccessAllowed():
https://cs.chromium.org/chromium/src/content/browser/gpu/gpu_data_manager_imp...
I'm honestly not sure how. Disabling the hardware GPU automatically enables
SwiftShader. I'll check if GpuAccessAllowed() can return false even if
SwiftShader is enabled without breaking other things.

[sugoi, 2017/03/06 18:31:42]

After looking into this, I think this code is better this way. Essentially,
GpuAccessAllowed() has never meant that the Hardware GPU was being used, it
meant that a GPU process is created. SwiftShader was already there before, but
because SwiftShader was Chrome branded only, the test never knew about
SwiftShader, which is not the experience that actual Chrome users would have
gotten. If you want to know that a real hardware GPU is being used, it seems
normal to check that no software renderer is running. I could add a function to
GpuDataManager that combines both checks, but it would only be used here.

[emaxx, 2017/03/06 20:10:51]

I see, thanks for the explanation.

I'd still find it more clear if this knowledge would be kept inside
GpuDataManager under special-purpose method (name something like like
"HardwareGpuAccessAllowed").
First, because it would be more visible in case of some changes on the side of
the GPU component - e.g. (just speculating) introducing a different software
engine apart from SwiftShader.
Second, because in the test here we want to verify that the hardware
acceleration is actually disabled, meanwhile the test now will pass if
ShouldUseSwiftShader() returns true, and that's regardless of the other state.
This is not equivalent (considering the potential case when SwiftShader is
disabled at runtime and an attempt to use real GPU takes place) and may mask
bugs.

But if you're opposed to this change, then, please, drop an explanatory comment
here in the test - something like that:

"The policy controls the state of the hardware GPU acceleration - it's not
applied to the state of the software emulation SwiftShader. But
GpuAccessAllowed() returns true when either the hardware acceleration or
SwiftShader are allowed. ShouldUseSwiftShader() returning true indicates that
SwiftShader is used instead of the hardware acceleration. Therefore the results
of these two methods are combined here in order to check that the hardware GPU
acceleration is not going to be used."

+      !content::GpuDataManager::GetInstance()->ShouldUseSwiftShader());
 }
 #endif  // !defined(OS_CHROMEOS) && !defined(OS_ANDROID)