options: fix content settings exceptions dialog regression.

options: fix content settings exceptions dialog regression.

Stupid natural numbers.

BUG=367636
R=estade@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=267975

[Dan Beam, 2014-05-01 23:43:33]

https://codereview.chromium.org/264713008/diff/1/chrome/browser/ui/webui/opti...
File
chrome/browser/ui/webui/options/content_settings_exception_area_browsertest.js
(right):

https://codereview.chromium.org/264713008/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/content_settings_exception_area_browsertest.js:85:
});
^ the test that took me 2 tries and 12h to write :(

https://codereview.chromium.org/264713008/diff/1/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/content_settings_handler.cc (right):

https://codereview.chromium.org/264713008/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/content_settings_handler.cc:1214: if
(args->GetSize() >= 4U) {
^ the fix that took me 3s to write

[Lei Zhang, 2014-05-01 23:54:29]

I shouldn't be reviewing JS.

https://codereview.chromium.org/264713008/diff/1/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/content_settings_handler.cc (right):

https://codereview.chromium.org/264713008/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/content_settings_handler.cc:1214: if
(args->GetSize() >= 4U) {
On 2014/05/01 23:43:33, Dan Beam wrote:
> ^ the fix that took me 3s to write

Curious, what's args->GetString(0, &foo) ?

[Dan Beam, 2014-05-02 00:43:14]

> I shouldn't be reviewing JS.

thestig@ -> CC, estade -> R= then

https://codereview.chromium.org/264713008/diff/1/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/content_settings_handler.cc (right):

https://codereview.chromium.org/264713008/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/content_settings_handler.cc:1214: if
(args->GetSize() >= 4U) {
On 2014/05/01 23:54:30, Lei Zhang wrote:
> On 2014/05/01 23:43:33, Dan Beam wrote:
> > ^ the fix that took me 3s to write
> 
> Curious, what's args->GetString(0, &foo) ?

see below

https://codereview.chromium.org/264713008/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/content_settings_handler.cc:1374:
CHECK(args->GetString(0, &type_string));
^ the type of exception to remove (e.g. "cookies", "location")

[Evan Stade, 2014-05-02 01:18:32]

lgtm

lgtm with nits

https://codereview.chromium.org/264713008/diff/1/chrome/browser/resources/opt...
File chrome/browser/resources/options/content_settings.js (right):

https://codereview.chromium.org/264713008/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/content_settings.js:171:
ContentSettings.setExceptions = function(type, list) {
s/list/exceptions please

https://codereview.chromium.org/264713008/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/content_settings.js:183:
ContentSettings.setOTRExceptions = function(type, list) {
ditto

https://codereview.chromium.org/264713008/diff/1/chrome/browser/ui/webui/opti...
File
chrome/browser/ui/webui/options/content_settings_exception_area_browsertest.js
(right):

https://codereview.chromium.org/264713008/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/content_settings_exception_area_browsertest.js:64:
setExceptionsCallback = function() {
most confusing pattern ever. I think it would be easier to read as:

var setExceptionsCounter = 0;
var setExceptionsWasCalled = function() {
  setExceptionsCounter++;
  if (setExceptionsCounter == 1) {
    do something;
  } else if (setExceptionsCounter == 2) {
    do something else;
  }
}

even if it comes out slightly longer

https://codereview.chromium.org/264713008/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/content_settings_exception_area_browsertest.js:66:
ContentSettings.setExceptions = origSetExceptions;
really surprising to me this works even though origSetExceptions is defined
below it

https://codereview.chromium.org/264713008/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/content_settings_exception_area_browsertest.js:83:
//chrome.send('setException', ['cookies', 'normal', origin, origin]);
why is this line here?

[Dan Beam, 2014-05-02 18:28:04]

https://codereview.chromium.org/264713008/diff/1/chrome/browser/resources/opt...
File chrome/browser/resources/options/content_settings.js (right):

https://codereview.chromium.org/264713008/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/content_settings.js:171:
ContentSettings.setExceptions = function(type, list) {
On 2014/05/02 01:18:33, Evan Stade wrote:
> s/list/exceptions please

Done.

https://codereview.chromium.org/264713008/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/content_settings.js:183:
ContentSettings.setOTRExceptions = function(type, list) {
On 2014/05/02 01:18:33, Evan Stade wrote:
> ditto

Done.

https://codereview.chromium.org/264713008/diff/1/chrome/browser/ui/webui/opti...
File
chrome/browser/ui/webui/options/content_settings_exception_area_browsertest.js
(right):

https://codereview.chromium.org/264713008/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/content_settings_exception_area_browsertest.js:64:
setExceptionsCallback = function() {
On 2014/05/02 01:18:33, Evan Stade wrote:
> most confusing pattern ever. I think it would be easier to read as:
> 
> var setExceptionsCounter = 0;
> var setExceptionsWasCalled = function() {
>   setExceptionsCounter++;
>   if (setExceptionsCounter == 1) {
>     do something;
>   } else if (setExceptionsCounter == 2) {
>     do something else;
>   }
> }
> 
> even if it comes out slightly longer

Done.

https://codereview.chromium.org/264713008/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/content_settings_exception_area_browsertest.js:66:
ContentSettings.setExceptions = origSetExceptions;
On 2014/05/02 01:18:33, Evan Stade wrote:
> really surprising to me this works even though origSetExceptions is defined
> below it

it's defined when this is executed, which is all that matters.  also, all vars
are "hoisted" to the "top" of a function scope either way.  

moved this declaration higher anyway.

https://codereview.chromium.org/264713008/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/content_settings_exception_area_browsertest.js:83:
//chrome.send('setException', ['cookies', 'normal', origin, origin]);
On 2014/05/02 01:18:33, Evan Stade wrote:
> why is this line here?

Done.

[Dan Beam, 2014-05-02 18:28:56]

The CQ bit was checked by dbeam@chromium.org

[commit-bot: I haz the power, 2014-05-02 18:37:52]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dbeam@chromium.org/264713008/20001

[commit-bot: I haz the power, 2014-05-02 19:20:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-02 19:20:11]

Try jobs failed on following builders:
  win_chromium_x64_rel on tryserver.chromium

[Dan Beam, 2014-05-02 22:00:03]

The CQ bit was checked by dbeam@chromium.org

[commit-bot: I haz the power, 2014-05-02 22:00:41]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dbeam@chromium.org/264713008/20001

[commit-bot: I haz the power, 2014-05-02 23:45:39]

Change committed as 267975