Adding <keygen> Content Setting (UI)

Adding <keygen> Content Setting (UI)

Adding the KEYGEN content setting non-Android UI.

BUG=514767
Committed: https://crrev.com/18120959576e11ad4dec9edd2c3539ed0bba7cc7
Cr-Commit-Position: refs/heads/master@{#369542}

[svaldez, 2015-11-03 15:38:25]

svaldez@chromium.org changed reviewers:
+ jochen@chromium.org, raymes@chromium.org, rsleevi@chromium.org

[raymes, 2015-11-05 02:10:17]

raymes@chromium.org changed reviewers:
+ bauerb@google.com, msramek@chromium.org
- jochen@chromium.org, rsleevi@chromium.org

[raymes, 2015-11-05 02:10:18]

+msramek and bauerb who know this code

[Bernhard Bauer, 2015-11-05 11:13:31]

bauerb@chromium.org changed reviewers:
+ bauerb@chromium.org

[Bernhard Bauer, 2015-11-05 11:13:32]

https://codereview.chromium.org/1412523003/diff/20001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1412523003/diff/20001/chrome/app/generated_re...
chrome/app/generated_resources.grd:306: +      <message
name="IDS_WEBSITE_SETTINGS_TYPE_KEYGEN" desc="The label used for Keygen
permission controls in the Website Settings popup.">
These strings will be translated into a ton of languages. You will have to
explain what "Keygen" is in the description if you want these translations to
make any sense whatsoever.

https://codereview.chromium.org/1412523003/diff/20001/chrome/app/generated_re...
chrome/app/generated_resources.grd:7952: +        Allow all sites to use keygen.
If the primary use case for this content setting is enterprise, consider not
exposing it in the UI at all, or not allowing it by default.

https://codereview.chromium.org/1412523003/diff/20001/chrome/app/generated_re...
chrome/app/generated_resources.grd:7955: +        Do not allow any site to use
keygen (recommended)
At the very least you should be consistent in whether you capitalize keygen or
not. Even better would be a more descriptive term.

[msramek, 2015-11-05 16:22:32]

Please improve the CL description in at least one of the CLs in this series.

It's hard to judge whether and how to expose this setting without any
background.

https://codereview.chromium.org/1412523003/diff/20001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1412523003/diff/20001/chrome/app/generated_re...
chrome/app/generated_resources.grd:7952: +        Allow all sites to use keygen.
On 2015/11/05 11:13:31, Bernhard Bauer wrote:
> If the primary use case for this content setting is enterprise, consider not
> exposing it in the UI at all, or not allowing it by default.

+1

https://codereview.chromium.org/1412523003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/options/content_settings_exceptions_area.html
(right):

https://codereview.chromium.org/1412523003/diff/20001/chrome/browser/resource...
chrome/browser/resources/options/content_settings_exceptions_area.html:46: <div
contentType="keygen">
I would recommend putting this towards the bottom of the list, as it is probably
not going to be used by a lot of users.

https://codereview.chromium.org/1412523003/diff/20001/chrome/browser/ui/websi...
File chrome/browser/ui/website_settings/website_settings.cc (right):

https://codereview.chromium.org/1412523003/diff/20001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/website_settings.cc:285: type ==
CONTENT_SETTINGS_TYPE_KEYGEN ||
This sounds like a security feature to me, so I would expect it to be set per
origin (FromURLNoWildcard) rather than broadly (FromURL means all subdomains).
Please double-check.

[svaldez, 2015-11-05 16:29:56]

https://codereview.chromium.org/1412523003/diff/20001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1412523003/diff/20001/chrome/app/generated_re...
chrome/app/generated_resources.grd:306: +      <message
name="IDS_WEBSITE_SETTINGS_TYPE_KEYGEN" desc="The label used for Keygen
permission controls in the Website Settings popup.">
On 2015/11/05 11:13:31, Bernhard Bauer wrote:
> These strings will be translated into a ton of languages. You will have to
> explain what "Keygen" is in the description if you want these translations to
> make any sense whatsoever.

Done.

https://codereview.chromium.org/1412523003/diff/20001/chrome/app/generated_re...
chrome/app/generated_resources.grd:7952: +        Allow all sites to use keygen.
On 2015/11/05 11:13:31, Bernhard Bauer wrote:
> If the primary use case for this content setting is enterprise, consider not
> exposing it in the UI at all, or not allowing it by default.

Its currently set to be disabled by default, but there a small number of
non-enterprise users that might need to use keygen until we've deprecated it.

https://codereview.chromium.org/1412523003/diff/20001/chrome/app/generated_re...
chrome/app/generated_resources.grd:7955: +        Do not allow any site to use
keygen (recommended)
On 2015/11/05 11:13:31, Bernhard Bauer wrote:
> At the very least you should be consistent in whether you capitalize keygen or
> not. Even better would be a more descriptive term.

Fixed, hopefully.

[svaldez, 2015-11-05 16:35:12]

https://codereview.chromium.org/1412523003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/options/content_settings_exceptions_area.html
(right):

https://codereview.chromium.org/1412523003/diff/20001/chrome/browser/resource...
chrome/browser/resources/options/content_settings_exceptions_area.html:46: <div
contentType="keygen">
On 2015/11/05 16:22:31, msramek wrote:
> I would recommend putting this towards the bottom of the list, as it is
probably
> not going to be used by a lot of users.

Done.

https://codereview.chromium.org/1412523003/diff/20001/chrome/browser/ui/websi...
File chrome/browser/ui/website_settings/website_settings.cc (right):

https://codereview.chromium.org/1412523003/diff/20001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/website_settings.cc:285: type ==
CONTENT_SETTINGS_TYPE_KEYGEN ||
On 2015/11/05 16:22:31, msramek wrote:
> This sounds like a security feature to me, so I would expect it to be set per
> origin (FromURLNoWildcard) rather than broadly (FromURL means all subdomains).
> Please double-check.

Done.

[svaldez, 2015-11-05 16:41:48]

On 2015/11/05 16:22:32, msramek wrote:
> Please improve the CL description in at least one of the CLs in this series.
> 
> It's hard to judge whether and how to expose this setting without any
> background.
> 
>
https://codereview.chromium.org/1412523003/diff/20001/chrome/app/generated_re...
> File chrome/app/generated_resources.grd (right):
> 
>
https://codereview.chromium.org/1412523003/diff/20001/chrome/app/generated_re...
> chrome/app/generated_resources.grd:7952: +        Allow all sites to use
keygen.
> On 2015/11/05 11:13:31, Bernhard Bauer wrote:
> > If the primary use case for this content setting is enterprise, consider not
> > exposing it in the UI at all, or not allowing it by default.
> 
> +1
> 
>
https://codereview.chromium.org/1412523003/diff/20001/chrome/browser/resource...
> File chrome/browser/resources/options/content_settings_exceptions_area.html
> (right):
> 
>
https://codereview.chromium.org/1412523003/diff/20001/chrome/browser/resource...
> chrome/browser/resources/options/content_settings_exceptions_area.html:46:
<div
> contentType="keygen">
> I would recommend putting this towards the bottom of the list, as it is
probably
> not going to be used by a lot of users.
> 
>
https://codereview.chromium.org/1412523003/diff/20001/chrome/browser/ui/websi...
> File chrome/browser/ui/website_settings/website_settings.cc (right):
> 
>
https://codereview.chromium.org/1412523003/diff/20001/chrome/browser/ui/websi...
> chrome/browser/ui/website_settings/website_settings.cc:285: type ==
> CONTENT_SETTINGS_TYPE_KEYGEN ||
> This sounds like a security feature to me, so I would expect it to be set per
> origin (FromURLNoWildcard) rather than broadly (FromURL means all subdomains).
> Please double-check.

Updated the description of https://codereview.chromium.org/1417173010/ with the
purpose of the Content Setting and a link to the blink-dev thread.

[Bernhard Bauer, 2015-11-05 16:42:42]

https://codereview.chromium.org/1412523003/diff/20001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1412523003/diff/20001/chrome/app/generated_re...
chrome/app/generated_resources.grd:7952: +        Allow all sites to use keygen.
On 2015/11/05 16:29:56, svaldez wrote:
> On 2015/11/05 11:13:31, Bernhard Bauer wrote:
> > If the primary use case for this content setting is enterprise, consider not
> > exposing it in the UI at all, or not allowing it by default.
> 
> Its currently set to be disabled by default, but there a small number of
> non-enterprise users that might need to use keygen until we've deprecated it.

You could remove the option to have it enabled by default for all websites, but
up to you.

https://codereview.chromium.org/1412523003/diff/60001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1412523003/diff/60001/chrome/app/generated_re...
chrome/app/generated_resources.grd:2634: +        A form attempting to use key
generation was blocked on this page.
Feel free to throw in some mention of "deprecated", "legacy" or "insecure" to
explain why this is a Bad Thing™ 😃

And in general it might be a good idea to have a UI person look over the
strings.

https://codereview.chromium.org/1412523003/diff/60001/chrome/app/generated_re...
chrome/app/generated_resources.grd:7946: +        Key Generation
The second word probably should be lowercase.

https://codereview.chromium.org/1412523003/diff/60001/chrome/app/generated_re...
chrome/app/generated_resources.grd:7949: +        Keyg Generation exceptions
"Key", and the second word lowercase.

[svaldez, 2015-11-05 17:49:32]

I'll ping a UI person to look over the strings for both this and the Android UI
CL.

https://codereview.chromium.org/1412523003/diff/60001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1412523003/diff/60001/chrome/app/generated_re...
chrome/app/generated_resources.grd:2634: +        A form attempting to use key
generation was blocked on this page.
On 2015/11/05 16:42:42, Bernhard Bauer wrote:
> Feel free to throw in some mention of "deprecated", "legacy" or "insecure" to
> explain why this is a Bad Thing™ 😃
> 
> And in general it might be a good idea to have a UI person look over the
> strings.

Acknowledged.

https://codereview.chromium.org/1412523003/diff/60001/chrome/app/generated_re...
chrome/app/generated_resources.grd:7946: +        Key Generation
On 2015/11/05 16:42:42, Bernhard Bauer wrote:
> The second word probably should be lowercase.

Done.

https://codereview.chromium.org/1412523003/diff/60001/chrome/app/generated_re...
chrome/app/generated_resources.grd:7949: +        Keyg Generation exceptions
On 2015/11/05 16:42:42, Bernhard Bauer wrote:
> "Key", and the second word lowercase.

Done.

[svaldez, 2015-12-01 20:08:22]

On 2015/11/05 17:49:32, svaldez wrote:
> I'll ping a UI person to look over the strings for both this and the Android
UI
> CL.
> 
>
https://codereview.chromium.org/1412523003/diff/60001/chrome/app/generated_re...
> File chrome/app/generated_resources.grd (right):
> 
>
https://codereview.chromium.org/1412523003/diff/60001/chrome/app/generated_re...
> chrome/app/generated_resources.grd:2634: +        A form attempting to use key
> generation was blocked on this page.
> On 2015/11/05 16:42:42, Bernhard Bauer wrote:
> > Feel free to throw in some mention of "deprecated", "legacy" or "insecure"
to
> > explain why this is a Bad Thing™ 😃
> > 
> > And in general it might be a good idea to have a UI person look over the
> > strings.
> 
> Acknowledged.
> 
>
https://codereview.chromium.org/1412523003/diff/60001/chrome/app/generated_re...
> chrome/app/generated_resources.grd:7946: +        Key Generation
> On 2015/11/05 16:42:42, Bernhard Bauer wrote:
> > The second word probably should be lowercase.
> 
> Done.
> 
>
https://codereview.chromium.org/1412523003/diff/60001/chrome/app/generated_re...
> chrome/app/generated_resources.grd:7949: +        Keyg Generation exceptions
> On 2015/11/05 16:42:42, Bernhard Bauer wrote:
> > "Key", and the second word lowercase.
> 
> Done.

Updates to Keygen for review:
- Adding the Keygen setting to the list of content settings to track in the UI.
- Making Keygen default to non-wildcard patterns since Android can't handle
wildcards correctly.

[Bernhard Bauer, 2015-12-02 11:29:03]

lgtm

[msramek, 2015-12-03 17:48:04]

LGTM.

https://codereview.chromium.org/1412523003/diff/170001/chrome/browser/ui/cont...
File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right):

