MD Settings: Allow editing a cookie site exception.

MD Settings: Allow editing a cookie site exception.

BUG=657639, 695578
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2699013002
Cr-Commit-Position: refs/heads/master@{#452712}
Committed: https://chromium.googlesource.com/chromium/src/+/188b7bdc8292a7b740f7caa3d26bfd89aac5a924

[dpapad, 2017-02-16 23:15:02]

Description was changed from

==========
MD Settings: Allow editing a cookie site exception.

BUG=657639
==========

to

==========
MD Settings: Allow editing a cookie site exception.

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

[dpapad, 2017-02-21 19:57:31]

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

[commit-bot: I haz the power, 2017-02-21 19:58:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dpapad, 2017-02-21 20:00:56]

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

[dpapad, 2017-02-21 20:01:08]

Patchset #5 (id:80001) has been deleted

[commit-bot: I haz the power, 2017-02-21 20:02:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dpapad, 2017-02-21 20:31:01]

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

[dpapad, 2017-02-21 20:31:02]

Screenshots at http://imgur.com/a/Zm6ZN.

[commit-bot: I haz the power, 2017-02-21 22:07:30]

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

[commit-bot: I haz the power, 2017-02-21 22:07:31]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)

[dpapad, 2017-02-21 22:46:08]

https://codereview.chromium.org/2699013002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/settings/site_settings/edit_exception_dialog.html
(right):

https://codereview.chromium.org/2699013002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/edit_exception_dialog.html:12:
<div class="title">Edit exception</div>
Oops, forgot to localize this string. Localizing in follow up patch (same CL).

[dpapad, 2017-02-21 23:17:29]

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

[dpapad, 2017-02-21 23:18:20]

https://codereview.chromium.org/2699013002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/settings/site_settings/edit_exception_dialog.html
(right):

https://codereview.chromium.org/2699013002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/edit_exception_dialog.html:12:
<div class="title">Edit exception</div>
On 2017/02/21 at 22:46:08, dpapad wrote:
> Oops, forgot to localize this string. Localizing in follow up patch (same CL).

Done.

[commit-bot: I haz the power, 2017-02-21 23:19:14]

Dry run: 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-02-22 01:24:44]

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

[commit-bot: I haz the power, 2017-02-22 01:24:45]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  closure_compilation on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)

[dpapad, 2017-02-22 18:34:17]

On 2017/02/21 at 23:18:20, dpapad wrote:
>
https://codereview.chromium.org/2699013002/diff/120001/chrome/browser/resourc...
> File
chrome/browser/resources/settings/site_settings/edit_exception_dialog.html
(right):
> 
>
https://codereview.chromium.org/2699013002/diff/120001/chrome/browser/resourc...
> chrome/browser/resources/settings/site_settings/edit_exception_dialog.html:12:
<div class="title">Edit exception</div>
> On 2017/02/21 at 22:46:08, dpapad wrote:
> > Oops, forgot to localize this string. Localizing in follow up patch (same
CL).
> 
> Done.

Friendly ping.

[dschuyler, 2017-02-23 01:31:49]

https://codereview.chromium.org/2699013002/diff/140001/chrome/app/settings_st...
File chrome/app/settings_strings.grdp (right):

https://codereview.chromium.org/2699013002/diff/140001/chrome/app/settings_st...
chrome/app/settings_strings.grdp:2021: <message
name="IDS_SETTINGS_EDIT_SITE_DIALOG_TITLE" desc="Title for the Edit Site
dialog">
The labels here are verbose and similar, consider dropping "DIALOG_" from line
2018 and 2021.

https://codereview.chromium.org/2699013002/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/settings/site_settings/edit_exception_dialog.html
(right):

https://codereview.chromium.org/2699013002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/edit_exception_dialog.html:2:
<link rel="import" href="chrome://resources/html/polymer.html">
Let's import polymer.html first; I hear it's the right thing to do.

https://codereview.chromium.org/2699013002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/edit_exception_dialog.html:23:
disabled="[[!origin_.length]]"
Please validate the url with something like this.browserProxy.isPatternValid to
control whether the action button is disabled.

https://codereview.chromium.org/2699013002/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/settings/site_settings/edit_exception_dialog.js
(right):

https://codereview.chromium.org/2699013002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/edit_exception_dialog.js:43: var
category = settings.ContentSettingsTypes.COOKIES;
I see that the bug is specific to Cookies, but this edit feature was present in
other sections (other than cookies) in the old options. Can we make this match
that? Or at a minimum, add a TODO here and a crbug saying that this feature
needs to be expanded to the other content settings.

[dpapad, 2017-02-23 19:28:15]

Description was changed from

==========
MD Settings: Allow editing a cookie site exception.

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

to

==========
MD Settings: Allow editing a cookie site exception.

BUG=657639,695578
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[dpapad, 2017-02-23 19:39:17]

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

[dpapad, 2017-02-23 19:39:47]

https://codereview.chromium.org/2699013002/diff/140001/chrome/app/settings_st...
File chrome/app/settings_strings.grdp (right):

https://codereview.chromium.org/2699013002/diff/140001/chrome/app/settings_st...
chrome/app/settings_strings.grdp:2021: <message
name="IDS_SETTINGS_EDIT_SITE_DIALOG_TITLE" desc="Title for the Edit Site
dialog">
On 2017/02/23 at 01:31:49, dschuyler wrote:
> The labels here are verbose and similar, consider dropping "DIALOG_" from line
2018 and 2021.

