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

Issue 3038013: Disable proxy config button and show banner if proxy prefs are managed. (Closed)

Created:
10 years, 5 months ago by Mattias Nissler (ping if slow)
Modified:
9 years, 7 months ago
CC:
chromium-reviews, John Grabowski, pam+watch_chromium.org, ben+cc_chromium.org, Paweł Hajdan Jr.
Base URL:
http://src.chromium.org/git/chromium.git
Visibility:
Public.

Description

Disable proxy config button and show banner if proxy prefs are managed. Preferences.xib changes: Connect Enabled of the proxies configuration button to a getter in PreferencesWindowController. BUG=49538 TEST=Unit test in pref_set_observer_unittest.cc, manually configure proxy policies, check advanced options page. Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=53457

Patch Set 1 #

Total comments: 4

Patch Set 2 : address danno's comments #

Patch Set 3 : now including a unit test. #

Patch Set 4 : PrefSetObserver interface changes, use @property for MAC ui #

Total comments: 2

Patch Set 5 : fix nits #

Unified diffs Side-by-side diffs Delta from patch set Stats (+280 lines, -4 lines) Patch
M chrome/app/nibs/Preferences.xib View 1 2 3 1 chunk +16 lines, -0 lines 0 comments Download
M chrome/browser/cocoa/preferences_window_controller.h View 1 2 3 3 chunks +4 lines, -0 lines 0 comments Download
M chrome/browser/cocoa/preferences_window_controller.mm View 1 2 3 4 3 chunks +10 lines, -0 lines 0 comments Download
M chrome/browser/gtk/options/advanced_contents_gtk.cc View 1 2 3 4 4 chunks +23 lines, -3 lines 0 comments Download
M chrome/browser/managed_prefs_banner_base.cc View 1 chunk +5 lines, -0 lines 0 comments Download
A chrome/browser/pref_set_observer.h View 1 2 3 4 1 chunk +53 lines, -0 lines 0 comments Download
A chrome/browser/pref_set_observer.cc View 1 2 3 4 1 chunk +66 lines, -0 lines 0 comments Download
A chrome/browser/pref_set_observer_unittest.cc View 3 4 1 chunk +88 lines, -0 lines 0 comments Download
M chrome/browser/views/options/advanced_contents_view.cc View 1 2 3 4 3 chunks +11 lines, -0 lines 0 comments Download
M chrome/chrome_browser.gypi View 1 2 3 4 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/chrome_tests.gypi View 3 4 2 chunks +2 lines, -1 line 0 comments Download

Messages

Total messages: 6 (0 generated)
Mattias Nissler (ping if slow)
Please review. Do you feel comfortable reviewing the small win/linux changes or should I bring ...
10 years, 5 months ago (2010-07-21 14:12:13 UTC) #1
danno
unit tests for set observer? http://codereview.chromium.org/3038013/diff/1/4 File chrome/browser/cocoa/preferences_window_controller.mm (right): http://codereview.chromium.org/3038013/diff/1/4#newcode1388 chrome/browser/cocoa/preferences_window_controller.mm:1388: [self didChangeValueForKey:@"isProxiesConfigureButtonEnabled"]; As discussed, ...
10 years, 5 months ago (2010-07-21 16:16:08 UTC) #2
Mattias Nissler (ping if slow)
I'll write the unit test tomorrow. http://codereview.chromium.org/3038013/diff/1/4 File chrome/browser/cocoa/preferences_window_controller.mm (right): http://codereview.chromium.org/3038013/diff/1/4#newcode1388 chrome/browser/cocoa/preferences_window_controller.mm:1388: [self didChangeValueForKey:@"isProxiesConfigureButtonEnabled"]; On ...
10 years, 5 months ago (2010-07-21 16:38:52 UTC) #3
Mattias Nissler (ping if slow)
Danno, care to take another look?
10 years, 5 months ago (2010-07-22 14:20:32 UTC) #4
Paweł Hajdan Jr.
Drive-by with a minor test nit. http://codereview.chromium.org/3038013/diff/16001/17008 File chrome/browser/pref_set_observer_unittest.cc (right): http://codereview.chromium.org/3038013/diff/16001/17008#newcode56 chrome/browser/pref_set_observer_unittest.cc:56: std::wstring* pstr = ...
10 years, 5 months ago (2010-07-22 17:33:30 UTC) #5
danno
10 years, 5 months ago (2010-07-23 07:20:57 UTC) #6
LGTM

http://codereview.chromium.org/3038013/diff/16001/17007
File chrome/browser/pref_set_observer.h (right):

http://codereview.chromium.org/3038013/diff/16001/17007#newcode22
chrome/browser/pref_set_observer.h:22: // Add a |pref| to the set of preferences
to watch.
minor nit: is some places in the commands you use watch, some places track, in
other places is observed. Why not use observed everywhere? Perhaps also rename
the methods IsObserved?

Powered by Google App Engine
This is Rietveld 408576698