https://codereview.chromium.org/1412523003/diff/170001/chrome/browser/ui/cont...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:124:
{CONTENT_SETTINGS_TYPE_KEYGEN, IDS_BLOCKED_KEYGEN_TITLE},
Please reorder according to the position of CONTENT_SETTINGS_TYPE_KEYGEN in
ContentSettingsType. Here and elsewhere.

[svaldez, 2015-12-04 15:25:31]

https://codereview.chromium.org/1412523003/diff/170001/chrome/browser/ui/cont...
File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right):

https://codereview.chromium.org/1412523003/diff/170001/chrome/browser/ui/cont...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:124:
{CONTENT_SETTINGS_TYPE_KEYGEN, IDS_BLOCKED_KEYGEN_TITLE},
On 2015/12/03 17:48:04, msramek wrote:
> Please reorder according to the position of CONTENT_SETTINGS_TYPE_KEYGEN in
> ContentSettingsType. Here and elsewhere.

Done.

[svaldez, 2015-12-09 22:24:02]

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

[commit-bot: I haz the power, 2015-12-09 22:27:32]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1412523003/190001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1412523003/190001

[commit-bot: I haz the power, 2015-12-10 00:22:14]

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

[commit-bot: I haz the power, 2015-12-10 00:22:15]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[svaldez, 2015-12-16 22:15:54]

