Use GetPermissionStatus instead of DesktopNotificationProfileUtil::GetContentSetting

Use GetPermissionStatus instead of DesktopNotificationProfileUtil::GetContentSetting

DesktopNotificationProfileUtil::GetContentSetting() will call directly into
HostContentSettingsMap::GetContentSetting() and thus miss some checks in
permission layer.

In this CL DesktopNotificationProfileUtil::GetContentSetting()is removed and all
its call sites are changed to call PermissionManager::GetPermissionStatus().

Committed: https://crrev.com/106c1779974bbe7570713d9503929b1be3e0de62
Cr-Commit-Position: refs/heads/master@{#397960}

[lshang, 2016-05-31 04:39:27]

Description was changed from

==========
Use GetPermissionStatus instead of
DesktopNotificationProfileUtil::GetContentSetting

use GetPermissionStatus

BUG=
==========

to

==========
Use GetPermissionStatus instead of
DesktopNotificationProfileUtil::GetContentSetting

DesktopNotificationProfileUtil::GetContentSetting() will call directly into 
HostContentSettingsMap::GetContentSetting() and thus miss some checks in 
permission layer. 

In this CL DesktopNotificationProfileUtil::GetContentSetting()is removed and all

its call sites are changed to call PermissionManager::GetPermissionStatus().
==========

[lshang, 2016-05-31 06:49:08]

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

[lshang, 2016-05-31 06:49:08]

Hi Raymes, I changed call sites of
DesktopNotificationProfileUtil::GetContentSetting() to
PermissionManager::GetPermissionStatus() as we discussed. 

There is another call site in android's website_preference_bridge.cc which code
search didn't show to me in the beginning. I changed it here, but on second
thoughts I think we'd better leave it there since other places in android all
call into content setting layer. But I'm happy to hear about your thoughts:-)

[raymes, 2016-06-01 07:24:29]

On 2016/05/31 06:49:08, Liu Shang wrote:
> Hi Raymes, I changed call sites of
> DesktopNotificationProfileUtil::GetContentSetting() to
> PermissionManager::GetPermissionStatus() as we discussed. 
> 
> There is another call site in android's website_preference_bridge.cc which
code
> search didn't show to me in the beginning. I changed it here, but on second
> thoughts I think we'd better leave it there since other places in android all
> call into content setting layer. But I'm happy to hear about your thoughts:-)

I agree - let's leave it out of there for now. We'll need to solve the larger
layering problem at some point but this fixes things well enough for now.

lgtm after website_preference_bridge.cc is removed from this CL.

[lshang, 2016-06-02 00:32:03]

lshang@chromium.org changed reviewers:
+ benwells@chromium.org, peter@chromium.org

[lshang, 2016-06-02 00:32:03]

+OWNERs

peter@, please review changes in chrome/browser/notifications

benwells@, please review changes in chrome/browser/extensions

thanks!

[lshang, 2016-06-02 00:47:41]

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

[lshang, 2016-06-02 00:47:42]

and +bauerb@ for chrome/browser/android OWNER review:-)

[lshang, 2016-06-02 00:48:05]

Patchset #2 (id:20001) has been deleted

[lshang, 2016-06-02 00:48:15]

Patchset #1 (id:1) has been deleted

[lshang, 2016-06-02 00:48:35]

Patchset #2 (id:60001) has been deleted

[benwells, 2016-06-02 01:16:17]

c/b/e lgtm

[Bernhard Bauer, 2016-06-02 11:19:02]

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

https://codereview.chromium.org/2020063003/diff/80001/chrome/browser/android/...
chrome/browser/android/preferences/website_preference_bridge.cc:319: origin,
origin, is_incognito);
Is it on purpose that we ignore the embedder and use the origin instead?

[Michael van Ouwerkerk, 2016-06-02 12:03:01]

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

https://codereview.chromium.org/2020063003/diff/80001/chrome/browser/android/...
chrome/browser/android/preferences/website_preference_bridge.cc:319: origin,
origin, is_incognito);
On 2016/06/02 11:19:01, Bernhard Bauer wrote:
> Is it on purpose that we ignore the embedder and use the origin instead?

Yes, that's on purpose: https://codereview.chromium.org/921803002/

[lshang, 2016-06-02 12:05:25]

Thanks Bernhard!

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

https://codereview.chromium.org/2020063003/diff/80001/chrome/browser/android/...
chrome/browser/android/preferences/website_preference_bridge.cc:319: origin,
origin, is_incognito);
On 2016/06/02 11:19:01, Bernhard Bauer wrote:
> Is it on purpose that we ignore the embedder and use the origin instead?

I use origins for both params here mainly because the previous code
DesktopNotificationProfileUtil::GetContentSetting() only uses origins, and I'm
making them consistent before and after the migration to introduce bugs. 

On more thoughts, CONTENT_SETTINGS_TYPE_NOTIFICATIONS is
REQUESTING_ORIGIN_ONLY_SCOPE, and it only uses origin when setting content
settings. So I think notifications only cares about origin.

[Bernhard Bauer, 2016-06-02 12:23:04]

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

