More content settings exceptions

[Options DOMUI] More content settings exceptions work

BUG=48862
TEST=editing image exceptions works

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

[Evan Stade, 2010-07-29 00:17:28]

http://codereview.chromium.org/2883045/diff/5001/6003
File chrome/browser/resources/options/content_settings_exceptions_area.js
(right):

http://codereview.chromium.org/2883045/diff/5001/6003#newcode117
chrome/browser/resources/options/content_settings_exceptions_area.js:117: set
editing(editing) {
largely copied from bookmark_list.js

http://codereview.chromium.org/2883045/diff/5001/6003#newcode215
chrome/browser/resources/options/content_settings_exceptions_area.js:215: //
TODO(estade): update the C++ model.
I haven't done this part yet because it needs work on the C++ side. Currently if
you edit an exception, it removes the old exception and adds a new one, which
makes the edited exception go to the end of the list. This is already pretty
ugly in the current native UI, but would be even worse with inline editing.

[arv (Not doing code reviews), 2010-07-29 07:20:13]

I'm in all day meetings Thursday. I might not be able to get to this
until Friday.

erik



On Wed, Jul 28, 2010 at 17:17,  <estade@chromium.org> wrote:
> Reviewers: arv,
>
>
> http://codereview.chromium.org/2883045/diff/5001/6003
> File
> chrome/browser/resources/options/content_settings_exceptions_area.js
> (right):
>
> http://codereview.chromium.org/2883045/diff/5001/6003#newcode117
> chrome/browser/resources/options/content_settings_exceptions_area.js:117:
> set editing(editing) {
> largely copied from bookmark_list.js
>
> http://codereview.chromium.org/2883045/diff/5001/6003#newcode215
> chrome/browser/resources/options/content_settings_exceptions_area.js:215:
> // TODO(estade): update the C++ model.
> I haven't done this part yet because it needs work on the C++ side.
> Currently if you edit an exception, it removes the old exception and
> adds a new one, which makes the edited exception go to the end of the
> list. This is already pretty ugly in the current native UI, but would be
> even worse with inline editing.
>
> Description:
> [Options DOMUI] More content settings exceptions work
>
> BUG=48862
> TEST=editing image exceptions works (although doesn't actually persist the
> changes)
>
> Please review this at http://codereview.chromium.org/2883045/show
>
> SVN Base: http://src.chromium.org/git/chromium.git
>
> Affected files:
>  M chrome/browser/dom_ui/content_settings_handler.cc
>  M chrome/browser/resources/options/content_settings_exceptions_area.css
>  M chrome/browser/resources/options/content_settings_exceptions_area.js
>
>
>

[Evan Stade, 2010-07-30 02:56:27]

On 2010/07/29 00:17:28, Evan Stade wrote:
> http://codereview.chromium.org/2883045/diff/5001/6003
> File chrome/browser/resources/options/content_settings_exceptions_area.js
> (right):
> 
> http://codereview.chromium.org/2883045/diff/5001/6003#newcode117
> chrome/browser/resources/options/content_settings_exceptions_area.js:117: set
> editing(editing) {
> largely copied from bookmark_list.js
> 
> http://codereview.chromium.org/2883045/diff/5001/6003#newcode215
> chrome/browser/resources/options/content_settings_exceptions_area.js:215: //
> TODO(estade): update the C++ model.
> I haven't done this part yet because it needs work on the C++ side. Currently
if
> you edit an exception, it removes the old exception and adds a new one, which
> makes the edited exception go to the end of the list. This is already pretty
> ugly in the current native UI, but would be even worse with inline editing.

I looked at this some and I don't think it's feasible to fix this without major
surgery on contentsettingsmap. It will just have to be counter-intuitive (at
least for now). So anyway, I hooked up the editing.

[arv (Not doing code reviews), 2010-07-30 18:28:49]

http://codereview.chromium.org/2883045/diff/5001/6003
File chrome/browser/resources/options/content_settings_exceptions_area.js
(right):

http://codereview.chromium.org/2883045/diff/5001/6003#newcode50
chrome/browser/resources/options/content_settings_exceptions_area.js:50:
input.classList.add('hidden');
input.className = 'exceptionInput hidden';

http://codereview.chromium.org/2883045/diff/5001/6003#newcode50
chrome/browser/resources/options/content_settings_exceptions_area.js:50:
input.classList.add('hidden');
For the bookmark manager I had to minimize the number of DOM nodes since
creating hidden ones slowed down the drawing noticable.

http://codereview.chromium.org/2883045/diff/5001/6003#newcode75
chrome/browser/resources/options/content_settings_exceptions_area.js:75: * @type
{Object}
Looks like {string} to me

http://codereview.chromium.org/2883045/diff/5001/6003#newcode105
chrome/browser/resources/options/content_settings_exceptions_area.js:105:
this.optionAllow.selected = 'selected';
The DOM property for this is a boolean

this.optionAllow.selected = true;

http://codereview.chromium.org/2883045/diff/5001/6003#newcode117
chrome/browser/resources/options/content_settings_exceptions_area.js:117: set
editing(editing) {
On 2010/07/29 00:17:28, Evan Stade wrote:
> largely copied from bookmark_list.js

This should really be refactored so we can share code.

http://codereview.chromium.org/2883045/diff/5001/6003#newcode132
chrome/browser/resources/options/content_settings_exceptions_area.js:132:
patternLabel.classList.toggle('hidden');
See earlier comment about minimizing the DOM.

http://codereview.chromium.org/2883045/diff/5001/6003#newcode161
chrome/browser/resources/options/content_settings_exceptions_area.js:161: if
(activeElement != input && activeElement != select) {
You can use listItem.contains(activeElement) here

http://codereview.chromium.org/2883045/diff/5001/6003#newcode178
chrome/browser/resources/options/content_settings_exceptions_area.js:178:
input.addEventListener(type, stopPropagation);
For the bookmark manager the input is removed after editing is done. Here it
remains and if we edit the same item multiple times this will add too many
listeners

http://codereview.chromium.org/2883045/diff/5001/6003#newcode193
chrome/browser/resources/options/content_settings_exceptions_area.js:193: //
TODO(estade): check for pattern validity.
FYI: Inputs support a patter property.

[Evan Stade, 2010-08-03 17:26:44]

http://codereview.chromium.org/2883045/diff/5001/6003
File chrome/browser/resources/options/content_settings_exceptions_area.js
(right):

http://codereview.chromium.org/2883045/diff/5001/6003#newcode50
chrome/browser/resources/options/content_settings_exceptions_area.js:50:
input.classList.add('hidden');
On 2010/07/30 18:28:50, arv wrote:
> For the bookmark manager I had to minimize the number of DOM nodes since
> creating hidden ones slowed down the drawing noticable.

Hmm. That surprises me. But I doubt this will suffer from the same fate. The
bookmark manager takes up the whole browser window, whereas this table will only
be a few rows high at most (10? it's up to Cole I guess).

Testing with 7 rows, drawing only takes 0-2 ms (as reported by the redraw
benchmark). I did notice that redraw gets called more often than it seems it
should, but I don't think that's related to the number of DOM nodes.

http://codereview.chromium.org/2883045/diff/5001/6003#newcode75
chrome/browser/resources/options/content_settings_exceptions_area.js:75: * @type
{Object}
On 2010/07/30 18:28:50, arv wrote:
> Looks like {string} to me

yes. I made a couple of changes (including fixing this) after I initially sent
out the review. Sorry for the noise.

http://codereview.chromium.org/2883045/diff/5001/6003#newcode105
chrome/browser/resources/options/content_settings_exceptions_area.js:105:
this.optionAllow.selected = 'selected';
On 2010/07/30 18:28:50, arv wrote:
> The DOM property for this is a boolean
> 
> this.optionAllow.selected = true;
> 
> 

Done.

http://codereview.chromium.org/2883045/diff/5001/6003#newcode117
chrome/browser/resources/options/content_settings_exceptions_area.js:117: set
editing(editing) {
On 2010/07/30 18:28:50, arv wrote:
> On 2010/07/29 00:17:28, Evan Stade wrote:
> > largely copied from bookmark_list.js
> 
> This should really be refactored so we can share code.

Well, to be more specific, the control flow and concept is copied, but if you
diffed the two functions against each other you wouldn't get many lines that are
exactly the same, so I don't know if refactoring would buy us much.

http://codereview.chromium.org/2883045/diff/5001/6003#newcode161
chrome/browser/resources/options/content_settings_exceptions_area.js:161: if
(activeElement != input && activeElement != select) {
On 2010/07/30 18:28:50, arv wrote:
> You can use listItem.contains(activeElement) here

Done.

http://codereview.chromium.org/2883045/diff/5001/6003#newcode178
chrome/browser/resources/options/content_settings_exceptions_area.js:178:
input.addEventListener(type, stopPropagation);
On 2010/07/30 18:28:50, arv wrote:
> For the bookmark manager the input is removed after editing is done. Here it
> remains and if we edit the same item multiple times this will add too many
> listeners

fixed

[arv (Not doing code reviews), 2010-08-03 17:52:24]

LGTM

http://codereview.chromium.org/2883045/diff/19001/15005
File chrome/browser/resources/options/content_settings_exceptions_area.js
(right):

http://codereview.chromium.org/2883045/diff/19001/15005#newcode119
chrome/browser/resources/options/content_settings_exceptions_area.js:119: *
@type {String}
Use {string}. String is the object wrapper for string values

[Evan Stade, 2010-08-03 18:01:08]

thanks again for the handholding

http://codereview.chromium.org/2883045/diff/19001/15005
File chrome/browser/resources/options/content_settings_exceptions_area.js
(right):

http://codereview.chromium.org/2883045/diff/19001/15005#newcode119
chrome/browser/resources/options/content_settings_exceptions_area.js:119: *
@type {String}
On 2010/08/03 17:52:24, arv wrote:
> Use {string}. String is the object wrapper for string values

Done.