Content settings lists moved to sub-sub pages.

Content settings lists moved to sub-sub pages.

BUG=64153
TEST=manual

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

[stuartmorgan, 2010-12-20 22:55:37]

http://codereview.chromium.org/5964003/diff/2001/chrome/browser/resources/opt...
File chrome/browser/resources/options/content_settings.css (right):

http://codereview.chromium.org/5964003/diff/2001/chrome/browser/resources/opt...
chrome/browser/resources/options/content_settings.css:7:
#contentSettingsExceptionsArea list {
Lets move this style block to options_page.css with a class (.framed-list
maybe?) and then we can all just add the class as we convert lists instead of
duplicating style.

http://codereview.chromium.org/5964003/diff/2001/chrome/browser/resources/opt...
File chrome/browser/resources/options/content_settings_exceptions_area.js
(right):

http://codereview.chromium.org/5964003/diff/2001/chrome/browser/resources/opt...
chrome/browser/resources/options/content_settings_exceptions_area.js:568:
exceptionsLists[i].redraw();
Do you still need this? The expectation for autoexpanding lists is that redraw
shouldn't be necessary.

[Evan Stade, 2010-12-20 23:09:02]

I am adding the column headers, and it turns out the html structure has to
change a bit to accomodate them. please hold off on further review for now

http://codereview.chromium.org/5964003/diff/2001/chrome/browser/resources/opt...
File chrome/browser/resources/options/content_settings.css (right):

http://codereview.chromium.org/5964003/diff/2001/chrome/browser/resources/opt...
chrome/browser/resources/options/content_settings.css:7:
#contentSettingsExceptionsArea list {
On 2010/12/20 22:55:37, stuartmorgan wrote:
> Lets move this style block to options_page.css with a class (.framed-list
> maybe?) and then we can all just add the class as we convert lists instead of
> duplicating style.

ok

http://codereview.chromium.org/5964003/diff/2001/chrome/browser/resources/opt...
File chrome/browser/resources/options/content_settings_exceptions_area.js
(right):

http://codereview.chromium.org/5964003/diff/2001/chrome/browser/resources/opt...
chrome/browser/resources/options/content_settings_exceptions_area.js:568:
exceptionsLists[i].redraw();
On 2010/12/20 22:55:37, stuartmorgan wrote:
> Do you still need this? The expectation for autoexpanding lists is that redraw
> shouldn't be necessary.

I'll check

[Evan Stade, 2010-12-21 00:23:15]

both suggestions implemented. PTAL

[arv (Not doing code reviews), 2010-12-21 00:34:27]

http://codereview.chromium.org/5964003/diff/14001/chrome/browser/resources/op...
File chrome/browser/resources/options/content_settings.css (right):

http://codereview.chromium.org/5964003/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/content_settings.css:21: margin-left: 3px;
-webkit-margin-start

http://codereview.chromium.org/5964003/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/content_settings.css:25:
#exceptionColumnHeaders > span {
maybe use an hx?

http://codereview.chromium.org/5964003/diff/14001/chrome/browser/resources/op...
File chrome/browser/resources/options/content_settings_exceptions_area.js
(right):

http://codereview.chromium.org/5964003/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/content_settings_exceptions_area.js:525:
args.push(dataItem['origin']);
push takes var args

args.push(ataItem['origin'], dataItem['embeddingOrigin']);