https://codereview.chromium.org/2020063003/diff/80001/chrome/browser/android/...
chrome/browser/android/preferences/website_preference_bridge.cc:319: origin,
origin, is_incognito);
On 2016/06/02 12:05:25, Liu Shang wrote:
> On 2016/06/02 11:19:01, Bernhard Bauer wrote:
> > Is it on purpose that we ignore the embedder and use the origin instead?
> 
> I use origins for both params here mainly because the previous code
> DesktopNotificationProfileUtil::GetContentSetting() only uses origins, and I'm
> making them consistent before and after the migration to introduce bugs. 
> 
> On more thoughts, CONTENT_SETTINGS_TYPE_NOTIFICATIONS is
> REQUESTING_ORIGIN_ONLY_SCOPE, and it only uses origin when setting content
> settings. So I think notifications only cares about origin.

Why don't we get rid of the parameter then?

[Peter Beverloo, 2016-06-02 14:15:47]

lgtm

[lshang, 2016-06-03 06:34:48]

On 2016/06/02 12:23:04, Bernhard Bauer wrote:
>
https://codereview.chromium.org/2020063003/diff/80001/chrome/browser/android/...
> File chrome/browser/android/preferences/website_preference_bridge.cc (right):
> 
>
https://codereview.chromium.org/2020063003/diff/80001/chrome/browser/android/...
> chrome/browser/android/preferences/website_preference_bridge.cc:319: origin,
> origin, is_incognito);
> On 2016/06/02 12:05:25, Liu Shang wrote:
> > On 2016/06/02 11:19:01, Bernhard Bauer wrote:
> > > Is it on purpose that we ignore the embedder and use the origin instead?
> > 
> > I use origins for both params here mainly because the previous code
> > DesktopNotificationProfileUtil::GetContentSetting() only uses origins, and
I'm
> > making them consistent before and after the migration to introduce bugs. 
> > 
> > On more thoughts, CONTENT_SETTINGS_TYPE_NOTIFICATIONS is
> > REQUESTING_ORIGIN_ONLY_SCOPE, and it only uses origin when setting content
> > settings. So I think notifications only cares about origin.
> 
> Why don't we get rid of the parameter then?

Done.

[Bernhard Bauer, 2016-06-03 08:03:33]

LGTM, with another possible yak shaving (doesn't have to be in this CL):

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

https://codereview.chromium.org/2020063003/diff/100001/chrome/browser/android...
chrome/browser/android/preferences/website_preference_bridge.cc:325: const
JavaParamRef<jstring>& embedder,
This parameter could also be removed.

[lshang, 2016-06-06 00:29:14]

The CQ bit was checked by lshang@chromium.org

[lshang, 2016-06-06 00:29:15]

The patchset sent to the CQ was uploaded after l-g-t-m from raymes@chromium.org,
peter@chromium.org, benwells@chromium.org
Link to the patchset: https://codereview.chromium.org/2020063003/#ps100001
(title: "remove embber param for notifications")

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

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2020063003/100001

[commit-bot: I haz the power, 2016-06-06 01:43:46]

Description was changed from

==========
Use GetPermissionStatus instead of
DesktopNotificationProfileUtil::GetContentSetting

DesktopNotificationProfileUtil::GetContentSetting() will call directly into 
HostContentSettingsMap::GetContentSetting() and thus miss some checks in 
permission layer. 

In this CL DesktopNotificationProfileUtil::GetContentSetting()is removed and all

its call sites are changed to call PermissionManager::GetPermissionStatus().
==========

to

==========
Use GetPermissionStatus instead of
DesktopNotificationProfileUtil::GetContentSetting

DesktopNotificationProfileUtil::GetContentSetting() will call directly into 
HostContentSettingsMap::GetContentSetting() and thus miss some checks in 
permission layer. 

In this CL DesktopNotificationProfileUtil::GetContentSetting()is removed and all

its call sites are changed to call PermissionManager::GetPermissionStatus().
==========

[commit-bot: I haz the power, 2016-06-06 01:43:47]

Committed patchset #3 (id:100001)

[commit-bot: I haz the power, 2016-06-06 01:45:07]

Description was changed from

==========
Use GetPermissionStatus instead of
DesktopNotificationProfileUtil::GetContentSetting

DesktopNotificationProfileUtil::GetContentSetting() will call directly into 
HostContentSettingsMap::GetContentSetting() and thus miss some checks in 
permission layer. 

In this CL DesktopNotificationProfileUtil::GetContentSetting()is removed and all

its call sites are changed to call PermissionManager::GetPermissionStatus().
==========

to

==========
Use GetPermissionStatus instead of
DesktopNotificationProfileUtil::GetContentSetting

DesktopNotificationProfileUtil::GetContentSetting() will call directly into
HostContentSettingsMap::GetContentSetting() and thus miss some checks in
permission layer.

In this CL DesktopNotificationProfileUtil::GetContentSetting()is removed and all
its call sites are changed to call PermissionManager::GetPermissionStatus().

Committed: https://crrev.com/106c1779974bbe7570713d9503929b1be3e0de62
Cr-Commit-Position: refs/heads/master@{#397960}
==========

[commit-bot: I haz the power, 2016-06-06 01:45:08]

Patchset 3 (id:??) landed as
https://crrev.com/106c1779974bbe7570713d9503929b1be3e0de62
Cr-Commit-Position: refs/heads/master@{#397960}