MD Settings: Allow deleting read-only content setting exceptions.

MD Settings: Allow deleting read-only content setting exceptions.

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

Review-Url: https://codereview.chromium.org/2769453002
Cr-Commit-Position: refs/heads/master@{#459196}
Committed: https://chromium.googlesource.com/chromium/src/+/23db0471ac32d68bb40a9cb53e14a55a371b7f4e

[tommycli, 2017-03-21 20:32:16]

Description was changed from

==========
MD Settings: Allow deleting read-only content setting exceptions.

BUG=702857
==========

to

==========
MD Settings: Allow deleting read-only content setting exceptions.

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

[tommycli, 2017-03-21 20:32:20]

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

[tommycli, 2017-03-21 20:33:02]

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

[tommycli, 2017-03-21 20:33:03]

dschuyler: PTAL, thanks

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

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-03-21 21:42:11]

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

[commit-bot: I haz the power, 2017-03-21 21:42:13]

Dry run: This issue passed the CQ dry run.

[tommycli, 2017-03-22 17:44:59]

On 2017/03/21 20:33:03, tommycli wrote:
> dschuyler: PTAL, thanks

dschuyler: ping, thanks!

[dschuyler, 2017-03-22 18:33:08]

https://codereview.chromium.org/2769453002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/site_settings/site_list.html (right):

https://codereview.chromium.org/2769453002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/site_list.html:85:
hidden="[[!readOnlyList]]" on-tap="onResetButtonTap_"
What will happen if this is policy enforced?
I looks like we might show the delete icon in that case and it seems like we
should not.