Done. I originally was trying to be consistent with other names in this file,
see
https://cs.chromium.org/search/?q=DIALOG_TITLE+file:settings_strings.grdp&typ....

https://codereview.chromium.org/2699013002/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/settings/site_settings/edit_exception_dialog.html
(right):

https://codereview.chromium.org/2699013002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/edit_exception_dialog.html:2:
<link rel="import" href="chrome://resources/html/polymer.html">
On 2017/02/23 at 01:31:49, dschuyler wrote:
> Let's import polymer.html first; I hear it's the right thing to do.

Correct. Done.

https://codereview.chromium.org/2699013002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/edit_exception_dialog.html:23:
disabled="[[!origin_.length]]"
On 2017/02/23 at 01:31:49, dschuyler wrote:
> Please validate the url with something like this.browserProxy.isPatternValid
to control whether the action button is disabled.

Thanks. Done (and updated tests).

https://codereview.chromium.org/2699013002/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/settings/site_settings/edit_exception_dialog.js
(right):

https://codereview.chromium.org/2699013002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/edit_exception_dialog.js:43: var
category = settings.ContentSettingsTypes.COOKIES;
On 2017/02/23 at 01:31:49, dschuyler wrote:
> I see that the bug is specific to Cookies, but this edit feature was present
in other sections (other than cookies) in the old options. Can we make this
match that? Or at a minimum, add a TODO here and a crbug saying that this
feature needs to be expanded to the other content settings.

Done, https://bugs.chromium.org/p/chromium/issues/detail?id=695578.

https://codereview.chromium.org/2699013002/diff/160001/chrome/browser/resourc...
File chrome/browser/resources/settings/site_settings/add_site_dialog.html
(left):

https://codereview.chromium.org/2699013002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/add_site_dialog.html:26:
<paper-input type="text" id="site" autofocus
Made minor changes here, since the code seemed obsolete (using a separate span
instead of paper-input's  floating label, using type="text" which is not
applicable). I can move those to a separate CL if you prefer.

[commit-bot: I haz the power, 2017-02-23 19:39:55]

Dry run: 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-02-23 20:52:13]

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

[commit-bot: I haz the power, 2017-02-23 20:52:15]

Dry run: This issue passed the CQ dry run.

[dschuyler, 2017-02-23 22:24:51]

lgtm

https://codereview.chromium.org/2699013002/diff/160001/chrome/browser/resourc...
File chrome/browser/resources/settings/site_settings/add_site_dialog.html
(left):

https://codereview.chromium.org/2699013002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/add_site_dialog.html:26:
<paper-input type="text" id="site" autofocus
On 2017/02/23 19:39:47, dpapad wrote:
> Made minor changes here, since the code seemed obsolete (using a separate span
> instead of paper-input's  floating label, using type="text" which is not
> applicable). I can move those to a separate CL if you prefer.

Thanks for pointing this out. It really helps (so I'm not trying to figure out
how this fits with the other changes).

https://codereview.chromium.org/2699013002/diff/160001/chrome/browser/resourc...
File chrome/browser/resources/settings/site_settings/edit_exception_dialog.js
(right):

https://codereview.chromium.org/2699013002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/edit_exception_dialog.js:1: //
Copyright 2016 The Chromium Authors. All rights reserved.
nit: copyright year.

https://codereview.chromium.org/2699013002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/edit_exception_dialog.js:45: 
nit: Maybe a comment about why the reset* before the set*

[dpapad, 2017-02-24 00:15:28]

Patchset #9 (id:180001) has been deleted

[dpapad, 2017-02-24 00:17:19]

https://codereview.chromium.org/2699013002/diff/160001/chrome/browser/resourc...
File chrome/browser/resources/settings/site_settings/edit_exception_dialog.js
(right):

https://codereview.chromium.org/2699013002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/edit_exception_dialog.js:1: //
Copyright 2016 The Chromium Authors. All rights reserved.
On 2017/02/23 at 22:24:50, dschuyler wrote:
> nit: copyright year.

Done.

https://codereview.chromium.org/2699013002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/edit_exception_dialog.js:45: 
On 2017/02/23 at 22:24:50, dschuyler wrote:
> nit: Maybe a comment about why the reset* before the set*

Done.

[dpapad, 2017-02-24 00:19:26]

The CQ bit was checked by dpapad@chromium.org

[dpapad, 2017-02-24 00:19:26]

The patchset sent to the CQ was uploaded after l-g-t-m from
dschuyler@chromium.org
Link to the patchset: https://codereview.chromium.org/2699013002/#ps200001
(title: "Address nits.")

[commit-bot: I haz the power, 2017-02-24 00:20:10]

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-02-24 01:49:26]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1487895566572590,
"parent_rev": "45c6392a896fefd5bdaef09d80920a55e1ea2ac4", "commit_rev":
"188b7bdc8292a7b740f7caa3d26bfd89aac5a924"}

[commit-bot: I haz the power, 2017-02-24 01:50:25]

Description was changed from

==========
MD Settings: Allow editing a cookie site exception.

BUG=657639,695578
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: Allow editing a cookie site exception.

BUG=657639,695578
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2699013002
Cr-Commit-Position: refs/heads/master@{#452712}
Committed:
https://chromium.googlesource.com/chromium/src/+/188b7bdc8292a7b740f7caa3d26b...
==========

[commit-bot: I haz the power, 2017-02-24 01:50:27]

Committed patchset #9 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/188b7bdc8292a7b740f7caa3d26b...