Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(236)

Issue 2715563002: Moving SwiftShader from component to bundled library (Closed)

Created:
3 years, 10 months ago by sugoi1
Modified:
3 years, 9 months ago
CC:
chromium-reviews, grt+watch_chromium.org, jam, pennymac+watch_chromium.org, darin-cc_chromium.org, wfh+watch_chromium.org, piman+watch_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

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

Patch Set 1 #

Total comments: 13

Patch Set 2 : Fixed some comments #

Total comments: 2

Patch Set 3 : Trying to fix analysis pass #

Patch Set 4 : 2nd fix for analysis pass #

Patch Set 5 : Fix for content_unittests #

Patch Set 6 : Specify x11 only support on Linux #

Total comments: 11

Patch Set 7 : Fixed nits and some unit tests #

Total comments: 2

Patch Set 8 : Prevented !GpuAccessAllowed() from enabling SwiftShader #

Total comments: 12

Patch Set 9 : Restored GpuAccessAllowed in EnableSwiftShaderIfNecessary #

Patch Set 10 : Made the BUILD.gn modification Windows only for now #

Patch Set 11 : Added comment to policy browser test #

Patch Set 12 : Update past extension test fixes #

Unified diffs Side-by-side diffs Delta from patch set Stats (+159 lines, -414 lines) Patch
M chrome/browser/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +0 lines, -2 lines 0 comments Download
M chrome/browser/chrome_browser_main.cc View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +0 lines, -2 lines 0 comments Download
D chrome/browser/component_updater/swiftshader_component_installer.h View 1 chunk +0 lines, -19 lines 0 comments Download
D chrome/browser/component_updater/swiftshader_component_installer.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +0 lines, -284 lines 0 comments Download
M chrome/installer/mini_installer/chrome.release View 1 2 1 chunk +6 lines, -0 lines 0 comments Download
M chrome/tools/build/win/FILES.cfg View 1 2 3 4 2 chunks +21 lines, -0 lines 0 comments Download
M content/browser/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +2 lines, -0 lines 0 comments Download
M content/browser/gpu/gpu_data_manager_impl.h View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +0 lines, -2 lines 0 comments Download
M content/browser/gpu/gpu_data_manager_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +0 lines, -6 lines 0 comments Download
M content/browser/gpu/gpu_data_manager_impl_private.h View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +0 lines, -3 lines 0 comments Download
M content/browser/gpu/gpu_data_manager_impl_private.cc View 1 2 3 4 5 6 7 8 9 10 11 5 chunks +6 lines, -17 lines 0 comments Download
M content/browser/gpu/gpu_data_manager_impl_private_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 15 chunks +100 lines, -46 lines 0 comments Download
M content/public/browser/gpu_data_manager.h View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +0 lines, -7 lines 0 comments Download
M ui/gl/BUILD.gn View 1 2 3 4 5 6 7 8 1 chunk +1 line, -1 line 0 comments Download
M ui/gl/gl_switches.h View 1 2 1 chunk +0 lines, -1 line 0 comments Download
M ui/gl/gl_switches.cc View 1 2 1 chunk +0 lines, -2 lines 0 comments Download
M ui/gl/init/BUILD.gn View 1 2 3 1 chunk +4 lines, -1 line 0 comments Download
M ui/gl/init/gl_initializer_win.cc View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +5 lines, -15 lines 0 comments Download
M ui/gl/init/gl_initializer_x11.cc View 1 2 3 4 5 6 3 chunks +14 lines, -6 lines 0 comments Download

Messages

