Site Settings Desktop: Flesh out controlled exceptions a little more.

Site Settings Desktop: Flesh out controlled exceptions a little more.

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

Committed: https://crrev.com/b0fca5c853d275637871e0ac86f84953f2071346
Cr-Commit-Position: refs/heads/master@{#420302}

[Finnur, 2016-09-21 16:15:46]

Description was changed from

==========
Site Settings Desktop: Flesh out controlled exceptions a little more.

BUG=TBD
==========

to

==========
Site Settings Desktop: Flesh out controlled exceptions a little more.

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

[Finnur, 2016-09-21 16:16:17]

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

[commit-bot: I haz the power, 2016-09-21 16:16:51]

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

[Finnur, 2016-09-21 16:17:04]

Description was changed from

==========
Site Settings Desktop: Flesh out controlled exceptions a little more.

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

to

==========
Site Settings Desktop: Flesh out controlled exceptions a little more.

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

[Finnur, 2016-09-21 16:19:36]

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

[commit-bot: I haz the power, 2016-09-21 16:20:12]

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

[Finnur, 2016-09-21 16:45:23]

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

[Finnur, 2016-09-21 16:45:24]

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

https://codereview.chromium.org/2354313002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_list.js:407: return;
I took this out because it doesn't make sense to me to block the Site Details
page just because the exception one clicked on is controlled. If an admin blocks
plugins on youtube.com and the user has a popup exception, the user would expect
to get to the details page, see that she can't change the plugins exception but
is still able to control the popup exception.

[commit-bot: I haz the power, 2016-09-21 17:05:41]

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

[commit-bot: I haz the power, 2016-09-21 17:05:42]

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

[dschuyler, 2016-09-21 17:25:20]

lgtm

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

https://codereview.chromium.org/2354313002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_list.js:407: return;
On 2016/09/21 16:45:24, Finnur wrote:
> I took this out because it doesn't make sense to me to block the Site Details
> page just because the exception one clicked on is controlled. If an admin
blocks
> plugins on http://youtube.com and the user has a popup exception, the user
would expect
> to get to the details page, see that she can't change the plugins exception
but
> is still able to control the popup exception.

Acknowledged.

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

https://codereview.chromium.org/2354313002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_list.js:204: return '';
optional: A handful of conditionals is not a big deal,
so this may be fine as is, but around this point I'd 
suggest switching to a dictionary (object) and doing
something like:

iconControlledBy: {
...
},
computeIconControlledBy_: function(item) {
  return iconControlledBy[item.source] || '';
},

[Finnur, 2016-09-22 10:32:06]

The CQ bit was checked by finnur@chromium.org

[Finnur, 2016-09-22 10:32:06]

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/2354313002/#ps40001
(title: "Address feedback")

[commit-bot: I haz the power, 2016-09-22 10:32:24]

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, 2016-09-22 11:29:36]

Description was changed from

==========
Site Settings Desktop: Flesh out controlled exceptions a little more.

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

to

==========
Site Settings Desktop: Flesh out controlled exceptions a little more.

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

[commit-bot: I haz the power, 2016-09-22 11:29:37]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-09-22 11:31:07]

Description was changed from

==========
Site Settings Desktop: Flesh out controlled exceptions a little more.

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

to

==========
Site Settings Desktop: Flesh out controlled exceptions a little more.

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

Committed: https://crrev.com/b0fca5c853d275637871e0ac86f84953f2071346
Cr-Commit-Position: refs/heads/master@{#420302}
==========

[commit-bot: I haz the power, 2016-09-22 11:31:08]

Patchset 3 (id:??) landed as
https://crrev.com/b0fca5c853d275637871e0ac86f84953f2071346
Cr-Commit-Position: refs/heads/master@{#420302}

[Finnur, 2016-09-22 12:58:00]

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

https://codereview.chromium.org/2354313002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_list.js:204: return '';
On 2016/09/21 17:25:20, dschuyler wrote:
> optional: A handful of conditionals is not a big deal,
> so this may be fine as is, but around this point I'd 
> suggest switching to a dictionary (object) and doing
> something like:
> 
> iconControlledBy: {
> ...
> },
> computeIconControlledBy_: function(item) {
>   return iconControlledBy[item.source] || '';
> },
> 

Done.