Restore the permission selection listing in the Origin Info Bubble.

Restore the permission selection listing in the Origin Info Bubble.

There was going to be a replacement UI, but it never materialized, so restore
the ready-to-hand functionality.

BUG=483899
Committed: https://crrev.com/64a09998a6930a0e32308b243f7c763c6ece758c
Cr-Commit-Position: refs/heads/master@{#380227}

[palmer, 2016-03-03 23:53:23]

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

[palmer, 2016-03-03 23:53:23]

felt: PTAL

[palmer, 2016-03-03 23:53:43]

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

[commit-bot: I haz the power, 2016-03-03 23:54:03]

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

[felt, 2016-03-03 23:58:03]

Description was changed from

==========
Restore the permission selection listing in the Origin Info Bubble.

There was going to be a replacement UI, but it never materialized, so restore
the ready-to-hand functionality.

BUG=483899
==========

to

==========
Restore the permission selection listing in the Origin Info Bubble.

There was going to be a replacement UI, but it never materialized, so restore
the ready-to-hand functionality.

BUG=483899
==========

[felt, 2016-03-03 23:58:03]

felt@chromium.org changed reviewers:
+ lgarron@chromium.org

[felt, 2016-03-03 23:58:31]

lgarron mentioned wanting to do a review. lucas, can you please review first and
then i will review?

[commit-bot: I haz the power, 2016-03-04 00:26:11]

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

[commit-bot: I haz the power, 2016-03-04 00:26:12]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[lgarron, 2016-03-04 01:45:29]

https://codereview.chromium.org/1766493002/diff/1/chrome/browser/ui/website_s...
File chrome/browser/ui/website_settings/website_settings.cc (right):

https://codereview.chromium.org/1766493002/diff/1/chrome/browser/ui/website_s...
chrome/browser/ui/website_settings/website_settings.cc:703:
permission_info.type)) {
This was introduced in [1].

To me, it seems like we should either:
- Also always show this permission.
- Show the permission when it's non-default, as follows:

    if (permission_info.type == CONTENT_SETTINGS_TYPE_KEYGEN &&
           (permission_info.setting == CONTENT_SETTING_DEFAULT ||
            permission_info.setting == permission_info.default_setting)) {
        continue;
    }

Arguably, you could also exclude the `permission_info.setting ==
CONTENT_SETTING_DEFAULT` line.



[1] https://codereview.chromium.org/1412523003

[palmer, 2016-03-04 18:43:01]

https://codereview.chromium.org/1766493002/diff/1/chrome/browser/ui/website_s...
File chrome/browser/ui/website_settings/website_settings.cc (right):

https://codereview.chromium.org/1766493002/diff/1/chrome/browser/ui/website_s...
chrome/browser/ui/website_settings/website_settings.cc:703:
permission_info.type)) {
> - Show the permission when it's non-default, as follows:
> 
>     if (permission_info.type == CONTENT_SETTINGS_TYPE_KEYGEN &&
>            (permission_info.setting == CONTENT_SETTING_DEFAULT ||
>             permission_info.setting == permission_info.default_setting)) {
>         continue;
>     }

Oh yeah, you are totally right, thanks. Doing this.

> Arguably, you could also exclude the `permission_info.setting ==
> CONTENT_SETTING_DEFAULT` line.

I like keeping it for defense in depth against weird values.

[palmer, 2016-03-04 19:02:29]

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

[commit-bot: I haz the power, 2016-03-04 19:02:49]

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

[palmer, 2016-03-04 19:23:36]

The CQ bit was unchecked by palmer@chromium.org

[palmer, 2016-03-04 20:27:07]

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

[commit-bot: I haz the power, 2016-03-04 20:27:26]

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

[commit-bot: I haz the power, 2016-03-04 21:23:52]

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

[commit-bot: I haz the power, 2016-03-04 21:23:53]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[lgarron, 2016-03-04 22:32:55]

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

https://codereview.chromium.org/1766493002/diff/40001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/website_settings.cc:701: if
(permission_info.type == CONTENT_SETTINGS_TYPE_KEYGEN &&
So, after some more testing, it seems that we want to preserve the
tab_specific_content_settings()->IsContentBlocked() condition, too.

In order to preserve the logic, but only apply it to <keygen>, we can move the
<keygen> check to a top-level conjunction.

Let's let
A = permission_info.setting == CONTENT_SETTING_DEFAULT
B = permission_info.setting == permission_info.default_setting
K = permission_info.type == CONTENT_SETTINGS_TYPE_KEYGEN
T = tab_specific_content_settings()->IsContentBlocked(
             permission_info.type

We have:
    if ((!A && !B) || (K && T))) { show }

We want:
    if (K && !((!A && !B) || T)) { continue }

By de Morgan, this is:
    if (K && (A || B) && !T) { continue }

Which is all a roundabout way of saying that adding a
!tab_specific_content_settings()->IsContentBlocked(permission_info.type) to the
conjunction matches the old behaviour for <keygen>.
That makes sense to me; we want to hide the permission only if:
- the permission is <keygen>,
- the permission is default, and
- there is no tab-specific permission (due to a <keygen> element on the page).

Could you add it back in?


    if (permission_info.type == CONTENT_SETTINGS_TYPE_KEYGEN &&
        (permission_info.setting == CONTENT_SETTING_DEFAULT ||
         permission_info.setting == permission_info.default_setting) &&
        !tab_specific_content_settings()->IsContentBlocked(
            permission_info.type)) {
      continue;
    }

[lgarron, 2016-03-04 22:42:05]

Patch 4 LGTM. I just tested it on Linux, and it works for <keygen> on
https://adrifelt.github.io/demos/all-permissions.html

[palmer, 2016-03-05 00:22:58]

Note: The tests fail on Mac because 2 extra permissions are present in the
*tests* on Mac, but not in the UX in the browser:

0 'Location:', 'Ask by default'
1 'Camera:', 'Ask by default'
2 'Microphone:', 'Ask by default'
3 'Notifications:', 'Ask by default'
4 'Images:', 'Allowed by default'
5 'JavaScript:', 'Allowed by default'
6 'Popups:', 'Blocked by default'
7 'Fullscreen:', 'Ask by default'
8 'Automatic Downloads:', 'Ask by default'
9 'Plugins:', 'Detect important content by default'
10 'Mouse Lock:', 'Ask by default'
11 'MIDI devices full control:', 'Ask by default'

The browser does not have Fullscreen or Mouse Lock. So for some reason the tests
differ, and we get failures like:

../../chrome/browser/ui/views/website_settings/website_settings_popup_view_unittest.cc:220:
Failure
Value of: api_->permissions_content()->child_count()
  Actual: 12
Expected: 10

[palmer, 2016-03-05 00:25:17]

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

[palmer, 2016-03-05 00:25:18]

tapted: Any chance you have an idea why the tests fail only on Mac?

[palmer, 2016-03-05 00:32:44]

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

[palmer, 2016-03-05 00:32:45]

Also maybe mgiuca knows?

[palmer, 2016-03-05 00:45:13]

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

[commit-bot: I haz the power, 2016-03-05 00:45:32]

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

[commit-bot: I haz the power, 2016-03-05 01:46:29]

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

[commit-bot: I haz the power, 2016-03-05 01:46:30]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[tapted, 2016-03-06 23:02:08]

https://codereview.chromium.org/1766493002/diff/80001/chrome/browser/ui/views...
File
chrome/browser/ui/views/website_settings/website_settings_popup_view_unittest.cc
(right):

https://codereview.chromium.org/1766493002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/website_settings/website_settings_popup_view_unittest.cc:168:
// Mac OS X still has Fullscreen and Mouse Lock permissions.
This is actually in a fieldtrial on Mac, to catch up to the behaviour on other
platforms, and https://codereview.chromium.org/1709803004/ means that the bots
run with the fieldtrial enabled by default.

Maybe instead of the #ifdef, it should be a runtime check for
ExclusiveAccessManager::IsSimplifiedFullscreenUIEnabled()

or base::FeatureList::IsEnabled(features::kSimplifiedFullscreenUI); (if it's
needed outside of c/b/ui but that also needs mgiuca's
https://codereview.chromium.org/1763653002/ to land which is Android's edition
of kSimplifiedFullscreenUI). This would make it consistent with the
`ShouldShowPermission()` function in website_settings.cc's anonymous namepace.

Anyway, I think IsSimplifiedFullscreenUIEnabled() is the right fix, but it needs
to be applied in WebsiteSettingsPopupViewTest.SetPermissionInfoWithUsbDevice as
well as WebsiteSettingsPopupViewTest.SetPermissionInfo

However, looking at the failures, there does seem to be something else at play.
E.g. I think just using kExpectedChildren in SetPermissionInfoWithUsbDevice
would also "work". I think the problem is that this is a unittest, but
base::FeatureList::IsEnabled() only populates the contents of the fieldtrial
testing config in a browsertest. So
ExclusiveAccessManager::IsSimplifiedFullscreenUIEnabled() will still be
consistent, but it will be false on Mac where really it should be true (as it
would be in a browsertest).

Really there's some code in ChromeBrowserMainParts::SetupMetricsAndFieldTrials()
that should be exposed to unit tests, so that they can trigger a call
chrome_variations::AssociateDefaultFieldTrialConfig to get the same parameters
as a browsertest. But that's probably a yak that doesn't need shaving before
this.

[palmer, 2016-03-08 02:21:47]

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

[commit-bot: I haz the power, 2016-03-08 02:22:28]

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

[commit-bot: I haz the power, 2016-03-08 02:49:32]

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

[commit-bot: I haz the power, 2016-03-08 02:49:33]

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

[palmer, 2016-03-09 20:13:07]

https://codereview.chromium.org/1766493002/diff/80001/chrome/browser/ui/views...
File
chrome/browser/ui/views/website_settings/website_settings_popup_view_unittest.cc
(right):

https://codereview.chromium.org/1766493002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/website_settings/website_settings_popup_view_unittest.cc:168:
// Mac OS X still has Fullscreen and Mouse Lock permissions.
OK, I've changed the tests to use
ExclusiveAccessManager::IsSimplifiedFullscreenUIEnabled(). Seems to work.
Thanks!

[palmer, 2016-03-09 20:14:58]

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

[palmer, 2016-03-09 20:15:12]

felt: LGTY?

[commit-bot: I haz the power, 2016-03-09 20:15:22]

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

[felt, 2016-03-09 20:30:39]

lgtm

[commit-bot: I haz the power, 2016-03-09 21:10:28]

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

[commit-bot: I haz the power, 2016-03-09 21:10:29]

Dry run: This issue passed the CQ dry run.

[palmer, 2016-03-09 21:19:27]

The CQ bit was checked by palmer@chromium.org

[palmer, 2016-03-09 21:19:28]

The patchset sent to the CQ was uploaded after l-g-t-m from lgarron@chromium.org
Link to the patchset: https://codereview.chromium.org/1766493002/#ps120001
(title: "Use ExclusiveAccessManager::IsSimplifiedFullscreenUIEnabled().")

[commit-bot: I haz the power, 2016-03-09 21:19:56]

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

[commit-bot: I haz the power, 2016-03-09 21:26:19]

Description was changed from

==========
Restore the permission selection listing in the Origin Info Bubble.

There was going to be a replacement UI, but it never materialized, so restore
the ready-to-hand functionality.

BUG=483899
==========

to

==========
Restore the permission selection listing in the Origin Info Bubble.

There was going to be a replacement UI, but it never materialized, so restore
the ready-to-hand functionality.

BUG=483899
==========

[commit-bot: I haz the power, 2016-03-09 21:26:20]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-03-09 21:28:00]

Description was changed from

==========
Restore the permission selection listing in the Origin Info Bubble.

There was going to be a replacement UI, but it never materialized, so restore
the ready-to-hand functionality.

BUG=483899
==========

to

==========
Restore the permission selection listing in the Origin Info Bubble.

There was going to be a replacement UI, but it never materialized, so restore
the ready-to-hand functionality.

BUG=483899

Committed: https://crrev.com/64a09998a6930a0e32308b243f7c763c6ece758c
Cr-Commit-Position: refs/heads/master@{#380227}
==========

[commit-bot: I haz the power, 2016-03-09 21:28:02]

Patchset 7 (id:??) landed as
https://crrev.com/64a09998a6930a0e32308b243f7c763c6ece758c
Cr-Commit-Position: refs/heads/master@{#380227}