On 2015/12/10 00:22:15, commit-bot: I haz the power wrote:
> Dry run: Try jobs failed on following builders:
>   win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

Now we're instead forcing the content setting to show up in the Info Box instead
of as a page action, based on UX feedback. The change was to add a map to
host_content_settings_map to track when we've used a keygen element.

[Bernhard Bauer, 2015-12-17 11:56:03]

FTR, if you have a CL that already has LGTMs, and the requirements change, it is
usually better to make the additional changes in a followup CL.

https://codereview.chromium.org/1412523003/diff/270001/components/content_set...
File components/content_settings/core/browser/host_content_settings_map.h
(right):

https://codereview.chromium.org/1412523003/diff/270001/components/content_set...
components/content_settings/core/browser/host_content_settings_map.h:339:
std::map<GURL, std::set<ContentSettingsType>> display_map_;
This should not live in the HostContentSettingsMap, because it's not a content
setting. Also, content settings are usually persistent, while this will stay
around only until the HCSM is destroyed, right? If what you want is to indicate
usage of a content type (allowed or blocked), TabSpecificContentSettings is the
right place for this.

[svaldez, 2015-12-17 15:25:42]

Updated. Good to know about creating separate CLs.

https://codereview.chromium.org/1412523003/diff/270001/components/content_set...
File components/content_settings/core/browser/host_content_settings_map.h
(right):

