Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1153263009:
Chromecast: updates Android crash uploader to use HttpURLConnection. (Closed)
DescriptionChromecast: updates Android crash uploader to use HttpURLConnection.
Android deprecated the Apache APIs in L and removed them in M.
R=halliwell@chromium.org,byungchul@chromium.org,alokp@chromium.org
BUG=488200
Committed: https://crrev.com/5c7431f6e59769a81f42616b9b6977e1f65e4abc
Cr-Commit-Position: refs/heads/master@{#333141}
Patch Set 1 #Patch Set 2 : log exception #Patch Set 3 : don't need android.util.log after all #Patch Set 4 : disconnect #
Total comments: 4
Patch Set 5 : reword comment for clarity #
Total comments: 8
Patch Set 6 : address byungchul's comments #Messages
Total messages: 13 (2 generated)
Note: this change is largely analogous to how Clank did the same update. For reference, see: * Tracking bug (crbug/468871, includes link to internal CL) * Current Clank upload code (https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/jav..., recently made public) Note the new "streamCopy" method here is pulled from their implementation. Byungchul/Luke to review, Alok for committer stamp.
https://codereview.chromium.org/1153263009/diff/60001/chromecast/browser/andr... File chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastCrashUploader.java (right): https://codereview.chromium.org/1153263009/diff/60001/chromecast/browser/andr... chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastCrashUploader.java:169: (HttpURLConnection) new URL(mCrashReportUploadUrl).openConnection(); Is there a failure case to handle here? (either return null or exception). https://codereview.chromium.org/1153263009/diff/60001/chromecast/browser/andr... chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastCrashUploader.java:198: // OkHttp fires FNFE on some error codes. is OkHttp comment relevant still?
https://codereview.chromium.org/1153263009/diff/60001/chromecast/browser/andr... File chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastCrashUploader.java (right): https://codereview.chromium.org/1153263009/diff/60001/chromecast/browser/andr... chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastCrashUploader.java:169: (HttpURLConnection) new URL(mCrashReportUploadUrl).openConnection(); On 2015/06/05 01:29:39, halliwell wrote: > Is there a failure case to handle here? (either return null or exception). "new URL" can cause MalformedURLException (caught in IOException below, though since we're hardcoding URLs that's not an issue). openConnection can technically cause an IOException, but per the docs HttpURLConnection only lazily connects as needed. https://codereview.chromium.org/1153263009/diff/60001/chromecast/browser/andr... chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastCrashUploader.java:198: // OkHttp fires FNFE on some error codes. On 2015/06/05 01:29:39, halliwell wrote: > is OkHttp comment relevant still? It is newly relevant. OkHttp is the underlying Android implementation. Reworded for clarity.
On 2015/06/05 17:23:34, gunsch wrote: > https://codereview.chromium.org/1153263009/diff/60001/chromecast/browser/andr... > File > chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastCrashUploader.java > (right): > > https://codereview.chromium.org/1153263009/diff/60001/chromecast/browser/andr... > chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastCrashUploader.java:169: > (HttpURLConnection) new URL(mCrashReportUploadUrl).openConnection(); > On 2015/06/05 01:29:39, halliwell wrote: > > Is there a failure case to handle here? (either return null or exception). > > "new URL" can cause MalformedURLException (caught in IOException below, though > since we're hardcoding URLs that's not an issue). openConnection can technically > cause an IOException, but per the docs HttpURLConnection only lazily connects as > needed. > > https://codereview.chromium.org/1153263009/diff/60001/chromecast/browser/andr... > chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastCrashUploader.java:198: > // OkHttp fires FNFE on some error codes. > On 2015/06/05 01:29:39, halliwell wrote: > > is OkHttp comment relevant still? > > It is newly relevant. OkHttp is the underlying Android implementation. Reworded > for clarity. lgtm
https://codereview.chromium.org/1153263009/diff/80001/chromecast/browser/andr... File chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastCrashUploader.java (right): https://codereview.chromium.org/1153263009/diff/80001/chromecast/browser/andr... chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastCrashUploader.java:171: connection.setRequestProperty("Connection", "Keep-Alive"); Isn't keep-alive default in http 1.1? https://codereview.chromium.org/1153263009/diff/80001/chromecast/browser/andr... chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastCrashUploader.java:183: || responseCode == HttpURLConnection.HTTP_ACCEPTED) { Are you adding 2 more success cases? https://codereview.chromium.org/1153263009/diff/80001/chromecast/browser/andr... chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastCrashUploader.java:194: return; need to clean up resources using "finally"? https://codereview.chromium.org/1153263009/diff/80001/chromecast/browser/andr... chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastCrashUploader.java:203: uploadCrashDumpStream.close(); already closed by streamCopy()
https://codereview.chromium.org/1153263009/diff/80001/chromecast/browser/andr... File chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastCrashUploader.java (right): https://codereview.chromium.org/1153263009/diff/80001/chromecast/browser/andr... chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastCrashUploader.java:171: connection.setRequestProperty("Connection", "Keep-Alive"); On 2015/06/05 17:55:24, byungchul wrote: > Isn't keep-alive default in http 1.1? Looks like HttpURLConnection does by default unless the system property "http.keepAlive" is off (http://developer.android.com/reference/java/net/HttpURLConnection.html). I copied from here: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/jav... but I guess it can be removed. It's certainly not specific to our uploader. https://codereview.chromium.org/1153263009/diff/80001/chromecast/browser/andr... chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastCrashUploader.java:183: || responseCode == HttpURLConnection.HTTP_ACCEPTED) { On 2015/06/05 17:55:24, byungchul wrote: > Are you adding 2 more success cases? Yes, see https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/jav... https://codereview.chromium.org/1153263009/diff/80001/chromecast/browser/andr... chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastCrashUploader.java:194: return; On 2015/06/05 17:55:24, byungchul wrote: > need to clean up resources using "finally"? Done. https://codereview.chromium.org/1153263009/diff/80001/chromecast/browser/andr... chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastCrashUploader.java:203: uploadCrashDumpStream.close(); On 2015/06/05 17:55:24, byungchul wrote: > already closed by streamCopy() Done.
lgtm
lgtm
The CQ bit was checked by gunsch@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from halliwell@chromium.org Link to the patchset: https://codereview.chromium.org/1153263009/#ps100001 (title: "address byungchul's comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1153263009/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/5c7431f6e59769a81f42616b9b6977e1f65e4abc Cr-Commit-Position: refs/heads/master@{#333141} |
