Moving SwiftShader from component to bundled library

Moving SwiftShader from component to bundled library

Multiple things are modified here:
- The SwiftShader component was entirely removed
- The kSwiftShaderPath switch was removed, since SwiftShader
  will now be in a constant location
- SwiftShader files were added to FILES.cfg and chrome.release

BUG=630728
TBR=piman@chromium.org
TBR_REASON=This is only for changes in gpu_data_manager.h, which are trivial

Review-Url: https://codereview.chromium.org/2715563002
Cr-Commit-Position: refs/heads/master@{#456764}
Committed: https://chromium.googlesource.com/chromium/src/+/842efc6922b0cd66febd6f36f04318ebf396c184

[sugoi1, 2017-02-22 21:07:56]

sugoi@chromium.org changed reviewers:
+ grt@chromium.org, jbauman@chromium.org, kbr@chromium.org,
laforge@chromium.org, sebmarchand@chromium.org

[sugoi1, 2017-02-22 21:07:56]

I can split this in multiple cls if required, I just need help making sure I'm
doing this right.

[Ken Russell (switch to Gerrit), 2017-02-23 01:58:06]

https://codereview.chromium.org/2715563002/diff/1/chrome/tools/build/win/FILE...
File chrome/tools/build/win/FILES.cfg (right):

https://codereview.chromium.org/2715563002/diff/1/chrome/tools/build/win/FILE...
chrome/tools/build/win/FILES.cfg:395: },
There are conditions like BUILDFLAG(ENABLE_SWIFTSHADER) throughout the code but
I don't see similar conditions in the bundling files. What's the situation here?

[jbauman, 2017-02-23 02:06:42]

Does the browser build target have a dep on swiftshader yet?

Also have we made sure we're willing to take the download size hit for having
this always installed?

[sugoi, 2017-02-23 03:40:52]

On 2017/02/23 02:06:42, jbauman wrote:
> Does the browser build target have a dep on swiftshader yet?
I don't think so, but I can easily add one.
 
> Also have we made sure we're willing to take the download size hit for having
> this always installed?
The bundled library should be ~1MB, now that it's using Subzero as the JIT
compiler, instead of LLVM, and that size is supposed to be acceptable (It was
the main reason we switched from LLVM to Subzero).

[sugoi, 2017-02-23 03:41:31]

sugoi@google.com changed reviewers:
+ sugoi@google.com

[sugoi, 2017-02-23 03:41:31]

https://codereview.chromium.org/2715563002/diff/1/chrome/tools/build/win/FILE...
File chrome/tools/build/win/FILES.cfg (right):

https://codereview.chromium.org/2715563002/diff/1/chrome/tools/build/win/FILE...
chrome/tools/build/win/FILES.cfg:395: },
On 2017/02/23 01:58:05, Ken Russell wrote:
> There are conditions like BUILDFLAG(ENABLE_SWIFTSHADER) throughout the code
but
> I don't see similar conditions in the bundling files. What's the situation
here?

Not sure yet, I'm not super knowledgeable about how to configure this file. That
being said, this file is for Windows and ENABLE_SWIFTSHADER is defined as
"is_chrome_branded && is_win", which is what this file is used for, if I
understand correctly.

[Ken Russell (switch to Gerrit), 2017-02-23 03:52:39]

OK. LGTM from my standpoint. The PMs will have to give a thumbs-up on adding
this to the installer.

[laforge, 2017-02-23 04:18:48]

On 2017/02/23 03:52:39, Ken Russell wrote:
> OK. LGTM from my standpoint. The PMs will have to give a thumbs-up on adding
> this to the installer.

LGTM

Part of the calculus for shifting Flash player to be  distributed purely as a
component (i.e. completely decoupling it from the install payload - in Chrome
54) was that it gave us the opportunity to pull in components that were core to
the web platform (CDM and Swiftshader) since the size increase would be more
than offset by the size decrease of pulling Flash.

The caveat, at the time, was that SwiftShader (compressed) was too big to pull
into the product.  With the transition to using Subzero, bringing the size down
to 1MB, that brought it under the threshold (i.e. size wise now it's a wash).

So that it to say, we've been planning this size swap for a while, we're just
now able to realize it.

[jbauman, 2017-02-23 04:23:51]

On 2017/02/23 03:40:52, sugoi wrote:
> On 2017/02/23 02:06:42, jbauman wrote:
> > Does the browser build target have a dep on swiftshader yet?
> I don't think so, but I can easily add one.

Yeah, I think we need this.
>  
> > Also have we made sure we're willing to take the download size hit for
having
> > this always installed?
> The bundled library should be ~1MB, now that it's using Subzero as the JIT
> compiler, instead of LLVM, and that size is supposed to be acceptable (It was
> the main reason we switched from LLVM to Subzero).

Ok, sounds good.

[jbauman, 2017-02-23 04:30:45]

https://codereview.chromium.org/2715563002/diff/1/content/browser/gpu/gpu_dat...
File content/browser/gpu/gpu_data_manager_impl_private.cc (right):

https://codereview.chromium.org/2715563002/diff/1/content/browser/gpu/gpu_dat...
content/browser/gpu/gpu_data_manager_impl_private.cc:1235: if
(!base::CommandLine::ForCurrentProcess()->HasSwitch(
Check BUILDFLAG(ENABLE_SWIFTSHADER) here. Otherwise this will automatically
switch to using swiftshader even when it's disabled.

https://codereview.chromium.org/2715563002/diff/1/ui/gl/init/gl_initializer_w...
File ui/gl/init/gl_initializer_win.cc (right):

https://codereview.chromium.org/2715563002/diff/1/ui/gl/init/gl_initializer_w...
ui/gl/init/gl_initializer_win.cc:105: (use_gl ==
kGLImplementationSwiftShaderName) ||
This should return false if !BUILDFLAG(ENABLE_SWIFTSHADER) and
using_swift_shader is true. Otherwise the browser process could think it's using
swiftshader and the gpu process would ignore that and use D3D.

[grt (UTC plus 2), 2017-02-23 15:00:50]

https://codereview.chromium.org/2715563002/diff/1/chrome/installer/mini_insta...
File chrome/installer/mini_installer/chrome.release (right):

https://codereview.chromium.org/2715563002/diff/1/chrome/installer/mini_insta...
chrome/installer/mini_installer/chrome.release:79: swiftshader\libEGL.dll:
%(VersionDir)s\swiftshader\
are these only for is_chrome_branded=true builds? do you need the inclusion of
these files to be conditional on the BUILDFLAG?

https://codereview.chromium.org/2715563002/diff/1/chrome/tools/build/win/FILE...
File chrome/tools/build/win/FILES.cfg (right):

https://codereview.chromium.org/2715563002/diff/1/chrome/tools/build/win/FILE...
chrome/tools/build/win/FILES.cfg:387: 'filename': 'swiftshader/libEGL.dll',
are there pdb files for these as well?

[sugoi, 2017-02-23 15:22:55]

A few changes:
1) Moved swiftshader deps from "all" to "browser".
2) Removed "is_chrome_branded" for the ENABLE_SWIFTSHADER build flag.
SwiftShader is open source and should be available in chromium also.
3) Fixed ENABLE_SWIFTSHADER build flag related comments.

https://codereview.chromium.org/2715563002/diff/1/chrome/installer/mini_insta...
File chrome/installer/mini_installer/chrome.release (right):

https://codereview.chromium.org/2715563002/diff/1/chrome/installer/mini_insta...
chrome/installer/mini_installer/chrome.release:79: swiftshader\libEGL.dll:
%(VersionDir)s\swiftshader\
On 2017/02/23 15:00:50, grt (UTC plus 1) wrote:
> are these only for is_chrome_branded=true builds?
They could be used with any version, but is_chrome_branded would be good enough
for now. Just out of curiosity, does this file also create an installer for non
chrome branded versions?

> do you need the inclusion of these files to be conditional on the BUILDFLAG?
I think these should always be bundled on Windows. Note that these libraries are
only built for x86 or x64, but I think that covers what chrome supports on
Windows, right? (We have that restriction because of MIPS on Linux and I guess
ARM would also not work for SwiftShader at the moment).