https://codereview.chromium.org/2769453002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/site_list.html:88:
<paper-icon-button id="actionMenuButton" icon="cr:more-vert"
FYI (doesn't necessarily require a change)

I'm not saying that "dots" is a great id name, but let me point out this style
guide entry:
https://google.github.io/styleguide/htmlcssguide.html#ID_and_Class_Name_Style
(I believe the issue here is size, since the label is not compiled away like it
would be in C++ (which calls for more verbose names)).

https://codereview.chromium.org/2769453002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/site_settings/site_list.js (right):

https://codereview.chromium.org/2769453002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/site_list.js:500:
this.onResetTap_();
It looks like only
this.onActionMenuActivate_(settings.PermissionValues.DEFAULT);
is necessary?
I'm not clear on why we'd call this.closeActionMenu_(). I suppose it's probably
harmless and would be fine if it were really hard to avoid that code path, but
it looks easy to avoid that path (though maybe I'm missing something?).

[tommycli, 2017-03-22 21:05:37]

thanks! I also used git cl format --js, so there could be some whitespace
changes too.

https://codereview.chromium.org/2769453002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/site_settings/site_list.html (right):

https://codereview.chromium.org/2769453002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/site_list.html:85:
hidden="[[!readOnlyList]]" on-tap="onResetButtonTap_"
On 2017/03/22 18:33:08, dschuyler wrote:
> What will happen if this is policy enforced?
> I looks like we might show the delete icon in that case and it seems like we
> should not.

Done.

https://codereview.chromium.org/2769453002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/site_list.html:88:
<paper-icon-button id="actionMenuButton" icon="cr:more-vert"
On 2017/03/22 18:33:08, dschuyler wrote:
> FYI (doesn't necessarily require a change)
> 
> I'm not saying that "dots" is a great id name, but let me point out this style
> guide entry:
> https://google.github.io/styleguide/htmlcssguide.html#ID_and_Class_Name_Style
> (I believe the issue here is size, since the label is not compiled away like
it
> would be in C++ (which calls for more verbose names)).

Acknowledged.

https://codereview.chromium.org/2769453002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/site_settings/site_list.js (right):

https://codereview.chromium.org/2769453002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/site_list.js:500:
this.onResetTap_();
On 2017/03/22 18:33:08, dschuyler wrote:
> It looks like only
> this.onActionMenuActivate_(settings.PermissionValues.DEFAULT);
> is necessary?
> I'm not clear on why we'd call this.closeActionMenu_(). I suppose it's
probably
> harmless and would be fine if it were really hard to avoid that code path, but
> it looks easy to avoid that path (though maybe I'm missing something?).

Done.

[tommycli, 2017-03-22 21:05:46]

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

[commit-bot: I haz the power, 2017-03-22 21:06:37]

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

[dschuyler, 2017-03-22 21:19:20]

https://codereview.chromium.org/2769453002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/site_list.js (right):

https://codereview.chromium.org/2769453002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_list.js:512:
this.actionMenuSite_ = null;
Hmm, it looks like onActionMenuActivate_ is an event handler because the name
starts with "on", but it appears to only be used as a private function (and not
as an event handler -- maybe the name should be changed).

Your choice:

Could you do me the favor of adding a 
// TODO(dschuyler): Change onActionMenuActivate_() to take a actionMenuSite_ so
that it doesn't need to be set/reset like this.

or

Go ahead and change onActionMenuActivate_() to take a actionMenuSite_ so that it
doesn't need to be set/reset like this.

[dschuyler, 2017-03-22 21:22:31]

lgtm

[tommycli, 2017-03-22 21:45:24]

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

[commit-bot: I haz the power, 2017-03-22 21:46:10]

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

[tommycli, 2017-03-22 21:51:29]

thanks!

https://codereview.chromium.org/2769453002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/site_list.js (right):

https://codereview.chromium.org/2769453002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_list.js:512:
this.actionMenuSite_ = null;
On 2017/03/22 21:19:19, dschuyler wrote:
> Hmm, it looks like onActionMenuActivate_ is an event handler because the name
> starts with "on", but it appears to only be used as a private function (and
not
> as an event handler -- maybe the name should be changed).
> 
> Your choice:
> 
> Could you do me the favor of adding a 
> // TODO(dschuyler): Change onActionMenuActivate_() to take a actionMenuSite_
so
> that it doesn't need to be set/reset like this.
> 
> or
> 
> Go ahead and change onActionMenuActivate_() to take a actionMenuSite_ so that
it
> doesn't need to be set/reset like this.

Done. Based on your feedback, I refactored it a bit to make it more obvious.
Still room for more improvement, but this is a decent state I think.

[tommycli, 2017-03-22 22:19:57]

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

[commit-bot: I haz the power, 2017-03-22 22:20:33]

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-03-22 22:54:57]

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

[commit-bot: I haz the power, 2017-03-22 22:54:58]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[dschuyler, 2017-03-22 23:06:40]

On 2017/03/22 21:51:29, tommycli wrote:
> thanks!
> 
>
https://codereview.chromium.org/2769453002/diff/20001/chrome/browser/resource...
> File chrome/browser/resources/settings/site_settings/site_list.js (right):
> 
>
https://codereview.chromium.org/2769453002/diff/20001/chrome/browser/resource...
> chrome/browser/resources/settings/site_settings/site_list.js:512:
> this.actionMenuSite_ = null;
> On 2017/03/22 21:19:19, dschuyler wrote:
> > Hmm, it looks like onActionMenuActivate_ is an event handler because the
name
> > starts with "on", but it appears to only be used as a private function (and
> not
> > as an event handler -- maybe the name should be changed).
> > 
> > Your choice:
> > 
> > Could you do me the favor of adding a 
> > // TODO(dschuyler): Change onActionMenuActivate_() to take a actionMenuSite_
> so
> > that it doesn't need to be set/reset like this.
> > 
> > or
> > 
> > Go ahead and change onActionMenuActivate_() to take a actionMenuSite_ so
that
> it
> > doesn't need to be set/reset like this.
> 
> Done. Based on your feedback, I refactored it a bit to make it more obvious.
> Still room for more improvement, but this is a decent state I think.

Thanks! slgtm

[tommycli, 2017-03-22 23:07:07]

On 2017/03/22 23:06:40, dschuyler wrote:
> On 2017/03/22 21:51:29, tommycli wrote:
> > thanks!
> > 
> >
>
https://codereview.chromium.org/2769453002/diff/20001/chrome/browser/resource...
> > File chrome/browser/resources/settings/site_settings/site_list.js (right):
> > 
> >
>
https://codereview.chromium.org/2769453002/diff/20001/chrome/browser/resource...
> > chrome/browser/resources/settings/site_settings/site_list.js:512:
> > this.actionMenuSite_ = null;
> > On 2017/03/22 21:19:19, dschuyler wrote:
> > > Hmm, it looks like onActionMenuActivate_ is an event handler because the
> name
> > > starts with "on", but it appears to only be used as a private function
(and
> > not
> > > as an event handler -- maybe the name should be changed).
> > > 
> > > Your choice:
> > > 
> > > Could you do me the favor of adding a 
> > > // TODO(dschuyler): Change onActionMenuActivate_() to take a
actionMenuSite_
> > so
> > > that it doesn't need to be set/reset like this.
> > > 
> > > or
> > > 
> > > Go ahead and change onActionMenuActivate_() to take a actionMenuSite_ so
> that
> > it
> > > doesn't need to be set/reset like this.
> > 
> > Done. Based on your feedback, I refactored it a bit to make it more obvious.
> > Still room for more improvement, but this is a decent state I think.
> 
> Thanks! slgtm

Great, sending it in!

[tommycli, 2017-03-22 23:07:15]

The CQ bit was checked by tommycli@chromium.org

[tommycli, 2017-03-22 23:07:15]

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/2769453002/#ps60001
(title: "refactor some more")

[commit-bot: I haz the power, 2017-03-22 23:08:28]

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-03-22 23:27:01]

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

[commit-bot: I haz the power, 2017-03-22 23:27:03]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tommycli, 2017-03-23 18:46:13]

The CQ bit was checked by tommycli@chromium.org

[tommycli, 2017-03-23 18:46:13]

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/2769453002/#ps80001
(title: "Merge")

[commit-bot: I haz the power, 2017-03-23 18:47:01]

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-03-23 19:11:33]

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

[commit-bot: I haz the power, 2017-03-23 19:11:35]

Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[tommycli, 2017-03-23 19:12:58]

The CQ bit was checked by tommycli@chromium.org

[commit-bot: I haz the power, 2017-03-23 19:13:25]

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-03-23 20:16:42]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1490296378525060,
"parent_rev": "37a094089091a096d1a539867240a6b491d9f59f", "commit_rev":
"23db0471ac32d68bb40a9cb53e14a55a371b7f4e"}

[commit-bot: I haz the power, 2017-03-23 20:17:25]

Description was changed from

==========
MD Settings: Allow deleting read-only content setting exceptions.

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

to

==========
MD Settings: Allow deleting read-only content setting exceptions.

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

Review-Url: https://codereview.chromium.org/2769453002
Cr-Commit-Position: refs/heads/master@{#459196}
Committed:
https://chromium.googlesource.com/chromium/src/+/23db0471ac32d68bb40a9cb53e14...
==========

[commit-bot: I haz the power, 2017-03-23 20:17:26]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/23db0471ac32d68bb40a9cb53e14...