Disable the Developer Tools when Javascript is disabled.

Deprecate the JavascriptEnabled policy.

It now has the same effect as DefaultJavaScriptSetting = BLOCK.
Added pyauto test for Javascript policies.

BUG=100621, 102668
TEST=policy.PolicyTest.testJavascriptPolicies

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=108661

[Joao da Silva, 2011-10-26 14:47:01]

Please review

@nirnimesh: automation, pyautolib & functional test changes
@mnissler: policy changes

[Mattias Nissler (ping if slow), 2011-10-26 15:09:27]

http://codereview.chromium.org/8395044/diff/1/chrome/browser/policy/configura...
File chrome/browser/policy/configuration_policy_handler.cc (right):

http://codereview.chromium.org/8395044/diff/1/chrome/browser/policy/configura...
chrome/browser/policy/configuration_policy_handler.cc:568: if (value &&
value_type_ != value->GetType()) {
This is almost GetTypeCheckedPolicy(). Share code?

http://codereview.chromium.org/8395044/diff/1/chrome/browser/policy/configura...
chrome/browser/policy/configuration_policy_handler.cc:1343:
Value::CreateBooleanValue(disabled));
I'm not sure this is the way to go. It only fixes the problem of DevTools being
inaccessible when Javascript is disabled through policy. If a user disables
Javascript through content settings, they can still try and open DevTools and
they don't get an indication why that fails. I think this is rather a bug on the
DevTools side, and one solution would be to just disable all DevTools shortcuts
and menu items if we detect Javascript is not available. Can you talk to the
DevTools team? If they agree, this block eher is actually not necessary.

[Nirnimesh, 2011-10-26 19:12:59]

Automation code LGTM

[Paweł Hajdan Jr., 2011-10-27 08:26:20]

Drive-by, no need to wait for me.

http://codereview.chromium.org/8395044/diff/1/chrome/test/pyautolib/pyautolib.cc
File chrome/test/pyautolib/pyautolib.cc (right):

http://codereview.chromium.org/8395044/diff/1/chrome/test/pyautolib/pyautolib...
chrome/test/pyautolib/pyautolib.cc:130:
automation()->GetBrowserWindow(window_index);
Please NULL-check this. It *will* crash randomly if you don't. A good example is
right above.

[Joao da Silva, 2011-11-03 18:00:09]

Thanks all for reviewing!

@mnissler: Please have another look

http://codereview.chromium.org/8395044/diff/1/chrome/browser/policy/configura...
File chrome/browser/policy/configuration_policy_handler.cc (right):

http://codereview.chromium.org/8395044/diff/1/chrome/browser/policy/configura...
chrome/browser/policy/configuration_policy_handler.cc:1343:
Value::CreateBooleanValue(disabled));
On 2011/10/26 15:09:28, Mattias Nissler wrote:
> I'm not sure this is the way to go. It only fixes the problem of DevTools
being
> inaccessible when Javascript is disabled through policy. If a user disables
> Javascript through content settings, they can still try and open DevTools and
> they don't get an indication why that fails. I think this is rather a bug on
the
> DevTools side, and one solution would be to just disable all DevTools
shortcuts
> and menu items if we detect Javascript is not available. Can you talk to the
> DevTools team? If they agree, this block eher is actually not necessary.

It turns out that content settings never disable the DevTools. So deprecating
JavascriptEnabled and wiring it to the content setting behind the scenes will
allow the DevTools to keep working when Javascript is disabled.

The discussion on bug 102668 argues that this is the right thing to do, since
the policy is meant to disable javascript from internet sites; chrome pages
always have javascript enabled anyways, and the DevTools are part of chrome.
They run in the renderer however, and that's why completely disabling javascript
in the renderer led to the DevTool glitch. Admins can still disable the DevTools
through its own policy.

http://codereview.chromium.org/8395044/diff/1/chrome/test/pyautolib/pyautolib.cc
File chrome/test/pyautolib/pyautolib.cc (right):

http://codereview.chromium.org/8395044/diff/1/chrome/test/pyautolib/pyautolib...
chrome/test/pyautolib/pyautolib.cc:130:
automation()->GetBrowserWindow(window_index);
On 2011/10/27 08:26:21, Paweł Hajdan Jr. wrote:
> Please NULL-check this. It *will* crash randomly if you don't. A good example
is
> right above.

Thanks for driving-by :-) Done.

[Mattias Nissler (ping if slow), 2011-11-03 18:42:02]

http://codereview.chromium.org/8395044/diff/6002/chrome/app/policy/policy_tem...
File chrome/app/policy/policy_templates.json (right):