https://codereview.chromium.org/2715563002/diff/1/chrome/tools/build/win/FILE...
File chrome/tools/build/win/FILES.cfg (right):

https://codereview.chromium.org/2715563002/diff/1/chrome/tools/build/win/FILE...
chrome/tools/build/win/FILES.cfg:387: 'filename': 'swiftshader/libEGL.dll',
On 2017/02/23 15:00:50, grt (UTC plus 1) wrote:
> are there pdb files for these as well?

Yes, I think so (I'm not in front of my Windows desktop at the moment, but I
think that there are pdb files generated in the out\Default\swiftshader
directory).

[sugoi, 2017-02-23 15:23:03]

The CQ bit was checked by sugoi@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-23 15:23:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-23 15:26:55]

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

[commit-bot: I haz the power, 2017-02-23 15:26:56]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[grt (UTC plus 2), 2017-02-23 15:33:39]

https://codereview.chromium.org/2715563002/diff/1/chrome/installer/mini_insta...
File chrome/installer/mini_installer/chrome.release (right):

https://codereview.chromium.org/2715563002/diff/1/chrome/installer/mini_insta...
chrome/installer/mini_installer/chrome.release:79: swiftshader\libEGL.dll:
%(VersionDir)s\swiftshader\
On 2017/02/23 15:22:55, sugoi wrote:
> On 2017/02/23 15:00:50, grt (UTC plus 1) wrote:
> > are these only for is_chrome_branded=true builds?
> They could be used with any version, but is_chrome_branded would be good
enough
> for now. Just out of curiosity, does this file also create an installer for
non
> chrome branded versions?

Yes. Since the BUILDFLAG is no longer conditional on is_chrome_branded=true,
it's set for all Windows builds, correct? If so, please move this new block so
that it immediately precedes [HIDPI].

> > do you need the inclusion of these files to be conditional on the BUILDFLAG?
> I think these should always be bundled on Windows. Note that these libraries
are
> only built for x86 or x64, but I think that covers what chrome supports on
> Windows, right? (We have that restriction because of MIPS on Linux and I guess
> ARM would also not work for SwiftShader at the moment).

Yup, Windows is x86 and x64 only.

https://codereview.chromium.org/2715563002/diff/1/chrome/tools/build/win/FILE...
File chrome/tools/build/win/FILES.cfg (right):

https://codereview.chromium.org/2715563002/diff/1/chrome/tools/build/win/FILE...
chrome/tools/build/win/FILES.cfg:387: 'filename': 'swiftshader/libEGL.dll',
On 2017/02/23 15:22:55, sugoi wrote:
> On 2017/02/23 15:00:50, grt (UTC plus 1) wrote:
> > are there pdb files for these as well?
> 
> Yes, I think so (I'm not in front of my Windows desktop at the moment, but I
> think that there are pdb files generated in the out\Default\swiftshader
> directory).

In that case, please add stanzas for the .pdbs as well (see chrome.dll.pdb for
an example).

https://codereview.chromium.org/2715563002/diff/20001/chrome/browser/BUILD.gn
File chrome/browser/BUILD.gn (right):

https://codereview.chromium.org/2715563002/diff/20001/chrome/browser/BUILD.gn...
chrome/browser/BUILD.gn:3609: deps += [ "//third_party/swiftshader" ]
is it //chrome/browser that depends on SS, or something down in //content?

[sugoi, 2017-02-23 15:36:34]

https://codereview.chromium.org/2715563002/diff/20001/chrome/browser/BUILD.gn
File chrome/browser/BUILD.gn (right):

https://codereview.chromium.org/2715563002/diff/20001/chrome/browser/BUILD.gn...
chrome/browser/BUILD.gn:3609: deps += [ "//third_party/swiftshader" ]
On 2017/02/23 15:33:38, grt (UTC plus 1) wrote:
> is it //chrome/browser that depends on SS, or something down in //content?

Oh yeah, it should be content/browser, not chrome/browser, will fix.

[Sébastien Marchand, 2017-02-23 16:09:34]

LGTM, please try this on the win-pgo trybot before submitting.

[sugoi, 2017-02-23 18:26:39]

The CQ bit was checked by sugoi@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-23 18:27:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-23 18:34:26]

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

[commit-bot: I haz the power, 2017-02-23 18:34:27]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sugoi, 2017-02-23 18:48:01]

The CQ bit was checked by sugoi@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-23 18:48:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-23 19:17:49]

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

[commit-bot: I haz the power, 2017-02-23 19:17:50]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)

[sugoi1, 2017-02-24 21:21:49]

The CQ bit was checked by sugoi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-24 21:22:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sugoi1, 2017-02-24 21:24:31]

More changes, jbauman@ PTAL.

- Modified EnableSwiftShaderIfNecessary so that use_swiftshader_ can also be
turned off when updating the GPU info
- Many fixes to GpuDataManager unittests to separate expected results between
platforms which support SwiftShader and those that do not.
- Added GPU_FEATURE_TYPE_WEBGL2 as a blacklisted feature for SwiftShader
- Added the .pdb files to FILES.cfg

[commit-bot: I haz the power, 2017-02-24 21:52:44]

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

[commit-bot: I haz the power, 2017-02-24 21:52:45]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)

[sugoi, 2017-02-24 22:14:01]

The CQ bit was checked by sugoi@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-24 22:14:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-25 00:28:54]

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

[commit-bot: I haz the power, 2017-02-25 00:28:56]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[jbauman, 2017-02-25 00:30:15]

https://codereview.chromium.org/2715563002/diff/100001/content/browser/gpu/gp...
File content/browser/gpu/gpu_data_manager_impl_private.cc (left):

https://codereview.chromium.org/2715563002/diff/100001/content/browser/gpu/gp...
content/browser/gpu/gpu_data_manager_impl_private.cc:674: if (use_swiftshader_)
I think we still need this so we don't try to update the GPU info using
swiftshader's info, as our real blacklisting information should happen based on
the real GPU. Though I think we should eventually change this to be more
explicit about rejecting an update from swiftshader.

[grt (UTC plus 2), 2017-02-27 09:31:56]

lgtm with some nits from me. please wait for seb to confirm that the duplicate
symbol name isn't an issue. thanks.

https://codereview.chromium.org/2715563002/diff/100001/chrome/tools/build/win...
File chrome/tools/build/win/FILES.cfg (right):

https://codereview.chromium.org/2715563002/diff/100001/chrome/tools/build/win...
chrome/tools/build/win/FILES.cfg:621: 'filename': 'swiftshader/libEGL.dll.pdb',
+sebmarchand: this has the same filename as the EGL dll above. do you know if
the archive process preserves the 'swiftshader' directory name?

https://codereview.chromium.org/2715563002/diff/100001/content/browser/gpu/gp...
File content/browser/gpu/gpu_data_manager_impl_private.cc (right):

https://codereview.chromium.org/2715563002/diff/100001/content/browser/gpu/gp...
content/browser/gpu/gpu_data_manager_impl_private.cc:300: feature ==
gpu::GPU_FEATURE_TYPE_WEBGL2)
nit: add braces since the condition now spans two lines. alternatively, simplify
to:
    return (feature == gpu::GPU_FEATURE_TYPE_ACCELERATED_2D_CANVAS ||
            feature == gpu::GPU_FEATURE_TYPE_WEBGL2);

