Display content settings for extensions with extension name.

Display content settings and zoomlevel for extensions with extension name instead of showing a cryptic id.
If an extension has a favicon, it is shown as well.

https://screenshot.googleplex.com/7rMF5f26CmW
https://screenshot.googleplex.com/9TbvP1zDYWJ

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

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

[dullweber, 2016-11-23 15:12:50]

Description was changed from

==========
Display content settings for extensions with extension name.

Instead of showing a cryptic id for content settings for extensions,
display the extension name instead. If an extension has a favicon it is
shown as well.

BUG=412740
==========

to

==========
Display content settings for extensions with extension name.

Instead of showing a cryptic id for content settings for extensions,
display the extension name instead. If an extension has a favicon it is
shown as well.

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

[dullweber, 2016-11-23 15:13:06]

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

[commit-bot: I haz the power, 2016-11-23 15:13:22]

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, 2016-11-23 15:33:34]

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

[commit-bot: I haz the power, 2016-11-23 15:33:34]

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_...)

[dullweber, 2016-11-23 15:40:17]

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

[commit-bot: I haz the power, 2016-11-23 15:40: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, 2016-11-23 16:16:32]

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

[commit-bot: I haz the power, 2016-11-23 16:16:33]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[dullweber, 2016-11-23 16:28:07]

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

[commit-bot: I haz the power, 2016-11-23 16:28:27]

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, 2016-11-23 17:05:52]

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

[commit-bot: I haz the power, 2016-11-23 17:05:53]

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_...)

[dullweber, 2016-11-24 08:43:27]

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

[commit-bot: I haz the power, 2016-11-24 08:43:54]

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

[dullweber, 2016-11-24 08:50:50]

Description was changed from

==========
Display content settings for extensions with extension name.

Instead of showing a cryptic id for content settings for extensions,
display the extension name instead. If an extension has a favicon it is
shown as well.

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

to

==========
Display content settings for extensions with extension name.

Instead of showing a cryptic id for content settings for extensions,
display the extension name instead. If an extension has a favicon it is
shown as well.

https://screenshot.googleplex.com/7rMF5f26CmW

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

[dullweber, 2016-11-24 08:52:54]

Description was changed from

==========
Display content settings for extensions with extension name.

Instead of showing a cryptic id for content settings for extensions,
display the extension name instead. If an extension has a favicon it is
shown as well.

https://screenshot.googleplex.com/7rMF5f26CmW

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

to

==========
Display content settings for extensions with extension name.

Instead of showing a cryptic id for content settings for extensions,
display the extension name instead. If an extension has a favicon it is
shown as well.

I only applied this fix to the new MD settings. Should it be fixed for the old
settings as well?

https://screenshot.googleplex.com/7rMF5f26CmW

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

[dullweber, 2016-11-24 08:54:59]

Description was changed from

==========
Display content settings for extensions with extension name.

Instead of showing a cryptic id for content settings for extensions,
display the extension name instead. If an extension has a favicon it is
shown as well.

I only applied this fix to the new MD settings. Should it be fixed for the old
settings as well?

https://screenshot.googleplex.com/7rMF5f26CmW

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

to

==========
Display content settings for extensions with extension name.

Instead of showing a cryptic id for content settings for extensions,
display the extension name instead. If an extension has a favicon it is
shown as well.

I only applied this fix to the new MD settings. Should it be fixed for the old
settings as well?

https://screenshot.googleplex.com/7rMF5f26CmW
https://screenshot.googleplex.com/9TbvP1zDYWJ

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

[dullweber, 2016-11-24 09:54:31]

Description was changed from

==========
Display content settings for extensions with extension name.

Instead of showing a cryptic id for content settings for extensions,
display the extension name instead. If an extension has a favicon it is
shown as well.

I only applied this fix to the new MD settings. Should it be fixed for the old
settings as well?

https://screenshot.googleplex.com/7rMF5f26CmW
https://screenshot.googleplex.com/9TbvP1zDYWJ

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

to

==========
Display content settings and zoomlevel for extensions with extension name
instead of showing a cryptic id.
If an extension has a favicon it is shown as well.

