[SmartText selection] Add LocaleList to the provider interface

[SmartText selection] Add Locale array to the provider interface

Android TextClassifier API accepts optional LocaleList parameter,
this CL passes locales as Locale array to the interface. There is no
implementation that retrieves locales of the selection, we pass null
to the provider.

BUG=710505
Review-Url: https://codereview.chromium.org/2815833003
Cr-Commit-Position: refs/heads/master@{#464578}
Committed: https://chromium.googlesource.com/chromium/src/+/0509fea9753de15947d230e379e2cc2338e108dc

[Tima Vaisburd, 2017-04-13 01:35:49]

The CQ bit was checked by timav@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-13 01:36:52]

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

[Tima Vaisburd, 2017-04-13 01:48:10]

timav@chromium.org changed reviewers:
+ boliu@chromium.org, tedchoc@chromium.org

[Tima Vaisburd, 2017-04-13 01:48:10]

PTAL.

[commit-bot: I haz the power, 2017-04-13 02:14:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-13 02:14:54]

Dry run: This issue passed the CQ dry run.

[boliu, 2017-04-13 02:52:31]

https://codereview.chromium.org/2815833003/diff/1/content/public/android/java...
File
content/public/android/java/src/org/chromium/content/browser/ContextSelectionClient.java
(right):

https://codereview.chromium.org/2815833003/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/ContextSelectionClient.java:140:
mProvider.sendSuggestAndClassifyRequest(text, start, end, null);
so this is not implemented yet?

[boliu, 2017-04-13 02:55:48]

lgtm

[Ted C, 2017-04-13 17:17:10]

https://codereview.chromium.org/2815833003/diff/1/content/public/android/java...
File
content/public/android/java/src/org/chromium/content/browser/ContextSelectionProvider.java
(right):

https://codereview.chromium.org/2815833003/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/ContextSelectionProvider.java:9:
import android.os.LocaleList;
LocaleList is api 24...are any of the apis below called for older versions of
Android?  If so, that probably crashes.

[Tima Vaisburd, 2017-04-13 18:11:32]

https://codereview.chromium.org/2815833003/diff/1/content/public/android/java...
File
content/public/android/java/src/org/chromium/content/browser/ContextSelectionProvider.java
(right):

https://codereview.chromium.org/2815833003/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/ContextSelectionProvider.java:9:
import android.os.LocaleList;
On 2017/04/13 17:17:10, Ted C wrote:
> LocaleList is api 24...are any of the apis below called for older versions of
> Android?  If so, that probably crashes.

The implementation only calls to TextClassifier API which is O and above.
Do you suggest making this insterface robust for all Android versions, e.g. by
providing an array of strings and converting them on the implementation side?

[Ted C, 2017-04-13 18:14:19]

On 2017/04/13 18:11:32, Tima Vaisburd wrote:
>
https://codereview.chromium.org/2815833003/diff/1/content/public/android/java...
> File
>
content/public/android/java/src/org/chromium/content/browser/ContextSelectionProvider.java
> (right):
> 
>
https://codereview.chromium.org/2815833003/diff/1/content/public/android/java...
>
content/public/android/java/src/org/chromium/content/browser/ContextSelectionProvider.java:9:
> import android.os.LocaleList;
> On 2017/04/13 17:17:10, Ted C wrote:
> > LocaleList is api 24...are any of the apis below called for older versions
of
> > Android?  If so, that probably crashes.
> 
> The implementation only calls to TextClassifier API which is O and above.
> Do you suggest making this insterface robust for all Android versions, e.g. by
> providing an array of strings and converting them on the implementation side?

LocaleList is just a list of Locale objects.  I think you can just pass either
an array/list of Locale objects and convert to LocaleList in the O API.

[boliu, 2017-04-13 18:17:15]

side discussion.. it's not actually clear to me if the current code is safe or
not. like is it safe to load the class? It went through cq fine, but I think
that's because this code is not exercised by tests at all.

I think err-ing on side of safety is a good idea. This interface itself isn't
meant to be O only.

[Ted C, 2017-04-13 18:20:10]

On 2017/04/13 18:17:15, boliu wrote:
> side discussion.. it's not actually clear to me if the current code is safe or
> not. like is it safe to load the class? It went through cq fine, but I think
> that's because this code is not exercised by tests at all.
> 
> I think err-ing on side of safety is a good idea. This interface itself isn't
> meant to be O only.