https://codereview.chromium.org/2715563002/diff/100001/content/browser/gpu/gp...
content/browser/gpu/gpu_data_manager_impl_private.cc:1234:
(!GpuAccessAllowed(NULL) ||
nit: NULL -> nullptr

https://codereview.chromium.org/2715563002/diff/100001/ui/gl/init/gl_initiali...
File ui/gl/init/gl_initializer_win.cc (right):

https://codereview.chromium.org/2715563002/diff/100001/ui/gl/init/gl_initiali...
ui/gl/init/gl_initializer_win.cc:110: gles_path =
module_path.Append(L"swiftshader/");
nit: put these next three lines within #else/#endif so that you don't get a
warning/error from the compiler about unreachable code when !SS

[Sébastien Marchand, 2017-02-27 16:00:22]

https://codereview.chromium.org/2715563002/diff/100001/chrome/tools/build/win...
File chrome/tools/build/win/FILES.cfg (right):

https://codereview.chromium.org/2715563002/diff/100001/chrome/tools/build/win...
chrome/tools/build/win/FILES.cfg:621: 'filename': 'swiftshader/libEGL.dll.pdb',
On 2017/02/27 09:31:55, grt (UTC plus 1) wrote:
> +sebmarchand: this has the same filename as the EGL dll above. do you know if
> the archive process preserves the 'swiftshader' directory name?

We don't publish the symbols for the stuff not directly in |build_dir| (see
https://chrome-internal.googlesource.com/chrome/tools/build/+/master/scripts/...
, we don't process the sub directories), you'll need to do this explicitly (like
we do for the Syzygy stuff).

[sugoi, 2017-02-28 16:45:10]

I still have to land a modification to the symbols related script, as
sebmarchand@ pointed out.

https://codereview.chromium.org/2715563002/diff/1/chrome/installer/mini_insta...
File chrome/installer/mini_installer/chrome.release (right):

https://codereview.chromium.org/2715563002/diff/1/chrome/installer/mini_insta...
chrome/installer/mini_installer/chrome.release:79: swiftshader\libEGL.dll:
%(VersionDir)s\swiftshader\
On 2017/02/23 15:33:38, grt (UTC plus 1) wrote:
> On 2017/02/23 15:22:55, sugoi wrote:
> > On 2017/02/23 15:00:50, grt (UTC plus 1) wrote:
> > > are these only for is_chrome_branded=true builds?
> > They could be used with any version, but is_chrome_branded would be good
> enough
> > for now. Just out of curiosity, does this file also create an installer for
> non
> > chrome branded versions?
> 
> Yes. Since the BUILDFLAG is no longer conditional on is_chrome_branded=true,
> it's set for all Windows builds, correct? If so, please move this new block so
> that it immediately precedes [HIDPI].
> 
> > > do you need the inclusion of these files to be conditional on the
BUILDFLAG?
> > I think these should always be bundled on Windows. Note that these libraries
> are
> > only built for x86 or x64, but I think that covers what chrome supports on
> > Windows, right? (We have that restriction because of MIPS on Linux and I
guess
> > ARM would also not work for SwiftShader at the moment).
> 
> Yup, Windows is x86 and x64 only.

Done.

https://codereview.chromium.org/2715563002/diff/1/content/browser/gpu/gpu_dat...
File content/browser/gpu/gpu_data_manager_impl_private.cc (right):

https://codereview.chromium.org/2715563002/diff/1/content/browser/gpu/gpu_dat...
content/browser/gpu/gpu_data_manager_impl_private.cc:1235: if
(!base::CommandLine::ForCurrentProcess()->HasSwitch(
On 2017/02/23 04:30:45, jbauman wrote:
> Check BUILDFLAG(ENABLE_SWIFTSHADER) here. Otherwise this will automatically
> switch to using swiftshader even when it's disabled.

Done.

https://codereview.chromium.org/2715563002/diff/1/ui/gl/init/gl_initializer_w...
File ui/gl/init/gl_initializer_win.cc (right):

https://codereview.chromium.org/2715563002/diff/1/ui/gl/init/gl_initializer_w...
ui/gl/init/gl_initializer_win.cc:105: (use_gl ==
kGLImplementationSwiftShaderName) ||
On 2017/02/23 04:30:45, jbauman wrote:
> This should return false if !BUILDFLAG(ENABLE_SWIFTSHADER) and
> using_swift_shader is true. Otherwise the browser process could think it's
using
> swiftshader and the gpu process would ignore that and use D3D.

Done.

https://codereview.chromium.org/2715563002/diff/100001/chrome/tools/build/win...
File chrome/tools/build/win/FILES.cfg (right):

https://codereview.chromium.org/2715563002/diff/100001/chrome/tools/build/win...
chrome/tools/build/win/FILES.cfg:621: 'filename': 'swiftshader/libEGL.dll.pdb',
On 2017/02/27 16:00:22, Sébastien Marchand wrote:
> On 2017/02/27 09:31:55, grt (UTC plus 1) wrote:
> > +sebmarchand: this has the same filename as the EGL dll above. do you know
if
> > the archive process preserves the 'swiftshader' directory name?
> 
> We don't publish the symbols for the stuff not directly in |build_dir| (see
>
https://chrome-internal.googlesource.com/chrome/tools/build/+/master/scripts/...
> , we don't process the sub directories), you'll need to do this explicitly
(like
> we do for the Syzygy stuff).

Will add a special case for SwiftShader in that script

https://codereview.chromium.org/2715563002/diff/100001/content/browser/gpu/gp...
File content/browser/gpu/gpu_data_manager_impl_private.cc (left):

https://codereview.chromium.org/2715563002/diff/100001/content/browser/gpu/gp...
content/browser/gpu/gpu_data_manager_impl_private.cc:674: if (use_swiftshader_)
On 2017/02/25 00:30:15, jbauman wrote:
> I think we still need this so we don't try to update the GPU info using
> swiftshader's info, as our real blacklisting information should happen based
on
> the real GPU. Though I think we should eventually change this to be more
> explicit about rejecting an update from swiftshader.

Done.

https://codereview.chromium.org/2715563002/diff/100001/content/browser/gpu/gp...
File content/browser/gpu/gpu_data_manager_impl_private.cc (right):

https://codereview.chromium.org/2715563002/diff/100001/content/browser/gpu/gp...
content/browser/gpu/gpu_data_manager_impl_private.cc:300: feature ==
gpu::GPU_FEATURE_TYPE_WEBGL2)
On 2017/02/27 09:31:55, grt (UTC plus 1) wrote:
> nit: add braces since the condition now spans two lines. alternatively,
simplify
> to:
>     return (feature == gpu::GPU_FEATURE_TYPE_ACCELERATED_2D_CANVAS ||
>             feature == gpu::GPU_FEATURE_TYPE_WEBGL2);

Done.

https://codereview.chromium.org/2715563002/diff/100001/content/browser/gpu/gp...
content/browser/gpu/gpu_data_manager_impl_private.cc:1234:
(!GpuAccessAllowed(NULL) ||
On 2017/02/27 09:31:55, grt (UTC plus 1) wrote:
> nit: NULL -> nullptr

Done.

https://codereview.chromium.org/2715563002/diff/100001/ui/gl/init/gl_initiali...
File ui/gl/init/gl_initializer_win.cc (right):

https://codereview.chromium.org/2715563002/diff/100001/ui/gl/init/gl_initiali...
ui/gl/init/gl_initializer_win.cc:110: gles_path =
module_path.Append(L"swiftshader/");
On 2017/02/27 09:31:56, grt (UTC plus 1) wrote:
> nit: put these next three lines within #else/#endif so that you don't get a
> warning/error from the compiler about unreachable code when !SS

Done. Also did a similar fix in the x11 path.

[sugoi, 2017-02-28 16:46:12]

The CQ bit was checked by sugoi@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-28 16:46:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-28 18:39:31]

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

[commit-bot: I haz the power, 2017-02-28 18:39:32]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[grt (UTC plus 2), 2017-02-28 19:51:29]

still lgtm w/ one more nit

https://codereview.chromium.org/2715563002/diff/120001/content/browser/gpu/gp...
File content/browser/gpu/gpu_data_manager_impl.h (right):

https://codereview.chromium.org/2715563002/diff/120001/content/browser/gpu/gp...
content/browser/gpu/gpu_data_manager_impl.h:15: #include
"base/files/file_path.h"
unused

[jbauman, 2017-03-01 00:34:29]

lgtm. Once you commit this we should check GPU.EGLDisplayType and ensure it
doesn't change too much.

[sugoi1, 2017-03-01 16:50:22]

https://codereview.chromium.org/2715563002/diff/120001/content/browser/gpu/gp...
File content/browser/gpu/gpu_data_manager_impl_private.cc (right):

https://codereview.chromium.org/2715563002/diff/120001/content/browser/gpu/gp...
content/browser/gpu/gpu_data_manager_impl_private.cc:1235: if
(!GpuAccessAllowed(nullptr) ||
This call to GpuAccessAllowed() causes SwiftShader to be enabled too often.
This is due to:
https://cs.chromium.org/chromium/src/content/browser/gpu/gpu_data_manager_imp...

Because EnableSwiftShaderIfNecessary() is called within UpdateGpuInfo(),
GpuAccessAllowed() can return false simply because
UpdatePreliminaryBlacklistedFeatures() hasn't been called yet. I don't think
GpuAccessAllowed() is necessary here, correct me if I'm wrong.

I'll remove it so that SwiftShader is only enabled when WebGL is explictly
blacklisted, which should be enough for now.

[sugoi, 2017-03-01 18:40:30]

The CQ bit was checked by sugoi@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-01 18:40:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sugoi, 2017-03-01 19:45:08]

sugoi@google.com changed reviewers:
+ piman@chromium.org, sugoi@chromium.org, thakis@chromium.org

[sugoi, 2017-03-01 19:45:09]

Adding thakis@ for the changes in chrome/browser.

[sugoi, 2017-03-01 19:46:40]

Description was changed from

==========
Moving SwiftShader from component to bundled library

Multiple things are modified here and I can split this in
multiple cls if required by the reviewers:
- The SwiftShader component was entirely removed
- The kSwiftShaderPath switch was removed, since SwiftShader
  will now be in a constant location
- SwiftShader files were added to FILES.cfg and chrome.release

BUG=630728
==========

to

==========
Moving SwiftShader from component to bundled library

Multiple things are modified here and I can split this in
multiple cls if required by the reviewers:
- The SwiftShader component was entirely removed
- The kSwiftShaderPath switch was removed, since SwiftShader
  will now be in a constant location
- SwiftShader files were added to FILES.cfg and chrome.release

BUG=630728
TBR=piman@chromium.org
TBR_REASON=This is only for changes in gpu_data_manager.h, which are trivial
==========

[commit-bot: I haz the power, 2017-03-01 20:47:43]

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

[commit-bot: I haz the power, 2017-03-01 20:47:45]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[jbauman, 2017-03-01 22:37:04]

On 2017/03/01 16:50:22, sugoi1 wrote:
>
https://codereview.chromium.org/2715563002/diff/120001/content/browser/gpu/gp...
> File content/browser/gpu/gpu_data_manager_impl_private.cc (right):
> 
>
https://codereview.chromium.org/2715563002/diff/120001/content/browser/gpu/gp...
> content/browser/gpu/gpu_data_manager_impl_private.cc:1235: if
> (!GpuAccessAllowed(nullptr) ||
> This call to GpuAccessAllowed() causes SwiftShader to be enabled too often.
> This is due to:
>
https://cs.chromium.org/chromium/src/content/browser/gpu/gpu_data_manager_imp...
> 
> Because EnableSwiftShaderIfNecessary() is called within UpdateGpuInfo(),
> GpuAccessAllowed() can return false simply because
> UpdatePreliminaryBlacklistedFeatures() hasn't been called yet. I don't think
> GpuAccessAllowed() is necessary here, correct me if I'm wrong.
> 
> I'll remove it so that SwiftShader is only enabled when WebGL is explictly
> blacklisted, which should be enough for now.

This is necessary if the GPU isn't available but isn't blacklisted. For example
if it failed to launch, crashed three times, or is blocked because the set of
preliminarily blacklisted features is different from the full set of blacklisted
features.

[Nico, 2017-03-01 22:41:00]

Please get a reviewer from c/b/extensions/OWNERS and c/b/policy/OWNERS for the
test changes. Especially the policy change looks like it needs eyes from someone
with domain expertise.

[sugoi1, 2017-03-01 23:41:17]

sugoi@chromium.org changed reviewers:
+ asargent@chromium.org, emaxx@chromium.org, rdevlin.cronin@chromium.org
- sugoi@google.com

[sugoi1, 2017-03-01 23:41:18]

Adding emaxx@ for c/b/policy
Adding asargent@ for c/b/extensions/api/webstore_private
Adding rdevlin.cronin@ for c/b/extensions

[Devlin, 2017-03-01 23:47:24]

https://codereview.chromium.org/2715563002/diff/140001/chrome/browser/extensi...
File chrome/browser/extensions/api/webstore_private/webstore_private_apitest.cc
(right):

https://codereview.chromium.org/2715563002/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/api/webstore_private/webstore_private_apitest.cc:435:
if (content::GpuDataManager::GetInstance()->ShouldUseSwiftShader()) {
It seems strange that this change means the GpuDataManager no longer respects
the blacklist - why is that?

[sugoi1, 2017-03-02 00:17:08]

https://codereview.chromium.org/2715563002/diff/140001/chrome/browser/extensi...
File chrome/browser/extensions/api/webstore_private/webstore_private_apitest.cc
(right):

https://codereview.chromium.org/2715563002/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/api/webstore_private/webstore_private_apitest.cc:435:
if (content::GpuDataManager::GetInstance()->ShouldUseSwiftShader()) {
On 2017/03/01 23:47:24, Devlin wrote:
> It seems strange that this change means the GpuDataManager no longer respects
> the blacklist - why is that?

When WebGL is blacklisted on a given GPU, SwiftShader is automatically enabled
and is used to render WebGL. WebGL will now be available on 100% of Windows
machines, so it can't be disabled.

[emaxx, 2017-03-02 16:06:30]

https://codereview.chromium.org/2715563002/diff/140001/chrome/browser/policy/...
File chrome/browser/policy/policy_browsertest.cc (right):

https://codereview.chromium.org/2715563002/diff/140001/chrome/browser/policy/...
chrome/browser/policy/policy_browsertest.cc:4081:
content::GpuDataManager::GetInstance()->GpuAccessAllowed(nullptr) &&
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:17]

https://codereview.chromium.org/2715563002/diff/140001/chrome/browser/policy/...
File chrome/browser/policy/policy_browsertest.cc (right):

https://codereview.chromium.org/2715563002/diff/140001/chrome/browser/policy/...
chrome/browser/policy/policy_browsertest.cc:4081:
content::GpuDataManager::GetInstance()->GpuAccessAllowed(nullptr) &&
On 2017/03/02 16:06:29, emaxx wrote:
> 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?

That's indeed the first line of GpuAccessAllowed():
https://cs.chromium.org/chromium/src/content/browser/gpu/gpu_data_manager_imp...

> 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().

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.

[sugoi1, 2017-03-02 19:03:08]

sugoi@chromium.org changed reviewers:
- asargent@chromium.org

[sugoi1, 2017-03-02 19:03:09]

Removing asargent@ from the reviewers list since he no longer works on Chrome.

[Devlin, 2017-03-02 21:59:29]

https://codereview.chromium.org/2715563002/diff/140001/chrome/browser/extensi...
File chrome/browser/extensions/api/webstore_private/webstore_private_apitest.cc
(right):

https://codereview.chromium.org/2715563002/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/api/webstore_private/webstore_private_apitest.cc:436:
EXPECT_FALSE(content::GpuDataManager::GetInstance()->IsFeatureBlacklisted(
nit: How about
content::GpuDataManager* gpu_manager = content::GpuDataManager::GetInstance();
EXPECT_EQ(gpu_manager->ShouldUseSwiftShader(),
          gpu_manager->IsFeatureBlacklisted(gpu::GPU_FEATURE_TYPE_WEBGL);

https://codereview.chromium.org/2715563002/diff/140001/chrome/browser/extensi...
File chrome/browser/extensions/extension_service_unittest.cc (right):

https://codereview.chromium.org/2715563002/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/extension_service_unittest.cc:4610:
EXPECT_TRUE(service()->IsExtensionEnabled(id));
Unfortunately, this defeats the purpose of the test, which is checking what
happens when requirements aren't satisfied.  If they are, then we didn't test
anything. :)

https://codereview.chromium.org/2715563002/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/extension_service_unittest.cc:4696: EXPECT_EQ(0u,
GetErrors().size());
ditto here

[Nico, 2017-03-03 15:54:13]

I guess the question here is if there are other good reasons for disabling
webgl from a policy or extension, other than that it needs GPU access. My
gut says "yes" and making "webgl disabled" mean "use swiftshader instead of
gpu for webgl" seems pretty surprising to me. Are all stakeholders aware of
this change in behavior?

On Thu, Mar 2, 2017 at 4:59 PM, <rdevlin.cronin@chromium.org> wrote:

>
> https://codereview.chromium.org/2715563002/diff/140001/
> chrome/browser/extensions/api/webstore_private/webstore_private_apitest.cc
> File
> chrome/browser/extensions/api/webstore_private/webstore_private_apitest.cc
> (right):
>
> https://codereview.chromium.org/2715563002/diff/140001/
> chrome/browser/extensions/api/webstore_private/webstore_
> private_apitest.cc#newcode436
> chrome/browser/extensions/api/webstore_private/webstore_
> private_apitest.cc:436:
> EXPECT_FALSE(content::GpuDataManager::GetInstance()->IsFeatureBlacklisted(
> nit: How about
> content::GpuDataManager* gpu_manager =
> content::GpuDataManager::GetInstance();
> EXPECT_EQ(gpu_manager->ShouldUseSwiftShader(),
>
> gpu_manager->IsFeatureBlacklisted(gpu::GPU_FEATURE_TYPE_WEBGL);
>
> https://codereview.chromium.org/2715563002/diff/140001/
> chrome/browser/extensions/extension_service_unittest.cc
> File chrome/browser/extensions/extension_service_unittest.cc (right):
>
> https://codereview.chromium.org/2715563002/diff/140001/
> chrome/browser/extensions/extension_service_unittest.cc#newcode4610
> chrome/browser/extensions/extension_service_unittest.cc:4610:
> EXPECT_TRUE(service()->IsExtensionEnabled(id));
> Unfortunately, this defeats the purpose of the test, which is checking
> what happens when requirements aren't satisfied. If they are, then we
> didn't test anything. :)
>
> https://codereview.chromium.org/2715563002/diff/140001/
> chrome/browser/extensions/extension_service_unittest.cc#newcode4696
> chrome/browser/extensions/extension_service_unittest.cc:4696:
> EXPECT_EQ(0u, GetErrors().size());
> ditto here
>
> https://codereview.chromium.org/2715563002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sugoi1, 2017-03-03 16:11:45]

On 2017/03/03 15:54:13, Nico wrote:
> I guess the question here is if there are other good reasons for disabling
> webgl from a policy or extension, other than that it needs GPU access. My
> gut says "yes" and making "webgl disabled" mean "use swiftshader instead of
> gpu for webgl" seems pretty surprising to me. Are all stakeholders aware of
> this change in behavior?

Well, the main goal of the SwiftShader integration is to be able to tell Web
Developers that they can use WebGL and that it will always be available.
If it's important that WebGL be sometimes disabled (I honestly can't see why
that would be the case, but if anyone has an example of a case where it would be
important, I'll be glad to look into it), then I'll need a way to distinguish
blacklisted features between "unsupported by the hardware" and "requested
through some API".
 
> On Thu, Mar 2, 2017 at 4:59 PM, <mailto:rdevlin.cronin@chromium.org> wrote:
> 
> >
> > https://codereview.chromium.org/2715563002/diff/140001/
> > chrome/browser/extensions/api/webstore_private/webstore_private_apitest.cc
> > File
> > chrome/browser/extensions/api/webstore_private/webstore_private_apitest.cc
> > (right):
> >
> > https://codereview.chromium.org/2715563002/diff/140001/
> > chrome/browser/extensions/api/webstore_private/webstore_
> > private_apitest.cc#newcode436
> > chrome/browser/extensions/api/webstore_private/webstore_
> > private_apitest.cc:436:
> > EXPECT_FALSE(content::GpuDataManager::GetInstance()->IsFeatureBlacklisted(
> > nit: How about
> > content::GpuDataManager* gpu_manager =
> > content::GpuDataManager::GetInstance();
> > EXPECT_EQ(gpu_manager->ShouldUseSwiftShader(),
> >
> > gpu_manager->IsFeatureBlacklisted(gpu::GPU_FEATURE_TYPE_WEBGL);
> >
> > https://codereview.chromium.org/2715563002/diff/140001/
> > chrome/browser/extensions/extension_service_unittest.cc
> > File chrome/browser/extensions/extension_service_unittest.cc (right):
> >
> > https://codereview.chromium.org/2715563002/diff/140001/
> > chrome/browser/extensions/extension_service_unittest.cc#newcode4610
> > chrome/browser/extensions/extension_service_unittest.cc:4610:
> > EXPECT_TRUE(service()->IsExtensionEnabled(id));
> > Unfortunately, this defeats the purpose of the test, which is checking
> > what happens when requirements aren't satisfied. If they are, then we
> > didn't test anything. :)
> >
> > https://codereview.chromium.org/2715563002/diff/140001/
> > chrome/browser/extensions/extension_service_unittest.cc#newcode4696
> > chrome/browser/extensions/extension_service_unittest.cc:4696:
> > EXPECT_EQ(0u, GetErrors().size());
> > ditto here
> >
> > https://codereview.chromium.org/2715563002/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[emaxx, 2017-03-03 16:42:52]

On 2017/03/03 16:11:45, sugoi1 wrote:
> On 2017/03/03 15:54:13, Nico wrote:
> > I guess the question here is if there are other good reasons for disabling
> > webgl from a policy or extension, other than that it needs GPU access. My
> > gut says "yes" and making "webgl disabled" mean "use swiftshader instead of
> > gpu for webgl" seems pretty surprising to me. Are all stakeholders aware of
> > this change in behavior?
> 
> Well, the main goal of the SwiftShader integration is to be able to tell Web
> Developers that they can use WebGL and that it will always be available.
> If it's important that WebGL be sometimes disabled (I honestly can't see why
> that would be the case, but if anyone has an example of a case where it would
be
> important, I'll be glad to look into it), then I'll need a way to distinguish
> blacklisted features between "unsupported by the hardware" and "requested
> through some API".
>  

Looking at http://crbug.com/337713 that tracked adding of the
HardwareAccelerationModeEnabled admin policy, it seems that the motivation
behind it was:
1) dealing with buggy hardware (causing black screen etc.);
2) dealing with very high CPU usage in virtualized environment.

So I guess #2 is important for deciding on whether SwiftShader should be
subject to the policy. Is it possible for the software renderer to cause
excessively high CPU load in typical usage patterns when working under
VM?


Another thought: looking at that bug, the policy was considered by many as
a way for enterprises to trigger the effect of the "--disable-gpu" command
line flag. Does this command line flag disable SwiftShader too?


On 2017/03/02 16:23:17, sugoi1 wrote:
> That's indeed the first line of GpuAccessAllowed():
>
https://cs.chromium.org/chromium/src/content/browser/gpu/gpu_data_manager_imp...

I saw this, but I was trying to say that this behavior seems rather
unexpected from interface perspective - the method name seems misleading
to me.

> 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.

I'm not familiar with that code, but what about splitting this method
into two - one is _truely_ about hardware access, and other about GPU in
total (either hardware accelerated or software emulated)? Then the test
(and the other callsites) could be made to verify exactly what it wants to.

[sugoi1, 2017-03-03 16:56:05]

On 2017/03/03 16:42:52, emaxx wrote:
> On 2017/03/03 16:11:45, sugoi1 wrote:
> > On 2017/03/03 15:54:13, Nico wrote:
> > > I guess the question here is if there are other good reasons for disabling
> > > webgl from a policy or extension, other than that it needs GPU access. My
> > > gut says "yes" and making "webgl disabled" mean "use swiftshader instead
of
> > > gpu for webgl" seems pretty surprising to me. Are all stakeholders aware
of
> > > this change in behavior?
> > 
> > Well, the main goal of the SwiftShader integration is to be able to tell Web
> > Developers that they can use WebGL and that it will always be available.
> > If it's important that WebGL be sometimes disabled (I honestly can't see why
> > that would be the case, but if anyone has an example of a case where it
would
> be
> > important, I'll be glad to look into it), then I'll need a way to
distinguish
> > blacklisted features between "unsupported by the hardware" and "requested
> > through some API".
> >  
> 
> Looking at http://crbug.com/337713 that tracked adding of the
> HardwareAccelerationModeEnabled admin policy, it seems that the motivation
> behind it was:
> 1) dealing with buggy hardware (causing black screen etc.);
> 2) dealing with very high CPU usage in virtualized environment.
> 
> So I guess #2 is important for deciding on whether SwiftShader should be
> subject to the policy. Is it possible for the software renderer to cause
> excessively high CPU load in typical usage patterns when working under
> VM?
> 
> 
> Another thought: looking at that bug, the policy was considered by many as
> a way for enterprises to trigger the effect of the "--disable-gpu" command
> line flag. Does this command line flag disable SwiftShader too?

--disable-gpu is how you enable SwiftShader

> On 2017/03/02 16:23:17, sugoi1 wrote:
> > That's indeed the first line of GpuAccessAllowed():
> >
>
https://cs.chromium.org/chromium/src/content/browser/gpu/gpu_data_manager_imp...
> 
> I saw this, but I was trying to say that this behavior seems rather
> unexpected from interface perspective - the method name seems misleading
> to me.

AFAIK, GpuAccessAllowed means that chrome has a GPU process, which it does when
using SwiftShader.

> > 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.
> 
> I'm not familiar with that code, but what about splitting this method
> into two - one is _truely_ about hardware access, and other about GPU in
> total (either hardware accelerated or software emulated)? Then the test
> (and the other callsites) could be made to verify exactly what it wants to.

Yes, I think it has to be done that avoid issues from modifying the current
meaning of GpuAccessAllowed().

[sugoi1, 2017-03-03 16:57:01]

On 2017/03/03 16:56:05, sugoi1 wrote:
> On 2017/03/03 16:42:52, emaxx wrote:
> > On 2017/03/03 16:11:45, sugoi1 wrote:
> > > On 2017/03/03 15:54:13, Nico wrote:
> > > > I guess the question here is if there are other good reasons for
disabling
> > > > webgl from a policy or extension, other than that it needs GPU access.
My
> > > > gut says "yes" and making "webgl disabled" mean "use swiftshader instead
> of
> > > > gpu for webgl" seems pretty surprising to me. Are all stakeholders aware
> of
> > > > this change in behavior?
> > > 
> > > Well, the main goal of the SwiftShader integration is to be able to tell
Web
> > > Developers that they can use WebGL and that it will always be available.
> > > If it's important that WebGL be sometimes disabled (I honestly can't see
why
> > > that would be the case, but if anyone has an example of a case where it
> would
> > be
> > > important, I'll be glad to look into it), then I'll need a way to
> distinguish
> > > blacklisted features between "unsupported by the hardware" and "requested
> > > through some API".
> > >  
> > 
> > Looking at http://crbug.com/337713 that tracked adding of the
> > HardwareAccelerationModeEnabled admin policy, it seems that the motivation
> > behind it was:
> > 1) dealing with buggy hardware (causing black screen etc.);
> > 2) dealing with very high CPU usage in virtualized environment.
> > 
> > So I guess #2 is important for deciding on whether SwiftShader should be
> > subject to the policy. Is it possible for the software renderer to cause
> > excessively high CPU load in typical usage patterns when working under
> > VM?
> > 
> > 
> > Another thought: looking at that bug, the policy was considered by many as
> > a way for enterprises to trigger the effect of the "--disable-gpu" command
> > line flag. Does this command line flag disable SwiftShader too?
> 
> --disable-gpu is how you enable SwiftShader
> 
> > On 2017/03/02 16:23:17, sugoi1 wrote:
> > > That's indeed the first line of GpuAccessAllowed():
> > >
> >
>
https://cs.chromium.org/chromium/src/content/browser/gpu/gpu_data_manager_imp...
> > 
> > I saw this, but I was trying to say that this behavior seems rather
> > unexpected from interface perspective - the method name seems misleading
> > to me.
> 
> AFAIK, GpuAccessAllowed means that chrome has a GPU process, which it does
when
> using SwiftShader.
> 
> > > 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.
> > 
> > I'm not familiar with that code, but what about splitting this method
> > into two - one is _truely_ about hardware access, and other about GPU in
> > total (either hardware accelerated or software emulated)? Then the test
> > (and the other callsites) could be made to verify exactly what it wants to.
> 
* Yes, I think it has to be done that way in order to avoid issues from
modifying the current meaning of GpuAccessAllowed().

[sugoi, 2017-03-06 18:08:38]

The CQ bit was checked by sugoi@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-06 18:08:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

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

Hi rdevlin.cronin@ and emaxx@,

  I've looked into the issues you pointed out and I can't figure out better
fixes than the ones I already proposed. If you feel that there would be better
fixes, I'd be glad to discuss it with you, possibly over VC, but as far as I can
see, the changes proposed here are working as intended.

https://codereview.chromium.org/2715563002/diff/140001/chrome/browser/extensi...
File chrome/browser/extensions/api/webstore_private/webstore_private_apitest.cc
(right):

https://codereview.chromium.org/2715563002/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/api/webstore_private/webstore_private_apitest.cc:436:
EXPECT_FALSE(content::GpuDataManager::GetInstance()->IsFeatureBlacklisted(
On 2017/03/02 21:59:29, Devlin wrote:
> nit: How about
> content::GpuDataManager* gpu_manager = content::GpuDataManager::GetInstance();
> EXPECT_EQ(gpu_manager->ShouldUseSwiftShader(),
>           gpu_manager->IsFeatureBlacklisted(gpu::GPU_FEATURE_TYPE_WEBGL);
>           

Done.

https://codereview.chromium.org/2715563002/diff/140001/chrome/browser/extensi...
File chrome/browser/extensions/extension_service_unittest.cc (right):

https://codereview.chromium.org/2715563002/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/extension_service_unittest.cc:4610:
EXPECT_TRUE(service()->IsExtensionEnabled(id));
On 2017/03/02 21:59:29, Devlin wrote:
> Unfortunately, this defeats the purpose of the test, which is checking what
> happens when requirements aren't satisfied.  If they are, then we didn't test
> anything. :)

I've looked into this, and I still think this is the proper fix. The
BlackListWebGL() function installs GPU info where WebGL is blacklisted, which,
as of this cl, has the effect of enabling SwiftShader, which is the desired
behavior. If I were to make a testing specific fix, it would not represent
reality.

The test is still useful IMHO, since it verifies that, when SwiftShader is
enabled, WebGL doesn't get disabled. Also note that, currently, SwiftShader is
only being enabled on Windows, so the test still behaves as usual on other
platforms.

https://codereview.chromium.org/2715563002/diff/140001/chrome/browser/policy/...
File chrome/browser/policy/policy_browsertest.cc (right):

https://codereview.chromium.org/2715563002/diff/140001/chrome/browser/policy/...
chrome/browser/policy/policy_browsertest.cc:4081:
content::GpuDataManager::GetInstance()->GpuAccessAllowed(nullptr) &&
On 2017/03/02 16:23:16, sugoi1 wrote:
> On 2017/03/02 16:06:29, emaxx wrote:
> > 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?
> 
> That's indeed the first line of GpuAccessAllowed():
>
https://cs.chromium.org/chromium/src/content/browser/gpu/gpu_data_manager_imp...
> 
> > 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().
> 
> 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.

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.

[sugoi1, 2017-03-06 18:49:47]

sugoi@chromium.org changed reviewers:
+ vmiura@chromium.org
- sugoi@google.com

[sugoi1, 2017-03-06 19:12:01]

The CQ bit was checked by sugoi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-06 19:12:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

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

https://codereview.chromium.org/2715563002/diff/140001/chrome/browser/policy/...
File chrome/browser/policy/policy_browsertest.cc (right):

https://codereview.chromium.org/2715563002/diff/140001/chrome/browser/policy/...
chrome/browser/policy/policy_browsertest.cc:4081:
content::GpuDataManager::GetInstance()->GpuAccessAllowed(nullptr) &&
On 2017/03/06 18:31:42, sugoi wrote:
> On 2017/03/02 16:23:16, sugoi1 wrote:
> > On 2017/03/02 16:06:29, emaxx wrote:
> > > 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?
> > 
> > That's indeed the first line of GpuAccessAllowed():
> >
>
https://cs.chromium.org/chromium/src/content/browser/gpu/gpu_data_manager_imp...
> > 
> > > 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().
> > 
> > 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.
> 
> 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.

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."

[sugoi1, 2017-03-06 20:40:24]

On 2017/03/06 20:10:51, emaxx wrote:
>
https://codereview.chromium.org/2715563002/diff/140001/chrome/browser/policy/...
> File chrome/browser/policy/policy_browsertest.cc (right):
> 
>
https://codereview.chromium.org/2715563002/diff/140001/chrome/browser/policy/...
> chrome/browser/policy/policy_browsertest.cc:4081:
> content::GpuDataManager::GetInstance()->GpuAccessAllowed(nullptr) &&
> On 2017/03/06 18:31:42, sugoi wrote:
> > On 2017/03/02 16:23:16, sugoi1 wrote:
> > > On 2017/03/02 16:06:29, emaxx wrote:
> > > > 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?
> > > 
> > > That's indeed the first line of GpuAccessAllowed():
> > >
> >
>
https://cs.chromium.org/chromium/src/content/browser/gpu/gpu_data_manager_imp...
> > > 
> > > > 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().
> > > 
> > > 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.
> > 
> > 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.
> 
> 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."

I'm not strongly opposed to making the change, but in order to avoid making too
many changes in this cl, I'll add the comment you suggested. If you would prefer
getting a function specifically for this test, similar to the
HardwareGpuAccessAllowed function you proposed, I will gladly do it in a follow
up to this cl, once this has landed and passes tests on all bots. I just want to
land this ASAP in order to get as much coverage as possible before M59. Thanks.

[emaxx, 2017-03-06 20:50:29]

On 2017/03/06 20:40:24, sugoi1 wrote:
> I'm not strongly opposed to making the change, but in order to avoid making
too
> many changes in this cl, I'll add the comment you suggested. If you would
prefer
> getting a function specifically for this test, similar to the
> HardwareGpuAccessAllowed function you proposed, I will gladly do it in a
follow
> up to this cl, once this has landed and passes tests on all bots. I just want
to
> land this ASAP in order to get as much coverage as possible before M59.
Thanks.

LGTM

[Devlin, 2017-03-06 22:58:10]

https://codereview.chromium.org/2715563002/diff/140001/chrome/browser/extensi...
File chrome/browser/extensions/extension_service_unittest.cc (right):

https://codereview.chromium.org/2715563002/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/extension_service_unittest.cc:4610:
EXPECT_TRUE(service()->IsExtensionEnabled(id));
On 2017/03/06 18:31:42, sugoi wrote:
> On 2017/03/02 21:59:29, Devlin wrote:
> > Unfortunately, this defeats the purpose of the test, which is checking what
> > happens when requirements aren't satisfied.  If they are, then we didn't
test
> > anything. :)
> 
> I've looked into this, and I still think this is the proper fix. The
> BlackListWebGL() function installs GPU info where WebGL is blacklisted, which,
> as of this cl, has the effect of enabling SwiftShader, which is the desired
> behavior. If I were to make a testing specific fix, it would not represent
> reality.
> 
> The test is still useful IMHO, since it verifies that, when SwiftShader is
> enabled, WebGL doesn't get disabled. Also note that, currently, SwiftShader is
> only being enabled on Windows, so the test still behaves as usual on other
> platforms.

The thing is, this isn't actually a GPU test, WebGL test, swiftshader test, etc.
 The behavior it's testing is that if an extension is disabled because of
unsupported requirements, and then updates and no longer has unsupported
requirements, it's automatically re-enabled. (See also the comment on line
4587.)  Disabling WebGL was an easy way of triggering an unsupported
requirement, because it's entirely cross-platform and was easy to do from a
unittest, but it's not the sole purpose of the test.  The way it's rewritten
now, we don't actually test the flow that we're supposed to be exercising, so we
lose test coverage.

If we can no longer use webgl as an unavailable requirement, we'll have to
substitute something else or find a test-only way of marking a requirement as
unavailable.

[grt (UTC plus 2), 2017-03-07 09:49:27]

nit: Please remove "and I can split this in
multiple cls if required by the reviewers" from the CL description. While that's
a useful comment for reviewers, it isn't relevant to the history of this CL once
it lands.

[sugoi1, 2017-03-07 13:08:54]

Description was changed from

==========
Moving SwiftShader from component to bundled library

Multiple things are modified here and I can split this in
multiple cls if required by the reviewers:
- The SwiftShader component was entirely removed
- The kSwiftShaderPath switch was removed, since SwiftShader
  will now be in a constant location
- SwiftShader files were added to FILES.cfg and chrome.release

BUG=630728
TBR=piman@chromium.org
TBR_REASON=This is only for changes in gpu_data_manager.h, which are trivial
==========

to

==========
Moving SwiftShader from component to bundled library

Multiple things are modified here:
- The SwiftShader component was entirely removed
- The kSwiftShaderPath switch was removed, since SwiftShader
  will now be in a constant location
- SwiftShader files were added to FILES.cfg and chrome.release

BUG=630728
TBR=piman@chromium.org
TBR_REASON=This is only for changes in gpu_data_manager.h, which are trivial
==========

[sugoi1, 2017-03-07 13:09:06]

On 2017/03/07 09:49:27, grt (UTC plus 1) wrote:
> nit: Please remove "and I can split this in
> multiple cls if required by the reviewers" from the CL description. While
that's
> a useful comment for reviewers, it isn't relevant to the history of this CL
once
> it lands.

Done

[sugoi1, 2017-03-07 13:15:26]

On 2017/03/06 22:58:10, Devlin wrote:
>
https://codereview.chromium.org/2715563002/diff/140001/chrome/browser/extensi...
> File chrome/browser/extensions/extension_service_unittest.cc (right):
> 
>
https://codereview.chromium.org/2715563002/diff/140001/chrome/browser/extensi...
> chrome/browser/extensions/extension_service_unittest.cc:4610:
> EXPECT_TRUE(service()->IsExtensionEnabled(id));
> On 2017/03/06 18:31:42, sugoi wrote:
> > On 2017/03/02 21:59:29, Devlin wrote:
> > > Unfortunately, this defeats the purpose of the test, which is checking
what
> > > happens when requirements aren't satisfied.  If they are, then we didn't
> test
> > > anything. :)
> > 
> > I've looked into this, and I still think this is the proper fix. The
> > BlackListWebGL() function installs GPU info where WebGL is blacklisted,
which,
> > as of this cl, has the effect of enabling SwiftShader, which is the desired
> > behavior. If I were to make a testing specific fix, it would not represent
> > reality.
> > 
> > The test is still useful IMHO, since it verifies that, when SwiftShader is
> > enabled, WebGL doesn't get disabled. Also note that, currently, SwiftShader
is
> > only being enabled on Windows, so the test still behaves as usual on other
> > platforms.
> 
> The thing is, this isn't actually a GPU test, WebGL test, swiftshader test,
etc.
>  The behavior it's testing is that if an extension is disabled because of
> unsupported requirements, and then updates and no longer has unsupported
> requirements, it's automatically re-enabled. (See also the comment on line
> 4587.)  Disabling WebGL was an easy way of triggering an unsupported
> requirement, because it's entirely cross-platform and was easy to do from a
> unittest, but it's not the sole purpose of the test.  The way it's rewritten
> now, we don't actually test the flow that we're supposed to be exercising, so
we
> lose test coverage.
> 
> If we can no longer use webgl as an unavailable requirement, we'll have to
> substitute something else or find a test-only way of marking a requirement as
> unavailable.

Understood. I'm not the most knowledgeable person about chrome extensions and
wouldn't exactly know where to start. Could I add a TODO for someone who knows
about extensions to look into other options than disabling WebGL? Otherwise, do
you have any suggestions? I understand the purpose of the test, but I would
prefer if it wasn't based on a state that would never be experienced by Chrome
users, if possible.

[Devlin, 2017-03-08 02:37:24]

On 2017/03/07 13:15:26, sugoi1 wrote:
> On 2017/03/06 22:58:10, Devlin wrote:
> >
>
https://codereview.chromium.org/2715563002/diff/140001/chrome/browser/extensi...
> > File chrome/browser/extensions/extension_service_unittest.cc (right):
> > 
> >
>
https://codereview.chromium.org/2715563002/diff/140001/chrome/browser/extensi...
> > chrome/browser/extensions/extension_service_unittest.cc:4610:
> > EXPECT_TRUE(service()->IsExtensionEnabled(id));
> > On 2017/03/06 18:31:42, sugoi wrote:
> > > On 2017/03/02 21:59:29, Devlin wrote:
> > > > Unfortunately, this defeats the purpose of the test, which is checking
> what
> > > > happens when requirements aren't satisfied.  If they are, then we didn't
> > test
> > > > anything. :)
> > > 
> > > I've looked into this, and I still think this is the proper fix. The
> > > BlackListWebGL() function installs GPU info where WebGL is blacklisted,
> which,
> > > as of this cl, has the effect of enabling SwiftShader, which is the
desired
> > > behavior. If I were to make a testing specific fix, it would not represent
> > > reality.
> > > 
> > > The test is still useful IMHO, since it verifies that, when SwiftShader is
> > > enabled, WebGL doesn't get disabled. Also note that, currently,
SwiftShader
> is
> > > only being enabled on Windows, so the test still behaves as usual on other
> > > platforms.
> > 
> > The thing is, this isn't actually a GPU test, WebGL test, swiftshader test,
> etc.
> >  The behavior it's testing is that if an extension is disabled because of
> > unsupported requirements, and then updates and no longer has unsupported
> > requirements, it's automatically re-enabled. (See also the comment on line
> > 4587.)  Disabling WebGL was an easy way of triggering an unsupported
> > requirement, because it's entirely cross-platform and was easy to do from a
> > unittest, but it's not the sole purpose of the test.  The way it's rewritten
> > now, we don't actually test the flow that we're supposed to be exercising,
so
> we
> > lose test coverage.
> > 
> > If we can no longer use webgl as an unavailable requirement, we'll have to
> > substitute something else or find a test-only way of marking a requirement
as
> > unavailable.
> 
> Understood. I'm not the most knowledgeable person about chrome extensions and
> wouldn't exactly know where to start. Could I add a TODO for someone who knows
> about extensions to look into other options than disabling WebGL? Otherwise,
do
> you have any suggestions? I understand the purpose of the test, but I would
> prefer if it wasn't based on a state that would never be experienced by Chrome
> users, if possible.

(Talked offline, but tl;dr is that I'd recommend allowing for tests to supply
their own result of a requirements check so that we move away from dependency on
any particular feature.  Feel free to ping if you have any questions/run into
any issues.)

[sugoi1, 2017-03-08 19:12:39]

On 2017/03/08 02:37:24, Devlin wrote:
> On 2017/03/07 13:15:26, sugoi1 wrote:
> > On 2017/03/06 22:58:10, Devlin wrote:
> > >
> >
>
https://codereview.chromium.org/2715563002/diff/140001/chrome/browser/extensi...
> > > File chrome/browser/extensions/extension_service_unittest.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2715563002/diff/140001/chrome/browser/extensi...
> > > chrome/browser/extensions/extension_service_unittest.cc:4610:
> > > EXPECT_TRUE(service()->IsExtensionEnabled(id));
> > > On 2017/03/06 18:31:42, sugoi wrote:
> > > > On 2017/03/02 21:59:29, Devlin wrote:
> > > > > Unfortunately, this defeats the purpose of the test, which is checking
> > what
> > > > > happens when requirements aren't satisfied.  If they are, then we
didn't
> > > test
> > > > > anything. :)
> > > > 
> > > > I've looked into this, and I still think this is the proper fix. The
> > > > BlackListWebGL() function installs GPU info where WebGL is blacklisted,
> > which,
> > > > as of this cl, has the effect of enabling SwiftShader, which is the
> desired
> > > > behavior. If I were to make a testing specific fix, it would not
represent
> > > > reality.
> > > > 
> > > > The test is still useful IMHO, since it verifies that, when SwiftShader
is
> > > > enabled, WebGL doesn't get disabled. Also note that, currently,
> SwiftShader
> > is
> > > > only being enabled on Windows, so the test still behaves as usual on
other
> > > > platforms.
> > > 
> > > The thing is, this isn't actually a GPU test, WebGL test, swiftshader
test,
> > etc.
> > >  The behavior it's testing is that if an extension is disabled because of
> > > unsupported requirements, and then updates and no longer has unsupported
> > > requirements, it's automatically re-enabled. (See also the comment on line
> > > 4587.)  Disabling WebGL was an easy way of triggering an unsupported
> > > requirement, because it's entirely cross-platform and was easy to do from
a
> > > unittest, but it's not the sole purpose of the test.  The way it's
rewritten
> > > now, we don't actually test the flow that we're supposed to be exercising,
> so
> > we
> > > lose test coverage.
> > > 
> > > If we can no longer use webgl as an unavailable requirement, we'll have to
> > > substitute something else or find a test-only way of marking a requirement
> as
> > > unavailable.
> > 
> > Understood. I'm not the most knowledgeable person about chrome extensions
and
> > wouldn't exactly know where to start. Could I add a TODO for someone who
knows
> > about extensions to look into other options than disabling WebGL? Otherwise,
> do
> > you have any suggestions? I understand the purpose of the test, but I would
> > prefer if it wasn't based on a state that would never be experienced by
Chrome
> > users, if possible.
> 
> (Talked offline, but tl;dr is that I'd recommend allowing for tests to supply
> their own result of a requirements check so that we move away from dependency
on
> any particular feature.  Feel free to ping if you have any questions/run into
> any issues.)

