Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
414423002:
Removing ContentViewCore dependencies from few functions which acts as direct wrapper to WebContents (Closed)
|
Created:
6 years, 5 months ago by AKVT Modified:
6 years, 4 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, shatch Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionIn the current Android code structure, ContentViewCore is just acting as a wrapper to many WebContents functions.
In this patch we are trying to position the WebContents functionalities to web_contents_android file from
ContentViewCoreImpl to make it functionally readable. Ensuring there is a one-to-one mapping between
WebContents Java and native counterparts.
BUG=398263
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=289261
Patch Set 1 #
Total comments: 3
Patch Set 2 : Fixed review comments and ignored RWHVA functions from this patch #
Total comments: 4
Patch Set 3 : Removed RVHI functions from this patch. #Patch Set 4 : Fixed few nits and new line issues. #
Total comments: 2
Patch Set 5 : Restructured WillHandleDeferAfterResponseStarted and DidDeferAfterResponseStarted APIs. #
Total comments: 13
Patch Set 6 : Removed NavigationTransitionDelegate interface from ContentViewCore. #
Total comments: 4
Patch Set 7 : Added helper function for redundant codes. #Patch Set 8 : Formatted native file changes to correct inclusion order. #
Total comments: 6
Patch Set 9 : Fixed review comments and added forward declaration of Class. #Patch Set 10 : Fixed include order under flag and removed unwanted headers. #
Total comments: 3
Patch Set 11 : Removed unnecessary virtual function. #Patch Set 12 : Removed unwanted forward class declarations. #Patch Set 13 : Rebased the patch. #
Total comments: 2
Patch Set 14 : Removed setHasPendingNavigationTransitionForTesting from CVC. #Messages
Total messages: 41 (0 generated)
@Yaron & Ted C PTAL
https://codereview.chromium.org/414423002/diff/1/content/browser/android/cont... File content/browser/android/content_view_core_impl.cc (left): https://codereview.chromium.org/414423002/diff/1/content/browser/android/cont... content/browser/android/content_view_core_impl.cc:1166: RenderWidgetHostViewAndroid* rwhv = GetRenderWidgetHostViewAndroid(); Ted, feel free to disagree but I'm thinking to not muck with RenderWidgetHostViewAndroid just yet. I think we can tackle the easy/obvious wrappers of WebContents and then I'd like to see a longer-term plan for how to handle RWHVA instead of moving some parts and not others (e.g. why did you move this but ::MoveCaret is untouched?).
https://codereview.chromium.org/414423002/diff/1/content/browser/android/cont... File content/browser/android/content_view_core_impl.cc (left): https://codereview.chromium.org/414423002/diff/1/content/browser/android/cont... content/browser/android/content_view_core_impl.cc:1166: RenderWidgetHostViewAndroid* rwhv = GetRenderWidgetHostViewAndroid(); On 2014/07/29 00:05:58, Yaron wrote: > Ted, feel free to disagree but I'm thinking to not muck with > RenderWidgetHostViewAndroid just yet. I think we can tackle the easy/obvious > wrappers of WebContents and then I'd like to see a longer-term plan for how to > handle RWHVA instead of moving some parts and not others (e.g. why did you move > this but ::MoveCaret is untouched?). @Yaron MoveCaret() requires dpi_scale. I was not sure where to keep ownership of dpi_scale rather than replicating on both classes. So I kept it untouched. If we get a clarity on that I can proceed further on other related functions as well.
https://codereview.chromium.org/414423002/diff/1/content/browser/android/cont... File content/browser/android/content_view_core_impl.cc (left): https://codereview.chromium.org/414423002/diff/1/content/browser/android/cont... content/browser/android/content_view_core_impl.cc:1166: RenderWidgetHostViewAndroid* rwhv = GetRenderWidgetHostViewAndroid(); On 2014/07/29 00:05:58, Yaron wrote: > Ted, feel free to disagree but I'm thinking to not muck with > RenderWidgetHostViewAndroid just yet. I think we can tackle the easy/obvious > wrappers of WebContents and then I'd like to see a longer-term plan for how to > handle RWHVA instead of moving some parts and not others (e.g. why did you move > this but ::MoveCaret is untouched?). I agree. For now, only things that directly deal with web contents should be moved to WebContentsAndroid. I think we may want to create a android counterpart to RenderWidgetHostViewAndroid, but at the same time I don't want to see WebContentsAndroid just morph into ContentViewCore as a dumping ground for all native methods.
@Yaron and Ted, I removed RWHVA functionalities here. Will take care as separate patch. PTAL
please update the cl description to further explain what you're doing. Also add a BUG= for the bug I filed for you. https://codereview.chromium.org/414423002/diff/20001/content/browser/android/... File content/browser/android/content_view_core_impl.cc (left): https://codereview.chromium.org/414423002/diff/20001/content/browser/android/... content/browser/android/content_view_core_impl.cc:822: void ContentViewCoreImpl::SelectPopupMenuItems(JNIEnv* env, jobject obj, Sorry but it's unclear why this is moving.
https://codereview.chromium.org/414423002/diff/20001/content/browser/android/... File content/browser/android/content_view_core_impl.cc (left): https://codereview.chromium.org/414423002/diff/20001/content/browser/android/... content/browser/android/content_view_core_impl.cc:822: void ContentViewCoreImpl::SelectPopupMenuItems(JNIEnv* env, jobject obj, On 2014/08/01 16:21:12, Yaron wrote: > Sorry but it's unclear why this is moving. I thought to untouch only RWHVA functions. This function where shall we migrate ? Also one more doubt I have is who will own dpi_scale and other related members ? Please share your view.
https://codereview.chromium.org/414423002/diff/20001/content/browser/android/... File content/browser/android/content_view_core_impl.cc (left): https://codereview.chromium.org/414423002/diff/20001/content/browser/android/... content/browser/android/content_view_core_impl.cc:822: void ContentViewCoreImpl::SelectPopupMenuItems(JNIEnv* env, jobject obj, On 2014/08/01 17:09:58, AKV wrote: > On 2014/08/01 16:21:12, Yaron wrote: > > Sorry but it's unclear why this is moving. > I thought to untouch only RWHVA functions. This function where shall we migrate > ? Also one more doubt I have is who will own dpi_scale and other related members > ? Please share your view. > The goal of the java WebContents class is to act as a simple bridge to the native implementation of those methods. Ideally, the methods on the java side would have a one to one mapping with those in web_contents.cc. As this is simply using the web_contents as a means for getting the RenderViewHost, it does not belong in the WebContents class. In cases like this, it might be more appropriate to extract the popup methods out to a completely separate class that handles all of the interactions and makes them a single isolated unit that doesn't get muddled with the rest of the calls. Granted, that may not be possible due to the message routing etc, but it would certainly be nice to start building smaller components instead of monolithic classes.
@Ted and Yaron PTAL https://codereview.chromium.org/414423002/diff/20001/content/browser/android/... File content/browser/android/content_view_core_impl.cc (left): https://codereview.chromium.org/414423002/diff/20001/content/browser/android/... content/browser/android/content_view_core_impl.cc:822: void ContentViewCoreImpl::SelectPopupMenuItems(JNIEnv* env, jobject obj, On 2014/08/01 17:26:20, Ted C wrote: > On 2014/08/01 17:09:58, AKV wrote: > > On 2014/08/01 16:21:12, Yaron wrote: > > > Sorry but it's unclear why this is moving. > > I thought to untouch only RWHVA functions. This function where shall we > migrate > > ? Also one more doubt I have is who will own dpi_scale and other related > members > > ? Please share your view. > > > > The goal of the java WebContents class is to act as a simple bridge to the > native implementation of those methods. Ideally, the methods on the java side > would have a one to one mapping with those in web_contents.cc. As this is > simply using the web_contents as a means for getting the RenderViewHost, it does > not belong in the WebContents class. > > In cases like this, it might be more appropriate to extract the popup methods > out to a completely separate class that handles all of the interactions and > makes them a single isolated unit that doesn't get muddled with the rest of the > calls. Granted, that may not be possible due to the message routing etc, but it > would certainly be nice to start building smaller components instead of > monolithic classes. Thanks Ted. I have removed this function from the patch to take care of only direct dependent WebContents functions. As discussed will take care of RWHVA functions to new class and remaining independent functions I will club into one Class for better restructuring.
+shatch cc who wrote the original cl https://codereview.chromium.org/414423002/diff/60001/content/browser/android/... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/414423002/diff/60001/content/browser/android/... content/browser/android/content_view_core_impl.cc:1556: bool ContentViewCoreImpl::WillHandleDeferAfterResponseStarted() { I think we should probably move these two functions along with it. Effectively moving the whole feature to WebContents (i.e. these are teh calls that come back from teh renderer). See https://codereview.chromium.org/297973002 which was the original patch that added these. It would also mean that we can remove the CVC dependency from web_contents_impl.cc.
PTAL https://codereview.chromium.org/414423002/diff/60001/content/browser/android/... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/414423002/diff/60001/content/browser/android/... content/browser/android/content_view_core_impl.cc:1556: bool ContentViewCoreImpl::WillHandleDeferAfterResponseStarted() { On 2014/08/04 17:08:30, Yaron wrote: > I think we should probably move these two functions along with it. Effectively > moving the whole feature to WebContents (i.e. these are teh calls that come > back from teh renderer). See https://codereview.chromium.org/297973002 which was > the original patch that added these. It would also mean that we can remove the > CVC dependency from web_contents_impl.cc. Thank you for reference patch link. I have modified the code based on this patchset. https://codereview.chromium.org/414423002/diff/80001/content/browser/web_cont... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/414423002/diff/80001/content/browser/web_cont... content/browser/web_contents/web_contents_impl.cc:3643: if (!web_contents_android) { This part please correct if I added extra code.
+oysteine cc I think you also need to make some changes to TransitionTest.java to reflect all your changes to CVC. FYI: I'm on leave at the moment. Will try to keep up with this CL, but will be pretty unreliable. Oystein is handling navigation transition related stuff, please give him a chance to patch this in and make sure everything works as intended before landing this. https://codereview.chromium.org/414423002/diff/80001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (left): https://codereview.chromium.org/414423002/diff/80001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:366: private NavigationTransitionDelegate mNavigationTransitionDelegate = null; Move this as well? https://codereview.chromium.org/414423002/diff/80001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:3069: public void didStartNavigationTransitionForFrame(long frameId) { Seems like this would need to move as well? https://codereview.chromium.org/414423002/diff/80001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:3088: mNavigationTransitionDelegate = delegate; And this? https://codereview.chromium.org/414423002/diff/80001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/414423002/diff/80001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:234: public interface NavigationTransitionDelegate { If you're going to move didDeferAfterResponseStarted, etc... to WebContents.java, shouldn't you also move this delegate there? (and associated member variable, set function, etc...)
@Yaron and shatch, Please share your opinion after seeing my inline comments. https://codereview.chromium.org/414423002/diff/80001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (left): https://codereview.chromium.org/414423002/diff/80001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:366: private NavigationTransitionDelegate mNavigationTransitionDelegate = null; On 2014/08/05 14:54:24, shatch (OOO til Nov) wrote: > Move this as well? Currently I have duplicated by fearing build failure. If I remove this if no build breaks, then I am happy to remove. Please share your thoughts. https://codereview.chromium.org/414423002/diff/80001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:3069: public void didStartNavigationTransitionForFrame(long frameId) { On 2014/08/05 14:54:24, shatch (OOO til Nov) wrote: > Seems like this would need to move as well? Earlier Yaron suggested to move 2 functions which are dependent on the previous change, so I have taken care only those 2 methods only. I can move this one as well, as all are relative functions. https://codereview.chromium.org/414423002/diff/80001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:3088: mNavigationTransitionDelegate = delegate; On 2014/08/05 14:54:24, shatch (OOO til Nov) wrote: > And this? This too I duplicated as I mentioned on top. Please share your thoughts on this about build issues. https://codereview.chromium.org/414423002/diff/80001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/414423002/diff/80001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:234: public interface NavigationTransitionDelegate { On 2014/08/05 14:54:24, shatch (OOO til Nov) wrote: > If you're going to move didDeferAfterResponseStarted, etc... to > WebContents.java, shouldn't you also move this delegate there? (and associated > member variable, set function, etc...) I already moved the interface in WebContents, but since ContentViewCore is the main interaction point at the moment Chrome App layer may be still relying on ContentViewcore NavigationTransitionDelegate, so to avoid Compilation issues for Chrome App, I have kept it here also. If build bot will pass by moving this completely, then I can remove this. Please share your thoughts on this
On 2014/08/05 14:54:24, shatch (OOO til Nov) wrote: > +oysteine cc > > I think you also need to make some changes to TransitionTest.java to reflect all > your changes to CVC. > > FYI: I'm on leave at the moment. Will try to keep up with this CL, but will be > pretty unreliable. Oystein is handling navigation transition related stuff, > please give him a chance to patch this in and make sure everything works as > intended before landing this. > > https://codereview.chromium.org/414423002/diff/80001/content/public/android/j... > File > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java > (left): > > https://codereview.chromium.org/414423002/diff/80001/content/public/android/j... > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:366: > private NavigationTransitionDelegate mNavigationTransitionDelegate = null; > Move this as well? > > https://codereview.chromium.org/414423002/diff/80001/content/public/android/j... > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:3069: > public void didStartNavigationTransitionForFrame(long frameId) { > Seems like this would need to move as well? > > https://codereview.chromium.org/414423002/diff/80001/content/public/android/j... > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:3088: > mNavigationTransitionDelegate = delegate; > And this? > > https://codereview.chromium.org/414423002/diff/80001/content/public/android/j... > File > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java > (right): > > https://codereview.chromium.org/414423002/diff/80001/content/public/android/j... > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:234: > public interface NavigationTransitionDelegate { > If you're going to move didDeferAfterResponseStarted, etc... to > WebContents.java, shouldn't you also move this delegate there? (and associated > member variable, set function, etc...) @shatch Thanks for pointing it. I will change test too, If app is not using this interface I can remove this interface without any issues. @oysteine Please share your thoughts after review and comments,
https://codereview.chromium.org/414423002/diff/80001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (left): https://codereview.chromium.org/414423002/diff/80001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:366: private NavigationTransitionDelegate mNavigationTransitionDelegate = null; On 2014/08/05 16:07:23, AKV wrote: > On 2014/08/05 14:54:24, shatch (OOO til Nov) wrote: > > Move this as well? > Currently I have duplicated by fearing build failure. If I remove this if no > build breaks, then I am happy to remove. Please share your thoughts. > Which build breaks are you referring to? If everything in chromium builds, we should be fine. YOu can just omit the parameter to ninja to build everything: ninja -C out/Debug If it's in the internal repo, don't worry, I'll fix it to work with changes in this patch when I approve it https://codereview.chromium.org/414423002/diff/80001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:3069: public void didStartNavigationTransitionForFrame(long frameId) { On 2014/08/05 16:07:24, AKV wrote: > On 2014/08/05 14:54:24, shatch (OOO til Nov) wrote: > > Seems like this would need to move as well? > Earlier Yaron suggested to move 2 functions which are dependent on the previous > change, so I have taken care only those 2 methods only. I can move this one as > well, as all are relative functions. > Well logically it doesn't make sense to split these functions up as they control the same functionality. I agree with Simon. https://codereview.chromium.org/414423002/diff/80001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:3088: mNavigationTransitionDelegate = delegate; On 2014/08/05 16:07:23, AKV wrote: > On 2014/08/05 14:54:24, shatch (OOO til Nov) wrote: > > And this? > This too I duplicated as I mentioned on top. Please share your thoughts on this > about build issues. > I don't see the benefit of duplicating this. Please take a step back and consider that we are moving this functionality out of ContentViewCore into WebContents. https://codereview.chromium.org/414423002/diff/80001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/414423002/diff/80001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:234: public interface NavigationTransitionDelegate { On 2014/08/05 16:07:23, AKV wrote: > On 2014/08/05 14:54:24, shatch (OOO til Nov) wrote: > > If you're going to move didDeferAfterResponseStarted, etc... to > > WebContents.java, shouldn't you also move this delegate there? (and associated > > member variable, set function, etc...) > > I already moved the interface in WebContents, but since ContentViewCore is the > main interaction point at the moment Chrome App layer may be still relying on > ContentViewcore NavigationTransitionDelegate, so to avoid Compilation issues for > Chrome App, I have kept it here also. If build bot will pass by moving this > completely, then I can remove this. Please share your thoughts on this I don't see any references to it in chrome/ directory
@Yaron PTAL, I have moved NavigationTransitionDelegate interface to new file inside content_public due to build dependency from WebContents.java.
getting close. You'll need a separate OWNER for web_contents_impl.cc https://codereview.chromium.org/414423002/diff/100001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/414423002/diff/100001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:2439: WebContentsAndroid* web_contents_android = Please extract a helper function and place alongside GetJavaWebContents https://codereview.chromium.org/414423002/diff/100001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:3641: ; remove
Note that some of the relocated functions will change once this shortly lands: https://codereview.chromium.org/435833002 I'll patch this in locally tomorrow afternoon PST to doublecheck that the transition navigations still work; please hold off on any commits until then :).
@Yaron as suggested I fixed those issues. PTAL @sky PTAL web_contents_impl changes. @Oystein - Thanks for the information. I made the final patch ready. Pls use this for patching locally. I will rebase it once again after https://codereview.chromium.org/435833002 lands. https://codereview.chromium.org/414423002/diff/100001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/414423002/diff/100001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:2439: WebContentsAndroid* web_contents_android = On 2014/08/07 01:20:44, Yaron wrote: > Please extract a helper function and place alongside GetJavaWebContents Done. https://codereview.chromium.org/414423002/diff/100001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:3641: ; On 2014/08/07 01:20:44, Yaron wrote: > remove Done.
PTAL. Corrected some formatting issues.
https://codereview.chromium.org/414423002/diff/140001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/414423002/diff/140001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:2439: web_contents_android->DidStartNavigationTransitionForFrame(render_frame_id); I wouldn't bother with the local variable for WebContentsAndroid in these functions. https://codereview.chromium.org/414423002/diff/140001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4106: WebContentsAndroid* WebContentsImpl::GetWebContentsAndroid() { Make position match header. https://codereview.chromium.org/414423002/diff/140001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.h (right): https://codereview.chromium.org/414423002/diff/140001/content/browser/web_con... content/browser/web_contents/web_contents_impl.h:27: #include "content/browser/web_contents/web_contents_android.h" Forward declare WebContentsAndroid. In the future conditional ifdefs are at the end of a section. Do the same for forward declarations.
@sky I have fixed review comments. @avi PTAL web_contents.h @Oystein I have rebased the patch after dependent patch landed. PTAL and use this patch for patching locally. https://codereview.chromium.org/414423002/diff/140001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/414423002/diff/140001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:2439: web_contents_android->DidStartNavigationTransitionForFrame(render_frame_id); On 2014/08/07 16:17:31, sky wrote: > I wouldn't bother with the local variable for WebContentsAndroid in these > functions. Removed Local variable. Thanks https://codereview.chromium.org/414423002/diff/140001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4106: WebContentsAndroid* WebContentsImpl::GetWebContentsAndroid() { On 2014/08/07 16:17:31, sky wrote: > Make position match header. Positioned the declaration immediately after GetJavaWebContents() declaration. https://codereview.chromium.org/414423002/diff/140001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.h (right): https://codereview.chromium.org/414423002/diff/140001/content/browser/web_con... content/browser/web_contents/web_contents_impl.h:27: #include "content/browser/web_contents/web_contents_android.h" On 2014/08/07 16:17:31, sky wrote: > Forward declare WebContentsAndroid. > In the future conditional ifdefs are at the end of a section. Do the same for > forward declarations. Thanks Done.
https://codereview.chromium.org/414423002/diff/180001/content/public/browser/... File content/public/browser/web_contents.h (right): https://codereview.chromium.org/414423002/diff/180001/content/public/browser/... content/public/browser/web_contents.h:583: virtual WebContentsAndroid* GetWebContentsAndroid() = 0; I don't get the relationship here. From line 582 I see that there's something called a "JavaWebContents" that's a jobject. From line 581 I see the opposite of that function, but that calls the jobject a "web_contents_android". And from line 583 I see that a "WebContentsAndroid" is an entirely different thing, a C++ class. Can you please clarify your naming? Can we have only one special Android class in correspondence here?
https://codereview.chromium.org/414423002/diff/180001/content/public/browser/... File content/public/browser/web_contents.h (right): https://codereview.chromium.org/414423002/diff/180001/content/public/browser/... content/public/browser/web_contents.h:583: virtual WebContentsAndroid* GetWebContentsAndroid() = 0; On 2014/08/08 15:23:25, Avi wrote: > I don't get the relationship here. > > From line 582 I see that there's something called a "JavaWebContents" that's a > jobject. From line 581 I see the opposite of that function, but that calls the > jobject a "web_contents_android". And from line 583 I see that a > "WebContentsAndroid" is an entirely different thing, a C++ class. > > Can you please clarify your naming? Can we have only one special Android class > in correspondence here? Thanks, I think GetNativeWebContents() suits correctly. Please share your opinion ?
https://codereview.chromium.org/414423002/diff/180001/content/public/browser/... File content/public/browser/web_contents.h (right): https://codereview.chromium.org/414423002/diff/180001/content/public/browser/... content/public/browser/web_contents.h:583: virtual WebContentsAndroid* GetWebContentsAndroid() = 0; On 2014/08/08 16:15:51, AKV wrote: > On 2014/08/08 15:23:25, Avi wrote: > > I don't get the relationship here. > > > > From line 582 I see that there's something called a "JavaWebContents" that's a > > jobject. From line 581 I see the opposite of that function, but that calls the > > jobject a "web_contents_android". And from line 583 I see that a > > "WebContentsAndroid" is an entirely different thing, a C++ class. > > > > Can you please clarify your naming? Can we have only one special Android class > > in correspondence here? > > Thanks, I think GetNativeWebContents() suits correctly. Please share your > opinion ? The name GetWebContentsAndroid is fine if you rename the parameter on line 580 (to java_web_contents or something like that). That doesn't solve the real problem. Before, it was clear that there was an Android corresponding object. There were two functions, one to go WebContents → Android object (GetJavaWebContents), and one to go Android object → WebContents (FromJavaWebContents). OK, I can deal with that. Now there are three functions? You're proposing that there are two different objects that WebContents is in 1:1 relationship with. It's not immediately obvious via naming what the difference is, and it's weird to me that one of them has two-way conversion while the other has only one-way conversion (WebContents → WebContentsAndroid).
On 2014/08/08 16:26:04, Avi wrote: > https://codereview.chromium.org/414423002/diff/180001/content/public/browser/... > File content/public/browser/web_contents.h (right): > > https://codereview.chromium.org/414423002/diff/180001/content/public/browser/... > content/public/browser/web_contents.h:583: virtual WebContentsAndroid* > GetWebContentsAndroid() = 0; > On 2014/08/08 16:15:51, AKV wrote: > > On 2014/08/08 15:23:25, Avi wrote: > > > I don't get the relationship here. > > > > > > From line 582 I see that there's something called a "JavaWebContents" that's > a > > > jobject. From line 581 I see the opposite of that function, but that calls > the > > > jobject a "web_contents_android". And from line 583 I see that a > > > "WebContentsAndroid" is an entirely different thing, a C++ class. > > > > > > Can you please clarify your naming? Can we have only one special Android > class > > > in correspondence here? > > > > Thanks, I think GetNativeWebContents() suits correctly. Please share your > > opinion ? > > The name GetWebContentsAndroid is fine if you rename the parameter on line 580 > (to java_web_contents or something like that). That doesn't solve the real > problem. > > Before, it was clear that there was an Android corresponding object. There were > two functions, one to go WebContents → Android object (GetJavaWebContents), and > one to go Android object → WebContents (FromJavaWebContents). > > OK, I can deal with that. > > Now there are three functions? > > You're proposing that there are two different objects that WebContents is in 1:1 > relationship with. It's not immediately obvious via naming what the difference > is, and it's weird to me that one of them has two-way conversion while the other > has only one-way conversion (WebContents → WebContentsAndroid). Right. Actually if I am keeping a normal function inside WebContentsImpl class, it will resolve the ambiguity. I will do that. Thanks.
@sky PTAL, I have fixed review comments.
On 2014/08/08 15:08:25, AKV wrote: > @Oystein I have rebased the patch after dependent patch landed. PTAL and use > this patch for patching locally. > Tested briefly, and this doesn't seem like it'll create any problems for the navigation transitions.
On 2014/08/08 17:38:21, Oystein wrote: > On 2014/08/08 15:08:25, AKV wrote: > > @Oystein I have rebased the patch after dependent patch landed. PTAL and use > > this patch for patching locally. > > > > Tested briefly, and this doesn't seem like it'll create any problems for the > navigation transitions. Thanks Oystein. I am proceeding with review.
On 2014/08/08 16:26:04, Avi wrote: > https://codereview.chromium.org/414423002/diff/180001/content/public/browser/... > File content/public/browser/web_contents.h (right): > > https://codereview.chromium.org/414423002/diff/180001/content/public/browser/... > content/public/browser/web_contents.h:583: virtual WebContentsAndroid* > GetWebContentsAndroid() = 0; > On 2014/08/08 16:15:51, AKV wrote: > > On 2014/08/08 15:23:25, Avi wrote: > > > I don't get the relationship here. > > > > > > From line 582 I see that there's something called a "JavaWebContents" that's > a > > > jobject. From line 581 I see the opposite of that function, but that calls > the > > > jobject a "web_contents_android". And from line 583 I see that a > > > "WebContentsAndroid" is an entirely different thing, a C++ class. > > > > > > Can you please clarify your naming? Can we have only one special Android > class > > > in correspondence here? > > > > Thanks, I think GetNativeWebContents() suits correctly. Please share your > > opinion ? > > The name GetWebContentsAndroid is fine if you rename the parameter on line 580 > (to java_web_contents or something like that). That doesn't solve the real > problem. > > Before, it was clear that there was an Android corresponding object. There were > two functions, one to go WebContents → Android object (GetJavaWebContents), and > one to go Android object → WebContents (FromJavaWebContents). > > OK, I can deal with that. > > Now there are three functions? I think the confusion is that previously we only would try and retrieve the Java wrapper for a given WebContents so that the WebContents can be passed safely through Java code. With this change, WebContentsImpl needs the c++ version of the Java wrapper so that it can call a function on it which gets passed up to java. If you want so simplify, I suppose we could probably keep the latter and add an accessor to it which returns the paired java instance > > You're proposing that there are two different objects that WebContents is in 1:1 > relationship with. It's not immediately obvious via naming what the difference > is, and it's weird to me that one of them has two-way conversion while the other > has only one-way conversion (WebContents → WebContentsAndroid).
On 2014/08/08 20:29:39, Yaron wrote: > If you want so simplify, I suppose we could probably keep the latter and > add an accessor to it which returns the paired java instance That would clean things up here; if you would do that that would be great.
On 2014/08/08 20:29:39, Yaron wrote: > On 2014/08/08 16:26:04, Avi wrote: > > > https://codereview.chromium.org/414423002/diff/180001/content/public/browser/... > > File content/public/browser/web_contents.h (right): > > > > > https://codereview.chromium.org/414423002/diff/180001/content/public/browser/... > > content/public/browser/web_contents.h:583: virtual WebContentsAndroid* > > GetWebContentsAndroid() = 0; > > On 2014/08/08 16:15:51, AKV wrote: > > > On 2014/08/08 15:23:25, Avi wrote: > > > > I don't get the relationship here. > > > > > > > > From line 582 I see that there's something called a "JavaWebContents" > that's > > a > > > > jobject. From line 581 I see the opposite of that function, but that calls > > the > > > > jobject a "web_contents_android". And from line 583 I see that a > > > > "WebContentsAndroid" is an entirely different thing, a C++ class. > > > > > > > > Can you please clarify your naming? Can we have only one special Android > > class > > > > in correspondence here? > > > > > > Thanks, I think GetNativeWebContents() suits correctly. Please share your > > > opinion ? > > > > The name GetWebContentsAndroid is fine if you rename the parameter on line 580 > > (to java_web_contents or something like that). That doesn't solve the real > > problem. > > > > Before, it was clear that there was an Android corresponding object. There > were > > two functions, one to go WebContents → Android object (GetJavaWebContents), > and > > one to go Android object → WebContents (FromJavaWebContents). > > > > OK, I can deal with that. > > > > Now there are three functions? > > I think the confusion is that previously we only would try and retrieve the Java > wrapper for a given WebContents so that the WebContents can be passed safely > through Java code. With this change, WebContentsImpl needs the c++ version of > the Java wrapper so that it can call a function on it which gets passed up to > java. If you want so simplify, I suppose we could probably keep the latter and > add an accessor to it which returns the paired java instance > > > > You're proposing that there are two different objects that WebContents is in > 1:1 > > relationship with. It's not immediately obvious via naming what the difference > > is, and it's weird to me that one of them has two-way conversion while the > other > > has only one-way conversion (WebContents → WebContentsAndroid). @Yaron & Avi - Sorry, I didn't understand the point "paired java instance". Could you elaborate little more with some snippet of code which we are discussing. By the way in patch 12 I have removed virtual and dealing only in web_contents_impl calls. Is it going to be Ok ?
I'm happier with this patch set (12). content/browser/web_contents LGTM. IMO you still should clean up your naming conventions. In web_contents.h (no longer a part of this CL, I know), for FromJavaWebContents I'd prefer the parameter to be java_web_contents. Then the naming convention would be "Java Web Contents" is the Java object and "Web Contents Android" is the C++ object. But I'm not gating my approval on that.
On 2014/08/11 16:51:22, Avi wrote: > I'm happier with this patch set (12). content/browser/web_contents LGTM. > > IMO you still should clean up your naming conventions. In web_contents.h (no > longer a part of this CL, I know), for FromJavaWebContents I'd prefer the > parameter to be java_web_contents. Then the naming convention would be "Java Web > Contents" is the Java object and "Web Contents Android" is the C++ object. > > But I'm not gating my approval on that. Thanks Avi. I have just started isolating related functionality. Will take care of this while I eliminate FromJavaWebContents, which may replace with WebContentsAndroid. Yaron can comment more on this.
Rebased patchset PTAL.
@Yaron & Ted PTAL
lgtm https://codereview.chromium.org/414423002/diff/240001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/414423002/diff/240001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:3000: void setHasPendingNavigationTransitionForTesting() { Nit: please remove this accessor and update the test to use WebContents directly. It's part of the same feature for which you moved all the underlying code to WebContents.
The CQ bit was checked by ajith.v@samsung.com
PTAL https://codereview.chromium.org/414423002/diff/240001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/414423002/diff/240001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:3000: void setHasPendingNavigationTransitionForTesting() { On 2014/08/12 17:37:04, Yaron wrote: > Nit: please remove this accessor and update the test to use WebContents > directly. It's part of the same feature for which you moved all the underlying > code to WebContents. Done. Thanks.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ajith.v@samsung.com/414423002/260001
Message was sent while issue was closed.
Change committed as 289261 |
