Use GURLs instead of patterns for SetContentSetting() in WebsitePreferenceBridge

Use GURLs instead of patterns for SetContentSetting() in WebsitePreferenceBridge

Migrate SetContentSetting() in WebsitePreferenceBridge to take GURLs instead of
patterns.

BUG=551747
Committed: https://crrev.com/f85700f54fe76ffb38e40629d9b8109745fe3c33
Cr-Commit-Position: refs/heads/master@{#385121}

[lshang, 2016-03-21 22:59:18]

Description was changed from

==========
Use GURLs instead of patterns for SetContentSetting() in WebsitePreferenceBridge


change to use setcontentsettingdefaultscope

BUG=551747
==========

to

==========
Use GURLs instead of patterns for SetContentSetting() in WebsitePreferenceBridge

Migrate SetContentSetting() in WebsitePreferenceBridge to take GURLs instead of 
patterns.

BUG=551747
==========

[lshang, 2016-03-21 23:36:19]

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

[lshang, 2016-03-21 23:36:20]

Hi Raymes, PTAL thanks!

One thing needs to be noticed is that for keygen it's originally using both the
primary pattern and secondary pattern. After we change it to use
SetContentSettingDefaultScope(), its secondary pattern will be wildcard(). I'm
not very sure we could do this, or do we need to migrate its old settings?

[raymes, 2016-03-22 06:00:47]

lgtm

[lshang, 2016-03-22 06:45:25]

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

[lshang, 2016-03-22 06:45:26]

PTAL thanks!

[Bernhard Bauer, 2016-03-22 10:17:34]

On 2016/03/21 23:36:20, Liu Shang wrote:
> Hi Raymes, PTAL thanks!
> 
> One thing needs to be noticed is that for keygen it's originally using both
the
> primary pattern and secondary pattern. After we change it to use
> SetContentSettingDefaultScope(), its secondary pattern will be wildcard(). I'm
> not very sure we could do this, or do we need to migrate its old settings?

+rsleevi, who added the keygen content setting. LGTM from my side.

[Bernhard Bauer, 2016-03-22 10:18:05]

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

[Bernhard Bauer, 2016-03-22 10:18:06]

Grr, actually +rsleevi this time

[Ryan Sleevi, 2016-03-22 15:51:06]

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

[Ryan Sleevi, 2016-03-22 15:51:06]

svaldez added it - punting to him.

[svaldez, 2016-03-22 16:05:24]

Not entirely sure why its NULL here. But it should be fine to migrate to using
wildcard for the embedder.

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

https://codereview.chromium.org/1819773002/diff/20001/chrome/browser/android/...
chrome/browser/android/preferences/website_preference_bridge.cc:228:
SetSettingForOrigin(env, CONTENT_SETTINGS_TYPE_KEYGEN, origin, NULL,
Should this be NULL, possibly add a comment?

[Bernhard Bauer, 2016-03-22 16:11:58]

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

https://codereview.chromium.org/1819773002/diff/20001/chrome/browser/android/...
chrome/browser/android/preferences/website_preference_bridge.cc:228:
SetSettingForOrigin(env, CONTENT_SETTINGS_TYPE_KEYGEN, origin, NULL,
On 2016/03/22 16:05:24, svaldez wrote:
> Should this be NULL, possibly add a comment?

And in any case it should be nullptr, not NULL.

https://codereview.chromium.org/1819773002/diff/20001/chrome/browser/android/...
chrome/browser/android/preferences/website_preference_bridge.cc:229:
(ContentSetting)value, is_incognito);
Ooh, also we should use static_cast<ContentSetting>(). Sorry for missing that!

[lshang, 2016-03-23 01:46:12]

svaldez@, bauerb@, thanks for your review! Can you take a look again?

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

https://codereview.chromium.org/1819773002/diff/20001/chrome/browser/android/...
chrome/browser/android/preferences/website_preference_bridge.cc:228:
SetSettingForOrigin(env, CONTENT_SETTINGS_TYPE_KEYGEN, origin, NULL,
On 2016/03/22 16:11:58, Bernhard Bauer wrote:
> On 2016/03/22 16:05:24, svaldez wrote:
> > Should this be NULL, possibly add a comment?
> 
> And in any case it should be nullptr, not NULL.

Done.

https://codereview.chromium.org/1819773002/diff/20001/chrome/browser/android/...
chrome/browser/android/preferences/website_preference_bridge.cc:228:
SetSettingForOrigin(env, CONTENT_SETTINGS_TYPE_KEYGEN, origin, NULL,
On 2016/03/22 16:05:24, svaldez wrote:
> Should this be NULL, possibly add a comment?

Done. I changed it to nullptr, and it indicates that wildcard is used for
embedder. 

If it looks weird to you, I can change it back to use embedder. For keygen,
microphone and camera, implicitly SetContentSettingDefaultScope() will ignore
the embedder and directly use wildcard for secondary_pattern in either ways. I
think nullptr makes this point more clear to developers, but I'm happy to accept
either way you like. :-)

https://codereview.chromium.org/1819773002/diff/20001/chrome/browser/android/...
chrome/browser/android/preferences/website_preference_bridge.cc:229:
(ContentSetting)value, is_incognito);
On 2016/03/22 16:11:58, Bernhard Bauer wrote:
> Ooh, also we should use static_cast<ContentSetting>(). Sorry for missing that!

Done.

[lshang, 2016-03-23 13:52:39]

Patchset #7 (id:120001) has been deleted

[lshang, 2016-03-23 13:52:57]

Patchset #6 (id:100001) has been deleted

[lshang, 2016-03-23 13:53:07]

Patchset #4 (id:60001) has been deleted

[lshang, 2016-03-23 13:53:24]

Patchset #4 (id:80001) has been deleted

[lshang, 2016-03-23 13:57:26]

On 2016/03/23 01:46:12, Liu Shang wrote:
> svaldez@, bauerb@, thanks for your review! Can you take a look again?
> 
And migration code and test for keygen are also added to change old setting's
secondary pattern to be wildcard(). This can prevent introducing bugs.

[Bernhard Bauer, 2016-03-23 14:12:15]

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

https://codereview.chromium.org/1819773002/diff/20001/chrome/browser/android/...
chrome/browser/android/preferences/website_preference_bridge.cc:228:
SetSettingForOrigin(env, CONTENT_SETTINGS_TYPE_KEYGEN, origin, NULL,
On 2016/03/23 01:46:12, Liu Shang wrote:
> On 2016/03/22 16:05:24, svaldez wrote:
> > Should this be NULL, possibly add a comment?
> 
> Done. I changed it to nullptr, and it indicates that wildcard is used for
> embedder. 
> 
> If it looks weird to you, I can change it back to use embedder. For keygen,
> microphone and camera, implicitly SetContentSettingDefaultScope() will ignore
> the embedder and directly use wildcard for secondary_pattern in either ways. I
> think nullptr makes this point more clear to developers, but I'm happy to
accept
> either way you like. :-)

The biggest issue I have with nullptr specifically is that the argument will be
converted to a std::string, and passing in null to ConvertJavaStringToUTF8()
isn't actually allowed, it's just that the code silently accepts it (see
https://code.google.com/p/chromium/codesearch#chromium/src/base/android/jni_s...).
So, we should probably guard that call by checking for null in
SetSettingForOrigin().

Also, can we get rid of the |embedder| parameter here if it's going to be
ignored anyway?

[svaldez, 2016-03-23 14:41:53]

Migration code and keygen-specific bits lgtm.

[lshang, 2016-03-24 00:19:36]

> The biggest issue I have with nullptr specifically is that the argument will
be
> converted to a std::string, and passing in null to ConvertJavaStringToUTF8()
> isn't actually allowed, it's just that the code silently accepts it (see
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/android/jni_s...).
> So, we should probably guard that call by checking for null in
> SetSettingForOrigin().
> 

Yeah, I saw it, and I thought it's ok to have a warning log. Is that we should
try not to touch abnormal logs, even warning logs from our code?
And I totally agree with adding a guard for embedder in SetSettingForOrigin() is
a better solution. It's done. Thanks for your good idea!


> Also, can we get rid of the |embedder| parameter here if it's going to be
> ignored anyway?

Yep, done.

[Bernhard Bauer, 2016-03-24 09:30:33]

Thanks! LGTM affirmed :)

On 2016/03/24 00:19:36, Liu Shang wrote:
> > The biggest issue I have with nullptr specifically is that the argument will
> be
> > converted to a std::string, and passing in null to ConvertJavaStringToUTF8()
> > isn't actually allowed, it's just that the code silently accepts it (see
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/android/jni_s...).
> > So, we should probably guard that call by checking for null in
> > SetSettingForOrigin().
> > 
> 
> Yeah, I saw it, and I thought it's ok to have a warning log. Is that we should
> try not to touch abnormal logs, even warning logs from our code?

Sorry, I can't parse that sentence. 

The issue is really inside of ConvertJavaStringToUTF8(), which states that
passing null isn't allowed, but logging a warning is not going to result in any
misuse getting fixed, because no one is going to notice it. The correct thing in
ConvertJavaStringToUTF8() is a DCHECK, which will a) serve as a more explicit
statement of the precondition, b) crash in a Debug build, and c) not add any
runtime overhead to a Release build.