https://codereview.chromium.org/1412523003/diff/270001/components/content_set...
components/content_settings/core/browser/host_content_settings_map.h:339:
std::map<GURL, std::set<ContentSettingsType>> display_map_;
On 2015/12/17 11:56:03, Bernhard Bauer wrote:
> This should not live in the HostContentSettingsMap, because it's not a content
> setting. Also, content settings are usually persistent, while this will stay
> around only until the HCSM is destroyed, right? If what you want is to
indicate
> usage of a content type (allowed or blocked), TabSpecificContentSettings is
the
> right place for this.

Done.

[Bernhard Bauer, 2015-12-17 15:51:05]

https://codereview.chromium.org/1412523003/diff/270001/components/content_set...
File components/content_settings/core/browser/host_content_settings_map.h
(right):

https://codereview.chromium.org/1412523003/diff/270001/components/content_set...
components/content_settings/core/browser/host_content_settings_map.h:339:
std::map<GURL, std::set<ContentSettingsType>> display_map_;
On 2015/12/17 15:25:42, svaldez wrote:
> On 2015/12/17 11:56:03, Bernhard Bauer wrote:
> > This should not live in the HostContentSettingsMap, because it's not a
content
> > setting. Also, content settings are usually persistent, while this will stay
> > around only until the HCSM is destroyed, right? If what you want is to
> indicate
> > usage of a content type (allowed or blocked), TabSpecificContentSettings is
> the
> > right place for this.
> 
> Done.

