Adding <keygen> Content Setting (Android UI)

Adding <keygen> Content Setting (Android UI)

This adds Android UI to allow the user to change the Content Setting to block/allow keygen usage on a per-site basis.

To trigger the exception, we also create a Content Exception on any site that uses a <keygen> element.

BUG=514767
Committed: https://crrev.com/34a7682cd49f8fdfb6a0f715a6118076f9ad8224
Cr-Commit-Position: refs/heads/master@{#370416}

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

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

[raymes, 2015-11-05 02:09:11]

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

[raymes, 2015-11-05 02:09:12]

finnur@ are you a good person to review this?

[Bernhard Bauer, 2015-11-05 10:49:38]

Description was changed from

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

Adding the KEYGEN content setting Android UI.

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

to

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

Adding the KEYGEN content setting Android UI.

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

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

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

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

https://codereview.chromium.org/1432573002/diff/1/chrome/android/java/strings...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/1432573002/diff/1/chrome/android/java/strings...
chrome/android/java/strings/android_chrome_strings.grd:526: <message
name="IDS_WEBSITE_SETTINGS_ADD_SITE_DESCRIPTION_KEYGEN" desc="The description
for the allow Keygen for website dialog.">
Same comment as in https://codereview.chromium.org/1412523003/: You should
explain what "Keygen" is, otherwise it won't be possible to translate these.

[Finnur, 2015-11-05 15:42:53]

Yes, I am one of probably two persons who is most intimately familiar with this
code in its current form (having overhauled/written it in large parts). You'll
still need an OWNERS from newt@, though.

One thing to keep in mind also is that the Site Settings categories and
individual permissions under Site Details are ordered by most-sensitive ones
first, and something like this is probably low on that list, so it should appear
near the bottom. Rebecca (rolfe@) can help you determine where. 

But overall I'm surprised we'd add this as a first-class category under Site
Settings (midi, for example, isn't), because this seems obscure and hard to
explain to users what it does.

I suspect your goal is just to get the Keygen permission to appear only under
All Sites and under Site Details for a given origin and for it there to be
toggleable and clearable. A separate category for Keygen seems like overkill to
me. If that intuition is correct then some of the changes proposed are
redundant.

Maybe we should start by clearing up what the goal is?

https://codereview.chromium.org/1432573002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java
(right):

https://codereview.chromium.org/1432573002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java:73:
public static final String PREF_KEYGEN_PERMISSION = "keygen_permission_list";
I am confused. Is this category a toggle-only category or will it show websites
below the toggle that you can change?

If former: Why are we modifying this file?

If it is the latter: Why is there no corresponding keygen_permission_list in
this file:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/jav...

Because if that doesn't exist, the change won't show the Keygen permission in
the Site Details page (how Keygen is set for a particular site).

Wait... lets take a step back here. This file is also not showing any changes:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/jav...

How do you access this new Keygen category in the UI?? At this point I think it
would be prudent to upload some screenshots to the bug, showing what is going to
change...

https://codereview.chromium.org/1432573002/diff/1/chrome/android/java/strings...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/1432573002/diff/1/chrome/android/java/strings...
chrome/android/java/strings/android_chrome_strings.grd:526: <message
name="IDS_WEBSITE_SETTINGS_ADD_SITE_DESCRIPTION_KEYGEN" desc="The description
for the allow Keygen for website dialog.">
I'll go a step further and say that Keygen isn't something users will understand
(I certainly don't know what it refers to), so I'm not sure what value it has
for users to be able to configure this. 

In any case, you should run your design by rolfe@ who oversaw the Content
Setting UX overhaul on Chrome for Android.

[svaldez, 2015-11-05 17:12:42]

On 2015/11/05 15:42:53, Finnur wrote:
> Yes, I am one of probably two persons who is most intimately familiar with
this
> code in its current form (having overhauled/written it in large parts). You'll
> still need an OWNERS from newt@, though.
> 
> One thing to keep in mind also is that the Site Settings categories and
> individual permissions under Site Details are ordered by most-sensitive ones
> first, and something like this is probably low on that list, so it should
appear
> near the bottom. Rebecca (rolfe@) can help you determine where. 
> 
> But overall I'm surprised we'd add this as a first-class category under Site
> Settings (midi, for example, isn't), because this seems obscure and hard to
> explain to users what it does.
> 
> I suspect your goal is just to get the Keygen permission to appear only under
> All Sites and under Site Details for a given origin and for it there to be
> toggleable and clearable. A separate category for Keygen seems like overkill
to
> me. If that intuition is correct then some of the changes proposed are
> redundant.
> 
> Maybe we should start by clearing up what the goal is?

I'm not too sure about how content settings work on Android.

The goal is to have UI for the user to selectively enable/disable the use of
keygen on a particular site. The usage of keygen is low enough that we don't
particularly need to have this be a first-class setting on Android, but it
should still be possible for a user to change this setting through some path.

> 
>
https://codereview.chromium.org/1432573002/diff/1/chrome/android/java/src/org...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java
> (right):
> 
>
https://codereview.chromium.org/1432573002/diff/1/chrome/android/java/src/org...
>
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java:73:
> public static final String PREF_KEYGEN_PERMISSION = "keygen_permission_list";
> I am confused. Is this category a toggle-only category or will it show
websites
> below the toggle that you can change?
> 
> If former: Why are we modifying this file?
> 
> If it is the latter: Why is there no corresponding keygen_permission_list in
> this file:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/jav...
> 
> Because if that doesn't exist, the change won't show the Keygen permission in
> the Site Details page (how Keygen is set for a particular site).
> 
> Wait... lets take a step back here. This file is also not showing any changes:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/jav...
> 
> How do you access this new Keygen category in the UI?? At this point I think
it
> would be prudent to upload some screenshots to the bug, showing what is going
to
> change...
> 
>
https://codereview.chromium.org/1432573002/diff/1/chrome/android/java/strings...
> File chrome/android/java/strings/android_chrome_strings.grd (right):
> 
>
https://codereview.chromium.org/1432573002/diff/1/chrome/android/java/strings...
> chrome/android/java/strings/android_chrome_strings.grd:526: <message
> name="IDS_WEBSITE_SETTINGS_ADD_SITE_DESCRIPTION_KEYGEN" desc="The description
> for the allow Keygen for website dialog.">
> I'll go a step further and say that Keygen isn't something users will
understand
> (I certainly don't know what it refers to), so I'm not sure what value it has
> for users to be able to configure this. 
> 
> In any case, you should run your design by rolfe@ who oversaw the Content
> Setting UX overhaul on Chrome for Android.

[svaldez, 2015-11-05 17:40:52]

https://codereview.chromium.org/1432573002/diff/1/chrome/android/java/strings...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/1432573002/diff/1/chrome/android/java/strings...
chrome/android/java/strings/android_chrome_strings.grd:526: <message
name="IDS_WEBSITE_SETTINGS_ADD_SITE_DESCRIPTION_KEYGEN" desc="The description
for the allow Keygen for website dialog.">
On 2015/11/05 11:21:31, Bernhard Bauer wrote:
> Same comment as in https://codereview.chromium.org/1412523003/: You should
> explain what "Keygen" is, otherwise it won't be possible to translate these.

Done.

[Finnur, 2015-11-05 20:44:41]

> The goal is to have UI for the user to selectively enable/disable 
> the use of keygen on a particular site. The usage of keygen is low
> enough that we don't particularly need to have this be a first-class
> setting on Android, but it should still be possible for a user to 
> change this setting through some path.

Excellent. You should model this after the MIDI permission then, and note where
code has _not_ been added for MIDI. I've added some pointers in comments.

The MIDI permission is only discoverable through All Sites (or via the Page Info
bubble), which is what it sounds like you are after.

https://codereview.chromium.org/1432573002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java
(right):

https://codereview.chromium.org/1432573002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:439:
return R.drawable.permission_javascript;
Same image as javascript? Please run that by Rebecca.

https://codereview.chromium.org/1432573002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ContentSettingsResources.java
(right):

https://codereview.chromium.org/1432573002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ContentSettingsResources.java:119:
case ContentSettingsType.CONTENT_SETTINGS_TYPE_KEYGEN:
If you only want to show the permission in site details, then the global default
enabled value does not need to be specified (much like MIDI).

https://codereview.chromium.org/1432573002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ContentSettingsResources.java:145:
case ContentSettingsType.CONTENT_SETTINGS_TYPE_KEYGEN:
Ditto.

https://codereview.chromium.org/1432573002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ContentSettingsResources.java:226:
case ContentSettingsType.CONTENT_SETTINGS_TYPE_KEYGEN:
Ditto (I think)...

https://codereview.chromium.org/1432573002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java
(right):

https://codereview.chromium.org/1432573002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java:73:
public static final String PREF_KEYGEN_PERMISSION = "keygen_permission_list";
You definitely need the xml changes I asked about to show the permission on the
site details page.

https://codereview.chromium.org/1432573002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SiteSettingsCategory.java
(right):

https://codereview.chromium.org/1432573002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SiteSettingsCategory.java:281:
if (showKeygenSites()) return prefs.keygenManaged();
I don't think you need any of these changes if you just want to show the
permission on the site details page.

https://codereview.chromium.org/1432573002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SiteSettingsPreferences.java
(right):

https://codereview.chromium.org/1432573002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SiteSettingsPreferences.java:111:
checked = PrefServiceBridge.getInstance().keygenEnabled();
I don't think you need any of these changes either.

https://codereview.chromium.org/1432573002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePermissionsFetcher.java
(right):

https://codereview.chromium.org/1432573002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePermissionsFetcher.java:131:
queue.add(new KeygenExceptionInfoFetcher());
This is only required if you have a specific first-class citizen category. Note
that Midi doesn't need it.

And then the question becomes: Do you need the showKeygenSites function?

https://codereview.chromium.org/1432573002/diff/20001/chrome/android/java/str...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/1432573002/diff/20001/chrome/android/java/str...
chrome/android/java/strings/android_chrome_strings.grd:528: </message>
Ummmm... where are these two strings used?

[jochen (gone - plz use gerrit), 2015-11-06 05:43:12]

jochen@chromium.org changed reviewers:
- jochen@chromium.org

[jochen (gone - plz use gerrit), 2015-11-06 05:43:13]

i'm not a good reviewer for any of this code, removing myself

[svaldez, 2015-11-10 15:25:02]

Fixed up, however we aren't currently getting the fact that Blink has rendered a
keygen element piped through, so we don't know when to display the keygen
setting, without there already being a content setting exception for this page.

Looking into how to implement this.

https://codereview.chromium.org/1432573002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java
(right):

https://codereview.chromium.org/1432573002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:439:
return R.drawable.permission_javascript;
On 2015/11/05 20:44:40, Finnur wrote:
> Same image as javascript? Please run that by Rebecca.

Acknowledged.

https://codereview.chromium.org/1432573002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ContentSettingsResources.java
(right):

https://codereview.chromium.org/1432573002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ContentSettingsResources.java:119:
case ContentSettingsType.CONTENT_SETTINGS_TYPE_KEYGEN:
On 2015/11/05 20:44:41, Finnur wrote:
> If you only want to show the permission in site details, then the global
default
> enabled value does not need to be specified (much like MIDI).

Done.

https://codereview.chromium.org/1432573002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ContentSettingsResources.java:145:
case ContentSettingsType.CONTENT_SETTINGS_TYPE_KEYGEN:
On 2015/11/05 20:44:41, Finnur wrote:
> Ditto.

Done.

https://codereview.chromium.org/1432573002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ContentSettingsResources.java:226:
case ContentSettingsType.CONTENT_SETTINGS_TYPE_KEYGEN:
On 2015/11/05 20:44:41, Finnur wrote:
> Ditto (I think)...

Done.

https://codereview.chromium.org/1432573002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java
(right):

https://codereview.chromium.org/1432573002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java:73:
public static final String PREF_KEYGEN_PERMISSION = "keygen_permission_list";
On 2015/11/05 20:44:41, Finnur wrote:
> You definitely need the xml changes I asked about to show the permission on
the
> site details page.

Done.

https://codereview.chromium.org/1432573002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SiteSettingsCategory.java
(right):

https://codereview.chromium.org/1432573002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SiteSettingsCategory.java:281:
if (showKeygenSites()) return prefs.keygenManaged();
On 2015/11/05 20:44:41, Finnur wrote:
> I don't think you need any of these changes if you just want to show the
> permission on the site details page.

Done.

https://codereview.chromium.org/1432573002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SiteSettingsPreferences.java
(right):

https://codereview.chromium.org/1432573002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SiteSettingsPreferences.java:111:
checked = PrefServiceBridge.getInstance().keygenEnabled();
On 2015/11/05 20:44:41, Finnur wrote:
> I don't think you need any of these changes either.

Done.

https://codereview.chromium.org/1432573002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePermissionsFetcher.java
(right):

https://codereview.chromium.org/1432573002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePermissionsFetcher.java:131:
queue.add(new KeygenExceptionInfoFetcher());
On 2015/11/05 20:44:41, Finnur wrote:
> This is only required if you have a specific first-class citizen category.
Note
> that Midi doesn't need it.
> 
> And then the question becomes: Do you need the showKeygenSites function?

Done.

https://codereview.chromium.org/1432573002/diff/20001/chrome/android/java/str...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/1432573002/diff/20001/chrome/android/java/str...
chrome/android/java/strings/android_chrome_strings.grd:528: </message>
On 2015/11/05 20:44:41, Finnur wrote:
> Ummmm... where are these two strings used?

First one is used by ContentSettingsResource.

[Finnur, 2015-11-11 10:25:45]

> Fixed up, however we aren't currently getting the
> fact that Blink has rendered a keygen element piped
> through, so we don't know when to display the keygen
> setting, without there already being a content setting 
> exception for this page.

I'm not sure I follow... The whole SiteSetting category and code "below" that
work on the premise that there is a content setting exception to display and
manipulate. If the exception is there, it lists it. Otherwise, not. What makes
this different and what are the implications?

But ignoring that for a moment, I think we're getting much closer. Once the
issue above has been addressed I think it is time to think about writing a test.
It would be great to model your test on a pre-existing Midi test, since that's
closest to what you are implementing, but I suspect Midi never got enough
attention to get to that level (to have a test, I think it was kind of orphaned
before I started working on Chrome for Android). However, there are a bunch of
tests in SiteSettingsPreferencesTest.java, for example, which can serve as an
inspiration, but perhaps it is best to create a new file, e.g.
SiteSettingsSiteDetailTest.java (or something) for these "second class"
permission-citizens.

https://codereview.chromium.org/1432573002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ContentSettingsResources.java
(right):

https://codereview.chromium.org/1432573002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ContentSettingsResources.java:62:
return R.string.keygen_permission_title;
This is not required by MIDI, so it should not be required of Keygen.

https://codereview.chromium.org/1432573002/diff/40001/chrome/android/java/str...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/1432573002/diff/40001/chrome/android/java/str...
chrome/android/java/strings/android_chrome_strings.grd:499: <message
name="IDS_KEYGEN_PERMISSION_TITLE" desc="Title of the permission to run key
generation in a form [CHAR-LIMIT=32]">
I don't think this is enough context for a translator to figure out how to
translate this into other languages.

https://codereview.chromium.org/1432573002/diff/40001/chrome/android/java/str...
chrome/android/java/strings/android_chrome_strings.grd:500: Key Generation
I'd run this wording by Rebecca as well as the image. I suspect users will find
it a bit cryptic.

https://codereview.chromium.org/1432573002/diff/40001/chrome/browser/ui/andro...
File chrome/browser/ui/android/website_settings_popup_android.cc (right):

https://codereview.chromium.org/1432573002/diff/40001/chrome/browser/ui/andro...
chrome/browser/ui/android/website_settings_popup_android.cc:105:
permissions_to_display.push_back(CONTENT_SETTINGS_TYPE_KEYGEN);
I don't quite understand how this list work and why it is only a subset of the
first-class types that we show...? For example, where are Cookies, Popups,
Fullscreen, and Protected Content?

Images are also listed here, even though they are not shown under Site Settings.

What is the deal?

[svaldez, 2015-11-19 17:50:25]

https://codereview.chromium.org/1432573002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ContentSettingsResources.java
(right):

https://codereview.chromium.org/1432573002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ContentSettingsResources.java:62:
return R.string.keygen_permission_title;
On 2015/11/11 10:25:45, Finnur wrote:
> This is not required by MIDI, so it should not be required of Keygen.

Done.

https://codereview.chromium.org/1432573002/diff/40001/chrome/android/java/str...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/1432573002/diff/40001/chrome/android/java/str...
chrome/android/java/strings/android_chrome_strings.grd:499: <message
name="IDS_KEYGEN_PERMISSION_TITLE" desc="Title of the permission to run key
generation in a form [CHAR-LIMIT=32]">
On 2015/11/11 10:25:45, Finnur wrote:
> I don't think this is enough context for a translator to figure out how to
> translate this into other languages.

Acknowledged.

https://codereview.chromium.org/1432573002/diff/40001/chrome/android/java/str...
chrome/android/java/strings/android_chrome_strings.grd:500: Key Generation
On 2015/11/11 10:25:45, Finnur wrote:
> I'd run this wording by Rebecca as well as the image. I suspect users will
find
> it a bit cryptic.

I think the plan is to ping UI folks to check over the strings/images for both
this CL and the non-Android UI CL.

https://codereview.chromium.org/1432573002/diff/40001/chrome/browser/ui/andro...
File chrome/browser/ui/android/website_settings_popup_android.cc (right):

https://codereview.chromium.org/1432573002/diff/40001/chrome/browser/ui/andro...
chrome/browser/ui/android/website_settings_popup_android.cc:105:
permissions_to_display.push_back(CONTENT_SETTINGS_TYPE_KEYGEN);
On 2015/11/11 10:25:45, Finnur wrote:
> I don't quite understand how this list work and why it is only a subset of the
> first-class types that we show...? For example, where are Cookies, Popups,
> Fullscreen, and Protected Content?
> 
> Images are also listed here, even though they are not shown under Site
Settings.
> 
> What is the deal?

I think this is a slightly different UI than the Site Settings window? I don't
think KEYGEN needs to be implemented here.

[svaldez, 2015-12-01 20:11:32]

On 2015/11/19 17:50:25, svaldez wrote:
>
https://codereview.chromium.org/1432573002/diff/40001/chrome/android/java/src...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ContentSettingsResources.java
> (right):
> 
>
https://codereview.chromium.org/1432573002/diff/40001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ContentSettingsResources.java:62:
> return R.string.keygen_permission_title;
> On 2015/11/11 10:25:45, Finnur wrote:
> > This is not required by MIDI, so it should not be required of Keygen.
> 
> Done.
> 
>
https://codereview.chromium.org/1432573002/diff/40001/chrome/android/java/str...
> File chrome/android/java/strings/android_chrome_strings.grd (right):
> 
>
https://codereview.chromium.org/1432573002/diff/40001/chrome/android/java/str...
> chrome/android/java/strings/android_chrome_strings.grd:499: <message
> name="IDS_KEYGEN_PERMISSION_TITLE" desc="Title of the permission to run key
> generation in a form [CHAR-LIMIT=32]">
> On 2015/11/11 10:25:45, Finnur wrote:
> > I don't think this is enough context for a translator to figure out how to
> > translate this into other languages.
> 
> Acknowledged.
> 
>
https://codereview.chromium.org/1432573002/diff/40001/chrome/android/java/str...
> chrome/android/java/strings/android_chrome_strings.grd:500: Key Generation
> On 2015/11/11 10:25:45, Finnur wrote:
> > I'd run this wording by Rebecca as well as the image. I suspect users will
> find
> > it a bit cryptic.
> 
> I think the plan is to ping UI folks to check over the strings/images for both
> this CL and the non-Android UI CL.
> 
>
https://codereview.chromium.org/1432573002/diff/40001/chrome/browser/ui/andro...
> File chrome/browser/ui/android/website_settings_popup_android.cc (right):
> 
>
https://codereview.chromium.org/1432573002/diff/40001/chrome/browser/ui/andro...
> chrome/browser/ui/android/website_settings_popup_android.cc:105:
> permissions_to_display.push_back(CONTENT_SETTINGS_TYPE_KEYGEN);
> On 2015/11/11 10:25:45, Finnur wrote:
> > I don't quite understand how this list work and why it is only a subset of
the
> > first-class types that we show...? For example, where are Cookies, Popups,
> > Fullscreen, and Protected Content?
> > 
> > Images are also listed here, even though they are not shown under Site
> Settings.
> > 
> > What is the deal?
> 
> I think this is a slightly different UI than the Site Settings window? I don't
> think KEYGEN needs to be implemented here.

Changes for review:
- Creating a Keygen "exception" whenever Keygen is used and blocked.

Since the current Android UI only shows a setting if it has an exception, the
current approach is to create one whenever <keygen> is blocked. Its fairly
hackish, but the alternative is to pipe through a list of psuedo-exceptions
through to the Android UI, which seems similarly hackish. Thoughts?

[Finnur, 2015-12-01 23:19:57]

Still needs a test, but we're getting to a point where my input is getting less
and less helpful and it would be good to have Newton (newt@) take a look -- he's
an owner for this.

https://codereview.chromium.org/1432573002/diff/40001/chrome/browser/ui/andro...
File chrome/browser/ui/android/website_settings_popup_android.cc (right):

https://codereview.chromium.org/1432573002/diff/40001/chrome/browser/ui/andro...
chrome/browser/ui/android/website_settings_popup_android.cc:105:
permissions_to_display.push_back(CONTENT_SETTINGS_TYPE_KEYGEN);
If I'm not mistaken, this is the gatekeeper for the information that shows when
you click on the lock/page icon for a given site (the origin info bubble). It
would be good to keep that in sync with the site settings UI, so I suspect
keygen *should* be listed here (and UI changes added to show keygen as well).
You should consult egm@ on this.

https://codereview.chromium.org/1432573002/diff/200001/chrome/browser/content...
File chrome/browser/content_settings/tab_specific_content_settings.cc (right):

https://codereview.chromium.org/1432573002/diff/200001/chrome/browser/content...
chrome/browser/content_settings/tab_specific_content_settings.cc:340: }
I believe this is what you are referring to in your review comment earlier. I
agree, it is a bit strange to have the exception injected on a blocked request,
but this is probably something someone with more expertise on content settings
should weigh in on.

[svaldez, 2015-12-02 17:13:13]

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

[svaldez, 2015-12-02 17:13:13]

Started a thread with egm@ to figure out UX details.

What's the *right* way of doing testing here?

[svaldez, 2015-12-02 17:17:48]

On 2015/12/02 17:13:13, svaldez wrote:
> Started a thread with egm@ to figure out UX details.
> 
> What's the *right* way of doing testing here?

All the existing tests seem to depend on the the SiteSettingsCategory UI, which
doesn't exist for Keygen, and there doesn't seem to be an existing intent to
open up the single Site Settings page itself.

[newt (away), 2015-12-02 18:45:54]

Could you provide screenshots of the new UI on the bug?

[svaldez, 2015-12-02 19:11:00]

On 2015/12/02 18:45:54, newt wrote:
> Could you provide screenshots of the new UI on the bug?

Updated the bug with the Site Setting UI.

[Finnur, 2015-12-02 23:16:13]

> All the existing tests seem to depend on the the SiteSettingsCategory UI,
which
> doesn't exist for Keygen, and there doesn't seem to be an existing intent to
> open up the single Site Settings page itself.

Yeah, you'd need to create it, probably in a similar fashion as the category
tests.

[svaldez, 2015-12-11 16:22:32]

Partially added tests. Since the testing requires the content setting to
actually block keygen, but I don't want to submit the keygen blocking until
everything else is ready and approved, this will probably can't be committed
until everything else is.

[Finnur, 2015-12-11 17:53:06]

https://codereview.chromium.org/1432573002/diff/300001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java
(right):

https://codereview.chromium.org/1432573002/diff/300001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java:89:
PREF_PROTECTED_MEDIA_IDENTIFIER_PERMISSION, PREF_PUSH_NOTIFICATIONS_PERMISSION,
Was this done by the git format command? I think this is a net readability loss
and should be reverted.

https://codereview.chromium.org/1432573002/diff/300001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/website/SiteSettingsPreferencesTest.java
(right):

https://codereview.chromium.org/1432573002/diff/300001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/website/SiteSettingsPreferencesTest.java:133:
return (Preferences) getInstrumentation().startActivitySync(intent);
I don't quite understand how this test works because SingleWebsitePreferences
needs extra params to load the right site (EXTRA_SITE or EXTRA_ORIGIN).

https://codereview.chromium.org/1432573002/diff/300001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/website/SiteSettingsPreferencesTest.java:391:
assertEquals("true", runJavaScriptCodeInCurrentTab("didKeygen();"));
Shouldn't this assert check for "false"?

https://codereview.chromium.org/1432573002/diff/300001/chrome/browser/ui/andr...
File chrome/browser/ui/android/website_settings_popup_android.cc (right):

https://codereview.chromium.org/1432573002/diff/300001/chrome/browser/ui/andr...
chrome/browser/ui/android/website_settings_popup_android.cc:109:
permissions_to_display.push_back(CONTENT_SETTINGS_TYPE_KEYGEN);
Don't you need code in the OIB bubble to show this permission?

https://codereview.chromium.org/1432573002/diff/300001/chrome/test/data/andro...
File chrome/test/data/android/keygen.html (right):

https://codereview.chromium.org/1432573002/diff/300001/chrome/test/data/andro...
chrome/test/data/android/keygen.html:13: keygen =
(window.location.search.indexOf('?key=MIIC') == 0)
What's this used for? Should it be removed?

[svaldez, 2015-12-11 18:13:11]

https://codereview.chromium.org/1432573002/diff/300001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java
(right):

https://codereview.chromium.org/1432573002/diff/300001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java:89:
PREF_PROTECTED_MEDIA_IDENTIFIER_PERMISSION, PREF_PUSH_NOTIFICATIONS_PERMISSION,
On 2015/12/11 17:53:05, Finnur wrote:
> Was this done by the git format command? I think this is a net readability
loss
> and should be reverted.

Done.

https://codereview.chromium.org/1432573002/diff/300001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/website/SiteSettingsPreferencesTest.java
(right):

https://codereview.chromium.org/1432573002/diff/300001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/website/SiteSettingsPreferencesTest.java:133:
return (Preferences) getInstrumentation().startActivitySync(intent);
On 2015/12/11 17:53:05, Finnur wrote:
> I don't quite understand how this test works because SingleWebsitePreferences
> needs extra params to load the right site (EXTRA_SITE or EXTRA_ORIGIN).

Those params are created through createFragmentArgsForSite.

https://codereview.chromium.org/1432573002/diff/300001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/website/SiteSettingsPreferencesTest.java:391:
assertEquals("true", runJavaScriptCodeInCurrentTab("didKeygen();"));
On 2015/12/11 17:53:05, Finnur wrote:
> Shouldn't this assert check for "false"?

Done.

https://codereview.chromium.org/1432573002/diff/300001/chrome/browser/ui/andr...
File chrome/browser/ui/android/website_settings_popup_android.cc (right):

https://codereview.chromium.org/1432573002/diff/300001/chrome/browser/ui/andr...
chrome/browser/ui/android/website_settings_popup_android.cc:109:
permissions_to_display.push_back(CONTENT_SETTINGS_TYPE_KEYGEN);
On 2015/12/11 17:53:05, Finnur wrote:
> Don't you need code in the OIB bubble to show this permission?

OIB bubble? This is just for displaying it in the WebsiteSettingsPopup.

https://codereview.chromium.org/1432573002/diff/300001/chrome/test/data/andro...
File chrome/test/data/android/keygen.html (right):

https://codereview.chromium.org/1432573002/diff/300001/chrome/test/data/andro...
chrome/test/data/android/keygen.html:13: keygen =
(window.location.search.indexOf('?key=MIIC') == 0)
On 2015/12/11 17:53:05, Finnur wrote:
> What's this used for? Should it be removed?

This verifies that the keygen element has either been blocked from generating a
key or has created a key. I've added a comment.

[Finnur, 2015-12-11 18:29:29]

https://codereview.chromium.org/1432573002/diff/300001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/website/SiteSettingsPreferencesTest.java
(right):

https://codereview.chromium.org/1432573002/diff/300001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/website/SiteSettingsPreferencesTest.java:133:
return (Preferences) getInstrumentation().startActivitySync(intent);
Ah, right, right. Serves me right for making this review one of my first morning
tasks. :)

https://codereview.chromium.org/1432573002/diff/300001/chrome/test/data/andro...
File chrome/test/data/android/keygen.html (right):

https://codereview.chromium.org/1432573002/diff/300001/chrome/test/data/andro...
chrome/test/data/android/keygen.html:13: keygen =
(window.location.search.indexOf('?key=MIIC') == 0)
My point was that I don't see any code adding/checking for MIIC. Is this for
debugging? Probably shouldn't be checked in then.

[svaldez, 2015-12-11 19:07:43]

On 2015/12/11 18:29:29, Finnur wrote:
>
https://codereview.chromium.org/1432573002/diff/300001/chrome/android/javates...
> File
>
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/website/SiteSettingsPreferencesTest.java
> (right):
> 
>
https://codereview.chromium.org/1432573002/diff/300001/chrome/android/javates...
>
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/website/SiteSettingsPreferencesTest.java:133:
> return (Preferences) getInstrumentation().startActivitySync(intent);
> Ah, right, right. Serves me right for making this review one of my first
morning
> tasks. :)
> 
>
https://codereview.chromium.org/1432573002/diff/300001/chrome/test/data/andro...
> File chrome/test/data/android/keygen.html (right):
> 
>
https://codereview.chromium.org/1432573002/diff/300001/chrome/test/data/andro...
> chrome/test/data/android/keygen.html:13: keygen =
> (window.location.search.indexOf('?key=MIIC') == 0)
> My point was that I don't see any code adding/checking for MIIC. Is this for
> debugging? Probably shouldn't be checked in then.

So <keygen> generates a key on the client and then submits a form value
representing the signed public key. This is checking the first three bytes to
see whether the keygen signature was sent as part of the form submission (MIIC
being base64 of 0x30, 0x82, 0x02). The more rigorous version of this test would
require implementing a javascript ASN.1 parser to parse the form submission,
which seems like overkill for this deprecation.

[Finnur, 2015-12-11 20:08:57]

https://codereview.chromium.org/1432573002/diff/300001/chrome/test/data/andro...
File chrome/test/data/android/keygen.html (right):

https://codereview.chromium.org/1432573002/diff/300001/chrome/test/data/andro...
chrome/test/data/android/keygen.html:13: keygen =
(window.location.search.indexOf('?key=MIIC') == 0)
How this works should probably be documented in a comment, since this is totally
not obvious to a casual reader.

[svaldez, 2015-12-11 20:55:18]

On 2015/12/11 20:08:57, Finnur wrote:
>
https://codereview.chromium.org/1432573002/diff/300001/chrome/test/data/andro...
> File chrome/test/data/android/keygen.html (right):
> 
>
https://codereview.chromium.org/1432573002/diff/300001/chrome/test/data/andro...
> chrome/test/data/android/keygen.html:13: keygen =
> (window.location.search.indexOf('?key=MIIC') == 0)
> How this works should probably be documented in a comment, since this is
totally
> not obvious to a casual reader.
Done.

[Finnur, 2015-12-11 21:06:12]

https://codereview.chromium.org/1432573002/diff/340001/chrome/test/data/andro...
File chrome/test/data/android/keygen.html (right):

https://codereview.chromium.org/1432573002/diff/340001/chrome/test/data/andro...
chrome/test/data/android/keygen.html:14: // We check for base64 'MIIC' to match
the first 3 bytes: 0x30 0x82 0x02
nit: Generally, comments should end with some punctuation.

More importantly, though, this comment doesn't explain to a casual reader how
this check fits in the grand scheme of things. As-is this looks like debugging
code that should be taken out, not something that would cause the test to fail
if taken out.

[svaldez, 2015-12-11 21:21:35]

Updated.

[Finnur, 2015-12-11 22:05:03]

Thanks. Where does this leave us with this CL? You mentioned the test isn't (or
might not be) working. Anything else?

Let me rephrase that...

What is left to do on your part and are you waiting on us (me and/or Newton) for
something?

[svaldez, 2015-12-14 14:46:17]

On 2015/12/11 22:05:03, Finnur wrote:
> Thanks. Where does this leave us with this CL? You mentioned the test isn't
(or
> might not be) working. Anything else?
> 
> Let me rephrase that...
> 
> What is left to do on your part and are you waiting on us (me and/or Newton)
for
> something?

I believe that other than changing patchset dependency so that the test works
here, most of the work here is done, modulo getting this through the launch
process and getting LGTMs on it.

[newt (away), 2015-12-15 00:10:01]

https://codereview.chromium.org/1432573002/diff/420001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePermissionsFetcher.java
(right):

https://codereview.chromium.org/1432573002/diff/420001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePermissionsFetcher.java:60:
// Keygen permissions are per-origin.
this needs to come after MidiInfoFetcher, as per the comment above: "Populate
features from more specific to less specific."

The logic for merging sites depends on these lookups happening in the right
order.

https://codereview.chromium.org/1432573002/diff/420001/chrome/android/java/st...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/1432573002/diff/420001/chrome/android/java/st...
chrome/android/java/strings/android_chrome_strings.grd:497: Key Generation
s/Generation/generation/

Chrome on Android uses "Sentence case" for its title, i.e. only the first letter
of the first word is capitalized.

https://codereview.chromium.org/1432573002/diff/420001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/website/SiteSettingsPreferencesTest.java
(right):

https://codereview.chromium.org/1432573002/diff/420001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/website/SiteSettingsPreferencesTest.java:128:
TestHttpServerClient.getUrl(""));
Why not use a real page here? Seems like TestHttpServerClient.getUrl("") could
reasonably throw an error or otherwise misbehave; I wouldn't count on
TestHttpServerClient.getUrl("") returning anything meaningful.