Spun off another cl for fixing extension tests here:
https://codereview.chromium.org/2737983002/
Will be back to this cl once extensions tests are fixed.

[sugoi1, 2017-03-14 15:08:37]

The CQ bit was checked by sugoi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-14 15:09:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sugoi1, 2017-03-14 15:50:34]

sugoi@chromium.org changed reviewers:
- emaxx@chromium.org, rdevlin.cronin@chromium.org

[sugoi1, 2017-03-14 15:50:35]

Removing emaxx@ and rdevlin.cronin@ from reviewers list.
Fixes for policy tests and extension tests were landed in separate cls.

[commit-bot: I haz the power, 2017-03-14 16:15:51]

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

[commit-bot: I haz the power, 2017-03-14 16:15:53]

Dry run: This issue passed the CQ dry run.

[Nico, 2017-03-14 18:02:38]

chrome/ lgtm

[sugoi1, 2017-03-14 18:03:49]

The CQ bit was checked by sugoi@chromium.org

[sugoi1, 2017-03-14 18:03:50]

The patchset sent to the CQ was uploaded after l-g-t-m from kbr@chromium.org,
laforge@chromium.org, sebmarchand@chromium.org, jbauman@chromium.org,
grt@chromium.org, emaxx@chromium.org
Link to the patchset: https://codereview.chromium.org/2715563002/#ps220001
(title: "Update past extension test fixes")

