Flesh out the content settings exceptions lists a bit more.

Flesh out the content settings exceptions lists a bit more.

- Add "Add" button. Doesn't work.
- Add "Remove" button. Does work.
- Update the view when the model changes.

(Still only works for the images content type.)

BUG=48862
TEST=manual; unit tests

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

[Evan Stade, 2010-07-27 00:54:53]

jochen: HostContentSettingsMap changes (basically the reason for these changes
is to be able to update the view when the model changes. The current native
Options views do not update when the settings map changes, which is a bug.)

arv: the rest

[arv (Not doing code reviews), 2010-07-27 18:12:03]

http://codereview.chromium.org/3068005/diff/2001/3001
File chrome/browser/dom_ui/content_settings_handler.cc (right):

http://codereview.chromium.org/3068005/diff/2001/3001#newcode320
chrome/browser/dom_ui/content_settings_handler.cc:320: list_value->GetString(i,
&pattern);
DCHECK return value?

http://codereview.chromium.org/3068005/diff/2001/3004
File chrome/browser/host_content_settings_map.h (right):

http://codereview.chromium.org/3068005/diff/2001/3004#newcode85
chrome/browser/host_content_settings_map.h:85: explicit ContentSettingsDetails()
Isn't explicit supposed to be used with one param constructors?

http://codereview.chromium.org/3068005/diff/2001/3010
File chrome/browser/resources/options/content_settings_exceptions_list.js
(right):

http://codereview.chromium.org/3068005/diff/2001/3010#newcode40
chrome/browser/resources/options/content_settings_exceptions_list.js:40: //
actively being edited.
Can you style them when they are not focused to look like plain text?

http://codereview.chromium.org/3068005/diff/2001/3010#newcode45
chrome/browser/resources/options/content_settings_exceptions_list.js:45:
input.classList.add('exceptionInput');
I would use className = '...' since you know that there are no class names
present here

http://codereview.chromium.org/3068005/diff/2001/3010#newcode55
chrome/browser/resources/options/content_settings_exceptions_list.js:55:
select.classList.add('exceptionSelect');
and here

http://codereview.chromium.org/3068005/diff/2001/3010#newcode84
chrome/browser/resources/options/content_settings_exceptions_list.js:84:
this.parentNode.appendChild(addRow);
This seems incorrect. The component should not touch elements outside of it

http://codereview.chromium.org/3068005/diff/2001/3010#newcode97
chrome/browser/resources/options/content_settings_exceptions_list.js:97: };
or

removeReomve.onclick = cr.bind(this.removeSelectedRows, this);

http://codereview.chromium.org/3068005/diff/2001/3010#newcode132
chrome/browser/resources/options/content_settings_exceptions_list.js:132: var
removePatterns = new Array();
var removePatterns = [];

