[Field trial params] Getting field trial params by feature name in Java.

[Field trial params] Getting field trial params by feature name in Java.

This CL adds support to read field trial params in Java using the name
of the feature they are associated to. It also brings parsing values to
primitive types on par to what is available in c++ code.

In follow-up CLs I plan to 
 - extend ChromeFeatureList to support overriding param values from junit tests;
 - use the new functions from ntp_snippets java code.

BUG=689937
Review-Url: https://codereview.chromium.org/2686763003
Cr-Commit-Position: refs/heads/master@{#449952}
Committed: https://chromium.googlesource.com/chromium/src/+/abfc1c53a0963f4ac88a634895a5ac9afbb677eb

[jkrcal, 2017-02-08 11:18:21]

jkrcal@chromium.org changed reviewers:
+ asvitkine@chromium.org, bauerb@chromium.org

[jkrcal, 2017-02-08 11:18:22]

Bernhard, could you PTAL as the owner of android?
Alexei, could you PTAL from the perspective of field trials API?

[jkrcal, 2017-02-08 11:18:54]

Description was changed from

==========
[Field trial params] Getting field trial params by feature name in Java.

This CL adds support to read field trial params in Java using the name
of the feature they are associated to. It also brings parsing values to
primitive types on par to what is available in c++ code.

BUG=689937
==========

to

==========
[Field trial params] Getting field trial params by feature name in Java.

This CL adds support to read field trial params in Java using the name
of the feature they are associated to. It also brings parsing values to
primitive types on par to what is available in c++ code.

In follow-up CLs I plan to 
 - extend ChromeFeatureList to support overriding param values from junit tests;
 - use the new functions from ntp_snippets java code.

BUG=689937
==========

[Alexei Svitkine (slow), 2017-02-08 16:36:00]

https://codereview.chromium.org/2686763003/diff/1/chrome/browser/android/chro...
File chrome/browser/android/chrome_feature_list.cc (right):

https://codereview.chromium.org/2686763003/diff/1/chrome/browser/android/chro...
chrome/browser/android/chrome_feature_list.cc:169: for (size_t i = 0; i <
arraysize(kFeaturesExposedToJava); ++i) {
Nit: Since we loop over features in all of these functions, how about a helper
function to find the feature?

[jkrcal, 2017-02-09 10:00:45]

Thanks, PTAL again!

https://codereview.chromium.org/2686763003/diff/1/chrome/browser/android/chro...
File chrome/browser/android/chrome_feature_list.cc (right):

https://codereview.chromium.org/2686763003/diff/1/chrome/browser/android/chro...
chrome/browser/android/chrome_feature_list.cc:169: for (size_t i = 0; i <
arraysize(kFeaturesExposedToJava); ++i) {
On 2017/02/08 at 16:36:00, Alexei Svitkine (slow) wrote:
> Nit: Since we loop over features in all of these functions, how about a helper
function to find the feature?

Good point, done!

[Alexei Svitkine (slow), 2017-02-09 15:09:37]

lgtm

https://codereview.chromium.org/2686763003/diff/20001/chrome/browser/android/...
File chrome/browser/android/chrome_feature_list.cc (right):

https://codereview.chromium.org/2686763003/diff/20001/chrome/browser/android/...
chrome/browser/android/chrome_feature_list.cc:77: const base::Feature*
FindFeatureExposedToJava(const std::string feature_name) {
const std::string&

https://codereview.chromium.org/2686763003/diff/20001/chrome/browser/android/...
chrome/browser/android/chrome_feature_list.cc:166: if (feature != nullptr) {
Nit: No {}

[jkrcal, 2017-02-09 15:27:36]

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

[commit-bot: I haz the power, 2017-02-09 15:28:01]

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

[jkrcal, 2017-02-09 15:28:31]

Thanks!

Bernhard, could you PTAL?

[commit-bot: I haz the power, 2017-02-09 16:00:46]

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

[commit-bot: I haz the power, 2017-02-09 16:00:47]

Dry run: This issue passed the CQ dry run.

[Bernhard Bauer, 2017-02-09 17:04:29]

https://codereview.chromium.org/2686763003/diff/40001/chrome/browser/android/...
File chrome/browser/android/chrome_feature_list.cc (right):

https://codereview.chromium.org/2686763003/diff/40001/chrome/browser/android/...
chrome/browser/android/chrome_feature_list.cc:169: NOTREACHED();
The NOTREACHED() made sense in the old version where the compiler requires a
return value, but here you have a check, and then code that handles when the
check fails, which is not allowed per the style guide. Just DCHECK(feature) and
return base::FeatureList::IsEnabled(*feature).

[jkrcal, 2017-02-09 19:10:40]

Thanks, PTAL, again!

https://codereview.chromium.org/2686763003/diff/40001/chrome/browser/android/...
File chrome/browser/android/chrome_feature_list.cc (right):

https://codereview.chromium.org/2686763003/diff/40001/chrome/browser/android/...
chrome/browser/android/chrome_feature_list.cc:169: NOTREACHED();
On 2017/02/09 17:04:29, Bernhard Bauer wrote:
> The NOTREACHED() made sense in the old version where the compiler requires a
> return value, but here you have a check, and then code that handles when the
> check fails, which is not allowed per the style guide. Just DCHECK(feature)
and
> return base::FeatureList::IsEnabled(*feature).

Ah, sure! Thanks!

I've moved the DCHECK into the helper function to make it even crisper.

[Alexei Svitkine (slow), 2017-02-09 22:49:39]

lgtm

[Bernhard Bauer, 2017-02-10 16:27:07]

https://codereview.chromium.org/2686763003/diff/80001/chrome/browser/android/...
File chrome/browser/android/chrome_feature_list.cc (right):

https://codereview.chromium.org/2686763003/diff/80001/chrome/browser/android/...
chrome/browser/android/chrome_feature_list.cc:167:
FindFeatureExposedToJava(ConvertJavaStringToUTF8(env, jfeature_name));
You are passing the returned feature by value, but then you only keep a
reference to it... where would the actual feature object live? The old approach
with returning a pointer would work better. (And you don't actually have to care
about what happens if the method returns a nullptr, because you are explicitly
saying "this should never happen". Crashing from a null pointer dereference if
it does happen is totally fine.)

[jkrcal, 2017-02-10 16:53:44]

https://codereview.chromium.org/2686763003/diff/80001/chrome/browser/android/...
File chrome/browser/android/chrome_feature_list.cc (right):

https://codereview.chromium.org/2686763003/diff/80001/chrome/browser/android/...
chrome/browser/android/chrome_feature_list.cc:167:
FindFeatureExposedToJava(ConvertJavaStringToUTF8(env, jfeature_name));
On 2017/02/10 16:27:07, Bernhard Bauer wrote:
> You are passing the returned feature by value, but then you only keep a
> reference to it... where would the actual feature object live? The old
approach
> with returning a pointer would work better. (And you don't actually have to
care
> about what happens if the method returns a nullptr, because you are explicitly
> saying "this should never happen". Crashing from a null pointer dereference if
> it does happen is totally fine.)

Damn, I meant to return by reference. 

The point of the change was to make the contract clear - FindFeature will always
return a feature (or crash) so that the caller does not need to care. Do I
misunderstand the implicit contracts about returning "const type*" and returning
"const type&"?

Do you still prefer me returning by pointer? Why?

[Bernhard Bauer, 2017-02-10 17:56:37]

https://codereview.chromium.org/2686763003/diff/80001/chrome/browser/android/...
File chrome/browser/android/chrome_feature_list.cc (right):

https://codereview.chromium.org/2686763003/diff/80001/chrome/browser/android/...
chrome/browser/android/chrome_feature_list.cc:167:
FindFeatureExposedToJava(ConvertJavaStringToUTF8(env, jfeature_name));
On 2017/02/10 16:53:44, jkrcal wrote:
> On 2017/02/10 16:27:07, Bernhard Bauer wrote:
> > You are passing the returned feature by value, but then you only keep a
> > reference to it... where would the actual feature object live? The old
> approach
> > with returning a pointer would work better. (And you don't actually have to
> care
> > about what happens if the method returns a nullptr, because you are
explicitly
> > saying "this should never happen". Crashing from a null pointer dereference
if
> > it does happen is totally fine.)
> 
> Damn, I meant to return by reference. 
> 
> The point of the change was to make the contract clear - FindFeature will
always
> return a feature (or crash) so that the caller does not need to care. Do I
> misunderstand the implicit contracts about returning "const type*" and
returning
> "const type&"?
> 
> Do you still prefer me returning by pointer? Why?

I don't have a problem with returning a reference per se. The only issue is
going to be making the compiler happy in the case where the feature is not
found. If you return a reference to a temporary object on the stack, it's going
to be invalid once the method returns, and I do realize that I just said earlier
we shouldn't care about what happens if a DCHECK fails, but this would
potentially access invalid memory, which is security-critical, so avoiding that
takes precedence over style.

You could return a null reference (which can in fact exist, see
https://groups.google.com/a/chromium.org/d/msg/cxx/j5rFewBzSBQ/8I8-MJ_9AAAJ),
but that is undefined behavior, so it might trigger a warning if we're
sanitizing for that. Returning a pointer would avoid that, as we would never
have code that is guaranteed to dereference a null pointer, and given that you
control all the callers, you can just _make_ them not care :)

[jkrcal, 2017-02-13 07:57:27]

I hope I got it right! :)

(I am sorry about the minor rebase mixed-in; it is easy to separate from my
changes so I did not resubmit it separately.)

https://codereview.chromium.org/2686763003/diff/80001/chrome/browser/android/...
File chrome/browser/android/chrome_feature_list.cc (right):

https://codereview.chromium.org/2686763003/diff/80001/chrome/browser/android/...
chrome/browser/android/chrome_feature_list.cc:167:
FindFeatureExposedToJava(ConvertJavaStringToUTF8(env, jfeature_name));
On 2017/02/10 17:56:37, Bernhard Bauer wrote:
> On 2017/02/10 16:53:44, jkrcal wrote:
> > On 2017/02/10 16:27:07, Bernhard Bauer wrote:
> > > You are passing the returned feature by value, but then you only keep a
> > > reference to it... where would the actual feature object live? The old
> > approach
> > > with returning a pointer would work better. (And you don't actually have
to
> > care
> > > about what happens if the method returns a nullptr, because you are
> explicitly
> > > saying "this should never happen". Crashing from a null pointer
dereference
> if
> > > it does happen is totally fine.)
> > 
> > Damn, I meant to return by reference. 
> > 
> > The point of the change was to make the contract clear - FindFeature will
> always
> > return a feature (or crash) so that the caller does not need to care. Do I
> > misunderstand the implicit contracts about returning "const type*" and
> returning
> > "const type&"?
> > 
> > Do you still prefer me returning by pointer? Why?
> 
> I don't have a problem with returning a reference per se. The only issue is
> going to be making the compiler happy in the case where the feature is not
> found. If you return a reference to a temporary object on the stack, it's
going
> to be invalid once the method returns, and I do realize that I just said
earlier
> we shouldn't care about what happens if a DCHECK fails, but this would
> potentially access invalid memory, which is security-critical, so avoiding
that
> takes precedence over style.
> 
> You could return a null reference (which can in fact exist, see
> https://groups.google.com/a/chromium.org/d/msg/cxx/j5rFewBzSBQ/8I8-MJ_9AAAJ),
> but that is undefined behavior, so it might trigger a warning if we're
> sanitizing for that. Returning a pointer would avoid that, as we would never
> have code that is guaranteed to dereference a null pointer, and given that you
> control all the callers, you can just _make_ them not care :)

Oh, clear! Thanks for bearing with me and explaining in detail! That was really
helpful!

[Bernhard Bauer, 2017-02-13 14:00:30]

LGTM!

[jkrcal, 2017-02-13 14:04:33]

The CQ bit was checked by jkrcal@chromium.org

[jkrcal, 2017-02-13 14:04:33]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/2686763003/#ps100001
(title: "Bernhard's comments #2")

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

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-02-13 14:39:04]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1486994673411410,
"parent_rev": "82a74bea631238cfe4ef13ec8cd564bd01def533", "commit_rev":
"abfc1c53a0963f4ac88a634895a5ac9afbb677eb"}

[commit-bot: I haz the power, 2017-02-13 14:39:48]

Description was changed from

==========
[Field trial params] Getting field trial params by feature name in Java.

This CL adds support to read field trial params in Java using the name
of the feature they are associated to. It also brings parsing values to
primitive types on par to what is available in c++ code.

In follow-up CLs I plan to 
 - extend ChromeFeatureList to support overriding param values from junit tests;
 - use the new functions from ntp_snippets java code.

BUG=689937
==========

to

==========
[Field trial params] Getting field trial params by feature name in Java.

This CL adds support to read field trial params in Java using the name
of the feature they are associated to. It also brings parsing values to
primitive types on par to what is available in c++ code.

In follow-up CLs I plan to 
 - extend ChromeFeatureList to support overriding param values from junit tests;
 - use the new functions from ntp_snippets java code.

BUG=689937

Review-Url: https://codereview.chromium.org/2686763003
Cr-Commit-Position: refs/heads/master@{#449952}
Committed:
https://chromium.googlesource.com/chromium/src/+/abfc1c53a0963f4ac88a634895a5...
==========

[commit-bot: I haz the power, 2017-02-13 14:39:49]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/abfc1c53a0963f4ac88a634895a5...