Removed callbacks to JNI functions + added gzip compressed seed support & pulling response time

Removed callbacks to JNI functions to simplify code structure since they are no longer needed.

Also added gzip compressed seed support and pulling the actual time of HTTP response.

BUG=551029
Committed: https://crrev.com/ff14aff0758577956d4704050ef3d546a9bac624
Cr-Commit-Position: refs/heads/master@{#360292}

[Alexander Agulenko, 2015-11-12 03:20:53]

Description was changed from

==========
Removed callbacks to JNI functions to simplify code structure since they are no
longer needed

BUG=551029
==========

to

==========
Removed callbacks to JNI functions to simplify code structure since they are no
longer needed

BUG=551029
==========

[Alexander Agulenko, 2015-11-12 03:20:53]

agulenko@google.com changed reviewers:
+ asvitkine@chromium.org, holte@chromium.org

[Alexei Svitkine (slow), 2015-11-12 16:26:55]

https://codereview.chromium.org/1438123002/diff/40001/chrome/browser/metrics/...
File chrome/browser/metrics/variations/chrome_variations_service_client.h
(left):

https://codereview.chromium.org/1438123002/diff/40001/chrome/browser/metrics/...
chrome/browser/metrics/variations/chrome_variations_service_client.h:8: #include
<string>
Keep this. It's still used on line 28 and elsewhere.

https://codereview.chromium.org/1438123002/diff/40001/components/variations/a...
File
components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java
(right):

https://codereview.chromium.org/1438123002/diff/40001/components/variations/a...
components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java:65:
boolean isGzipCompressed = (compression.indexOf("gzip") != -1);
This logic doesn't match the C++ side.

You want to split on ",", iterate and trim each entry and compare. Otherwise,
this would match e.g. "gzip-foo". How about just passing the string value of
this header to the C++ side and having the C++ code use the same logic as it
currently does to parse it.

https://codereview.chromium.org/1438123002/diff/40001/components/variations/a...
File components/variations/android/variations_seed_bridge.cc (right):

https://codereview.chromium.org/1438123002/diff/40001/components/variations/a...
components/variations/android/variations_seed_bridge.cc:61:
base::Time::FromString(ConvertJavaStringToUTF8(env, j_response_time).c_str(),
I think this logic should be in the variations code. This file should just plumb
things without extra logic.

https://codereview.chromium.org/1438123002/diff/40001/components/variations/v...
File components/variations/variations_seed_store.cc (left):

https://codereview.chromium.org/1438123002/diff/40001/components/variations/v...
components/variations/variations_seed_store.cc:146:
FIRST_RUN_SEED_IMPORT_FAIL_NO_CALLBACK,
Keep this entry.

https://codereview.chromium.org/1438123002/diff/40001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1438123002/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:75736: +  <int value="2" label="Seed
import failed - failed to store seed"/>
Histograms shouldn't be changed. Keep the old entry but mark it as "(obsolete)".

[Alexander Agulenko, 2015-11-12 20:06:42]

https://codereview.chromium.org/1438123002/diff/40001/chrome/browser/metrics/...
File chrome/browser/metrics/variations/chrome_variations_service_client.h
(left):

https://codereview.chromium.org/1438123002/diff/40001/chrome/browser/metrics/...
chrome/browser/metrics/variations/chrome_variations_service_client.h:8: #include
<string>
On 2015/11/12 16:26:55, Alexei Svitkine (slow) wrote:
> Keep this. It's still used on line 28 and elsewhere.

I've just removed the include that I have added to this file before, when I
implemented GetVariationsFirstRunSeedCallback(). The code successfully builds
even without this include, but anyway, I do not mind returning it back.

Done.

https://codereview.chromium.org/1438123002/diff/40001/components/variations/v...
File components/variations/variations_seed_store.cc (left):

https://codereview.chromium.org/1438123002/diff/40001/components/variations/v...
components/variations/variations_seed_store.cc:146:
FIRST_RUN_SEED_IMPORT_FAIL_NO_CALLBACK,
On 2015/11/12 16:26:55, Alexei Svitkine (slow) wrote:
> Keep this entry.

Done.

https://codereview.chromium.org/1438123002/diff/40001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1438123002/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:75736: +  <int value="2" label="Seed
import failed - failed to store seed"/>
On 2015/11/12 16:26:55, Alexei Svitkine (slow) wrote:
> Histograms shouldn't be changed. Keep the old entry but mark it as
"(obsolete)".

Done.

[Alexander Agulenko, 2015-11-12 20:07:42]

https://codereview.chromium.org/1438123002/diff/40001/components/variations/a...
File
components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java
(right):

https://codereview.chromium.org/1438123002/diff/40001/components/variations/a...
components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java:65:
boolean isGzipCompressed = (compression.indexOf("gzip") != -1);
On 2015/11/12 16:26:55, Alexei Svitkine (slow) wrote:
> This logic doesn't match the C++ side.
> 
> You want to split on ",", iterate and trim each entry and compare. Otherwise,
> this would match e.g. "gzip-foo". How about just passing the string value of
> this header to the C++ side and having the C++ code use the same logic as it
> currently does to parse it.

Done.

https://codereview.chromium.org/1438123002/diff/40001/components/variations/a...
File components/variations/android/variations_seed_bridge.cc (right):

https://codereview.chromium.org/1438123002/diff/40001/components/variations/a...
components/variations/android/variations_seed_bridge.cc:61:
base::Time::FromString(ConvertJavaStringToUTF8(env, j_response_time).c_str(),
On 2015/11/12 16:26:55, Alexei Svitkine (slow) wrote:
> I think this logic should be in the variations code. This file should just
plumb
> things without extra logic.

Acknowledged (still working on it).

[Alexander Agulenko, 2015-11-12 20:15:41]

https://codereview.chromium.org/1438123002/diff/40001/components/variations/a...
File components/variations/android/variations_seed_bridge.cc (right):

https://codereview.chromium.org/1438123002/diff/40001/components/variations/a...
components/variations/android/variations_seed_bridge.cc:61:
base::Time::FromString(ConvertJavaStringToUTF8(env, j_response_time).c_str(),
On 2015/11/12 16:26:55, Alexei Svitkine (slow) wrote:
> I think this logic should be in the variations code. This file should just
plumb
> things without extra logic.

Done.

[Alexei Svitkine (slow), 2015-11-12 20:30:30]

https://codereview.chromium.org/1438123002/diff/80001/components/variations/a...
File
components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java
(right):

https://codereview.chromium.org/1438123002/diff/80001/components/variations/a...
components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java:71:
}
Nit: Remove this logging.

https://codereview.chromium.org/1438123002/diff/80001/components/variations/a...
components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java:86:
List<String> values = Arrays.asList(header.split(","));
Nit: No need for asList()

You can use Arrays.indexOf(). However, I think there may be spaces around it, so
I think you need to manually iterate and trim() the entries before comparing.

[Alexander Agulenko, 2015-11-12 21:47:39]

https://codereview.chromium.org/1438123002/diff/80001/components/variations/a...
File
components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java
(right):

https://codereview.chromium.org/1438123002/diff/80001/components/variations/a...
components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java:71:
}
On 2015/11/12 20:30:30, Alexei Svitkine (slow) wrote:
> Nit: Remove this logging.

Sorry, forgot about it before uploading.

Done.

https://codereview.chromium.org/1438123002/diff/80001/components/variations/a...
components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java:86:
List<String> values = Arrays.asList(header.split(","));
On 2015/11/12 20:30:30, Alexei Svitkine (slow) wrote:
> Nit: No need for asList()
> 
> You can use Arrays.indexOf(). However, I think there may be spaces around it,
so
> I think you need to manually iterate and trim() the entries before comparing.

Hmm, I did not find any standard Java library method to find an element in
unsorted array, that's why I used conversion to List. Are you sure that
Arrays.indexOf() exists?

[Alexander Agulenko, 2015-11-12 22:06:13]

https://codereview.chromium.org/1438123002/diff/80001/components/variations/a...
File
components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java
(right):

https://codereview.chromium.org/1438123002/diff/80001/components/variations/a...
components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java:86:
List<String> values = Arrays.asList(header.split(","));
On 2015/11/12 20:30:30, Alexei Svitkine (slow) wrote:
> Nit: No need for asList()
> 
> You can use Arrays.indexOf(). However, I think there may be spaces around it,
so
> I think you need to manually iterate and trim() the entries before comparing.

Done.

[Alexei Svitkine (slow), 2015-11-12 22:25:29]

https://codereview.chromium.org/1438123002/diff/120001/components/variations/...
File
components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java
(right):

https://codereview.chromium.org/1438123002/diff/120001/components/variations/...
components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java:86:
if (imCount != values.length) {
Hmm, so thinking about this more - if there's anything other than a single
"gzip" entry, then we shouldn't support it.

So how about then just:

boolean isGzipCompressed = header.trim().equals("gzip");

If you check for equality (after trimming) explicitly, then it won't match other
cases - which is fine, because if there's other comma-separated things there,
then we can't decode it anyway!

https://codereview.chromium.org/1438123002/diff/120001/components/variations/...
File components/variations/android/variations_seed_bridge.cc (right):

https://codereview.chromium.org/1438123002/diff/120001/components/variations/...
components/variations/android/variations_seed_bridge.cc:55:
ScopedJavaLocalRef<jstring> j_response_time =
Nit: use "date" terminology throughout. Right now, you mix date and time.

https://codereview.chromium.org/1438123002/diff/120001/components/variations/...
File components/variations/variations_seed_store.cc (right):

https://codereview.chromium.org/1438123002/diff/120001/components/variations/...
components/variations/variations_seed_store.cc:15: #include
"components/variations/android/variations_seed_bridge.h"
Put this include in ifdef and put it below the other includes.

Since it includes <jni.h> it won't compile on other platforms.

https://codereview.chromium.org/1438123002/diff/120001/components/variations/...
components/variations/variations_seed_store.cc:369: bool is_gzip_compressed;
Move this right above the time parsing code.

https://codereview.chromium.org/1438123002/diff/120001/components/variations/...
components/variations/variations_seed_store.cc:377:
base::Time::FromString(response_time.c_str(), &current_time);
To match how we do it on the C++ side, this should be Time::FromUTCString().

https://code.google.com/p/chromium/codesearch#chromium/src/net/http/http_resp...

[Alexander Agulenko, 2015-11-12 22:37:55]

https://codereview.chromium.org/1438123002/diff/120001/components/variations/...
File
components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java
(right):

https://codereview.chromium.org/1438123002/diff/120001/components/variations/...
components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java:86:
if (imCount != values.length) {
On 2015/11/12 22:25:29, Alexei Svitkine (slow) wrote:
> Hmm, so thinking about this more - if there's anything other than a single
> "gzip" entry, then we shouldn't support it.
> 
> So how about then just:
> 
> boolean isGzipCompressed = header.trim().equals("gzip");
> 
> If you check for equality (after trimming) explicitly, then it won't match
other
> cases - which is fine, because if there's other comma-separated things there,
> then we can't decode it anyway!

Done.

https://codereview.chromium.org/1438123002/diff/120001/components/variations/...
File components/variations/android/variations_seed_bridge.cc (right):

https://codereview.chromium.org/1438123002/diff/120001/components/variations/...
components/variations/android/variations_seed_bridge.cc:55:
ScopedJavaLocalRef<jstring> j_response_time =
On 2015/11/12 22:25:29, Alexei Svitkine (slow) wrote:
> Nit: use "date" terminology throughout. Right now, you mix date and time.

Done.

https://codereview.chromium.org/1438123002/diff/120001/components/variations/...
File components/variations/variations_seed_store.cc (right):

https://codereview.chromium.org/1438123002/diff/120001/components/variations/...
components/variations/variations_seed_store.cc:15: #include
"components/variations/android/variations_seed_bridge.h"
On 2015/11/12 22:25:29, Alexei Svitkine (slow) wrote:
> Put this include in ifdef and put it below the other includes.
> 
> Since it includes <jni.h> it won't compile on other platforms.

Done.

https://codereview.chromium.org/1438123002/diff/120001/components/variations/...
components/variations/variations_seed_store.cc:369: bool is_gzip_compressed;
On 2015/11/12 22:25:29, Alexei Svitkine (slow) wrote:
> Move this right above the time parsing code.

Done.

https://codereview.chromium.org/1438123002/diff/120001/components/variations/...
components/variations/variations_seed_store.cc:377:
base::Time::FromString(response_time.c_str(), &current_time);
On 2015/11/12 22:25:29, Alexei Svitkine (slow) wrote:
> To match how we do it on the C++ side, this should be Time::FromUTCString().
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/http/http_resp...

Done.

[Steven Holte, 2015-11-12 22:41:32]

Please update the CL description to mention the gzip/date support.

[Alexei Svitkine (slow), 2015-11-12 22:41:59]

lgtm % remaining comments

https://codereview.chromium.org/1438123002/diff/140001/components/variations/...
File
components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java
(right):

https://codereview.chromium.org/1438123002/diff/140001/components/variations/...
components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java:50:
// TODO(agulenko): add gzip compression support.
Remove TODO!

https://codereview.chromium.org/1438123002/diff/140001/components/variations/...
components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java:64:
boolean isGzipCompressed =
connection.getHeaderField("IM").trim().equals("gzip");
Hmm, I looked at the API doc for getHeaderField() and seems it can return null -
so looks like my exact suggestion isn't quite right.

http://developer.android.com/reference/java/net/URLConnection.html#getHeaderF...

I suggest making a short helper getHeaderFieldOrEmpty(name) and use it here and
for the three things above.

[Steven Holte, 2015-11-12 22:48:42]

lgtm

[Alexei Svitkine (slow), 2015-11-12 22:49:30]

https://codereview.chromium.org/1438123002/diff/140001/components/variations/...
File
components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java
(right):

https://codereview.chromium.org/1438123002/diff/140001/components/variations/...
components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java:64:
boolean isGzipCompressed =
connection.getHeaderField("IM").trim().equals("gzip");
On 2015/11/12 22:41:59, Alexei Svitkine (slow) wrote:
> Hmm, I looked at the API doc for getHeaderField() and seems it can return null
-
> so looks like my exact suggestion isn't quite right.
> 
>
http://developer.android.com/reference/java/net/URLConnection.html#getHeaderF...
> 
> I suggest making a short helper getHeaderFieldOrEmpty(name) and use it here
and
> for the three things above.

Sorry: getHeaderFieldOrEmpty(connection, name)

[Alexander Agulenko, 2015-11-12 23:03:08]

Description was changed from

==========
Removed callbacks to JNI functions to simplify code structure since they are no
longer needed

BUG=551029
==========

to

==========
Removed callbacks to JNI functions to simplify code structure since they are no
longer needed.

Also added gzip compressed seed support and pulling the actual time of HTTP
response.

BUG=551029
==========

[Alexander Agulenko, 2015-11-12 23:03:18]

The CQ bit was checked by agulenko@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-11-12 23:10:43]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1438123002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1438123002/140001

[Alexander Agulenko, 2015-11-12 23:17:24]

https://codereview.chromium.org/1438123002/diff/140001/components/variations/...
File
components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java
(right):

https://codereview.chromium.org/1438123002/diff/140001/components/variations/...
components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java:50:
// TODO(agulenko): add gzip compression support.
On 2015/11/12 22:41:59, Alexei Svitkine (slow) wrote:
> Remove TODO!

Done.

https://codereview.chromium.org/1438123002/diff/140001/components/variations/...
components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java:64:
boolean isGzipCompressed =
connection.getHeaderField("IM").trim().equals("gzip");
On 2015/11/12 22:49:30, Alexei Svitkine (slow) wrote:
> On 2015/11/12 22:41:59, Alexei Svitkine (slow) wrote:
> > Hmm, I looked at the API doc for getHeaderField() and seems it can return
null
> -
> > so looks like my exact suggestion isn't quite right.
> > 
> >
>
http://developer.android.com/reference/java/net/URLConnection.html#getHeaderF...
> > 
> > I suggest making a short helper getHeaderFieldOrEmpty(name) and use it here
> and
> > for the three things above.
> 
> Sorry: getHeaderFieldOrEmpty(connection, name)

Done.

[Alexei Svitkine (slow), 2015-11-12 23:18:12]

lgtm

[Alexander Agulenko, 2015-11-13 21:48:59]

The CQ bit was checked by agulenko@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-11-13 21:49:52]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1438123002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1438123002/160001

[commit-bot: I haz the power, 2015-11-13 21:54:21]

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

[commit-bot: I haz the power, 2015-11-13 21:54:22]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Alexei Svitkine (slow), 2015-11-13 22:30:39]

I think you need to sync/rebase

[Alexander Agulenko, 2015-11-13 22:37:50]

The CQ bit was checked by agulenko@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-11-13 22:40:36]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1438123002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1438123002/180001

[commit-bot: I haz the power, 2015-11-13 23:11:34]

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

[commit-bot: I haz the power, 2015-11-13 23:11:35]

Dry run: Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[Alexander Agulenko, 2015-11-14 00:18:12]

The CQ bit was checked by agulenko@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-11-14 00:18:47]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1438123002/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1438123002/200001

[commit-bot: I haz the power, 2015-11-14 01:45:50]

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

[commit-bot: I haz the power, 2015-11-14 01:45:51]

Dry run: This issue passed the CQ dry run.

[Alexander Agulenko, 2015-11-18 03:41:44]

The CQ bit was checked by agulenko@google.com

[Alexander Agulenko, 2015-11-18 03:41:44]

The patchset sent to the CQ was uploaded after l-g-t-m from holte@chromium.org,
asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/1438123002/#ps220001
(title: "Merging this CL with master")

[commit-bot: I haz the power, 2015-11-18 03:42:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1438123002/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1438123002/220001

[commit-bot: I haz the power, 2015-11-18 04:52:59]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2015-11-18 04:53:52]

Patchset 12 (id:??) landed as
https://crrev.com/ff14aff0758577956d4704050ef3d546a9bac624
Cr-Commit-Position: refs/heads/master@{#360292}