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

Issue 10827141: Move handling of dialog preferences to Preferences class (Closed)

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

Description

Move handling of dialog preferences to Preferences class This CL extends the Preferences class to distinguish between prefs that get committed to Chrome automatically (regular prefs) and those that require an explicit commit or rollback (dialog prefs). Changes to either type of pref cause notifications to propagate throughout the UI, ensuring that UI elements are updated without any special-case handling of dialog preferences. The UI element classes are updated to work with the new Preferences class. They still contain a lot of copy & pasted code. I will clean this up in a follow-up CL. BUG=104955 This CL depends on https://chromiumcodereview.appspot.com/10834109/ Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=154676

Patch Set 1 #

Patch Set 2 : Add missing property definition. #

Patch Set 3 : Fix my rushed copy & paste :( #

Total comments: 14

Patch Set 4 : Addressed comments, added tests. #

Patch Set 5 : clang compilation fixes. #

Patch Set 6 : Fixed PrefCheckbox. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1133 lines, -604 lines) Patch
M chrome/browser/resources/options/browser_options.js View 1 2 3 4 5 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/resources/options/chromeos/network_list.js View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/resources/options/chromeos/proxy_rules_list.js View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/resources/options/controlled_setting.js View 1 2 3 2 chunks +11 lines, -1 line 0 comments Download
M chrome/browser/resources/options/font_settings.js View 1 2 3 3 chunks +34 lines, -28 lines 0 comments Download
M chrome/browser/resources/options/import_data_overlay.js View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/resources/options/language_list.js View 1 2 3 1 chunk +2 lines, -2 lines 0 comments Download
M chrome/browser/resources/options/language_options.js View 1 2 3 2 chunks +2 lines, -2 lines 0 comments Download
M chrome/browser/resources/options/pref_ui.js View 1 2 3 4 5 22 chunks +183 lines, -247 lines 0 comments Download
M chrome/browser/resources/options/preferences.js View 1 2 3 6 chunks +174 lines, -23 lines 0 comments Download
M chrome/browser/resources/options/settings_dialog.js View 1 2 3 2 chunks +6 lines, -4 lines 0 comments Download
M chrome/browser/resources/options/spelling_confirm_overlay.js View 1 2 3 1 chunk +4 lines, -2 lines 0 comments Download
M chrome/browser/ui/webui/options/core_options_handler.cc View 1 2 3 1 chunk +4 lines, -0 lines 0 comments Download
M chrome/browser/ui/webui/options/preferences_browsertest.h View 1 2 3 4 4 chunks +127 lines, -33 lines 0 comments Download
M chrome/browser/ui/webui/options/preferences_browsertest.cc View 1 2 3 4 5 chunks +582 lines, -258 lines 0 comments Download

Messages

Total messages: 13 (0 generated)
bartfab (slow)
Hi James Here is the next CL that implements dialog pref handling in a central ...
8 years, 4 months ago (2012-08-02 18:00:37 UTC) #1
James Hawkins
https://chromiumcodereview.appspot.com/10827141/diff/5001/chrome/browser/resources/options2/browser_options.js File chrome/browser/resources/options2/browser_options.js (right): https://chromiumcodereview.appspot.com/10827141/diff/5001/chrome/browser/resources/options2/browser_options.js#newcode369 chrome/browser/resources/options2/browser_options.js:369: true); Optional nit: Condense parameters to save a line, ...
8 years, 4 months ago (2012-08-02 20:33:32 UTC) #2
bartfab (slow)
https://chromiumcodereview.appspot.com/10827141/diff/5001/chrome/browser/resources/options2/browser_options.js File chrome/browser/resources/options2/browser_options.js (right): https://chromiumcodereview.appspot.com/10827141/diff/5001/chrome/browser/resources/options2/browser_options.js#newcode369 chrome/browser/resources/options2/browser_options.js:369: true); On 2012/08/02 20:33:32, James Hawkins wrote: > Optional ...
8 years, 3 months ago (2012-08-31 14:14:30 UTC) #3
James Hawkins
lgtm
8 years, 3 months ago (2012-08-31 15:18:30 UTC) #4
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bartfab@chromium.org/10827141/12001
8 years, 3 months ago (2012-08-31 15:43:12 UTC) #5
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bartfab@chromium.org/10827141/16005
8 years, 3 months ago (2012-08-31 16:38:08 UTC) #6
commit-bot: I haz the power
Failed to request the patch to try. Please note that binary filesare still unsupported at ...
8 years, 3 months ago (2012-08-31 16:52:05 UTC) #7
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bartfab@chromium.org/10827141/16005
8 years, 3 months ago (2012-08-31 16:56:55 UTC) #8
commit-bot: I haz the power
Failed to request the patch to try. Please note that binary filesare still unsupported at ...
8 years, 3 months ago (2012-08-31 17:08:39 UTC) #9
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bartfab@chromium.org/10827141/16005
8 years, 3 months ago (2012-08-31 20:31:14 UTC) #10
commit-bot: I haz the power
Try job failure for 10827141-16005 (retry) on linux_rel for step "browser_tests". It's a second try, ...
8 years, 3 months ago (2012-08-31 22:11:45 UTC) #11
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bartfab@chromium.org/10827141/21002
8 years, 3 months ago (2012-09-03 08:36:56 UTC) #12
commit-bot: I haz the power
8 years, 3 months ago (2012-09-03 10:57:44 UTC) #13
Change committed as 154676

Powered by Google App Engine
This is Rietveld 408576698