https://codereview.chromium.org/1432573002/diff/420001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/website/SiteSettingsPreferencesTest.java:275:
ChromeSwitchPreference keygen = (ChromeSwitchPreference)
Doesn't this cast fail? I thought PREF_KEYGEN_PERMISSION was a ListPreference.

https://codereview.chromium.org/1432573002/diff/420001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/website/SiteSettingsPreferencesTest.java:278:
websitePreferences.onPreferenceChange(keygen, (enabled ? "ALLOW" : "BLOCK"));
I'd simulate clicking the switch, rather than calling
websitePreferences.onPreferenceChange() directly:

  keygen.setValue(enabled ? "ALLOW" : "BLOCK");

That'll better simulate what actually happens when the user clicks on the
ListPreference. (To really simulate it, you'd have to open the list preference
dialog; but I wouldn't worry about that here.)

https://codereview.chromium.org/1432573002/diff/420001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/website/SiteSettingsPreferencesTest.java:386:
public void testKeygenBlocked() throws Exception {
Alternatively, you could just test the Preferences UI here. e.g. Ensure that
changing the keygen preference to "BLOCKED" actually updates the keygen content
setting in C++.

Then, in separate, cross-platform tests, you could test that the <keygen>
element actually respects the stored content settings value.

https://codereview.chromium.org/1432573002/diff/420001/chrome/browser/android...
File chrome/browser/android/preferences/website_preference_bridge.cc (right):

https://codereview.chromium.org/1432573002/diff/420001/chrome/browser/android...
chrome/browser/android/preferences/website_preference_bridge.cc:253:
ContentSettingsPattern::Wildcard(),
why not "ContentSettingsPattern::FromURLNoWildcard(embedder_url)" ?

[newt (away), 2015-12-15 00:11:45]

Also, please add some detail to the commit message.

[svaldez, 2015-12-15 15:30:04]

Description was changed from

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

Adding the KEYGEN content setting Android UI.

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

to

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

This adds Android UI to allow the user to change the Content Setting to
block/allow keygen usage on a per-site basis. 

To trigger the exception, we also create a Content Exception on any site that
uses a <keygen> element.

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

[svaldez, 2015-12-15 21:45:57]

Fixed modulo testing fixes.

https://codereview.chromium.org/1432573002/diff/420001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePermissionsFetcher.java
(right):

https://codereview.chromium.org/1432573002/diff/420001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePermissionsFetcher.java:60:
// Keygen permissions are per-origin.
On 2015/12/15 00:10:01, newt wrote:
> this needs to come after MidiInfoFetcher, as per the comment above: "Populate
> features from more specific to less specific."
> 
> The logic for merging sites depends on these lookups happening in the right
> order.

Done.

https://codereview.chromium.org/1432573002/diff/420001/chrome/android/java/st...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/1432573002/diff/420001/chrome/android/java/st...
chrome/android/java/strings/android_chrome_strings.grd:497: Key Generation
On 2015/12/15 00:10:01, newt wrote:
> s/Generation/generation/
> 
> Chrome on Android uses "Sentence case" for its title, i.e. only the first
letter
> of the first word is capitalized.

Done.

https://codereview.chromium.org/1432573002/diff/420001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/website/SiteSettingsPreferencesTest.java
(right):

https://codereview.chromium.org/1432573002/diff/420001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/website/SiteSettingsPreferencesTest.java:128:
TestHttpServerClient.getUrl(""));
On 2015/12/15 00:10:01, newt wrote:
> Why not use a real page here? Seems like TestHttpServerClient.getUrl("") could
> reasonably throw an error or otherwise misbehave; I wouldn't count on
> TestHttpServerClient.getUrl("") returning anything meaningful.

Done.

https://codereview.chromium.org/1432573002/diff/420001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/website/SiteSettingsPreferencesTest.java:275:
ChromeSwitchPreference keygen = (ChromeSwitchPreference)
On 2015/12/15 00:10:01, newt wrote:
> Doesn't this cast fail? I thought PREF_KEYGEN_PERMISSION was a ListPreference.

Done.

https://codereview.chromium.org/1432573002/diff/420001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/website/SiteSettingsPreferencesTest.java:278:
websitePreferences.onPreferenceChange(keygen, (enabled ? "ALLOW" : "BLOCK"));
On 2015/12/15 00:10:01, newt wrote:
> I'd simulate clicking the switch, rather than calling
> websitePreferences.onPreferenceChange() directly:
> 
>   keygen.setValue(enabled ? "ALLOW" : "BLOCK");
> 
> That'll better simulate what actually happens when the user clicks on the
> ListPreference. (To really simulate it, you'd have to open the list preference
> dialog; but I wouldn't worry about that here.)

Done.

https://codereview.chromium.org/1432573002/diff/420001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/website/SiteSettingsPreferencesTest.java:386:
public void testKeygenBlocked() throws Exception {
On 2015/12/15 00:10:01, newt wrote:
> Alternatively, you could just test the Preferences UI here. e.g. Ensure that
> changing the keygen preference to "BLOCKED" actually updates the keygen
content
> setting in C++.
> 
> Then, in separate, cross-platform tests, you could test that the <keygen>
> element actually respects the stored content settings value.

Done.

https://codereview.chromium.org/1432573002/diff/420001/chrome/browser/android...
File chrome/browser/android/preferences/website_preference_bridge.cc (right):

https://codereview.chromium.org/1432573002/diff/420001/chrome/browser/android...
chrome/browser/android/preferences/website_preference_bridge.cc:253:
ContentSettingsPattern::Wildcard(),
On 2015/12/15 00:10:01, newt wrote:
> why not "ContentSettingsPattern::FromURLNoWildcard(embedder_url)" ?

Done.

[newt (away), 2015-12-17 21:00:27]

lgtm after question

https://codereview.chromium.org/1432573002/diff/540001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/website/SiteSettingsPreferencesTest.java
(right):

https://codereview.chromium.org/1432573002/diff/540001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/website/SiteSettingsPreferencesTest.java:270:
website.setKeygenInfo(new KeygenInfo(origin, origin, false));
Should you be setting the embedder here, or just the origin?

[svaldez, 2015-12-17 21:07:25]

On 2015/12/17 21:00:27, newt wrote:
> lgtm after question
> 
>
https://codereview.chromium.org/1432573002/diff/540001/chrome/android/javates...
> File
>
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/website/SiteSettingsPreferencesTest.java
> (right):
> 
>
https://codereview.chromium.org/1432573002/diff/540001/chrome/android/javates...
>
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/website/SiteSettingsPreferencesTest.java:270:
> website.setKeygenInfo(new KeygenInfo(origin, origin, false));
> Should you be setting the embedder here, or just the origin?

I believe we need to set both here, since the code assumes both are set,
possibly as a wildcard.

[svaldez, 2015-12-21 15:11:36]

On 2015/12/17 21:07:25, svaldez wrote:
> On 2015/12/17 21:00:27, newt wrote:
> > lgtm after question
> > 
> >
>
https://codereview.chromium.org/1432573002/diff/540001/chrome/android/javates...
> > File
> >
>
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/website/SiteSettingsPreferencesTest.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/1432573002/diff/540001/chrome/android/javates...
> >
>
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/website/SiteSettingsPreferencesTest.java:270:
> > website.setKeygenInfo(new KeygenInfo(origin, origin, false));
> > Should you be setting the embedder here, or just the origin?
> 
> I believe we need to set both here, since the code assumes both are set,
> possibly as a wildcard.

As a result of some updates to the Desktop UI CL, we need to start using
WebContents to get the state of IsContentBlocked from
TabSpecificContentSettings.
I also wasn't able to get simulating the switch click to work in the test, which
I think is why the other tests also use onPreferenceChanged to test it instead.

[svaldez, 2015-12-21 15:15:23]

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

[svaldez, 2015-12-21 15:15:24]

yfriedman: 

Could you take a quick look at the following file, we are just adding the Keygen
Content Setting to it to support the Content Setting on Android:

chrome/browser/ui/android/website_settings_popup_android.cc

[Yaron, 2015-12-21 15:23:26]

lgtm

[svaldez, 2016-01-05 22:29:02]

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

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

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

[commit-bot: I haz the power, 2016-01-06 00:52:29]

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

[commit-bot: I haz the power, 2016-01-06 00:52:30]

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

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

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:18:35]

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

[commit-bot: I haz the power, 2016-01-12 17:21:20]

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

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

Dry run: Try jobs failed on following builders:
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[svaldez, 2016-01-14 21:12:24]

The CQ bit was checked by svaldez@chromium.org

[svaldez, 2016-01-14 21:12:24]

The patchset sent to the CQ was uploaded after l-g-t-m from newt@chromium.org,
yfriedman@chromium.org
Link to the patchset: https://codereview.chromium.org/1432573002/#ps660001
(title: "Fixing GetOrigins.")

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

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

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

This CL has an open dependency (Issue 1412523003 Patch 370001). Please resolve
the dependency and try again.

[svaldez, 2016-01-14 21:13:54]

The CQ bit was checked by svaldez@chromium.org

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

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

[svaldez, 2016-01-14 23:24:09]

The CQ bit was checked by svaldez@chromium.org

[svaldez, 2016-01-14 23:24:09]

The patchset sent to the CQ was uploaded after l-g-t-m from newt@chromium.org,
yfriedman@chromium.org
Link to the patchset: https://codereview.chromium.org/1432573002/#ps700001
(title: "Trying to do after rebase.")

[commit-bot: I haz the power, 2016-01-14 23:24:45]

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

[svaldez, 2016-01-20 16:29:23]

The CQ bit was checked by svaldez@chromium.org

[svaldez, 2016-01-20 16:29:24]

The patchset sent to the CQ was uploaded after l-g-t-m from newt@chromium.org,
yfriedman@chromium.org
Link to the patchset: https://codereview.chromium.org/1432573002/#ps960001
(title: "Trying tested code.")

[commit-bot: I haz the power, 2016-01-20 16:29:36]

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

[commit-bot: I haz the power, 2016-01-20 16:47:59]

Description was changed from

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

This adds Android UI to allow the user to change the Content Setting to
block/allow keygen usage on a per-site basis. 

To trigger the exception, we also create a Content Exception on any site that
uses a <keygen> element.

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

to

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

This adds Android UI to allow the user to change the Content Setting to
block/allow keygen usage on a per-site basis. 

To trigger the exception, we also create a Content Exception on any site that
uses a <keygen> element.

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

[commit-bot: I haz the power, 2016-01-20 16:48:00]

Committed patchset #49 (id:960001)

[commit-bot: I haz the power, 2016-01-20 16:49:15]

Description was changed from

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

This adds Android UI to allow the user to change the Content Setting to
block/allow keygen usage on a per-site basis. 

To trigger the exception, we also create a Content Exception on any site that
uses a <keygen> element.

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

to

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

This adds Android UI to allow the user to change the Content Setting to
block/allow keygen usage on a per-site basis.

To trigger the exception, we also create a Content Exception on any site that
uses a <keygen> element.

BUG=514767

Committed: https://crrev.com/34a7682cd49f8fdfb6a0f715a6118076f9ad8224
Cr-Commit-Position: refs/heads/master@{#370416}
==========

[commit-bot: I haz the power, 2016-01-20 16:49:16]

Patchset 49 (id:??) landed as
https://crrev.com/34a7682cd49f8fdfb6a0f715a6118076f9ad8224
Cr-Commit-Position: refs/heads/master@{#370416}