Sorry, I should have been a bit more detailed. TabSpecificContentSettings
already has code to store and find out whether a given content setting was
blocked, with OnContentBlockedWithDetail() and IsContentBlocked(). This is not
keyed on the URL, but as TabSpecificContentSettings has a 1:1 relation to
WebContents, it is stored per tab. Now, I don't know the exact specification in
this case, but for all other content settings this is the desired behavior, as
all the UI elements that indicate blocking are also per tab.

So, you would get by without modifications to TabSpecificContentSettings, and
just check IsContentBlocked() in WebsiteSettingsUI. (You might want to only do
that for the KEYGEN content setting type, just to make sure we don't run into
any weird edge cases with other blocked content setting types.)

[svaldez, 2015-12-17 16:34:45]

On 2015/12/17 15:51:05, Bernhard Bauer wrote:
>
https://codereview.chromium.org/1412523003/diff/270001/components/content_set...
> File components/content_settings/core/browser/host_content_settings_map.h
> (right):
> 
>
https://codereview.chromium.org/1412523003/diff/270001/components/content_set...
> components/content_settings/core/browser/host_content_settings_map.h:339:
> std::map<GURL, std::set<ContentSettingsType>> display_map_;
> On 2015/12/17 15:25:42, svaldez wrote:
> > On 2015/12/17 11:56:03, Bernhard Bauer wrote:
> > > This should not live in the HostContentSettingsMap, because it's not a
> content
> > > setting. Also, content settings are usually persistent, while this will
stay
> > > around only until the HCSM is destroyed, right? If what you want is to
> > indicate
> > > usage of a content type (allowed or blocked), TabSpecificContentSettings
is
> > the
> > > right place for this.
> > 
> > Done.
> 
> Sorry, I should have been a bit more detailed. TabSpecificContentSettings
> already has code to store and find out whether a given content setting was
> blocked, with OnContentBlockedWithDetail() and IsContentBlocked(). This is not
> keyed on the URL, but as TabSpecificContentSettings has a 1:1 relation to
> WebContents, it is stored per tab. Now, I don't know the exact specification
in
> this case, but for all other content settings this is the desired behavior, as
> all the UI elements that indicate blocking are also per tab.
> 
> So, you would get by without modifications to TabSpecificContentSettings, and
> just check IsContentBlocked() in WebsiteSettingsUI. (You might want to only do
> that for the KEYGEN content setting type, just to make sure we don't run into
> any weird edge cases with other blocked content setting types.)

I think the semantics are slightly different, since it would be nice to display
the content setting in the OIB when a Keygen is involved, not just when its
blocked. Though thinking again, its probably fine not to display it when Keygen
has been set to be globally allowed.

