[Android] Trigger native event batching through Feature API

[Android] Trigger native event batching through Feature API

Previous CL https://crrev.com/2710683003 added command line flag to disable
Android native batching. However it's hard to run Finch Trial with command line flags.

This CL created 'ContentFeatureList.java' (similar to 'ChromeFeatureList.java') as
suggested by isherman@ in go/finch-trial-on-command-line-flags, thus we can use
Feature API instead of command line flags.

BUG=625689
Review-Url: https://codereview.chromium.org/2755673005
Cr-Commit-Position: refs/heads/master@{#457540}
Committed: https://chromium.googlesource.com/chromium/src/+/8bdf2092362cd7cb60c38ce99c9ee8904d482b4e

[chongz, 2017-03-15 21:56:58]

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

[commit-bot: I haz the power, 2017-03-15 21:57:51]

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

[chongz, 2017-03-15 22:00:21]

Description was changed from

==========
Trigger RequestUnbufferedDispatch through Feature API

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

to

==========
[Android] Trigger native event batching through Feature API

Previous CL crrev/2710683003

BUG=625689
==========

[chongz, 2017-03-15 22:06:12]

Description was changed from

==========
[Android] Trigger native event batching through Feature API

Previous CL crrev/2710683003

BUG=625689
==========

to

==========
[Android] Trigger native event batching through Feature API

Previous CL https://crrev.com/2710683003 added command line flag to disable
Android native batching. However it's hard to run Finch Trial with command line
flags.

This CL created 'ContentFeatureList.java' (similar to 'ChromeFeatureList.java')
as
suggested by isherman@ in
https://groups.google.com/a/google.com/d/msg/finch-users/5yS45pMsD0Q/XPY8AwxF...,
thus we can use Feature API instead of
command line flags.

BUG=625689
==========

[chongz, 2017-03-15 22:07:52]

Description was changed from

==========
[Android] Trigger native event batching through Feature API

Previous CL https://crrev.com/2710683003 added command line flag to disable
Android native batching. However it's hard to run Finch Trial with command line
flags.

This CL created 'ContentFeatureList.java' (similar to 'ChromeFeatureList.java')
as
suggested by isherman@ in
https://groups.google.com/a/google.com/d/msg/finch-users/5yS45pMsD0Q/XPY8AwxF...,
thus we can use Feature API instead of
command line flags.

BUG=625689
==========

to

==========
[Android] Trigger native event batching through Feature API

Previous CL https://crrev.com/2710683003 added command line flag to disable
Android native batching. However it's hard to run Finch Trial with command line
flags.

This CL created 'ContentFeatureList.java' (similar to 'ChromeFeatureList.java')
as
suggested by isherman@ in go/questions-about-finch-trial-on-command-line-flags,
thus we can use Feature API instead of
command line flags.

BUG=625689
==========

[chongz, 2017-03-15 22:09:11]

Description was changed from

==========
[Android] Trigger native event batching through Feature API

Previous CL https://crrev.com/2710683003 added command line flag to disable
Android native batching. However it's hard to run Finch Trial with command line
flags.

This CL created 'ContentFeatureList.java' (similar to 'ChromeFeatureList.java')
as
suggested by isherman@ in go/questions-about-finch-trial-on-command-line-flags,
thus we can use Feature API instead of
command line flags.

BUG=625689
==========

to

==========
[Android] Trigger native event batching through Feature API

Previous CL https://crrev.com/2710683003 added command line flag to disable
Android native batching. However it's hard to run Finch Trial with command line
flags.

This CL created 'ContentFeatureList.java' (similar to 'ChromeFeatureList.java')
as
suggested by isherman@ in go/finch-trial-on-command-line-flags, thus we can use
Feature API instead of
command line flags.

BUG=625689
==========

[chongz, 2017-03-15 22:10:09]

Description was changed from

==========
[Android] Trigger native event batching through Feature API

Previous CL https://crrev.com/2710683003 added command line flag to disable
Android native batching. However it's hard to run Finch Trial with command line
flags.

This CL created 'ContentFeatureList.java' (similar to 'ChromeFeatureList.java')
as
suggested by isherman@ in go/finch-trial-on-command-line-flags, thus we can use
Feature API instead of
command line flags.

BUG=625689
==========

to

==========
[Android] Trigger native event batching through Feature API

Previous CL https://crrev.com/2710683003 added command line flag to disable
Android native batching. However it's hard to run Finch Trial with command line
flags.

This CL created 'ContentFeatureList.java' (similar to 'ChromeFeatureList.java')
as
suggested by isherman@ in go/finch-trial-on-command-line-flags, thus we can use
Feature API instead of command line flags.

BUG=625689
==========

[commit-bot: I haz the power, 2017-03-15 23:20:52]

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

[commit-bot: I haz the power, 2017-03-15 23:20:53]

Dry run: This issue passed the CQ dry run.

[chongz, 2017-03-16 00:39:21]

chongz@chromium.org changed reviewers:
+ isherman@chromium.org

[chongz, 2017-03-16 00:39:22]

isherman@ PTAL, thanks!

[Ilya Sherman, 2017-03-16 03:39:50]

Thanks!  LGTM % inline comments.  In particular, please make sure to check with
a reviewer more familiar with Java what they'd recommend in terms of sharing
code with the ChromeFeatureList implementation.

https://codereview.chromium.org/2755673005/diff/1/content/browser/android/con...
File content/browser/android/content_feature_list.cc (right):

https://codereview.chromium.org/2755673005/diff/1/content/browser/android/con...
content/browser/android/content_feature_list.cc:22: // in other locations in the
code base (e.g. chrome/, components/, etc).
nit: Probably not in chrome/ nor components/ ;-)