Total messages: 100 (49 generated)
sugoi1
I can split this in multiple cls if required, I just need help making sure ...
3 years, 10 months ago (2017-02-22 21:07:56 UTC) #2
Ken Russell (switch to Gerrit)
https://codereview.chromium.org/2715563002/diff/1/chrome/tools/build/win/FILES.cfg File chrome/tools/build/win/FILES.cfg (right): https://codereview.chromium.org/2715563002/diff/1/chrome/tools/build/win/FILES.cfg#newcode395 chrome/tools/build/win/FILES.cfg:395: }, There are conditions like BUILDFLAG(ENABLE_SWIFTSHADER) throughout the code ...
3 years, 10 months ago (2017-02-23 01:58:06 UTC) #3
jbauman
Does the browser build target have a dep on swiftshader yet? Also have we made ...
3 years, 10 months ago (2017-02-23 02:06:42 UTC) #4
sugoi
On 2017/02/23 02:06:42, jbauman wrote: > Does the browser build target have a dep on ...
3 years, 10 months ago (2017-02-23 03:40:52 UTC) #5
sugoi
https://codereview.chromium.org/2715563002/diff/1/chrome/tools/build/win/FILES.cfg File chrome/tools/build/win/FILES.cfg (right): https://codereview.chromium.org/2715563002/diff/1/chrome/tools/build/win/FILES.cfg#newcode395 chrome/tools/build/win/FILES.cfg:395: }, On 2017/02/23 01:58:05, Ken Russell wrote: > There ...
3 years, 10 months ago (2017-02-23 03:41:31 UTC) #7
Ken Russell (switch to Gerrit)
OK. LGTM from my standpoint. The PMs will have to give a thumbs-up on adding ...
3 years, 10 months ago (2017-02-23 03:52:39 UTC) #8
laforge
On 2017/02/23 03:52:39, Ken Russell wrote: > OK. LGTM from my standpoint. The PMs will ...
3 years, 10 months ago (2017-02-23 04:18:48 UTC) #9
jbauman
On 2017/02/23 03:40:52, sugoi wrote: > On 2017/02/23 02:06:42, jbauman wrote: > > Does the ...
3 years, 10 months ago (2017-02-23 04:23:51 UTC) #10
jbauman
https://codereview.chromium.org/2715563002/diff/1/content/browser/gpu/gpu_data_manager_impl_private.cc File content/browser/gpu/gpu_data_manager_impl_private.cc (right): https://codereview.chromium.org/2715563002/diff/1/content/browser/gpu/gpu_data_manager_impl_private.cc#newcode1235 content/browser/gpu/gpu_data_manager_impl_private.cc:1235: if (!base::CommandLine::ForCurrentProcess()->HasSwitch( Check BUILDFLAG(ENABLE_SWIFTSHADER) here. Otherwise this will automatically ...
3 years, 10 months ago (2017-02-23 04:30:45 UTC) #11
grt (UTC plus 2)
https://codereview.chromium.org/2715563002/diff/1/chrome/installer/mini_installer/chrome.release File chrome/installer/mini_installer/chrome.release (right): https://codereview.chromium.org/2715563002/diff/1/chrome/installer/mini_installer/chrome.release#newcode79 chrome/installer/mini_installer/chrome.release:79: swiftshader\libEGL.dll: %(VersionDir)s\swiftshader\ are these only for is_chrome_branded=true builds? do ...
3 years, 10 months ago (2017-02-23 15:00:50 UTC) #12
sugoi
A few changes: 1) Moved swiftshader deps from "all" to "browser". 2) Removed "is_chrome_branded" for ...
3 years, 10 months ago (2017-02-23 15:22:55 UTC) #13
grt (UTC plus 2)
https://codereview.chromium.org/2715563002/diff/1/chrome/installer/mini_installer/chrome.release File chrome/installer/mini_installer/chrome.release (right): https://codereview.chromium.org/2715563002/diff/1/chrome/installer/mini_installer/chrome.release#newcode79 chrome/installer/mini_installer/chrome.release:79: swiftshader\libEGL.dll: %(VersionDir)s\swiftshader\ On 2017/02/23 15:22:55, sugoi wrote: > On ...
3 years, 10 months ago (2017-02-23 15:33:39 UTC) #18
sugoi
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#newcode3609 chrome/browser/BUILD.gn:3609: deps += [ "//third_party/swiftshader" ] On 2017/02/23 15:33:38, grt ...
3 years, 10 months ago (2017-02-23 15:36:34 UTC) #19
Sébastien Marchand
LGTM, please try this on the win-pgo trybot before submitting.
3 years, 10 months ago (2017-02-23 16:09:34 UTC) #20
sugoi1
More changes, jbauman@ PTAL. - Modified EnableSwiftShaderIfNecessary so that use_swiftshader_ can also be turned off ...
3 years, 10 months ago (2017-02-24 21:24:31 UTC) #31
jbauman
https://codereview.chromium.org/2715563002/diff/100001/content/browser/gpu/gpu_data_manager_impl_private.cc File content/browser/gpu/gpu_data_manager_impl_private.cc (left): https://codereview.chromium.org/2715563002/diff/100001/content/browser/gpu/gpu_data_manager_impl_private.cc#oldcode674 content/browser/gpu/gpu_data_manager_impl_private.cc:674: if (use_swiftshader_) I think we still need this so ...
3 years, 10 months ago (2017-02-25 00:30:15 UTC) #38
grt (UTC plus 2)
lgtm with some nits from me. please wait for seb to confirm that the duplicate ...
3 years, 9 months ago (2017-02-27 09:31:56 UTC) #39
Sébastien Marchand
https://codereview.chromium.org/2715563002/diff/100001/chrome/tools/build/win/FILES.cfg File chrome/tools/build/win/FILES.cfg (right): https://codereview.chromium.org/2715563002/diff/100001/chrome/tools/build/win/FILES.cfg#newcode621 chrome/tools/build/win/FILES.cfg:621: 'filename': 'swiftshader/libEGL.dll.pdb', On 2017/02/27 09:31:55, grt (UTC plus 1) ...
3 years, 9 months ago (2017-02-27 16:00:22 UTC) #40
sugoi
I still have to land a modification to the symbols related script, as sebmarchand@ pointed ...
3 years, 9 months ago (2017-02-28 16:45:10 UTC) #41
grt (UTC plus 2)
still lgtm w/ one more nit https://codereview.chromium.org/2715563002/diff/120001/content/browser/gpu/gpu_data_manager_impl.h File content/browser/gpu/gpu_data_manager_impl.h (right): https://codereview.chromium.org/2715563002/diff/120001/content/browser/gpu/gpu_data_manager_impl.h#newcode15 content/browser/gpu/gpu_data_manager_impl.h:15: #include "base/files/file_path.h" unused
3 years, 9 months ago (2017-02-28 19:51:29 UTC) #46
jbauman
lgtm. Once you commit this we should check GPU.EGLDisplayType and ensure it doesn't change too ...
3 years, 9 months ago (2017-03-01 00:34:29 UTC) #47
sugoi1
https://codereview.chromium.org/2715563002/diff/120001/content/browser/gpu/gpu_data_manager_impl_private.cc File content/browser/gpu/gpu_data_manager_impl_private.cc (right): https://codereview.chromium.org/2715563002/diff/120001/content/browser/gpu/gpu_data_manager_impl_private.cc#newcode1235 content/browser/gpu/gpu_data_manager_impl_private.cc:1235: if (!GpuAccessAllowed(nullptr) || This call to GpuAccessAllowed() causes SwiftShader ...
3 years, 9 months ago (2017-03-01 16:50:22 UTC) #48
sugoi
Adding thakis@ for the changes in chrome/browser.
3 years, 9 months ago (2017-03-01 19:45:09 UTC) #52
jbauman
On 2017/03/01 16:50:22, sugoi1 wrote: > https://codereview.chromium.org/2715563002/diff/120001/content/browser/gpu/gpu_data_manager_impl_private.cc > File content/browser/gpu/gpu_data_manager_impl_private.cc (right): > > https://codereview.chromium.org/2715563002/diff/120001/content/browser/gpu/gpu_data_manager_impl_private.cc#newcode1235 > ...
3 years, 9 months ago (2017-03-01 22:37:04 UTC) #56
Nico
Please get a reviewer from c/b/extensions/OWNERS and c/b/policy/OWNERS for the test changes. Especially the policy ...
3 years, 9 months ago (2017-03-01 22:41:00 UTC) #57
sugoi1
Adding emaxx@ for c/b/policy Adding asargent@ for c/b/extensions/api/webstore_private Adding rdevlin.cronin@ for c/b/extensions
3 years, 9 months ago (2017-03-01 23:41:18 UTC) #59
Devlin
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#newcode435 chrome/browser/extensions/api/webstore_private/webstore_private_apitest.cc:435: if (content::GpuDataManager::GetInstance()->ShouldUseSwiftShader()) { It seems strange that this change ...
3 years, 9 months ago (2017-03-01 23:47:24 UTC) #60
sugoi1
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#newcode435 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: > ...
3 years, 9 months ago (2017-03-02 00:17:08 UTC) #61
emaxx
https://codereview.chromium.org/2715563002/diff/140001/chrome/browser/policy/policy_browsertest.cc File chrome/browser/policy/policy_browsertest.cc (right): https://codereview.chromium.org/2715563002/diff/140001/chrome/browser/policy/policy_browsertest.cc#newcode4081 chrome/browser/policy/policy_browsertest.cc:4081: content::GpuDataManager::GetInstance()->GpuAccessAllowed(nullptr) && I'm not entirely following this. Is this ...
3 years, 9 months ago (2017-03-02 16:06:30 UTC) #62
sugoi1
https://codereview.chromium.org/2715563002/diff/140001/chrome/browser/policy/policy_browsertest.cc File chrome/browser/policy/policy_browsertest.cc (right): https://codereview.chromium.org/2715563002/diff/140001/chrome/browser/policy/policy_browsertest.cc#newcode4081 chrome/browser/policy/policy_browsertest.cc:4081: content::GpuDataManager::GetInstance()->GpuAccessAllowed(nullptr) && On 2017/03/02 16:06:29, emaxx wrote: > I'm ...
3 years, 9 months ago (2017-03-02 16:23:17 UTC) #63
sugoi1
Removing asargent@ from the reviewers list since he no longer works on Chrome.
3 years, 9 months ago (2017-03-02 19:03:09 UTC) #65
Devlin
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(), ...
3 years, 9 months ago (2017-03-02 21:59:29 UTC) #66
Nico
I guess the question here is if there are other good reasons for disabling webgl ...
3 years, 9 months ago (2017-03-03 15:54:13 UTC) #67
sugoi1
On 2017/03/03 15:54:13, Nico wrote: > I guess the question here is if there are ...
3 years, 9 months ago (2017-03-03 16:11:45 UTC) #68
emaxx
On 2017/03/03 16:11:45, sugoi1 wrote: > On 2017/03/03 15:54:13, Nico wrote: > > I guess ...
3 years, 9 months ago (2017-03-03 16:42:52 UTC) #69
sugoi1
On 2017/03/03 16:42:52, emaxx wrote: > On 2017/03/03 16:11:45, sugoi1 wrote: > > On 2017/03/03 ...
3 years, 9 months ago (2017-03-03 16:56:05 UTC) #70
sugoi1
On 2017/03/03 16:56:05, sugoi1 wrote: > On 2017/03/03 16:42:52, emaxx wrote: > > On 2017/03/03 ...
3 years, 9 months ago (2017-03-03 16:57:01 UTC) #71
sugoi
Hi rdevlin.cronin@ and emaxx@, I've looked into the issues you pointed out and I can't ...
3 years, 9 months ago (2017-03-06 18:31:42 UTC) #74
emaxx
https://codereview.chromium.org/2715563002/diff/140001/chrome/browser/policy/policy_browsertest.cc File chrome/browser/policy/policy_browsertest.cc (right): https://codereview.chromium.org/2715563002/diff/140001/chrome/browser/policy/policy_browsertest.cc#newcode4081 chrome/browser/policy/policy_browsertest.cc:4081: content::GpuDataManager::GetInstance()->GpuAccessAllowed(nullptr) && On 2017/03/06 18:31:42, sugoi wrote: > On ...
3 years, 9 months ago (2017-03-06 20:10:51 UTC) #78
sugoi1
On 2017/03/06 20:10:51, emaxx wrote: > https://codereview.chromium.org/2715563002/diff/140001/chrome/browser/policy/policy_browsertest.cc > File chrome/browser/policy/policy_browsertest.cc (right): > > https://codereview.chromium.org/2715563002/diff/140001/chrome/browser/policy/policy_browsertest.cc#newcode4081 > ...
3 years, 9 months ago (2017-03-06 20:40:24 UTC) #79
emaxx
On 2017/03/06 20:40:24, sugoi1 wrote: > I'm not strongly opposed to making the change, but ...
3 years, 9 months ago (2017-03-06 20:50:29 UTC) #80
Devlin
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)); On 2017/03/06 18:31:42, sugoi wrote: > On 2017/03/02 ...
3 years, 9 months ago (2017-03-06 22:58:10 UTC) #81
grt (UTC plus 2)
nit: Please remove "and I can split this in multiple cls if required by the ...
3 years, 9 months ago (2017-03-07 09:49:27 UTC) #82
sugoi1
On 2017/03/07 09:49:27, grt (UTC plus 1) wrote: > nit: Please remove "and I can ...
3 years, 9 months ago (2017-03-07 13:09:06 UTC) #84
sugoi1
On 2017/03/06 22:58:10, Devlin wrote: > 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 > ...
3 years, 9 months ago (2017-03-07 13:15:26 UTC) #85
Devlin
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/extensions/extension_service_unittest.cc ...
3 years, 9 months ago (2017-03-08 02:37:24 UTC) #86
sugoi1
On 2017/03/08 02:37:24, Devlin wrote: > On 2017/03/07 13:15:26, sugoi1 wrote: > > On 2017/03/06 ...
3 years, 9 months ago (2017-03-08 19:12:39 UTC) #87
sugoi1
Removing emaxx@ and rdevlin.cronin@ from reviewers list. Fixes for policy tests and extension tests were ...
3 years, 9 months ago (2017-03-14 15:50:35 UTC) #91
Nico
chrome/ lgtm
3 years, 9 months ago (2017-03-14 18:02:38 UTC) #94
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2715563002/220001
3 years, 9 months ago (2017-03-14 18:04:22 UTC) #97
commit-bot: I haz the power
3 years, 9 months ago (2017-03-14 18:11:29 UTC) #100
Message was sent while issue was closed.
Committed patchset #12 (id:220001) as
https://chromium.googlesource.com/chromium/src/+/842efc6922b0cd66febd6f36f043...

Powered by Google App Engine
This is Rietveld 408576698