[Bernhard Bauer, 2015-12-17 16:50:40]

On 2015/12/17 16:34:45, svaldez wrote:
> On 2015/12/17 15:51:05, Bernhard Bauer wrote:
> >
>
https://codereview.chromium.org/1412523003/diff/270001/components/content_set...
> > File components/content_settings/core/browser/host_content_settings_map.h
> > (right):
> > 
> >
>
https://codereview.chromium.org/1412523003/diff/270001/components/content_set...
> > components/content_settings/core/browser/host_content_settings_map.h:339:
> > std::map<GURL, std::set<ContentSettingsType>> display_map_;
> > On 2015/12/17 15:25:42, svaldez wrote:
> > > On 2015/12/17 11:56:03, Bernhard Bauer wrote:
> > > > This should not live in the HostContentSettingsMap, because it's not a
> > content
> > > > setting. Also, content settings are usually persistent, while this will
> stay
> > > > around only until the HCSM is destroyed, right? If what you want is to
> > > indicate
> > > > usage of a content type (allowed or blocked), TabSpecificContentSettings
> is
> > > the
> > > > right place for this.
> > > 
> > > Done.
> > 
> > Sorry, I should have been a bit more detailed. TabSpecificContentSettings
> > already has code to store and find out whether a given content setting was
> > blocked, with OnContentBlockedWithDetail() and IsContentBlocked(). This is
not
> > keyed on the URL, but as TabSpecificContentSettings has a 1:1 relation to
> > WebContents, it is stored per tab. Now, I don't know the exact specification
> in
> > this case, but for all other content settings this is the desired behavior,
as
> > all the UI elements that indicate blocking are also per tab.
> > 
> > So, you would get by without modifications to TabSpecificContentSettings,
and
> > just check IsContentBlocked() in WebsiteSettingsUI. (You might want to only
do
> > that for the KEYGEN content setting type, just to make sure we don't run
into
> > any weird edge cases with other blocked content setting types.)
> 
> I think the semantics are slightly different, since it would be nice to
display
> the content setting in the OIB when a Keygen is involved, not just when its
> blocked. Though thinking again, its probably fine not to display it when
Keygen
> has been set to be globally allowed.

You actually can do that as well, just call OnContentAllowed() /
IsContentAllowed(). That way the user will see something anytime <keygen> is
used on a page, and the icon should indicate whether is was allowed or blocked.

[svaldez, 2015-12-17 16:58:25]

On 2015/12/17 16:50:40, Bernhard Bauer wrote:
> On 2015/12/17 16:34:45, svaldez wrote:
> > On 2015/12/17 15:51:05, Bernhard Bauer wrote:
> > >
> >
>
https://codereview.chromium.org/1412523003/diff/270001/components/content_set...
> > > File components/content_settings/core/browser/host_content_settings_map.h
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1412523003/diff/270001/components/content_set...
> > > components/content_settings/core/browser/host_content_settings_map.h:339:
> > > std::map<GURL, std::set<ContentSettingsType>> display_map_;
> > > On 2015/12/17 15:25:42, svaldez wrote:
> > > > On 2015/12/17 11:56:03, Bernhard Bauer wrote:
> > > > > This should not live in the HostContentSettingsMap, because it's not a
> > > content
> > > > > setting. Also, content settings are usually persistent, while this
will
> > stay
> > > > > around only until the HCSM is destroyed, right? If what you want is to
> > > > indicate
> > > > > usage of a content type (allowed or blocked),
TabSpecificContentSettings
> > is
> > > > the
> > > > > right place for this.
> > > > 
> > > > Done.
> > > 
> > > Sorry, I should have been a bit more detailed. TabSpecificContentSettings
> > > already has code to store and find out whether a given content setting was
> > > blocked, with OnContentBlockedWithDetail() and IsContentBlocked(). This is
> not
> > > keyed on the URL, but as TabSpecificContentSettings has a 1:1 relation to
> > > WebContents, it is stored per tab. Now, I don't know the exact
specification
> > in
> > > this case, but for all other content settings this is the desired
behavior,
> as
> > > all the UI elements that indicate blocking are also per tab.
> > > 
> > > So, you would get by without modifications to TabSpecificContentSettings,
> and
> > > just check IsContentBlocked() in WebsiteSettingsUI. (You might want to
only
> do
> > > that for the KEYGEN content setting type, just to make sure we don't run
> into
> > > any weird edge cases with other blocked content setting types.)
> > 
> > I think the semantics are slightly different, since it would be nice to
> display
> > the content setting in the OIB when a Keygen is involved, not just when its
> > blocked. Though thinking again, its probably fine not to display it when
> Keygen
> > has been set to be globally allowed.
> 
> You actually can do that as well, just call OnContentAllowed() /
> IsContentAllowed(). That way the user will see something anytime <keygen> is
> used on a page, and the icon should indicate whether is was allowed or
blocked.

