Issue 2804039: First cut at the tabbed content settings page.

Issue 2804039: First cut at the tabbed content settings page. (Closed)

Created:
10 years, 5 months ago by Evan Stade

Modified:
9 years, 6 months ago

Reviewers:
dhg, arv (Not doing code reviews), zel

CC:
chromium-reviews, arv (Not doing code reviews), ben+cc_chromium.org, csilv

Base URL:
http://src.chromium.org/git/chromium.git

Visibility:
Public.

Description

First cut at the tabbed content settings page. Missing pieces: - Add the other tabs besides the first two. - The tab switching mechanism might need some work (it definitely needs to look better, and perhaps should also work with the back/forward buttons). - none of the <button>s work - I added the Content Settings page as one of the main options pages for now, but in the end it should show up when a user clicks the "Content Settings..." button in Under the Hood. There is a bit of extra code because content settings don't use prefs, so I had to sort of replicate the pref_ui code. BUG=none TEST=manual Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=52139

Patch Set 1 #

Total comments: 50

Patch Set 2 : arv comments #

Total comments: 1

Patch Set 3 : more CSSy #

Patch Set 4 : undo whitespace change #

Total comments: 18

Patch Set 5 : more improvements from arv #

Patch Set 6 : remove id from radios #

Total comments: 5

Patch Set 7 : more arv comments #

Patch Set 8 : no change #

Patch Set 9 : . #

Patch Set 10 : no change #

Created: 10 years, 5 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+457 lines, -3 lines)			Patch
A	chrome/browser/dom_ui/content_settings_handler.h	View	1 2	1 chunk	+28 lines, -0 lines	0 comments	Download
A	chrome/browser/dom_ui/content_settings_handler.cc	View	1 2 3 4	1 chunk	+182 lines, -0 lines	0 comments	Download
M	chrome/browser/dom_ui/dom_ui.h	View		1 chunk	+2 lines, -2 lines	0 comments	Download
M	chrome/browser/dom_ui/options_ui.cc	View	1 2 3 4 5 6 7	2 chunks	+2 lines, -0 lines	0 comments	Download
M	chrome/browser/resources/options.html	View	1 2 3 4 5 6 7 8	5 chunks	+7 lines, -1 line	0 comments	Download
A	chrome/browser/resources/options/content_settings.html	View	1 2 3 4 5	1 chunk	+96 lines, -0 lines	0 comments	Download
A	chrome/browser/resources/options/content_settings.js	View	1 2 3 4	1 chunk	+96 lines, -0 lines	0 comments	Download
A	chrome/browser/resources/options/content_settings_page.css	View		1 chunk	+16 lines, -0 lines	0 comments	Download
A	chrome/browser/resources/options/content_settings_ui.js	View	1 2 3 4 5 6	1 chunk	+26 lines, -0 lines	0 comments	Download
M	chrome/chrome_browser.gypi	View	1 2 3 4 5 6 7 8	1 chunk	+2 lines, -0 lines	0 comments	Download

Messages

Total messages: 13 (0 generated)

Expand Messages | Collapse Messages

arv (Not doing code reviews)

http://codereview.chromium.org/2804039/diff/1/2 File chrome/browser/dom_ui/content_settings_handler.cc (right): http://codereview.chromium.org/2804039/diff/1/2#newcode60 chrome/browser/dom_ui/content_settings_handler.cc:60: return std::string(); You used return ""; on line 35. ...

10 years, 5 months ago (2010-07-07 17:02:44 UTC) #2

Evan Stade

Thanks for your patience. I've fixed the patch according to your comments to the best ...

10 years, 5 months ago (2010-07-07 19:35:52 UTC) #3

Thanks for your patience. I've fixed the patch according to your comments to the
best of my abilities.

As I understood it, the plan was to get in a basically functional options page
and let you (arv) or alcor later improve the css/html/js it for best form and
function.

