Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
909893003:
Add code to prefetch a DNS resolution for a given URL. (Closed)
|
Created:
5 years, 10 months ago by Benoit L Modified:
5 years, 9 months ago Reviewers:
cbentzel, jochen (gone - plz use gerrit), pasko-google - do not use, nyquist, bengr, pasko CC:
chromium-reviews, cbentzel+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd code to prefetch a DNS resolution for a given URL.
BUG=458216
Committed: https://crrev.com/77eea007f704f0e4f0eae49889f58fd8584004a5
Cr-Commit-Position: refs/heads/master@{#319255}
Patch Set 1 #Patch Set 2 : Added BUG. #
Total comments: 2
Patch Set 3 : . #
Total comments: 7
Patch Set 4 : Address comments. #
Total comments: 10
Patch Set 5 : Address comments. #
Total comments: 2
Patch Set 6 : . #Patch Set 7 : Rebase. #Patch Set 8 : Rebase. #Patch Set 9 : Finx findbugs fase positive. #
Messages
Total messages: 41 (14 generated)
lizeb@chromium.org changed reviewers: + pasko@chromium.org
lizeb@chromium.org changed reviewers: + bengr@chromium.orghello
lizeb@chromium.org changed reviewers: + bengr@chromium.orghello
Hello, Here is a CL adding code to prefetch a DNS request on startup with intent, for Android. It is based off the following discussion on net-dev@chromium.org: https://groups.google.com/a/chromium.org/forum/#!topic/net-dev/cfsOYoe73Z8
lizeb@chromium.org changed reviewers: + bengr@chromium.org - bengr@chromium.orghello
DRPSettings.java lgtm with nit https://codereview.chromium.org/909893003/diff/20001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettings.java (right): https://codereview.chromium.org/909893003/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettings.java:62: SharedPreferences preferences = PreferenceManager.getDefaultSharedPreferences(context); Do you need the temporary? E.g., PreferenceManager.getDefaultSharedPreferencess(context).getBoolean(...);
New patchsets have been uploaded after l-g-t-m from bengr@chromium.org
https://codereview.chromium.org/909893003/diff/20001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettings.java (right): https://codereview.chromium.org/909893003/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettings.java:62: SharedPreferences preferences = PreferenceManager.getDefaultSharedPreferences(context); On 2015/02/13 16:46:39, bengr wrote: > Do you need the temporary? E.g., > PreferenceManager.getDefaultSharedPreferencess(context).getBoolean(...); Done.
lizeb@chromium.org changed reviewers: + cbentzel@chromium.org
Hello, Here is a CL implementing DNS pre-resolution for Chromium on Android for intent-based starts, as discussed here: https://groups.google.com/a/chromium.org/forum/#!topic/net-dev/cfsOYoe73Z8 . Please take a look, thank you.
Friendly ping. :-)
non-owner lgtm https://codereview.chromium.org/909893003/diff/40001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettings.java (right): https://codereview.chromium.org/909893003/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettings.java:114: nativeSetDataReductionProxyEnabled(mNativeDataReductionProxySettings, enabled); this suggests keeping DRP state (enabled/disabled) in two places. Can we make one authoritative place for that (in prefs, as you suggest)?
lizeb@chromium.org changed reviewers: + miguelg@chromium.org
miguelg@chromium.org: Please review changes in
On 2015/02/20 09:15:14, lizeb wrote: > mailto:miguelg@chromium.org: Please review changes in It doesn't look like this was completed. Anyway, I'm not an OWNER for any of this but glad that it's happening.
pasko@google.com changed reviewers: + nyquist@chromium.org - miguelg@chromium.org
pasko@google.com changed reviewers: + pasko@google.com
nyquist: you were suggested as a better reviewer by Miguel, congrats and please review
https://codereview.chromium.org/909893003/diff/40001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/WarmupManager.java (right): https://codereview.chromium.org/909893003/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/WarmupManager.java:177: InetAddress address = InetAddress.getByName(url.getHost()); Since address is unused, is it necessary to have it at all? Does findbugs not complain? https://codereview.chromium.org/909893003/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/WarmupManager.java:195: public static void maybePrefetchDnsForUrlInBackground(Context context, String url) { I believe we usually put static methods at the top of the class. I don't feel strongly about it though. https://codereview.chromium.org/909893003/diff/40001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettings.java (right): https://codereview.chromium.org/909893003/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettings.java:114: nativeSetDataReductionProxyEnabled(mNativeDataReductionProxySettings, enabled); On 2015/02/19 16:09:44, pasko wrote: > this suggests keeping DRP state (enabled/disabled) in two places. Can we make > one authoritative place for that (in prefs, as you suggest)? Yes, please at least keep them in sync. So when native loads, at least the Java pref should be updated. It seems to me that it is possible to change the settings in the component directly, which would mean that the Java-pref could be become out of date.
Thank you for the review! https://codereview.chromium.org/909893003/diff/40001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/WarmupManager.java (right): https://codereview.chromium.org/909893003/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/WarmupManager.java:177: InetAddress address = InetAddress.getByName(url.getHost()); On 2015/02/25 18:26:11, nyquist wrote: > Since address is unused, is it necessary to have it at all? Does findbugs not > complain? Done. https://codereview.chromium.org/909893003/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/WarmupManager.java:195: public static void maybePrefetchDnsForUrlInBackground(Context context, String url) { On 2015/02/25 18:26:11, nyquist wrote: > I believe we usually put static methods at the top of the class. I don't feel > strongly about it though. Acknowledged. https://codereview.chromium.org/909893003/diff/40001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettings.java (right): https://codereview.chromium.org/909893003/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettings.java:114: nativeSetDataReductionProxyEnabled(mNativeDataReductionProxySettings, enabled); On 2015/02/25 18:26:11, nyquist wrote: > On 2015/02/19 16:09:44, pasko wrote: > > this suggests keeping DRP state (enabled/disabled) in two places. Can we make > > one authoritative place for that (in prefs, as you suggest)? > > Yes, please at least keep them in sync. So when native loads, at least the Java > pref should be updated. It seems to me that it is possible to change the > settings in the component directly, which would mean that the Java-pref could be > become out of date. I've added a method to update the java-side value after native load. The alternatives didn't seem very appealing to me (mostly since the Java preference need a Context to be read and updated), but I'm really open to a better suggestion. This requires a call to initialize() after native load (which is already done in a different form downstream).
https://codereview.chromium.org/909893003/diff/60001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettings.java (right): https://codereview.chromium.org/909893003/diff/60001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettings.java:61: return PreferenceManager.getDefaultSharedPreferences(context).getBoolean( This still does not cover the case where DataReductionProxySettings::SetDataReductionProxyEnabled(...) is called directly in native. At that point, the Java preference would be out of date. https://codereview.chromium.org/909893003/diff/60001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettings.java:75: final boolean enabled = sSettings.isDataReductionProxyEnabled(); Nit: Unnecessary final. https://codereview.chromium.org/909893003/diff/60001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettings.java:76: PreferenceManager.getDefaultSharedPreferences(context).edit() This is a potential slow operation on many devices, and it is unfortunate to have to do this on startup.
Thank you for the review and comments. https://codereview.chromium.org/909893003/diff/60001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettings.java (right): https://codereview.chromium.org/909893003/diff/60001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettings.java:61: return PreferenceManager.getDefaultSharedPreferences(context).getBoolean( On 2015/02/27 04:28:24, nyquist wrote: > This still does not cover the case where > DataReductionProxySettings::SetDataReductionProxyEnabled(...) is called directly > in native. At that point, the Java preference would be out of date. Yes, I agree, this is clearly not ideal. But this is only used at startup, and being wrong is not a big issue here. That is, unless DataReductionProxySettings::SetDataReductionProxyEnabled() is called from native during each session, this will be right. Also, detecting that the proxy is enabled whereas it is not would only lead to a worse startup time (for one startup at most), and enabling the proxy without going through the UI seem unlikely (it is not done today). Do you have a suggestion on how to make it better? https://codereview.chromium.org/909893003/diff/60001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettings.java:76: PreferenceManager.getDefaultSharedPreferences(context).edit() On 2015/02/27 04:28:24, nyquist wrote: > This is a potential slow operation on many devices, and it is unfortunate to > have to do this on startup. I have profiled it as being a handful of ms at most in another context (in ResourceExtractor). Also, we call getDefaultSharedPreferences() on each startup in ResourceExtractor (although in this case it is in an AsyncTask). So I think that the added startup time is not significant, in regards of the savings. What do you think?
https://codereview.chromium.org/909893003/diff/60001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettings.java (right): https://codereview.chromium.org/909893003/diff/60001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettings.java:61: return PreferenceManager.getDefaultSharedPreferences(context).getBoolean( On 2015/02/27 16:15:56, lizeb wrote: > On 2015/02/27 04:28:24, nyquist wrote: > > This still does not cover the case where > > DataReductionProxySettings::SetDataReductionProxyEnabled(...) is called > directly > > in native. At that point, the Java preference would be out of date. > > Yes, I agree, this is clearly not ideal. > But this is only used at startup, and being wrong is not a big issue here. That > is, unless DataReductionProxySettings::SetDataReductionProxyEnabled() is called > from native during each session, this will be right. > > Also, detecting that the proxy is enabled whereas it is not would only lead to a > worse startup time (for one startup at most), and enabling the proxy without > going through the UI seem unlikely (it is not done today). > > Do you have a suggestion on how to make it better? In case going forward, there are other callers of DataReductionProxySettings::SetDataReductionProxyEnabled that do not go through this class, this feels a bit iffy. Say it at some point a new feature gets added that enables the proxy to send a specially crafted response to turn off the proxy on the client. This would probably be implemented in a cross-platform way in native code. On Android then, the cached value in Java might be wrong on the first startup after that. I would prefer adding a listener to the native preference when native has loaded for that reason. However, I wouldn't go so far as to say it would be a requirement for this though, as it currently does not happen and I don't know if it ever will. Maybe you could add a clarifying comment that the cached Java preference might be out of date (both false-positive and false-negative)? Anyway, I've voiced my concern and I will leave it up to you to decide. https://codereview.chromium.org/909893003/diff/60001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettings.java:76: PreferenceManager.getDefaultSharedPreferences(context).edit() On 2015/02/27 16:15:56, lizeb wrote: > On 2015/02/27 04:28:24, nyquist wrote: > > This is a potential slow operation on many devices, and it is unfortunate to > > have to do this on startup. > > I have profiled it as being a handful of ms at most in another context (in > ResourceExtractor). Also, we call getDefaultSharedPreferences() on each startup > in ResourceExtractor (although in this case it is in an AsyncTask). So I think > that the added startup time is not significant, in regards of the savings. > What do you think? Do you think you could live with .apply() instead of .commit()? That causes the sync-to-disk to happen in the background.
Thank you for the review. Here is an updated CL. https://codereview.chromium.org/909893003/diff/60001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettings.java (right): https://codereview.chromium.org/909893003/diff/60001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettings.java:61: return PreferenceManager.getDefaultSharedPreferences(context).getBoolean( On 2015/02/27 18:30:15, nyquist wrote: > On 2015/02/27 16:15:56, lizeb wrote: > > On 2015/02/27 04:28:24, nyquist wrote: > > > This still does not cover the case where > > > DataReductionProxySettings::SetDataReductionProxyEnabled(...) is called > > directly > > > in native. At that point, the Java preference would be out of date. > > > > Yes, I agree, this is clearly not ideal. > > But this is only used at startup, and being wrong is not a big issue here. > That > > is, unless DataReductionProxySettings::SetDataReductionProxyEnabled() is > called > > from native during each session, this will be right. > > > > Also, detecting that the proxy is enabled whereas it is not would only lead to > a > > worse startup time (for one startup at most), and enabling the proxy without > > going through the UI seem unlikely (it is not done today). > > > > Do you have a suggestion on how to make it better? > > In case going forward, there are other callers of > DataReductionProxySettings::SetDataReductionProxyEnabled that do not go through > this class, this feels a bit iffy. Say it at some point a new feature gets added > that enables the proxy to send a specially crafted response to turn off the > proxy on the client. This would probably be implemented in a cross-platform way > in native code. On Android then, the cached value in Java might be wrong on the > first startup after that. > > I would prefer adding a listener to the native preference when native has loaded > for that reason. However, I wouldn't go so far as to say it would be a > requirement for this though, as it currently does not happen and I don't know if > it ever will. > > Maybe you could add a clarifying comment that the cached Java preference might > be out of date (both false-positive and false-negative)? > > Anyway, I've voiced my concern and I will leave it up to you to decide. Thank you for the explanation. I suggest to keep it as it is for now, as this is simpler, and is only being used as an optimization. I have added a comment in the javadoc clarifying the behavior, and a TODO assigned to me with your suggestion. https://codereview.chromium.org/909893003/diff/60001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettings.java:75: final boolean enabled = sSettings.isDataReductionProxyEnabled(); On 2015/02/27 04:28:24, nyquist wrote: > Nit: Unnecessary final. Done. https://codereview.chromium.org/909893003/diff/60001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettings.java:76: PreferenceManager.getDefaultSharedPreferences(context).edit() On 2015/02/27 18:30:15, nyquist wrote: > On 2015/02/27 16:15:56, lizeb wrote: > > On 2015/02/27 04:28:24, nyquist wrote: > > > This is a potential slow operation on many devices, and it is unfortunate > to > > > have to do this on startup. > > > > I have profiled it as being a handful of ms at most in another context (in > > ResourceExtractor). Also, we call getDefaultSharedPreferences() on each > startup > > in ResourceExtractor (although in this case it is in an AsyncTask). So I think > > that the added startup time is not significant, in regards of the savings. > > What do you think? > > Do you think you could live with .apply() instead of .commit()? That causes the > sync-to-disk to happen in the background. Yes, thank you for the suggestion. Done.
https://codereview.chromium.org/909893003/diff/80001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettings.java (right): https://codereview.chromium.org/909893003/diff/80001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettings.java:85: .putBoolean(ENABLED_PREFERENCE_TAG, enabled).commit(); So, do you think it would be possible to do .apply() here as well?
https://codereview.chromium.org/909893003/diff/80001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettings.java (right): https://codereview.chromium.org/909893003/diff/80001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettings.java:85: .putBoolean(ENABLED_PREFERENCE_TAG, enabled).commit(); On 2015/03/04 09:19:53, nyquist wrote: > So, do you think it would be possible to do .apply() here as well? Done.
Thanks. lgtm
The CQ bit was checked by lizeb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pasko@chromium.org, bengr@chromium.org, nyquist@chromium.org Link to the patchset: https://codereview.chromium.org/909893003/#ps160001 (title: "Finx findbugs fase positive.")
The CQ bit was unchecked by lizeb@chromium.org
lizeb@chromium.org changed reviewers: + jochen@chromium.org
Hello Jochen, Please review the build/android/findbugs_filter_findbugs_known_bugs.txt change. It is required to silence the findbugs warning that is introduced by this CL. The findbugs error is found in DataReductionProxySettings.java, with a singleton (private static member) being constructed in initialize() and accessed in getInstance(). These two calls need to take place on the same thread (as checked by ThreadUtils.assertOnUiThread()), and getInstance has to be called after initialize (as checked by sSettings != null in getInstance). So I believe that there is no race here. Thank you.
findbugs_known_bugs.txt lgtm
The CQ bit was checked by lizeb@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/909893003/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/77eea007f704f0e4f0eae49889f58fd8584004a5 Cr-Commit-Position: refs/heads/master@{#319255} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
