[subresource_filter] Always display the default setting in Android Page Info

[subresource_filter] Always display the default setting in Android Page Info

This patch also slightly reorders the setting to align with the ordering
defined in crbug.com/610358

BUG=689487
Review-Url: https://codereview.chromium.org/2874073002
Cr-Commit-Position: refs/heads/master@{#471963}
Committed: https://chromium.googlesource.com/chromium/src/+/d516a06227530ca47ccf5f872b334326e0137221

[Charlie Harrison, 2017-05-10 20:16:10]

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

[Charlie Harrison, 2017-05-10 20:16:36]

Description was changed from

==========
[subresource_filter] Display the default setting in Android Page Info

BUG=689487
==========

to

==========
[subresource_filter] Display the default setting in Android Page Info

This patch also slightly reorders the setting to align with the ordering
defined in crbug.com/610358

BUG=689487
==========

[Charlie Harrison, 2017-05-10 20:17:45]

Description was changed from

==========
[subresource_filter] Display the default setting in Android Page Info

This patch also slightly reorders the setting to align with the ordering
defined in crbug.com/610358

BUG=689487
==========

to

==========
[subresource_filter] Always display the default setting in Android Page Info

This patch also slightly reorders the setting to align with the ordering
defined in crbug.com/610358

BUG=689487
==========

[commit-bot: I haz the power, 2017-05-10 20:17:58]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Charlie Harrison, 2017-05-10 20:19:16]

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

[Charlie Harrison, 2017-05-10 20:19:16]

raymes: PTAL? This should probably land after
https://codereview.chromium.org/2873313002/ but I'm not marking it a dependent
PS to avoid the inevitable patch error babysitting. 

LMK if this isn't the right place to be making this policy decision.

[commit-bot: I haz the power, 2017-05-10 22:16:08]

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

[commit-bot: I haz the power, 2017-05-10 22:16:09]

Dry run: This issue passed the CQ dry run.

[raymes, 2017-05-11 00:09:42]

https://codereview.chromium.org/2874073002/diff/1/chrome/browser/ui/android/p...
File chrome/browser/ui/android/page_info/page_info_popup_android.cc (right):

https://codereview.chromium.org/2874073002/diff/1/chrome/browser/ui/android/p...
chrome/browser/ui/android/page_info/page_info_popup_android.cc:121:
permissions_to_display.push_back(CONTENT_SETTINGS_TYPE_AUTOPLAY);
Hmm, not sure I understand. I don't see autoplay or subresource filter in that
bug?

https://codereview.chromium.org/2874073002/diff/1/chrome/browser/ui/android/p...
chrome/browser/ui/android/page_info/page_info_popup_android.cc:146:
permission.default_setting;
Not quite sure I understand this either? Shouldn't it just show the actual
setting here?

[Charlie Harrison, 2017-05-11 00:15:34]

https://codereview.chromium.org/2874073002/diff/1/chrome/browser/ui/android/p...
File chrome/browser/ui/android/page_info/page_info_popup_android.cc (right):

https://codereview.chromium.org/2874073002/diff/1/chrome/browser/ui/android/p...
chrome/browser/ui/android/page_info/page_info_popup_android.cc:121:
permissions_to_display.push_back(CONTENT_SETTINGS_TYPE_AUTOPLAY);
On 2017/05/11 00:09:42, raymes wrote:
> Hmm, not sure I understand. I don't see autoplay or subresource filter in that
> bug?

Sorry, the bug links to a document with all the relevant settings. I've just
used it as a link because it's peppered throughout the settings UI code already
when ordering comes up.

https://codereview.chromium.org/2874073002/diff/1/chrome/browser/ui/android/p...
chrome/browser/ui/android/page_info/page_info_popup_android.cc:146:
permission.default_setting;
On 2017/05/11 00:09:42, raymes wrote:
> Not quite sure I understand this either? Shouldn't it just show the actual
> setting here?

That's what I'm attempting to do :)

As far as I understand, this branch is taken if permission.setting ==
CONTENT_SETTING_DEFAULT, and permission.default_setting is the "actual" setting.

[raymes, 2017-05-11 00:32:46]

Thanks

https://codereview.chromium.org/2874073002/diff/1/chrome/browser/ui/android/p...
File chrome/browser/ui/android/page_info/page_info_popup_android.cc (right):

