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

Issue 11078023: Add controlled setting indicators for content settings (Closed)

Created:
8 years, 2 months ago by bartfab (slow)
Modified:
8 years, 2 months ago
CC:
chromium-reviews, arv (Not doing code reviews), stevenjb+watch_chromium.org, oshima+watch_chromium.org
Visibility:
Public.

Description

Add controlled setting indicators for content settings This CL adds controlled setting indicators to the content settings configuration dialog at chrome://chrome/settings/content and its popup dialogs for configuring per-host exceptions. BUG=104955 Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=162897

Patch Set 1 #

Patch Set 2 : Mark which of the policies considered can be recommended. #

Total comments: 11

Patch Set 3 : Comments addressed. #

Patch Set 4 : Rebased and updated element nesting in line with CL 11141022. #

Patch Set 5 : Removed unnecessary <span> elements. #

Patch Set 6 : Rebased. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+476 lines, -150 lines) Patch
M chrome/app/policy/policy_templates.json View 1 2 3 4 1 chunk +3 lines, -7 lines 0 comments Download
M chrome/browser/policy/policy_prefs_browsertest.cc View 1 2 3 4 5 10 chunks +44 lines, -9 lines 0 comments Download
M chrome/browser/resources/options/chromeos/network_list.js View 1 2 3 4 1 chunk +0 lines, -1 line 0 comments Download
M chrome/browser/resources/options/content_settings.css View 1 2 3 2 chunks +5 lines, -17 lines 0 comments Download
M chrome/browser/resources/options/content_settings.html View 1 2 3 4 8 chunks +212 lines, -84 lines 0 comments Download
M chrome/browser/resources/options/content_settings.js View 1 2 3 4 1 chunk +17 lines, -2 lines 0 comments Download
M chrome/browser/resources/options/content_settings_exceptions_area.js View 1 2 2 chunks +24 lines, -7 lines 0 comments Download
M chrome/browser/resources/options/controlled_setting.js View 1 chunk +1 line, -0 lines 0 comments Download
M chrome/test/data/policy/policy_test_cases.json View 1 2 5 chunks +170 lines, -23 lines 0 comments Download

Messages

Total messages: 10 (0 generated)
bartfab (slow)
Hi James, could you please review the options UI side? Hi Joao, could you please ...
8 years, 2 months ago (2012-10-12 16:44:05 UTC) #1
James Hawkins
https://codereview.chromium.org/11078023/diff/3001/chrome/browser/policy/policy_prefs_browsertest.cc File chrome/browser/policy/policy_prefs_browsertest.cc (right): https://codereview.chromium.org/11078023/diff/3001/chrome/browser/policy/policy_prefs_browsertest.cc#newcode165 chrome/browser/policy/policy_prefs_browsertest.cc:165: std::wstring indicator_test_setup_js_; Why are you using wstring?
8 years, 2 months ago (2012-10-12 22:23:32 UTC) #2
bartfab (slow)
https://codereview.chromium.org/11078023/diff/3001/chrome/browser/policy/policy_prefs_browsertest.cc File chrome/browser/policy/policy_prefs_browsertest.cc (right): https://codereview.chromium.org/11078023/diff/3001/chrome/browser/policy/policy_prefs_browsertest.cc#newcode165 chrome/browser/policy/policy_prefs_browsertest.cc:165: std::wstring indicator_test_setup_js_; This value will be passed to ExecuteJavaScript() ...
8 years, 2 months ago (2012-10-13 07:21:58 UTC) #3
Joao da Silva
policy/ changes lgtm after fixing the comment below http://codereview.chromium.org/11078023/diff/3001/chrome/browser/policy/policy_prefs_browsertest.cc File chrome/browser/policy/policy_prefs_browsertest.cc (right): http://codereview.chromium.org/11078023/diff/3001/chrome/browser/policy/policy_prefs_browsertest.cc#newcode526 chrome/browser/policy/policy_prefs_browsertest.cc:526: policy_test_case->indicator_test_setup_js())); ...
8 years, 2 months ago (2012-10-15 08:02:32 UTC) #4
bartfab (slow)
https://chromiumcodereview.appspot.com/11078023/diff/3001/chrome/browser/policy/policy_prefs_browsertest.cc File chrome/browser/policy/policy_prefs_browsertest.cc (right): https://chromiumcodereview.appspot.com/11078023/diff/3001/chrome/browser/policy/policy_prefs_browsertest.cc#newcode526 chrome/browser/policy/policy_prefs_browsertest.cc:526: policy_test_case->indicator_test_setup_js())); On 2012/10/15 08:02:32, Joao da Silva wrote: > ...
8 years, 2 months ago (2012-10-15 12:34:07 UTC) #5
Joao da Silva
policy/ still lgtm, thanks! https://chromiumcodereview.appspot.com/11078023/diff/3001/chrome/browser/resources/options/content_settings_exceptions_area.js File chrome/browser/resources/options/content_settings_exceptions_area.js (right): https://chromiumcodereview.appspot.com/11078023/diff/3001/chrome/browser/resources/options/content_settings_exceptions_area.js#newcode59 chrome/browser/resources/options/content_settings_exceptions_area.js:59: this.dataItem.source : null; On 2012/10/15 ...
8 years, 2 months ago (2012-10-15 12:53:59 UTC) #6
markusheintz_
If James has no further comments about he JS/HTML/CSS code LGTM from my side.
8 years, 2 months ago (2012-10-16 15:36:36 UTC) #7
James Hawkins
lgtm
8 years, 2 months ago (2012-10-16 17:35:26 UTC) #8
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bartfab@chromium.org/11078023/20003
8 years, 2 months ago (2012-10-18 22:54:23 UTC) #9
commit-bot: I haz the power
8 years, 2 months ago (2012-10-19 02:59:50 UTC) #10
Change committed as 162897

Powered by Google App Engine
This is Rietveld 408576698