Okay, I think for now we'll probably stick with only on blocked to match the
other settings. In which case the only change from pre-LGTM should be adding the
new case for website_settings and removing the page actions.

[Bernhard Bauer, 2015-12-17 17:01:26]

LGTM

[svaldez, 2016-01-05 22:06:59]

The CQ bit was checked by svaldez@chromium.org

[svaldez, 2016-01-05 22:07:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
msramek@chromium.org, bauerb@chromium.org
Link to the patchset: https://codereview.chromium.org/1412523003/#ps330001
(title: "Rebase?")

[commit-bot: I haz the power, 2016-01-05 22:08:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1412523003/330001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1412523003/330001

[commit-bot: I haz the power, 2016-01-05 22:22:00]

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

[commit-bot: I haz the power, 2016-01-05 22:22:01]

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

[svaldez, 2016-01-05 22:28:11]

svaldez@chromium.org changed reviewers:
+ felt@chromium.org, holte@chromium.org, oshima@chromium.org

[svaldez, 2016-01-05 22:28:11]

svaldez@chromium.org changed required reviewers:
+ felt@chromium.org, holte@chromium.org, oshima@chromium.org

[svaldez, 2016-01-05 22:28:12]

Hi,

Could I get OWNERS reviews on these files:

oshima: chrome/app/theme/theme_resources.grd
felt: chrome/browser/ui/website_settings/website_settings.cc
felt: chrome/browser/ui/website_settings/website_settings_ui.cc
holte: tools/metrics/actions/actions.xml

Relevant launch bug with UI approval:
crbug.com/567876

[oshima, 2016-01-05 22:38:48]

https://codereview.chromium.org/1412523003/diff/330001/chrome/app/theme/theme...
File chrome/app/theme/theme_resources.grd (right):

https://codereview.chromium.org/1412523003/diff/330001/chrome/app/theme/theme...
chrome/app/theme/theme_resources.grd:104: <structure type="chrome_scaled_image"
name="IDR_BLOCKED_KEYGEN" file="common/blocked_script.png" />
what's the reason to add new IDR for the same image? Are you going to add new
png later?

[svaldez, 2016-01-05 22:45:09]

On 2016/01/05 22:38:48, oshima wrote:
>
https://codereview.chromium.org/1412523003/diff/330001/chrome/app/theme/theme...
> File chrome/app/theme/theme_resources.grd (right):
> 
>
https://codereview.chromium.org/1412523003/diff/330001/chrome/app/theme/theme...
> chrome/app/theme/theme_resources.grd:104: <structure
type="chrome_scaled_image"
> name="IDR_BLOCKED_KEYGEN" file="common/blocked_script.png" />
> what's the reason to add new IDR for the same image? Are you going to add new
> png later?

I think at this point the usage of keygen is low enough that using the script
icon with the keygen text is probably safer than adding a new icon that might
increase use confusion (passwords vs keygen). The change is made in
theme_resources.grd so that we can easily swap in a new icon if it ends up being
necessary or if the javascript icon changes. Otherwise I can just update
website_settings_ui to use the script IDR for keygen usage. Which do you think
works better?

[oshima, 2016-01-05 23:15:26]

On 2016/01/05 22:45:09, svaldez wrote:
> On 2016/01/05 22:38:48, oshima wrote:
> >
>
https://codereview.chromium.org/1412523003/diff/330001/chrome/app/theme/theme...
> > File chrome/app/theme/theme_resources.grd (right):
> > 
> >
>
https://codereview.chromium.org/1412523003/diff/330001/chrome/app/theme/theme...
> > chrome/app/theme/theme_resources.grd:104: <structure
> type="chrome_scaled_image"
> > name="IDR_BLOCKED_KEYGEN" file="common/blocked_script.png" />
> > what's the reason to add new IDR for the same image? Are you going to add
new
> > png later?
> 
> I think at this point the usage of keygen is low enough that using the script
> icon with the keygen text is probably safer than adding a new icon that might
> increase use confusion (passwords vs keygen). The change is made in
> theme_resources.grd so that we can easily swap in a new icon if it ends up
being
> necessary or if the javascript icon changes. Otherwise I can just update
> website_settings_ui to use the script IDR for keygen usage. Which do you think
> works better?

Thank you for clarification. Just adding comment would be enough then. lgtm

[Steven Holte, 2016-01-06 19:37:02]

actions.xml lgtm

[svaldez, 2016-01-12 17:09:06]

svaldez@chromium.org changed reviewers:
+ palmer@chromium.org

[svaldez, 2016-01-12 17:09:06]

svaldez@chromium.org changed required reviewers:
- felt@chromium.org

[svaldez, 2016-01-12 17:09:07]

felt, palmer: Any chance you could take a look at the
chrome/browser/ui/website_settings/ changes?

[svaldez, 2016-01-12 17:40:11]

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

[commit-bot: I haz the power, 2016-01-12 17:40:56]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1412523003/350001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1412523003/350001

[commit-bot: I haz the power, 2016-01-12 18:48:10]

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

[commit-bot: I haz the power, 2016-01-12 18:48:11]

Dry run: This issue passed the CQ dry run.

[palmer, 2016-01-12 18:51:36]

website_settings LGTM.

[svaldez, 2016-01-14 20:06:00]

The CQ bit was checked by svaldez@chromium.org

[svaldez, 2016-01-14 20:06:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
msramek@chromium.org, bauerb@chromium.org, oshima@chromium.org,
holte@chromium.org, palmer@chromium.org
Link to the patchset: https://codereview.chromium.org/1412523003/#ps370001
(title: "Using new Keygen icons.")

[commit-bot: I haz the power, 2016-01-14 20:06:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1412523003/370001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1412523003/370001

[commit-bot: I haz the power, 2016-01-14 21:12:57]

Committed patchset #20 (id:370001)

[commit-bot: I haz the power, 2016-01-14 22:03:27]

Description was changed from

==========
Adding <keygen> Content Setting (UI)

Adding the KEYGEN content setting non-Android UI.

BUG=514767
==========

to

==========
Adding <keygen> Content Setting (UI)

Adding the KEYGEN content setting non-Android UI.

BUG=514767

Committed: https://crrev.com/18120959576e11ad4dec9edd2c3539ed0bba7cc7
Cr-Commit-Position: refs/heads/master@{#369542}
==========

[commit-bot: I haz the power, 2016-01-14 22:03:28]

Patchset 20 (id:??) landed as
https://crrev.com/18120959576e11ad4dec9edd2c3539ed0bba7cc7
Cr-Commit-Position: refs/heads/master@{#369542}