http://codereview.chromium.org/8395044/diff/6002/chrome/app/policy/policy_tem...
chrome/app/policy/policy_templates.json:276: If this setting is disabled, web
pages cannot use JavaScript. This will also force DefaultJavaScriptSetting to
block JavaScript on all sites.''',
I guess you want to say that this force-enables the DefaultJavaScriptSetting
policy? From just reading it that's not obvious and the two sentences here feel
redundant. Maybe say "This policy will force the DefaultJavaScriptSetting policy
to X".

http://codereview.chromium.org/8395044/diff/6002/chrome/browser/policy/config...
File chrome/browser/policy/configuration_policy_handler.cc (right):

http://codereview.chromium.org/8395044/diff/6002/chrome/browser/policy/config...
chrome/browser/policy/configuration_policy_handler.cc:1283: // This policy is
deprecated, and currently overrides the content setting.
I would expect this to be the other way round. Is there a good reason to have
something that is deprecated be overriden by the supported way of doing things?

http://codereview.chromium.org/8395044/diff/6002/chrome/browser/policy/config...
File chrome/browser/policy/configuration_policy_pref_store_unittest.cc (right):

http://codereview.chromium.org/8395044/diff/6002/chrome/browser/policy/config...
chrome/browser/policy/configuration_policy_pref_store_unittest.cc:303:
prefs::kManagedDefaultGeolocationSetting),
Why did you remove all these?

http://codereview.chromium.org/8395044/diff/6002/chrome/browser/policy/config...
chrome/browser/policy/configuration_policy_pref_store_unittest.cc:918:
store_->GetValue(prefs::kAutofillEnabled, NULL));
good catches!

[Joao da Silva, 2011-11-03 20:46:54]

@mnissler: good point about the overriding order, it's been changed. PTAL.

http://codereview.chromium.org/8395044/diff/6002/chrome/app/policy/policy_tem...
File chrome/app/policy/policy_templates.json (right):

http://codereview.chromium.org/8395044/diff/6002/chrome/app/policy/policy_tem...
chrome/app/policy/policy_templates.json:276: If this setting is disabled, web
pages cannot use JavaScript. This will also force DefaultJavaScriptSetting to
block JavaScript on all sites.''',
On 2011/11/03 18:42:02, Mattias Nissler wrote:
> I guess you want to say that this force-enables the DefaultJavaScriptSetting
> policy? From just reading it that's not obvious and the two sentences here
feel
> redundant. Maybe say "This policy will force the DefaultJavaScriptSetting
policy
> to X".

Yes, that's what I meant. DefaultJavaScriptSetting is an enum, and in this case
would be forced to the "BlockJavaScript" value, thus the wording.

The wording has changed to reflect the new overriding order.

http://codereview.chromium.org/8395044/diff/6002/chrome/browser/policy/config...
File chrome/browser/policy/configuration_policy_handler.cc (right):

http://codereview.chromium.org/8395044/diff/6002/chrome/browser/policy/config...
chrome/browser/policy/configuration_policy_handler.cc:1283: // This policy is
deprecated, and currently overrides the content setting.
On 2011/11/03 18:42:02, Mattias Nissler wrote:
> I would expect this to be the other way round. Is there a good reason to have
> something that is deprecated be overriden by the supported way of doing
things?

No particular reason for it. The other deprecated policies also override the
deprecated policy in favor of the "new" one, so this has been changed as
suggested.

http://codereview.chromium.org/8395044/diff/6002/chrome/browser/policy/config...
File chrome/browser/policy/configuration_policy_pref_store_unittest.cc (right):

http://codereview.chromium.org/8395044/diff/6002/chrome/browser/policy/config...
chrome/browser/policy/configuration_policy_pref_store_unittest.cc:303:
prefs::kManagedDefaultGeolocationSetting),
On 2011/11/03 18:42:02, Mattias Nissler wrote:
> Why did you remove all these?

What do you mean? I'm adding these now since they weren't being tested before.

http://codereview.chromium.org/8395044/diff/6002/chrome/browser/policy/config...
chrome/browser/policy/configuration_policy_pref_store_unittest.cc:918:
store_->GetValue(prefs::kAutofillEnabled, NULL));
On 2011/11/03 18:42:02, Mattias Nissler wrote:
> good catches!

Hopefully the test still passes!

[Mattias Nissler (ping if slow), 2011-11-04 09:47:30]

lgtm

[commit-bot: I haz the power, 2011-11-04 09:53:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joaodasilva@chromium.org/8395044/19001

[commit-bot: I haz the power, 2011-11-04 11:59:35]

Try job failure for 8395044-19001 (retry) on win_rel for step
"chrome_frame_net_tests".
It's a second try, previously, step "chrome_frame_net_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2011-11-04 12:48:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joaodasilva@chromium.org/8395044/19001

[commit-bot: I haz the power, 2011-11-04 14:20:46]

Try job failure for 8395044-19001 (retry) on win_rel for step
"chrome_frame_net_tests".
It's a second try, previously, step "chrome_frame_net_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2011-11-04 14:27:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joaodasilva@chromium.org/8395044/19001

[commit-bot: I haz the power, 2011-11-04 14:28:35]

Can't apply patch for file
chrome/browser/policy/configuration_policy_handler.cc.
While running patch -p1 --forward --force;
patching file chrome/browser/policy/configuration_policy_handler.cc
Hunk #1 FAILED at 1244.
1 out of 1 hunk FAILED -- saving rejects to file
chrome/browser/policy/configuration_policy_handler.cc.rej

[commit-bot: I haz the power, 2011-11-04 14:37:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joaodasilva@chromium.org/8395044/23001

[commit-bot: I haz the power, 2011-11-04 16:00:56]

Change committed as 108661