Generate Cronet Version.java based on VERSION and LASTCHANGE files.

Generate Cronet Version.java based on VERSION and LASTCHANGE files.
Use Cronet Version in user agent and request factory names.

BUG=354143
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278891

[mef, 2014-06-19 16:50:05]

Matt,

could you take a look? This should replace "TBD" with actual cronet build
version.

thanks,
-m

https://codereview.chromium.org/344793006/diff/1/components/cronet/DEPS
File components/cronet/DEPS (right):

https://codereview.chromium.org/344793006/diff/1/components/cronet/DEPS#newcode2
components/cronet/DEPS:2: "+cronet",
Generated cronet_version.h goes into out/Debug/gen/cronet, and checkdeps
complained about #include "cronet/cronet_version.h", but now it says that I need
to get LGTM from OWNERS of +cronet.

[mmenke, 2014-06-19 19:45:28]

https://codereview.chromium.org/344793006/diff/20001/components/cronet/androi...
File components/cronet/android/org_chromium_net_UrlRequestContext.cc (right):

https://codereview.chromium.org/344793006/diff/20001/components/cronet/androi...
components/cronet/android/org_chromium_net_UrlRequestContext.cc:22: 
nit:  Only need one blank line here.

https://codereview.chromium.org/344793006/diff/20001/components/cronet/androi...
components/cronet/android/org_chromium_net_UrlRequestContext.cc:25: const char
kVersion[] = CRONET_VERSION "/" LAST_CHANGE;
Should we copy the strings used in about:version here?  "37.0.2058.2 (Official
Build 278259) canary"

https://codereview.chromium.org/344793006/diff/20001/components/cronet/androi...
File
components/cronet/android/sample/javatests/src/org/chromium/cronet_sample_apk/HttpUrlRequestFactoryTest.java
(right):

https://codereview.chromium.org/344793006/diff/20001/components/cronet/androi...
components/cronet/android/sample/javatests/src/org/chromium/cronet_sample_apk/HttpUrlRequestFactoryTest.java:14:
import java.util.regex.Pattern;
nit:  Add line break

https://codereview.chromium.org/344793006/diff/20001/components/cronet/androi...
components/cronet/android/sample/javatests/src/org/chromium/cronet_sample_apk/HttpUrlRequestFactoryTest.java:40:
assertEquals("HttpUrlConnection", factory.getName());
So we aren't versioning the wrapper?

https://codereview.chromium.org/344793006/diff/20001/components/cronet/cronet...
File components/cronet/cronet_version.h.version (right):

https://codereview.chromium.org/344793006/diff/20001/components/cronet/cronet...
components/cronet/cronet_version.h.version:10: #define IS_OFFICIAL_BUILD
@OFFICIAL_BUILD@
I don't think @OFFICIAL_BUILD@ is a bool, but rather a string, since it's used
like:

#define OFFICIAL_BUILD_STRING "@OFFICIAL_BUILD@"

And using that to define a string that just contains false or true seems
unlikely.

[mef, 2014-06-19 21:23:29]

thanks, ptal!

https://codereview.chromium.org/344793006/diff/20001/components/cronet/androi...
File components/cronet/android/org_chromium_net_UrlRequestContext.cc (right):

https://codereview.chromium.org/344793006/diff/20001/components/cronet/androi...
components/cronet/android/org_chromium_net_UrlRequestContext.cc:22: 
On 2014/06/19 19:45:28, mmenke wrote:
> nit:  Only need one blank line here.

Done.

https://codereview.chromium.org/344793006/diff/20001/components/cronet/androi...
components/cronet/android/org_chromium_net_UrlRequestContext.cc:25: const char
kVersion[] = CRONET_VERSION "/" LAST_CHANGE;
On 2014/06/19 19:45:28, mmenke wrote:
> Should we copy the strings used in about:version here?  "37.0.2058.2 (Official
> Build 278259) canary"
Maybe. That seems to require a mix of official builder and custom hardcoded
logic like
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/chro...

https://codereview.chromium.org/344793006/diff/20001/components/cronet/androi...
File
components/cronet/android/sample/javatests/src/org/chromium/cronet_sample_apk/HttpUrlRequestFactoryTest.java
(right):

https://codereview.chromium.org/344793006/diff/20001/components/cronet/androi...
components/cronet/android/sample/javatests/src/org/chromium/cronet_sample_apk/HttpUrlRequestFactoryTest.java:14:
import java.util.regex.Pattern;
On 2014/06/19 19:45:28, mmenke wrote:
> nit:  Add line break

Done.

