Fix crashes when using translate in incognito by overriding some methods.

Fix crash when using translate in incognito.

Crash is fixed by returning the given browser state in 
BrowserStateKeyedServiceFactory even if browser state is off the record.

BUG=739986
Review-Url: https://codereview.chromium.org/2973173002
Cr-Commit-Position: refs/heads/master@{#484972}
Committed: https://chromium.googlesource.com/chromium/src/+/299f4d2472f0bb17bb10f4bb3e0caa7947f8858a

[jzw1, 2017-07-07 03:27:48]

Description was changed from

==========
Fix crashes when using translate in incognito by implementing extra
method in BrowserStateKeyedServiceFactory.

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

to

==========
Fix crashes when using translate in incognito by implementing extra
method in BrowserStateKeyedServiceFactory.

BUG=739986
==========

[jzw1, 2017-07-07 03:33:21]

jzw@chromium.org changed reviewers:
+ eugenebut@chromium.org, ichikawa@chromium.org, michaeldo@chromium.org

[jzw1, 2017-07-07 03:33:29]

On 2017/07/07 03:33:21, jzw1 wrote:
> mailto:jzw@chromium.org changed reviewers:
> + mailto:eugenebut@chromium.org, mailto:ichikawa@chromium.org,
mailto:michaeldo@chromium.org

hey guys PTAL.

[Hiroshi Ichikawa, 2017-07-07 05:15:21]

https://codereview.chromium.org/2973173002/diff/1/ios/web_view/internal/trans...
File
ios/web_view/internal/translate/web_view_translate_accept_languages_factory.cc
(right):

https://codereview.chromium.org/2973173002/diff/1/ios/web_view/internal/trans...
ios/web_view/internal/translate/web_view_translate_accept_languages_factory.cc:81:
return context;
Can you add a comment explaining that this class overrides this function here to
make this class available in incognito mode too? I was confused why this is
needed until I read the definition of
BrowserStateKeyedServiceFactory::GetBrowserStateToUse().

https://codereview.chromium.org/2973173002/diff/1/ios/web_view/internal/trans...
File ios/web_view/internal/translate/web_view_translate_ranker_factory.cc
(right):

https://codereview.chromium.org/2973173002/diff/1/ios/web_view/internal/trans...
ios/web_view/internal/translate/web_view_translate_ranker_factory.cc:58: return
context;
Can you add a comment explaining that this class overrides this function here to
make this class available in incognito mode too? I was confused why this is
needed until I read the definition of
BrowserStateKeyedServiceFactory::GetBrowserStateToUse().

[jzw1, 2017-07-07 06:20:44]

https://codereview.chromium.org/2973173002/diff/1/ios/web_view/internal/trans...
File
ios/web_view/internal/translate/web_view_translate_accept_languages_factory.cc
(right):

https://codereview.chromium.org/2973173002/diff/1/ios/web_view/internal/trans...
ios/web_view/internal/translate/web_view_translate_accept_languages_factory.cc:81:
return context;
On 2017/07/07 05:15:21, Hiroshi Ichikawa wrote:
> Can you add a comment explaining that this class overrides this function here
to
> make this class available in incognito mode too? I was confused why this is
> needed until I read the definition of
> BrowserStateKeyedServiceFactory::GetBrowserStateToUse().

Done.

https://codereview.chromium.org/2973173002/diff/1/ios/web_view/internal/trans...
File ios/web_view/internal/translate/web_view_translate_ranker_factory.cc
(right):

https://codereview.chromium.org/2973173002/diff/1/ios/web_view/internal/trans...
ios/web_view/internal/translate/web_view_translate_ranker_factory.cc:58: return
context;
On 2017/07/07 05:15:21, Hiroshi Ichikawa wrote:
> Can you add a comment explaining that this class overrides this function here
to
> make this class available in incognito mode too? I was confused why this is
> needed until I read the definition of
> BrowserStateKeyedServiceFactory::GetBrowserStateToUse().

Done.

[Hiroshi Ichikawa, 2017-07-07 07:55:44]

lgtm

[michaeldo, 2017-07-07 15:42:18]

Please update description to have one-line title in addition to current
description.

In Chrome, it looks like we always return the non-incognito browser state:

https://cs.chromium.org/chromium/src/ios/chrome/browser/web_data_service_fact...

We can't do that in ios/web_view because it's possible only the incognito
browser state exists but we should understand the difference (I'm not sure why
we always use the non-incognito one for example in Chrome) and maybe add an
explanation where we return the context here about why we return the param
directly.

[Eugene But (OOO till 7-30), 2017-07-07 16:39:52]

lgtm

[jzw1, 2017-07-07 17:13:41]

Description was changed from

==========
Fix crashes when using translate in incognito by implementing extra
method in BrowserStateKeyedServiceFactory.

BUG=739986
==========

to

==========
Fix crash when using translate in incognito.

Crash is fixed by implementing missing method in
BrowserStateKeyedServiceFactory.

BUG=739986
==========

[jzw1, 2017-07-07 17:15:43]

Description was changed from

==========
Fix crash when using translate in incognito.

Crash is fixed by implementing missing method in
BrowserStateKeyedServiceFactory.

BUG=739986
==========

to

==========
Fix crash when using translate in incognito.

Crash is fixed by returning the given browser state in 
BrowserStateKeyedServiceFactory even if browser state is off the record.

BUG=739986
==========

[jzw1, 2017-07-07 17:24:51]

On 2017/07/07 15:42:18, michaeldo wrote:
> Please update description to have one-line title in addition to current
> description.
> 
> In Chrome, it looks like we always return the non-incognito browser state:
> 
>
https://cs.chromium.org/chromium/src/ios/chrome/browser/web_data_service_fact...
> 
> We can't do that in ios/web_view because it's possible only the incognito
> browser state exists but we should understand the difference (I'm not sure why
> we always use the non-incognito one for example in Chrome) and maybe add an
> explanation where we return the context here about why we return the param
> directly.

Michael, I looked into it and I believe Chrome returns the non-incognito one to
share translate preferences. For example, if I mark Japanese has never translate
in normal browsing, it will not ask to translate when I visit a Japanese site in
Incognito.
However, incognito always blocks auto translation.

[michaeldo, 2017-07-07 17:27:18]

On 2017/07/07 17:24:51, jzw1 wrote:
> On 2017/07/07 15:42:18, michaeldo wrote:
> > Please update description to have one-line title in addition to current
> > description.
> > 
> > In Chrome, it looks like we always return the non-incognito browser state:
> > 
> >
>
https://cs.chromium.org/chromium/src/ios/chrome/browser/web_data_service_fact...
> > 
> > We can't do that in ios/web_view because it's possible only the incognito
> > browser state exists but we should understand the difference (I'm not sure
why
> > we always use the non-incognito one for example in Chrome) and maybe add an
> > explanation where we return the context here about why we return the param
> > directly.
> 
> Michael, I looked into it and I believe Chrome returns the non-incognito one
to
> share translate preferences. For example, if I mark Japanese has never
translate
> in normal browsing, it will not ask to translate when I visit a Japanese site
in
> Incognito.
> However, incognito always blocks auto translation.

I see, thank you for the explanation. That makes sense.

lgtm

[jzw1, 2017-07-07 17:31:17]

On 2017/07/07 17:24:51, jzw1 wrote:
> On 2017/07/07 15:42:18, michaeldo wrote:
> > Please update description to have one-line title in addition to current
> > description.
> > 
> > In Chrome, it looks like we always return the non-incognito browser state:
> > 
> >
>
https://cs.chromium.org/chromium/src/ios/chrome/browser/web_data_service_fact...
> > 
> > We can't do that in ios/web_view because it's possible only the incognito
> > browser state exists but we should understand the difference (I'm not sure
why
> > we always use the non-incognito one for example in Chrome) and maybe add an
> > explanation where we return the context here about why we return the param
> > directly.
> 
> Michael, I looked into it and I believe Chrome returns the non-incognito one
to
> share translate preferences. For example, if I mark Japanese has never
translate
> in normal browsing, it will not ask to translate when I visit a Japanese site
in
> Incognito.
> However, incognito always blocks auto translation.

I should add that what we are doing here has the same effect because we don't
have a different preferences path for incognito browser states.

[jzw1, 2017-07-07 17:31:57]

The CQ bit was checked by jzw@chromium.org

[commit-bot: I haz the power, 2017-07-07 17:32:19]

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-07-07 17:42:51]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1499448717957610,
"parent_rev": "6df2d002f6306446ce5267c15b76bf7ba9a7b152", "commit_rev":
"b2ce2518a92ac0d54168c3fb8f9e7a398e8a0d87"}