I only applied this fix to the new MD settings. Should it be fixed for the old
settings as well?

https://screenshot.googleplex.com/7rMF5f26CmW
https://screenshot.googleplex.com/9TbvP1zDYWJ

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

[dullweber, 2016-11-24 10:12:17]

dullweber@chromium.org changed reviewers:
+ msramek@chromium.org

[dullweber, 2016-11-24 10:12:17]

Martin, please review these changes

[dullweber, 2016-11-24 10:14:22]

Description was changed from

==========
Display content settings and zoomlevel for extensions with extension name
instead of showing a cryptic id.
If an extension has a favicon it is shown as well.

I only applied this fix to the new MD settings. Should it be fixed for the old
settings as well?

https://screenshot.googleplex.com/7rMF5f26CmW
https://screenshot.googleplex.com/9TbvP1zDYWJ

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

to

==========
Display content settings and zoomlevel for extensions with extension name
instead of showing a cryptic id.
If an extension has a favicon, it is shown as well.

https://screenshot.googleplex.com/7rMF5f26CmW
https://screenshot.googleplex.com/9TbvP1zDYWJ

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

[dullweber, 2016-11-24 10:14:32]

On 2016/11/24 10:12:17, dullweber wrote:
> Martin, please review these changes

I only applied this fix to the new MD settings. Should it be fixed for the old
settings as well?

[commit-bot: I haz the power, 2016-11-24 10:43:05]

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

[commit-bot: I haz the power, 2016-11-24 10:43:06]

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

[msramek, 2016-11-24 11:38:20]

https://codereview.chromium.org/2525943002/diff/50001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/site_settings_handler.cc (right):

https://codereview.chromium.org/2525943002/diff/50001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/site_settings_handler.cc:424: map,
content_type, profile_, web_ui(), /*incognito=*/false,
nit: Not your code, but while you're here - these comments seem to miss some
whitespace.

