Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1127983002:
Add connectivity check for Android feedback. (Closed)
DescriptionAdd connectivity check for Android feedback.
This CL adds functionality to check whether
http://clients4.google.com/generate_204 URL can be reached and returns
status code HTTP 204. The cache is bypassed and no redirects are allowed.
The functionality this provides will be used by the feedback system to
provide additional data in the report akin to a "Is this thing on?" type
test.
BUG=386395
Committed: https://crrev.com/4f93cf666056c263520d0b922c009594d330220d
Cr-Commit-Position: refs/heads/master@{#329981}
Patch Set 1 #Patch Set 2 : Update main comment and change how test works #Patch Set 3 : Add support for timeout. #Patch Set 4 : Post all callbacks #Patch Set 5 : Fix all tests and add temporary server fix #
Total comments: 12
Patch Set 6 : Addressed comments #
Total comments: 4
Patch Set 7 : Rebased #Patch Set 8 : added timeout as mandatory arg, fixed ScopedJavaGlobalRef constructor. #Patch Set 9 : Removed null-checks for profile and url, fixed scoped_ptr style #
Total comments: 2
Patch Set 10 : Change to not store TimeDelta by reference. #Patch Set 11 : Move test to new test package, and merge with upstream (conflict in chrome_jni_registrar.cc) #
Messages
Total messages: 39 (17 generated)
The CQ bit was checked by nyquist@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1127983002/80001
nyquist@chromium.org changed reviewers: + cjhopman@chromium.org, jbudorick@chromium.org
cjhopman: PTAL (except net/ if you don't want to... it's more or less a copy-paste from https://codereview.chromium.org/869083004/diff/1/net/test/android/javatests/s... ). jbudorick: PTAL chrome/android/javatests focusing on how the server is used. Please do the server fix in net/ in another CL like we discussed. I'll happily rebase this CL once it lands.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
no objections to the use of BaseHttpTestServer https://codereview.chromium.org/1127983002/diff/80001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/feedback/ConnectivityCheckerTest.java (right): https://codereview.chromium.org/1127983002/diff/80001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/feedback/ConnectivityCheckerTest.java:180: semaphore.acquire(); nit: Most tests using a semaphore like this appear to use the timeout overload of tryAcquire.
https://codereview.chromium.org/1127983002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/feedback/ConnectivityChecker.java (right): https://codereview.chromium.org/1127983002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/feedback/ConnectivityChecker.java:55: ThreadUtils.runOnUiThread(new Runnable() { will the callback always be called on the ui thread? Can you just crash if one of these is null instead? https://codereview.chromium.org/1127983002/diff/80001/chrome/browser/android/... File chrome/browser/android/feedback/connectivity_checker.cc (right): https://codereview.chromium.org/1127983002/diff/80001/chrome/browser/android/... chrome/browser/android/feedback/connectivity_checker.cc:42: new base::android::ScopedJavaGlobalRef<jobject>(); You can just pass the j_callback to the constructor here. I'd probably even just inline it in the PostTask call below https://codereview.chromium.org/1127983002/diff/80001/chrome/browser/android/... chrome/browser/android/feedback/connectivity_checker.cc:96: base::MessageLoop::current()->DeleteSoon(FROM_HERE, this); There's a race here that could cause a double delete: basically if the url fetcher and the timeout happen at essentially the same time (fetcher first), two delete tasks could be posted. In that case the callback would actually be called twice, also.
The CQ bit was checked by nyquist@chromium.org to run a CQ dry run
cjhopman: PTAL https://codereview.chromium.org/1127983002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/feedback/ConnectivityChecker.java (right): https://codereview.chromium.org/1127983002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/feedback/ConnectivityChecker.java:55: ThreadUtils.runOnUiThread(new Runnable() { On 2015/05/07 00:09:09, cjhopman wrote: > will the callback always be called on the ui thread? Yes. Both the URLFetcher and the timer for timeout use the same thread as they are created on, and getting the network request context requires us being on the UI thread, so all this stuff happens on the UI thread. > Can you just crash if one of these is null instead? I personally prefer not crashing and just letting the user fill in the feedback form without any helpful info from this. I would be OK with changing the response from a boolean to an enum though, in these failure-cases. That might even be helpful. I'd rather do that as a follow-up though, and possibly with some UMA added around it. https://codereview.chromium.org/1127983002/diff/80001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/feedback/ConnectivityCheckerTest.java (right): https://codereview.chromium.org/1127983002/diff/80001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/feedback/ConnectivityCheckerTest.java:180: semaphore.acquire(); On 2015/05/06 23:57:35, jbudorick wrote: > nit: Most tests using a semaphore like this appear to use the timeout overload > of tryAcquire. Done. https://codereview.chromium.org/1127983002/diff/80001/chrome/browser/android/... File chrome/browser/android/feedback/connectivity_checker.cc (right): https://codereview.chromium.org/1127983002/diff/80001/chrome/browser/android/... chrome/browser/android/feedback/connectivity_checker.cc:42: new base::android::ScopedJavaGlobalRef<jobject>(); On 2015/05/07 00:09:09, cjhopman wrote: > You can just pass the j_callback to the constructor here. I'd probably even just > inline it in the PostTask call below I for some reason could not get that to work. Happy to change this if you have a tips for how to get it to work though, because I get the following error when doing that: n file included from ../../chrome/browser/android/feedback/connectivity_checker.cc:5: In file included from ../../chrome/browser/android/feedback/connectivity_checker.h:8: In file included from ../../base/android/jni_android.h:13: ../../base/android/scoped_java_ref.h:196:28: error: member reference type '_jobject *const' is a pointer; maybe you meant to use '->'? this->Reset(NULL, other.obj()); ~~~~~^ -> ../../base/android/scoped_java_ref.h:183:11: note: in instantiation of function template specialization 'base::android::ScopedJavaGlobalRef<_jobject *>::Reset<_jobject *>' requested here this->Reset(other); ^ ../../chrome/browser/android/feedback/connectivity_checker.cc:42:11: note: in instantiation of function template specialization 'base::android::ScopedJavaGlobalRef<_jobject *>::ScopedJavaGlobalRef<_jobject *>' requested here new base::android::ScopedJavaGlobalRef<jobject>(j_callback); ^ In file included from ../../chrome/browser/android/feedback/connectivity_checker.cc:5: In file included from ../../chrome/browser/android/feedback/connectivity_checker.h:8: In file included from ../../base/android/jni_android.h:13: ../../base/android/scoped_java_ref.h:196:29: error: no member named 'obj' in '_jobject' this->Reset(NULL, other.obj()); ~~~~~ ^ 2 errors generated. ninja: build stopped: subcommand failed. https://codereview.chromium.org/1127983002/diff/80001/chrome/browser/android/... chrome/browser/android/feedback/connectivity_checker.cc:96: base::MessageLoop::current()->DeleteSoon(FROM_HERE, this); On 2015/05/07 00:09:10, cjhopman wrote: > There's a race here that could cause a double delete: basically if the url > fetcher and the timeout happen at essentially the same time (fetcher first), two > delete tasks could be posted. In that case the callback would actually be called > twice, also. Doh! Thanks. Added a boolean to track the destruction. Everything happens on the same thread, so there shouldn't be concurrency issues with that.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1127983002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
https://codereview.chromium.org/1127983002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/feedback/ConnectivityChecker.java (right): https://codereview.chromium.org/1127983002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/feedback/ConnectivityChecker.java:55: ThreadUtils.runOnUiThread(new Runnable() { On 2015/05/07 05:35:46, nyquist wrote: > On 2015/05/07 00:09:09, cjhopman wrote: > > will the callback always be called on the ui thread? > > Yes. Both the URLFetcher and the timer for timeout use the same thread as they > are created on, and getting the network request context requires us being on the > UI thread, so all this stuff happens on the UI thread. > > > Can you just crash if one of these is null instead? > > I personally prefer not crashing and just letting the user fill in the feedback > form without any helpful info from this. I would be OK with changing the > response from a boolean to an enum though, in these failure-cases. That might > even be helpful. I'd rather do that as a follow-up though, and possibly with > some UMA added around it. Sure, but I was thinking that handling null profile/url could be pushed up to the caller. There, they may know that one or the other or both will never be null. https://codereview.chromium.org/1127983002/diff/80001/chrome/browser/android/... File chrome/browser/android/feedback/connectivity_checker.cc (right): https://codereview.chromium.org/1127983002/diff/80001/chrome/browser/android/... chrome/browser/android/feedback/connectivity_checker.cc:42: new base::android::ScopedJavaGlobalRef<jobject>(); On 2015/05/07 05:35:46, nyquist wrote: > On 2015/05/07 00:09:09, cjhopman wrote: > > You can just pass the j_callback to the constructor here. I'd probably even > just > > inline it in the PostTask call below > > I for some reason could not get that to work. Happy to change this if you have a > tips for how to get it to work though, because I get the following error when > doing that: > n file included from > ../../chrome/browser/android/feedback/connectivity_checker.cc:5: > In file included from > ../../chrome/browser/android/feedback/connectivity_checker.h:8: > In file included from ../../base/android/jni_android.h:13: > ../../base/android/scoped_java_ref.h:196:28: error: member reference type > '_jobject *const' is a pointer; maybe you meant to use '->'? > this->Reset(NULL, other.obj()); > ~~~~~^ > -> > ../../base/android/scoped_java_ref.h:183:11: note: in instantiation of function > template specialization 'base::android::ScopedJavaGlobalRef<_jobject > *>::Reset<_jobject *>' requested here > this->Reset(other); > ^ > ../../chrome/browser/android/feedback/connectivity_checker.cc:42:11: note: in > instantiation of function template specialization > 'base::android::ScopedJavaGlobalRef<_jobject *>::ScopedJavaGlobalRef<_jobject > *>' requested here > new base::android::ScopedJavaGlobalRef<jobject>(j_callback); > ^ > In file included from > ../../chrome/browser/android/feedback/connectivity_checker.cc:5: > In file included from > ../../chrome/browser/android/feedback/connectivity_checker.h:8: > In file included from ../../base/android/jni_android.h:13: > ../../base/android/scoped_java_ref.h:196:29: error: no member named 'obj' in > '_jobject' > this->Reset(NULL, other.obj()); > ~~~~~ ^ > 2 errors generated. > ninja: build stopped: subcommand failed. I'd say you should just add the appropriate constructor to ScopedJavaGlobalRef... it's silly that it doesn't have it. But, up to you. https://codereview.chromium.org/1127983002/diff/100001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/feedback/ConnectivityCheckerTest.java (right): https://codereview.chromium.org/1127983002/diff/100001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/feedback/ConnectivityCheckerTest.java:181: semaphore.tryAcquire(TIMEOUT_MS, TimeUnit.MILLISECONDS); Should this fail if tryAcquire fails? https://codereview.chromium.org/1127983002/diff/100001/chrome/browser/android... File chrome/browser/android/feedback/connectivity_checker.cc (right): https://codereview.chromium.org/1127983002/diff/100001/chrome/browser/android... chrome/browser/android/feedback/connectivity_checker.cc:92: DCHECK_EQ(url_fetcher_.get(), source); i'd put this after the early return stuff
The CQ bit was checked by nyquist@chromium.org to run a CQ dry run
cjhopman: PTAL https://codereview.chromium.org/1127983002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/feedback/ConnectivityChecker.java (right): https://codereview.chromium.org/1127983002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/feedback/ConnectivityChecker.java:55: ThreadUtils.runOnUiThread(new Runnable() { On 2015/05/08 18:58:39, cjhopman wrote: > On 2015/05/07 05:35:46, nyquist wrote: > > On 2015/05/07 00:09:09, cjhopman wrote: > > > will the callback always be called on the ui thread? > > > > Yes. Both the URLFetcher and the timer for timeout use the same thread as they > > are created on, and getting the network request context requires us being on > the > > UI thread, so all this stuff happens on the UI thread. > > > > > Can you just crash if one of these is null instead? > > > > I personally prefer not crashing and just letting the user fill in the > feedback > > form without any helpful info from this. I would be OK with changing the > > response from a boolean to an enum though, in these failure-cases. That might > > even be helpful. I'd rather do that as a follow-up though, and possibly with > > some UMA added around it. > > Sure, but I was thinking that handling null profile/url could be pushed up to > the caller. There, they may know that one or the other or both will never be > null. Done. https://codereview.chromium.org/1127983002/diff/80001/chrome/browser/android/... File chrome/browser/android/feedback/connectivity_checker.cc (right): https://codereview.chromium.org/1127983002/diff/80001/chrome/browser/android/... chrome/browser/android/feedback/connectivity_checker.cc:42: new base::android::ScopedJavaGlobalRef<jobject>(); On 2015/05/08 18:58:39, cjhopman wrote: > On 2015/05/07 05:35:46, nyquist wrote: > > On 2015/05/07 00:09:09, cjhopman wrote: > > > You can just pass the j_callback to the constructor here. I'd probably even > > just > > > inline it in the PostTask call below > > > > I for some reason could not get that to work. Happy to change this if you have > a > > tips for how to get it to work though, because I get the following error when > > doing that: > > n file included from > > ../../chrome/browser/android/feedback/connectivity_checker.cc:5: > > In file included from > > ../../chrome/browser/android/feedback/connectivity_checker.h:8: > > In file included from ../../base/android/jni_android.h:13: > > ../../base/android/scoped_java_ref.h:196:28: error: member reference type > > '_jobject *const' is a pointer; maybe you meant to use '->'? > > this->Reset(NULL, other.obj()); > > ~~~~~^ > > -> > > ../../base/android/scoped_java_ref.h:183:11: note: in instantiation of > function > > template specialization 'base::android::ScopedJavaGlobalRef<_jobject > > *>::Reset<_jobject *>' requested here > > this->Reset(other); > > ^ > > ../../chrome/browser/android/feedback/connectivity_checker.cc:42:11: note: in > > instantiation of function template specialization > > 'base::android::ScopedJavaGlobalRef<_jobject *>::ScopedJavaGlobalRef<_jobject > > *>' requested here > > new base::android::ScopedJavaGlobalRef<jobject>(j_callback); > > ^ > > In file included from > > ../../chrome/browser/android/feedback/connectivity_checker.cc:5: > > In file included from > > ../../chrome/browser/android/feedback/connectivity_checker.h:8: > > In file included from ../../base/android/jni_android.h:13: > > ../../base/android/scoped_java_ref.h:196:29: error: no member named 'obj' in > > '_jobject' > > this->Reset(NULL, other.obj()); > > ~~~~~ ^ > > 2 errors generated. > > ninja: build stopped: subcommand failed. > > I'd say you should just add the appropriate constructor to > ScopedJavaGlobalRef... it's silly that it doesn't have it. > > But, up to you. Done. https://codereview.chromium.org/1127983002/diff/100001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/feedback/ConnectivityCheckerTest.java (right): https://codereview.chromium.org/1127983002/diff/100001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/feedback/ConnectivityCheckerTest.java:181: semaphore.tryAcquire(TIMEOUT_MS, TimeUnit.MILLISECONDS); On 2015/05/08 18:58:39, cjhopman wrote: > Should this fail if tryAcquire fails? Done. https://codereview.chromium.org/1127983002/diff/100001/chrome/browser/android... File chrome/browser/android/feedback/connectivity_checker.cc (right): https://codereview.chromium.org/1127983002/diff/100001/chrome/browser/android... chrome/browser/android/feedback/connectivity_checker.cc:92: DCHECK_EQ(url_fetcher_.get(), source); On 2015/05/08 18:58:39, cjhopman wrote: > i'd put this after the early return stuff Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1127983002/160001
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
yfriedman@chromium.org changed reviewers: + yfriedman@chromium.org
https://codereview.chromium.org/1127983002/diff/160001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/feedback/ConnectivityCheckerTest.java (right): https://codereview.chromium.org/1127983002/diff/160001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/feedback/ConnectivityCheckerTest.java:10: import org.apache.http.HttpException; org.apache.http is deprecated and being removed: https://code.google.com/p/chromium/issues/detail?id=468871&q=owner%3Apkotwicz... Please don't add new usages
The CQ bit was checked by nyquist@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from cjhopman@chromium.org Link to the patchset: https://codereview.chromium.org/1127983002/#ps180001 (title: "Change to not store TimeDelta by reference.")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1127983002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...) android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by nyquist@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from cjhopman@chromium.org Link to the patchset: https://codereview.chromium.org/1127983002/#ps200001 (title: "Move test to new test package, and merge with upstream (conflict in chrome_jni_registrar.cc)")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1127983002/200001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by nyquist@chromium.org
The CQ bit was unchecked by nyquist@chromium.org
https://codereview.chromium.org/1127983002/diff/160001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/feedback/ConnectivityCheckerTest.java (right): https://codereview.chromium.org/1127983002/diff/160001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/feedback/ConnectivityCheckerTest.java:10: import org.apache.http.HttpException; On 2015/05/13 19:16:57, Yaron wrote: > org.apache.http is deprecated and being removed: > https://code.google.com/p/chromium/issues/detail?id=468871&q=owner%3Apkotwicz... > > Please don't add new usages Discussed offline. jbudorick@ will update this code when he updates the test server to not depend on org.apache.http.
The CQ bit was checked by nyquist@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1127983002/200001
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/4f93cf666056c263520d0b922c009594d330220d Cr-Commit-Position: refs/heads/master@{#329981} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
