[MD settings] clear incognito only checkbox

[MD settings] clear incognito only checkbox

This CL clears the 'incognito only' checkbox in the add site dialog when
the incognito status changes. A unit test has been added to test this
behavior.

BUG=715433
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2854753002
Cr-Commit-Position: refs/heads/master@{#468732}
Committed: https://chromium.googlesource.com/chromium/src/+/f83540958fb0ffff1080a759eb3e7b2a06e32902

[dschuyler, 2017-05-01 23:45:05]

Description was changed from

==========
[MD settings] clear incognito only checkbox

This CL clears the 'incognito only' checkbox in the add site dialog when
the incognito status changes. A unit test has been added to test this
behavior.

BUG=715433
==========

to

==========
[MD settings] clear incognito only checkbox

This CL clears the 'incognito only' checkbox in the add site dialog when
the incognito status changes. A unit test has been added to test this
behavior.

BUG=715433
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[dschuyler, 2017-05-01 23:45:08]

The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-01 23:45:42]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dschuyler, 2017-05-01 23:46:28]

dschuyler@chromium.org changed reviewers:
+ dpapad@chromium.org

[dpapad, 2017-05-02 00:36:35]

LGTM with nits.

https://codereview.chromium.org/2854753002/diff/1/chrome/test/data/webui/sett...
File chrome/test/data/webui/settings/site_list_tests.js (right):

https://codereview.chromium.org/2854753002/diff/1/chrome/test/data/webui/sett...
chrome/test/data/webui/settings/site_list_tests.js:1164: dialog.open();
add-site-dialog#open method expects a |type| parameter [1]. Upon further
inspection, the |type| parameter is not used anywhere in that function (guessing
obsolete). Can we update the signature of that method?

https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/site_s...

https://codereview.chromium.org/2854753002/diff/1/chrome/test/data/webui/sett...
chrome/test/data/webui/settings/site_list_tests.js:1174: /*hasIncognito=*/true);
Can we remove the "=" part of the comment? "=" is for JSCompiler to indicate an
optional param, whereas an inline comment like this is to inform the reader
about the meaning of the literal value.

Also Assuming this is clang-formatted too? I was expecting more spaces, like 
/* hasIncognito */ true

https://codereview.chromium.org/2854753002/diff/1/chrome/test/data/webui/sett...
File chrome/test/data/webui/settings/test_site_settings_prefs_browser_proxy.js
(left):

https://codereview.chromium.org/2854753002/diff/1/chrome/test/data/webui/sett...
chrome/test/data/webui/settings/test_site_settings_prefs_browser_proxy.js:96:
'setProtocolDefault'
Nit (optional): Can you try adding |updateIncognitoStatus| at the end of this
list, but include a trailing comma, and re-run clang-format? I suspect that this
will cause clang-format to keep the 1-per-line formatting (which I think is more
readable).

[dschuyler, 2017-05-02 01:04:42]

The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-02 01:05:17]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dschuyler, 2017-05-02 01:05:22]

https://codereview.chromium.org/2854753002/diff/1/chrome/test/data/webui/sett...
File chrome/test/data/webui/settings/site_list_tests.js (right):

https://codereview.chromium.org/2854753002/diff/1/chrome/test/data/webui/sett...
chrome/test/data/webui/settings/site_list_tests.js:1164: dialog.open();
On 2017/05/02 00:36:34, dpapad wrote:
> add-site-dialog#open method expects a |type| parameter [1]. Upon further
> inspection, the |type| parameter is not used anywhere in that function
(guessing
> obsolete). Can we update the signature of that method?
> 
>
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/site_s...

Done.

https://codereview.chromium.org/2854753002/diff/1/chrome/test/data/webui/sett...
chrome/test/data/webui/settings/site_list_tests.js:1174: /*hasIncognito=*/true);
On 2017/05/02 00:36:34, dpapad wrote:
> Can we remove the "=" part of the comment? "=" is for JSCompiler to indicate
an
> optional param, whereas an inline comment like this is to inform the reader
> about the meaning of the literal value.
> 
> Also Assuming this is clang-formatted too? I was expecting more spaces, like 
> /* hasIncognito */ true
> 

It's part of the C++ style guide (and I find it really helpful).
Since it's not a closure comment, there's no ambiguity there.

https://codereview.chromium.org/2854753002/diff/1/chrome/test/data/webui/sett...
File chrome/test/data/webui/settings/test_site_settings_prefs_browser_proxy.js
(left):

https://codereview.chromium.org/2854753002/diff/1/chrome/test/data/webui/sett...
chrome/test/data/webui/settings/test_site_settings_prefs_browser_proxy.js:96:
'setProtocolDefault'
On 2017/05/02 00:36:35, dpapad wrote:
> Nit (optional): Can you try adding |updateIncognitoStatus| at the end of this
> list, but include a trailing comma, and re-run clang-format? I suspect that
this
> will cause clang-format to keep the 1-per-line formatting (which I think is
more
> readable).

Yep! that worked thanks.

Done.

[commit-bot: I haz the power, 2017-05-02 02:28:06]

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

[commit-bot: I haz the power, 2017-05-02 02:28:07]

Dry run: This issue passed the CQ dry run.

[dschuyler, 2017-05-02 18:58:15]

The CQ bit was checked by dschuyler@chromium.org

[dschuyler, 2017-05-02 18:58:15]

The patchset sent to the CQ was uploaded after l-g-t-m from dpapad@chromium.org
Link to the patchset: https://codereview.chromium.org/2854753002/#ps20001
(title: "review changes")

[commit-bot: I haz the power, 2017-05-02 18:58:39]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-02 19:05:57]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1493751495226630,
"parent_rev": "c87903ecca1345c4363da94e0120adedd83963b0", "commit_rev":
"f83540958fb0ffff1080a759eb3e7b2a06e32902"}

[commit-bot: I haz the power, 2017-05-02 19:06:12]

Description was changed from

==========
[MD settings] clear incognito only checkbox

This CL clears the 'incognito only' checkbox in the add site dialog when
the incognito status changes. A unit test has been added to test this
behavior.

BUG=715433
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
[MD settings] clear incognito only checkbox

This CL clears the 'incognito only' checkbox in the add site dialog when
the incognito status changes. A unit test has been added to test this
behavior.

BUG=715433
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2854753002
Cr-Commit-Position: refs/heads/master@{#468732}
Committed:
https://chromium.googlesource.com/chromium/src/+/f83540958fb0ffff1080a759eb3e...
==========

[commit-bot: I haz the power, 2017-05-02 19:06:14]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/f83540958fb0ffff1080a759eb3e...