https://codereview.chromium.org/2874073002/diff/1/chrome/browser/ui/android/p...
chrome/browser/ui/android/page_info/page_info_popup_android.cc:121:
permissions_to_display.push_back(CONTENT_SETTINGS_TYPE_AUTOPLAY);
On 2017/05/11 00:15:34, Charlie Harrison wrote:
> On 2017/05/11 00:09:42, raymes wrote:
> > Hmm, not sure I understand. I don't see autoplay or subresource filter in
that
> > bug?
> 
> Sorry, the bug links to a document with all the relevant settings. I've just
> used it as a link because it's peppered throughout the settings UI code
already
> when ordering comes up.

Ah I see, thanks.

https://codereview.chromium.org/2874073002/diff/1/chrome/browser/ui/android/p...
chrome/browser/ui/android/page_info/page_info_popup_android.cc:146:
permission.default_setting;
On 2017/05/11 00:15:34, Charlie Harrison wrote:
> On 2017/05/11 00:09:42, raymes wrote:
> > Not quite sure I understand this either? Shouldn't it just show the actual
> > setting here?
> 
> That's what I'm attempting to do :)
> 
> As far as I understand, this branch is taken if permission.setting ==
> CONTENT_SETTING_DEFAULT, and permission.default_setting is the "actual"
setting.

Ah I see - thanks. It's quite subtle and I'm not super familiar with this code.
Maybe we could reorder this code to make it a bit clearer?

if (permission.type == GEOLOCATION) {
  if (permission.setting == DEFAULT)
    ..
  else
    ..
} else if (permission.type == SUBRESOURCE_FILTER) {
  if (permission.setting == DEFAULT)
    ..
  else
    ..
} else {
  // Only show most permissions if they are in a non-default state.
  if (permission.setting != DEFAULT) 
    ..
}

It's more verbose but I think clearer. WDYT?

I'm assuming you've tested that the ShouldShowPermission check applies to the
list passed in here (I haven't looked closely).

[Charlie Harrison, 2017-05-11 15:45:54]

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

[commit-bot: I haz the power, 2017-05-11 15:46:20]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Charlie Harrison, 2017-05-11 15:46:48]

raymes: I ended up refactoring some logic into a separate private method. WDYT?

[commit-bot: I haz the power, 2017-05-11 16:29:00]

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

[commit-bot: I haz the power, 2017-05-11 16:29:01]

Dry run: This issue passed the CQ dry run.

[raymes, 2017-05-11 20:40:21]

non-owner lgtm, thanks for the cleanup. 

Is there a way to test this?

[Charlie Harrison, 2017-05-11 21:22:21]

csharrison@chromium.org changed reviewers:
+ tedchoc@chromium.org

[Charlie Harrison, 2017-05-11 21:22:22]

tedchoc: Could you PTAL as an android ui owner?

raymes: I couldn't find a way to unit test this change without adding some meaty
infrastructure changes but possibly there's a way to spin up the browser and
confirm these UI elements exist? tedchoc can you help advise?

[Ted C, 2017-05-15 20:39:41]

lgtm

[Charlie Harrison, 2017-05-15 23:08:41]

The CQ bit was checked by csharrison@chromium.org

[commit-bot: I haz the power, 2017-05-15 23:10:01]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-16 01:05:27]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1494889721659360,
"parent_rev": "63e2b3c73a2be03b588f73b80f45bb1141133ade", "commit_rev":
"d516a06227530ca47ccf5f872b334326e0137221"}

[commit-bot: I haz the power, 2017-05-16 01:05:39]

Description was changed from

==========
[subresource_filter] Always display the default setting in Android Page Info

This patch also slightly reorders the setting to align with the ordering
defined in crbug.com/610358

BUG=689487
==========

to

==========
[subresource_filter] Always display the default setting in Android Page Info

This patch also slightly reorders the setting to align with the ordering
defined in crbug.com/610358

BUG=689487

Review-Url: https://codereview.chromium.org/2874073002
Cr-Commit-Position: refs/heads/master@{#471963}
Committed:
https://chromium.googlesource.com/chromium/src/+/d516a06227530ca47ccf5f872b33...
==========

[commit-bot: I haz the power, 2017-05-16 01:05:40]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/d516a06227530ca47ccf5f872b33...