Disable learning text suggestions if in incognito.

Disable learning text suggestions if in incognito (via Omnibox).

BUG=725543
Review-Url: https://codereview.chromium.org/2901023002
Cr-Commit-Position: refs/heads/master@{#474118}
Committed: https://chromium.googlesource.com/chromium/src/+/5c08787e71f7f758001542e6cc2f87f50bf12064

[Ted C, 2017-05-23 16:31:49]

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

[Ted C, 2017-05-23 16:31:49]

PTAL

[Ted C, 2017-05-23 16:34:09]

Description was changed from

==========
Disable learning text suggestions if in incognito.

BUG=725543
==========

to

==========
Disable learning text suggestions if in incognito (via Omnibox).

BUG=725543
==========

[Changwan Ryu, 2017-05-23 18:54:23]

https://codereview.chromium.org/2901023002/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/omnibox/UrlBar.java
(right):

https://codereview.chromium.org/2901023002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/UrlBar.java:657: if
(mUrlBarDelegate != null && mUrlBarDelegate.isIncognito()) {
This should arguably be

assert mUrlBarDelegate != null;
if (mUrlBarDelegate.isIncognito()) {

to ensure the flag option without having to manually check all the call sites of
setDelegate(), but it's a bit scary because we need to depend on unspecified
android framework behavior..

Maybe we could have both assert and null check if we want to avoid crash while
keeping track of flag issue.. WDYT?

https://codereview.chromium.org/2901023002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java
(right):

https://codereview.chromium.org/2901023002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java:249:
return super.isIncognito();
Hmm.. We probably don't need this?

[Ted C, 2017-05-23 19:17:56]

https://codereview.chromium.org/2901023002/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/omnibox/UrlBar.java
(right):

https://codereview.chromium.org/2901023002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/UrlBar.java:657: if
(mUrlBarDelegate != null && mUrlBarDelegate.isIncognito()) {
On 2017/05/23 18:54:23, Changwan Ryu wrote:
> This should arguably be
> 
> assert mUrlBarDelegate != null;
> if (mUrlBarDelegate.isIncognito()) {
> 
> to ensure the flag option without having to manually check all the call sites
of
> setDelegate(), but it's a bit scary because we need to depend on unspecified
> android framework behavior..
> 
> Maybe we could have both assert and null check if we want to avoid crash while
> keeping track of flag issue.. WDYT?

I think the null and assert seems safest.  I would hope no android version
would call this as part of the constructor, but I have also no confidence
that some version doesn't do just that.

https://codereview.chromium.org/2901023002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java
(right):

https://codereview.chromium.org/2901023002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java:249:
return super.isIncognito();
On 2017/05/23 18:54:23, Changwan Ryu wrote:
> Hmm.. We probably don't need this?

The super version is protected and the interface requires public, so this is
required to increase the visibility.

[Changwan Ryu, 2017-05-23 19:47:47]

https://codereview.chromium.org/2901023002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java
(right):

https://codereview.chromium.org/2901023002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java:2260:
if (mToolbarDataProvider == null) return false;
We actually set an empty version in ToolbarLayout#onFinishInflate(),
and set a real version after ToolbarLayout#initialize() which is part of post
inflation startup. If inputconnection is activated between the two calls, then
we may report the flag incorrectly, but likely not possible because
onFinishInflate() and initialize() will likely be called in one UI loop. Could
you confirm?

[Ted C, 2017-05-23 20:07:19]

https://codereview.chromium.org/2901023002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java
(right):

https://codereview.chromium.org/2901023002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java:2260:
if (mToolbarDataProvider == null) return false;
On 2017/05/23 19:47:47, Changwan Ryu wrote:
> We actually set an empty version in ToolbarLayout#onFinishInflate(),
> and set a real version after ToolbarLayout#initialize() which is part of post
> inflation startup. If inputconnection is activated between the two calls, then
> we may report the flag incorrectly, but likely not possible because
> onFinishInflate() and initialize() will likely be called in one UI loop. Could
> you confirm?

The null check here is for consistency with above.  So this seems somewhat
orthogonal and whether we should investigate removing the null checks
entirely.

The reason I kept these here and I don't think it to be too terribly
unreasonable
is that these are called by Android methods on View/TextView.  I have no
control over whether the super constructor or onFinishInflate for TextView
will trigger an input connection create and thus call into this very early
in the construction flow before any of these calls you show are triggered.

So I guess I'm wondering what you want to be confirmed?

[Ted C, 2017-05-23 21:09:15]

https://codereview.chromium.org/2901023002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java
(right):

https://codereview.chromium.org/2901023002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java:2260:
if (mToolbarDataProvider == null) return false;
On 2017/05/23 20:07:19, Ted C wrote:
> On 2017/05/23 19:47:47, Changwan Ryu wrote:
> > We actually set an empty version in ToolbarLayout#onFinishInflate(),
> > and set a real version after ToolbarLayout#initialize() which is part of
post
> > inflation startup. If inputconnection is activated between the two calls,
then
> > we may report the flag incorrectly, but likely not possible because
> > onFinishInflate() and initialize() will likely be called in one UI loop.
Could
> > you confirm?
> 
> The null check here is for consistency with above.  So this seems somewhat
> orthogonal and whether we should investigate removing the null checks
> entirely.
> 
> The reason I kept these here and I don't think it to be too terribly
> unreasonable
> is that these are called by Android methods on View/TextView.  I have no
> control over whether the super constructor or onFinishInflate for TextView
> will trigger an input connection create and thus call into this very early
> in the construction flow before any of these calls you show are triggered.
> 
> So I guess I'm wondering what you want to be confirmed?

Per the offline discussion, this was more around us opt-ing into keyboard
sharing when the state was unknown.  That is a great point and we shouldn't do
that.

Switched to default off until we can actually determine the incognito-ness.  All
the boolean logic should be inverted now.

[Changwan Ryu, 2017-05-23 21:19:37]

lgtm

[Ted C, 2017-05-23 22:32:16]

The CQ bit was checked by tedchoc@chromium.org

[commit-bot: I haz the power, 2017-05-23 22:34:17]

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-24 00:18:22]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1495578736522410,
"parent_rev": "f9e6c47b2075f812ea974cf72df85b35a5936ebe", "commit_rev":
"5c08787e71f7f758001542e6cc2f87f50bf12064"}

[commit-bot: I haz the power, 2017-05-24 00:18:35]

Description was changed from

==========
Disable learning text suggestions if in incognito (via Omnibox).

BUG=725543
==========

to

==========
Disable learning text suggestions if in incognito (via Omnibox).

BUG=725543

Review-Url: https://codereview.chromium.org/2901023002
Cr-Commit-Position: refs/heads/master@{#474118}
Committed:
https://chromium.googlesource.com/chromium/src/+/5c08787e71f7f758001542e6cc2f...
==========

[commit-bot: I haz the power, 2017-05-24 00:18:36]

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