https://codereview.chromium.org/2525943002/diff/50001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/site_settings_handler.cc:634: for (auto i =
zoom_levels.cbegin(); i != zoom_levels.cend(); ++i) {
This looks like it could be replaced by a range-based loop.

https://codereview.chromium.org/2525943002/diff/50001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/site_settings_helper.cc (right):

https://codereview.chromium.org/2525943002/diff/50001/chrome/browser/ui/webui...
chrome/browser/ui/webui/site_settings_helper.cc:164: if (extension) {
optional style nit: I wouldn't use braces with one-line return.

https://codereview.chromium.org/2525943002/diff/50001/chrome/browser/ui/webui...
chrome/browser/ui/webui/site_settings_helper.cc:173: Profile* profile,
I would pass ExtensionRegistry directly.

The current state is:
- inconsistent with GetPolicyAllowedUrls
- inconsistent with itself; ExtensionRegistry is created from Profile by the
method, but HCSM is created from the same profile by the caller

https://codereview.chromium.org/2525943002/diff/50001/components/content_sett...
File components/content_settings/core/common/content_settings_pattern.cc
(right):

https://codereview.chromium.org/2525943002/diff/50001/components/content_sett...
components/content_settings/core/common/content_settings_pattern.cc:607: return
parts_.host;
This is a bit misleading, because the pattern might have a domain wildcard, in
which case the string representation of host is "[*.]<host>". Which is also not
very practical, as it must be parsed by the receiver again.

Maybe it's better to work around it?

GURL url(pattern.ToString())
DCHECK(url.is_valid());  // For the extension scheme, the pattern must be a
valid URL
GetExtensionById(url.host())

[dullweber, 2016-11-24 12:18:56]

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

[commit-bot: I haz the power, 2016-11-24 12:19:19]

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

[dullweber, 2016-11-24 12:46:20]

https://codereview.chromium.org/2525943002/diff/50001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/site_settings_handler.cc (right):

https://codereview.chromium.org/2525943002/diff/50001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/site_settings_handler.cc:634: for (auto i =
zoom_levels.cbegin(); i != zoom_levels.cend(); ++i) {
On 2016/11/24 11:38:19, msramek wrote:
> This looks like it could be replaced by a range-based loop.

that's even better

https://codereview.chromium.org/2525943002/diff/50001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/site_settings_helper.cc (right):

https://codereview.chromium.org/2525943002/diff/50001/chrome/browser/ui/webui...
chrome/browser/ui/webui/site_settings_helper.cc:164: if (extension) {
On 2016/11/24 11:38:20, msramek wrote:
> optional style nit: I wouldn't use braces with one-line return.

Done.

https://codereview.chromium.org/2525943002/diff/50001/chrome/browser/ui/webui...
chrome/browser/ui/webui/site_settings_helper.cc:173: Profile* profile,
On 2016/11/24 11:38:20, msramek wrote:
> I would pass ExtensionRegistry directly.
> 
> The current state is:
> - inconsistent with GetPolicyAllowedUrls
> - inconsistent with itself; ExtensionRegistry is created from Profile by the
> method, but HCSM is created from the same profile by the caller

Done.

https://codereview.chromium.org/2525943002/diff/50001/components/content_sett...
File components/content_settings/core/common/content_settings_pattern.cc
(right):

https://codereview.chromium.org/2525943002/diff/50001/components/content_sett...
components/content_settings/core/common/content_settings_pattern.cc:607: return
parts_.host;
On 2016/11/24 11:38:20, msramek wrote:
> This is a bit misleading, because the pattern might have a domain wildcard, in
> which case the string representation of host is "[*.]<host>". Which is also
not
> very practical, as it must be parsed by the receiver again.
> 
> Maybe it's better to work around it?
> 
> GURL url(pattern.ToString())
> DCHECK(url.is_valid());  // For the extension scheme, the pattern must be a
> valid URL
> GetExtensionById(url.host())
 
ok, that works too and doesn't require a new method, great :D

[msramek, 2016-11-24 12:54:00]

LGTM, thanks!

I didn't review the rest thoroughly, leaving that to the respective owners.

https://codereview.chromium.org/2525943002/diff/70001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/site_settings_helper.cc (right):

https://codereview.chromium.org/2525943002/diff/70001/chrome/browser/ui/webui...
chrome/browser/ui/webui/site_settings_helper.cc:162: // For the extension
scheme, the pattern must be a valid URL
nit: Period at the end of the sentence.

[dullweber, 2016-11-24 13:05:02]

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

[dullweber, 2016-11-24 13:05:02]

dschuyler@chromium.org: Please review these changes

[dschuyler, 2016-11-28 21:58:43]

https://codereview.chromium.org/2525943002/diff/90001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/site_list.html (right):

https://codereview.chromium.org/2525943002/diff/90001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_list.html:69:
style$="[[computeSiteIcon(item.origin)]]">
I'm not clear on why this change was made origin rather
than leaving it as originForDisplay or maybe originForFavicon.
Could ya tell me more about this change?

https://codereview.chromium.org/2525943002/diff/90001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/zoom_levels.html (right):

https://codereview.chromium.org/2525943002/diff/90001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/zoom_levels.html:29:
<div>[[item.displayName]]</div>
WDYT: Would it help to filter zoom level sites 
through expandSiteException() and use originForDisplay
here? If a zoom level will never have an embedded origin
and will always have a displayName then maybe it won't help.

https://codereview.chromium.org/2525943002/diff/90001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/site_settings_handler.cc (right):

https://codereview.chromium.org/2525943002/diff/90001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/site_settings_handler.cc:653: if (host.length()
== 32) {
The code is ok, maybe change the comments around a bit.

The current comment gave me the impression that we are
using the host length to determine whether this is an
extension, which sounds like a potential security issue.
After reading it more closely, it looks like we could
treat every entry as a possible extension; and checking
the host length is only an optimization (to reduce time
spend on checking hosts that will never be an extension)
and therefor not a security concern.

If I'm correct on that, please alter the comments to so
that it's more obvious to the next reader. 

Maybe

// As an optimization, only check hosts that could be an extension.
if (host.lenth() == 32) {
  // Look up the host as an extension, if found then it is an extension.
  [...]

https://codereview.chromium.org/2525943002/diff/90001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/site_settings_helper_unittest.cc (right):

https://codereview.chromium.org/2525943002/diff/90001/chrome/browser/ui/webui...
chrome/browser/ui/webui/site_settings_helper_unittest.cc:60: false /* incognito
*/, nullptr /* filter */, &exceptions);
Optional: 

I know that argument comments appear as
pre and post -fix in the code; and that the post-fix
was already used in this file, but it makes sense 
to me to migrate one way or the other, so I'd like 
to suggest going with the style guide suggestion of
prefix comments like:
  ..., /*extension_registry=*/nullptr , ...
prefixing and taking the spaces out makes it much
easier for me to pick out the parameters in the
list.

Described here:
https://google.github.io/styleguide/cppguide.html#Function_Comments

[dullweber, 2016-11-29 09:34:18]

https://codereview.chromium.org/2525943002/diff/90001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/site_list.html (right):

https://codereview.chromium.org/2525943002/diff/90001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_list.html:69:
style$="[[computeSiteIcon(item.origin)]]">
On 2016/11/28 21:58:42, dschuyler wrote:
> I'm not clear on why this change was made origin rather
> than leaving it as originForDisplay or maybe originForFavicon.
> Could ya tell me more about this change?

In case of extensions, the originForDisplay is now a text, e.g. "Bookmark
Manager". The favicon function needs a "chrome-extension://asdfghjkl..." url.
Maybe I should rename "originForDisplay" to something like "name" to indicate
that it might not have anything to do with an origin?
I don't need originForFavicon for the site-list because origin already contains
the scheme. Only Zoomlevels need a separate handling for this.

https://codereview.chromium.org/2525943002/diff/90001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/zoom_levels.html (right):

https://codereview.chromium.org/2525943002/diff/90001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/zoom_levels.html:29:
<div>[[item.displayName]]</div>
On 2016/11/28 21:58:42, dschuyler wrote:
> WDYT: Would it help to filter zoom level sites 
> through expandSiteException() and use originForDisplay
> here? If a zoom level will never have an embedded origin
> and will always have a displayName then maybe it won't help.

I don't think the expansion code is useful because exceptions and zoomlevels
work differently.
Content-settings use scheme+host as origin and maybe an additional
embeddedOrigin to refer to a rule. Zoomlevels usually use just the host as an
identifier to have a consistent behavior between http and https urls. This is
the reason, why I have to use the weird extension detection code, to find out if
something is probably an extension and get the right extension url to find the
favicon.

https://codereview.chromium.org/2525943002/diff/90001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/site_settings_handler.cc (right):

https://codereview.chromium.org/2525943002/diff/90001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/site_settings_handler.cc:653: if (host.length()
== 32) {
On 2016/11/28 21:58:42, dschuyler wrote:
> The code is ok, maybe change the comments around a bit.
> 
> The current comment gave me the impression that we are
> using the host length to determine whether this is an
> extension, which sounds like a potential security issue.
> After reading it more closely, it looks like we could
> treat every entry as a possible extension; and checking
> the host length is only an optimization (to reduce time
> spend on checking hosts that will never be an extension)
> and therefor not a security concern.
> 
> If I'm correct on that, please alter the comments to so
> that it's more obvious to the next reader. 
> 
> Maybe
> 
> // As an optimization, only check hosts that could be an extension.
> if (host.lenth() == 32) {
>   // Look up the host as an extension, if found then it is an extension.
>   [...]
> 

That's right. I wanted to avoid checking every entry. I used your comment
suggestion.

https://codereview.chromium.org/2525943002/diff/90001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/site_settings_helper_unittest.cc (right):

https://codereview.chromium.org/2525943002/diff/90001/chrome/browser/ui/webui...
chrome/browser/ui/webui/site_settings_helper_unittest.cc:60: false /* incognito
*/, nullptr /* filter */, &exceptions);
On 2016/11/28 21:58:42, dschuyler wrote:
> Optional: 
> 
> I know that argument comments appear as
> pre and post -fix in the code; and that the post-fix
> was already used in this file, but it makes sense 
> to me to migrate one way or the other, so I'd like 
> to suggest going with the style guide suggestion of
> prefix comments like:
>   ..., /*extension_registry=*/nullptr , ...
> prefixing and taking the spaces out makes it much
> easier for me to pick out the parameters in the
> list.
> 
> Described here:
> https://google.github.io/styleguide/cppguide.html#Function_Comments

Done.

[dschuyler, 2016-11-29 22:46:59]

LGTM (with a suggestion that I think would be an improvement).

https://codereview.chromium.org/2525943002/diff/90001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/site_list.html (right):

https://codereview.chromium.org/2525943002/diff/90001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_list.html:69:
style$="[[computeSiteIcon(item.origin)]]">
On 2016/11/29 09:34:18, dullweber wrote:
> On 2016/11/28 21:58:42, dschuyler wrote:
> > I'm not clear on why this change was made origin rather
> > than leaving it as originForDisplay or maybe originForFavicon.
> > Could ya tell me more about this change?
> 
> In case of extensions, the originForDisplay is now a text, e.g. "Bookmark
> Manager". The favicon function needs a "chrome-extension://asdfghjkl..." url.
> Maybe I should rename "originForDisplay" to something like "name" to indicate
> that it might not have anything to do with an origin?
> I don't need originForFavicon for the site-list because origin already
contains
> the scheme. Only Zoomlevels need a separate handling for this.

I like the variable label of |displayName| used elsewhere in this CL.
That could be an optional or separate CL. I.e. I don't want to hold up
this CL for that change, but I think it would be nicer than |origin|
which may not be an origin. 

Maybe the C++ side could set |origin| and
|displayName| separately rather than 
interpreting it on the JS side.

Said another way, I'm suggesting this series of steps:
- that this change back to originForDisplay
- originForDisplay be renamed as displayName
- displayName set on the C++ side to be either an origin intended for display or
an extension name (or something else in the future). [With this CL setting
displayName - does displayName get set in all cases?]
- origin with then only be an origin (or maybe empty if there is not relevant
origin).

WDYT? I'm open to chatting about this outside of this CL if you prefer.

[dullweber, 2016-11-30 09:29:51]

On 2016/11/29 22:46:59, dschuyler wrote:
> LGTM (with a suggestion that I think would be an improvement).
> 
>
https://codereview.chromium.org/2525943002/diff/90001/chrome/browser/resource...
> File chrome/browser/resources/settings/site_settings/site_list.html (right):
> 
>
https://codereview.chromium.org/2525943002/diff/90001/chrome/browser/resource...
> chrome/browser/resources/settings/site_settings/site_list.html:69:
> style$="[[computeSiteIcon(item.origin)]]">
> On 2016/11/29 09:34:18, dullweber wrote:
> > On 2016/11/28 21:58:42, dschuyler wrote:
> > > I'm not clear on why this change was made origin rather
> > > than leaving it as originForDisplay or maybe originForFavicon.
> > > Could ya tell me more about this change?
> > 
> > In case of extensions, the originForDisplay is now a text, e.g. "Bookmark
> > Manager". The favicon function needs a "chrome-extension://asdfghjkl..."
url.
> > Maybe I should rename "originForDisplay" to something like "name" to
indicate
> > that it might not have anything to do with an origin?
> > I don't need originForFavicon for the site-list because origin already
> contains
> > the scheme. Only Zoomlevels need a separate handling for this.
> 
> I like the variable label of |displayName| used elsewhere in this CL.
> That could be an optional or separate CL. I.e. I don't want to hold up
> this CL for that change, but I think it would be nicer than |origin|
> which may not be an origin. 
> 
> Maybe the C++ side could set |origin| and
> |displayName| separately rather than 
> interpreting it on the JS side.
> 
> Said another way, I'm suggesting this series of steps:
> - that this change back to originForDisplay
> - originForDisplay be renamed as displayName
> - displayName set on the C++ side to be either an origin intended for display
or
> an extension name (or something else in the future). [With this CL setting
> displayName - does displayName get set in all cases?]
> - origin with then only be an origin (or maybe empty if there is not relevant
> origin).
> 
> WDYT? I'm open to chatting about this outside of this CL if you prefer.

That sounds like a good idea. There is also |embeddingOriginForDisplay|. I will
rename it to |embeddingDisplayName|. 
I already implemented this change now to improve the consistency. I checked the
browserProxy calls and |displayName| should be set in all cases in
GetExceptionForPage() (site_settings_helper.cc).
site_details.html is also affected but I'm not sure if it is used anywhere so I
couldn't test it.

[dullweber, 2016-11-30 09:51:49]

dullweber@chromium.org changed reviewers:
+ dbeam@chromium.org

[dullweber, 2016-11-30 09:51:50]

dbeam@chromium.org: Please review changes in webui/ and webui/options/

[dschuyler, 2016-11-30 19:23:33]

On 2016/11/30 09:29:51, dullweber wrote:
> On 2016/11/29 22:46:59, dschuyler wrote:
> > LGTM (with a suggestion that I think would be an improvement).
> > 
> >
>
https://codereview.chromium.org/2525943002/diff/90001/chrome/browser/resource...
> > File chrome/browser/resources/settings/site_settings/site_list.html (right):
> > 
> >
>
https://codereview.chromium.org/2525943002/diff/90001/chrome/browser/resource...
> > chrome/browser/resources/settings/site_settings/site_list.html:69:
> > style$="[[computeSiteIcon(item.origin)]]">
> > On 2016/11/29 09:34:18, dullweber wrote:
> > > On 2016/11/28 21:58:42, dschuyler wrote:
> > > > I'm not clear on why this change was made origin rather
> > > > than leaving it as originForDisplay or maybe originForFavicon.
> > > > Could ya tell me more about this change?
> > > 
> > > In case of extensions, the originForDisplay is now a text, e.g. "Bookmark
> > > Manager". The favicon function needs a "chrome-extension://asdfghjkl..."
> url.
> > > Maybe I should rename "originForDisplay" to something like "name" to
> indicate
> > > that it might not have anything to do with an origin?
> > > I don't need originForFavicon for the site-list because origin already
> > contains
> > > the scheme. Only Zoomlevels need a separate handling for this.
> > 
> > I like the variable label of |displayName| used elsewhere in this CL.
> > That could be an optional or separate CL. I.e. I don't want to hold up
> > this CL for that change, but I think it would be nicer than |origin|
> > which may not be an origin. 
> > 
> > Maybe the C++ side could set |origin| and
> > |displayName| separately rather than 
> > interpreting it on the JS side.
> > 
> > Said another way, I'm suggesting this series of steps:
> > - that this change back to originForDisplay
> > - originForDisplay be renamed as displayName
> > - displayName set on the C++ side to be either an origin intended for
display
> or
> > an extension name (or something else in the future). [With this CL setting
> > displayName - does displayName get set in all cases?]
> > - origin with then only be an origin (or maybe empty if there is not
relevant
> > origin).
> > 
> > WDYT? I'm open to chatting about this outside of this CL if you prefer.
> 
> That sounds like a good idea. There is also |embeddingOriginForDisplay|. I
will
> rename it to |embeddingDisplayName|. 
> I already implemented this change now to improve the consistency. I checked
the
> browserProxy calls and |displayName| should be set in all cases in
> GetExceptionForPage() (site_settings_helper.cc).
> site_details.html is also affected but I'm not sure if it is used anywhere so
I
> couldn't test it.

I believe site_details.html is only accessible when the chrome://flags "site
settings"
flag is enabled. That page is a work-in-progress. It likely won't get seen much
until
January or later, so it's not critical. (If you get a chance to check it, that
would be a kindness).

[dullweber, 2016-12-01 09:08:09]

On 2016/11/30 19:23:33, dschuyler wrote:
> I believe site_details.html is only accessible when the chrome://flags "site
> settings"
> flag is enabled. That page is a work-in-progress. It likely won't get seen
much
> until
> January or later, so it's not critical. (If you get a chance to check it, that
> would be a kindness).


"All sites" and "Site details" look good and changing permissions for extensions
works fine!

[Dan Beam, 2016-12-02 00:44:50]

ooooh, fancy

lgtm

https://codereview.chromium.org/2525943002/diff/130001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/settings/site_settings_handler.cc (right):

https://codereview.chromium.org/2525943002/diff/130001/chrome/browser/ui/webu...
chrome/browser/ui/webui/settings/site_settings_handler.cc:650: std::string
displayName = host;
cpp_vars_like_this

https://codereview.chromium.org/2525943002/diff/130001/chrome/browser/ui/webu...
chrome/browser/ui/webui/settings/site_settings_handler.cc:653: if (host.length()
== 32) {
there must be a way to check for valid extension IDs or hosts other than this...

[dullweber, 2016-12-02 10:27:25]

https://codereview.chromium.org/2525943002/diff/130001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/settings/site_settings_handler.cc (right):

https://codereview.chromium.org/2525943002/diff/130001/chrome/browser/ui/webu...
chrome/browser/ui/webui/settings/site_settings_handler.cc:650: std::string
displayName = host;
On 2016/12/02 00:44:50, Dan Beam wrote:
> cpp_vars_like_this

Done.

https://codereview.chromium.org/2525943002/diff/130001/chrome/browser/ui/webu...
chrome/browser/ui/webui/settings/site_settings_handler.cc:653: if (host.length()
== 32) {
On 2016/12/02 00:44:50, Dan Beam wrote:
> there must be a way to check for valid extension IDs or hosts other than
this...

I think I found the proper id validation code:
crx_file::id_util::IdIsValid()

[dullweber, 2016-12-02 10:27:40]

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

[commit-bot: I haz the power, 2016-12-02 10:27:57]

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, 2016-12-02 11:07:23]

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

[commit-bot: I haz the power, 2016-12-02 11:07:24]

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

[dullweber, 2016-12-02 12:09:49]

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

[commit-bot: I haz the power, 2016-12-02 12:10:02]

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, 2016-12-02 13:03:10]

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

[commit-bot: I haz the power, 2016-12-02 13:03:11]

Dry run: This issue passed the CQ dry run.

[dullweber, 2016-12-05 13:25:06]

The CQ bit was checked by dullweber@chromium.org

[dullweber, 2016-12-05 13:25:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
msramek@chromium.org, dschuyler@chromium.org, dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/2525943002/#ps210001
(title: "deduplicate by origin")

[commit-bot: I haz the power, 2016-12-05 13:25: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, 2016-12-05 14:49:47]

CQ is committing da patch.

Bot data: {"patchset_id": 210001, "attempt_start_ts": 1480944306571040,
"parent_rev": "723242d38a9e7dd8e4f65b8b9c743c2f4c3a5283", "commit_rev":
"751ecb224e81f13db39d5193327ca157eb40308f"}

[commit-bot: I haz the power, 2016-12-05 14:50:14]

Description was changed from

==========
Display content settings and zoomlevel for extensions with extension name
instead of showing a cryptic id.
If an extension has a favicon, it is shown as well.

https://screenshot.googleplex.com/7rMF5f26CmW
https://screenshot.googleplex.com/9TbvP1zDYWJ

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

to

==========
Display content settings and zoomlevel for extensions with extension name
instead of showing a cryptic id.
If an extension has a favicon, it is shown as well.

https://screenshot.googleplex.com/7rMF5f26CmW
https://screenshot.googleplex.com/9TbvP1zDYWJ

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

[commit-bot: I haz the power, 2016-12-05 14:50:16]

Committed patchset #12 (id:210001)

[commit-bot: I haz the power, 2016-12-05 14:52:45]

Description was changed from

==========
Display content settings and zoomlevel for extensions with extension name
instead of showing a cryptic id.
If an extension has a favicon, it is shown as well.

https://screenshot.googleplex.com/7rMF5f26CmW
https://screenshot.googleplex.com/9TbvP1zDYWJ

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

to

==========
Display content settings and zoomlevel for extensions with extension name
instead of showing a cryptic id.
If an extension has a favicon, it is shown as well.

https://screenshot.googleplex.com/7rMF5f26CmW
https://screenshot.googleplex.com/9TbvP1zDYWJ

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

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

[commit-bot: I haz the power, 2016-12-05 14:52:47]

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