https://codereview.chromium.org/2755673005/diff/1/content/browser/android/con...
content/browser/android/content_feature_list.cc:33: "|kFeaturesExposedToJava|.";
nit: Could you please move this string into a comment?  There's no need to
compile it into the binary.  Alternately, we could make this message more useful
by printing the name of the queried feature that isn't present in the list.

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

https://codereview.chromium.org/2755673005/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/ContentFeatureList.java:15:
public abstract class ContentFeatureList {
Hmm, I think it would be nice to provide a more full implementation, to match
what ChromeFeatureList offers.  I'd definitely encourage you to write tests for
your feature, which means you'll likely want a way to explicitly enable or
disable the feature for tests.

But, I'm realizing now that the ChromeFeatureList implementation is all static
methods... I'm not sure what a good way to share the implementation details
would be.  Something something generics?  Please check with someone who's more
familiar with Java best-practices as to what they'd recommend.

[chongz, 2017-03-16 15:34:45]

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

[commit-bot: I haz the power, 2017-03-16 15:35:20]

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

[chongz, 2017-03-16 16:12:21]

chongz@chromium.org changed reviewers:
+ agrieve@chromium.org

[chongz, 2017-03-16 16:12:22]

agrieve@ Can you take a look at this CL? Thanks!

https://codereview.chromium.org/2755673005/diff/1/content/browser/android/con...
File content/browser/android/content_feature_list.cc (right):

https://codereview.chromium.org/2755673005/diff/1/content/browser/android/con...
content/browser/android/content_feature_list.cc:22: // in other locations in the
code base (e.g. chrome/, components/, etc).
On 2017/03/16 03:39:49, Ilya Sherman wrote:
> nit: Probably not in chrome/ nor components/ ;-)

Done.

https://codereview.chromium.org/2755673005/diff/1/content/browser/android/con...
content/browser/android/content_feature_list.cc:33: "|kFeaturesExposedToJava|.";
On 2017/03/16 03:39:49, Ilya Sherman wrote:
> nit: Could you please move this string into a comment?  There's no need to
> compile it into the binary.  Alternately, we could make this message more
useful
> by printing the name of the queried feature that isn't present in the list.

Done.

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