I "think" you can include new classes and it will work on older platforms
as long as the code is never called (it will just print logs saying the class
can't be found).  Granted, I don't think I've ever explicitly done this with
an interface vs a class, but I would expect them to be the same.

But in this case, it is very easy to pass something that works on all platfroms
without a bunch of work.

[Tima Vaisburd, 2017-04-13 18:30:45]

On 2017/04/13 18:20:10, Ted C wrote:
> On 2017/04/13 18:17:15, boliu wrote:
> > side discussion.. it's not actually clear to me if the current code is safe
or
> > not. like is it safe to load the class? It went through cq fine, but I think
> > that's because this code is not exercised by tests at all.
> > 
> > I think err-ing on side of safety is a good idea. This interface itself
isn't
> > meant to be O only.
> 
> I "think" you can include new classes and it will work on older platforms
> as long as the code is never called (it will just print logs saying the class
> can't be found).  Granted, I don't think I've ever explicitly done this with
> an interface vs a class, but I would expect them to be the same.
> 
> But in this case, it is very easy to pass something that works on all
platfroms
> without a bunch of work.

As far as I can get the LocaleList cannot be created from a list of
java.util.Locale objects,
but instead can be created from a single string by forLanguageTags().
https://developer.android.com/reference/android/os/LocaleList.html#forLanguag...

I will use the string. It might be even more convenient since the information
will eventually come from Blink through JNI.

[boliu, 2017-04-13 18:32:08]

On 2017/04/13 18:30:45, Tima Vaisburd wrote:
> On 2017/04/13 18:20:10, Ted C wrote:
> > On 2017/04/13 18:17:15, boliu wrote:
> > > side discussion.. it's not actually clear to me if the current code is
safe
> or
> > > not. like is it safe to load the class? It went through cq fine, but I
think
> > > that's because this code is not exercised by tests at all.
> > > 
> > > I think err-ing on side of safety is a good idea. This interface itself
> isn't
> > > meant to be O only.
> > 
> > I "think" you can include new classes and it will work on older platforms
> > as long as the code is never called (it will just print logs saying the
class
> > can't be found).  Granted, I don't think I've ever explicitly done this with
> > an interface vs a class, but I would expect them to be the same.
> > 
> > But in this case, it is very easy to pass something that works on all
> platfroms
> > without a bunch of work.
> 
> As far as I can get the LocaleList cannot be created from a list of
> java.util.Locale objects,
> but instead can be created from a single string by forLanguageTags().
>
https://developer.android.com/reference/android/os/LocaleList.html#forLanguag...
> 
> I will use the string. It might be even more convenient since the information
> will eventually come from Blink through JNI.

safer would be to use Object and then do a cast in O-only code then? so long
type safety :(

[Ted C, 2017-04-13 18:35:16]

On 2017/04/13 18:30:45, Tima Vaisburd wrote:
> On 2017/04/13 18:20:10, Ted C wrote:
> > On 2017/04/13 18:17:15, boliu wrote:
> > > side discussion.. it's not actually clear to me if the current code is
safe
> or
> > > not. like is it safe to load the class? It went through cq fine, but I
think
> > > that's because this code is not exercised by tests at all.
> > > 
> > > I think err-ing on side of safety is a good idea. This interface itself
> isn't
> > > meant to be O only.
> > 
> > I "think" you can include new classes and it will work on older platforms
> > as long as the code is never called (it will just print logs saying the
class
> > can't be found).  Granted, I don't think I've ever explicitly done this with
> > an interface vs a class, but I would expect them to be the same.
> > 
> > But in this case, it is very easy to pass something that works on all
> platfroms
> > without a bunch of work.
> 
> As far as I can get the LocaleList cannot be created from a list of
> java.util.Locale objects,
> but instead can be created from a single string by forLanguageTags().
>
https://developer.android.com/reference/android/os/LocaleList.html#forLanguag...
> 
> I will use the string. It might be even more convenient since the information
> will eventually come from Blink through JNI.

Doesn't the public constructor work?
https://developer.android.com/reference/android/os/LocaleList.html#LocaleList...

[Tima Vaisburd, 2017-04-13 18:36:54]

On 2017/04/13 18:32:08, boliu wrote:
> On 2017/04/13 18:30:45, Tima Vaisburd wrote:
> > As far as I can get the LocaleList cannot be created from a list of
> > java.util.Locale objects,

Oops, missed constructor:
https://developer.android.com/reference/android/os/LocaleList.html#LocaleList...

> safer would be to use Object and then do a cast in O-only code then? so long
> type safety :(

List<Locale> then? We would still receive a string from JNI...

[boliu, 2017-04-13 18:38:11]

On 2017/04/13 18:36:54, Tima Vaisburd wrote:
> On 2017/04/13 18:32:08, boliu wrote:
> > On 2017/04/13 18:30:45, Tima Vaisburd wrote:
> > > As far as I can get the LocaleList cannot be created from a list of
> > > java.util.Locale objects,
> 
> Oops, missed constructor:
>
https://developer.android.com/reference/android/os/LocaleList.html#LocaleList...
> 
> > safer would be to use Object and then do a cast in O-only code then? so long
> > type safety :(
> 
> List<Locale> then? We would still receive a string from JNI...

Locale[]. You still need to convert List I think.

[Tima Vaisburd, 2017-04-13 18:46:14]

On 2017/04/13 18:38:11, boliu wrote:
> On 2017/04/13 18:36:54, Tima Vaisburd wrote:
> > On 2017/04/13 18:32:08, boliu wrote:
> > > On 2017/04/13 18:30:45, Tima Vaisburd wrote:
> > > > As far as I can get the LocaleList cannot be created from a list of
> > > > java.util.Locale objects,
> > 
> > Oops, missed constructor:
> >
>
https://developer.android.com/reference/android/os/LocaleList.html#LocaleList...
> > 
> > > safer would be to use Object and then do a cast in O-only code then? so
long
> > > type safety :(
> > 
> > List<Locale> then? We would still receive a string from JNI...
> 
> Locale[]. You still need to convert List I think.

Yes, thank you. It is possible to use ArrayList<Locale> in principle...

From the purity of the interface I think List<String> or List<Locale> are better
options, I prefer the first, WDYT?

[boliu, 2017-04-13 18:49:53]

On 2017/04/13 18:46:14, Tima Vaisburd wrote:
> On 2017/04/13 18:38:11, boliu wrote:
> > On 2017/04/13 18:36:54, Tima Vaisburd wrote:
> > > On 2017/04/13 18:32:08, boliu wrote:
> > > > On 2017/04/13 18:30:45, Tima Vaisburd wrote:
> > > > > As far as I can get the LocaleList cannot be created from a list of
> > > > > java.util.Locale objects,
> > > 
> > > Oops, missed constructor:
> > >
> >
>
https://developer.android.com/reference/android/os/LocaleList.html#LocaleList...
> > > 
> > > > safer would be to use Object and then do a cast in O-only code then? so
> long
> > > > type safety :(
> > > 
> > > List<Locale> then? We would still receive a string from JNI...
> > 
> > Locale[]. You still need to convert List I think.
> 
> Yes, thank you. It is possible to use ArrayList<Locale> in principle...
> 
> From the purity of the interface I think List<String> or List<Locale> are
better
> options, I prefer the first, WDYT?

why do you think List is better? it's not like you are gonna be modifying it in
the provider

[Tima Vaisburd, 2017-04-13 19:02:01]

On 2017/04/13 18:49:53, boliu wrote:
> why do you think List is better? it's not like you are gonna be modifying it
in
> the provider

I thought List<String> is better that just String because clearly expresses that
there can be
several items or none at all and does not require the comment like "this is a
comma-separated list,
empty string is allowed and means the locales are not known" etc. We are not
imposing Android interface
decisions which might not be always cleanest.

I liked List<Locale> more than ArrayList<Locale> because I thought it is wrong
to impose any
requirement on the container structure if we do not have to.

Having said that, however, I will gladly accept your own ideas.

[boliu, 2017-04-13 19:04:07]

On 2017/04/13 19:02:01, Tima Vaisburd wrote:
> On 2017/04/13 18:49:53, boliu wrote:
> > why do you think List is better? it's not like you are gonna be modifying it
> in
> > the provider
> 
> I thought List<String> is better that just String because clearly expresses
that
> there can be
> several items or none at all and does not require the comment like "this is a
> comma-separated list,
> empty string is allowed and means the locales are not known" etc. We are not
> imposing Android interface
> decisions which might not be always cleanest.
> 
> I liked List<Locale> more than ArrayList<Locale> because I thought it is wrong
> to impose any
> requirement on the container structure if we do not have to.
> 
> Having said that, however, I will gladly accept your own ideas.

I didn't say string though. I said Locale[], ie *array* of Locale?

[Tima Vaisburd, 2017-04-13 19:11:21]

On 2017/04/13 19:04:07, boliu wrote:
> On 2017/04/13 19:02:01, Tima Vaisburd wrote:
> > On 2017/04/13 18:49:53, boliu wrote:
> > > why do you think List is better? it's not like you are gonna be modifying
it
> > in
> > > the provider
> > 
> > I thought List<String> is better that just String because clearly expresses
> that
> > there can be
> > several items or none at all and does not require the comment like "this is
a
> > comma-separated list,
> > empty string is allowed and means the locales are not known" etc. We are not
> > imposing Android interface
> > decisions which might not be always cleanest.
> > 
> > I liked List<Locale> more than ArrayList<Locale> because I thought it is
wrong
> > to impose any
> > requirement on the container structure if we do not have to.
> > 
> > Having said that, however, I will gladly accept your own ideas.
> 
> I didn't say string though. I said Locale[], ie *array* of Locale?

Ah, ok. The doc says
"For empty lists of Locale items it is better to use getEmptyLocaleList(), which
returns a pre-constructed empty list."
so a check is still required, but seems quite possible and the interface is
clear, I think.

I would still like the strings that represent the language tags better because
in principle there is no guarantee
that LocaleList.forLanguageTags() is doing what we think and what we get from
Blink are strings. Let me know whether
it is a valid thinking, I'll go with Locale[] meanwhile.

[Tima Vaisburd, 2017-04-13 21:01:15]

The CQ bit was checked by timav@chromium.org to run a CQ dry run

[Tima Vaisburd, 2017-04-13 21:02:36]

Description was changed from

==========
[SmartText selection] Add LocaleList to the provider interface

Android TextClassifier API accepts optional LocaleList parameter,
this CL add this to the interface. There is no implementation
that retrieves locales of the selection, we pass null to the
provider.

BUG=710505
==========

to

==========
[SmartText selection] Add Locale array to the provider interface

Android TextClassifier API accepts optional LocaleList parameter,
this CL passes locales as Locale array to the interface. There is no
implementation that retrieves locales of the selection, we pass null
to the provider.

BUG=710505
==========

[commit-bot: I haz the power, 2017-04-13 21:02:47]

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

[Tima Vaisburd, 2017-04-13 21:11:09]

On 2017/04/13 19:11:21, Tima Vaisburd wrote:
> On 2017/04/13 19:04:07, boliu wrote:
> > On 2017/04/13 19:02:01, Tima Vaisburd wrote:
> > > On 2017/04/13 18:49:53, boliu wrote:
> > > > why do you think List is better? it's not like you are gonna be
modifying
> it
> > > in
> > > > the provider
> > > 
> > > I thought List<String> is better that just String because clearly
expresses
> > that
> > > there can be
> > > several items or none at all and does not require the comment like "this
is
> a
> > > comma-separated list,
> > > empty string is allowed and means the locales are not known" etc. We are
not
> > > imposing Android interface
> > > decisions which might not be always cleanest.
> > > 
> > > I liked List<Locale> more than ArrayList<Locale> because I thought it is
> wrong
> > > to impose any
> > > requirement on the container structure if we do not have to.
> > > 
> > > Having said that, however, I will gladly accept your own ideas.
> > 
> > I didn't say string though. I said Locale[], ie *array* of Locale?
> 
> Ah, ok. The doc says
> "For empty lists of Locale items it is better to use getEmptyLocaleList(),
which
> returns a pre-constructed empty list."
> so a check is still required, but seems quite possible and the interface is
> clear, I think.
> 
> I would still like the strings that represent the language tags better because
> in principle there is no guarantee
> that LocaleList.forLanguageTags() is doing what we think and what we get from
> Blink are strings. Let me know whether
> it is a valid thinking, I'll go with Locale[] meanwhile.

Done.

[boliu, 2017-04-13 21:13:12]

lgtm

[Ted C, 2017-04-13 21:15:01]

lgtm

[commit-bot: I haz the power, 2017-04-13 21:51:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-13 21:52:00]

Dry run: This issue passed the CQ dry run.

[Tima Vaisburd, 2017-04-13 22:02:29]

The CQ bit was checked by timav@chromium.org

[commit-bot: I haz the power, 2017-04-13 22:04:02]

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-04-13 22:37:29]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1492120949893380,
"parent_rev": "9cccac38eeff5383d850ca65039f89b1091cbeeb", "commit_rev":
"0509fea9753de15947d230e379e2cc2338e108dc"}

[commit-bot: I haz the power, 2017-04-13 22:38:24]

Description was changed from

==========
[SmartText selection] Add Locale array to the provider interface

Android TextClassifier API accepts optional LocaleList parameter,
this CL passes locales as Locale array to the interface. There is no
implementation that retrieves locales of the selection, we pass null
to the provider.

BUG=710505
==========

to

==========
[SmartText selection] Add Locale array to the provider interface

Android TextClassifier API accepts optional LocaleList parameter,
this CL passes locales as Locale array to the interface. There is no
implementation that retrieves locales of the selection, we pass null
to the provider.

BUG=710505

Review-Url: https://codereview.chromium.org/2815833003
Cr-Commit-Position: refs/heads/master@{#464578}
Committed:
https://chromium.googlesource.com/chromium/src/+/0509fea9753de15947d230e379e2...
==========

[commit-bot: I haz the power, 2017-04-13 22:38:25]

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