Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1217333002:
Add a group policy for hardware acceleration. (Closed)
|
Created:
5 years, 5 months ago by Zhenyao Mo Modified:
5 years, 5 months ago CC:
chromium-reviews, asvitkine+watch_chromium.org, Ken Russell (switch to Gerrit), vmiura Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd a group policy for hardware acceleration.
BUG=337713
TEST=browser_tests
R=atwilson@chromium.org
Committed: https://crrev.com/d6486a6280378ac0f858c1027e6c389ab6a342c6
Cr-Commit-Position: refs/heads/master@{#339202}
Patch Set 1 #
Total comments: 25
Patch Set 2 : #
Total comments: 2
Patch Set 3 : #
Total comments: 2
Patch Set 4 : add indicator tests #Patch Set 5 : fix #Patch Set 6 : fix test failures #Patch Set 7 : rebase #
Total comments: 5
Patch Set 8 : #Patch Set 9 : #
Messages
Total messages: 41 (12 generated)
atwilson or bartfab: can one of you review this? jar: histograms.xml owner kbr, vmiura: FYI
Note that this control is also part of the chrome://settings -> Advanced -> System I tested manually on Linux.
On 2015/06/30 23:15:52, Zhenyao Mo wrote: > Note that this control is also part of the chrome://settings -> Advanced -> > System > > I tested manually on Linux. One thing I noticed when I tested this: even if I delete the policy, the chrome://settings is still disabled, i.e., the policy is still effective with default value (=true). Is this expected behavior? Or is it I misunderstood certain things and got it wrong?
On 2015/06/30 23:18:06, Zhenyao Mo wrote: > On 2015/06/30 23:15:52, Zhenyao Mo wrote: > > Note that this control is also part of the chrome://settings -> Advanced -> > > System > > > > I tested manually on Linux. > > One thing I noticed when I tested this: even if I delete the policy, the > chrome://settings is still disabled, i.e., the policy is still effective with > default value (=true). > > Is this expected behavior? Or is it I misunderstood certain things and got it > wrong? Now I delete the entire /etc/chromium folder, it seems to give back user control for chrome://settings.
https://codereview.chromium.org/1217333002/diff/1/chrome/browser/policy/polic... File chrome/browser/policy/policy_browsertest.cc (right): https://codereview.chromium.org/1217333002/diff/1/chrome/browser/policy/polic... chrome/browser/policy/policy_browsertest.cc:3844: ~HardwareAccelerationModePolicyTest() override {} Nit: No need to override the destructor here. https://codereview.chromium.org/1217333002/diff/1/chrome/browser/policy/polic... chrome/browser/policy/policy_browsertest.cc:3859: EXPECT_TRUE(content::GpuDataManager::GetInstance()->GpuAccessAllowed(NULL)); 1: Nit: s/NULL/nullptr/ 2: PolicyTest.Disable3DAPIs seems to assume that hardware acceleration may not always be available in tests (e.g. unavailable when running under Xvfb?). Please make sure the test actually works under Xvfb and on bots. https://codereview.chromium.org/1217333002/diff/1/chrome/test/data/policy/pol... File chrome/test/data/policy/policy_test_cases.json (right): https://codereview.chromium.org/1217333002/diff/1/chrome/test/data/policy/pol... chrome/test/data/policy/policy_test_cases.json:2250: { "pref": "hardware_acceleration_mode.enabled" } Since this pref is exposed in chrome://settings, please add a controlled setting indicator for it and extend the test case accordingly. https://codereview.chromium.org/1217333002/diff/1/components/policy/resources... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/1217333002/diff/1/components/policy/resources... components/policy/resources/policy_templates.json:7401: 'name': 'HardwareAccelerationModeEnabled', How does this policy interact with Disable3DAPIs? https://codereview.chromium.org/1217333002/diff/1/components/policy/resources... components/policy/resources/policy_templates.json:7406: 'dynamic_refresh': False, What happens if the policy value changes while Chrome is running? Will content::GpuDataManager::GetInstance()->GpuAccessAllowed(NULL)'s return value really not change? https://codereview.chromium.org/1217333002/diff/1/components/policy/resources... components/policy/resources/policy_templates.json:7407: 'per_profile': True, Can this really be controlled on a per-profile basis? Based on the test, it seems like it is a browser-global setting. Also, if it is browser-global and needs to be set before the browser is created, does the policy actually work on Chrome OS (which has no platform policy, but cloud policy only). https://codereview.chromium.org/1217333002/diff/1/components/policy/resources... components/policy/resources/policy_templates.json:7411: 'caption': '''Use Hardware Acceleration When Available''', Nit: Why is this in title case? https://codereview.chromium.org/1217333002/diff/1/components/policy/resources... components/policy/resources/policy_templates.json:7414: If you enable this setting, hardware acceleration will be enabled unless a certain GPU feature is blacklisted. What is that "certain GPU feature" which disables hardware acceleration? Or do you mean that parts of the acceleration may be selectively disabled, depending on your GPU's capabilities? https://codereview.chromium.org/1217333002/diff/1/components/policy/resources... components/policy/resources/policy_templates.json:7416: If you disable this setting, no hardware acceleration will be disabled. This sentence is broken. https://codereview.chromium.org/1217333002/diff/1/components/policy/resources... components/policy/resources/policy_templates.json:7418: If this policy is left not set, it is equivalent to being enabled, see description above.''', This is unnecessarily complex. How about: If this policy is left not set, hardware acceleration will be enabled. https://codereview.chromium.org/1217333002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1217333002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:53664: + <int value="303" label="Use hardware acceleration when available"/> Please keep this in sync with the 'caption' in policy_templates.json.
Thanks for reviewing this. I revised my CL accordingly, except for the test part, where I need some help. https://codereview.chromium.org/1217333002/diff/1/chrome/browser/policy/polic... File chrome/browser/policy/policy_browsertest.cc (right): https://codereview.chromium.org/1217333002/diff/1/chrome/browser/policy/polic... chrome/browser/policy/policy_browsertest.cc:3844: ~HardwareAccelerationModePolicyTest() override {} On 2015/07/01 09:16:54, bartfab wrote: > Nit: No need to override the destructor here. Done. https://codereview.chromium.org/1217333002/diff/1/chrome/browser/policy/polic... chrome/browser/policy/policy_browsertest.cc:3859: EXPECT_TRUE(content::GpuDataManager::GetInstance()->GpuAccessAllowed(NULL)); On 2015/07/01 09:16:54, bartfab wrote: > 1: Nit: s/NULL/nullptr/ > 2: PolicyTest.Disable3DAPIs seems to assume that hardware acceleration may not > always be available in tests (e.g. unavailable when running under Xvfb?). Please > make sure the test actually works under Xvfb and on bots. Although it seems to run OK on all try bots, but I agree this is fragile, because we might just block the GPUs on future bots through software rendering list. Remove this test. https://codereview.chromium.org/1217333002/diff/1/chrome/test/data/policy/pol... File chrome/test/data/policy/policy_test_cases.json (right): https://codereview.chromium.org/1217333002/diff/1/chrome/test/data/policy/pol... chrome/test/data/policy/policy_test_cases.json:2250: { "pref": "hardware_acceleration_mode.enabled" } On 2015/07/01 09:16:54, bartfab wrote: > Since this pref is exposed in chrome://settings, please add a controlled setting > indicator for it and extend the test case accordingly. I read the documentation and also examples in this file, but I still have no good idea how to set up indicator_test_setup_js and indicator_selector. Can you give me a hand here? The settings is Advanced Settings -> System -> Use hardware acceleration when available. https://codereview.chromium.org/1217333002/diff/1/components/policy/resources... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/1217333002/diff/1/components/policy/resources... components/policy/resources/policy_templates.json:7401: 'name': 'HardwareAccelerationModeEnabled', On 2015/07/01 09:16:54, bartfab wrote: > How does this policy interact with Disable3DAPIs? Disable3DAPIs will be true if this is false. I add some text in Disable3DAPIs. https://codereview.chromium.org/1217333002/diff/1/components/policy/resources... components/policy/resources/policy_templates.json:7406: 'dynamic_refresh': False, On 2015/07/01 09:16:54, bartfab wrote: > What happens if the policy value changes while Chrome is running? Will > content::GpuDataManager::GetInstance()->GpuAccessAllowed(NULL)'s return value > really not change? This is only applied at browser startup time, so even if the value changes, it is only effective on next browser start. https://codereview.chromium.org/1217333002/diff/1/components/policy/resources... components/policy/resources/policy_templates.json:7407: 'per_profile': True, On 2015/07/01 09:16:54, bartfab wrote: > Can this really be controlled on a per-profile basis? Based on the test, it > seems like it is a browser-global setting. > > Also, if it is browser-global and needs to be set before the browser is created, > does the policy actually work on Chrome OS (which has no platform policy, but > cloud policy only). Switched to global setting. You are right, this should not be an option for Chrome OS (they make sure hardware acceleration always works better than software rendering). I also exclude Android because I don't see a need there. We can revisit if the needs ever arise. https://codereview.chromium.org/1217333002/diff/1/components/policy/resources... components/policy/resources/policy_templates.json:7411: 'caption': '''Use Hardware Acceleration When Available''', On 2015/07/01 09:16:54, bartfab wrote: > Nit: Why is this in title case? Switch to regular text mode. FYI: I am simply following the example from the above Key Permissions. https://codereview.chromium.org/1217333002/diff/1/components/policy/resources... components/policy/resources/policy_templates.json:7414: If you enable this setting, hardware acceleration will be enabled unless a certain GPU feature is blacklisted. On 2015/07/01 09:16:54, bartfab wrote: > What is that "certain GPU feature" which disables hardware acceleration? Or do > you mean that parts of the acceleration may be selectively disabled, depending > on your GPU's capabilities? For example, accelerated canvas 2D, or accelerated rasterization, or accelerated video decoding. Each feature can be turned off independently through chrome's software rendering list. https://codereview.chromium.org/1217333002/diff/1/components/policy/resources... components/policy/resources/policy_templates.json:7416: If you disable this setting, no hardware acceleration will be disabled. On 2015/07/01 09:16:54, bartfab wrote: > This sentence is broken. Done. https://codereview.chromium.org/1217333002/diff/1/components/policy/resources... components/policy/resources/policy_templates.json:7418: If this policy is left not set, it is equivalent to being enabled, see description above.''', On 2015/07/01 09:16:54, bartfab wrote: > This is unnecessarily complex. How about: > > If this policy is left not set, hardware acceleration will be enabled. That doesn't cover the case of features being blacklisted. I think referring back to the enabled case is easier (also the wording is copied from examples above). https://codereview.chromium.org/1217333002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1217333002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:53664: + <int value="303" label="Use hardware acceleration when available"/> On 2015/07/01 09:16:54, bartfab wrote: > Please keep this in sync with the 'caption' in policy_templates.json. Revised in the policy_templates.json so now they match.
zmo@chromium.org changed reviewers: + isherman@chromium.org - jar@chromium.org
Also, -jar, +isherman for histograms.xml change
histograms.xml lgtm
lg, but I'll let bartfab have the final say since he did the first review. https://codereview.chromium.org/1217333002/diff/20001/chrome/browser/policy/p... File chrome/browser/policy/policy_browsertest.cc (right): https://codereview.chromium.org/1217333002/diff/20001/chrome/browser/policy/p... chrome/browser/policy/policy_browsertest.cc:3841: #if !defined(OS_CHROMEOS) && !defined(OS_ANDROID) Why is this not available on Android and ChromeOS? https://codereview.chromium.org/1217333002/diff/20001/components/policy/resou... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/1217333002/diff/20001/components/policy/resou... components/policy/resources/policy_templates.json:3242: If HardwareAccelerationModeEnabled is set to false, support for 3D graphics APIs is implicitly disabled.''', Should we explicitly say that the Disable3DAPIs policy is ignored in that case?
On 2015/07/03 15:45:28, Andrew T Wilson wrote: > lg, but I'll let bartfab have the final say since he did the first review. > > https://codereview.chromium.org/1217333002/diff/20001/chrome/browser/policy/p... > File chrome/browser/policy/policy_browsertest.cc (right): > > https://codereview.chromium.org/1217333002/diff/20001/chrome/browser/policy/p... > chrome/browser/policy/policy_browsertest.cc:3841: #if !defined(OS_CHROMEOS) && > !defined(OS_ANDROID) > Why is this not available on Android and ChromeOS? ChromeOS is definitely a no no, because we fine tuned the performance to work well with GPU acceleration. Turning it off will be total disaster. I feel Android might be the same situation, so I also leave it out. It will be easy to add it if the needs ever arises. > > https://codereview.chromium.org/1217333002/diff/20001/components/policy/resou... > File components/policy/resources/policy_templates.json (right): > > https://codereview.chromium.org/1217333002/diff/20001/components/policy/resou... > components/policy/resources/policy_templates.json:3242: If > HardwareAccelerationModeEnabled is set to false, support for 3D graphics APIs is > implicitly disabled.''', > Should we explicitly say that the Disable3DAPIs policy is ignored in that case? I update the text, but it's not ignored. Instead, it's forced to be true.
From patch 2 and patch 3, only the desc text changed, but why patch 2 compiled OK on win8_chromium_np and patch 3 failed?
https://codereview.chromium.org/1217333002/diff/1/chrome/test/data/policy/pol... File chrome/test/data/policy/policy_test_cases.json (right): https://codereview.chromium.org/1217333002/diff/1/chrome/test/data/policy/pol... chrome/test/data/policy/policy_test_cases.json:2250: { "pref": "hardware_acceleration_mode.enabled" } On 2015/07/01 17:56:07, Zhenyao Mo wrote: > On 2015/07/01 09:16:54, bartfab wrote: > > Since this pref is exposed in chrome://settings, please add a controlled > setting > > indicator for it and extend the test case accordingly. > > I read the documentation and also examples in this file, but I still have no > good idea how to set up indicator_test_setup_js and indicator_selector. Can you > give me a hand here? The settings is Advanced Settings -> System -> Use > hardware acceleration when available. You do not need to worry about |indicator_test_setup_js| and |indicator_selector|. These are for special cases only where our default mechanisms do not work. In your case, the default mechanisms should work: 1) Add a controlled-setting-indicator element to the HTML page that contains the hardware acceleration setting and set up its attributes so that it listens for the right pref. 2) Add one or more |indicator_tests|. They will locate the correct indicator automatically. https://codereview.chromium.org/1217333002/diff/1/components/policy/resources... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/1217333002/diff/1/components/policy/resources... components/policy/resources/policy_templates.json:7411: 'caption': '''Use Hardware Acceleration When Available''', On 2015/07/01 17:56:08, Zhenyao Mo wrote: > On 2015/07/01 09:16:54, bartfab wrote: > > Nit: Why is this in title case? > > Switch to regular text mode. > > FYI: I am simply following the example from the above Key Permissions. I guess our template could do with a bit of clean-up :). https://codereview.chromium.org/1217333002/diff/1/components/policy/resources... components/policy/resources/policy_templates.json:7418: If this policy is left not set, it is equivalent to being enabled, see description above.''', On 2015/07/01 17:56:08, Zhenyao Mo wrote: > On 2015/07/01 09:16:54, bartfab wrote: > > This is unnecessarily complex. How about: > > > > If this policy is left not set, hardware acceleration will be enabled. > > That doesn't cover the case of features being blacklisted. I think referring > back to the enabled case is easier (also the wording is copied from examples > above). A common way to solve this is to change the first sentence to: "If this policy is set to true or left unset, ..." https://codereview.chromium.org/1217333002/diff/40001/components/policy/resou... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/1217333002/diff/40001/components/policy/resou... components/policy/resources/policy_templates.json:3242: If HardwareAccelerationModeEnabled is set to false, the Disable3DAPIs policy is forced to true.''', Do we actually force the policy to true (i.e., will you see a value of |true| when you visit chrome://policy), or are we just breaking the API, which has similar/equivalent effects to setting the policy to true? https://codereview.chromium.org/1217333002/diff/40001/components/policy/resou... components/policy/resources/policy_templates.json:7416: If you enable this setting, hardware acceleration will be enabled unless a certain GPU feature is blacklisted. Nit: As you saw with Disable3DAPIs, we (unfortunately) have some policies with negative polarity (where *disabling* a policy *enables* a feature), leading to a lot of confusion. We are trying to be super explicit about actual policy values these days. Please replace "enable" with "set to true" and "disable" with "set to false" to be 100% clear.
On 2015/07/08 12:40:36, bartfab (OOO till 3rd August) wrote: atwilson@: it seems bartfab is out until Aug 3rd. Can you review this instead?
The CQ bit was checked by zmo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1217333002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/1217333002/diff/120001/chrome/browser/policy/... File chrome/browser/policy/policy_browsertest.cc (right): https://codereview.chromium.org/1217333002/diff/120001/chrome/browser/policy/... chrome/browser/policy/policy_browsertest.cc:3860: IN_PROC_BROWSER_TEST_F(HardwareAccelerationModePolicyTest, What about the true case? Should we test that hardware acceleration defaults to true, or at least setting it explicitly to true leaves it enabled? https://codereview.chromium.org/1217333002/diff/120001/chrome/test/data/polic... File chrome/test/data/policy/policy_test_cases.json (right): https://codereview.chromium.org/1217333002/diff/120001/chrome/test/data/polic... chrome/test/data/policy/policy_test_cases.json:2264: { "policy": { "HardwareAccelerationModeEnabled": false } } Just curious, should we be also setting read_only: true?
https://codereview.chromium.org/1217333002/diff/120001/chrome/browser/policy/... File chrome/browser/policy/policy_browsertest.cc (right): https://codereview.chromium.org/1217333002/diff/120001/chrome/browser/policy/... chrome/browser/policy/policy_browsertest.cc:3860: IN_PROC_BROWSER_TEST_F(HardwareAccelerationModePolicyTest, On 2015/07/16 09:59:55, Andrew T Wilson wrote: > What about the true case? Should we test that hardware acceleration defaults to > true, or at least setting it explicitly to true leaves it enabled? In the first CL I did test that, but unfortunately GPU can just be blacklisted, so if we leave this to true or unset, it is not guaranteed the GpuAccessAllowed() returns true. That's why I removed the test case. https://codereview.chromium.org/1217333002/diff/120001/chrome/test/data/polic... File chrome/test/data/policy/policy_test_cases.json (right): https://codereview.chromium.org/1217333002/diff/120001/chrome/test/data/polic... chrome/test/data/policy/policy_test_cases.json:2264: { "policy": { "HardwareAccelerationModeEnabled": false } } On 2015/07/16 09:59:55, Andrew T Wilson wrote: > Just curious, should we be also setting read_only: true? Done.
The CQ bit was checked by zmo@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org, atwilson@chromium.org Link to the patchset: https://codereview.chromium.org/1217333002/#ps140001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1217333002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
zmo@chromium.org changed reviewers: + bauerb@chromium.org
bauerb: owner approval for chrome/browser/resources/options/browser_options.html
https://codereview.chromium.org/1217333002/diff/120001/chrome/test/data/polic... File chrome/test/data/policy/policy_test_cases.json (right): https://codereview.chromium.org/1217333002/diff/120001/chrome/test/data/polic... chrome/test/data/policy/policy_test_cases.json:2264: { "policy": { "HardwareAccelerationModeEnabled": false } } On 2015/07/16 18:06:31, Zhenyao Mo wrote: > On 2015/07/16 09:59:55, Andrew T Wilson wrote: > > Just curious, should we be also setting read_only: true? > > Done. OK, re-read the explanation in the template on top of this file, readonly is used in the test where setting a policy makes another pref readonly, not the pref that's directly connected with the policy. So it doesn't apply here. Revert this change.
The CQ bit was checked by zmo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1217333002/160001
zmo@chromium.org changed reviewers: + estade@chromium.org - bauerb@chromium.org
Actually estade should be a better owner to review chrome/browser/resources/options/browser_options.html
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
On 2015/07/16 23:30:31, commit-bot: I haz the power wrote: > Dry run: Try jobs failed on following builders: > linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...) lgtm
The CQ bit was checked by zmo@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from atwilson@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/1217333002/#ps160001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1217333002/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/d6486a6280378ac0f858c1027e6c389ab6a342c6 Cr-Commit-Position: refs/heads/master@{#339202} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
