Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
45693002:
Add ExternalPrerenderRequestHandler and related API (Closed)
|
Created:
7 years, 1 month ago by Yusuf Modified:
7 years, 1 month ago CC:
chromium-reviews, tburkard+watch_chromium.org, gavinp+prer_chromium.org, cbentzel+watch_chromium.org, dominich+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd ExternalPrerenderRequestHandler and related API
This class will be used to handle prerender requests coming from external applications.
For better tracking and also supportig referrer, a separate PrerenderManager API is
added.
BUG=297859
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=233457
Patch Set 1 #Patch Set 2 : Added the android files #
Total comments: 13
Patch Set 3 : Styling fixes and added other API #
Total comments: 28
Patch Set 4 : cbentzel's comments #Patch Set 5 : Renamed the files #
Total comments: 18
Patch Set 6 : Nit fixes and fixing the test #
Total comments: 8
Patch Set 7 : Fixes to cbnetzel's comments and added more tests #
Total comments: 4
Patch Set 8 : Fixed testCancelPrerender to use private Runnable #
Total comments: 14
Patch Set 9 : Testing fixes #
Total comments: 3
Patch Set 10 : Removed polling for criteria #
Total comments: 2
Patch Set 11 : Fixed imports #Messages
Total messages: 32 (0 generated)
PTAL I know the class name is kind of long, but I think it describes the purpose best this way. Will add prerender/ OWNERS if this looks good.
mostly style comments. I think you avoid the jni wrapper though. https://codereview.chromium.org/45693002/diff/30001/chrome/android/java/src/o... File chrome/android/java/src/org/chromium/chrome/browser/prerender/ExternalPrerenderRequestHandler.java (right): https://codereview.chromium.org/45693002/diff/30001/chrome/android/java/src/o... chrome/android/java/src/org/chromium/chrome/browser/prerender/ExternalPrerenderRequestHandler.java:8: * Wrapper that allows passing a Profile reference around in the Java layer. copy paste-o https://codereview.chromium.org/45693002/diff/30001/chrome/browser/android/ex... File chrome/browser/android/external_prerender_request_handler.cc (right): https://codereview.chromium.org/45693002/diff/30001/chrome/browser/android/ex... chrome/browser/android/external_prerender_request_handler.cc:21: using base::android::AttachCurrentThread; unused https://codereview.chromium.org/45693002/diff/30001/chrome/browser/android/ex... chrome/browser/android/external_prerender_request_handler.cc:23: using content::WebContents; inline https://codereview.chromium.org/45693002/diff/30001/chrome/browser/android/ex... chrome/browser/android/external_prerender_request_handler.cc:25: using prerender::PrerenderManager; I think your code should be in the prerender namespace https://codereview.chromium.org/45693002/diff/30001/chrome/browser/android/ex... chrome/browser/android/external_prerender_request_handler.cc:33: Profile* profile = g_browser_process->profile_manager()->GetDefaultProfile(); Pass this in. https://codereview.chromium.org/45693002/diff/30001/chrome/browser/android/ex... chrome/browser/android/external_prerender_request_handler.cc:65: return prerender::PrerenderManagerFactory::GetForProfile(profile); Inline this (you only need this line) https://codereview.chromium.org/45693002/diff/30001/chrome/browser/android/ex... chrome/browser/android/external_prerender_request_handler.cc:72: if (prerender_handle_.get()) Hmm. The destructor is never called. so this is kind of dead code. You could switch this to just be a util function as I mentioned in the internal review
On 2013/10/25 23:32:44, Yaron wrote: > mostly style comments. I think you avoid the jni wrapper though. > > https://codereview.chromium.org/45693002/diff/30001/chrome/android/java/src/o... > File > chrome/android/java/src/org/chromium/chrome/browser/prerender/ExternalPrerenderRequestHandler.java > (right): > > https://codereview.chromium.org/45693002/diff/30001/chrome/android/java/src/o... > chrome/android/java/src/org/chromium/chrome/browser/prerender/ExternalPrerenderRequestHandler.java:8: > * Wrapper that allows passing a Profile reference around in the Java layer. > copy paste-o > > https://codereview.chromium.org/45693002/diff/30001/chrome/browser/android/ex... > File chrome/browser/android/external_prerender_request_handler.cc (right): > > https://codereview.chromium.org/45693002/diff/30001/chrome/browser/android/ex... > chrome/browser/android/external_prerender_request_handler.cc:21: using > base::android::AttachCurrentThread; > unused > > https://codereview.chromium.org/45693002/diff/30001/chrome/browser/android/ex... > chrome/browser/android/external_prerender_request_handler.cc:23: using > content::WebContents; > inline > > https://codereview.chromium.org/45693002/diff/30001/chrome/browser/android/ex... > chrome/browser/android/external_prerender_request_handler.cc:25: using > prerender::PrerenderManager; > I think your code should be in the prerender namespace > > https://codereview.chromium.org/45693002/diff/30001/chrome/browser/android/ex... > chrome/browser/android/external_prerender_request_handler.cc:33: Profile* > profile = g_browser_process->profile_manager()->GetDefaultProfile(); > Pass this in. > > https://codereview.chromium.org/45693002/diff/30001/chrome/browser/android/ex... > chrome/browser/android/external_prerender_request_handler.cc:65: return > prerender::PrerenderManagerFactory::GetForProfile(profile); > Inline this (you only need this line) > > https://codereview.chromium.org/45693002/diff/30001/chrome/browser/android/ex... > chrome/browser/android/external_prerender_request_handler.cc:72: if > (prerender_handle_.get()) > Hmm. The destructor is never called. so this is kind of dead code. You could > switch this to just be a util function as I mentioned in the internal review Will do. I am not sure I understand the jni wrapper comment though. I need it to be a native side class so that we have something that holds on to the prerender_handle instance as the prerender gets canceled on the manager side when the prerender_handle doesnt have an owner, I think.
On 2013/10/25 23:46:58, Yusuf wrote: > On 2013/10/25 23:32:44, Yaron wrote: > > mostly style comments. I think you avoid the jni wrapper though. > > > > > https://codereview.chromium.org/45693002/diff/30001/chrome/android/java/src/o... > > File > > > chrome/android/java/src/org/chromium/chrome/browser/prerender/ExternalPrerenderRequestHandler.java > > (right): > > > > > https://codereview.chromium.org/45693002/diff/30001/chrome/android/java/src/o... > > > chrome/android/java/src/org/chromium/chrome/browser/prerender/ExternalPrerenderRequestHandler.java:8: > > * Wrapper that allows passing a Profile reference around in the Java layer. > > copy paste-o > > > > > https://codereview.chromium.org/45693002/diff/30001/chrome/browser/android/ex... > > File chrome/browser/android/external_prerender_request_handler.cc (right): > > > > > https://codereview.chromium.org/45693002/diff/30001/chrome/browser/android/ex... > > chrome/browser/android/external_prerender_request_handler.cc:21: using > > base::android::AttachCurrentThread; > > unused > > > > > https://codereview.chromium.org/45693002/diff/30001/chrome/browser/android/ex... > > chrome/browser/android/external_prerender_request_handler.cc:23: using > > content::WebContents; > > inline > > > > > https://codereview.chromium.org/45693002/diff/30001/chrome/browser/android/ex... > > chrome/browser/android/external_prerender_request_handler.cc:25: using > > prerender::PrerenderManager; > > I think your code should be in the prerender namespace > > > > > https://codereview.chromium.org/45693002/diff/30001/chrome/browser/android/ex... > > chrome/browser/android/external_prerender_request_handler.cc:33: Profile* > > profile = g_browser_process->profile_manager()->GetDefaultProfile(); > > Pass this in. > > > > > https://codereview.chromium.org/45693002/diff/30001/chrome/browser/android/ex... > > chrome/browser/android/external_prerender_request_handler.cc:65: return > > prerender::PrerenderManagerFactory::GetForProfile(profile); > > Inline this (you only need this line) > > > > > https://codereview.chromium.org/45693002/diff/30001/chrome/browser/android/ex... > > chrome/browser/android/external_prerender_request_handler.cc:72: if > > (prerender_handle_.get()) > > Hmm. The destructor is never called. so this is kind of dead code. You could > > switch this to just be a util function as I mentioned in the internal review > > Will do. I am not sure I understand the jni wrapper comment though. I need it to > be a native side class so that we have something > that holds on to the prerender_handle instance as the prerender gets canceled on > the manager side when the prerender_handle doesnt > have an owner, I think. I see. You could use a LazyInstance holder for that or just construct the instance on the native side. Not sure which is clearer in this case but I think it would be good to avoid the xtra
On 2013/10/25 23:46:58, Yusuf wrote: > On 2013/10/25 23:32:44, Yaron wrote: > > mostly style comments. I think you avoid the jni wrapper though. > > > > > https://codereview.chromium.org/45693002/diff/30001/chrome/android/java/src/o... > > File > > > chrome/android/java/src/org/chromium/chrome/browser/prerender/ExternalPrerenderRequestHandler.java > > (right): > > > > > https://codereview.chromium.org/45693002/diff/30001/chrome/android/java/src/o... > > > chrome/android/java/src/org/chromium/chrome/browser/prerender/ExternalPrerenderRequestHandler.java:8: > > * Wrapper that allows passing a Profile reference around in the Java layer. > > copy paste-o > > > > > https://codereview.chromium.org/45693002/diff/30001/chrome/browser/android/ex... > > File chrome/browser/android/external_prerender_request_handler.cc (right): > > > > > https://codereview.chromium.org/45693002/diff/30001/chrome/browser/android/ex... > > chrome/browser/android/external_prerender_request_handler.cc:21: using > > base::android::AttachCurrentThread; > > unused > > > > > https://codereview.chromium.org/45693002/diff/30001/chrome/browser/android/ex... > > chrome/browser/android/external_prerender_request_handler.cc:23: using > > content::WebContents; > > inline > > > > > https://codereview.chromium.org/45693002/diff/30001/chrome/browser/android/ex... > > chrome/browser/android/external_prerender_request_handler.cc:25: using > > prerender::PrerenderManager; > > I think your code should be in the prerender namespace > > > > > https://codereview.chromium.org/45693002/diff/30001/chrome/browser/android/ex... > > chrome/browser/android/external_prerender_request_handler.cc:33: Profile* > > profile = g_browser_process->profile_manager()->GetDefaultProfile(); > > Pass this in. > > > > > https://codereview.chromium.org/45693002/diff/30001/chrome/browser/android/ex... > > chrome/browser/android/external_prerender_request_handler.cc:65: return > > prerender::PrerenderManagerFactory::GetForProfile(profile); > > Inline this (you only need this line) > > > > > https://codereview.chromium.org/45693002/diff/30001/chrome/browser/android/ex... > > chrome/browser/android/external_prerender_request_handler.cc:72: if > > (prerender_handle_.get()) > > Hmm. The destructor is never called. so this is kind of dead code. You could > > switch this to just be a util function as I mentioned in the internal review > > Will do. I am not sure I understand the jni wrapper comment though. I need it to > be a native side class so that we have something > that holds on to the prerender_handle instance as the prerender gets canceled on > the manager side when the prerender_handle doesnt > have an owner, I think. I see. You could use a LazyInstance holder for that or just construct the instance on the native side. Not sure which is clearer in this case but I think it would be good to avoid the xtra jni
Kept the jni upstream. This class will be also used to test prerender functionality through java. Currently we can do this only through typing in the omnibox. +cbentzel for /prerender changes https://codereview.chromium.org/45693002/diff/30001/chrome/browser/android/ex... File chrome/browser/android/external_prerender_request_handler.cc (right): https://codereview.chromium.org/45693002/diff/30001/chrome/browser/android/ex... chrome/browser/android/external_prerender_request_handler.cc:21: using base::android::AttachCurrentThread; On 2013/10/25 23:32:44, Yaron wrote: > unused Done. https://codereview.chromium.org/45693002/diff/30001/chrome/browser/android/ex... chrome/browser/android/external_prerender_request_handler.cc:23: using content::WebContents; On 2013/10/25 23:32:44, Yaron wrote: > inline Done. https://codereview.chromium.org/45693002/diff/30001/chrome/browser/android/ex... chrome/browser/android/external_prerender_request_handler.cc:25: using prerender::PrerenderManager; On 2013/10/25 23:32:44, Yaron wrote: > I think your code should be in the prerender namespace Done. https://codereview.chromium.org/45693002/diff/30001/chrome/browser/android/ex... chrome/browser/android/external_prerender_request_handler.cc:33: Profile* profile = g_browser_process->profile_manager()->GetDefaultProfile(); On 2013/10/25 23:32:44, Yaron wrote: > Pass this in. Done. https://codereview.chromium.org/45693002/diff/30001/chrome/browser/android/ex... chrome/browser/android/external_prerender_request_handler.cc:65: return prerender::PrerenderManagerFactory::GetForProfile(profile); On 2013/10/25 23:32:44, Yaron wrote: > Inline this (you only need this line) Done. https://codereview.chromium.org/45693002/diff/30001/chrome/browser/android/ex... chrome/browser/android/external_prerender_request_handler.cc:72: if (prerender_handle_.get()) On 2013/10/25 23:32:44, Yaron wrote: > Hmm. The destructor is never called. so this is kind of dead code. You could > switch this to just be a util function as I mentioned in the internal review Done.
You'll want to modify histograms.xml with the new origin enum value.
On 2013/10/30 00:33:44, cbentzel wrote: > You'll want to modify histograms.xml with the new origin enum value. Actually, I'm wrong about this. Apologies.
tburkard may also be interested. Note that this is for prerenders which come via Android intents, so a new Origin was introduced for analysis. https://codereview.chromium.org/45693002/diff/130001/chrome/browser/android/e... File chrome/browser/android/external_prerender_request_handler.cc (right): https://codereview.chromium.org/45693002/diff/130001/chrome/browser/android/e... chrome/browser/android/external_prerender_request_handler.cc:44: prerender::PrerenderManagerFactory::GetForProfile(profile); Note: It's possible for PrerenderManager to be NULL in certain cases (low-memory devices, incognito profiles). Probably safest to check for NULL here and early return, although it's possible to handle all of the constraints at the Java layer. https://codereview.chromium.org/45693002/diff/130001/chrome/browser/android/e... chrome/browser/android/external_prerender_request_handler.cc:46: content::WebContents::Create(content::WebContents::CreateParams(profile)); So is this going to always create a new tab/webcontents? It feels like that might be better to do outside this method and pass it in. What happens, for example, if the prerender gets canceled? https://codereview.chromium.org/45693002/diff/130001/chrome/browser/android/e... chrome/browser/android/external_prerender_request_handler.cc:50: ignore_result(prerender_handle_.release()); TODO(cbentzel): Understand this. https://codereview.chromium.org/45693002/diff/130001/chrome/browser/android/e... chrome/browser/android/external_prerender_request_handler.cc:50: ignore_result(prerender_handle_.release()); TODO(cbentzel): Understand this. https://codereview.chromium.org/45693002/diff/130001/chrome/browser/android/e... chrome/browser/android/external_prerender_request_handler.cc:60: return reinterpret_cast<jint>(web_contents); If you are going this route definitely want reinterpret_cast<int> rather than <jint> However - is there a pointer type which could be used instead? Not sure if Android has plans to go to 64-bit binaries but given that iOS has I worry that there are cases where this will bite in the future. https://codereview.chromium.org/45693002/diff/130001/chrome/browser/android/e... chrome/browser/android/external_prerender_request_handler.cc:75: prerender::PrerenderManagerFactory::GetForProfile(profile); Again - probably good to check for NULL prerender_manager. https://codereview.chromium.org/45693002/diff/130001/chrome/browser/android/e... chrome/browser/android/external_prerender_request_handler.cc:79: std::vector<content::WebContents*> contents = I'm surprised that we don't have a method like this already on PrerenderManager - but I'm assuming you checked. https://codereview.chromium.org/45693002/diff/130001/chrome/browser/android/e... chrome/browser/android/external_prerender_request_handler.cc:90: static jboolean HasPrerenderedUrl(JNIEnv* env, Is this being used at all? I didn't see it declared in the header and it isn't being used in this file as far as I can tell. https://codereview.chromium.org/45693002/diff/130001/chrome/browser/android/e... chrome/browser/android/external_prerender_request_handler.cc:111: static jint Init(JNIEnv* env, Is Init being used at all? https://codereview.chromium.org/45693002/diff/130001/chrome/browser/android/e... File chrome/browser/android/external_prerender_request_handler.h (right): https://codereview.chromium.org/45693002/diff/130001/chrome/browser/android/e... chrome/browser/android/external_prerender_request_handler.h:25: class ExternalPrerenderRequestHandler { Perhaps IntentTriggeredPrerenderRequestHandler? External is a bit of a vague name, https://codereview.chromium.org/45693002/diff/130001/chrome/browser/android/e... chrome/browser/android/external_prerender_request_handler.h:27: int AddPrerender(JNIEnv* env, Please document these methods - what does the return value mean, for example? https://codereview.chromium.org/45693002/diff/130001/chrome/browser/android/e... chrome/browser/android/external_prerender_request_handler.h:31: jstring referrer, referrer is probably OK for now but I could imagine generalizing this to other headers. https://codereview.chromium.org/45693002/diff/130001/chrome/browser/android/e... chrome/browser/android/external_prerender_request_handler.h:43: explicit ExternalPrerenderRequestHandler(); Nit: this should be at the top of the methods. https://codereview.chromium.org/45693002/diff/130001/chrome/browser/android/e... chrome/browser/android/external_prerender_request_handler.h:51: } // namespace prerender Not sure if this should be in the prerender namespace given that it is not in that directory.
Thanks cbentzel@. A few quick responses with a few questions. I will upload a new patch based on what you would prefer. https://codereview.chromium.org/45693002/diff/130001/chrome/browser/android/e... File chrome/browser/android/external_prerender_request_handler.cc (right): https://codereview.chromium.org/45693002/diff/130001/chrome/browser/android/e... chrome/browser/android/external_prerender_request_handler.cc:79: std::vector<content::WebContents*> contents = I did check and I agree that it would be nice to have in prerender_manager. Can I just put this in there as opposed to having it in this class? On 2013/10/30 14:40:33, cbentzel wrote: > I'm surprised that we don't have a method like this already on PrerenderManager > - but I'm assuming you checked. https://codereview.chromium.org/45693002/diff/130001/chrome/browser/android/e... chrome/browser/android/external_prerender_request_handler.cc:90: static jboolean HasPrerenderedUrl(JNIEnv* env, Calls with JNIEnv as first argument, jclass as second and no class association are usually static calls that will come in from Java in Android. See ExternalPrerenderRequestHandler.java. At the end of the file there are nativeHasPrerenderedUrl and nativeInit. We have scripts that create JNI bindings automatically out of these. On 2013/10/30 14:40:33, cbentzel wrote: > Is this being used at all? I didn't see it declared in the header and it isn't > being used in this file as far as I can tell. https://codereview.chromium.org/45693002/diff/130001/chrome/browser/android/e... chrome/browser/android/external_prerender_request_handler.cc:111: static jint Init(JNIEnv* env, See above. On 2013/10/30 14:40:33, cbentzel wrote: > Is Init being used at all? https://codereview.chromium.org/45693002/diff/130001/chrome/browser/android/e... File chrome/browser/android/external_prerender_request_handler.h (right): https://codereview.chromium.org/45693002/diff/130001/chrome/browser/android/e... chrome/browser/android/external_prerender_request_handler.h:25: class ExternalPrerenderRequestHandler { One thing with mentioning intents is, the prerender itself wont actually be triggered by the intent. It will be triggered by a message coming to our service. The intent will be used to eventually load the url. On 2013/10/30 14:40:33, cbentzel wrote: > Perhaps IntentTriggeredPrerenderRequestHandler? External is a bit of a vague > name, https://codereview.chromium.org/45693002/diff/130001/chrome/browser/android/e... chrome/browser/android/external_prerender_request_handler.h:51: } // namespace prerender Would you prefer this to be in prerender/? On 2013/10/30 14:40:33, cbentzel wrote: > Not sure if this should be in the prerender namespace given that it is not in > that directory.
https://codereview.chromium.org/45693002/diff/130001/chrome/browser/android/e... File chrome/browser/android/external_prerender_request_handler.cc (right): https://codereview.chromium.org/45693002/diff/130001/chrome/browser/android/e... chrome/browser/android/external_prerender_request_handler.cc:79: std::vector<content::WebContents*> contents = On 2013/10/30 17:28:55, Yusuf wrote: > I did check and I agree that it would be nice to have in prerender_manager. Can > I just put this in there as opposed to having it in this class? Yes. > > > On 2013/10/30 14:40:33, cbentzel wrote: > > I'm surprised that we don't have a method like this already on > PrerenderManager > > - but I'm assuming you checked. > https://codereview.chromium.org/45693002/diff/130001/chrome/browser/android/e... chrome/browser/android/external_prerender_request_handler.cc:90: static jboolean HasPrerenderedUrl(JNIEnv* env, On 2013/10/30 17:28:55, Yusuf wrote: > Calls with JNIEnv as first argument, jclass as second and no class association > are usually static calls that will come in from Java in Android. > > See ExternalPrerenderRequestHandler.java. At the end of the file there are > nativeHasPrerenderedUrl and nativeInit. We have scripts that create JNI bindings > automatically out of these. Thanks for clarifying that. > > > > On 2013/10/30 14:40:33, cbentzel wrote: > > Is this being used at all? I didn't see it declared in the header and it isn't > > being used in this file as far as I can tell. > https://codereview.chromium.org/45693002/diff/130001/chrome/browser/android/e... File chrome/browser/android/external_prerender_request_handler.h (right): https://codereview.chromium.org/45693002/diff/130001/chrome/browser/android/e... chrome/browser/android/external_prerender_request_handler.h:51: } // namespace prerender On 2013/10/30 17:28:55, Yusuf wrote: > Would you prefer this to be in prerender/? I think it makes more sense there (and the ChromeOS one should also move there). > > > On 2013/10/30 14:40:33, cbentzel wrote: > > Not sure if this should be in the prerender namespace given that it is not in > > that directory. > > > >
PTAL https://codereview.chromium.org/45693002/diff/130001/chrome/browser/android/e... File chrome/browser/android/external_prerender_request_handler.cc (right): https://codereview.chromium.org/45693002/diff/130001/chrome/browser/android/e... chrome/browser/android/external_prerender_request_handler.cc:44: prerender::PrerenderManagerFactory::GetForProfile(profile); On 2013/10/30 14:40:33, cbentzel wrote: > Note: It's possible for PrerenderManager to be NULL in certain cases (low-memory > devices, incognito profiles). Probably safest to check for NULL here and early > return, although it's possible to handle all of the constraints at the Java > layer. Done. https://codereview.chromium.org/45693002/diff/130001/chrome/browser/android/e... chrome/browser/android/external_prerender_request_handler.cc:46: content::WebContents::Create(content::WebContents::CreateParams(profile)); On 2013/10/30 14:40:33, cbentzel wrote: > So is this going to always create a new tab/webcontents? It feels like that > might be better to do outside this method and pass it in. What happens, for > example, if the prerender gets canceled? Done. https://codereview.chromium.org/45693002/diff/130001/chrome/browser/android/e... chrome/browser/android/external_prerender_request_handler.cc:60: return reinterpret_cast<jint>(web_contents); On 2013/10/30 14:40:33, cbentzel wrote: > If you are going this route definitely want reinterpret_cast<int> rather than > <jint> > > However - is there a pointer type which could be used instead? Not sure if > Android has plans to go to 64-bit binaries but given that iOS has I worry that > there are cases where this will bite in the future. Done. https://codereview.chromium.org/45693002/diff/130001/chrome/browser/android/e... File chrome/browser/android/external_prerender_request_handler.h (right): https://codereview.chromium.org/45693002/diff/130001/chrome/browser/android/e... chrome/browser/android/external_prerender_request_handler.h:27: int AddPrerender(JNIEnv* env, On 2013/10/30 14:40:33, cbentzel wrote: > Please document these methods - what does the return value mean, for example? Done. https://codereview.chromium.org/45693002/diff/130001/chrome/browser/android/e... chrome/browser/android/external_prerender_request_handler.h:31: jstring referrer, Noted. On 2013/10/30 14:40:33, cbentzel wrote: > referrer is probably OK for now but I could imagine generalizing this to other > headers. https://codereview.chromium.org/45693002/diff/130001/chrome/browser/android/e... chrome/browser/android/external_prerender_request_handler.h:43: explicit ExternalPrerenderRequestHandler(); On 2013/10/30 14:40:33, cbentzel wrote: > Nit: this should be at the top of the methods. Done.
Now that I have moved the hasPrerenderedUrl to PrerenderManager, looking at what is left, I dont see much reason to have the new class compiling in other platforms. Renaming it with _android. PTAL and let me know if that makes sense.
Mostly nits at this point. https://codereview.chromium.org/45693002/diff/420001/chrome/browser/prerender... File chrome/browser/prerender/external_prerender_handler_android.cc (right): https://codereview.chromium.org/45693002/diff/420001/chrome/browser/prerender... chrome/browser/prerender/external_prerender_handler_android.cc:26: jobject obj, Nit: indent by one line. https://codereview.chromium.org/45693002/diff/420001/chrome/browser/prerender... chrome/browser/prerender/external_prerender_handler_android.cc:37: return 0; Nit: return false; https://codereview.chromium.org/45693002/diff/420001/chrome/browser/prerender... chrome/browser/prerender/external_prerender_handler_android.cc:49: reinterpret_cast<content::WebContents*>(web_contents_ptr); Is this the only way to do the casting - really wish there was a safer way, but I don't know enough about the Java/JNI bindings used. https://codereview.chromium.org/45693002/diff/420001/chrome/browser/prerender... chrome/browser/prerender/external_prerender_handler_android.cc:55: prerender_handle_.reset( t's possible that this won't be able to create a prerender - would you still want to cancel the old prerender in that case? https://codereview.chromium.org/45693002/diff/420001/chrome/browser/prerender... chrome/browser/prerender/external_prerender_handler_android.cc:68: if(prerender_handle_) Nit: space between if and ( https://codereview.chromium.org/45693002/diff/420001/chrome/browser/prerender... chrome/browser/prerender/external_prerender_handler_android.cc:77: Profile* profile = ProfileAndroid::FromProfileAndroid(jprofile); Nit: these all look incorrectly indented - should only be by 2. https://codereview.chromium.org/45693002/diff/420001/chrome/browser/prerender... chrome/browser/prerender/external_prerender_handler_android.cc:79: if (jurl != NULL) Does it make sense for jurl to be NULL? Should we just early return in that case? https://codereview.chromium.org/45693002/diff/420001/chrome/browser/prerender... File chrome/browser/prerender/external_prerender_handler_android.h (right): https://codereview.chromium.org/45693002/diff/420001/chrome/browser/prerender... chrome/browser/prerender/external_prerender_handler_android.h:30: // the given profile. The prerender_handle_ will be resetted with the Nit: prerender_handle_ is an internal implementation detail. You may want to mention that this restricts to just one external prerender at a time. https://codereview.chromium.org/45693002/diff/420001/chrome/browser/prerender... chrome/browser/prerender/external_prerender_handler_android.h:33: jobject obj, Nit: indent by one space to line up with previous line.
https://codereview.chromium.org/45693002/diff/420001/chrome/browser/prerender... File chrome/browser/prerender/external_prerender_handler_android.cc (right): https://codereview.chromium.org/45693002/diff/420001/chrome/browser/prerender... chrome/browser/prerender/external_prerender_handler_android.cc:26: jobject obj, On 2013/11/01 10:45:20, cbentzel wrote: > Nit: indent by one line. Done. https://codereview.chromium.org/45693002/diff/420001/chrome/browser/prerender... chrome/browser/prerender/external_prerender_handler_android.cc:37: return 0; On 2013/11/01 10:45:20, cbentzel wrote: > Nit: return false; Done. https://codereview.chromium.org/45693002/diff/420001/chrome/browser/prerender... chrome/browser/prerender/external_prerender_handler_android.cc:49: reinterpret_cast<content::WebContents*>(web_contents_ptr); AFAIK, this the way to have Java classes hold on to pointers to native instances. In classes where Java side manages the life cycle we have a lot of int mNativeClassNameHere. On 2013/11/01 10:45:20, cbentzel wrote: > Is this the only way to do the casting - really wish there was a safer way, but > I don't know enough about the Java/JNI bindings used. https://codereview.chromium.org/45693002/diff/420001/chrome/browser/prerender... chrome/browser/prerender/external_prerender_handler_android.cc:55: prerender_handle_.reset( Yes. I think I would want cancelCurrentPrerender to function like relieving the prerenderHandler instance from its current task. On 2013/11/01 10:45:20, cbentzel wrote: > t's possible that this won't be able to create a prerender - would you still > want to cancel the old prerender in that case? https://codereview.chromium.org/45693002/diff/420001/chrome/browser/prerender... chrome/browser/prerender/external_prerender_handler_android.cc:68: if(prerender_handle_) On 2013/11/01 10:45:20, cbentzel wrote: > Nit: space between if and ( Done. https://codereview.chromium.org/45693002/diff/420001/chrome/browser/prerender... chrome/browser/prerender/external_prerender_handler_android.cc:77: Profile* profile = ProfileAndroid::FromProfileAndroid(jprofile); On 2013/11/01 10:45:20, cbentzel wrote: > Nit: these all look incorrectly indented - should only be by 2. Done. https://codereview.chromium.org/45693002/diff/420001/chrome/browser/prerender... chrome/browser/prerender/external_prerender_handler_android.cc:79: if (jurl != NULL) On 2013/11/01 10:45:20, cbentzel wrote: > Does it make sense for jurl to be NULL? Should we just early return in that > case? Done. https://codereview.chromium.org/45693002/diff/420001/chrome/browser/prerender... File chrome/browser/prerender/external_prerender_handler_android.h (right): https://codereview.chromium.org/45693002/diff/420001/chrome/browser/prerender... chrome/browser/prerender/external_prerender_handler_android.h:30: // the given profile. The prerender_handle_ will be resetted with the On 2013/11/01 10:45:20, cbentzel wrote: > Nit: prerender_handle_ is an internal implementation detail. You may want to > mention that this restricts to just one external prerender at a time. Done. https://codereview.chromium.org/45693002/diff/420001/chrome/browser/prerender... chrome/browser/prerender/external_prerender_handler_android.h:33: jobject obj, On 2013/11/01 10:45:20, cbentzel wrote: > Nit: indent by one space to line up with previous line. Done.
Sorry I didn't discover these issues until now - but they should be easy to resolve. https://codereview.chromium.org/45693002/diff/730001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/prerender/ExternalPrerenderHandler.java (right): https://codereview.chromium.org/45693002/diff/730001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/prerender/ExternalPrerenderHandler.java:25: int webContentsPtr = ContentViewUtil.createNativeWebContents(false); Does this need to destroy the webContents if nativeAddPrerender doesn't succeed? https://codereview.chromium.org/45693002/diff/730001/chrome/browser/prerender... File chrome/browser/prerender/external_prerender_handler_android.cc (right): https://codereview.chromium.org/45693002/diff/730001/chrome/browser/prerender... chrome/browser/prerender/external_prerender_handler_android.cc:52: // The PrerenderHandle is now owned by the PrerenderManager. I'm confused - where do you see that the PrerenderHandle is owned by the PrerenderManager? This looks like a leak. When the PrerenderManager creates a PrerenderHandle (such as AddPrerender* calls) the returned pointer is owned by the caller. OnNavigateAway will keep the PrerenderData/PrerenderContents alive. However, the handle is still owned externally (and can be deleted once OnNavigateAWay is called). https://codereview.chromium.org/45693002/diff/730001/chrome/browser/prerender... chrome/browser/prerender/external_prerender_handler_android.cc:69: prerender_handle_->OnCancel(); You need to reset the prerender_handle_ here after canceling it as well. Definitely don't want Cancel called on the same handle multiple times - and CancelCurrentPrerender followed by AddPrerender also won't behave how you want.
https://codereview.chromium.org/45693002/diff/730001/chrome/android/javatests... File chrome/android/javatests/src/org/chromium/chrome/browser/prerender/ExternalPrerenderRequestTest.java (right): https://codereview.chromium.org/45693002/diff/730001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/prerender/ExternalPrerenderRequestTest.java:36: public void testAddPrerender() throws ExecutionException { I think you need to add more tests... - Multiple Adds in a row - Cancel without an Add - Add, Cancel, Add - What happens if a prerender can't be done when calling AddPrerender?
Added more tests. PTAL https://codereview.chromium.org/45693002/diff/730001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/prerender/ExternalPrerenderHandler.java (right): https://codereview.chromium.org/45693002/diff/730001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/prerender/ExternalPrerenderHandler.java:25: int webContentsPtr = ContentViewUtil.createNativeWebContents(false); On 2013/11/01 20:22:09, cbentzel wrote: > Does this need to destroy the webContents if nativeAddPrerender doesn't succeed? Done. https://codereview.chromium.org/45693002/diff/730001/chrome/android/javatests... File chrome/android/javatests/src/org/chromium/chrome/browser/prerender/ExternalPrerenderRequestTest.java (right): https://codereview.chromium.org/45693002/diff/730001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/prerender/ExternalPrerenderRequestTest.java:36: public void testAddPrerender() throws ExecutionException { On 2013/11/01 20:23:29, cbentzel wrote: > I think you need to add more tests... > > - Multiple Adds in a row > - Cancel without an Add > - Add, Cancel, Add > - What happens if a prerender can't be done when calling AddPrerender? Done. https://codereview.chromium.org/45693002/diff/730001/chrome/browser/prerender... File chrome/browser/prerender/external_prerender_handler_android.cc (right): https://codereview.chromium.org/45693002/diff/730001/chrome/browser/prerender... chrome/browser/prerender/external_prerender_handler_android.cc:52: // The PrerenderHandle is now owned by the PrerenderManager. On 2013/11/01 20:22:09, cbentzel wrote: > I'm confused - where do you see that the PrerenderHandle is owned by the > PrerenderManager? This looks like a leak. > > When the PrerenderManager creates a PrerenderHandle (such as AddPrerender* > calls) the returned pointer is owned by the caller. > > OnNavigateAway will keep the PrerenderData/PrerenderContents alive. However, the > handle is still owned externally (and can be deleted once OnNavigateAWay is > called). Done. https://codereview.chromium.org/45693002/diff/730001/chrome/browser/prerender... chrome/browser/prerender/external_prerender_handler_android.cc:69: prerender_handle_->OnCancel(); On 2013/11/01 20:22:09, cbentzel wrote: > You need to reset the prerender_handle_ here after canceling it as well. > Definitely don't want Cancel called on the same handle multiple times - and > CancelCurrentPrerender followed by AddPrerender also won't behave how you want. Done.
LGTM No need to wait for me to commit - just a question and a comment about the testing code. https://codereview.chromium.org/45693002/diff/980001/chrome/android/javatests... File chrome/android/javatests/src/org/chromium/chrome/browser/prerender/ExternalPrerenderRequestTest.java (right): https://codereview.chromium.org/45693002/diff/980001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/prerender/ExternalPrerenderRequestTest.java:84: assertTrue(CriteriaHelper.pollForCriteria(new Criteria() { Why do you need this rather than just ThreadUtils.runOnUiThreadBlocking() with mAddYouTubeCallable and look at the integer result? https://codereview.chromium.org/45693002/diff/980001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/prerender/ExternalPrerenderRequestTest.java:104: final Runnable cancelRunnable = new Runnable() { Why can't this use mCancelRunnable?
Thanks a lot! yfriedman@ can you take a look for android changes now? https://codereview.chromium.org/45693002/diff/980001/chrome/android/javatests... File chrome/android/javatests/src/org/chromium/chrome/browser/prerender/ExternalPrerenderRequestTest.java (right): https://codereview.chromium.org/45693002/diff/980001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/prerender/ExternalPrerenderRequestTest.java:84: assertTrue(CriteriaHelper.pollForCriteria(new Criteria() { Looks like there is a set duration where two prerenders can't happen back to back. kMinTimeBetweenPrerendersMs is I think set to 500. This was my way of polling until we get a good prerender. On 2013/11/05 00:39:44, cbentzel wrote: > Why do you need this rather than just ThreadUtils.runOnUiThreadBlocking() with > mAddYouTubeCallable and look at the integer result? https://codereview.chromium.org/45693002/diff/980001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/prerender/ExternalPrerenderRequestTest.java:104: final Runnable cancelRunnable = new Runnable() { Missed this one. Fixed now. On 2013/11/05 00:39:44, cbentzel wrote: > Why can't this use mCancelRunnable?
https://codereview.chromium.org/45693002/diff/1110001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/prerender/ExternalPrerenderHandler.java (right): https://codereview.chromium.org/45693002/diff/1110001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/prerender/ExternalPrerenderHandler.java:24: if (mNativeExternalPrerenderHandler == 0) return 0; This can't happen. You init it in the ctor https://codereview.chromium.org/45693002/diff/1110001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/prerender/ExternalPrerenderHandler.java:35: if (mNativeExternalPrerenderHandler == 0) return; remove https://codereview.chromium.org/45693002/diff/1110001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/prerender/ExternalPrerenderRequestTest.java (right): https://codereview.chromium.org/45693002/diff/1110001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/prerender/ExternalPrerenderRequestTest.java:27: TestHttpServerClient.getUrl("clank/test/data/android/prerender/google.html"); This will fail immediately on the bots. That file isn't accessible upstream. https://codereview.chromium.org/45693002/diff/1110001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/prerender/ExternalPrerenderRequestTest.java:58: mHandler = new ExternalPrerenderHandler(); I think this should be done on the ui thread and not test thread. https://codereview.chromium.org/45693002/diff/1110001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/prerender/ExternalPrerenderRequestTest.java:74: final Profile profile = mProfile; You shouldn't need these. mProfile should be fine (same for others) https://codereview.chromium.org/45693002/diff/1110001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/prerender/ExternalPrerenderRequestTest.java:81: assertFalse(ExternalPrerenderHandler.hasPrerenderedUrl( Is this class intentionally thread-safe? It doesn't look to be but you're using it that way. You might instead make these UiThreadTests: http://developer.android.com/reference/android/test/UiThreadTest.html which would likely simplify things. https://codereview.chromium.org/45693002/diff/1110001/chrome/browser/prerende... File chrome/browser/prerender/external_prerender_handler_android.cc (right): https://codereview.chromium.org/45693002/diff/1110001/chrome/browser/prerende... chrome/browser/prerender/external_prerender_handler_android.cc:97: jclass clazz) { Nit: move up to prev line https://codereview.chromium.org/45693002/diff/1110001/chrome/browser/prerende... File chrome/browser/prerender/external_prerender_handler_android.h (right): https://codereview.chromium.org/45693002/diff/1110001/chrome/browser/prerende... chrome/browser/prerender/external_prerender_handler_android.h:11: #include "base/memory/ref_counted.h" unused
https://codereview.chromium.org/45693002/diff/1110001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/prerender/ExternalPrerenderHandler.java (right): https://codereview.chromium.org/45693002/diff/1110001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/prerender/ExternalPrerenderHandler.java:24: if (mNativeExternalPrerenderHandler == 0) return 0; On 2013/11/05 01:17:54, Yaron wrote: > This can't happen. You init it in the ctor Done. https://codereview.chromium.org/45693002/diff/1110001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/prerender/ExternalPrerenderHandler.java:35: if (mNativeExternalPrerenderHandler == 0) return; On 2013/11/05 01:17:54, Yaron wrote: > remove Done. https://codereview.chromium.org/45693002/diff/1110001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/prerender/ExternalPrerenderRequestTest.java (right): https://codereview.chromium.org/45693002/diff/1110001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/prerender/ExternalPrerenderRequestTest.java:27: TestHttpServerClient.getUrl("clank/test/data/android/prerender/google.html"); Copied the data resources over to chrome. Will remove the downstream versions once these trickle in. On 2013/11/05 01:17:54, Yaron wrote: > This will fail immediately on the bots. That file isn't accessible upstream. https://codereview.chromium.org/45693002/diff/1110001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/prerender/ExternalPrerenderRequestTest.java:58: mHandler = new ExternalPrerenderHandler(); On 2013/11/05 01:17:54, Yaron wrote: > I think this should be done on the ui thread and not test thread. Made the tests UIThreadTest Done. https://codereview.chromium.org/45693002/diff/1110001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/prerender/ExternalPrerenderRequestTest.java:74: final Profile profile = mProfile; On 2013/11/05 01:17:54, Yaron wrote: > You shouldn't need these. mProfile should be fine (same for others) Done. https://codereview.chromium.org/45693002/diff/1110001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/prerender/ExternalPrerenderRequestTest.java:81: assertFalse(ExternalPrerenderHandler.hasPrerenderedUrl( On 2013/11/05 01:17:54, Yaron wrote: > Is this class intentionally thread-safe? It doesn't look to be but you're using > it that way. You might instead make these UiThreadTests: > http://developer.android.com/reference/android/test/UiThreadTest.html which > would likely simplify things. Done.
https://codereview.chromium.org/45693002/diff/1400001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/prerender/ExternalPrerenderRequestTest.java (right): https://codereview.chromium.org/45693002/diff/1400001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/prerender/ExternalPrerenderRequestTest.java:102: assertTrue(CriteriaHelper.pollForCriteria(new Criteria() { Isn't this synchronous? Not sure what the CriteriaHelper is helping with. You're now running the test on the ui thread so it's not like some posted task is getting a chance to run cause you haven't yielded the ui thread and we don't have a nested message loop
https://codereview.chromium.org/45693002/diff/1400001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/prerender/ExternalPrerenderRequestTest.java (right): https://codereview.chromium.org/45693002/diff/1400001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/prerender/ExternalPrerenderRequestTest.java:102: assertTrue(CriteriaHelper.pollForCriteria(new Criteria() { Due to an internal rate limit, I have to wait 0.5 s here to send another prerender and this looked nicer than doing Thread.sleep(). On 2013/11/06 01:22:20, Yaron wrote: > Isn't this synchronous? Not sure what the CriteriaHelper is helping with. You're > now running the test on the ui thread so it's not like some posted task is > getting a chance to run cause you haven't yielded the ui thread and we don't > have a nested message loop
https://codereview.chromium.org/45693002/diff/1400001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/prerender/ExternalPrerenderRequestTest.java (right): https://codereview.chromium.org/45693002/diff/1400001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/prerender/ExternalPrerenderRequestTest.java:102: assertTrue(CriteriaHelper.pollForCriteria(new Criteria() { On 2013/11/06 01:34:14, Yusuf wrote: > Due to an internal rate limit, I have to wait 0.5 s here to send another > prerender and this looked nicer than doing Thread.sleep(). > > > On 2013/11/06 01:22:20, Yaron wrote: > > Isn't this synchronous? Not sure what the CriteriaHelper is helping with. > You're > > now running the test on the ui thread so it's not like some posted task is > > getting a chance to run cause you haven't yielded the ui thread and we don't > > have a nested message loop > I'm don't really agree. This is considerably more complex and relies on use of AtomicInteger. It also is less deterministic in that it's not clear if it was the first prerender request that worked or the 100th. You're not just observing state and waiting for it to happen, each call is a potential mutation.
On 2013/11/06 01:41:38, Yaron wrote: > https://codereview.chromium.org/45693002/diff/1400001/chrome/android/javatest... > File > chrome/android/javatests/src/org/chromium/chrome/browser/prerender/ExternalPrerenderRequestTest.java > (right): > > https://codereview.chromium.org/45693002/diff/1400001/chrome/android/javatest... > chrome/android/javatests/src/org/chromium/chrome/browser/prerender/ExternalPrerenderRequestTest.java:102: > assertTrue(CriteriaHelper.pollForCriteria(new Criteria() { > On 2013/11/06 01:34:14, Yusuf wrote: > > Due to an internal rate limit, I have to wait 0.5 s here to send another > > prerender and this looked nicer than doing Thread.sleep(). > > > > > > On 2013/11/06 01:22:20, Yaron wrote: > > > Isn't this synchronous? Not sure what the CriteriaHelper is helping with. > > You're > > > now running the test on the ui thread so it's not like some posted task is > > > getting a chance to run cause you haven't yielded the ui thread and we don't > > > have a nested message loop > > > > I'm don't really agree. This is considerably more complex and relies on use of > AtomicInteger. It also is less deterministic in that it's not clear if it was > the first prerender request that worked or the 100th. You're not just observing > state and waiting for it to happen, each call is a potential mutation. Point taken :). Uploaded a new patch with Thread.sleep()
lgtm https://codereview.chromium.org/45693002/diff/1570001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/prerender/ExternalPrerenderRequestTest.java (right): https://codereview.chromium.org/45693002/diff/1570001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/prerender/ExternalPrerenderRequestTest.java:21: import java.util.concurrent.atomic.AtomicInteger; You can probably update imports now. Please also run these a few times to ensure they're not flaky given you're relying on sleeps. :)
https://codereview.chromium.org/45693002/diff/1570001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/prerender/ExternalPrerenderRequestTest.java (right): https://codereview.chromium.org/45693002/diff/1570001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/prerender/ExternalPrerenderRequestTest.java:21: import java.util.concurrent.atomic.AtomicInteger; On 2013/11/06 02:22:03, Yaron wrote: > You can probably update imports now. Please also run these a few times to ensure > they're not flaky given you're relying on sleeps. :) Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yusufo@chromium.org/45693002/1620001
Retried try job too often on win_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yusufo@chromium.org/45693002/1620001
Message was sent while issue was closed.
Change committed as 233457 |
