Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
558333007:
Setup initial mock url request job tests for Cronet (Closed)
DescriptionSetup initial mock url request job tests for Cronet
This CL adds two initial tests that use mock URLRequestJob to
intercept URL requests.
BUG=415781
Committed: https://crrev.com/a21168de1ab2004932a309db77bf9601a7b8df75
Cr-Commit-Position: refs/heads/master@{#296201}
Patch Set 1 #
Total comments: 12
Patch Set 2 : Added one assert on request url #
Total comments: 9
Patch Set 3 : Used matt's suggestion to dump test files from apk and addressed other comments #
Total comments: 28
Patch Set 4 : Addressed comments #
Total comments: 6
Patch Set 5 : Addressed Misha's comments #
Total comments: 8
Patch Set 6 : Addressed Matt's comments #
Total comments: 3
Patch Set 7 : added jni binding for AddUrlInterceptors() #
Total comments: 8
Patch Set 8 : Addressed Misha's comments #
Total comments: 13
Patch Set 9 : Addressed Matt's comments #Patch Set 10 : Rebased #Messages
Total messages: 40 (4 generated)
xunjieli@chromium.org changed reviewers: + mef@chromium.org, mmenke@chromium.org
Hi Misha and Matt, I wrote two simple tests. Could you take a look? I just want to make sure I am doing the right thing before I move on to writing more test cases. I have not found an easy way to include (in the application package) html and header files and load them from native code. We could load them from Java side easily, but with the current url_request_mock_http_job setup, I have not found a clean way. We probably won't need a lot of content in our test files, so writing and loading them dynamically might be sufficient? PTAL. Thanks! https://codereview.chromium.org/558333007/diff/1/components/cronet.gypi File components/cronet.gypi (right): https://codereview.chromium.org/558333007/diff/1/components/cronet.gypi#newco... components/cronet.gypi:430: '../third_party/icu/icu.gyp:icuuc', This is a little ugly to add dependencies back. But I guess it should be fine, since this target is only used for testing? WDYT? https://codereview.chromium.org/558333007/diff/1/components/cronet/android/te... File components/cronet/android/test/cronet_tests_jni.cc (right): https://codereview.chromium.org/558333007/diff/1/components/cronet/android/te... components/cronet/android/test/cronet_tests_jni.cc:52: void PrepareTestFiles() { Do you think we will need many test files? https://codereview.chromium.org/558333007/diff/1/components/cronet/android/te... components/cronet/android/test/cronet_tests_jni.cc:66: fprintf(success_headers_file.get(), "HTTP/1.1 200 OK"); This success.html.mock-http-headers is not actually needed, since it is a 200. I included it anyway to test that we can actually write and read a file with .mock-http-headers suffix.
https://codereview.chromium.org/558333007/diff/1/components/cronet/android/te... File components/cronet/android/test/cronet_tests_jni.cc (right): https://codereview.chromium.org/558333007/diff/1/components/cronet/android/te... components/cronet/android/test/cronet_tests_jni.cc:52: void PrepareTestFiles() { If so, we probably need this function to be in a new file (not in cronet_tests_jni)?
https://codereview.chromium.org/558333007/diff/1/components/cronet/android/te... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/MockURLRequestJobTest.java (right): https://codereview.chromium.org/558333007/diff/1/components/cronet/android/te... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/MockURLRequestJobTest.java:66: assertNotNull(activity); This and below should be "assertEquals(MOCK_CRONET_TEST_URL, request.getUrl());" Will change in next patch.
I would also create a crbug for test infrastructure. https://codereview.chromium.org/558333007/diff/1/components/cronet/android/te... File components/cronet/android/test/cronet_tests_jni.cc (right): https://codereview.chromium.org/558333007/diff/1/components/cronet/android/te... components/cronet/android/test/cronet_tests_jni.cc:52: void PrepareTestFiles() { On 2014/09/16 15:25:36, xunjieli wrote: > If so, we probably need this function to be in a new file (not in > cronet_tests_jni)? sgtm, we may need it to be more configurable / abstract. Would it make sense to prepare those files from java (e.g. by extracting from resources)? https://codereview.chromium.org/558333007/diff/1/components/cronet/android/te... components/cronet/android/test/cronet_tests_jni.cc:54: DCHECK(PathService::Get(base::DIR_SOURCE_ROOT, &test_files_root)); I think DIR_SOURCE_ROOT is actual src/ on desktop platforms. Suggest maybe using subdirectory of DIR_TEST_DATA? We should also see whether 'test isolation' has some provisions for putting test data onto device. https://codereview.chromium.org/558333007/diff/1/components/cronet/android/te... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/MockURLRequestJobTest.java (right): https://codereview.chromium.org/558333007/diff/1/components/cronet/android/te... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/MockURLRequestJobTest.java:20: private static final String MOCK_CRONET_TEST_URL = "http://mock.cronet.success/"; nit: line wrap. https://codereview.chromium.org/558333007/diff/20001/components/cronet.gypi File components/cronet.gypi (right): https://codereview.chromium.org/558333007/diff/20001/components/cronet.gypi#n... components/cronet.gypi:429: '../third_party/icu/icu.gyp:icui18n', Interesting, does it actually work? We are building cronet with USE_ICU_ALTERNATIVES_ON_ANDROID=1, I wonder whether this would collide and/or affect the results.
https://codereview.chromium.org/558333007/diff/1/components/cronet/android/te... File components/cronet/android/test/cronet_tests_jni.cc (right): https://codereview.chromium.org/558333007/diff/1/components/cronet/android/te... components/cronet/android/test/cronet_tests_jni.cc:52: void PrepareTestFiles() { On 2014/09/16 22:02:34, mef wrote: > On 2014/09/16 15:25:36, xunjieli wrote: > > If so, we probably need this function to be in a new file (not in > > cronet_tests_jni)? > > sgtm, we may need it to be more configurable / abstract. > Would it make sense to prepare those files from java (e.g. by extracting from > resources)? The problem is that URLRequestMockHTTPJob (net/test/url_request_mock_http_job.cc) is loading files from C++. If we would like to load from Java, I guess we will need to create a Mock URL request job that is similar to URLRequestMockHTTPJob, but has jni calls to read file data (and translate that into c++ data). Idk, It sounds a lot of work. https://codereview.chromium.org/558333007/diff/1/components/cronet/android/te... components/cronet/android/test/cronet_tests_jni.cc:54: DCHECK(PathService::Get(base::DIR_SOURCE_ROOT, &test_files_root)); On 2014/09/16 22:02:34, mef wrote: > I think DIR_SOURCE_ROOT is actual src/ on desktop platforms. Suggest maybe using > subdirectory of DIR_TEST_DATA? We should also see whether 'test isolation' has > some provisions for putting test data onto device. So on Android, this is the directory of external storage (https://code.google.com/p/chromium/codesearch#chromium/src/base/base_paths_an...). I guess we can append a subdirectory to it. (sth like "test/data/"). I was thinking of using base::DIR_ANDROID_APP_DATA, but that is not initialized yet (we don't have an application context). So I guess the only option is adding a sub directory to DIR_SOURCE_ROOT? https://codereview.chromium.org/558333007/diff/1/components/cronet/android/te... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/MockURLRequestJobTest.java (right): https://codereview.chromium.org/558333007/diff/1/components/cronet/android/te... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/MockURLRequestJobTest.java:20: private static final String MOCK_CRONET_TEST_URL = "http://mock.cronet.success/"; On 2014/09/16 22:02:34, mef wrote: > nit: line wrap. Acknowledged. Will do it in next patch. Thanks! https://codereview.chromium.org/558333007/diff/20001/components/cronet.gypi File components/cronet.gypi (right): https://codereview.chromium.org/558333007/diff/20001/components/cronet.gypi#n... components/cronet.gypi:429: '../third_party/icu/icu.gyp:icui18n', On 2014/09/16 22:02:35, mef wrote: > Interesting, does it actually work? We are building cronet with > USE_ICU_ALTERNATIVES_ON_ANDROID=1, I wonder whether this would collide and/or > affect the results. That's one of my concerns too. Matt, any thoughts? Just fyi, the test compiles and runs as expected.
I'll have a full set of comments tomorrow. https://codereview.chromium.org/558333007/diff/20001/components/cronet.gypi File components/cronet.gypi (right): https://codereview.chromium.org/558333007/diff/20001/components/cronet.gypi#n... components/cronet.gypi:429: '../third_party/icu/icu.gyp:icui18n', On 2014/09/16 22:02:35, mef wrote: > Interesting, does it actually work? We are building cronet with > USE_ICU_ALTERNATIVES_ON_ANDROID=1, I wonder whether this would collide and/or > affect the results. I believe this only works because we don't actually use any function that uses ICU - it's only used for file:// directory listing support, which isn't being called. We don't have the ICU data file on the device, so any attempt to load it, or call a function that depends on it, will fail.
https://codereview.chromium.org/558333007/diff/20001/components/cronet/androi... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/MockURLRequestJobTest.java (right): https://codereview.chromium.org/558333007/diff/20001/components/cronet/androi... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/MockURLRequestJobTest.java:67: assertEquals(200, request.getHttpStatusCode()); This is probably flaky and works due to lucky timing as request.finish() calls nativeDestroyRequestAdapter after onRequestComplete(), which posts delete to network thread. I suspect that posted delete is not done yet, thus avoiding the crash. Try to put Thread.sleep after blockForComplete() to confirm the theory.
On 2014/09/17 20:03:49, mef wrote: > https://codereview.chromium.org/558333007/diff/20001/components/cronet/androi... > File > components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/MockURLRequestJobTest.java > (right): > > https://codereview.chromium.org/558333007/diff/20001/components/cronet/androi... > components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/MockURLRequestJobTest.java:67: > assertEquals(200, request.getHttpStatusCode()); > This is probably flaky and works due to lucky timing as request.finish() calls > nativeDestroyRequestAdapter after onRequestComplete(), which posts delete to > network thread. > I suspect that posted delete is not done yet, thus avoiding the crash. > > Try to put Thread.sleep after blockForComplete() to confirm the theory. Thanks for the suggestion. I will try that!
On 2014/09/17 20:06:04, xunjieli wrote: > On 2014/09/17 20:03:49, mef wrote: > > > https://codereview.chromium.org/558333007/diff/20001/components/cronet/androi... > > File > > > components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/MockURLRequestJobTest.java > > (right): > > > > > https://codereview.chromium.org/558333007/diff/20001/components/cronet/androi... > > > components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/MockURLRequestJobTest.java:67: > > assertEquals(200, request.getHttpStatusCode()); > > This is probably flaky and works due to lucky timing as request.finish() calls > > nativeDestroyRequestAdapter after onRequestComplete(), which posts delete to > > network thread. > > I suspect that posted delete is not done yet, thus avoiding the crash. > > > > Try to put Thread.sleep after blockForComplete() to confirm the theory. > > Thanks for the suggestion. I will try that! Sorry, won't get to this today. :(
https://codereview.chromium.org/558333007/diff/20001/components/cronet/androi... File components/cronet/android/test/cronet_tests_jni.cc (right): https://codereview.chromium.org/558333007/diff/20001/components/cronet/androi... components/cronet/android/test/cronet_tests_jni.cc:34: net::URLRequestJob* CronetURLRequestJobFactory( Job factories, though heavily used, are semi-sort-kinda-unofficially deprecated. Should be using URLRequestInterceptors instead. https://codereview.chromium.org/558333007/diff/20001/components/cronet/androi... components/cronet/android/test/cronet_tests_jni.cc:66: fprintf(success_headers_file.get(), "HTTP/1.1 200 OK"); Is there an alternative way to do this? Embed a directory in the jar file and dump it to the test file? Make installing the test apk also install test files?
https://codereview.chromium.org/558333007/diff/20001/components/cronet/androi... File components/cronet/android/test/cronet_tests_jni.cc (right): https://codereview.chromium.org/558333007/diff/20001/components/cronet/androi... components/cronet/android/test/cronet_tests_jni.cc:34: net::URLRequestJob* CronetURLRequestJobFactory( On 2014/09/17 21:46:11, mmenke wrote: > Job factories, though heavily used, are semi-sort-kinda-unofficially deprecated. > Should be using URLRequestInterceptors instead. Acknowledged. Will do it in next patch. https://codereview.chromium.org/558333007/diff/20001/components/cronet/androi... components/cronet/android/test/cronet_tests_jni.cc:66: fprintf(success_headers_file.get(), "HTTP/1.1 200 OK"); On 2014/09/17 21:46:11, mmenke wrote: > Is there an alternative way to do this? Embed a directory in the jar file and > dump it to the test file? Make installing the test apk also install test files? I think this dumping mechanism might work. I will look into it. https://codereview.chromium.org/558333007/diff/20001/components/cronet/androi... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/MockURLRequestJobTest.java (right): https://codereview.chromium.org/558333007/diff/20001/components/cronet/androi... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/MockURLRequestJobTest.java:67: assertEquals(200, request.getHttpStatusCode()); On 2014/09/17 20:03:49, mef wrote: > This is probably flaky and works due to lucky timing as request.finish() calls > nativeDestroyRequestAdapter after onRequestComplete(), which posts delete to > network thread. > I suspect that posted delete is not done yet, thus avoiding the crash. > > Try to put Thread.sleep after blockForComplete() to confirm the theory. Done.Thanks Misha! That's exactly what happened.
Matt and Misha PTAL. I have changed the CL to use the "dumping test files" mechanism that Matt has suggested. I have also addressed other comments. Thanks! https://codereview.chromium.org/558333007/diff/1/components/cronet/android/te... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/MockURLRequestJobTest.java (right): https://codereview.chromium.org/558333007/diff/1/components/cronet/android/te... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/MockURLRequestJobTest.java:20: private static final String MOCK_CRONET_TEST_URL = "http://mock.cronet.success/"; On 2014/09/16 22:14:46, xunjieli wrote: > On 2014/09/16 22:02:34, mef wrote: > > nit: line wrap. > > Acknowledged. Will do it in next patch. Thanks! Done.
Looks good! I like this approach much better! https://codereview.chromium.org/558333007/diff/40001/components/cronet/androi... File components/cronet/android/test/assets/test/success.html.mock-http-headers (right): https://codereview.chromium.org/558333007/diff/40001/components/cronet/androi... components/cronet/android/test/assets/test/success.html.mock-http-headers:1: HTTP/1.1 200 OK How about a 404 response, too, to make sure that gets passed down properly. And maybe a redirect? Do we provide the URL after redirects to the embedder? https://codereview.chromium.org/558333007/diff/40001/components/cronet/androi... File components/cronet/android/test/cronet_tests_jni.cc (right): https://codereview.chromium.org/558333007/diff/40001/components/cronet/androi... components/cronet/android/test/cronet_tests_jni.cc:27: const char* const kMockCronetTestFailedUrl = "http://mock.cronet.failed/"; nit: const char kMockCronetTestFailedUrl[] is more common (x2), probably because it makes people think less. https://codereview.chromium.org/558333007/diff/40001/components/cronet/androi... components/cronet/android/test/cronet_tests_jni.cc:33: // static not static - in an anonymous namespace instead. https://codereview.chromium.org/558333007/diff/40001/components/cronet/androi... components/cronet/android/test/cronet_tests_jni.cc:34: class CronetMockJobInterceptor : public net::URLRequestInterceptor { Should include net/url_request/url_request_interceptor.h (Good job on the headers, by the way - very easy to miss one, but looks like you have them all). https://codereview.chromium.org/558333007/diff/40001/components/cronet/androi... components/cronet/android/test/cronet_tests_jni.cc:39: // net::URLRequestJobFactory::ProtocolHandler implementation nit: "net::URLRequestInterceptor implementation" (You probably copied a comment that I forgot to update in a refactor) https://codereview.chromium.org/558333007/diff/40001/components/cronet/androi... components/cronet/android/test/cronet_tests_jni.cc:44: DCHECK(PathService::Get(base::DIR_ANDROID_APP_DATA, &test_files_root)); Don't put code with side effects in DCHECKs (Or checks, for that matter, as they can regress). Can do: bool result = blah; DCHECK(result); Or just not check the return value. I'm fine, either way. Not sure we have a preferred style for C++ in Java-driven tests. https://codereview.chromium.org/558333007/diff/40001/components/cronet/androi... components/cronet/android/test/cronet_tests_jni.cc:62: DCHECK(filter->AddUrlInterceptor( Don't put code with side effects in DCHECKs. Otherwise, when DCHECKs aren't built into the binary, your code won't work. Could just make AddUrlInterceptors() return false on failure, and make JNI_OnLoad return -1 in that case, just like RegisterNativeMethods. https://codereview.chromium.org/558333007/diff/40001/components/cronet/androi... components/cronet/android/test/cronet_tests_jni.cc:95: extern "C" void JNI_OnUnLoad(JavaVM* vm, void* reserved) { Just for kicks, could call net::URLRequestFilter::GetInstance()->ClearHandlers(); https://codereview.chromium.org/558333007/diff/40001/components/cronet/androi... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/MockURLRequestJobTest.java (right): https://codereview.chromium.org/558333007/diff/40001/components/cronet/androi... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/MockURLRequestJobTest.java:17: // Tests that use mock URL request jobs to simulate URL requests. nit: mock URLRequestJobs https://codereview.chromium.org/558333007/diff/40001/components/cronet/androi... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/MockURLRequestJobTest.java:28: public String url; nit: mHttpStatusCode / mUrl https://codereview.chromium.org/558333007/diff/40001/components/cronet/androi... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/MockURLRequestJobTest.java:36: + request.getContentLength()); nit: Binary operators on previous line is the preferred style. https://codereview.chromium.org/558333007/diff/40001/components/cronet/androi... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/MockURLRequestJobTest.java:50: + request.getHttpStatusCode()); nit: Binary operators on previous line is the preferred style. (You may see Cronet code violating this - we basically started with something that wasn't fully reviewed, and haven't fully cleaned it up). https://codereview.chromium.org/558333007/diff/40001/components/cronet/androi... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/MockURLRequestJobTest.java:99: nit: Remove blank line. https://codereview.chromium.org/558333007/diff/40001/components/cronet/androi... File components/cronet/android/test/src/org/chromium/cronet_test_apk/CronetTestActivity.java (right): https://codereview.chromium.org/558333007/diff/40001/components/cronet/androi... components/cronet/android/test/src/org/chromium/cronet_test_apk/CronetTestActivity.java:137: private static boolean copyTestFile(AssetManager assetManager, String testFile, nit: While Java style guide allows longer lines, since we're still using rietveld, suggest sicking with the 80 line limit when reasonable to do so.
Thanks for the review! I have uploaded a new patch to address the comments. PTAL. https://codereview.chromium.org/558333007/diff/40001/components/cronet/androi... File components/cronet/android/test/assets/test/success.html.mock-http-headers (right): https://codereview.chromium.org/558333007/diff/40001/components/cronet/androi... components/cronet/android/test/assets/test/success.html.mock-http-headers:1: HTTP/1.1 200 OK Done. thanks! Cronet currently does not provide the URL after redirects. On 2014/09/18 18:31:44, mmenke wrote: > How about a 404 response, too, to make sure that gets passed down properly. And > maybe a redirect? Do we provide the URL after redirects to the embedder? https://codereview.chromium.org/558333007/diff/40001/components/cronet/androi... File components/cronet/android/test/cronet_tests_jni.cc (right): https://codereview.chromium.org/558333007/diff/40001/components/cronet/androi... components/cronet/android/test/cronet_tests_jni.cc:27: const char* const kMockCronetTestFailedUrl = "http://mock.cronet.failed/"; On 2014/09/18 18:31:44, mmenke wrote: > nit: const char kMockCronetTestFailedUrl[] is more common (x2), probably > because it makes people think less. Done. Thanks for explaining! https://codereview.chromium.org/558333007/diff/40001/components/cronet/androi... components/cronet/android/test/cronet_tests_jni.cc:33: // static On 2014/09/18 18:31:44, mmenke wrote: > not static - in an anonymous namespace instead. Done. Sorry about the stale doc. https://codereview.chromium.org/558333007/diff/40001/components/cronet/androi... components/cronet/android/test/cronet_tests_jni.cc:34: class CronetMockJobInterceptor : public net::URLRequestInterceptor { On 2014/09/18 18:31:45, mmenke wrote: > Should include net/url_request/url_request_interceptor.h (Good job on the > headers, by the way - very easy to miss one, but looks like you have them all). Done. Thanks! https://codereview.chromium.org/558333007/diff/40001/components/cronet/androi... components/cronet/android/test/cronet_tests_jni.cc:39: // net::URLRequestJobFactory::ProtocolHandler implementation On 2014/09/18 18:31:44, mmenke wrote: > nit: "net::URLRequestInterceptor implementation" (You probably copied a comment > that I forgot to update in a refactor) Done. Thanks! https://codereview.chromium.org/558333007/diff/40001/components/cronet/androi... components/cronet/android/test/cronet_tests_jni.cc:44: DCHECK(PathService::Get(base::DIR_ANDROID_APP_DATA, &test_files_root)); On 2014/09/18 18:31:45, mmenke wrote: > Don't put code with side effects in DCHECKs (Or checks, for that matter, as they > can regress). > > Can do: > bool result = blah; > DCHECK(result); > > Or just not check the return value. I'm fine, either way. Not sure we have a > preferred style for C++ in Java-driven tests. Done. Thanks for explaining! https://codereview.chromium.org/558333007/diff/40001/components/cronet/androi... components/cronet/android/test/cronet_tests_jni.cc:62: DCHECK(filter->AddUrlInterceptor( On 2014/09/18 18:31:44, mmenke wrote: > Don't put code with side effects in DCHECKs. Otherwise, when DCHECKs aren't > built into the binary, your code won't work. > > Could just make AddUrlInterceptors() return false on failure, and make > JNI_OnLoad return -1 in that case, just like RegisterNativeMethods. Done. https://codereview.chromium.org/558333007/diff/40001/components/cronet/androi... components/cronet/android/test/cronet_tests_jni.cc:95: extern "C" void JNI_OnUnLoad(JavaVM* vm, void* reserved) { On 2014/09/18 18:31:45, mmenke wrote: > Just for kicks, could call > net::URLRequestFilter::GetInstance()->ClearHandlers(); Done. https://codereview.chromium.org/558333007/diff/40001/components/cronet/androi... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/MockURLRequestJobTest.java (right): https://codereview.chromium.org/558333007/diff/40001/components/cronet/androi... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/MockURLRequestJobTest.java:17: // Tests that use mock URL request jobs to simulate URL requests. On 2014/09/18 18:31:45, mmenke wrote: > nit: mock URLRequestJobs Done. https://codereview.chromium.org/558333007/diff/40001/components/cronet/androi... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/MockURLRequestJobTest.java:28: public String url; On 2014/09/18 18:31:45, mmenke wrote: > nit: mHttpStatusCode / mUrl Done. https://codereview.chromium.org/558333007/diff/40001/components/cronet/androi... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/MockURLRequestJobTest.java:36: + request.getContentLength()); On 2014/09/18 18:31:45, mmenke wrote: > nit: Binary operators on previous line is the preferred style. Done. Sry! you mentioned this last time. https://codereview.chromium.org/558333007/diff/40001/components/cronet/androi... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/MockURLRequestJobTest.java:50: + request.getHttpStatusCode()); On 2014/09/18 18:31:45, mmenke wrote: > nit: Binary operators on previous line is the preferred style. > > (You may see Cronet code violating this - we basically started with something > that wasn't fully reviewed, and haven't fully cleaned it up). Done. https://codereview.chromium.org/558333007/diff/40001/components/cronet/androi... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/MockURLRequestJobTest.java:99: On 2014/09/18 18:31:45, mmenke wrote: > nit: Remove blank line. Done. https://codereview.chromium.org/558333007/diff/40001/components/cronet/androi... File components/cronet/android/test/src/org/chromium/cronet_test_apk/CronetTestActivity.java (right): https://codereview.chromium.org/558333007/diff/40001/components/cronet/androi... components/cronet/android/test/src/org/chromium/cronet_test_apk/CronetTestActivity.java:137: private static boolean copyTestFile(AssetManager assetManager, String testFile, On 2014/09/18 18:31:45, mmenke wrote: > nit: While Java style guide allows longer lines, since we're still using > rietveld, suggest sicking with the 80 line limit when reasonable to do so. Done.
Looks great! https://codereview.chromium.org/558333007/diff/60001/components/cronet/androi... File components/cronet/android/test/cronet_tests_jni.cc (right): https://codereview.chromium.org/558333007/diff/60001/components/cronet/androi... components/cronet/android/test/cronet_tests_jni.cc:37: class CronetMockJobInterceptor : public net::URLRequestInterceptor { Suggest: move CronetMockJobInterceptor into its own module, so cronet_tests_jni.cc is purely jni load stuff. https://codereview.chromium.org/558333007/diff/60001/components/cronet/androi... components/cronet/android/test/cronet_tests_jni.cc:48: if (request->url() == GURL(kMockCronetTestRedirectUrl)) { Would it make sense to make some kind of map between URL and test file, or just extract test file name from URL? This would help scaling to more tests. https://codereview.chromium.org/558333007/diff/60001/components/cronet/androi... components/cronet/android/test/cronet_tests_jni.cc:75: filter->AddUrlInterceptor(GURL(kMockCronetTestRedirectUrl), Again, having a map / list and a loop would avoid cut-n-paste.
Thanks Misha for the review! Your second and third comments are great suggestions. I have added it as a TODO. Will tackle it in the next CL. PTAL. https://codereview.chromium.org/558333007/diff/60001/components/cronet/androi... File components/cronet/android/test/cronet_tests_jni.cc (right): https://codereview.chromium.org/558333007/diff/60001/components/cronet/androi... components/cronet/android/test/cronet_tests_jni.cc:37: class CronetMockJobInterceptor : public net::URLRequestInterceptor { On 2014/09/19 14:12:00, mef wrote: > Suggest: move CronetMockJobInterceptor into its own module, so > cronet_tests_jni.cc is purely jni load stuff. Done. https://codereview.chromium.org/558333007/diff/60001/components/cronet/androi... components/cronet/android/test/cronet_tests_jni.cc:48: if (request->url() == GURL(kMockCronetTestRedirectUrl)) { On 2014/09/19 14:12:00, mef wrote: > Would it make sense to make some kind of map between URL and test file, or just > extract test file name from URL? > This would help scaling to more tests. Acknowledged. I think this is a great idea. But right now I am not very sure how the failures-during-different-stages-tests will look like, and whether we need other types of Mock Interceptors. I have added your suggestion as a TODO. I will try to fix this potential code duplication problem in the next CL. https://codereview.chromium.org/558333007/diff/60001/components/cronet/androi... components/cronet/android/test/cronet_tests_jni.cc:75: filter->AddUrlInterceptor(GURL(kMockCronetTestRedirectUrl), On 2014/09/19 14:12:00, mef wrote: > Again, having a map / list and a loop would avoid cut-n-paste. Acknowledged. Will tackle this in the next CL when I am more clear about what kind of mock interceptors we need :)
https://codereview.chromium.org/558333007/diff/80001/components/cronet/androi... File components/cronet/android/test/assets/test/success.html (right): https://codereview.chromium.org/558333007/diff/80001/components/cronet/androi... components/cronet/android/test/assets/test/success.html:7: </html> Can we make the body simpler and check that in the tests, too? Could just switch the mime-type to plain/test (And maybe the file extension as well, for consistency?) and switch the body to a simple one line string. https://codereview.chromium.org/558333007/diff/80001/components/cronet/androi... File components/cronet/android/test/assets/test/success.html.mock-http-headers (right): https://codereview.chromium.org/558333007/diff/80001/components/cronet/androi... components/cronet/android/test/assets/test/success.html.mock-http-headers:1: HTTP/1.1 404 Not Found Seems a little weird to make "success.html" return a not found error. Suggest two separate tests, one with a 200 page named success, one with 404. They'll basically do the same thing, but if we ever choose to HTTP handle errors differently (In cronet or in net/), seems best to have a test in place. https://codereview.chromium.org/558333007/diff/80001/components/cronet/androi... File components/cronet/android/test/cronet_mock_job_interceptor.cc (right): https://codereview.chromium.org/558333007/diff/80001/components/cronet/androi... components/cronet/android/test/cronet_mock_job_interceptor.cc:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. nit: Don't use (c) in new files. https://codereview.chromium.org/558333007/diff/80001/components/cronet/androi... components/cronet/android/test/cronet_mock_job_interceptor.cc:74: new CronetMockJobInterceptor())) && As Misha said, we really shouldn't be listing these individually. We can just use net::URLRequestMockHTTPJob::AddUrlHandler(test_files_root, base::MessageLoop::current()->message_loop_proxy()); And then access the files in tests via "http://mock.http/redirect.html", "http://mock.http/success.html", etc. Could optionally use net::URLRequestFailedJob::AddUrlHandler() as well, and just use "http://mock.failed.request/2" for ERR_FAILED. (The numeric value for ERR_FAILED is -2) I probably should have suggested these last time.
https://codereview.chromium.org/558333007/diff/80001/components/cronet/androi... File components/cronet/android/test/assets/test/success.html (right): https://codereview.chromium.org/558333007/diff/80001/components/cronet/androi... components/cronet/android/test/assets/test/success.html:7: </html> Can we make the body simpler and check that in the tests, too? Could just switch the mime-type to plain/test (And maybe the file extension as well, for consistency?) and switch the body to a simple one line string. https://codereview.chromium.org/558333007/diff/80001/components/cronet/androi... File components/cronet/android/test/assets/test/success.html.mock-http-headers (right): https://codereview.chromium.org/558333007/diff/80001/components/cronet/androi... components/cronet/android/test/assets/test/success.html.mock-http-headers:1: HTTP/1.1 404 Not Found Seems a little weird to make "success.html" return a not found error. Suggest two separate tests, one with a 200 page named success, one with 404. They'll basically do the same thing, but if we ever choose to HTTP handle errors differently (In cronet or in net/), seems best to have a test in place. https://codereview.chromium.org/558333007/diff/80001/components/cronet/androi... File components/cronet/android/test/cronet_mock_job_interceptor.cc (right): https://codereview.chromium.org/558333007/diff/80001/components/cronet/androi... components/cronet/android/test/cronet_mock_job_interceptor.cc:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. nit: Don't use (c) in new files. https://codereview.chromium.org/558333007/diff/80001/components/cronet/androi... components/cronet/android/test/cronet_mock_job_interceptor.cc:74: new CronetMockJobInterceptor())) && As Misha said, we really shouldn't be listing these individually. We can just use net::URLRequestMockHTTPJob::AddUrlHandler(test_files_root, base::MessageLoop::current()->message_loop_proxy()); And then access the files in tests via "http://mock.http/redirect.html", "http://mock.http/success.html", etc. Could optionally use net::URLRequestFailedJob::AddUrlHandler() as well, and just use "http://mock.failed.request/2" for ERR_FAILED. (The numeric value for ERR_FAILED is -2) I probably should have suggested these last time.
One more question from me. See the comment below. PTAL. Thanks! https://codereview.chromium.org/558333007/diff/80001/components/cronet/androi... File components/cronet/android/test/assets/test/success.html (right): https://codereview.chromium.org/558333007/diff/80001/components/cronet/androi... components/cronet/android/test/assets/test/success.html:7: </html> On 2014/09/19 17:15:55, mmenke wrote: > Can we make the body simpler and check that in the tests, too? Could just > switch the mime-type to plain/test (And maybe the file extension as well, for > consistency?) and switch the body to a simple one line string. Done. https://codereview.chromium.org/558333007/diff/80001/components/cronet/androi... File components/cronet/android/test/assets/test/success.html.mock-http-headers (right): https://codereview.chromium.org/558333007/diff/80001/components/cronet/androi... components/cronet/android/test/assets/test/success.html.mock-http-headers:1: HTTP/1.1 404 Not Found On 2014/09/19 17:15:55, mmenke wrote: > Seems a little weird to make "success.html" return a not found error. Suggest > two separate tests, one with a 200 page named success, one with 404. They'll > basically do the same thing, but if we ever choose to HTTP handle errors > differently (In cronet or in net/), seems best to have a test in place. Done. https://codereview.chromium.org/558333007/diff/80001/components/cronet/androi... File components/cronet/android/test/cronet_mock_job_interceptor.cc (right): https://codereview.chromium.org/558333007/diff/80001/components/cronet/androi... components/cronet/android/test/cronet_mock_job_interceptor.cc:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. On 2014/09/19 17:15:55, mmenke wrote: > nit: Don't use (c) in new files. Done. https://codereview.chromium.org/558333007/diff/80001/components/cronet/androi... components/cronet/android/test/cronet_mock_job_interceptor.cc:74: new CronetMockJobInterceptor())) && On 2014/09/19 17:15:55, mmenke wrote: > As Misha said, we really shouldn't be listing these individually. We can just > use net::URLRequestMockHTTPJob::AddUrlHandler(test_files_root, > base::MessageLoop::current()->message_loop_proxy()); > > And then access the files in tests via "http://mock.http/redirect.html", > "http://mock.http/success.html", etc. > > Could optionally use net::URLRequestFailedJob::AddUrlHandler() as well, and just > use "http://mock.failed.request/2" for ERR_FAILED. (The numeric value for > ERR_FAILED is -2) > > I probably should have suggested these last time. Done. I see! That's very smart! But I am having one problem with passing in the test_files_root. see my other comment on the new patch set. https://codereview.chromium.org/558333007/diff/100001/components/cronet/andro... File components/cronet/android/test/cronet_mock_job_interceptor.cc (right): https://codereview.chromium.org/558333007/diff/100001/components/cronet/andro... components/cronet/android/test/cronet_mock_job_interceptor.cc:18: // PathService::Get(base::DIR_ANDROID_APP_DATA, &test_files_root); Matt, I hardcoded a the directory above temporarily just to see if the tests pass. The problem I ran into is that I can't call PathService::Get(base::DIR_ANDROID_APP_DATA, & test_files_root) at this time. It is the complaining that the application context is null (fails DCHECK(!g_application_context.Get().is_null()); on line 144 of jni_android.cc).The native application context can only be initialized after the library is loaded. Since we are calling AddUrlInterceptors() before the library is loaded, there is no way we can initialize native application context (since the native implementations are only registered after the library is loaded). So I guess there are only two options: #1 keep the hardcoded file path (which is not ideal) #2 move AddUrlInterceptors() out of JNI_OnLoad and create JNI binding for it. Which do you prefer? Any thoughts?
https://codereview.chromium.org/558333007/diff/100001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/MockURLRequestJobTest.java (right): https://codereview.chromium.org/558333007/diff/100001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/MockURLRequestJobTest.java:77: // Currently Cronet does not expose the url after redirect. This doc is stale. will remove it in next patch.
https://codereview.chromium.org/558333007/diff/100001/components/cronet/andro... File components/cronet/android/test/cronet_mock_job_interceptor.cc (right): https://codereview.chromium.org/558333007/diff/100001/components/cronet/andro... components/cronet/android/test/cronet_mock_job_interceptor.cc:18: // PathService::Get(base::DIR_ANDROID_APP_DATA, &test_files_root); On 2014/09/19 20:39:15, xunjieli wrote: > Matt, I hardcoded a the directory above temporarily just to see if the tests > pass. > The problem I ran into is that I can't call > PathService::Get(base::DIR_ANDROID_APP_DATA, & test_files_root) at this time. It > is the complaining that the application context is null (fails > DCHECK(!g_application_context.Get().is_null()); on line 144 of > jni_android.cc).The native application context can only be initialized after the > library is loaded. Since we are calling AddUrlInterceptors() before the library > is loaded, there is no way we can initialize native application context (since > the native implementations are only registered after the library is loaded). > > So I guess there are only two options: > #1 keep the hardcoded file path (which is not ideal) > #2 move AddUrlInterceptors() out of JNI_OnLoad and create JNI binding for it. > > Which do you prefer? Any thoughts? That's unfortunate. I think the JNI binding approach is preferable, particularly if we can put the call in our test fixture, Java side.
Thanks! PTAL. I have added JNI binding and called the native function from the text fixture after CronetTestActivity is created. The reason for calling after CronetTestActivity creation is that native application context is only created in CreateRequestContextAdaptor which is invoked in onCreate() of CronetTextActivity.
https://codereview.chromium.org/558333007/diff/120001/components/cronet/andro... File components/cronet/android/test/cronet_tests_jni.cc (right): https://codereview.chromium.org/558333007/diff/120001/components/cronet/andro... components/cronet/android/test/cronet_tests_jni.cc:16: {"BaseAndroid", base::android::RegisterJni}, We actually don't need "baseandroid" here as it is already in Cronet. I've left it here as a placeholder. :) https://codereview.chromium.org/558333007/diff/120001/components/cronet/andro... components/cronet/android/test/cronet_tests_jni.cc:40: nit: spurious nl. https://codereview.chromium.org/558333007/diff/120001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/MockUrlRequestJobTest.java (right): https://codereview.chromium.org/558333007/diff/120001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/MockUrlRequestJobTest.java:148: HttpUrlRequest request = activity.mChromiumRequestFactory.createRequest( There seems to be a lot of repeating boilerplate code in these tests. Should we factor it out? Maybe in followup CL? https://codereview.chromium.org/558333007/diff/120001/components/cronet/andro... File components/cronet/android/test/mock_url_request_job_test.h (right): https://codereview.chromium.org/558333007/diff/120001/components/cronet/andro... components/cronet/android/test/mock_url_request_job_test.h:4: // nit: spurious //
Thanks Misha for the review! PTAL. https://codereview.chromium.org/558333007/diff/120001/components/cronet/andro... File components/cronet/android/test/cronet_tests_jni.cc (right): https://codereview.chromium.org/558333007/diff/120001/components/cronet/andro... components/cronet/android/test/cronet_tests_jni.cc:16: {"BaseAndroid", base::android::RegisterJni}, On 2014/09/22 14:35:56, mef wrote: > We actually don't need "baseandroid" here as it is already in Cronet. > I've left it here as a placeholder. :) Done. I see. Thanks! :) https://codereview.chromium.org/558333007/diff/120001/components/cronet/andro... components/cronet/android/test/cronet_tests_jni.cc:40: On 2014/09/22 14:35:56, mef wrote: > nit: spurious nl. Done. https://codereview.chromium.org/558333007/diff/120001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/MockUrlRequestJobTest.java (right): https://codereview.chromium.org/558333007/diff/120001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/MockUrlRequestJobTest.java:148: HttpUrlRequest request = activity.mChromiumRequestFactory.createRequest( On 2014/09/22 14:35:57, mef wrote: > There seems to be a lot of repeating boilerplate code in these tests. Should we > factor it out? Maybe in followup CL? Done. Great suggestion! I have refactored the repeating part in this test file. https://codereview.chromium.org/558333007/diff/120001/components/cronet/andro... File components/cronet/android/test/mock_url_request_job_test.h (right): https://codereview.chromium.org/558333007/diff/120001/components/cronet/andro... components/cronet/android/test/mock_url_request_job_test.h:4: // On 2014/09/22 14:35:57, mef wrote: > nit: spurious // Done.
This LGTM, modulo comments. https://codereview.chromium.org/558333007/diff/140001/components/cronet/andro... File components/cronet/android/test/cronet_tests_jni.cc (right): https://codereview.chromium.org/558333007/diff/140001/components/cronet/andro... components/cronet/android/test/cronet_tests_jni.cc:16: {"MockURLRequestJobTest", cronet::RegisterMockUrlRequestJobTest}, nit: 2 space indent is more common for initializer lists. https://codereview.chromium.org/558333007/diff/140001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/MockUrlRequestJobTest.java (right): https://codereview.chromium.org/558333007/diff/140001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/MockUrlRequestJobTest.java:62: private MockHttpUrlRequestListener createUrlRequestAndWaitForComplete(String url) { optional: Suggest putting the argument on the next line, just so it's easier to read on code reviews. https://codereview.chromium.org/558333007/diff/140001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/MockUrlRequestJobTest.java:112: assertEquals(404, listener.mHttpStatusCode); Should be able to check the request body here, too. https://codereview.chromium.org/558333007/diff/140001/components/cronet/andro... File components/cronet/android/test/src/org/chromium/cronet_test_apk/CronetTestActivity.java (right): https://codereview.chromium.org/558333007/diff/140001/components/cronet/andro... components/cronet/android/test/src/org/chromium/cronet_test_apk/CronetTestActivity.java:72: return; nit: 4 space indent https://codereview.chromium.org/558333007/diff/140001/components/cronet/andro... components/cronet/android/test/src/org/chromium/cronet_test_apk/CronetTestActivity.java:142: new File(testFileCopy).createNewFile(); Calling "new FileOutputStream" doesn't create a file if one doesn't already exist?
https://codereview.chromium.org/558333007/diff/140001/components/cronet/andro... File components/cronet/android/test/cronet_tests_jni.cc (right): https://codereview.chromium.org/558333007/diff/140001/components/cronet/andro... components/cronet/android/test/cronet_tests_jni.cc:16: {"MockURLRequestJobTest", cronet::RegisterMockUrlRequestJobTest}, On 2014/09/23 14:48:05, mmenke wrote: > nit: 2 space indent is more common for initializer lists. Done. https://codereview.chromium.org/558333007/diff/140001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/MockUrlRequestJobTest.java (right): https://codereview.chromium.org/558333007/diff/140001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/MockUrlRequestJobTest.java:62: private MockHttpUrlRequestListener createUrlRequestAndWaitForComplete(String url) { On 2014/09/23 14:48:05, mmenke wrote: > optional: Suggest putting the argument on the next line, just so it's easier to > read on code reviews. Done. https://codereview.chromium.org/558333007/diff/140001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/MockUrlRequestJobTest.java:112: assertEquals(404, listener.mHttpStatusCode); On 2014/09/23 14:48:05, mmenke wrote: > Should be able to check the request body here, too. Done. https://codereview.chromium.org/558333007/diff/140001/components/cronet/andro... File components/cronet/android/test/src/org/chromium/cronet_test_apk/CronetTestActivity.java (right): https://codereview.chromium.org/558333007/diff/140001/components/cronet/andro... components/cronet/android/test/src/org/chromium/cronet_test_apk/CronetTestActivity.java:72: return; On 2014/09/23 14:48:05, mmenke wrote: > nit: 4 space indent Done. https://codereview.chromium.org/558333007/diff/140001/components/cronet/andro... components/cronet/android/test/src/org/chromium/cronet_test_apk/CronetTestActivity.java:142: new File(testFileCopy).createNewFile(); On 2014/09/23 14:48:05, mmenke wrote: > Calling "new FileOutputStream" doesn't create a file if one doesn't already > exist? Done. You are right! It does create a file if not exists according to the doc (http://developer.android.com/reference/java/io/FileOutputStream.html#FileOutp...). I have removed and it works fine.
The CQ bit was checked by xunjieli@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/558333007/160001
The CQ bit was unchecked by mmenke@chromium.org
On 2014/09/23 15:22:17, I haz the power (commit-bot) wrote: > CQ is trying da patch. Follow status at > https://chromium-cq-status.appspot.com/patch-status/558333007/160001 Unecking the commit box - you should wait for Misha to sign off, too, since he's also a reviewer of the CL.
https://codereview.chromium.org/558333007/diff/140001/components/cronet.gypi File components/cronet.gypi (right): https://codereview.chromium.org/558333007/diff/140001/components/cronet.gypi#... components/cronet.gypi:420: '../net/url_request/url_request_file_job.cc', Is there particular reason to include these as source files instead of adding dependencies on other projects?
Sorry! https://codereview.chromium.org/558333007/diff/140001/components/cronet.gypi File components/cronet.gypi (right): https://codereview.chromium.org/558333007/diff/140001/components/cronet.gypi#... components/cronet.gypi:420: '../net/url_request/url_request_file_job.cc', On 2014/09/23 15:24:57, mef wrote: > Is there particular reason to include these as source files instead of adding > dependencies on other projects? I saw net.gyp (https://code.google.com/p/chromium/codesearch#chromium/src/net/net.gyp&l=172) excluded these source files if disable_file_support == 1. I don't see a target that has these files included by default. Matt, since you made the refactoring, any thoughts?
lgtm https://codereview.chromium.org/558333007/diff/140001/components/cronet.gypi File components/cronet.gypi (right): https://codereview.chromium.org/558333007/diff/140001/components/cronet.gypi#... components/cronet.gypi:420: '../net/url_request/url_request_file_job.cc', On 2014/09/23 15:35:58, xunjieli wrote: > On 2014/09/23 15:24:57, mef wrote: > > Is there particular reason to include these as source files instead of adding > > dependencies on other projects? > > I saw net.gyp > (https://code.google.com/p/chromium/codesearch#chromium/src/net/net.gyp&l=172) > excluded these source files if disable_file_support == 1. I don't see a target > that has these files included by default. > > Matt, since you made the refactoring, any thoughts? Ah, that makes sense.
On 2014/09/23 15:35:58, xunjieli wrote: > Sorry! > > https://codereview.chromium.org/558333007/diff/140001/components/cronet.gypi > File components/cronet.gypi (right): > > https://codereview.chromium.org/558333007/diff/140001/components/cronet.gypi#... > components/cronet.gypi:420: '../net/url_request/url_request_file_job.cc', > On 2014/09/23 15:24:57, mef wrote: > > Is there particular reason to include these as source files instead of adding > > dependencies on other projects? > > I saw net.gyp > (https://code.google.com/p/chromium/codesearch#chromium/src/net/net.gyp&l=172) > excluded these source files if disable_file_support == 1. I don't see a target > that has these files included by default. > > Matt, since you made the refactoring, any thoughts? Not really - the issue is there are two icu calls used for URLRequestFileJob getting a directory listing, and URLRequestMockHttpJob inherits from URLRequestFileJob, though it's not used for directory listings. Duplicating the file reading logic seems excessive, as does making a new parent class for the two, that just doesn't support directory listings. I suppose we could conditionally include URLRequestFileJob is the net_test_util project when excluding file support, but that doesn't really seem any better.
The CQ bit was checked by xunjieli@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/558333007/180001
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as 9f0f61faabc4d306bfc421167f9566cf1df8a0ae
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/a21168de1ab2004932a309db77bf9601a7b8df75 Cr-Commit-Position: refs/heads/master@{#296201} |