[commit-bot: I haz the power, 2017-03-14 18:04:22]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-14 18:10:44]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1489514629912500,
"parent_rev": "8e5a6ea2b78481bc734b96b2350dad28698161d2", "commit_rev":
"842efc6922b0cd66febd6f36f04318ebf396c184"}

[commit-bot: I haz the power, 2017-03-14 18:11:26]

Description was changed from

==========
Moving SwiftShader from component to bundled library

Multiple things are modified here:
- The SwiftShader component was entirely removed
- The kSwiftShaderPath switch was removed, since SwiftShader
  will now be in a constant location
- SwiftShader files were added to FILES.cfg and chrome.release

BUG=630728
TBR=piman@chromium.org
TBR_REASON=This is only for changes in gpu_data_manager.h, which are trivial
==========

to

==========
Moving SwiftShader from component to bundled library

Multiple things are modified here:
- The SwiftShader component was entirely removed
- The kSwiftShaderPath switch was removed, since SwiftShader
  will now be in a constant location
- SwiftShader files were added to FILES.cfg and chrome.release

BUG=630728
TBR=piman@chromium.org
TBR_REASON=This is only for changes in gpu_data_manager.h, which are trivial

Review-Url: https://codereview.chromium.org/2715563002
Cr-Commit-Position: refs/heads/master@{#456764}
Committed:
https://chromium.googlesource.com/chromium/src/+/842efc6922b0cd66febd6f36f043...
==========

[commit-bot: I haz the power, 2017-03-14 18:11:29]

Committed patchset #12 (id:220001) as
https://chromium.googlesource.com/chromium/src/+/842efc6922b0cd66febd6f36f043...