https://codereview.chromium.org/2755673005/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/ContentFeatureList.java:15:
public abstract class ContentFeatureList {
On 2017/03/16 03:39:49, Ilya Sherman wrote:
> Hmm, I think it would be nice to provide a more full implementation, to match
> what ChromeFeatureList offers.  I'd definitely encourage you to write tests
for
> your feature, which means you'll likely want a way to explicitly enable or
> disable the feature for tests.

My feature is about Android native event batching, and I'm not sure if there is
a way to simulate it other than checking if the flag was set...

Also I'm not sure about Android best-practices - I thought I should only add
APIs when I need them, no?

> 
> But, I'm realizing now that the ChromeFeatureList implementation is all static
> methods... I'm not sure what a good way to share the implementation details
> would be.  Something something generics?  Please check with someone who's more
> familiar with Java best-practices as to what they'd recommend.

agrieve@ Can I have some suggestions on the recommended pattern? Thanks!

FYI: There seems to be efforts on moving the implementation into //base but
failed due to access issue.
https://codereview.chromium.org/1538573004/diff/180001/chrome/android/java/sr...

[commit-bot: I haz the power, 2017-03-16 16:26:23]

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

[commit-bot: I haz the power, 2017-03-16 16:26:24]

Dry run: This issue passed the CQ dry run.

[agrieve, 2017-03-16 17:07:59]

On 2017/03/16 16:12:22, chongz wrote:
> agrieve@ Can you take a look at this CL? Thanks!
> 
>
https://codereview.chromium.org/2755673005/diff/1/content/browser/android/con...
> File content/browser/android/content_feature_list.cc (right):
> 
>
https://codereview.chromium.org/2755673005/diff/1/content/browser/android/con...
> content/browser/android/content_feature_list.cc:22: // in other locations in
the
> code base (e.g. chrome/, components/, etc).
> On 2017/03/16 03:39:49, Ilya Sherman wrote:
> > nit: Probably not in chrome/ nor components/ ;-)
> 
> Done.
> 
>
https://codereview.chromium.org/2755673005/diff/1/content/browser/android/con...
> content/browser/android/content_feature_list.cc:33:
"|kFeaturesExposedToJava|.";
> On 2017/03/16 03:39:49, Ilya Sherman wrote:
> > nit: Could you please move this string into a comment?  There's no need to
> > compile it into the binary.  Alternately, we could make this message more
> useful
> > by printing the name of the queried feature that isn't present in the list.
> 
> Done.
> 
>
https://codereview.chromium.org/2755673005/diff/1/content/public/android/java...
> File
>
content/public/android/java/src/org/chromium/content/browser/ContentFeatureList.java
> (right):
> 
>
https://codereview.chromium.org/2755673005/diff/1/content/public/android/java...
>
content/public/android/java/src/org/chromium/content/browser/ContentFeatureList.java:15:
> public abstract class ContentFeatureList {
> On 2017/03/16 03:39:49, Ilya Sherman wrote:
> > Hmm, I think it would be nice to provide a more full implementation, to
match
> > what ChromeFeatureList offers.  I'd definitely encourage you to write tests
> for
> > your feature, which means you'll likely want a way to explicitly enable or
> > disable the feature for tests.
> 
> My feature is about Android native event batching, and I'm not sure if there
is
> a way to simulate it other than checking if the flag was set...
> 
> Also I'm not sure about Android best-practices - I thought I should only add
> APIs when I need them, no?
> 
> > 
> > But, I'm realizing now that the ChromeFeatureList implementation is all
static
> > methods... I'm not sure what a good way to share the implementation details
> > would be.  Something something generics?  Please check with someone who's
more
> > familiar with Java best-practices as to what they'd recommend.
> 
> agrieve@ Can I have some suggestions on the recommended pattern? Thanks!
> 
> FYI: There seems to be efforts on moving the implementation into //base but
> failed due to access issue.
>
https://codereview.chromium.org/1538573004/diff/180001/chrome/android/java/sr...

Rather than duplicate the JNI call in ChromeFeatureList.java, I wonder if it
would be better to move them all to base as:
//base/android/java/src/org/chromium/base/FeatureList.java

But still keep all of the string constants where they are.

wdyt?

[Ilya Sherman, 2017-03-16 17:34:45]

On 2017/03/16 17:07:59, agrieve wrote:
> Rather than duplicate the JNI call in ChromeFeatureList.java, I wonder if it
> would be better to move them all to base as:
> //base/android/java/src/org/chromium/base/FeatureList.java
> 
> But still keep all of the string constants where they are.
> 
> wdyt?

I think that would be nice, but it's not clear to me exactly how that would be
implemented.  In the current implementation, the native IsEnabled API call loops
over an array of known features, specific to the layer.  Did you have an idea
for how to share these native implementations?

[Ilya Sherman, 2017-03-16 17:38:53]

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

https://codereview.chromium.org/2755673005/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/ContentFeatureList.java:15:
public abstract class ContentFeatureList {
On 2017/03/16 16:12:21, chongz wrote:
> On 2017/03/16 03:39:49, Ilya Sherman wrote:
> > Hmm, I think it would be nice to provide a more full implementation, to
match
> > what ChromeFeatureList offers.  I'd definitely encourage you to write tests
> for
> > your feature, which means you'll likely want a way to explicitly enable or
> > disable the feature for tests.
> 
> My feature is about Android native event batching, and I'm not sure if there
is
> a way to simulate it other than checking if the flag was set...

Hmm, it's not very clear to me exactly what you mean.  Could you please point me
to the code that tests the feature?  In particular, there is probably some code
that calls into your feature -- do you do anything to test that that call works
correctly?

> Also I'm not sure about Android best-practices - I thought I should only add
> APIs when I need them, no?

I agree that if we're going to be more or less copy/pasting the implementation,
then it's not appropriate to add more features at this time.  I'm just hoping
that we can somehow share the implementations.

https://codereview.chromium.org/2755673005/diff/20001/content/browser/android...
File content/browser/android/content_feature_list.cc (right):

https://codereview.chromium.org/2755673005/diff/20001/content/browser/android...
content/browser/android/content_feature_list.cc:33: << feature_name;
Thanks!  Would you mind similarly updating the chrome_feature_list
implementation as well?

https://codereview.chromium.org/2755673005/diff/20001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ContentFeatureList.java
(right):

https://codereview.chromium.org/2755673005/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentFeatureList.java:15:
public class ContentFeatureList {
Hmm, why did you remove "abstract" here?  Should it be removed from
ChromeFeatureList as well?

[agrieve, 2017-03-16 17:54:50]

On 2017/03/16 17:34:45, Ilya Sherman wrote:
> On 2017/03/16 17:07:59, agrieve wrote:
> > Rather than duplicate the JNI call in ChromeFeatureList.java, I wonder if it
> > would be better to move them all to base as:
> > //base/android/java/src/org/chromium/base/FeatureList.java
> > 
> > But still keep all of the string constants where they are.
> > 
> > wdyt?
> 
> I think that would be nice, but it's not clear to me exactly how that would be
> implemented.  In the current implementation, the native IsEnabled API call
loops
> over an array of known features, specific to the layer.  Did you have an idea
> for how to share these native implementations?

Ah, of course, that's the part I missed :P. If we were willing to duplicate the
default value in java, then the JNI could just construct & pass in a temporary
Feature& (they are just a string and a default value). I doubt it's worth the
risk of duplicating this value though, and we're not talking about a large
number of features, so lgtm.

[chongz, 2017-03-16 18:12:53]

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

[commit-bot: I haz the power, 2017-03-16 18:13:29]

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

[chongz, 2017-03-16 18:54:04]

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

https://codereview.chromium.org/2755673005/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/ContentFeatureList.java:15:
public abstract class ContentFeatureList {
On 2017/03/16 17:38:53, Ilya Sherman wrote:
> On 2017/03/16 16:12:21, chongz wrote:
> > On 2017/03/16 03:39:49, Ilya Sherman wrote:
> > > Hmm, I think it would be nice to provide a more full implementation, to
> match
> > > what ChromeFeatureList offers.  I'd definitely encourage you to write
tests
> > for
> > > your feature, which means you'll likely want a way to explicitly enable or
> > > disable the feature for tests.
> > 
> > My feature is about Android native event batching, and I'm not sure if there
> is
> > a way to simulate it other than checking if the flag was set...
> 
> Hmm, it's not very clear to me exactly what you mean.  Could you please point
me
> to the code that tests the feature?  In particular, there is probably some
code
> that calls into your feature -- do you do anything to test that that call
works
> correctly?

Sure. Basically everything about this feature is "Call
|View.requestUnbufferedDispatch()| => Android will dispatch events to Clank at a
higher frequency".
There is no tests and I can't think of a way to check if Android handles
|View.requestUnbufferedDispatch()| correctly or simulate an user scroll and see
if we indeed receive events at a higher frequency...
See:
https://codereview.chromium.org/2710683003/diff/40001/content/public/android/...


Or are you suggesting that I should check if the feature flag was passed in and
successfully received?

> > 
> > > Also I'm not sure about Android best-practices - I thought I should only
add
> > > APIs when I need them, no?
> > 
> > I agree that if we're going to be more or less copy/pasting the
implementation,
> > then it's not appropriate to add more features at this time.  I'm just
hoping
> > that we can somehow share the implementations.
> agrieve@chromium.org:
> Ah, of course, that's the part I missed :P. If we were willing to duplicate
the
> default value in java, then the JNI could just construct & pass in a temporary
> Feature& (they are just a string and a default value). I doubt it's worth the
> risk of duplicating this value though, and we're not talking about a large
> number of features, so lgtm.

I'm guessing it means we can keep things as it is until a better solution was
found?

https://codereview.chromium.org/2755673005/diff/20001/content/browser/android...
File content/browser/android/content_feature_list.cc (right):

https://codereview.chromium.org/2755673005/diff/20001/content/browser/android...
content/browser/android/content_feature_list.cc:33: << feature_name;
On 2017/03/16 17:38:53, Ilya Sherman wrote:
> Thanks!  Would you mind similarly updating the chrome_feature_list
> implementation as well?

Done.

https://codereview.chromium.org/2755673005/diff/20001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ContentFeatureList.java
(right):

https://codereview.chromium.org/2755673005/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentFeatureList.java:15:
public class ContentFeatureList {
On 2017/03/16 17:38:53, Ilya Sherman wrote:
> Hmm, why did you remove "abstract" here?  Should it be removed from
> ChromeFeatureList as well?

I'm not an expert on Java but I don't feel that we should have "abstract" here
as it doesn't have any abstract methods. Also I compared with some of other
classes with JNI bindings (e.g. ImeAdapter.java) and they don't have "abstract".

https://cs.chromium.org/chromium/src/content/public/android/java/src/org/chro...

I've removed "abstract" from ChromeFeatureList as well.

[Ilya Sherman, 2017-03-16 18:59:12]

LGTM, thanks for implementing this!

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

https://codereview.chromium.org/2755673005/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/ContentFeatureList.java:15:
public abstract class ContentFeatureList {
On 2017/03/16 18:54:04, chongz wrote:
> On 2017/03/16 17:38:53, Ilya Sherman wrote:
> > On 2017/03/16 16:12:21, chongz wrote:
> > > On 2017/03/16 03:39:49, Ilya Sherman wrote:
> > > > Hmm, I think it would be nice to provide a more full implementation, to
> > match
> > > > what ChromeFeatureList offers.  I'd definitely encourage you to write
> tests
> > > for
> > > > your feature, which means you'll likely want a way to explicitly enable
or
> > > > disable the feature for tests.
> > > 
> > > My feature is about Android native event batching, and I'm not sure if
there
> > is
> > > a way to simulate it other than checking if the flag was set...
> > 
> > Hmm, it's not very clear to me exactly what you mean.  Could you please
point
> me
> > to the code that tests the feature?  In particular, there is probably some
> code
> > that calls into your feature -- do you do anything to test that that call
> works
> > correctly?
> 
> Sure. Basically everything about this feature is "Call
> |View.requestUnbufferedDispatch()| => Android will dispatch events to Clank at
a
> higher frequency".
> There is no tests and I can't think of a way to check if Android handles
> |View.requestUnbufferedDispatch()| correctly or simulate an user scroll and
see
> if we indeed receive events at a higher frequency...
> See:
>
https://codereview.chromium.org/2710683003/diff/40001/content/public/android/...
> 
> 
> Or are you suggesting that I should check if the feature flag was passed in
and
> successfully received?

Ah, I think I see what you're saying now.  Okay, I suppose that makes sense. 
Thanks for the explanation!

> > > > Also I'm not sure about Android best-practices - I thought I should only
> add
> > > > APIs when I need them, no?
> > > 
> > > I agree that if we're going to be more or less copy/pasting the
> implementation,
> > > then it's not appropriate to add more features at this time.  I'm just
> hoping
> > > that we can somehow share the implementations.
> > mailto:agrieve@chromium.org:
> > Ah, of course, that's the part I missed :P. If we were willing to duplicate
> the
> > default value in java, then the JNI could just construct & pass in a
temporary
> > Feature& (they are just a string and a default value). I doubt it's worth
the
> > risk of duplicating this value though, and we're not talking about a large
> > number of features, so lgtm.
> 
> I'm guessing it means we can keep things as it is until a better solution was
> found? 

Yep, sounds reasonable for now.

https://codereview.chromium.org/2755673005/diff/20001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ContentFeatureList.java
(right):

https://codereview.chromium.org/2755673005/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentFeatureList.java:15:
public class ContentFeatureList {
On 2017/03/16 18:54:04, chongz wrote:
> On 2017/03/16 17:38:53, Ilya Sherman wrote:
> > Hmm, why did you remove "abstract" here?  Should it be removed from
> > ChromeFeatureList as well?
> 
> I'm not an expert on Java but I don't feel that we should have "abstract" here
> as it doesn't have any abstract methods. Also I compared with some of other
> classes with JNI bindings (e.g. ImeAdapter.java) and they don't have
"abstract".
> 
>
https://cs.chromium.org/chromium/src/content/public/android/java/src/org/chro...
> 
> I've removed "abstract" from ChromeFeatureList as well.

Okay.  I suspect it was to help ensure that nobody tried to instantiate the
class, but I suppose the private constructor accomplishes that too.

[chongz, 2017-03-16 19:08:01]

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

[commit-bot: I haz the power, 2017-03-16 19:08:20]

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

[chongz, 2017-03-16 19:16:53]

chongz@chromium.org changed reviewers:
+ avi@chromium.org

[chongz, 2017-03-16 19:16:54]

avi@ PTAL, thanks!

https://codereview.chromium.org/2755673005/diff/20001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ContentFeatureList.java
(right):

https://codereview.chromium.org/2755673005/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentFeatureList.java:15:
public class ContentFeatureList {
On 2017/03/16 18:59:12, Ilya Sherman wrote:
> On 2017/03/16 18:54:04, chongz wrote:
> > On 2017/03/16 17:38:53, Ilya Sherman wrote:
> > > Hmm, why did you remove "abstract" here?  Should it be removed from
> > > ChromeFeatureList as well?
> > 
> > I'm not an expert on Java but I don't feel that we should have "abstract"
here
> > as it doesn't have any abstract methods. Also I compared with some of other
> > classes with JNI bindings (e.g. ImeAdapter.java) and they don't have
> "abstract".
> > 
> >
>
https://cs.chromium.org/chromium/src/content/public/android/java/src/org/chro...
> > 
> > I've removed "abstract" from ChromeFeatureList as well.
> 
> Okay.  I suspect it was to help ensure that nobody tried to instantiate the
> class, but I suppose the private constructor accomplishes that too.

Oh I see, that sounds reasonable. Anyway I've added back both "abstract" to
maintain the original style.

[Avi (use Gerrit), 2017-03-16 19:19:42]

lgtm

stampity stamp

[commit-bot: I haz the power, 2017-03-16 20:17:56]

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

[commit-bot: I haz the power, 2017-03-16 20:17:58]

Dry run: This issue passed the CQ dry run.

[chongz, 2017-03-16 20:31:40]

The CQ bit was checked by chongz@chromium.org

[chongz, 2017-03-16 20:31:41]

The patchset sent to the CQ was uploaded after l-g-t-m from
agrieve@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2755673005/#ps60001
(title: "Add back "abstract"")

[commit-bot: I haz the power, 2017-03-16 20:33:15]

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-03-16 20:44:09]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1489696300695880,
"parent_rev": "1b1306a03f9c9650c23d2b0d3f48ce1a3282da0d", "commit_rev":
"8bdf2092362cd7cb60c38ce99c9ee8904d482b4e"}

[commit-bot: I haz the power, 2017-03-16 20:44:47]

Description was changed from

==========
[Android] Trigger native event batching through Feature API

Previous CL https://crrev.com/2710683003 added command line flag to disable
Android native batching. However it's hard to run Finch Trial with command line
flags.

This CL created 'ContentFeatureList.java' (similar to 'ChromeFeatureList.java')
as
suggested by isherman@ in go/finch-trial-on-command-line-flags, thus we can use
Feature API instead of command line flags.

BUG=625689
==========

to

==========
[Android] Trigger native event batching through Feature API

Previous CL https://crrev.com/2710683003 added command line flag to disable
Android native batching. However it's hard to run Finch Trial with command line
flags.

This CL created 'ContentFeatureList.java' (similar to 'ChromeFeatureList.java')
as
suggested by isherman@ in go/finch-trial-on-command-line-flags, thus we can use
Feature API instead of command line flags.

BUG=625689

Review-Url: https://codereview.chromium.org/2755673005
Cr-Commit-Position: refs/heads/master@{#457540}
Committed:
https://chromium.googlesource.com/chromium/src/+/8bdf2092362cd7cb60c38ce99c9e...
==========

[commit-bot: I haz the power, 2017-03-16 20:44:49]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/8bdf2092362cd7cb60c38ce99c9e...