http://codereview.chromium.org/5964003/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/content_settings_exceptions_area.js:550:
function ContentSettingsExceptionsArea() {
JSDoc?

http://codereview.chromium.org/5964003/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/content_settings_exceptions_area.js:577:
header.textContent = eval('templateData.' + type + '_header');
Do not use eval

object.foo is just syntactic sugar for object['foo']. With that knowledge you
can do:

header.textContent = templateData[type + '_header'];

http://codereview.chromium.org/5964003/diff/14001/chrome/browser/resources/op...
File chrome/browser/resources/options/startup_page_manager.css (right):

http://codereview.chromium.org/5964003/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/startup_page_manager.css:1:
#startupPagesFullList {
Can this file be removed now?

[stuartmorgan, 2010-12-21 01:19:17]

http://codereview.chromium.org/5964003/diff/14001/chrome/browser/resources/op...
File chrome/browser/resources/options/startup_page_manager.css (right):

http://codereview.chromium.org/5964003/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/startup_page_manager.css:1:
#startupPagesFullList {
On 2010/12/21 00:34:27, arv wrote:
> Can this file be removed now?

Yep; it's very likely that anything I need for the sub-page will be needed for
the main page given the duplicate UI in the current mocks.

[Evan Stade, 2010-12-21 02:09:00]

http://codereview.chromium.org/5964003/diff/14001/chrome/browser/resources/op...
File chrome/browser/resources/options/content_settings.css (right):

http://codereview.chromium.org/5964003/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/content_settings.css:21: margin-left: 3px;
On 2010/12/21 00:34:27, arv wrote:
> -webkit-margin-start

Done.

http://codereview.chromium.org/5964003/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/content_settings.css:25:
#exceptionColumnHeaders > span {
On 2010/12/21 00:34:27, arv wrote:
> maybe use an hx?

why?

http://codereview.chromium.org/5964003/diff/14001/chrome/browser/resources/op...
File chrome/browser/resources/options/content_settings_exceptions_area.js
(right):

http://codereview.chromium.org/5964003/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/content_settings_exceptions_area.js:525:
args.push(dataItem['origin']);
On 2010/12/21 00:34:27, arv wrote:
> push takes var args
> 
> args.push(ataItem['origin'], dataItem['embeddingOrigin']);

Done.

http://codereview.chromium.org/5964003/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/content_settings_exceptions_area.js:550:
function ContentSettingsExceptionsArea() {
On 2010/12/21 00:34:27, arv wrote:
> JSDoc?

Done.

http://codereview.chromium.org/5964003/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/content_settings_exceptions_area.js:577:
header.textContent = eval('templateData.' + type + '_header');
On 2010/12/21 00:34:27, arv wrote:
> Do not use eval
> 
> object.foo is just syntactic sugar for object['foo']. With that knowledge you
> can do:
> 
> header.textContent = templateData[type + '_header'];

Done.

http://codereview.chromium.org/5964003/diff/14001/chrome/browser/resources/op...
File chrome/browser/resources/options/startup_page_manager.css (right):

http://codereview.chromium.org/5964003/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/startup_page_manager.css:1:
#startupPagesFullList {
On 2010/12/21 01:19:17, stuartmorgan wrote:
> On 2010/12/21 00:34:27, arv wrote:
> > Can this file be removed now?
> 
> Yep; it's very likely that anything I need for the sub-page will be needed for
> the main page given the duplicate UI in the current mocks.

i removed the wrong file by mistake. I've fixed it, although for some reason
rietveld isn't showing the correct letter (M/D/etc.) for the files.

[stuartmorgan, 2010-12-21 17:25:43]

LGTM with nits

http://codereview.chromium.org/5964003/diff/20001/chrome/browser/resources/op...
File chrome/browser/resources/options/content_settings.css (right):

http://codereview.chromium.org/5964003/diff/20001/chrome/browser/resources/op...
chrome/browser/resources/options/content_settings.css:22: margin-bottom: 4px;
Alphabetize all of these.

http://codereview.chromium.org/5964003/diff/20001/chrome/browser/resources/op...
File chrome/browser/resources/options/content_settings_exceptions_area.html
(right):

http://codereview.chromium.org/5964003/diff/20001/chrome/browser/resources/op...
chrome/browser/resources/options/content_settings_exceptions_area.html:2:
<h1></h1>
Why is this empty?

[arv (Not doing code reviews), 2010-12-21 19:08:54]

LGTM with nits

http://codereview.chromium.org/5964003/diff/14001/chrome/browser/resources/op...
File chrome/browser/resources/options/content_settings.css (right):

http://codereview.chromium.org/5964003/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/content_settings.css:25:
#exceptionColumnHeaders > span {
On 2010/12/21 02:09:00, Evan Stade wrote:
> On 2010/12/21 00:34:27, arv wrote:
> > maybe use an hx?
> 
> why?

I saw the word "header" and went into default mode ;) Seeing that this is a
table header span is OK.

http://codereview.chromium.org/5964003/diff/20001/chrome/browser/resources/op...
chrome/browser/resources/options/content_settings.css:25:
#exceptionColumnHeaders > span {
Since you are using display block maybe you can use divs instead of spans?

http://codereview.chromium.org/5964003/diff/20001/chrome/browser/resources/op...
File chrome/browser/resources/options/content_settings_exceptions_area.js
(right):

http://codereview.chromium.org/5964003/diff/20001/chrome/browser/resources/op...
chrome/browser/resources/options/content_settings_exceptions_area.js:554: *
Encapsulated handling of content settings list subpage.
This goes on the ContentSettingsExceptionArea function.

[Evan Stade, 2010-12-21 19:54:08]

updated and synced

http://codereview.chromium.org/5964003/diff/20001/chrome/browser/resources/op...
File chrome/browser/resources/options/content_settings.css (right):

http://codereview.chromium.org/5964003/diff/20001/chrome/browser/resources/op...
chrome/browser/resources/options/content_settings.css:22: margin-bottom: 4px;
On 2010/12/21 17:25:44, stuartmorgan wrote:
> Alphabetize all of these.

Done.

http://codereview.chromium.org/5964003/diff/20001/chrome/browser/resources/op...
chrome/browser/resources/options/content_settings.css:25:
#exceptionColumnHeaders > span {
On 2010/12/21 19:08:54, arv wrote:
> Since you are using display block maybe you can use divs instead of spans?

ok.

http://codereview.chromium.org/5964003/diff/20001/chrome/browser/resources/op...
File chrome/browser/resources/options/content_settings_exceptions_area.html
(right):

http://codereview.chromium.org/5964003/diff/20001/chrome/browser/resources/op...
chrome/browser/resources/options/content_settings_exceptions_area.html:2:
<h1></h1>
On 2010/12/21 17:25:44, stuartmorgan wrote:
> Why is this empty?

the inner text is updated depending on which of the below divs is visible

http://codereview.chromium.org/5964003/diff/20001/chrome/browser/resources/op...
File chrome/browser/resources/options/content_settings_exceptions_area.js
(right):

http://codereview.chromium.org/5964003/diff/20001/chrome/browser/resources/op...
chrome/browser/resources/options/content_settings_exceptions_area.js:554: *
Encapsulated handling of content settings list subpage.
On 2010/12/21 19:08:54, arv wrote:
> This goes on the ContentSettingsExceptionArea function.

Done.