[commit-bot: I haz the power, 2017-07-07 17:43:05]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1499448717957610,
"parent_rev": "91e29608504eddd212d0ba112a784f5e365b9159", "commit_rev":
"299f4d2472f0bb17bb10f4bb3e0caa7947f8858a"}

[commit-bot: I haz the power, 2017-07-07 17:43:18]

Description was changed from

==========
Fix crash when using translate in incognito.

Crash is fixed by returning the given browser state in 
BrowserStateKeyedServiceFactory even if browser state is off the record.

BUG=739986
==========

to

==========
Fix crash when using translate in incognito.

Crash is fixed by returning the given browser state in 
BrowserStateKeyedServiceFactory even if browser state is off the record.

BUG=739986

Review-Url: https://codereview.chromium.org/2973173002
Cr-Commit-Position: refs/heads/master@{#484972}
Committed:
https://chromium.googlesource.com/chromium/src/+/299f4d2472f0bb17bb10f4bb3e0c...
==========

[commit-bot: I haz the power, 2017-07-07 17:43:22]

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

[jzw1, 2017-07-08 07:00:01]

Description was changed from

==========
Fix crash when using translate in incognito.

Crash is fixed by returning the given browser state in 
BrowserStateKeyedServiceFactory even if browser state is off the record.

BUG=739986

Review-Url: https://codereview.chromium.org/2973173002
Cr-Commit-Position: refs/heads/master@{#484972}
Committed:
https://chromium.googlesource.com/chromium/src/+/299f4d2472f0bb17bb10f4bb3e0c...
==========

to

==========
Fix crash when using translate in incognito.

Crash is fixed by returning the given browser state in 
BrowserStateKeyedServiceFactory even if browser state is off the record.

BUG=739986

Review-Url: https://codereview.chromium.org/2973173002
Cr-Commit-Position: refs/heads/master@{#484972}
Committed:
https://chromium.googlesource.com/chromium/src/+/299f4d2472f0bb17bb10f4bb3e0c...
==========