Modify Physical Web auto-optin behavior on Chrome for iOS

Modify Physical Web auto-optin behavior on Chrome for iOS

The Physical Web feature will auto-enable if certain conditions are met
when the browser first launches or switches modes. This CL adds a new
condition: Google must be configured as the default search provider.

The behavior when the auto-enable check fails is also changed. Previously,
if the check failed the preference would be left in the default/onboarding
state. With this CL, the preference will be explicitly disabled if the
check fails.

BUG=630769
Committed: https://crrev.com/40351cf5845e48c03a54f7a2873955e4c68c6c44
Cr-Commit-Position: refs/heads/master@{#438898}

[mattreynolds, 2016-12-13 18:33:05]

mattreynolds@chromium.org changed reviewers:
+ sdefresne@chromium.org

[mattreynolds, 2016-12-13 18:33:06]

Hi Sylvain, PTAL

[sdefresne, 2016-12-14 10:15:56]

https://codereview.chromium.org/2573923002/diff/1/ios/chrome/browser/physical...
File ios/chrome/browser/physical_web/start_physical_web_discovery.mm (right):

https://codereview.chromium.org/2573923002/diff/1/ios/chrome/browser/physical...
ios/chrome/browser/physical_web/start_physical_web_discovery.mm:57: const
base::string16& default_search_name =
components/google/core/browser/google_util.h has a method
google_util::IsGoogleSearchUrl(const GURL&). Can this be used here instead of
comparing strings?

https://codereview.chromium.org/2573923002/diff/1/ios/chrome/browser/physical...
ios/chrome/browser/physical_web/start_physical_web_discovery.mm:66:
pref_service->SetInteger(prefs::kIosPhysicalWebEnabled,
If I understand correctly, since all new install use Google as the default
search engine, and have no incognito tab opens, this will be enabled by default
for all new installs? Correct?

[mattreynolds, 2016-12-14 20:35:02]

https://codereview.chromium.org/2573923002/diff/1/ios/chrome/browser/physical...
File ios/chrome/browser/physical_web/start_physical_web_discovery.mm (right):

https://codereview.chromium.org/2573923002/diff/1/ios/chrome/browser/physical...
ios/chrome/browser/physical_web/start_physical_web_discovery.mm:57: const
base::string16& default_search_name =
On 2016/12/14 10:15:56, sdefresne wrote:
> components/google/core/browser/google_util.h has a method
> google_util::IsGoogleSearchUrl(const GURL&). Can this be used here instead of
> comparing strings?

I couldn't get IsGoogleSearchUrl to work since the url stored by TemplateURL
uses a keyword for the domain. But, TemplateURL has
IsGoogleSearchURLWithReplaceableKeyword which does what we want.

https://codereview.chromium.org/2573923002/diff/1/ios/chrome/browser/physical...
ios/chrome/browser/physical_web/start_physical_web_discovery.mm:66:
pref_service->SetInteger(prefs::kIosPhysicalWebEnabled,
On 2016/12/14 10:15:56, sdefresne wrote:
> If I understand correctly, since all new install use Google as the default
> search engine, and have no incognito tab opens, this will be enabled by
default
> for all new installs? Correct?

We have five requirements for auto-enable:

* Location Services is on
* Chrome is authorized to use your location
* Google.com is eligible for geolocation (AFAICT this is a no-op on iOS since
there's no Site Settings)
* Google Search is the default search provider
* User is not currently in incognito mode

On top of that, since we'll be launching under a Finch flag, it will take at
least one restart before the auto-enable logic will execute.

So, on first launch, I expect:

* Location Services: could be on or off
* Location app permission: off for first launch, but NTP will prompt for it
* Google.com geolocation: defaults to eligible, no way to disable it
* Default search provider: Google Search
* Incognito: no
* Finch: disabled

Supposing Location Services is on and the user grants the Location permission
when prompted, then on the second launch the Finch flag will be enabled and
we'll auto-enable. If the user doesn't grant Location or changes any of the
other settings, we'll auto-disable.

Eventually we'll remove Finch or have it default to enabled. In that case, I'd
expect the new install to never auto-enable since the location permission won't
be granted in time.

[sdefresne, 2016-12-15 10:46:40]

On 2016/12/14 20:35:02, mattreynolds wrote:
>
https://codereview.chromium.org/2573923002/diff/1/ios/chrome/browser/physical...
> File ios/chrome/browser/physical_web/start_physical_web_discovery.mm (right):
> 
>
https://codereview.chromium.org/2573923002/diff/1/ios/chrome/browser/physical...
> ios/chrome/browser/physical_web/start_physical_web_discovery.mm:57: const
> base::string16& default_search_name =
> On 2016/12/14 10:15:56, sdefresne wrote:
> > components/google/core/browser/google_util.h has a method
> > google_util::IsGoogleSearchUrl(const GURL&). Can this be used here instead
of
> > comparing strings?
> 
> I couldn't get IsGoogleSearchUrl to work since the url stored by TemplateURL
> uses a keyword for the domain. But, TemplateURL has
> IsGoogleSearchURLWithReplaceableKeyword which does what we want.
> 
>
https://codereview.chromium.org/2573923002/diff/1/ios/chrome/browser/physical...
> ios/chrome/browser/physical_web/start_physical_web_discovery.mm:66:
> pref_service->SetInteger(prefs::kIosPhysicalWebEnabled,
> On 2016/12/14 10:15:56, sdefresne wrote:
> > If I understand correctly, since all new install use Google as the default
> > search engine, and have no incognito tab opens, this will be enabled by
> default
> > for all new installs? Correct?
> 
> We have five requirements for auto-enable:
> 
> * Location Services is on
> * Chrome is authorized to use your location
> * http://Google.com is eligible for geolocation (AFAICT this is a no-op on iOS
since
> there's no Site Settings)
> * Google Search is the default search provider
> * User is not currently in incognito mode
> 
> On top of that, since we'll be launching under a Finch flag, it will take at
> least one restart before the auto-enable logic will execute.
> 
> So, on first launch, I expect:
> 
> * Location Services: could be on or off
> * Location app permission: off for first launch, but NTP will prompt for it
> * http://Google.com geolocation: defaults to eligible, no way to disable it
> * Default search provider: Google Search
> * Incognito: no
> * Finch: disabled
> 
> Supposing Location Services is on and the user grants the Location permission
> when prompted, then on the second launch the Finch flag will be enabled and
> we'll auto-enable. If the user doesn't grant Location or changes any of the
> other settings, we'll auto-disable.
> 
> Eventually we'll remove Finch or have it default to enabled. In that case, I'd
> expect the new install to never auto-enable since the location permission
won't
> be granted in time.

Thank you for the detailed explanation.

lgtm

[sdefresne, 2016-12-15 10:46:46]

https://codereview.chromium.org/2573923002/diff/20001/ios/chrome/browser/phys...
File ios/chrome/browser/physical_web/start_physical_web_discovery.mm (right):

https://codereview.chromium.org/2573923002/diff/20001/ios/chrome/browser/phys...
ios/chrome/browser/physical_web/start_physical_web_discovery.mm:9: #include
"base/strings/utf_string_conversions.h"
I think you can remove this include now that you use
IsGoogleSearchURLWithReplaceableKeyword().

[mattreynolds, 2016-12-15 18:42:52]

The CQ bit was checked by mattreynolds@chromium.org

[mattreynolds, 2016-12-15 18:42:53]

The patchset sent to the CQ was uploaded after l-g-t-m from
sdefresne@chromium.org
Link to the patchset: https://codereview.chromium.org/2573923002/#ps40001
(title: "remove utf_string_conversions include")

[commit-bot: I haz the power, 2016-12-15 18:43:08]

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

[mattreynolds, 2016-12-15 18:43:24]

Thanks Sylvain!

https://codereview.chromium.org/2573923002/diff/20001/ios/chrome/browser/phys...
File ios/chrome/browser/physical_web/start_physical_web_discovery.mm (right):

https://codereview.chromium.org/2573923002/diff/20001/ios/chrome/browser/phys...
ios/chrome/browser/physical_web/start_physical_web_discovery.mm:9: #include
"base/strings/utf_string_conversions.h"
On 2016/12/15 10:46:46, sdefresne wrote:
> I think you can remove this include now that you use
> IsGoogleSearchURLWithReplaceableKeyword().

Done.

[commit-bot: I haz the power, 2016-12-15 19:49:38]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1481827372856480,
"parent_rev": "dcc73c9c86075ef406106ebe2da1d2206b1a204f", "commit_rev":
"5dac5b9334d3fadb8cc288565a515879bcd22651"}

[commit-bot: I haz the power, 2016-12-15 19:50:05]

Description was changed from

==========
Modify Physical Web auto-optin behavior on Chrome for iOS

The Physical Web feature will auto-enable if certain conditions are met
when the browser first launches or switches modes. This CL adds a new
condition: Google must be configured as the default search provider.

The behavior when the auto-enable check fails is also changed. Previously,
if the check failed the preference would be left in the default/onboarding
state. With this CL, the preference will be explicitly disabled if the
check fails.

BUG=630769
==========

to

==========
Modify Physical Web auto-optin behavior on Chrome for iOS

The Physical Web feature will auto-enable if certain conditions are met
when the browser first launches or switches modes. This CL adds a new
condition: Google must be configured as the default search provider.

The behavior when the auto-enable check fails is also changed. Previously,
if the check failed the preference would be left in the default/onboarding
state. With this CL, the preference will be explicitly disabled if the
check fails.

BUG=630769

Review-Url: https://codereview.chromium.org/2573923002
==========

[commit-bot: I haz the power, 2016-12-15 19:50:06]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-12-15 19:52:55]

Description was changed from

==========
Modify Physical Web auto-optin behavior on Chrome for iOS

The Physical Web feature will auto-enable if certain conditions are met
when the browser first launches or switches modes. This CL adds a new
condition: Google must be configured as the default search provider.

The behavior when the auto-enable check fails is also changed. Previously,
if the check failed the preference would be left in the default/onboarding
state. With this CL, the preference will be explicitly disabled if the
check fails.

BUG=630769

Review-Url: https://codereview.chromium.org/2573923002
==========

to

==========
Modify Physical Web auto-optin behavior on Chrome for iOS

The Physical Web feature will auto-enable if certain conditions are met
when the browser first launches or switches modes. This CL adds a new
condition: Google must be configured as the default search provider.

The behavior when the auto-enable check fails is also changed. Previously,
if the check failed the preference would be left in the default/onboarding
state. With this CL, the preference will be explicitly disabled if the
check fails.

BUG=630769

Committed: https://crrev.com/40351cf5845e48c03a54f7a2873955e4c68c6c44
Cr-Commit-Position: refs/heads/master@{#438898}
==========

[commit-bot: I haz the power, 2016-12-15 19:52:56]

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