https://codereview.chromium.org/344793006/diff/20001/components/cronet/androi...
components/cronet/android/sample/javatests/src/org/chromium/cronet_sample_apk/HttpUrlRequestFactoryTest.java:40:
assertEquals("HttpUrlConnection", factory.getName());
On 2014/06/19 19:45:28, mmenke wrote:
> So we aren't versioning the wrapper?
Hmm, I guess not. Should we?

https://codereview.chromium.org/344793006/diff/20001/components/cronet/cronet...
File components/cronet/cronet_version.h.version (right):

https://codereview.chromium.org/344793006/diff/20001/components/cronet/cronet...
components/cronet/cronet_version.h.version:10: #define IS_OFFICIAL_BUILD
@OFFICIAL_BUILD@
On 2014/06/19 19:45:28, mmenke wrote:
> I don't think @OFFICIAL_BUILD@ is a bool, but rather a string, since it's used
> like:
> 
> #define OFFICIAL_BUILD_STRING "@OFFICIAL_BUILD@"
> 
> And using that to define a string that just contains false or true seems
> unlikely.

FWIW The result in cronet_version.h is:
#define IS_OFFICIAL_BUILD 0

[mmenke, 2014-06-19 21:28:02]

https://codereview.chromium.org/344793006/diff/20001/components/cronet/androi...
File
components/cronet/android/sample/javatests/src/org/chromium/cronet_sample_apk/HttpUrlRequestFactoryTest.java
(right):

https://codereview.chromium.org/344793006/diff/20001/components/cronet/androi...
components/cronet/android/sample/javatests/src/org/chromium/cronet_sample_apk/HttpUrlRequestFactoryTest.java:40:
assertEquals("HttpUrlConnection", factory.getName());
On 2014/06/19 21:23:29, mef wrote:
> On 2014/06/19 19:45:28, mmenke wrote:
> > So we aren't versioning the wrapper?
> Hmm, I guess not. Should we?

I think we should just have a single version number for both, actually.  Maybe
just add a function HttpUrlRequestFactory.getVersion?  Could be static, or the
Java equivalent.  I do like the change in this test, though - checking what the
string is, instead of what it isn't.

[mef, 2014-06-20 13:38:38]

On 2014/06/19 21:28:02, mmenke wrote:
>
https://codereview.chromium.org/344793006/diff/20001/components/cronet/androi...
> File
>
components/cronet/android/sample/javatests/src/org/chromium/cronet_sample_apk/HttpUrlRequestFactoryTest.java
> (right):
> 
>
https://codereview.chromium.org/344793006/diff/20001/components/cronet/androi...
>
components/cronet/android/sample/javatests/src/org/chromium/cronet_sample_apk/HttpUrlRequestFactoryTest.java:40:
> assertEquals("HttpUrlConnection", factory.getName());
> On 2014/06/19 21:23:29, mef wrote:
> > On 2014/06/19 19:45:28, mmenke wrote:
> > > So we aren't versioning the wrapper?
> > Hmm, I guess not. Should we?
> 
> I think we should just have a single version number for both, actually.  Maybe
> just add a function HttpUrlRequestFactory.getVersion?  Could be static, or the
> Java equivalent.  I do like the change in this test, though - checking what
the
> string is, instead of what it isn't.

So, I've tried to move version from C++ layer to Java, but it seems that it is
used in
https://code.google.com/p/chromium/codesearch#chromium/src/components/cronet/...
to append to user agent header.

I'm not sure whether this is a good idea, and what is a good alternative. There
is also a class UserAgent.java which generates UserAgent string based on Android
context. 
I wonder whether I should change that to also use the cronet version, so it
would be consistent?

[mef, 2014-06-20 15:42:46]

Matt, PTAL.
I'm considering changing UserAgent.java to include CRONET_VERSION. WDYT?

[mmenke, 2014-06-20 17:38:46]

On 2014/06/20 15:42:46, mef wrote:
> Matt, PTAL.
> I'm considering changing UserAgent.java to include CRONET_VERSION. WDYT?

That sounds pretty reasonable to me.

[mmenke, 2014-06-20 17:41:17]

https://codereview.chromium.org/344793006/diff/80001/components/cronet/androi...
File components/cronet/android/url_request_peer.cc (right):

https://codereview.chromium.org/344793006/diff/80001/components/cronet/androi...
components/cronet/android/url_request_peer.cc:113: user_agent =
context_->GetUserAgent(url_);
We never run this line, since we always pass in the user agent, right?

