Issue 2812063: Add starter code for 'Clear Browser Data' and 'Font Settings' overlay dialogs...

Issue 2812063: Add starter code for 'Clear Browser Data' and 'Font Settings' overlay dialogs... (Closed)

Created:
10 years, 5 months ago by csilv

Modified:
9 years, 7 months ago

Reviewers:
arv (Not doing code reviews), zel

CC:
chromium-reviews, ben+cc_chromium.org, James Hawkins, stuartmorgan, sargrass, Evan Stade

Base URL:
svn://svn.chromium.org/chrome/trunk/src/

Visibility:
Public.

Description

Add starter code for 'Clear Browser Data' and 'Font Settings' overlay dialogs. BUG=49037, 49038 TEST=Verify skeleton overlay dialogs for 'Clear Browser Data' and 'Font Settings' when using --enable-tabbed-options. Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=52930

Patch Set 1 #

Patch Set 2 : '' #

Total comments: 10

Patch Set 3 : '' #

Patch Set 4 : '' #

Created: 10 years, 5 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+187 lines, -16 lines)			Patch
A	chrome/browser/dom_ui/clear_browser_data_handler.h	View	1	1 chunk	+24 lines, -0 lines	0 comments	Download
A	chrome/browser/dom_ui/clear_browser_data_handler.cc	View	1	1 chunk	+25 lines, -0 lines	0 comments	Download
A	chrome/browser/dom_ui/font_settings_handler.h	View	1	1 chunk	+24 lines, -0 lines	0 comments	Download
A	chrome/browser/dom_ui/font_settings_handler.cc	View	1	1 chunk	+26 lines, -0 lines	0 comments	Download
M	chrome/browser/dom_ui/options_ui.cc	View	1 2 3	2 chunks	+4 lines, -0 lines	0 comments	Download
M	chrome/browser/resources/options.html	View	1 2 3	6 chunks	+9 lines, -9 lines	0 comments	Download
M	chrome/browser/resources/options/advanced_options.js	View	1 2 3	2 chunks	+3 lines, -7 lines	0 comments	Download
A	chrome/browser/resources/options/clear_browser_data_overlay.html	View		1 chunk	+3 lines, -0 lines	0 comments	Download
A	chrome/browser/resources/options/clear_browser_data_overlay.js	View	1 2 3	1 chunk	+31 lines, -0 lines	0 comments	Download
A	chrome/browser/resources/options/font_settings_overlay.html	View		1 chunk	+3 lines, -0 lines	0 comments	Download
A	chrome/browser/resources/options/font_settings_overlay.js	View	1 2 3	1 chunk	+31 lines, -0 lines	0 comments	Download
M	chrome/chrome_browser.gypi	View	1 2 3	1 chunk	+4 lines, -0 lines	0 comments	Download

Messages

Total messages: 4 (0 generated)

Expand Messages | Collapse Messages

arv (Not doing code reviews)

FYI http://codereview.chromium.org/2812063/diff/7001/8006 File chrome/browser/resources/options.html (right): http://codereview.chromium.org/2812063/diff/7001/8006#newcode45 chrome/browser/resources/options.html:45: // Create LocalStrings object. This comment adds no ...

10 years, 5 months ago (2010-07-19 16:38:29 UTC) #2

csilv

10 years, 5 months ago (2010-07-19 17:44:20 UTC) #4

http://codereview.chromium.org/2812063/diff/7001/8006
File chrome/browser/resources/options.html (right):

http://codereview.chromium.org/2812063/diff/7001/8006#newcode45
chrome/browser/resources/options.html:45: // Create LocalStrings object.
On 2010/07/19 16:38:30, arv wrote:
> This comment adds no useful information.
> 
> Comments should not duplicate the code, they should explain things that are
hard
> to understand by looking at the code or things that explains why the code is
> done in a certain way etc.

Done.

http://codereview.chromium.org/2812063/diff/7001/8006#newcode48
chrome/browser/resources/options.html:48: // Register pages.
On 2010/07/19 16:38:30, arv wrote:
> same here.

Done.

http://codereview.chromium.org/2812063/diff/7001/8006#newcode85
chrome/browser/resources/options.html:85: // TODO(csilv): Save/restore last
selected page.
On 2010/07/19 16:38:30, arv wrote:
> This is pretty easy to do. Inside showPageByName, store the shown page in
> localStorage and read it out in initialize.

Awesome, thanks for the pointer.

http://codereview.chromium.org/2812063/diff/7001/8009
File chrome/browser/resources/options/clear_browser_data_overlay.js (right):

http://codereview.chromium.org/2812063/diff/7001/8009#newcode21
chrome/browser/resources/options/clear_browser_data_overlay.js:21: // Initialize
ClearBrowserDataOverlay page.
On 2010/07/19 16:38:30, arv wrote:
> Use JSDoc comments instead.

Done.

http://codereview.chromium.org/2812063/diff/7001/8009#newcode26
chrome/browser/resources/options/clear_browser_data_overlay.js:26: //
TODO(csilv): add any needed initialization here or delete this method.
On 2010/07/19 16:38:30, arv wrote:
> or delete this class?

Done.

Expand Messages | Collapse Messages