> And I totally agree with adding a guard for embedder in SetSettingForOrigin()
is
> a better solution. It's done. Thanks for your good idea!
> 
> 
> > Also, can we get rid of the |embedder| parameter here if it's going to be
> > ignored anyway?
> 
> Yep, done.

[lshang, 2016-04-05 07:30:26]

The CQ bit was checked by lshang@chromium.org

[lshang, 2016-04-05 07:30:26]

The patchset sent to the CQ was uploaded after l-g-t-m from raymes@chromium.org,
svaldez@chromium.org, bauerb@chromium.org
Link to the patchset: https://codereview.chromium.org/1819773002/#ps180001
(title: "rebase")

[commit-bot: I haz the power, 2016-04-05 07:30:37]

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

[commit-bot: I haz the power, 2016-04-05 07:38:04]

Description was changed from

==========
Use GURLs instead of patterns for SetContentSetting() in WebsitePreferenceBridge

Migrate SetContentSetting() in WebsitePreferenceBridge to take GURLs instead of 
patterns.

BUG=551747
==========

to

==========
Use GURLs instead of patterns for SetContentSetting() in WebsitePreferenceBridge

Migrate SetContentSetting() in WebsitePreferenceBridge to take GURLs instead of 
patterns.

BUG=551747
==========

[commit-bot: I haz the power, 2016-04-05 07:38:05]

Committed patchset #6 (id:180001)

[commit-bot: I haz the power, 2016-04-05 07:39:31]

Description was changed from

==========
Use GURLs instead of patterns for SetContentSetting() in WebsitePreferenceBridge

Migrate SetContentSetting() in WebsitePreferenceBridge to take GURLs instead of 
patterns.

BUG=551747
==========

to

==========
Use GURLs instead of patterns for SetContentSetting() in WebsitePreferenceBridge

Migrate SetContentSetting() in WebsitePreferenceBridge to take GURLs instead of
patterns.

BUG=551747

Committed: https://crrev.com/f85700f54fe76ffb38e40629d9b8109745fe3c33
Cr-Commit-Position: refs/heads/master@{#385121}
==========

[commit-bot: I haz the power, 2016-04-05 07:39:32]

Patchset 6 (id:??) landed as
https://crrev.com/f85700f54fe76ffb38e40629d9b8109745fe3c33
Cr-Commit-Position: refs/heads/master@{#385121}