https://codereview.chromium.org/344793006/diff/80001/components/cronet/androi...
components/cronet/android/url_request_peer.cc:116:
net::HttpRequestHeaders::kUserAgent, user_agent, true /* override */);
I don't believe this does anything if
headers_.HasHeader(net::HttpRequestHeaders::kUserAgent) is true - we just
overwrite the old value with the same value.

https://codereview.chromium.org/344793006/diff/80001/components/cronet/androi...
components/cronet/android/url_request_peer.cc:118:
VLOG(context_->logging_level()) << "User agent: " << user_agent;
nit:  I don't think this is worth a VLOG, while we're here.

[mef, 2014-06-20 18:34:04]

Thanks, PTAL.

https://codereview.chromium.org/344793006/diff/80001/components/cronet/androi...
File components/cronet/android/url_request_peer.cc (right):

https://codereview.chromium.org/344793006/diff/80001/components/cronet/androi...
components/cronet/android/url_request_peer.cc:113: user_agent =
context_->GetUserAgent(url_);
On 2014/06/20 17:41:16, mmenke wrote:
> We never run this line, since we always pass in the user agent, right?
Isn't it possible, that there is no User-Agent header yet?

https://codereview.chromium.org/344793006/diff/80001/components/cronet/androi...
components/cronet/android/url_request_peer.cc:116:
net::HttpRequestHeaders::kUserAgent, user_agent, true /* override */);
On 2014/06/20 17:41:16, mmenke wrote:
> I don't believe this does anything if
> headers_.HasHeader(net::HttpRequestHeaders::kUserAgent) is true - we just
> overwrite the old value with the same value.
Done. It used to stick Cronet version into it, but that doesn't make sense now.
Done.

https://codereview.chromium.org/344793006/diff/80001/components/cronet/androi...
components/cronet/android/url_request_peer.cc:118:
VLOG(context_->logging_level()) << "User agent: " << user_agent;
On 2014/06/20 17:41:16, mmenke wrote:
> nit:  I don't think this is worth a VLOG, while we're here.

Done. I guess it was there for debugging?

[mmenke, 2014-06-20 19:03:59]

LGTM

https://codereview.chromium.org/344793006/diff/80001/components/cronet/androi...
File components/cronet/android/url_request_peer.cc (right):

https://codereview.chromium.org/344793006/diff/80001/components/cronet/androi...
components/cronet/android/url_request_peer.cc:113: user_agent =
context_->GetUserAgent(url_);
On 2014/06/20 18:34:04, mef wrote:
> On 2014/06/20 17:41:16, mmenke wrote:
> > We never run this line, since we always pass in the user agent, right?
> Isn't it possible, that there is no User-Agent header yet?

Ahh...you're right.  I was thinking we were adding it to the headers in
UrlRequest.java, not through the context (At some point, may want to switch - a
little more overhead to sending another string per request through Java, but
less code to do stuff here as well...Anyhow, not a huge difference)

[mef, 2014-06-20 19:15:40]

On 2014/06/20 19:03:59, mmenke wrote:
> LGTM
> 
>
https://codereview.chromium.org/344793006/diff/80001/components/cronet/androi...
> File components/cronet/android/url_request_peer.cc (right):
> 
>
https://codereview.chromium.org/344793006/diff/80001/components/cronet/androi...
> components/cronet/android/url_request_peer.cc:113: user_agent =
> context_->GetUserAgent(url_);
> On 2014/06/20 18:34:04, mef wrote:
> > On 2014/06/20 17:41:16, mmenke wrote:
> > > We never run this line, since we always pass in the user agent, right?
> > Isn't it possible, that there is no User-Agent header yet?
> 
> Ahh...you're right.  I was thinking we were adding it to the headers in
> UrlRequest.java, not through the context (At some point, may want to switch -
a
> little more overhead to sending another string per request through Java, but
> less code to do stuff here as well...Anyhow, not a huge difference)

sgtm, thanks!

[mef, 2014-06-20 19:33:13]

The CQ bit was checked by mef@chromium.org

[commit-bot: I haz the power, 2014-06-20 19:34:11]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mef@chromium.org/344793006/100001

[commit-bot: I haz the power, 2014-06-20 23:25:36]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/8...)
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[commit-bot: I haz the power, 2014-06-20 23:48:47]

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

[commit-bot: I haz the power, 2014-06-20 23:48:47]

Try jobs failed on following builders:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[mef, 2014-06-21 00:00:58]

The CQ bit was checked by mef@chromium.org

[commit-bot: I haz the power, 2014-06-21 00:03:12]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mef@chromium.org/344793006/100001

[commit-bot: I haz the power, 2014-06-21 01:35:00]

Change committed as 278891