Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(67)

Issue 26763005: android: Use new proxy from PROXY_CHANGE intent (Closed)

Created:
7 years, 2 months ago by Elly Fong-Jones
Modified:
7 years, 2 months ago
Reviewers:
cbentzel, Philippe, pliard
CC:
chromium-reviews, craigdh+watch_chromium.org, cbentzel+watch_chromium.org, bulach+watch_chromium.org, yfriedman+watch_chromium.org, ilevy-cc_chromium.org, klundberg+watch_chromium.org, frankf+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@lkgr
Visibility:
Public.

Description

android: Use new proxy from PROXY_CHANGE intent When we get a PROXY_CHANGE intent, the intent comes with the new proxy value; use this value instead of ignoring it and asking the system property store for the new proxy. This fixes a race condition where the system property store may not have the new proxy by the time we receive the intent. BUG=none yet TEST=trybot Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=230208

Patch Set 1 #

Total comments: 24

Patch Set 2 : Fixes for pliard #

Patch Set 3 : Remove dead comment #

Total comments: 1

Patch Set 4 : Use ConvertJavaStringToUTF8 #

Total comments: 9

Patch Set 5 : Add bug number and test instructions #

Patch Set 6 : Document arguments to ProxySettingsChanged #

Total comments: 1

Patch Set 7 : #

Patch Set 8 : Fall back to old behavior #

Patch Set 9 : Fall back to old mechanism #

Patch Set 10 : Fall back to old mechanism #

Patch Set 11 : Add findbugs entry for reflection goo #

Patch Set 12 : Kill remaining findbugs warning #

Patch Set 13 : Catch NPE from intent.getExtras() #

Unified diffs Side-by-side diffs Delta from patch set Stats (+95 lines, -5 lines) Patch
M net/android/java/src/org/chromium/net/ProxyChangeListener.java View 1 2 3 4 5 6 7 8 9 10 11 12 5 chunks +60 lines, -3 lines 0 comments Download
M net/proxy/proxy_config_service_android.h View 1 2 3 4 5 6 1 chunk +6 lines, -1 line 0 comments Download
M net/proxy/proxy_config_service_android.cc View 1 2 3 4 5 6 4 chunks +29 lines, -1 line 0 comments Download

Messages