http://codereview.chromium.org/2804039/diff/1/2
File chrome/browser/dom_ui/content_settings_handler.cc (right):

http://codereview.chromium.org/2804039/diff/1/2#newcode60
chrome/browser/dom_ui/content_settings_handler.cc:60: return std::string();
On 2010/07/07 17:02:44, arv wrote:
> You used return ""; on line 35.

Done.

http://codereview.chromium.org/2804039/diff/1/2#newcode128
chrome/browser/dom_ui/content_settings_handler.cc:128:
dom_ui_->RegisterMessageCallback("contentFilterRadioToggled",
On 2010/07/07 17:02:44, arv wrote:
> These 2 seems a bit too specific.

I'm not sure what you mean. "Content" applies to cookies, images, javascript,
plugins, popups, and geolocation. The ThirdPartyCookies checkbox is not a
pref---toggling it requires special handling so it has to have its own callback.

http://codereview.chromium.org/2804039/diff/1/2#newcode137
chrome/browser/dom_ui/content_settings_handler.cc:137: // We send a list of the
<input> IDs that should be checked.
On 2010/07/07 17:02:44, arv wrote:
> Can we do this in some other way? It seems wrong to use the IDs as a
> representation of the logical values of these settings values.

I started out a more traiditional route, by sending a dict that went something
like

"cookies"->"allow"
"images"->"block"

and then on the other end unraveling that and mapping it to which <input>s are
checked. I ended up adding some custom attributes that basically just repeated
the id. That was a lot more verbose, seemingly needlessly so, since the IDs I
had already used encoded this data (and I don't see any reason why we'd want to
change the IDs such that they didn't).

http://codereview.chromium.org/2804039/diff/1/2#newcode176
chrome/browser/dom_ui/content_settings_handler.cc:176: bool block = false;
On 2010/07/07 17:02:44, arv wrote:
> bool block = checked == L"true";

Done.

http://codereview.chromium.org/2804039/diff/1/4
File chrome/browser/dom_ui/dom_ui.h (right):

http://codereview.chromium.org/2804039/diff/1/4#newcode175
chrome/browser/dom_ui/dom_ui.h:175: bool ExtractIntegerValue(const Value* value,
int* out_int);
On 2010/07/07 17:02:44, arv wrote:
> We should change the type to ListValue*.

that means every caller would have to check the type and then cast it. Pretty
cumbersome.

http://codereview.chromium.org/2804039/diff/1/6
File chrome/browser/resources/options.html (right):

http://codereview.chromium.org/2804039/diff/1/6#newcode56
chrome/browser/resources/options.html:56: // 
OptionsPage.register(SystemOptions.getInstance());
On 2010/07/07 17:02:44, arv wrote:
> Why is this commented out?

oh yea, this is just temporary until csilv's patch goes in. The tabbed options
doesn't work if you include the chromeos options page on non-chromeos.

http://codereview.chromium.org/2804039/diff/1/7
File chrome/browser/resources/options/content_settings.html (right):

http://codereview.chromium.org/2804039/diff/1/7#newcode4
chrome/browser/resources/options/content_settings.html:4: <table
class="contentSettingsTabs"><tr>
On 2010/07/07 17:02:44, arv wrote:
> Why is this a table?

because I know next to no html

http://codereview.chromium.org/2804039/diff/1/7#newcode15
chrome/browser/resources/options/content_settings.html:15: <span
i18n-content="cookies_modify"></span>
On 2010/07/07 17:02:44, arv wrote:
> Is this some kind of header? How about using a Hx tag?

the mock doesn't have any special formatting for this text.

http://codereview.chromium.org/2804039/diff/1/7#newcode16
chrome/browser/resources/options/content_settings.html:16: <table
class="option-control-table">
On 2010/07/07 17:02:44, arv wrote:
> Why is this a table? It only has one column so the table seems like overkill.

the format is basically just copied from the chromeos options page.

http://codereview.chromium.org/2804039/diff/1/7#newcode20
chrome/browser/resources/options/content_settings.html:20: name="cookies-radio"
content-settings-filter="bla">
On 2010/07/07 17:02:44, arv wrote:
> What is "bla"?

vestigial---removed.

http://codereview.chromium.org/2804039/diff/1/7#newcode20
chrome/browser/resources/options/content_settings.html:20: name="cookies-radio"
content-settings-filter="bla">
On 2010/07/07 17:02:44, arv wrote:
> indent

Done.

http://codereview.chromium.org/2804039/diff/1/7#newcode21
chrome/browser/resources/options/content_settings.html:21: <span
i18n-content="cookies_allow"></span>
On 2010/07/07 17:02:44, arv wrote:
> unindent

Done.

http://codereview.chromium.org/2804039/diff/1/8
File chrome/browser/resources/options/content_settings.js (right):

http://codereview.chromium.org/2804039/diff/1/8#newcode13
chrome/browser/resources/options/content_settings.js:13: OptionsPage.call(this,
'content', "Content Settings",
On 2010/07/07 17:02:44, arv wrote:
> i18n?

Done.

http://codereview.chromium.org/2804039/diff/1/8#newcode23
chrome/browser/resources/options/content_settings.js:23:
ContentSettings.showTabByName = function(tabId) {
On 2010/07/07 17:02:44, arv wrote:
> Why is this a static function?

copied from options_page.js. I can easily change it but what is the advantage
(honest question---I'm sure there's a reason it should be an instance method but
I don't understand enough js to know why)

http://codereview.chromium.org/2804039/diff/1/8#newcode24
chrome/browser/resources/options/content_settings.js:24:
$('cookiesFilterTab').style.display = (tabId == 'cookies-nav-tab') ?
On 2010/07/07 17:02:44, arv wrote:
> Useless parentheses

Done.

http://codereview.chromium.org/2804039/diff/1/8#newcode24
chrome/browser/resources/options/content_settings.js:24:
$('cookiesFilterTab').style.display = (tabId == 'cookies-nav-tab') ?
On 2010/07/07 17:02:44, arv wrote:
> This is usually cleaner to handle in CSS. You can set a css class name or a
> custom attribute and then use CSS selectors to hide and show the different
> parts.

This is the same logic as showPageByName in OptionsPage.

http://codereview.chromium.org/2804039/diff/1/8#newcode44
chrome/browser/resources/options/content_settings.js:44:
$('cookies-nav-tab').onclick = ContentSettings.navTabClicked;
On 2010/07/07 17:02:44, arv wrote:
> Either use a local function for the onclick handlers here or bind it to an
> instance method. Mixing instance methods with statics is a recipe for failure.

Done.

http://codereview.chromium.org/2804039/diff/1/8#newcode45
chrome/browser/resources/options/content_settings.js:45:
$('images-nav-tab').onclick = ContentSettings.navTabClicked;
On 2010/07/07 17:02:44, arv wrote:
> If you expect there to be more tabs then it probably makes sense to listen to
> click at a higher level in the tree.

Done.

http://codereview.chromium.org/2804039/diff/1/8#newcode69
chrome/browser/resources/options/content_settings.js:69: for (var i in list) {
On 2010/07/07 17:02:44, arv wrote:
> Do not use for in loops for arrays.

Done.

http://codereview.chromium.org/2804039/diff/1/9
File chrome/browser/resources/options/content_settings_ui.js (right):

http://codereview.chromium.org/2804039/diff/1/9#newcode15
chrome/browser/resources/options/content_settings_ui.js:15: function(e) {
On 2010/07/07 17:02:44, arv wrote:
> Fix indentation

Done.

Evan Stade

Stuart's been helping me with the css. I'll work on it some more and get ...

10 years, 5 months ago (2010-07-08 01:27:16 UTC) #4

zel

FYI, David has css changed at http://codereview.chromium.org/2881013/show that put us visually much closer to the ...

10 years, 5 months ago (2010-07-08 01:32:13 UTC) #5

dhg

http://codereview.chromium.org/2804039/diff/9001/10005 File chrome/browser/resources/options.html (right): http://codereview.chromium.org/2804039/diff/9001/10005#newcode56 chrome/browser/resources/options.html:56: // OptionsPage.register(SystemOptions.getInstance()); Why is this commented out?

10 years, 5 months ago (2010-07-08 16:51:02 UTC) #6

arv (Not doing code reviews)

On Wed, Jul 7, 2010 at 12:35, <estade@chromium.org> wrote: > Thanks for your patience. I've ...

10 years, 5 months ago (2010-07-08 17:17:55 UTC) #7

On Wed, Jul 7, 2010 at 12:35,  <estade@chromium.org> wrote:
> Thanks for your patience. I've fixed the patch according to your comments to
> the
> best of my abilities.
>
> As I understood it, the plan was to get in a basically functional options
> page
> and let you (arv) or alcor later improve the css/html/js it for best form
> and
> function.

I don't think that is a viable plan. There is just too much work for
us to do to have time to ignore building things the right way. Sure,
this is early in the process and I expect a lot of iterations but I'd
rather see us have a solid base to iterate on.

One important thing to make the UI clean is OO JS. Another is a clean,
minimal DOM where we separate content from presentation.

> http://codereview.chromium.org/2804039/diff/1/2#newcode128
> chrome/browser/dom_ui/content_settings_handler.cc:128:
> dom_ui_->RegisterMessageCallback("contentFilterRadioToggled",
> On 2010/07/07 17:02:44, arv wrote:
>>
>> These 2 seems a bit too specific.
>
> I'm not sure what you mean. "Content" applies to cookies, images,
> javascript, plugins, popups, and geolocation. The ThirdPartyCookies
> checkbox is not a pref---toggling it requires special handling so it has
> to have its own callback.

The name implies a radio button being toggled. I think it would be
cleaner if this was called something like
chrome.send('setContentFilter', ['name', 'value'])

>
> http://codereview.chromium.org/2804039/diff/1/2#newcode137
> chrome/browser/dom_ui/content_settings_handler.cc:137: // We send a list
> of the <input> IDs that should be checked.
> On 2010/07/07 17:02:44, arv wrote:
>>
>> Can we do this in some other way? It seems wrong to use the IDs as a
>> representation of the logical values of these settings values.
>
> I started out a more traiditional route, by sending a dict that went
> something like
>
> "cookies"->"allow"
> "images"->"block"
>
> and then on the other end unraveling that and mapping it to which
> <input>s are checked. I ended up adding some custom attributes that
> basically just repeated the id. That was a lot more verbose, seemingly
> needlessly so, since the IDs I had already used encoded this data (and I
> don't see any reason why we'd want to change the IDs such that they
> didn't).

I always see the handler as an API. I would like to be able to
completely rewrite the HTML UI without having to make changes to the
handlers and making this API depend on IDs in the HTML seems a bit
brittle.

Another argument is that ID is for finding elements in the DOM, as
well as unique styling. Encoding arbitrary information in the ID seems
wrong.

I understand that it is tempting since it leads to less code but I'm
not sure it will lead to less work in the long run.

> http://codereview.chromium.org/2804039/diff/1/2#newcode176
> chrome/browser/dom_ui/content_settings_handler.cc:176: bool block =
> false;
> On 2010/07/07 17:02:44, arv wrote:
>>
>> bool block = checked == L"true";
>
> Done.
>
> http://codereview.chromium.org/2804039/diff/1/4
> File chrome/browser/dom_ui/dom_ui.h (right):
>
> http://codereview.chromium.org/2804039/diff/1/4#newcode175
> chrome/browser/dom_ui/dom_ui.h:175: bool ExtractIntegerValue(const
> Value* value, int* out_int);
> On 2010/07/07 17:02:44, arv wrote:
>>
>> We should change the type to ListValue*.
>
> that means every caller would have to check the type and then cast it.
> Pretty cumbersome.

The thing is the that chrome.send requires a list so we should just
update the whole call chain to use a ListValue. That would give us
static type checking here.

I wasn't asking you to do this now. Just that we should do this
eventually. I'll file a bug.

>
> http://codereview.chromium.org/2804039/diff/1/6
> File chrome/browser/resources/options.html (right):
>
> http://codereview.chromium.org/2804039/diff/1/6#newcode56
> chrome/browser/resources/options.html:56: //
> OptionsPage.register(SystemOptions.getInstance());
> On 2010/07/07 17:02:44, arv wrote:
>>
>> Why is this commented out?
>
> oh yea, this is just temporary until csilv's patch goes in. The tabbed
> options doesn't work if you include the chromeos options page on
> non-chromeos.
>
> http://codereview.chromium.org/2804039/diff/1/7
> File chrome/browser/resources/options/content_settings.html (right):
>
> http://codereview.chromium.org/2804039/diff/1/7#newcode4
> chrome/browser/resources/options/content_settings.html:4: <table
> class="contentSettingsTabs"><tr>
> On 2010/07/07 17:02:44, arv wrote:
>>
>> Why is this a table?
>
> because I know next to no html
>
> http://codereview.chromium.org/2804039/diff/1/7#newcode15
> chrome/browser/resources/options/content_settings.html:15: <span
> i18n-content="cookies_modify"></span>
> On 2010/07/07 17:02:44, arv wrote:
>>
>> Is this some kind of header? How about using a Hx tag?
>
> the mock doesn't have any special formatting for this text.
>
> http://codereview.chromium.org/2804039/diff/1/7#newcode16
> chrome/browser/resources/options/content_settings.html:16: <table
> class="option-control-table">
> On 2010/07/07 17:02:44, arv wrote:
>>
>> Why is this a table? It only has one column so the table seems like
>
> overkill.
>
> the format is basically just copied from the chromeos options page.
>
> http://codereview.chromium.org/2804039/diff/1/7#newcode20
> chrome/browser/resources/options/content_settings.html:20:
> name="cookies-radio" content-settings-filter="bla">
> On 2010/07/07 17:02:44, arv wrote:
>>
>> What is "bla"?
>
> vestigial---removed.
>
> http://codereview.chromium.org/2804039/diff/1/7#newcode20
> chrome/browser/resources/options/content_settings.html:20:
> name="cookies-radio" content-settings-filter="bla">
> On 2010/07/07 17:02:44, arv wrote:
>>
>> indent
>
> Done.
>
> http://codereview.chromium.org/2804039/diff/1/7#newcode21
> chrome/browser/resources/options/content_settings.html:21: <span
> i18n-content="cookies_allow"></span>
> On 2010/07/07 17:02:44, arv wrote:
>>
>> unindent
>
> Done.
>
> http://codereview.chromium.org/2804039/diff/1/8
> File chrome/browser/resources/options/content_settings.js (right):
>
> http://codereview.chromium.org/2804039/diff/1/8#newcode13
> chrome/browser/resources/options/content_settings.js:13:
> OptionsPage.call(this, 'content', "Content Settings",
> On 2010/07/07 17:02:44, arv wrote:
>>
>> i18n?
>
> Done.
>
> http://codereview.chromium.org/2804039/diff/1/8#newcode23
> chrome/browser/resources/options/content_settings.js:23:
> ContentSettings.showTabByName = function(tabId) {
> On 2010/07/07 17:02:44, arv wrote:
>>
>> Why is this a static function?
>
> copied from options_page.js. I can easily change it but what is the
> advantage (honest question---I'm sure there's a reason it should be an
> instance method but I don't understand enough js to know why)
>
> http://codereview.chromium.org/2804039/diff/1/8#newcode24
> chrome/browser/resources/options/content_settings.js:24:
> $('cookiesFilterTab').style.display = (tabId == 'cookies-nav-tab') ?
> On 2010/07/07 17:02:44, arv wrote:
>>
>> Useless parentheses
>
> Done.
>
> http://codereview.chromium.org/2804039/diff/1/8#newcode24
> chrome/browser/resources/options/content_settings.js:24:
> $('cookiesFilterTab').style.display = (tabId == 'cookies-nav-tab') ?
> On 2010/07/07 17:02:44, arv wrote:
>>
>> This is usually cleaner to handle in CSS. You can set a css class name
>
> or a
>>
>> custom attribute and then use CSS selectors to hide and show the
>
> different
>>
>> parts.
>
> This is the same logic as showPageByName in OptionsPage.
>
> http://codereview.chromium.org/2804039/diff/1/8#newcode44
> chrome/browser/resources/options/content_settings.js:44:
> $('cookies-nav-tab').onclick = ContentSettings.navTabClicked;
> On 2010/07/07 17:02:44, arv wrote:
>>
>> Either use a local function for the onclick handlers here or bind it
>
> to an
>>
>> instance method. Mixing instance methods with statics is a recipe for
>
> failure.
>
> Done.
>
> http://codereview.chromium.org/2804039/diff/1/8#newcode45
> chrome/browser/resources/options/content_settings.js:45:
> $('images-nav-tab').onclick = ContentSettings.navTabClicked;
> On 2010/07/07 17:02:44, arv wrote:
>>
>> If you expect there to be more tabs then it probably makes sense to
>
> listen to
>>
>> click at a higher level in the tree.
>
> Done.
>
> http://codereview.chromium.org/2804039/diff/1/8#newcode69
> chrome/browser/resources/options/content_settings.js:69: for (var i in
> list) {
> On 2010/07/07 17:02:44, arv wrote:
>>
>> Do not use for in loops for arrays.
>
> Done.
>
> http://codereview.chromium.org/2804039/diff/1/9
> File chrome/browser/resources/options/content_settings_ui.js (right):
>
> http://codereview.chromium.org/2804039/diff/1/9#newcode15
> chrome/browser/resources/options/content_settings_ui.js:15: function(e)
> {
> On 2010/07/07 17:02:44, arv wrote:
>>
>> Fix indentation
>
> Done.
>
> http://codereview.chromium.org/2804039/show
>

arv (Not doing code reviews)

http://codereview.chromium.org/2804039/diff/1/2 File chrome/browser/dom_ui/content_settings_handler.cc (right): http://codereview.chromium.org/2804039/diff/1/2#newcode161 chrome/browser/dom_ui/content_settings_handler.cc:161: SplitString(checked, '-', &name_components); Can we pass an array or ...

10 years, 5 months ago (2010-07-08 17:23:34 UTC) #8

Evan Stade

http://codereview.chromium.org/2804039/diff/1/2 File chrome/browser/dom_ui/content_settings_handler.cc (right): http://codereview.chromium.org/2804039/diff/1/2#newcode137 chrome/browser/dom_ui/content_settings_handler.cc:137: // We send a list of the <input> IDs ...

10 years, 5 months ago (2010-07-08 23:01:29 UTC) #9

http://codereview.chromium.org/2804039/diff/1/2
File chrome/browser/dom_ui/content_settings_handler.cc (right):

http://codereview.chromium.org/2804039/diff/1/2#newcode137
chrome/browser/dom_ui/content_settings_handler.cc:137: // We send a list of the
<input> IDs that should be checked.
re-done.

http://codereview.chromium.org/2804039/diff/1/2#newcode161
chrome/browser/dom_ui/content_settings_handler.cc:161: SplitString(checked, '-',
&name_components);
On 2010/07/08 17:23:34, arv wrote:
> Can we pass an array or object with the relevan parts instead of parsing a
> string?

Done.

http://codereview.chromium.org/2804039/diff/1/7
File chrome/browser/resources/options/content_settings.html (right):

http://codereview.chromium.org/2804039/diff/1/7#newcode4
chrome/browser/resources/options/content_settings.html:4: <table
class="contentSettingsTabs"><tr>
On 2010/07/08 17:23:34, arv wrote:
> On 2010/07/07 19:35:52, Evan Stade wrote:
> > On 2010/07/07 17:02:44, arv wrote:
> > > Why is this a table?
> > 
> > because I know next to no html
> 
> <div>
>   <span i18n-content="cookies_tab_label"></span>
>   <span i18n-content="images_tab_label"></span>
> </div>

Done.

http://codereview.chromium.org/2804039/diff/1/7#newcode47
chrome/browser/resources/options/content_settings.html:47: <table
class="option-control-table">
On 2010/07/07 17:02:44, arv wrote:
> same here

I left these tables for consistency with the other options pages. I could try to
use a different layout, but I assume we will want the css styling to apply to
all options pages in the same way.

http://codereview.chromium.org/2804039/diff/1/8
File chrome/browser/resources/options/content_settings.js (right):

http://codereview.chromium.org/2804039/diff/1/8#newcode24
chrome/browser/resources/options/content_settings.js:24:
$('cookiesFilterTab').style.display = (tabId == 'cookies-nav-tab') ?
On 2010/07/07 17:02:44, arv wrote:
> This is usually cleaner to handle in CSS. You can set a css class name or a
> custom attribute and then use CSS selectors to hide and show the different
> parts.

Done.

arv (Not doing code reviews)

A few more small things. http://codereview.chromium.org/2804039/diff/23001/24001 File chrome/browser/dom_ui/content_settings_handler.cc (right): http://codereview.chromium.org/2804039/diff/23001/24001#newcode78 chrome/browser/dom_ui/content_settings_handler.cc:78: ContentSettingsHandler::ContentSettingsHandler() { Should empty ...

10 years, 5 months ago (2010-07-09 00:19:54 UTC) #10

Evan Stade

http://codereview.chromium.org/2804039/diff/23001/24001 File chrome/browser/dom_ui/content_settings_handler.cc (right): http://codereview.chromium.org/2804039/diff/23001/24001#newcode78 chrome/browser/dom_ui/content_settings_handler.cc:78: ContentSettingsHandler::ContentSettingsHandler() { On 2010/07/09 00:19:54, arv wrote: > Should ...

10 years, 5 months ago (2010-07-09 01:31:41 UTC) #11

http://codereview.chromium.org/2804039/diff/23001/24001
File chrome/browser/dom_ui/content_settings_handler.cc (right):

http://codereview.chromium.org/2804039/diff/23001/24001#newcode78
chrome/browser/dom_ui/content_settings_handler.cc:78:
ContentSettingsHandler::ContentSettingsHandler() {
On 2010/07/09 00:19:54, arv wrote:
> Should empty constuctors/destructors go in the .h file?

the style doesn't seem to be consistent across the codebase---I've seen it both
ways.

http://codereview.chromium.org/2804039/diff/23001/24006
File chrome/browser/resources/options/content_settings.html (right):

http://codereview.chromium.org/2804039/diff/23001/24006#newcode4
chrome/browser/resources/options/content_settings.html:4: <span
i18n-content="cookies_tab_label" id="cookies-nav-tab"
On 2010/07/09 00:19:54, arv wrote:
> 2 space indentation

Done.

http://codereview.chromium.org/2804039/diff/23001/24006#newcode18
chrome/browser/resources/options/content_settings.html:18: <input
id="cookies-radio-allow" type="radio"
On 2010/07/09 00:19:54, arv wrote:
> Last comment on this...
> 
> It seems like the value should be "allow".
> 
> <input type=radio name=cookies value=allow>
> 
> Then you can use querySelector to find the selected value
> 
> document.querySelector('input[type=radio][name=cookies]:checked').value
> 
> and to set it you can do something like
> 
> document.querySelector('input[type=radio][name=cookies][value=allow]').checked
=
> true;

Done.

http://codereview.chromium.org/2804039/diff/23001/24007
File chrome/browser/resources/options/content_settings.js (right):

http://codereview.chromium.org/2804039/diff/23001/24007#newcode37
chrome/browser/resources/options/content_settings.js:37: } (this);
On 2010/07/09 00:19:54, arv wrote:
> A more common pattern is the following:
> 
> var self = this;
> $('content-settings-nav-tabs').onclick = function(event) {
>   self.showTab(event.srcElement);
> };

that looks a bit nicer!

http://codereview.chromium.org/2804039/diff/23001/24007#newcode81
chrome/browser/resources/options/content_settings.js:81: * @param {object} dict
A mapping from radio groups to the checked value for
On 2010/07/09 00:19:54, arv wrote:
> @param {Object}

Done.

http://codereview.chromium.org/2804039/diff/23001/24007#newcode82
chrome/browser/resources/options/content_settings.js:82: * that group.
On 2010/07/09 00:19:54, arv wrote:
> indent 4

Done.

http://codereview.chromium.org/2804039/diff/23001/24007#newcode87
chrome/browser/resources/options/content_settings.js:87: for (var i = 0; i <
nodes.length; i++) {
On 2010/07/09 00:19:54, arv wrote:
> I think the inner loop can be replaced with a single document.querySelector.

Done.

http://codereview.chromium.org/2804039/diff/23001/24009
File chrome/browser/resources/options/content_settings_ui.js (right):

http://codereview.chromium.org/2804039/diff/23001/24009#newcode21
chrome/browser/resources/options/content_settings_ui.js:21:
this.addEventListener('click',
On 2010/07/09 00:19:54, arv wrote:
> Use 'change' for input[type=radio]. Click fires even when the radio is already
> checked.

Done.

http://codereview.chromium.org/2804039/diff/23001/24009#newcode23
chrome/browser/resources/options/content_settings_ui.js:23:
chrome.send('setContentFilter', [this.name, this.setting]);
On 2010/07/09 00:19:54, arv wrote:
> If you used value instead of setting there is no need for the getter below.

Done.

arv (Not doing code reviews)

LGTM with the last few issues resolved. Thanks for your patience. http://codereview.chromium.org/2804039/diff/19003/35005 File chrome/browser/resources/options.html (right): ...

10 years, 5 months ago (2010-07-09 02:20:38 UTC) #12

Evan Stade

10 years, 5 months ago (2010-07-09 19:08:57 UTC) #13

thanks for your patience and guidance!

http://codereview.chromium.org/2804039/diff/19003/35005
File chrome/browser/resources/options.html (right):

http://codereview.chromium.org/2804039/diff/19003/35005#newcode91
chrome/browser/resources/options.html:91:
cr.ui.decorate('input[value][type=radio]', ContentSettingsRadio);
On 2010/07/09 02:20:40, arv wrote:
> This might need a bit more specific CSS selector. How about
> 
> #contentSettingsPage input[type=radio]
> 
> This will match all radio inputs inside the contentSettingsPage element.

Done.

http://codereview.chromium.org/2804039/diff/19003/35009
File chrome/browser/resources/options/content_settings_ui.js (right):

http://codereview.chromium.org/2804039/diff/19003/35009#newcode21
chrome/browser/resources/options/content_settings_ui.js:21:
this.addEventListener('click',
On 2010/07/09 02:20:40, arv wrote:
> s/click/change

huh, weird, thought I changed these two things already. fixed.

Expand Messages | Collapse Messages