http://codereview.chromium.org/3068005/diff/2001/3010#newcode133
chrome/browser/resources/options/content_settings_exceptions_list.js:133: for
(var i = selection.getFirstIndex(); i >= 0;
selection.selectedIndexes or list.selectedItems

I submitted a change last week that removed getFirstIndex and getNextIndex. They
are now used by the selectionController and they are used for navigating the
selection using the keyboard.

[jochen (gone - plz use gerrit), 2010-07-27 18:16:46]

please adopt the consumer of the CONTENT_SETTINGS_CHANGED notification in
chrome/browser/tab_contents/tab_contents.cc

http://codereview.chromium.org/3068005/diff/2001/3004
File chrome/browser/host_content_settings_map.h (right):

http://codereview.chromium.org/3068005/diff/2001/3004#newcode76
chrome/browser/host_content_settings_map.h:76: explicit
ContentSettingsDetails(const Pattern& pattern,
also no explicit here.

[jochen (gone - plz use gerrit), 2010-07-27 18:23:02]

On 2010/07/27 18:16:46, jochen wrote:
> please adopt the consumer of the CONTENT_SETTINGS_CHANGED notification in
> chrome/browser/tab_contents/tab_contents.cc
actually, ignore that comment. it should still work.

[jochen (gone - plz use gerrit), 2010-07-27 18:24:08]

On 2010/07/27 18:23:02, jochen wrote:
> On 2010/07/27 18:16:46, jochen wrote:
> > please adopt the consumer of the CONTENT_SETTINGS_CHANGED notification in
> > chrome/browser/tab_contents/tab_contents.cc
> actually, ignore that comment. it should still work.

btw, I don't know the time plan for replacing the pref ui, but if it's not going
to happen very soon, please consider making the content exceptions table model
use these notifications, too

[Evan Stade, 2010-07-27 21:04:25]

> btw, I don't know the time plan for replacing the pref ui, 

ASAP (definitely before m7), so I don't think it's a good use of time to work on
improving the native ui.

http://codereview.chromium.org/3068005/diff/2001/3001
File chrome/browser/dom_ui/content_settings_handler.cc (right):

http://codereview.chromium.org/3068005/diff/2001/3001#newcode320
chrome/browser/dom_ui/content_settings_handler.cc:320: list_value->GetString(i,
&pattern);
On 2010/07/27 18:12:03, arv wrote:
> DCHECK return value?

Done.

http://codereview.chromium.org/3068005/diff/2001/3004
File chrome/browser/host_content_settings_map.h (right):

http://codereview.chromium.org/3068005/diff/2001/3004#newcode76
chrome/browser/host_content_settings_map.h:76: explicit
ContentSettingsDetails(const Pattern& pattern,
On 2010/07/27 18:16:47, jochen wrote:
> also no explicit here.

Done.

http://codereview.chromium.org/3068005/diff/2001/3004#newcode85
chrome/browser/host_content_settings_map.h:85: explicit ContentSettingsDetails()
On 2010/07/27 18:12:03, arv wrote:
> Isn't explicit supposed to be used with one param constructors?

yea, copy pasta fail

http://codereview.chromium.org/3068005/diff/2001/3010
File chrome/browser/resources/options/content_settings_exceptions_list.js
(right):

http://codereview.chromium.org/3068005/diff/2001/3010#newcode40
chrome/browser/resources/options/content_settings_exceptions_list.js:40: //
actively being edited.
On 2010/07/27 18:12:03, arv wrote:
> Can you style them when they are not focused to look like plain text?

yea, I want to do something like the bookmark manager (plain text until you
edit, then it turns into an input), but the whole concept of having an editing
mode (and validating input, etc) is a TODO at the moment.

http://codereview.chromium.org/3068005/diff/2001/3010#newcode45
chrome/browser/resources/options/content_settings_exceptions_list.js:45:
input.classList.add('exceptionInput');
On 2010/07/27 18:12:03, arv wrote:
> I would use className = '...' since you know that there are no class names
> present here

Done.

http://codereview.chromium.org/3068005/diff/2001/3010#newcode55
chrome/browser/resources/options/content_settings_exceptions_list.js:55:
select.classList.add('exceptionSelect');
On 2010/07/27 18:12:03, arv wrote:
> and here

Done.

http://codereview.chromium.org/3068005/diff/2001/3010#newcode97
chrome/browser/resources/options/content_settings_exceptions_list.js:97: };
On 2010/07/27 18:12:03, arv wrote:
> or
> 
> removeReomve.onclick = cr.bind(this.removeSelectedRows, this);

Done.

http://codereview.chromium.org/3068005/diff/2001/3010#newcode132
chrome/browser/resources/options/content_settings_exceptions_list.js:132: var
removePatterns = new Array();
On 2010/07/27 18:12:03, arv wrote:
> var removePatterns = [];

Done.

[arv (Not doing code reviews), 2010-07-28 06:02:09]

LGTM

http://codereview.chromium.org/3068005/diff/10001/21
File chrome/browser/resources/options/content_settings_exceptions_area.js
(right):

http://codereview.chromium.org/3068005/diff/10001/21#newcode128
chrome/browser/resources/options/content_settings_exceptions_area.js:128:
this.list = imagesExceptionsList;
How about a var fo imagesExceptionsList?

[jochen (gone - plz use gerrit), 2010-07-28 06:49:12]

LGTM, please run it by the trybots :)

On 2010/07/28 06:02:09, arv wrote:
> LGTM
> 
> http://codereview.chromium.org/3068005/diff/10001/21
> File chrome/browser/resources/options/content_settings_exceptions_area.js
> (right):
> 
> http://codereview.chromium.org/3068005/diff/10001/21#newcode128
> chrome/browser/resources/options/content_settings_exceptions_area.js:128:
> this.list = imagesExceptionsList;
> How about a var fo imagesExceptionsList?