Total messages: 28 (0 generated)
Elly Fong-Jones
I'm looking especially for advice on the findbugs warnings, since I can't think of how ...
7 years, 2 months ago (2013-10-10 19:55:23 UTC) #1
cbentzel
You should get pliard to review this - he'll be far better than me. https://codereview.chromium.org/26763005/diff/1/net/android/java/src/org/chromium/net/ProxyChangeListener.java ...
7 years, 2 months ago (2013-10-15 12:29:06 UTC) #2
Elly Fong-Jones
pliard, please take a look?
7 years, 2 months ago (2013-10-15 13:55:06 UTC) #3
Philippe
Thanks Elly! LGTM modulo one "major" comment and a few minor nits. I wasn't aware ...
7 years, 2 months ago (2013-10-15 14:33:21 UTC) #4
Elly Fong-Jones
PTAL? https://codereview.chromium.org/26763005/diff/1/net/android/java/src/org/chromium/net/ProxyChangeListener.java File net/android/java/src/org/chromium/net/ProxyChangeListener.java (right): https://codereview.chromium.org/26763005/diff/1/net/android/java/src/org/chromium/net/ProxyChangeListener.java#newcode15 net/android/java/src/org/chromium/net/ProxyChangeListener.java:15: import java.util.Set; On 2013/10/15 14:33:21, Philippe wrote: > ...
7 years, 2 months ago (2013-10-15 16:37:14 UTC) #5
Philippe
Still LGTM, thanks Elly! https://codereview.chromium.org/26763005/diff/1/net/android/java/src/org/chromium/net/ProxyChangeListener.java File net/android/java/src/org/chromium/net/ProxyChangeListener.java (right): https://codereview.chromium.org/26763005/diff/1/net/android/java/src/org/chromium/net/ProxyChangeListener.java#newcode99 net/android/java/src/org/chromium/net/ProxyChangeListener.java:99: private ProxyConfig extractNewProxy(Intent intent) { ...
7 years, 2 months ago (2013-10-15 16:45:59 UTC) #6
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ellyjones@chromium.org/26763005/22001
7 years, 2 months ago (2013-10-15 17:34:36 UTC) #7
commit-bot: I haz the power
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_presubmit&number=30288
7 years, 2 months ago (2013-10-15 17:58:08 UTC) #8
cbentzel
Please add a bug number.
7 years, 2 months ago (2013-10-15 18:31:01 UTC) #9
cbentzel
https://codereview.chromium.org/26763005/diff/22001/net/android/java/src/org/chromium/net/ProxyChangeListener.java File net/android/java/src/org/chromium/net/ProxyChangeListener.java (right): https://codereview.chromium.org/26763005/diff/22001/net/android/java/src/org/chromium/net/ProxyChangeListener.java#newcode83 net/android/java/src/org/chromium/net/ProxyChangeListener.java:83: ProxyConfig cfg = extractNewProxy(intent); Do older versions of Android ...
7 years, 2 months ago (2013-10-15 18:59:42 UTC) #10
Elly Fong-Jones
https://codereview.chromium.org/26763005/diff/22001/net/android/java/src/org/chromium/net/ProxyChangeListener.java File net/android/java/src/org/chromium/net/ProxyChangeListener.java (right): https://codereview.chromium.org/26763005/diff/22001/net/android/java/src/org/chromium/net/ProxyChangeListener.java#newcode83 net/android/java/src/org/chromium/net/ProxyChangeListener.java:83: ProxyConfig cfg = extractNewProxy(intent); On 2013/10/15 18:59:42, cbentzel wrote: ...
7 years, 2 months ago (2013-10-15 19:50:41 UTC) #11
Philippe
https://codereview.chromium.org/26763005/diff/22001/net/android/java/src/org/chromium/net/ProxyChangeListener.java File net/android/java/src/org/chromium/net/ProxyChangeListener.java (right): https://codereview.chromium.org/26763005/diff/22001/net/android/java/src/org/chromium/net/ProxyChangeListener.java#newcode109 net/android/java/src/org/chromium/net/ProxyChangeListener.java:109: String host = (String)getHostMethod.invoke(props); On 2013/10/15 19:50:42, Elly Jones ...
7 years, 2 months ago (2013-10-15 20:23:37 UTC) #12
cbentzel
https://codereview.chromium.org/26763005/diff/35001/net/android/java/src/org/chromium/net/ProxyChangeListener.java File net/android/java/src/org/chromium/net/ProxyChangeListener.java (right): https://codereview.chromium.org/26763005/diff/35001/net/android/java/src/org/chromium/net/ProxyChangeListener.java#newcode84 net/android/java/src/org/chromium/net/ProxyChangeListener.java:84: if (cfg != null) { I wonder if we ...
7 years, 2 months ago (2013-10-16 09:49:23 UTC) #13
Elly Fong-Jones
On 2013/10/16 09:49:23, cbentzel wrote: > https://codereview.chromium.org/26763005/diff/35001/net/android/java/src/org/chromium/net/ProxyChangeListener.java > File net/android/java/src/org/chromium/net/ProxyChangeListener.java (right): > > https://codereview.chromium.org/26763005/diff/35001/net/android/java/src/org/chromium/net/ProxyChangeListener.java#newcode84 > ...
7 years, 2 months ago (2013-10-16 15:24:40 UTC) #14
cbentzel
On 2013/10/16 15:24:40, Elly Jones wrote: > On 2013/10/16 09:49:23, cbentzel wrote: > > > ...
7 years, 2 months ago (2013-10-16 16:03:47 UTC) #15
Elly Fong-Jones
On 2013/10/16 16:03:47, cbentzel wrote: > On 2013/10/16 15:24:40, Elly Jones wrote: > > On ...
7 years, 2 months ago (2013-10-16 20:20:10 UTC) #16
Elly Fong-Jones
On 2013/10/16 20:20:10, Elly Jones wrote: > On 2013/10/16 16:03:47, cbentzel wrote: > > On ...
7 years, 2 months ago (2013-10-16 20:39:53 UTC) #17
cbentzel
LGTM I get something about "old chunk failed" when doing diffs but perhaps that's a ...
7 years, 2 months ago (2013-10-16 20:44:25 UTC) #18
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ellyjones@chromium.org/26763005/56001
7 years, 2 months ago (2013-10-17 13:40:33 UTC) #19
commit-bot: I haz the power
Retried try job too often on android_dbg for step(s) slave_steps http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_dbg&number=113359
7 years, 2 months ago (2013-10-17 15:33:59 UTC) #20
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ellyjones@chromium.org/26763005/86001
7 years, 2 months ago (2013-10-17 15:55:44 UTC) #21
commit-bot: I haz the power
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_presubmit&number=31027
7 years, 2 months ago (2013-10-17 16:44:34 UTC) #22
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ellyjones@chromium.org/26763005/86002
7 years, 2 months ago (2013-10-17 17:35:42 UTC) #23
commit-bot: I haz the power
Retried try job too often on android_dbg for step(s) slave_steps http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_dbg&number=113540
7 years, 2 months ago (2013-10-17 23:16:48 UTC) #24
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ellyjones@chromium.org/26763005/86002
7 years, 2 months ago (2013-10-21 15:23:38 UTC) #25
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ellyjones@chromium.org/26763005/262001
7 years, 2 months ago (2013-10-22 15:14:45 UTC) #27
commit-bot: I haz the power
7 years, 2 months ago (2013-10-22 21:18:26 UTC) #28
Message was sent while issue was closed.
Change committed as 230208

Powered by Google App Engine
This is Rietveld 408576698