Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2598163002:
WebContentsObserver update for PlzNavigate methods (Closed)
|
Created:
3 years, 12 months ago by shaktisahu Modified:
3 years, 10 months ago CC:
chromium-reviews, dominickn+watch_chromium.org, nasko+codewatch_chromium.org, creis+watch_chromium.org, mdjones+watch_chromium.org, donnd+watch_chromium.org, lizeb+watch-custom-tabs_chromium.org, pkotwicz+watch_chromium.org, agrieve+watch_chromium.org, darin-cc_chromium.org, zpeng+watch_chromium.org, android-webview-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionWebContentsObserver update for PlzNavigate methods
This is the first CL to update the Java WebContentsObserver and its
usages to use the new methods for PlzNavigate. Added a Java class
for NavigationHandle and updated the WebContentsObserverProxy to use
the new methods. Replaced deprecated methods with new methods at
various call sites of WebContentsObserver.
I would upload a second CL which will update the same for
TabWebContentsObserver, TabObserver and its usages.
BUG=676139
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Patch Set 1 #Patch Set 2 : Fixed AwWebContentsObserver #
Total comments: 8
Patch Set 3 : Added isInMainFrame check #Patch Set 4 : Removed Java NavigationHandle #Patch Set 5 : AW fix #
Total comments: 3
Patch Set 6 : Renamed params and fixed crash due to PageTransition type #Patch Set 7 : Revert back a state CHECK #Depends on Patchset: Dependent Patchsets: Messages
Total messages: 30 (19 generated)
Description was changed from ========== WebContentsObserver update for PlzNavigate methods This is the first CL to update the Java WebContentsObserver and its usages to use the new methods for PlzNavigate. Added a Java class for NavigationHandle and updated the WebContentsObserverProxy to use the new methods. Replaced deprecated methods with new methods at various call sites of WebContentsObserver. I would upload a second CL which will update the same for TabWebContentsObserver, TabObserver and its usages. BUG=676139 ========== to ========== WebContentsObserver update for PlzNavigate methods This is the first CL to update the Java WebContentsObserver and its usages to use the new methods for PlzNavigate. Added a Java class for NavigationHandle and updated the WebContentsObserverProxy to use the new methods. Replaced deprecated methods with new methods at various call sites of WebContentsObserver. I would upload a second CL which will update the same for TabWebContentsObserver, TabObserver and its usages. BUG=676139 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by shaktisahu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
shaktisahu@chromium.org changed reviewers: + jam@chromium.org, mariakhomenko@chromium.org
mariakhomenko@chromium.org: Please review changes in jam@chromium.org: Please review changes in
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Thanks for helping :) I assume you added me for content/browser/frame_host. I don't think we want to remove the checks there, since they're meant to guard against calling the getters before the result can be known. If the dchecks are removed, we risk getting silent failures.
shaktisahu@chromium.org changed reviewers: + tedchoc@chromium.org
tedchoc@ : PTAL
A couple of top level comments: - You probably want a separate reviewer for webview code, but maybe after all the rest of the comments are resolved - Hmm, it seems odd that you needed to remove those DCHECKs in navigation_handle, I think we need to understand why the calls are invalid when they are made https://codereview.chromium.org/2598163002/diff/20001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwWebContentsObserver.java (right): https://codereview.chromium.org/2598163002/diff/20001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwWebContentsObserver.java:101: if (!navigation.isSamePage()) { Check for navigation.isInMainFrame() for everything below? You can navigate to a different page in a subframe... https://codereview.chromium.org/2598163002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java (right): https://codereview.chromium.org/2598163002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java:436: private boolean isHttpFailureCode(int httpResultCode) { I think this is now unused https://codereview.chromium.org/2598163002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/PostMessageHandler.java (right): https://codereview.chromium.org/2598163002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/customtabs/PostMessageHandler.java:72: public void didFinishNavigation(NavigationHandle navigation) { I think this should now include a check that we are in main frame navigation. https://codereview.chromium.org/2598163002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/ReaderModeManager.java (right): https://codereview.chromium.org/2598163002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/ReaderModeManager.java:467: // (like those from history.replaceState()). Add a check that inMainFrame() https://codereview.chromium.org/2598163002/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2598163002/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:151: if (navigation.isSamePage()) return; also check for isInMainFrame https://codereview.chromium.org/2598163002/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content_public/browser/NavigationHandle.java (right): https://codereview.chromium.org/2598163002/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content_public/browser/NavigationHandle.java:35: */ nit: How about a comment about this being constructed from native instead. https://codereview.chromium.org/2598163002/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content_public/browser/NavigationHandle.java:36: public NavigationHandle(String url, String validatedUrl, boolean isInMainFrame, I would make this private since nothing on the Java side needs to instantiate it. https://codereview.chromium.org/2598163002/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content_public/browser/WebContentsObserver.java (right): https://codereview.chromium.org/2598163002/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content_public/browser/WebContentsObserver.java:32: // plzNavigate methods. this comment is probably not necessary here. I would instead put @Deprecated tags on the methods we are replacing.
On 2016/12/27 19:31:10, Maria wrote: > A couple of top level comments: > > - You probably want a separate reviewer for webview code, but maybe after all > the rest of the comments are resolved > - Hmm, it seems odd that you needed to remove those DCHECKs in > navigation_handle, I think we need to understand why the calls are invalid when > they are made > > https://codereview.chromium.org/2598163002/diff/20001/android_webview/java/sr... > File > android_webview/java/src/org/chromium/android_webview/AwWebContentsObserver.java > (right): > > https://codereview.chromium.org/2598163002/diff/20001/android_webview/java/sr... > android_webview/java/src/org/chromium/android_webview/AwWebContentsObserver.java:101: > if (!navigation.isSamePage()) { > Check for navigation.isInMainFrame() for everything below? You can navigate to a > different page in a subframe... > > https://codereview.chromium.org/2598163002/diff/20001/chrome/android/java/src... > File > chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java > (right): > > https://codereview.chromium.org/2598163002/diff/20001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java:436: > private boolean isHttpFailureCode(int httpResultCode) { > I think this is now unused > > https://codereview.chromium.org/2598163002/diff/20001/chrome/android/java/src... > File > chrome/android/java/src/org/chromium/chrome/browser/customtabs/PostMessageHandler.java > (right): > > https://codereview.chromium.org/2598163002/diff/20001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/customtabs/PostMessageHandler.java:72: > public void didFinishNavigation(NavigationHandle navigation) { > I think this should now include a check that we are in main frame navigation. > > https://codereview.chromium.org/2598163002/diff/20001/chrome/android/java/src... > File > chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/ReaderModeManager.java > (right): > > https://codereview.chromium.org/2598163002/diff/20001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/ReaderModeManager.java:467: > // (like those from history.replaceState()). > Add a check that inMainFrame() > > https://codereview.chromium.org/2598163002/diff/20001/content/public/android/... > File > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java > (right): > > https://codereview.chromium.org/2598163002/diff/20001/content/public/android/... > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:151: > if (navigation.isSamePage()) return; > also check for isInMainFrame > > https://codereview.chromium.org/2598163002/diff/20001/content/public/android/... > File > content/public/android/java/src/org/chromium/content_public/browser/NavigationHandle.java > (right): > > https://codereview.chromium.org/2598163002/diff/20001/content/public/android/... > content/public/android/java/src/org/chromium/content_public/browser/NavigationHandle.java:35: > */ > nit: How about a comment about this being constructed from native instead. > > https://codereview.chromium.org/2598163002/diff/20001/content/public/android/... > content/public/android/java/src/org/chromium/content_public/browser/NavigationHandle.java:36: > public NavigationHandle(String url, String validatedUrl, boolean isInMainFrame, > I would make this private since nothing on the Java side needs to instantiate > it. > > https://codereview.chromium.org/2598163002/diff/20001/content/public/android/... > File > content/public/android/java/src/org/chromium/content_public/browser/WebContentsObserver.java > (right): > > https://codereview.chromium.org/2598163002/diff/20001/content/public/android/... > content/public/android/java/src/org/chromium/content_public/browser/WebContentsObserver.java:32: > // plzNavigate methods. > this comment is probably not necessary here. I would instead put @Deprecated > tags on the methods we are replacing. I spoke a bit with jam@ and I have some reservations about introducing a NavigationHandle class whose lifetime is different than that of the C++ implementation. In particular, this bit of documentation in web_contents_observer.cc scares me a bit (DidStartNavigation): // Called when a navigation started in the WebContents. |navigation_handle| // is unique to a specific navigation. The same |navigation_handle| will be // provided on subsequent calls to DidRedirectNavigation, DidFinishNavigation, // and ReadyToCommitNavigation when related to this navigation. Observers // should clear any references to |navigation_handle| in DidFinishNavigation, // just before it is destroyed. If we introduce a java NavigationHandle, it seems like it would be easy for a client to have the same expectations of the native one in terms of lifetime and uniqueness. i.e. based on the above comment, I would expect NavigationHandle.equals or even == to work across DidStartNavigation and DidFinishNavigation. Regardless of us using it right now, it seems very prone to misuse if the two apis are so different. I suspect this is also where you are having trouble with the CHECKS in navigation_handle_impl. We are trying to serialize the entire state of the navigation handle to a java object, but that violates the contract. I see two possible options to investigate further: 1.) Do not introduce a NavigationHandle java object yet. Instead, just expose the state that is needed in the various methods. This avoids the lifetime issue. 2.) Create a java object that is owned by the native NavigationHandle C++ object and have it simply forward calls from java to native. I don't have a super strong preference either way, but it might be good to see how many methods we'd need to expose from C++ to java to hit #1. If the number of params that we currently need to pass from C++ to java is small in each of the methods, then I think #1 is slightly preferable for now. If we have to pass the world, then #2 would be easier. If we do end up needing java callers to track uniqueness of navigation handles, then I think we can go the #2 route, but I don't know if we need to build out the machinery for something that might never be used.
On 2016/12/27 19:51:30, Ted C wrote: > On 2016/12/27 19:31:10, Maria wrote: > > A couple of top level comments: > > > > - You probably want a separate reviewer for webview code, but maybe after all > > the rest of the comments are resolved > > - Hmm, it seems odd that you needed to remove those DCHECKs in > > navigation_handle, I think we need to understand why the calls are invalid > when > > they are made > > > > > https://codereview.chromium.org/2598163002/diff/20001/android_webview/java/sr... > > File > > > android_webview/java/src/org/chromium/android_webview/AwWebContentsObserver.java > > (right): > > > > > https://codereview.chromium.org/2598163002/diff/20001/android_webview/java/sr... > > > android_webview/java/src/org/chromium/android_webview/AwWebContentsObserver.java:101: > > if (!navigation.isSamePage()) { > > Check for navigation.isInMainFrame() for everything below? You can navigate to > a > > different page in a subframe... > > > > > https://codereview.chromium.org/2598163002/diff/20001/chrome/android/java/src... > > File > > > chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java > > (right): > > > > > https://codereview.chromium.org/2598163002/diff/20001/chrome/android/java/src... > > > chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java:436: > > private boolean isHttpFailureCode(int httpResultCode) { > > I think this is now unused > > > > > https://codereview.chromium.org/2598163002/diff/20001/chrome/android/java/src... > > File > > > chrome/android/java/src/org/chromium/chrome/browser/customtabs/PostMessageHandler.java > > (right): > > > > > https://codereview.chromium.org/2598163002/diff/20001/chrome/android/java/src... > > > chrome/android/java/src/org/chromium/chrome/browser/customtabs/PostMessageHandler.java:72: > > public void didFinishNavigation(NavigationHandle navigation) { > > I think this should now include a check that we are in main frame navigation. > > > > > https://codereview.chromium.org/2598163002/diff/20001/chrome/android/java/src... > > File > > > chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/ReaderModeManager.java > > (right): > > > > > https://codereview.chromium.org/2598163002/diff/20001/chrome/android/java/src... > > > chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/ReaderModeManager.java:467: > > // (like those from history.replaceState()). > > Add a check that inMainFrame() > > > > > https://codereview.chromium.org/2598163002/diff/20001/content/public/android/... > > File > > > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java > > (right): > > > > > https://codereview.chromium.org/2598163002/diff/20001/content/public/android/... > > > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:151: > > if (navigation.isSamePage()) return; > > also check for isInMainFrame > > > > > https://codereview.chromium.org/2598163002/diff/20001/content/public/android/... > > File > > > content/public/android/java/src/org/chromium/content_public/browser/NavigationHandle.java > > (right): > > > > > https://codereview.chromium.org/2598163002/diff/20001/content/public/android/... > > > content/public/android/java/src/org/chromium/content_public/browser/NavigationHandle.java:35: > > */ > > nit: How about a comment about this being constructed from native instead. > > > > > https://codereview.chromium.org/2598163002/diff/20001/content/public/android/... > > > content/public/android/java/src/org/chromium/content_public/browser/NavigationHandle.java:36: > > public NavigationHandle(String url, String validatedUrl, boolean > isInMainFrame, > > I would make this private since nothing on the Java side needs to instantiate > > it. > > > > > https://codereview.chromium.org/2598163002/diff/20001/content/public/android/... > > File > > > content/public/android/java/src/org/chromium/content_public/browser/WebContentsObserver.java > > (right): > > > > > https://codereview.chromium.org/2598163002/diff/20001/content/public/android/... > > > content/public/android/java/src/org/chromium/content_public/browser/WebContentsObserver.java:32: > > // plzNavigate methods. > > this comment is probably not necessary here. I would instead put @Deprecated > > tags on the methods we are replacing. > > I spoke a bit with jam@ and I have some reservations about introducing a > NavigationHandle class whose lifetime is different than that of the C++ > implementation. > > In particular, this bit of documentation in web_contents_observer.cc > scares me a bit (DidStartNavigation): > > // Called when a navigation started in the WebContents. |navigation_handle| > // is unique to a specific navigation. The same |navigation_handle| will be > // provided on subsequent calls to DidRedirectNavigation, DidFinishNavigation, > // and ReadyToCommitNavigation when related to this navigation. Observers > // should clear any references to |navigation_handle| in DidFinishNavigation, > // just before it is destroyed. > > If we introduce a java NavigationHandle, it seems like it would be easy > for a client to have the same expectations of the native one in terms of > lifetime and uniqueness. > > i.e. based on the above comment, I would expect NavigationHandle.equals or > even == to work across DidStartNavigation and DidFinishNavigation. Regardless > of us using it right now, it seems very prone to misuse if the two apis are > so different. > > I suspect this is also where you are having trouble with the CHECKS in > navigation_handle_impl. We are trying to serialize the entire state > of the navigation handle to a java object, but that violates the contract. > > I see two possible options to investigate further: > > 1.) Do not introduce a NavigationHandle java object yet. Instead, just > expose the state that is needed in the various methods. This avoids > the lifetime issue. > 2.) Create a java object that is owned by the native NavigationHandle > C++ object and have it simply forward calls from java to native. > > I don't have a super strong preference either way, but it might be good > to see how many methods we'd need to expose from C++ to java to hit #1. > If the number of params that we currently need to pass from C++ to java > is small in each of the methods, then I think #1 is slightly preferable > for now. > > If we have to pass the world, then #2 would be easier. > > If we do end up needing java callers to track > uniqueness of navigation handles, then I think we can go the #2 route, but > I don't know if we need to build out the machinery for something that might > never be used. I am a bit worried about #1 slowly increasing the number of parameters over time (even if today we only use a handful) and becoming a big mess in the future, so my preference would be for #2.
On 2016/12/27 20:09:22, Maria wrote: > On 2016/12/27 19:51:30, Ted C wrote: > > On 2016/12/27 19:31:10, Maria wrote: > > > A couple of top level comments: > > > > > > - You probably want a separate reviewer for webview code, but maybe after > all > > > the rest of the comments are resolved > > > - Hmm, it seems odd that you needed to remove those DCHECKs in > > > navigation_handle, I think we need to understand why the calls are invalid > > when > > > they are made > > > > > > > > > https://codereview.chromium.org/2598163002/diff/20001/android_webview/java/sr... > > > File > > > > > > android_webview/java/src/org/chromium/android_webview/AwWebContentsObserver.java > > > (right): > > > > > > > > > https://codereview.chromium.org/2598163002/diff/20001/android_webview/java/sr... > > > > > > android_webview/java/src/org/chromium/android_webview/AwWebContentsObserver.java:101: > > > if (!navigation.isSamePage()) { > > > Check for navigation.isInMainFrame() for everything below? You can navigate > to > > a > > > different page in a subframe... > > > > > > > > > https://codereview.chromium.org/2598163002/diff/20001/chrome/android/java/src... > > > File > > > > > > chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java > > > (right): > > > > > > > > > https://codereview.chromium.org/2598163002/diff/20001/chrome/android/java/src... > > > > > > chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java:436: > > > private boolean isHttpFailureCode(int httpResultCode) { > > > I think this is now unused > > > > > > > > > https://codereview.chromium.org/2598163002/diff/20001/chrome/android/java/src... > > > File > > > > > > chrome/android/java/src/org/chromium/chrome/browser/customtabs/PostMessageHandler.java > > > (right): > > > > > > > > > https://codereview.chromium.org/2598163002/diff/20001/chrome/android/java/src... > > > > > > chrome/android/java/src/org/chromium/chrome/browser/customtabs/PostMessageHandler.java:72: > > > public void didFinishNavigation(NavigationHandle navigation) { > > > I think this should now include a check that we are in main frame > navigation. > > > > > > > > > https://codereview.chromium.org/2598163002/diff/20001/chrome/android/java/src... > > > File > > > > > > chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/ReaderModeManager.java > > > (right): > > > > > > > > > https://codereview.chromium.org/2598163002/diff/20001/chrome/android/java/src... > > > > > > chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/ReaderModeManager.java:467: > > > // (like those from history.replaceState()). > > > Add a check that inMainFrame() > > > > > > > > > https://codereview.chromium.org/2598163002/diff/20001/content/public/android/... > > > File > > > > > > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java > > > (right): > > > > > > > > > https://codereview.chromium.org/2598163002/diff/20001/content/public/android/... > > > > > > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:151: > > > if (navigation.isSamePage()) return; > > > also check for isInMainFrame > > > > > > > > > https://codereview.chromium.org/2598163002/diff/20001/content/public/android/... > > > File > > > > > > content/public/android/java/src/org/chromium/content_public/browser/NavigationHandle.java > > > (right): > > > > > > > > > https://codereview.chromium.org/2598163002/diff/20001/content/public/android/... > > > > > > content/public/android/java/src/org/chromium/content_public/browser/NavigationHandle.java:35: > > > */ > > > nit: How about a comment about this being constructed from native instead. > > > > > > > > > https://codereview.chromium.org/2598163002/diff/20001/content/public/android/... > > > > > > content/public/android/java/src/org/chromium/content_public/browser/NavigationHandle.java:36: > > > public NavigationHandle(String url, String validatedUrl, boolean > > isInMainFrame, > > > I would make this private since nothing on the Java side needs to > instantiate > > > it. > > > > > > > > > https://codereview.chromium.org/2598163002/diff/20001/content/public/android/... > > > File > > > > > > content/public/android/java/src/org/chromium/content_public/browser/WebContentsObserver.java > > > (right): > > > > > > > > > https://codereview.chromium.org/2598163002/diff/20001/content/public/android/... > > > > > > content/public/android/java/src/org/chromium/content_public/browser/WebContentsObserver.java:32: > > > // plzNavigate methods. > > > this comment is probably not necessary here. I would instead put @Deprecated > > > tags on the methods we are replacing. > > > > I spoke a bit with jam@ and I have some reservations about introducing a > > NavigationHandle class whose lifetime is different than that of the C++ > > implementation. > > > > In particular, this bit of documentation in web_contents_observer.cc > > scares me a bit (DidStartNavigation): > > > > // Called when a navigation started in the WebContents. |navigation_handle| > > // is unique to a specific navigation. The same |navigation_handle| will be > > // provided on subsequent calls to DidRedirectNavigation, > DidFinishNavigation, > > // and ReadyToCommitNavigation when related to this navigation. Observers > > // should clear any references to |navigation_handle| in > DidFinishNavigation, > > // just before it is destroyed. > > > > If we introduce a java NavigationHandle, it seems like it would be easy > > for a client to have the same expectations of the native one in terms of > > lifetime and uniqueness. > > > > i.e. based on the above comment, I would expect NavigationHandle.equals or > > even == to work across DidStartNavigation and DidFinishNavigation. Regardless > > of us using it right now, it seems very prone to misuse if the two apis are > > so different. > > > > I suspect this is also where you are having trouble with the CHECKS in > > navigation_handle_impl. We are trying to serialize the entire state > > of the navigation handle to a java object, but that violates the contract. > > > > I see two possible options to investigate further: > > > > 1.) Do not introduce a NavigationHandle java object yet. Instead, just > > expose the state that is needed in the various methods. This avoids > > the lifetime issue. > > 2.) Create a java object that is owned by the native NavigationHandle > > C++ object and have it simply forward calls from java to native. > > > > I don't have a super strong preference either way, but it might be good > > to see how many methods we'd need to expose from C++ to java to hit #1. > > If the number of params that we currently need to pass from C++ to java > > is small in each of the methods, then I think #1 is slightly preferable > > for now. > > > > If we have to pass the world, then #2 would be easier. > > > > If we do end up needing java callers to track > > uniqueness of navigation handles, then I think we can go the #2 route, but > > I don't know if we need to build out the machinery for something that might > > never be used. > > I am a bit worried about #1 slowly increasing the number of parameters over time > (even if today we only use a handful) and becoming a big mess in the future, so > my preference would be for #2. While that "could" happen, that is basically the condition I would switch to #2 in the future (if that ever happened). The question is whether we should build for the "now" or for a future that might never happen. Even now, we have params passed from native to java that aren't used (didFailLoad passes wasIgnoredByHandler but that doesn't seem to be used anywhere). But another point in #2s favor is that in the future if a single call sight needs one more piece of data, you don't need to change every observer in the code base as they would hang onto the NavigationHandle already. Again, I'm not super attached to either proposed solution. If you feel strongly that #2 is better, I can't really argue against it (I did propose it in the first place :-P).
On 2016/12/27 20:09:22, Maria wrote: > On 2016/12/27 19:51:30, Ted C wrote: > > On 2016/12/27 19:31:10, Maria wrote: > > > A couple of top level comments: > > > > > > - You probably want a separate reviewer for webview code, but maybe after > all > > > the rest of the comments are resolved > > > - Hmm, it seems odd that you needed to remove those DCHECKs in > > > navigation_handle, I think we need to understand why the calls are invalid > > when > > > they are made > > > > > > > > > https://codereview.chromium.org/2598163002/diff/20001/android_webview/java/sr... > > > File > > > > > > android_webview/java/src/org/chromium/android_webview/AwWebContentsObserver.java > > > (right): > > > > > > > > > https://codereview.chromium.org/2598163002/diff/20001/android_webview/java/sr... > > > > > > android_webview/java/src/org/chromium/android_webview/AwWebContentsObserver.java:101: > > > if (!navigation.isSamePage()) { > > > Check for navigation.isInMainFrame() for everything below? You can navigate > to > > a > > > different page in a subframe... > > > > > > > > > https://codereview.chromium.org/2598163002/diff/20001/chrome/android/java/src... > > > File > > > > > > chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java > > > (right): > > > > > > > > > https://codereview.chromium.org/2598163002/diff/20001/chrome/android/java/src... > > > > > > chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java:436: > > > private boolean isHttpFailureCode(int httpResultCode) { > > > I think this is now unused > > > > > > > > > https://codereview.chromium.org/2598163002/diff/20001/chrome/android/java/src... > > > File > > > > > > chrome/android/java/src/org/chromium/chrome/browser/customtabs/PostMessageHandler.java > > > (right): > > > > > > > > > https://codereview.chromium.org/2598163002/diff/20001/chrome/android/java/src... > > > > > > chrome/android/java/src/org/chromium/chrome/browser/customtabs/PostMessageHandler.java:72: > > > public void didFinishNavigation(NavigationHandle navigation) { > > > I think this should now include a check that we are in main frame > navigation. > > > > > > > > > https://codereview.chromium.org/2598163002/diff/20001/chrome/android/java/src... > > > File > > > > > > chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/ReaderModeManager.java > > > (right): > > > > > > > > > https://codereview.chromium.org/2598163002/diff/20001/chrome/android/java/src... > > > > > > chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/ReaderModeManager.java:467: > > > // (like those from history.replaceState()). > > > Add a check that inMainFrame() > > > > > > > > > https://codereview.chromium.org/2598163002/diff/20001/content/public/android/... > > > File > > > > > > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java > > > (right): > > > > > > > > > https://codereview.chromium.org/2598163002/diff/20001/content/public/android/... > > > > > > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:151: > > > if (navigation.isSamePage()) return; > > > also check for isInMainFrame > > > > > > > > > https://codereview.chromium.org/2598163002/diff/20001/content/public/android/... > > > File > > > > > > content/public/android/java/src/org/chromium/content_public/browser/NavigationHandle.java > > > (right): > > > > > > > > > https://codereview.chromium.org/2598163002/diff/20001/content/public/android/... > > > > > > content/public/android/java/src/org/chromium/content_public/browser/NavigationHandle.java:35: > > > */ > > > nit: How about a comment about this being constructed from native instead. > > > > > > > > > https://codereview.chromium.org/2598163002/diff/20001/content/public/android/... > > > > > > content/public/android/java/src/org/chromium/content_public/browser/NavigationHandle.java:36: > > > public NavigationHandle(String url, String validatedUrl, boolean > > isInMainFrame, > > > I would make this private since nothing on the Java side needs to > instantiate > > > it. > > > > > > > > > https://codereview.chromium.org/2598163002/diff/20001/content/public/android/... > > > File > > > > > > content/public/android/java/src/org/chromium/content_public/browser/WebContentsObserver.java > > > (right): > > > > > > > > > https://codereview.chromium.org/2598163002/diff/20001/content/public/android/... > > > > > > content/public/android/java/src/org/chromium/content_public/browser/WebContentsObserver.java:32: > > > // plzNavigate methods. > > > this comment is probably not necessary here. I would instead put @Deprecated > > > tags on the methods we are replacing. > > > > I spoke a bit with jam@ and I have some reservations about introducing a > > NavigationHandle class whose lifetime is different than that of the C++ > > implementation. > > > > In particular, this bit of documentation in web_contents_observer.cc > > scares me a bit (DidStartNavigation): > > > > // Called when a navigation started in the WebContents. |navigation_handle| > > // is unique to a specific navigation. The same |navigation_handle| will be > > // provided on subsequent calls to DidRedirectNavigation, > DidFinishNavigation, > > // and ReadyToCommitNavigation when related to this navigation. Observers > > // should clear any references to |navigation_handle| in > DidFinishNavigation, > > // just before it is destroyed. > > > > If we introduce a java NavigationHandle, it seems like it would be easy > > for a client to have the same expectations of the native one in terms of > > lifetime and uniqueness. > > > > i.e. based on the above comment, I would expect NavigationHandle.equals or > > even == to work across DidStartNavigation and DidFinishNavigation. Regardless > > of us using it right now, it seems very prone to misuse if the two apis are > > so different. > > > > I suspect this is also where you are having trouble with the CHECKS in > > navigation_handle_impl. We are trying to serialize the entire state > > of the navigation handle to a java object, but that violates the contract. > > > > I see two possible options to investigate further: > > > > 1.) Do not introduce a NavigationHandle java object yet. Instead, just > > expose the state that is needed in the various methods. This avoids > > the lifetime issue. > > 2.) Create a java object that is owned by the native NavigationHandle > > C++ object and have it simply forward calls from java to native. > > > > I don't have a super strong preference either way, but it might be good > > to see how many methods we'd need to expose from C++ to java to hit #1. > > If the number of params that we currently need to pass from C++ to java > > is small in each of the methods, then I think #1 is slightly preferable > > for now. > > > > If we have to pass the world, then #2 would be easier. > > > > If we do end up needing java callers to track > > uniqueness of navigation handles, then I think we can go the #2 route, but > > I don't know if we need to build out the machinery for something that might > > never be used. > > I am a bit worried about #1 slowly increasing the number of parameters over time > (even if today we only use a handful) and becoming a big mess in the future, so > my preference would be for #2. I am also a bit concerned that if we go #2 route, we need to make a JNI call for each param for each of the registered WebContentObservers (and TabObservers). Is that okay?
On 2016/12/27 20:35:26, shaktisahu wrote: > On 2016/12/27 20:09:22, Maria wrote: > > On 2016/12/27 19:51:30, Ted C wrote: > > > On 2016/12/27 19:31:10, Maria wrote: > > > > A couple of top level comments: > > > > > > > > - You probably want a separate reviewer for webview code, but maybe after > > all > > > > the rest of the comments are resolved > > > > - Hmm, it seems odd that you needed to remove those DCHECKs in > > > > navigation_handle, I think we need to understand why the calls are invalid > > > when > > > > they are made > > > > > > > > > > > > > > https://codereview.chromium.org/2598163002/diff/20001/android_webview/java/sr... > > > > File > > > > > > > > > > android_webview/java/src/org/chromium/android_webview/AwWebContentsObserver.java > > > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2598163002/diff/20001/android_webview/java/sr... > > > > > > > > > > android_webview/java/src/org/chromium/android_webview/AwWebContentsObserver.java:101: > > > > if (!navigation.isSamePage()) { > > > > Check for navigation.isInMainFrame() for everything below? You can > navigate > > to > > > a > > > > different page in a subframe... > > > > > > > > > > > > > > https://codereview.chromium.org/2598163002/diff/20001/chrome/android/java/src... > > > > File > > > > > > > > > > chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java > > > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2598163002/diff/20001/chrome/android/java/src... > > > > > > > > > > chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java:436: > > > > private boolean isHttpFailureCode(int httpResultCode) { > > > > I think this is now unused > > > > > > > > > > > > > > https://codereview.chromium.org/2598163002/diff/20001/chrome/android/java/src... > > > > File > > > > > > > > > > chrome/android/java/src/org/chromium/chrome/browser/customtabs/PostMessageHandler.java > > > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2598163002/diff/20001/chrome/android/java/src... > > > > > > > > > > chrome/android/java/src/org/chromium/chrome/browser/customtabs/PostMessageHandler.java:72: > > > > public void didFinishNavigation(NavigationHandle navigation) { > > > > I think this should now include a check that we are in main frame > > navigation. > > > > > > > > > > > > > > https://codereview.chromium.org/2598163002/diff/20001/chrome/android/java/src... > > > > File > > > > > > > > > > chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/ReaderModeManager.java > > > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2598163002/diff/20001/chrome/android/java/src... > > > > > > > > > > chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/ReaderModeManager.java:467: > > > > // (like those from history.replaceState()). > > > > Add a check that inMainFrame() > > > > > > > > > > > > > > https://codereview.chromium.org/2598163002/diff/20001/content/public/android/... > > > > File > > > > > > > > > > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java > > > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2598163002/diff/20001/content/public/android/... > > > > > > > > > > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:151: > > > > if (navigation.isSamePage()) return; > > > > also check for isInMainFrame > > > > > > > > > > > > > > https://codereview.chromium.org/2598163002/diff/20001/content/public/android/... > > > > File > > > > > > > > > > content/public/android/java/src/org/chromium/content_public/browser/NavigationHandle.java > > > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2598163002/diff/20001/content/public/android/... > > > > > > > > > > content/public/android/java/src/org/chromium/content_public/browser/NavigationHandle.java:35: > > > > */ > > > > nit: How about a comment about this being constructed from native instead. > > > > > > > > > > > > > > https://codereview.chromium.org/2598163002/diff/20001/content/public/android/... > > > > > > > > > > content/public/android/java/src/org/chromium/content_public/browser/NavigationHandle.java:36: > > > > public NavigationHandle(String url, String validatedUrl, boolean > > > isInMainFrame, > > > > I would make this private since nothing on the Java side needs to > > instantiate > > > > it. > > > > > > > > > > > > > > https://codereview.chromium.org/2598163002/diff/20001/content/public/android/... > > > > File > > > > > > > > > > content/public/android/java/src/org/chromium/content_public/browser/WebContentsObserver.java > > > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2598163002/diff/20001/content/public/android/... > > > > > > > > > > content/public/android/java/src/org/chromium/content_public/browser/WebContentsObserver.java:32: > > > > // plzNavigate methods. > > > > this comment is probably not necessary here. I would instead put > @Deprecated > > > > tags on the methods we are replacing. > > > > > > I spoke a bit with jam@ and I have some reservations about introducing a > > > NavigationHandle class whose lifetime is different than that of the C++ > > > implementation. > > > > > > In particular, this bit of documentation in web_contents_observer.cc > > > scares me a bit (DidStartNavigation): > > > > > > // Called when a navigation started in the WebContents. > |navigation_handle| > > > // is unique to a specific navigation. The same |navigation_handle| will > be > > > // provided on subsequent calls to DidRedirectNavigation, > > DidFinishNavigation, > > > // and ReadyToCommitNavigation when related to this navigation. Observers > > > // should clear any references to |navigation_handle| in > > DidFinishNavigation, > > > // just before it is destroyed. > > > > > > If we introduce a java NavigationHandle, it seems like it would be easy > > > for a client to have the same expectations of the native one in terms of > > > lifetime and uniqueness. > > > > > > i.e. based on the above comment, I would expect NavigationHandle.equals or > > > even == to work across DidStartNavigation and DidFinishNavigation. > Regardless > > > of us using it right now, it seems very prone to misuse if the two apis are > > > so different. > > > > > > I suspect this is also where you are having trouble with the CHECKS in > > > navigation_handle_impl. We are trying to serialize the entire state > > > of the navigation handle to a java object, but that violates the contract. > > > > > > I see two possible options to investigate further: > > > > > > 1.) Do not introduce a NavigationHandle java object yet. Instead, just > > > expose the state that is needed in the various methods. This avoids > > > the lifetime issue. > > > 2.) Create a java object that is owned by the native NavigationHandle > > > C++ object and have it simply forward calls from java to native. > > > > > > I don't have a super strong preference either way, but it might be good > > > to see how many methods we'd need to expose from C++ to java to hit #1. > > > If the number of params that we currently need to pass from C++ to java > > > is small in each of the methods, then I think #1 is slightly preferable > > > for now. > > > > > > If we have to pass the world, then #2 would be easier. > > > > > > If we do end up needing java callers to track > > > uniqueness of navigation handles, then I think we can go the #2 route, but > > > I don't know if we need to build out the machinery for something that might > > > never be used. > > > > I am a bit worried about #1 slowly increasing the number of parameters over > time > > (even if today we only use a handful) and becoming a big mess in the future, > so > > my preference would be for #2. > > I am also a bit concerned that if we go #2 route, we need to make a JNI call for > each param for each of the registered WebContentObservers (and TabObservers). Is > that okay? Discussed with shaktisahu@ and mariakhomenko@ and agreed that we could do the #2 route, but as shaktisahu@ points out we'd need to figure out a way to not undo the purpose of the web contents observer proxy (which is to prevent the number of jni calls per update). We could likely do this by having the java object cache values lazily and then introduce a signal to drop the cache on each navigation event. We think that is more overhead than we need in the scope of this change, so while we may need it at some point, #1 seems best for now.
The CQ bit was checked by shaktisahu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
mariakhomenko@, tedchoc@ - PTAL
Overall this seems very reasonable to me. I have some naming, api suggestions, but I think it should be good after those. https://codereview.chromium.org/2598163002/diff/80001/content/browser/android... File content/browser/android/web_contents_observer_proxy.cc (right): https://codereview.chromium.org/2598163002/diff/80001/content/browser/android... content/browser/android/web_contents_observer_proxy.cc:197: jboolean is_navigation_to_different_page = I would get rid of both of these computed values and pass the necessarily bits to java to allow it to compute it. In particular, I think that means dropping is_navigation_to_different_page and passing the raw PageTransition to java. We have generated constants in org.chromium.ui.base.PageTransition, so this can be done in java by doing: (pageTransition & PageTransition.CORE_MASK) == PageTransition.RELOAD; Not as clean as the C++ side, but makes the APIs much closer. https://codereview.chromium.org/2598163002/diff/80001/content/browser/frame_h... File content/browser/frame_host/navigation_handle_impl.cc (left): https://codereview.chromium.org/2598163002/diff/80001/content/browser/frame_h... content/browser/frame_host/navigation_handle_impl.cc:223: // enabled. https://crbug.com/621856 ah, the bug got fixed so you're just resolving the TODO it seems. That threw me off initially. https://codereview.chromium.org/2598163002/diff/80001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsObserverProxy.java (right): https://codereview.chromium.org/2598163002/diff/80001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsObserverProxy.java:88: public void didFinishNavigation(String validatedUrl, boolean isMainFrame, boolean isErrorPage, let's keep the java names the same as the navigation handle ones as well. validatedUrl = url isMainFrame = isInMainFrame isFragmentNavigation = isSamePage isReload ~= pageTransition Anyone that is currently using isNavigationToDifferentPage will need to do if (isMainFrame && !isSamePage) {...} The goal is to have anyone familiar with the C++ navigation handle to be able to look at the java api and immediately know how to use stuff.
The CQ bit was checked by shaktisahu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by shaktisahu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) |
