Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2528823002:
Separate SwipeRefreshHandler and ContentViewCore (Closed)
DescriptionSeparate SwipeRefreshHandler and ContentViewCore
Currently Android's native ContentViewCore class implements
ui::OverscrollRefreshHandler and handles swipe to refresh events. This
change refactors this functionality out of ContentViewCore into a new
native SwipeRefreshHandler class as part of a broader effort to
eliminate ContentViewCore.
BUG=627943
Committed: https://crrev.com/7f2ec7f88816aa89f872f068881d117047049cfe
Cr-Commit-Position: refs/heads/master@{#438397}
Patch Set 1 #Patch Set 2 : Fix compile error in test case #
Total comments: 28
Patch Set 3 : Move web_contents_view_android.h header in web_contents_android.h into cc file, modify CreateRefres… #
Total comments: 7
Patch Set 4 : Eliminate native SwipeRefreshHandler. Now we just pass around a Java reference in native code. Atta… #
Total comments: 4
Patch Set 5 : Remove test case that depends on native SwipeRefreshHandler #
Total comments: 1
Patch Set 6 : Remove unused SwipeRefreshHandler.mNativeSwipeRefreshHandler field #
Total comments: 11
Patch Set 7 : Refactor as discussed #Patch Set 8 : Fix non-Android compile issue #
Total comments: 31
Patch Set 9 : Make boliu's requested changes #
Total comments: 16
Patch Set 10 : boliu's requested changes + some lint fixes #
Total comments: 13
Patch Set 11 : Respond to boliu's comments, refactor CreateOverscrollController() function #
Total comments: 15
Patch Set 12 : Respond to more comments, all test cases should pass now #
Total comments: 5
Patch Set 13 : Remove unnecessary null check, add CreateOverscrollControllerIfPossible() in SetCVC() #Patch Set 14 : Re-add WindowAndroid null check #Patch Set 15 : Rebase (hopefully correctly) #
Total comments: 12
Patch Set 16 : tedchoc's requested changes #Messages
Total messages: 130 (64 generated)
rlanday@chromium.org changed reviewers: + aelias@chromium.org, boliu@chromium.org, jinsukkim@chromium.org, tedchoc@chromium.org
The CQ bit was checked by rlanday@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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by rlanday@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_TIMED_OUT, no build URL) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
Nice! Left some initial thought - some will be not valid any more once you fix build/test errors. https://codereview.chromium.org/2528823002/diff/20001/chrome/browser/android/... File chrome/browser/android/swipe_refresh_handler.cc (right): https://codereview.chromium.org/2528823002/diff/20001/chrome/browser/android/... chrome/browser/android/swipe_refresh_handler.cc:54: return reinterpret_cast<intptr_t>(swipe_refresh_handler); Alternatively, you can set the created object to WebContentViewAndroid here. You can get the native WebContents by passing the java WC to this method. Then the api SetORH needs to be defined in native rather than in Java. I think the advantage of this approach is that Java ORH interface can be removed entirely. It was sort of redundant since we already have its native counterpart. https://codereview.chromium.org/2528823002/diff/20001/chrome/browser/android/... chrome/browser/android/swipe_refresh_handler.cc:59: delete this; With the refactoring I think this object better be owned by OverscrollRefresh since OR is the only user of the instance, and ideally OverscrollRefreshHandler should be deleted together with OR. Then Java SRH doesn't have to know/keep the pointer to the native object. Let OR use unique_ptr to manage ORH. https://codereview.chromium.org/2528823002/diff/20001/content/browser/android... File content/browser/android/overscroll_controller_android.cc (right): https://codereview.chromium.org/2528823002/diff/20001/content/browser/android... content/browser/android/overscroll_controller_android.cc:90: if (base::CommandLine::ForCurrentProcess()->HasSwitch( Now that OverscrollRefreshHandler is not CVC but a separate object, this flag checking can be moved to native SwipeRefreshHandler Init() to avoid creating the instance in the first place rather than passing it all the way here and doing nothing in case the feature is disabled. https://codereview.chromium.org/2528823002/diff/20001/content/browser/android... content/browser/android/overscroll_controller_android.cc:104: : compositor_(content_view_core->GetWindowAndroid()->GetCompositor()), Now you can pass the compositor object directly - then you can get rid of the dependency on CVC. https://codereview.chromium.org/2528823002/diff/20001/content/browser/android... content/browser/android/overscroll_controller_android.cc:109: refresh_effect_ = CreateRefreshEffect(overscroll_refresh_handler); How about keep this intact but update CreateRefreshEffect to handle null pointer? https://codereview.chromium.org/2528823002/diff/20001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.h (right): https://codereview.chromium.org/2528823002/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.h:86: ui::OverscrollRefreshHandler* overscroll_refresh_handler); Consider adding a setter instead as this is an optional parameter used for a certain embedder, like SynchronousCompositorClient. https://codereview.chromium.org/2528823002/diff/20001/content/browser/web_con... File content/browser/web_contents/web_contents_android.h (right): https://codereview.chromium.org/2528823002/diff/20001/content/browser/web_con... content/browser/web_contents/web_contents_android.h:20: #include "content/browser/web_contents/web_contents_view_android.h" Not necessary https://codereview.chromium.org/2528823002/diff/20001/content/browser/web_con... File content/browser/web_contents/web_contents_view_android.cc (right): https://codereview.chromium.org/2528823002/diff/20001/content/browser/web_con... content/browser/web_contents/web_contents_view_android.cc:85: overscroll_refresh_handler_(nullptr), The signature in the header hasn't changed. Maybe just use a setter?
Publishing now since I got all the trybots passing. Jinsuk has written some comments I will respond to, and there's still a dependency on ContentViewCore I need to address. https://codereview.chromium.org/2528823002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/SwipeRefreshHandler.java (right): https://codereview.chromium.org/2528823002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/SwipeRefreshHandler.java:200: private void attachSwipeRefreshLayoutIfNecessary() { Unfortunately here and in the following method we're still depending on mContentViewCore for attaching/detaching the swipe refresh layout view...I'll look into what we can do about this
https://codereview.chromium.org/2528823002/diff/20001/chrome/browser/android/... File chrome/browser/android/swipe_refresh_handler.cc (right): https://codereview.chromium.org/2528823002/diff/20001/chrome/browser/android/... chrome/browser/android/swipe_refresh_handler.cc:54: return reinterpret_cast<intptr_t>(swipe_refresh_handler); On 2016/11/25 at 01:49:18, Jinsuk Kim wrote: > Alternatively, you can set the created object to WebContentViewAndroid here. You can get the native WebContents by passing the java WC to this method. > > Then the api SetORH needs to be defined in native rather than in Java. I think the advantage of this approach is that Java ORH interface can be removed entirely. It was sort of redundant since we already have its native counterpart. This seems potentially problematic to me; inside Tab, we have access to a WebContents object, not a WebContentsImpl (which I think we need to get the native pointer). I'm not super familiar with this code but my gut feeling is that it seems cleaner to keep most of the logic in Java and perform the appropriate native actions in parallel vs. doing stuff like directly calling methods on the native WebContents from here. If we do want to set the created object to WebContentViewAndroid when the SwipeRefreshHandler is created, would it make sense to expose this functionality on the Java WebContents? https://codereview.chromium.org/2528823002/diff/20001/chrome/browser/android/... chrome/browser/android/swipe_refresh_handler.cc:59: delete this; On 2016/11/25 at 01:49:18, Jinsuk Kim wrote: > With the refactoring I think this object better be owned by OverscrollRefresh since OR is the only user of the instance, and ideally OverscrollRefreshHandler should be deleted together with OR. Then Java SRH doesn't have to know/keep the pointer to the native object. Let OR use unique_ptr to manage ORH. This seems weird to me; I think the native SwipeRefreshHandler should live as long as the Java SwipeRefreshHandler, otherwise we're introducing an additional state to have to reason about (Java SwipeRefreshHandler exists but the native SwipeRefreshHandler has been destructed). This strongly suggests having the Java instance own the C++ instance.
https://codereview.chromium.org/2528823002/diff/20001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.h (right): https://codereview.chromium.org/2528823002/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.h:86: ui::OverscrollRefreshHandler* overscroll_refresh_handler); On 2016/11/25 at 01:49:18, Jinsuk Kim wrote: > Consider adding a setter instead as this is an optional parameter used for a certain embedder, like SynchronousCompositorClient. I'm not sure how much I like that, because if this is passed into the constructor, that's only one place allowable to set this that we have to be concerned with, whereas if we have a setter, we either have to worry about stuff like what if the caller screws up and tries to set the OverscrollRefreshHandler after we've already constructed the OverscrollController, or tries to set the handle again after it's already set. If we end up needing this functionality for some reason we can add a setter, otherwise I think it probably just makes stuff messier. https://codereview.chromium.org/2528823002/diff/20001/content/browser/web_con... File content/browser/web_contents/web_contents_view_android.cc (right): https://codereview.chromium.org/2528823002/diff/20001/content/browser/web_con... content/browser/web_contents/web_contents_view_android.cc:85: overscroll_refresh_handler_(nullptr), On 2016/11/25 at 01:49:18, Jinsuk Kim wrote: > The signature in the header hasn't changed. Maybe just use a setter? Sorry, I'm a little confused here; I did add a setter (SetOverscrollRefreshHandler), I just didn't think it made sense to use it to initialize the overscroll_refresh_handler_ pointer to null...
https://codereview.chromium.org/2528823002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/SwipeRefreshHandler.java (right): https://codereview.chromium.org/2528823002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/SwipeRefreshHandler.java:200: private void attachSwipeRefreshLayoutIfNecessary() { On 2016/11/28 at 19:23:54, rlanday wrote: > Unfortunately here and in the following method we're still depending on mContentViewCore for attaching/detaching the swipe refresh layout view...I'll look into what we can do about this I think the challenge here is that ContentViewCore's container view can change, so I'm not sure how to remove this dependency right now. I think this is ultimately going to have to be done as part of some larger refactoring effort that threads these container view attach/detach events through a class other than ContentViewCore.
Please mark as 'done' at the comment you addressed. That will make it easier to review. https://codereview.chromium.org/2528823002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/SwipeRefreshHandler.java (right): https://codereview.chromium.org/2528823002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/SwipeRefreshHandler.java:200: private void attachSwipeRefreshLayoutIfNecessary() { On 2016/11/28 22:03:47, rlanday wrote: > On 2016/11/28 at 19:23:54, rlanday wrote: > > Unfortunately here and in the following method we're still depending on > mContentViewCore for attaching/detaching the swipe refresh layout view...I'll > look into what we can do about this > > I think the challenge here is that ContentViewCore's container view can change, > so I'm not sure how to remove this dependency right now. I think this is > ultimately going to have to be done as part of some larger refactoring effort > that threads these container view attach/detach events through a class other > than ContentViewCore. Chrome's container view does not change; only WebView does. So it will be safe to just pass the container view not entire CVC. https://codereview.chromium.org/2528823002/diff/20001/chrome/browser/android/... File chrome/browser/android/swipe_refresh_handler.cc (right): https://codereview.chromium.org/2528823002/diff/20001/chrome/browser/android/... chrome/browser/android/swipe_refresh_handler.cc:54: return reinterpret_cast<intptr_t>(swipe_refresh_handler); On 2016/11/28 19:42:01, rlanday wrote: > On 2016/11/25 at 01:49:18, Jinsuk Kim wrote: > > Alternatively, you can set the created object to WebContentViewAndroid here. > You can get the native WebContents by passing the java WC to this method. > > > > Then the api SetORH needs to be defined in native rather than in Java. I think > the advantage of this approach is that Java ORH interface can be removed > entirely. It was sort of redundant since we already have its native counterpart. > > This seems potentially problematic to me; inside Tab, we have access to a > WebContents object, not a WebContentsImpl (which I think we need to get the > native pointer). I'm not super familiar with this code but my gut feeling is > that it seems cleaner to keep most of the logic in Java and perform the > appropriate native actions in parallel vs. doing stuff like directly calling > methods on the native WebContents from here. > > If we do want to set the created object to WebContentViewAndroid when the > SwipeRefreshHandler is created, would it make sense to expose this functionality > on the Java WebContents? YOu can add the api to native WebContents/WebContentsImpl instead of Java. I kind of prefer doing things in native if possible. The big advantage is, again, that then we can go without Java interface but only use native one. https://codereview.chromium.org/2528823002/diff/20001/chrome/browser/android/... chrome/browser/android/swipe_refresh_handler.cc:59: delete this; On 2016/11/28 19:42:01, rlanday wrote: > On 2016/11/25 at 01:49:18, Jinsuk Kim wrote: > > With the refactoring I think this object better be owned by OverscrollRefresh > since OR is the only user of the instance, and ideally OverscrollRefreshHandler > should be deleted together with OR. Then Java SRH doesn't have to know/keep the > pointer to the native object. Let OR use unique_ptr to manage ORH. > > This seems weird to me; I think the native SwipeRefreshHandler should live as > long as the Java SwipeRefreshHandler, otherwise we're introducing an additional > state to have to reason about (Java SwipeRefreshHandler exists but the native > SwipeRefreshHandler has been destructed). This strongly suggests having the Java > instance own the C++ instance. You don't have to worry about the destruction order if OR owns the object and delete it when it's done with it. Once it is deleted the flow ever reaches here, and Java instance can be gc'ed any time. What if Destroy is called before OR still needs to use the ORH...? one less thing to worry about. The reason it was not owned by OR I think is just because the actual object was CVC whose lifetime is not supposed to be in charge of OR. https://codereview.chromium.org/2528823002/diff/20001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.h (right): https://codereview.chromium.org/2528823002/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.h:86: ui::OverscrollRefreshHandler* overscroll_refresh_handler); On 2016/11/28 21:17:51, rlanday wrote: > On 2016/11/25 at 01:49:18, Jinsuk Kim wrote: > > Consider adding a setter instead as this is an optional parameter used for a > certain embedder, like SynchronousCompositorClient. > > I'm not sure how much I like that, because if this is passed into the > constructor, that's only one place allowable to set this that we have to be > concerned with, whereas if we have a setter, we either have to worry about stuff > like what if the caller screws up and tries to set the OverscrollRefreshHandler > after we've already constructed the OverscrollController, or tries to set the > handle again after it's already set. If we end up needing this functionality for > some reason we can add a setter, otherwise I think it probably just makes stuff > messier. We wouldn't want to keep adding parameters to the constructor but you have a point. https://codereview.chromium.org/2528823002/diff/20001/content/browser/web_con... File content/browser/web_contents/web_contents_view_android.cc (right): https://codereview.chromium.org/2528823002/diff/20001/content/browser/web_con... content/browser/web_contents/web_contents_view_android.cc:85: overscroll_refresh_handler_(nullptr), On 2016/11/28 21:17:51, rlanday wrote: > On 2016/11/25 at 01:49:18, Jinsuk Kim wrote: > > The signature in the header hasn't changed. Maybe just use a setter? > > Sorry, I'm a little confused here; I did add a setter > (SetOverscrollRefreshHandler), I just didn't think it made sense to use it to > initialize the overscroll_refresh_handler_ pointer to null... Acknowledged. https://codereview.chromium.org/2528823002/diff/40001/chrome/browser/android/... File chrome/browser/android/chrome_jni_registrar.cc (left): https://codereview.chromium.org/2528823002/diff/40001/chrome/browser/android/... chrome/browser/android/chrome_jni_registrar.cc:411: {"NativeCallbacks", This doesn't seem related to this CL? https://codereview.chromium.org/2528823002/diff/40001/content/browser/android... File content/browser/android/overscroll_controller_android.cc (right): https://codereview.chromium.org/2528823002/diff/40001/content/browser/android... content/browser/android/overscroll_controller_android.cc:90: if (handler == nullptr) { brace is not necessary here.
tedchoc@chromium.org changed reviewers: + ianwen@chromium.org
+ianwen for the tab.getView discussion https://codereview.chromium.org/2528823002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/SwipeRefreshHandler.java (right): https://codereview.chromium.org/2528823002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/SwipeRefreshHandler.java:200: private void attachSwipeRefreshLayoutIfNecessary() { On 2016/11/28 19:23:54, rlanday wrote: > Unfortunately here and in the following method we're still depending on > mContentViewCore for attaching/detaching the swipe refresh layout view...I'll > look into what we can do about this Tab exposes getView(), so you might be able to add to it directly instead of the CVC. I don't know if we really support adding arbitrary views to that class or if that might cause subtle issues. +ianwen@ for that discussion. https://codereview.chromium.org/2528823002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right): https://codereview.chromium.org/2528823002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1785: mContentViewCore.getWebContents().setOverscrollRefreshHandler(mSwipeRefreshHandler); any reason to not do that inside of the constructor for SwipeRefreshHandler? https://codereview.chromium.org/2528823002/diff/40001/content/browser/web_con... File content/browser/web_contents/web_contents_android.cc (right): https://codereview.chromium.org/2528823002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_android.cc:627: reinterpret_cast<ui::OverscrollRefreshHandler*>( I believe this is undefined behavior in C++ and somewhat dangerous. This "kind" of discusses it: http://stackoverflow.com/questions/7409565/how-to-use-reinterpret-cast-to-cas... In general, you want dynamic cast, which we don't have in Chrome. You also don't know the pointer type, so you can't really use static casts either. https://codereview.chromium.org/2528823002/diff/40001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/OverscrollRefreshHandler.java (right): https://codereview.chromium.org/2528823002/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/OverscrollRefreshHandler.java:45: public long getNativePtr(); public is implied on interfaces, so this method (nor any of the others in the file) need it. In general, we want to ever expose a native pointer outside of a class. You should prefer passing down a java obj and have the native side talk to it via the normal JNI boundary. For this class, could you just add @CalledByNative to all other methods in the interface and then get rid of your chrome/browser/android/swipe_refresh_handler.[cc|h] files? Those seem to only act as the jni bouncer.
https://codereview.chromium.org/2528823002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right): https://codereview.chromium.org/2528823002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1785: mContentViewCore.getWebContents().setOverscrollRefreshHandler(mSwipeRefreshHandler); On 2016/11/29 at 00:06:08, Ted C wrote: > any reason to not do that inside of the constructor for SwipeRefreshHandler? I think this is a good suggestion; @jinsukkim is suggesting the same thing https://codereview.chromium.org/2528823002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1785: mContentViewCore.getWebContents().setOverscrollRefreshHandler(mSwipeRefreshHandler); On 2016/11/29 at 00:06:08, Ted C wrote: > any reason to not do that inside of the constructor for SwipeRefreshHandler? I think this is a good suggestion, I believe it will work https://codereview.chromium.org/2528823002/diff/20001/content/browser/android... File content/browser/android/overscroll_controller_android.cc (right): https://codereview.chromium.org/2528823002/diff/20001/content/browser/android... content/browser/android/overscroll_controller_android.cc:90: if (base::CommandLine::ForCurrentProcess()->HasSwitch( On 2016/11/25 at 01:49:18, Jinsuk Kim wrote: > Now that OverscrollRefreshHandler is not CVC but a separate object, this flag checking can be moved to native SwipeRefreshHandler Init() to avoid creating the instance in the first place rather than passing it all the way here and doing nothing in case the feature is disabled. Sorry, I missed this comment before---I suppose we can do this; is it weird though to always construct the Java SwipeRefreshHandler but sometimes not construct the C++ one? https://codereview.chromium.org/2528823002/diff/20001/content/browser/android... content/browser/android/overscroll_controller_android.cc:104: : compositor_(content_view_core->GetWindowAndroid()->GetCompositor()), On 2016/11/25 at 01:49:18, Jinsuk Kim wrote: > Now you can pass the compositor object directly - then you can get rid of the dependency on CVC. Done https://codereview.chromium.org/2528823002/diff/20001/content/browser/android... content/browser/android/overscroll_controller_android.cc:109: refresh_effect_ = CreateRefreshEffect(overscroll_refresh_handler); On 2016/11/25 at 01:49:18, Jinsuk Kim wrote: > How about keep this intact but update CreateRefreshEffect to handle null pointer? Done https://codereview.chromium.org/2528823002/diff/40001/chrome/browser/android/... File chrome/browser/android/chrome_jni_registrar.cc (left): https://codereview.chromium.org/2528823002/diff/40001/chrome/browser/android/... chrome/browser/android/chrome_jni_registrar.cc:411: {"NativeCallbacks", On 2016/11/28 at 23:06:40, Jinsuk Kim wrote: > This doesn't seem related to this CL? I think this was from git cl format, I will undo it https://codereview.chromium.org/2528823002/diff/40001/content/browser/android... File content/browser/android/overscroll_controller_android.cc (right): https://codereview.chromium.org/2528823002/diff/40001/content/browser/android... content/browser/android/overscroll_controller_android.cc:90: if (handler == nullptr) { On 2016/11/28 at 23:06:41, Jinsuk Kim wrote: > brace is not necessary here. Ok, I'll remove it https://codereview.chromium.org/2528823002/diff/40001/content/browser/web_con... File content/browser/web_contents/web_contents_android.cc (right): https://codereview.chromium.org/2528823002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_android.cc:627: reinterpret_cast<ui::OverscrollRefreshHandler*>( On 2016/11/29 at 00:06:08, Ted C wrote: > I believe this is undefined behavior in C++ and somewhat dangerous. > > This "kind" of discusses it: > http://stackoverflow.com/questions/7409565/how-to-use-reinterpret-cast-to-cas... > > In general, you want dynamic cast, which we don't have in Chrome. You also don't know the pointer type, so you can't really use static casts either. Ah, interesting... Fixing this (maybe by adding @CalledByNative and eliminating swipe_refresh_handler.cc/.h as you're suggesting) seems like a fairly substantial refactor, I will do this and then re-evaluate the other feedback
The CQ bit was checked by rlanday@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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by rlanday@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...
https://codereview.chromium.org/2528823002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/SwipeRefreshHandler.java (right): https://codereview.chromium.org/2528823002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/SwipeRefreshHandler.java:200: private void attachSwipeRefreshLayoutIfNecessary() { On 2016/11/29 00:06:08, Ted C wrote: > On 2016/11/28 19:23:54, rlanday wrote: > > Unfortunately here and in the following method we're still depending on > > mContentViewCore for attaching/detaching the swipe refresh layout view...I'll > > look into what we can do about this > > Tab exposes getView(), so you might be able to add to it directly > instead of the CVC. I don't know if we really support adding arbitrary > views to that class or if that might cause subtle issues. > > +ianwen@ for that discussion. Tab#getView() returns a TabContentViewParent, which holds either a contentView or a nativePageView as its first child, and an infobar container as its second child. You could definitely try to add the swipe layout as its third view. An alternative way is to let the TabContentViewParent know of the swipe layout, and every time the tab's content is changed, it can add the swipe layout to the new content view (either web content view or native page view). A side effect of decoupling CVC with swipe handling is that we could finally use swipe-to-refresh in native pages, which is very handy IMO. :)
The CQ bit was checked by rlanday@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...
Eliminate native SwipeRefreshHandler. Now we just pass around a Java reference in native code. Attach SwipeRefreshHandler to WebContents in SwipeRefreshhHandler constructor. Remove some unnecessary uses of ContentViewCore in SwipeRefreshHandler. @tedchoc + @jinsukkim, it seems like you two have different opinions over the best way to structure this (keeping the native SwipeRefreshHander stuff vs. not keeping it)? https://codereview.chromium.org/2528823002/diff/20001/content/browser/android... File content/browser/android/overscroll_controller_android.cc (right): https://codereview.chromium.org/2528823002/diff/20001/content/browser/android... content/browser/android/overscroll_controller_android.cc:90: if (base::CommandLine::ForCurrentProcess()->HasSwitch( On 2016/11/29 at 00:46:25, rlanday wrote: > On 2016/11/25 at 01:49:18, Jinsuk Kim wrote: > > Now that OverscrollRefreshHandler is not CVC but a separate object, this flag checking can be moved to native SwipeRefreshHandler Init() to avoid creating the instance in the first place rather than passing it all the way here and doing nothing in case the feature is disabled. > > Sorry, I missed this comment before---I suppose we can do this; is it weird though to always construct the Java SwipeRefreshHandler but sometimes not construct the C++ one? I think this discussion is now moot with the latest version of the patch because there is no longer a native SwipeRefreshHandler. https://codereview.chromium.org/2528823002/diff/60001/ui/android/BUILD.gn File ui/android/BUILD.gn (right): https://codereview.chromium.org/2528823002/diff/60001/ui/android/BUILD.gn#new... ui/android/BUILD.gn:79: "../../chrome/android/java/src/org/chromium/chrome/browser/SwipeRefreshHandler.java", Is this acceptable? Referencing a Java file in another module like this felt a little weird when I was writing this. https://codereview.chromium.org/2528823002/diff/60001/ui/android/overscroll_r... File ui/android/overscroll_refresh.h (right): https://codereview.chromium.org/2528823002/diff/60001/ui/android/overscroll_r... ui/android/overscroll_refresh.h:77: const base::android::ScopedJavaGlobalRef<jobject> handler_; I'm not super familiar with how these Java references work; right now I'm keeping three strong references to the Java SwipeRefreshHandler in native code but I'm not sure if this is really the best thing to do. I thought it made sense that the SwipeRefreshHandler shouldn't be garbage collected as long as any of these native objects are referencing it, but it seems a little odd that there's not really a clear point of ownership. https://codereview.chromium.org/2528823002/diff/80001/ui/android/overscroll_r... File ui/android/overscroll_refresh_unittest.cc (left): https://codereview.chromium.org/2528823002/diff/80001/ui/android/overscroll_r... ui/android/overscroll_refresh_unittest.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. I assume we want to rewrite this test case if we go ahead with removing the native SwipeRefreshHandler. Right now it works by plugging an "OverscrollRefreshTest" handler into OverscrollController, which no longer works if OverscrollController is expecting a Java SwipeRefreshHandler instead. Not 100% sure how this could be rewritten, but maybe there's a way to do it by keeping the OverscrollRefreshTest class and mocking out the JNI calls to use it?
On 2016/11/29 at 22:34:18, ianwen wrote: > https://codereview.chromium.org/2528823002/diff/20001/chrome/android/java/src... > File chrome/android/java/src/org/chromium/chrome/browser/SwipeRefreshHandler.java (right): > > https://codereview.chromium.org/2528823002/diff/20001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/SwipeRefreshHandler.java:200: private void attachSwipeRefreshLayoutIfNecessary() { > On 2016/11/29 00:06:08, Ted C wrote: > > On 2016/11/28 19:23:54, rlanday wrote: > > > Unfortunately here and in the following method we're still depending on > > > mContentViewCore for attaching/detaching the swipe refresh layout view...I'll > > > look into what we can do about this > > > > Tab exposes getView(), so you might be able to add to it directly > > instead of the CVC. I don't know if we really support adding arbitrary > > views to that class or if that might cause subtle issues. > > > > +ianwen@ for that discussion. > > Tab#getView() returns a TabContentViewParent, which holds either a contentView or a nativePageView as its first child, and an infobar container as its second child. You could definitely try to add the swipe layout as its third view. > > An alternative way is to let the TabContentViewParent know of the swipe layout, and every time the tab's content is changed, it can add the swipe layout to the new content view (either web content view or native page view). > > A side effect of decoupling CVC with swipe handling is that we could finally use swipe-to-refresh in native pages, which is very handy IMO. :) @jinsukkim said it's OK to just pass in the view from ContentViewCore to SwipeRefreshHandler and not worry about it changing (he says it can only change for a WebView, not in Chrome), so that's how I have it right now.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: blimp_linux_dbg on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
Not sure what tedchoc@ intended to say but this doesn't look like a desirable change. https://codereview.chromium.org/2528823002/diff/60001/ui/android/BUILD.gn File ui/android/BUILD.gn (right): https://codereview.chromium.org/2528823002/diff/60001/ui/android/BUILD.gn#new... ui/android/BUILD.gn:79: "../../chrome/android/java/src/org/chromium/chrome/browser/SwipeRefreshHandler.java", On 2016/11/30 00:18:33, rlanday wrote: > Is this acceptable? Referencing a Java file in another module like this felt a > little weird when I was writing this. It's a layering violation, and not acceptable. This means something is wrong in the overall direction. https://codereview.chromium.org/2528823002/diff/60001/ui/android/overscroll_r... File ui/android/overscroll_refresh.h (right): https://codereview.chromium.org/2528823002/diff/60001/ui/android/overscroll_r... ui/android/overscroll_refresh.h:77: const base::android::ScopedJavaGlobalRef<jobject> handler_; On 2016/11/30 00:18:33, rlanday wrote: > I'm not super familiar with how these Java references work; right now I'm > keeping three strong references to the Java SwipeRefreshHandler in native code > but I'm not sure if this is really the best thing to do. I thought it made sense > that the SwipeRefreshHandler shouldn't be garbage collected as long as any of > these native objects are referencing it, but it seems a little odd that there's > not really a clear point of ownership. I think the reference should be nulled out when it passes the object to other, and only the final one should keep its reference.
I didn't read through all the discussion, but I think the refactor is mostly fine. There is a clear distinction between detecting overscroll, and code that draws the actual animation. This just moves animation interface into ui/, which is fine. https://codereview.chromium.org/2528823002/diff/100001/content/browser/web_co... File content/browser/web_contents/web_contents_view_android.cc (right): https://codereview.chromium.org/2528823002/diff/100001/content/browser/web_co... content/browser/web_contents/web_contents_view_android.cc:227: return new RenderWidgetHostViewAndroid(rwhi, nullptr, nullptr); this breaks pull to refresh for popups https://codereview.chromium.org/2528823002/diff/100001/content/browser/web_co... File content/browser/web_contents/web_contents_view_android.h (right): https://codereview.chromium.org/2528823002/diff/100001/content/browser/web_co... content/browser/web_contents/web_contents_view_android.h:47: void SetOverscrollRefreshHandler( this should not be inlined also what if RWHVA is already created when this is called? https://codereview.chromium.org/2528823002/diff/100001/ui/android/BUILD.gn File ui/android/BUILD.gn (right): https://codereview.chromium.org/2528823002/diff/100001/ui/android/BUILD.gn#ne... ui/android/BUILD.gn:79: "../../chrome/android/java/src/org/chromium/chrome/browser/SwipeRefreshHandler.java", should not include code from other components in this case, I think you need to move OverscrollRefreshHandler from content/ to ui/, and use that to generate bindings https://codereview.chromium.org/2528823002/diff/100001/ui/android/overscroll_... File ui/android/overscroll_refresh.cc (right): https://codereview.chromium.org/2528823002/diff/100001/ui/android/overscroll_... ui/android/overscroll_refresh.cc:26: handler_(nullptr, handler.obj()) { huh odd, null is actually ok here, but I don't see a reason why it should be null, or why that's even allowed by ScopedJavaGlobalRef just call AttachCurrentThread https://codereview.chromium.org/2528823002/diff/100001/ui/android/overscroll_... File ui/android/overscroll_refresh_unittest.cc (left): https://codereview.chromium.org/2528823002/diff/100001/ui/android/overscroll_... ui/android/overscroll_refresh_unittest.cc:11: class OverscrollRefreshTest : public OverscrollRefreshHandler, why are tests being deleted?
On 2016/12/01 at 00:01:58, boliu wrote: > I didn't read through all the discussion, but I think the refactor is mostly fine. There is a clear distinction between detecting overscroll, and code that draws the actual animation. This just moves animation interface into ui/, which is fine. > > https://codereview.chromium.org/2528823002/diff/100001/content/browser/web_co... > File content/browser/web_contents/web_contents_view_android.cc (right): > > https://codereview.chromium.org/2528823002/diff/100001/content/browser/web_co... > content/browser/web_contents/web_contents_view_android.cc:227: return new RenderWidgetHostViewAndroid(rwhi, nullptr, nullptr); > this breaks pull to refresh for popups > > https://codereview.chromium.org/2528823002/diff/100001/content/browser/web_co... > File content/browser/web_contents/web_contents_view_android.h (right): > > https://codereview.chromium.org/2528823002/diff/100001/content/browser/web_co... > content/browser/web_contents/web_contents_view_android.h:47: void SetOverscrollRefreshHandler( > this should not be inlined > > also what if RWHVA is already created when this is called? > > https://codereview.chromium.org/2528823002/diff/100001/ui/android/BUILD.gn > File ui/android/BUILD.gn (right): > > https://codereview.chromium.org/2528823002/diff/100001/ui/android/BUILD.gn#ne... > ui/android/BUILD.gn:79: "../../chrome/android/java/src/org/chromium/chrome/browser/SwipeRefreshHandler.java", > should not include code from other components > > in this case, I think you need to move OverscrollRefreshHandler from content/ to ui/, and use that to generate bindings > > https://codereview.chromium.org/2528823002/diff/100001/ui/android/overscroll_... > File ui/android/overscroll_refresh.cc (right): > > https://codereview.chromium.org/2528823002/diff/100001/ui/android/overscroll_... > ui/android/overscroll_refresh.cc:26: handler_(nullptr, handler.obj()) { > huh odd, null is actually ok here, but I don't see a reason why it should be null, or why that's even allowed by ScopedJavaGlobalRef > > just call AttachCurrentThread > > https://codereview.chromium.org/2528823002/diff/100001/ui/android/overscroll_... > File ui/android/overscroll_refresh_unittest.cc (left): > > https://codereview.chromium.org/2528823002/diff/100001/ui/android/overscroll_... > ui/android/overscroll_refresh_unittest.cc:11: class OverscrollRefreshTest : public OverscrollRefreshHandler, > why are tests being deleted? The test no longer works as written (it requires a substantial refactor) since it depends on the C++ OverscrollRefreshHandler interface existing. If we like the current approach of eliminating the native SwipeRefreshHandler and passing the Java pointer around, I'll figure out a way to rewrite it, I just didn't want to go through the work of doing that until we agreed on the approach to take.
https://codereview.chromium.org/2528823002/diff/100001/content/browser/web_co... File content/browser/web_contents/web_contents_view_android.cc (right): https://codereview.chromium.org/2528823002/diff/100001/content/browser/web_co... content/browser/web_contents/web_contents_view_android.cc:227: return new RenderWidgetHostViewAndroid(rwhi, nullptr, nullptr); On 2016/12/01 at 00:01:57, boliu wrote: > this breaks pull to refresh for popups Hmm, my understanding here was that since we were already passing null for the ContentViewCore (which was previously serving as the OverscrollRefreshHandler), we should also pass null for the new OverscrollRefreshHandler param...is that not correct? Let's discuss this https://codereview.chromium.org/2528823002/diff/100001/content/browser/web_co... File content/browser/web_contents/web_contents_view_android.h (right): https://codereview.chromium.org/2528823002/diff/100001/content/browser/web_co... content/browser/web_contents/web_contents_view_android.h:47: void SetOverscrollRefreshHandler( On 2016/12/01 at 00:01:57, boliu wrote: > this should not be inlined > > also what if RWHVA is already created when this is called? Yeah this is kind of sloppy...what's a good way to do this? I can think of a couple options, none of which seem great: 1. Enforce via invariant that this method can't be called after the RWHVA is constructed 2. Add a bunch of code that may never get used to support switching out the RenderWidgetHostViewAndroid's OverscrollRefreshHandler? 3. Allow calling the method but just not do anything with the OverscrollRefreshHandler? (the way it currently works) I don't think passing it in the WebContentsViewAndroid constructor really works unless we have it all the way back in WebContents::CreateParams when we create the WebContents object, which I suspect will not work
https://codereview.chromium.org/2528823002/diff/100001/content/browser/web_co... File content/browser/web_contents/web_contents_view_android.cc (right): https://codereview.chromium.org/2528823002/diff/100001/content/browser/web_co... content/browser/web_contents/web_contents_view_android.cc:227: return new RenderWidgetHostViewAndroid(rwhi, nullptr, nullptr); On 2016/12/01 00:44:51, rlanday wrote: > On 2016/12/01 at 00:01:57, boliu wrote: > > this breaks pull to refresh for popups > > Hmm, my understanding here was that since we were already passing null for the > ContentViewCore (which was previously serving as the OverscrollRefreshHandler), > we should also pass null for the new OverscrollRefreshHandler param...is that > not correct? Let's discuss this For popups, CVC is set later, in WebContentsViewAndroid::SetContentViewCore https://codereview.chromium.org/2528823002/diff/100001/content/browser/web_co... File content/browser/web_contents/web_contents_view_android.h (right): https://codereview.chromium.org/2528823002/diff/100001/content/browser/web_co... content/browser/web_contents/web_contents_view_android.h:47: void SetOverscrollRefreshHandler( On 2016/12/01 00:44:51, rlanday wrote: > On 2016/12/01 at 00:01:57, boliu wrote: > > this should not be inlined > > > > also what if RWHVA is already created when this is called? > > Yeah this is kind of sloppy...what's a good way to do this? I can think of a > couple options, none of which seem great: > 1. Enforce via invariant that this method can't be called after the RWHVA is > constructed RWHVA can be created before WCVA, ie popups > 2. Add a bunch of code that may never get used to support switching out the > RenderWidgetHostViewAndroid's OverscrollRefreshHandler? This looks like the only option. > 3. Allow calling the method but just not do anything with the > OverscrollRefreshHandler? (the way it currently works) > > I don't think passing it in the WebContentsViewAndroid constructor really works > unless we have it all the way back in WebContents::CreateParams when we create > the WebContents object, which I suspect will not work
My comment about passing the java object was purely to avoid the incorrect dynamic cast from a previous patch. I would encapsulate the java boundary inside of a C++ object instead of passing it into things like RWHA. My back of a napkin thinking is that I would: * Keep ui::OverscrollRefreshHandler, but instead of making it pure virtual, I would have it take the java object and handle the JNI communication. Then the chrome side would just create one of these and pass in your SwipeRefreshHandler instance. * Move OverscrollRefreshHandler.java from content to ui (adding the @CalledByNative) Since the lifetime of the SwipeRefreshHandler.java is 1:1 w/ the web contents, I wonder if we should expose a method to get to the swipe controller in RenderViewHostDelegateView. It does seem slightly odd that we are having to construct the swipe controller to pass into the RWHA when the lifetimes are different. Accessing though the RenderViewHostDelegate (i.e. webcontents -> webcontentsview) seems more correct to me. We should also consider how we can make DEPS smarter as the JNI inclusion that happened definitely was against the layering policy, but the tooling should have warned you of that first. https://codereview.chromium.org/2528823002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/SwipeRefreshHandler.java (right): https://codereview.chromium.org/2528823002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/SwipeRefreshHandler.java:61: public SwipeRefreshHandler(final Context context, Tab tab, WebContents webContents) { no need to pass in WebContents, mTab.getWebContents() will be fine https://codereview.chromium.org/2528823002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/SwipeRefreshHandler.java:128: @CalledByNative The @CalledByNative should be on the OverscrollRefreshHandler interface.
On 2016/12/01 at 00:53:56, tedchoc wrote: > My comment about passing the java object was purely to avoid the incorrect dynamic cast from a previous patch. > > I would encapsulate the java boundary inside of a C++ object instead of passing it into things like RWHA. My back of a napkin thinking is that I would: > * Keep ui::OverscrollRefreshHandler, but instead of making it pure virtual, I would have it take the java object and handle the JNI communication. Then the chrome side would just create one of these and pass in your SwipeRefreshHandler instance. > * Move OverscrollRefreshHandler.java from content to ui (adding the @CalledByNative) > > Since the lifetime of the SwipeRefreshHandler.java is 1:1 w/ the web contents, I wonder if we should expose a method to get to the swipe controller in RenderViewHostDelegateView. It does seem slightly odd that we are having to construct the swipe controller to pass into the RWHA when the lifetimes are different. Accessing though the RenderViewHostDelegate (i.e. webcontents -> webcontentsview) seems more correct to me. > > We should also consider how we can make DEPS smarter as the JNI inclusion that happened definitely was against the layering policy, but the tooling should have warned you of that first. > > https://codereview.chromium.org/2528823002/diff/100001/chrome/android/java/sr... > File chrome/android/java/src/org/chromium/chrome/browser/SwipeRefreshHandler.java (right): > > https://codereview.chromium.org/2528823002/diff/100001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/SwipeRefreshHandler.java:61: public SwipeRefreshHandler(final Context context, Tab tab, WebContents webContents) { > no need to pass in WebContents, mTab.getWebContents() will be fine > > https://codereview.chromium.org/2528823002/diff/100001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/SwipeRefreshHandler.java:128: @CalledByNative > The @CalledByNative should be on the OverscrollRefreshHandler interface. To clarify what you're proposing, you're saying that when RenderWidgetHostViewAndroid constructs the OverscrollRefresh object (and also when OverscrollRefresh needs access to the OverscrollRefreshHandler? or should it copy the C++ OverscrollRefreshHandler object?), we have RenderWidgetHostViewAndroid call host_->delegate()->GetDelegateView() to get the RenderViewHostDelegateView (really a WebContentsViewAndroid instance)? And we want to add a method to expose the OverscrollRefreshHandler (which is currently Android-specific) in the cross-platform RenderViewHostDelegateView interface? Or did you mean we want to cast the pointer to WebContentsViewAndroid and keep this Android-specific?
On 2016/12/01 19:25:18, rlanday wrote: > On 2016/12/01 at 00:53:56, tedchoc wrote: > > My comment about passing the java object was purely to avoid the incorrect > dynamic cast from a previous patch. > > > > I would encapsulate the java boundary inside of a C++ object instead of > passing it into things like RWHA. My back of a napkin thinking is that I would: > > * Keep ui::OverscrollRefreshHandler, but instead of making it pure virtual, > I would have it take the java object and handle the JNI communication. Then the > chrome side would just create one of these and pass in your SwipeRefreshHandler > instance. > > * Move OverscrollRefreshHandler.java from content to ui (adding the > @CalledByNative) > > > > Since the lifetime of the SwipeRefreshHandler.java is 1:1 w/ the web contents, > I wonder if we should expose a method to get to the swipe controller in > RenderViewHostDelegateView. It does seem slightly odd that we are having to > construct the swipe controller to pass into the RWHA when the lifetimes are > different. Accessing though the RenderViewHostDelegate (i.e. webcontents -> > webcontentsview) seems more correct to me. > > > > We should also consider how we can make DEPS smarter as the JNI inclusion that > happened definitely was against the layering policy, but the tooling should have > warned you of that first. > > > > > https://codereview.chromium.org/2528823002/diff/100001/chrome/android/java/sr... > > File > chrome/android/java/src/org/chromium/chrome/browser/SwipeRefreshHandler.java > (right): > > > > > https://codereview.chromium.org/2528823002/diff/100001/chrome/android/java/sr... > > > chrome/android/java/src/org/chromium/chrome/browser/SwipeRefreshHandler.java:61: > public SwipeRefreshHandler(final Context context, Tab tab, WebContents > webContents) { > > no need to pass in WebContents, mTab.getWebContents() will be fine > > > > > https://codereview.chromium.org/2528823002/diff/100001/chrome/android/java/sr... > > > chrome/android/java/src/org/chromium/chrome/browser/SwipeRefreshHandler.java:128: > @CalledByNative > > The @CalledByNative should be on the OverscrollRefreshHandler interface. > > To clarify what you're proposing, you're saying that when > RenderWidgetHostViewAndroid constructs the OverscrollRefresh object (and also > when OverscrollRefresh needs access to the OverscrollRefreshHandler? or should > it copy the C++ OverscrollRefreshHandler object?), we have > RenderWidgetHostViewAndroid call host_->delegate()->GetDelegateView() to get the > RenderViewHostDelegateView (really a WebContentsViewAndroid instance)? And we > want to add a method to expose the OverscrollRefreshHandler (which is currently > Android-specific) in the cross-platform RenderViewHostDelegateView interface? Or > did you mean we want to cast the pointer to WebContentsViewAndroid and keep this > Android-specific? I'll preface that much of what I was suggesting could be outside the scope of this change. I don't have enough context to know whether content owners would be happy adding ifdefs for Android specific APIs in RenderViewHostDelegateView (I wouldn't suggest adding it w/o the ifdef as long as we are the only client). If we went down the route that OverscrollRefreshHandler was exposed through that interface, then we wouldn't need to have any setters on RWHA as it would just pull it from the delegateview if it needs it. If that is too much churn, just passing a OverscrollRefreshHandler to RWHA seems sufficient to me.
On 2016/12/01 at 20:07:53, tedchoc wrote: > On 2016/12/01 19:25:18, rlanday wrote: > > On 2016/12/01 at 00:53:56, tedchoc wrote: > > > My comment about passing the java object was purely to avoid the incorrect > > dynamic cast from a previous patch. > > > > > > I would encapsulate the java boundary inside of a C++ object instead of > > passing it into things like RWHA. My back of a napkin thinking is that I would: > > > * Keep ui::OverscrollRefreshHandler, but instead of making it pure virtual, > > I would have it take the java object and handle the JNI communication. Then the > > chrome side would just create one of these and pass in your SwipeRefreshHandler > > instance. > > > * Move OverscrollRefreshHandler.java from content to ui (adding the > > @CalledByNative) > > > > > > Since the lifetime of the SwipeRefreshHandler.java is 1:1 w/ the web contents, > > I wonder if we should expose a method to get to the swipe controller in > > RenderViewHostDelegateView. It does seem slightly odd that we are having to > > construct the swipe controller to pass into the RWHA when the lifetimes are > > different. Accessing though the RenderViewHostDelegate (i.e. webcontents -> > > webcontentsview) seems more correct to me. > > > > > > We should also consider how we can make DEPS smarter as the JNI inclusion that > > happened definitely was against the layering policy, but the tooling should have > > warned you of that first. > > > > > > > > https://codereview.chromium.org/2528823002/diff/100001/chrome/android/java/sr... > > > File > > chrome/android/java/src/org/chromium/chrome/browser/SwipeRefreshHandler.java > > (right): > > > > > > > > https://codereview.chromium.org/2528823002/diff/100001/chrome/android/java/sr... > > > > > chrome/android/java/src/org/chromium/chrome/browser/SwipeRefreshHandler.java:61: > > public SwipeRefreshHandler(final Context context, Tab tab, WebContents > > webContents) { > > > no need to pass in WebContents, mTab.getWebContents() will be fine > > > > > > > > https://codereview.chromium.org/2528823002/diff/100001/chrome/android/java/sr... > > > > > chrome/android/java/src/org/chromium/chrome/browser/SwipeRefreshHandler.java:128: > > @CalledByNative > > > The @CalledByNative should be on the OverscrollRefreshHandler interface. > > > > To clarify what you're proposing, you're saying that when > > RenderWidgetHostViewAndroid constructs the OverscrollRefresh object (and also > > when OverscrollRefresh needs access to the OverscrollRefreshHandler? or should > > it copy the C++ OverscrollRefreshHandler object?), we have > > RenderWidgetHostViewAndroid call host_->delegate()->GetDelegateView() to get the > > RenderViewHostDelegateView (really a WebContentsViewAndroid instance)? And we > > want to add a method to expose the OverscrollRefreshHandler (which is currently > > Android-specific) in the cross-platform RenderViewHostDelegateView interface? Or > > did you mean we want to cast the pointer to WebContentsViewAndroid and keep this > > Android-specific? > > I'll preface that much of what I was suggesting could be outside the scope of > this change. > > I don't have enough context to know whether content owners would be happy adding > ifdefs for Android specific APIs in RenderViewHostDelegateView (I wouldn't > suggest adding it w/o the ifdef as long as we are the only client). > > If we went down the route that OverscrollRefreshHandler was exposed through > that interface, then we wouldn't need to have any setters on RWHA as it would > just pull it from the delegateview if it needs it. > > If that is too much churn, just passing a OverscrollRefreshHandler to RWHA > seems sufficient to me. I was just talking to boliu, he likes the idea of putting the API in RenderViewHostDelegateView and wrapping it with an ifdef. He says passing an OverscrollRefreshHandler to RenderWidgetHostViewAndroid as I'm doing now is problematic because RenderWidgetHostViewAndroid is created before WebContentsViewAndroid for popups. If someone objects to adding the API to RenderViewHostDelegateView, we'll have to come up with another solution.
On 2016/12/01 20:40:26, rlanday wrote: > I was just talking to boliu, he likes the idea of putting the API in > RenderViewHostDelegateView and wrapping it with an ifdef. He says passing an > OverscrollRefreshHandler to RenderWidgetHostViewAndroid as I'm doing now is > problematic because RenderWidgetHostViewAndroid is created before > WebContentsViewAndroid for popups. If someone objects to adding the API to > RenderViewHostDelegateView, we'll have to come up with another solution. I'm ok with defined(OS_ANDROID) in RenderViewHostDelegateView. And I technically own that code..
The CQ bit was checked by rlanday@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_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by rlanday@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: This issue passed the CQ dry run.
Ok, I have it refactored as we discussed. ui::OverscrollRefreshHandler now wraps the Java OverscrollRefreshHandler.
https://codereview.chromium.org/2528823002/diff/140001/content/browser/androi... File content/browser/android/overscroll_controller_android.cc (right): https://codereview.chromium.org/2528823002/diff/140001/content/browser/androi... content/browser/android/overscroll_controller_android.cc:12: #include "content/browser/android/content_view_core_impl.h" don't need this include https://codereview.chromium.org/2528823002/diff/140001/content/browser/androi... content/browser/android/overscroll_controller_android.cc:90: if (overscroll_refresh_handler == nullptr) should write RWHVA/WCVA such that this is never null here so I think instead of relying on RenderWidgetHostViewAndroid::SetContentViewCore to create OverscrollController, maybe should be an explicit signal when WCVA gets the OverscrollRefreshHandler? https://codereview.chromium.org/2528823002/diff/140001/content/browser/androi... File content/browser/android/overscroll_controller_android.h (right): https://codereview.chromium.org/2528823002/diff/140001/content/browser/androi... content/browser/android/overscroll_controller_android.h:40: explicit OverscrollControllerAndroid( don't need explicit (only matters if constructor has a single argument) https://codereview.chromium.org/2528823002/diff/140001/content/browser/render... File content/browser/renderer_host/render_view_host_delegate.h (right): https://codereview.chromium.org/2528823002/diff/140001/content/browser/render... content/browser/renderer_host/render_view_host_delegate.h:16: #include "content/browser/renderer_host/render_view_host_delegate_view.h" forward declaration should still be good enough? https://codereview.chromium.org/2528823002/diff/140001/content/browser/render... File content/browser/renderer_host/render_view_host_delegate_view.cc (right): https://codereview.chromium.org/2528823002/diff/140001/content/browser/render... content/browser/renderer_host/render_view_host_delegate_view.cc:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. remove (c) https://codereview.chromium.org/2528823002/diff/140001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2528823002/diff/140001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1891: if (!overscroll_controller_) style: need braces if the body is more than one line long (ie they are needed already before this change) https://codereview.chromium.org/2528823002/diff/140001/content/browser/web_co... File content/browser/web_contents/web_contents_android.cc (right): https://codereview.chromium.org/2528823002/diff/140001/content/browser/web_co... content/browser/web_contents/web_contents_android.cc:630: std::move(native_overscroll_refresh_handler)); nit: you can write these two statements in one, which doesn't require a move https://codereview.chromium.org/2528823002/diff/140001/content/browser/web_co... File content/browser/web_contents/web_contents_view_android.h (right): https://codereview.chromium.org/2528823002/diff/140001/content/browser/web_co... content/browser/web_contents/web_contents_view_android.h:49: ui::OverscrollRefreshHandler* GetOverscrollRefreshHandler() const override; Move this next to the other RenderViewHostDelegateView overrides https://codereview.chromium.org/2528823002/diff/140001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/OverscrollRefreshHandler.java (right): https://codereview.chromium.org/2528823002/diff/140001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/OverscrollRefreshHandler.java:12: public interface OverscrollRefreshHandler { you can add @JNINamespace("ui"), then the native impl class can move into the ui namespace https://codereview.chromium.org/2528823002/diff/140001/ui/android/overscroll_... File ui/android/overscroll_refresh.h (right): https://codereview.chromium.org/2528823002/diff/140001/ui/android/overscroll_... ui/android/overscroll_refresh.h:31: explicit OverscrollRefresh(ui::OverscrollRefreshHandler* handler); don't need this, already in the ui namespace here https://codereview.chromium.org/2528823002/diff/140001/ui/android/overscroll_... ui/android/overscroll_refresh.h:77: ui::OverscrollRefreshHandler* handler_; ditto https://codereview.chromium.org/2528823002/diff/140001/ui/android/overscroll_... File ui/android/overscroll_refresh_unittest.cc (right): https://codereview.chromium.org/2528823002/diff/140001/ui/android/overscroll_... ui/android/overscroll_refresh_unittest.cc:19: bool PullStart() { why remove overrides?
https://codereview.chromium.org/2528823002/diff/140001/content/browser/androi... File content/browser/android/overscroll_controller_android.cc (right): https://codereview.chromium.org/2528823002/diff/140001/content/browser/androi... content/browser/android/overscroll_controller_android.cc:12: #include "content/browser/android/content_view_core_impl.h" On 2016/12/03 at 01:05:28, boliu wrote: > don't need this include It's needed for member access into blink::WebGestureEvent& https://codereview.chromium.org/2528823002/diff/140001/content/browser/androi... content/browser/android/overscroll_controller_android.cc:90: if (overscroll_refresh_handler == nullptr) On 2016/12/03 at 01:05:28, boliu wrote: > should write RWHVA/WCVA such that this is never null here > > so I think instead of relying on RenderWidgetHostViewAndroid::SetContentViewCore to create OverscrollController, maybe should be an explicit signal when WCVA gets the OverscrollRefreshHandler? I'm not quite sure what you mean by "explicit signal", are you saying that WebContentsViewAndroid should construct the OverscrollController instead of constructing it here? https://codereview.chromium.org/2528823002/diff/140001/content/browser/androi... File content/browser/android/overscroll_controller_android.h (right): https://codereview.chromium.org/2528823002/diff/140001/content/browser/androi... content/browser/android/overscroll_controller_android.h:40: explicit OverscrollControllerAndroid( On 2016/12/03 at 01:05:28, boliu wrote: > don't need explicit (only matters if constructor has a single argument) Ok https://codereview.chromium.org/2528823002/diff/140001/content/browser/render... File content/browser/renderer_host/render_view_host_delegate.h (right): https://codereview.chromium.org/2528823002/diff/140001/content/browser/render... content/browser/renderer_host/render_view_host_delegate.h:16: #include "content/browser/renderer_host/render_view_host_delegate_view.h" On 2016/12/03 at 01:05:28, boliu wrote: > forward declaration should still be good enough? Oh right, this should go in content/browser/renderer_host/render_widget_host_view_android.cc https://codereview.chromium.org/2528823002/diff/140001/content/browser/render... File content/browser/renderer_host/render_view_host_delegate_view.cc (right): https://codereview.chromium.org/2528823002/diff/140001/content/browser/render... content/browser/renderer_host/render_view_host_delegate_view.cc:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. On 2016/12/03 at 01:05:28, boliu wrote: > remove (c) Good catch! Would be nice to have a presubmit rule for this... https://codereview.chromium.org/2528823002/diff/140001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2528823002/diff/140001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1891: if (!overscroll_controller_) On 2016/12/03 at 01:05:28, boliu wrote: > style: need braces if the body is more than one line long (ie they are needed already before this change) Ok Personally I like the "orthodontist's rule" (you always need braces) but I think there's another comment on this CL complaining that I added them somewhere :) https://codereview.chromium.org/2528823002/diff/140001/content/browser/web_co... File content/browser/web_contents/web_contents_android.cc (right): https://codereview.chromium.org/2528823002/diff/140001/content/browser/web_co... content/browser/web_contents/web_contents_android.cc:630: std::move(native_overscroll_refresh_handler)); On 2016/12/03 at 01:05:28, boliu wrote: > nit: you can write these two statements in one, which doesn't require a move Ok https://codereview.chromium.org/2528823002/diff/140001/content/browser/web_co... File content/browser/web_contents/web_contents_view_android.h (right): https://codereview.chromium.org/2528823002/diff/140001/content/browser/web_co... content/browser/web_contents/web_contents_view_android.h:49: ui::OverscrollRefreshHandler* GetOverscrollRefreshHandler() const override; On 2016/12/03 at 01:05:28, boliu wrote: > Move this next to the other RenderViewHostDelegateView overrides Ok https://codereview.chromium.org/2528823002/diff/140001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/OverscrollRefreshHandler.java (right): https://codereview.chromium.org/2528823002/diff/140001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/OverscrollRefreshHandler.java:12: public interface OverscrollRefreshHandler { On 2016/12/03 at 01:05:28, boliu wrote: > you can add @JNINamespace("ui"), then the native impl class can move into the ui namespace Isn't it already? The C++ OverscrollRefreshHandler class? https://codereview.chromium.org/2528823002/diff/140001/ui/android/overscroll_... File ui/android/overscroll_refresh.h (right): https://codereview.chromium.org/2528823002/diff/140001/ui/android/overscroll_... ui/android/overscroll_refresh.h:31: explicit OverscrollRefresh(ui::OverscrollRefreshHandler* handler); On 2016/12/03 01:05:28, boliu wrote: > don't need this, already in the ui namespace here Ok https://codereview.chromium.org/2528823002/diff/140001/ui/android/overscroll_... ui/android/overscroll_refresh.h:77: ui::OverscrollRefreshHandler* handler_; On 2016/12/03 01:05:28, boliu wrote: > ditto Ok https://codereview.chromium.org/2528823002/diff/140001/ui/android/overscroll_... File ui/android/overscroll_refresh_unittest.cc (right): https://codereview.chromium.org/2528823002/diff/140001/ui/android/overscroll_... ui/android/overscroll_refresh_unittest.cc:19: bool PullStart() { On 2016/12/03 01:05:28, boliu wrote: > why remove overrides? The override keyword can only be used with virtual methods. Since the OverscrollRefreshHandler class is becoming something that just forwards all the method calls to a Java implementation instead of being overridden itself, the methods are no longer virtual.
The CQ bit was checked by rlanday@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...
Updated with boliu's requested changes https://codereview.chromium.org/2528823002/diff/140001/content/browser/androi... File content/browser/android/overscroll_controller_android.cc (right): https://codereview.chromium.org/2528823002/diff/140001/content/browser/androi... content/browser/android/overscroll_controller_android.cc:90: if (overscroll_refresh_handler == nullptr) On 2016/12/05 at 19:53:56, rlanday wrote: > On 2016/12/03 at 01:05:28, boliu wrote: > > should write RWHVA/WCVA such that this is never null here > > > > so I think instead of relying on RenderWidgetHostViewAndroid::SetContentViewCore to create OverscrollController, maybe should be an explicit signal when WCVA gets the OverscrollRefreshHandler? > > I'm not quite sure what you mean by "explicit signal", are you saying that WebContentsViewAndroid should construct the OverscrollController instead of constructing it here? Ok, so I took the call to CreateOverscrollController() out of RenderWidgetHostViewAndroid::SetContentViewCore(). There are now three places where I have calls to CreateOverscrollAndroid(): 1. The RenderWidgetHostViewAndroid constructor. The constructor calls SetContentViewCore(), which before my change was creating the OverscrollController in the case where the handler is already set in WebContentsViewAndroid before RenderWidgetHostViewAndroid is constructed. 2. I'm adding a new method, RenderWidgetHostViewAndroid::OnSetOverscrollRefreshHandler(), which is called by WebContentsViewAndroid::SetOverscrollRefreshHandler() if the RWHVA has already been constructed. 3. RenderWidgetHostViewAndroid::OnAttachCompositor(). I don't 100% understand under what circumstances we need this one, but I assume it's necessary since the code is already doing it here. Does this sound reasonable? Should I add a helper method, either for host_->delegate()->GetDelegateView()->GetOverscrollRefreshHandler() or to actually create the OverscrollController, to reduce code duplication? Note that in OnSetOverscrollRefreshHandler(), we know the handler is not null (I think), but in the other cases we need to do the check. Could we have one function that just does the check whether or not it's needed, or would the unnecessary check be potentially confusing?
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_...)
replying to comments only https://codereview.chromium.org/2528823002/diff/140001/content/browser/androi... File content/browser/android/overscroll_controller_android.cc (right): https://codereview.chromium.org/2528823002/diff/140001/content/browser/androi... content/browser/android/overscroll_controller_android.cc:12: #include "content/browser/android/content_view_core_impl.h" On 2016/12/05 19:53:56, rlanday wrote: > On 2016/12/03 at 01:05:28, boliu wrote: > > don't need this include > > It's needed for member access into blink::WebGestureEvent& then forward declare that as well, or include the WebGestureEvent header https://codereview.chromium.org/2528823002/diff/140001/content/browser/androi... content/browser/android/overscroll_controller_android.cc:90: if (overscroll_refresh_handler == nullptr) On 2016/12/05 23:01:33, rlanday wrote: > On 2016/12/05 at 19:53:56, rlanday wrote: > > On 2016/12/03 at 01:05:28, boliu wrote: > > > should write RWHVA/WCVA such that this is never null here > > > > > > so I think instead of relying on > RenderWidgetHostViewAndroid::SetContentViewCore to create OverscrollController, > maybe should be an explicit signal when WCVA gets the OverscrollRefreshHandler? > > > > I'm not quite sure what you mean by "explicit signal", are you saying that > WebContentsViewAndroid should construct the OverscrollController instead of > constructing it here? > > Ok, so I took the call to CreateOverscrollController() out of > RenderWidgetHostViewAndroid::SetContentViewCore(). There are now three places > where I have calls to CreateOverscrollAndroid(): > > 1. The RenderWidgetHostViewAndroid constructor. The constructor calls > SetContentViewCore(), which before my change was creating the > OverscrollController in the case where the handler is already set in > WebContentsViewAndroid before RenderWidgetHostViewAndroid is constructed. > > 2. I'm adding a new method, > RenderWidgetHostViewAndroid::OnSetOverscrollRefreshHandler(), which is called by > WebContentsViewAndroid::SetOverscrollRefreshHandler() if the RWHVA has already > been constructed. > > 3. RenderWidgetHostViewAndroid::OnAttachCompositor(). I don't 100% understand > under what circumstances we need this one, but I assume it's necessary since the > code is already doing it here. > > Does this sound reasonable? Should I add a helper method, either for > host_->delegate()->GetDelegateView()->GetOverscrollRefreshHandler() > > or to actually create the OverscrollController, to reduce code duplication? Note > that in OnSetOverscrollRefreshHandler(), we know the handler is not null (I > think), but in the other cases we need to do the check. Could we have one > function that just does the check whether or not it's needed, or would the > unnecessary check be potentially confusing? Overall pretty reasonable to me. Probably fine either way whether you want to refactor calling CreateOverscrollController into a separate function. I think in this case, once the Handler is set, it's never unset, right? So either is is fine with me :) https://codereview.chromium.org/2528823002/diff/140001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/OverscrollRefreshHandler.java (right): https://codereview.chromium.org/2528823002/diff/140001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/OverscrollRefreshHandler.java:12: public interface OverscrollRefreshHandler { On 2016/12/05 19:53:56, rlanday wrote: > On 2016/12/03 at 01:05:28, boliu wrote: > > you can add @JNINamespace("ui"), then the native impl class can move into the > ui namespace > > Isn't it already? The C++ OverscrollRefreshHandler class? Err... how did that compile? Can you double check the generated OverscrollRefreshHandler_jni.h file? how does it even know the native code is in the ui namespace..? https://codereview.chromium.org/2528823002/diff/140001/ui/android/overscroll_... File ui/android/overscroll_refresh_unittest.cc (right): https://codereview.chromium.org/2528823002/diff/140001/ui/android/overscroll_... ui/android/overscroll_refresh_unittest.cc:19: bool PullStart() { On 2016/12/05 19:53:57, rlanday wrote: > On 2016/12/03 01:05:28, boliu wrote: > > why remove overrides? > > The override keyword can only be used with virtual methods. Since the > OverscrollRefreshHandler class is becoming something that just forwards all the > method calls to a Java implementation instead of being overridden itself, the > methods are no longer virtual. Err, then are you sure this test still works? I think none of these methods will be called then, because test always calls things through the parent type? I think the parent needs to declare these as virtual, just because they are overridden in tests. So do that and add a comment.
yay, much better https://codereview.chromium.org/2528823002/diff/160001/content/browser/androi... File content/browser/android/overscroll_controller_android.cc (right): https://codereview.chromium.org/2528823002/diff/160001/content/browser/androi... content/browser/android/overscroll_controller_android.cc:19: #include "ui/android/overscroll_refresh_handler.h" actually this is just a pointer that's passed through, might not need the include at all https://codereview.chromium.org/2528823002/diff/160001/content/browser/androi... File content/browser/android/overscroll_controller_android.h (right): https://codereview.chromium.org/2528823002/diff/160001/content/browser/androi... content/browser/android/overscroll_controller_android.h:16: #include "ui/android/overscroll_refresh_handler.h" I think forward declare this in header and then include in cc should be fine here https://codereview.chromium.org/2528823002/diff/160001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2528823002/diff/160001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:81: #include "ui/android/overscroll_refresh_handler.h" ditto, forward declare should be enough it seems https://codereview.chromium.org/2528823002/diff/160001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1236: overscroll_controller_ = CreateOverscrollController( hmm, DCHECK(!overscroll_controller_) first, to make sure we are not overwriting an existing one? https://codereview.chromium.org/2528823002/diff/160001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.h (right): https://codereview.chromium.org/2528823002/diff/160001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.h:240: void OnSetOverscrollRefreshHandler(); OverscrollRefreshHandlerSet? https://codereview.chromium.org/2528823002/diff/160001/content/browser/web_co... File content/browser/web_contents/web_contents_view_android.cc (right): https://codereview.chromium.org/2528823002/diff/160001/content/browser/web_co... content/browser/web_contents/web_contents_view_android.cc:118: web_contents_->GetRenderWidgetHostView()); need to update interstitial as well, see the ShowingInterstitialPage block in SetContentViewCore above https://codereview.chromium.org/2528823002/diff/160001/ui/android/overscroll_... File ui/android/overscroll_refresh.h (right): https://codereview.chromium.org/2528823002/diff/160001/ui/android/overscroll_... ui/android/overscroll_refresh.h:77: OverscrollRefreshHandler* handler_; can stay const I think? https://codereview.chromium.org/2528823002/diff/160001/ui/android/overscroll_... File ui/android/overscroll_refresh_handler.h (right): https://codereview.chromium.org/2528823002/diff/160001/ui/android/overscroll_... ui/android/overscroll_refresh_handler.h:35: }; nit: empty line below
On 2016/12/06 at 00:46:34, boliu wrote: > replying to comments only > > https://codereview.chromium.org/2528823002/diff/140001/content/browser/androi... > File content/browser/android/overscroll_controller_android.cc (right): > > https://codereview.chromium.org/2528823002/diff/140001/content/browser/androi... > content/browser/android/overscroll_controller_android.cc:12: #include "content/browser/android/content_view_core_impl.h" > On 2016/12/05 19:53:56, rlanday wrote: > > On 2016/12/03 at 01:05:28, boliu wrote: > > > don't need this include > > > > It's needed for member access into blink::WebGestureEvent& > > then forward declare that as well, or include the WebGestureEvent header > > https://codereview.chromium.org/2528823002/diff/140001/content/browser/androi... > content/browser/android/overscroll_controller_android.cc:90: if (overscroll_refresh_handler == nullptr) > On 2016/12/05 23:01:33, rlanday wrote: > > On 2016/12/05 at 19:53:56, rlanday wrote: > > > On 2016/12/03 at 01:05:28, boliu wrote: > > > > should write RWHVA/WCVA such that this is never null here > > > > > > > > so I think instead of relying on > > RenderWidgetHostViewAndroid::SetContentViewCore to create OverscrollController, > > maybe should be an explicit signal when WCVA gets the OverscrollRefreshHandler? > > > > > > I'm not quite sure what you mean by "explicit signal", are you saying that > > WebContentsViewAndroid should construct the OverscrollController instead of > > constructing it here? > > > > Ok, so I took the call to CreateOverscrollController() out of > > RenderWidgetHostViewAndroid::SetContentViewCore(). There are now three places > > where I have calls to CreateOverscrollAndroid(): > > > > 1. The RenderWidgetHostViewAndroid constructor. The constructor calls > > SetContentViewCore(), which before my change was creating the > > OverscrollController in the case where the handler is already set in > > WebContentsViewAndroid before RenderWidgetHostViewAndroid is constructed. > > > > 2. I'm adding a new method, > > RenderWidgetHostViewAndroid::OnSetOverscrollRefreshHandler(), which is called by > > WebContentsViewAndroid::SetOverscrollRefreshHandler() if the RWHVA has already > > been constructed. > > > > 3. RenderWidgetHostViewAndroid::OnAttachCompositor(). I don't 100% understand > > under what circumstances we need this one, but I assume it's necessary since the > > code is already doing it here. > > > > Does this sound reasonable? Should I add a helper method, either for > > host_->delegate()->GetDelegateView()->GetOverscrollRefreshHandler() > > > > or to actually create the OverscrollController, to reduce code duplication? Note > > that in OnSetOverscrollRefreshHandler(), we know the handler is not null (I > > think), but in the other cases we need to do the check. Could we have one > > function that just does the check whether or not it's needed, or would the > > unnecessary check be potentially confusing? > > Overall pretty reasonable to me. > > Probably fine either way whether you want to refactor calling CreateOverscrollController into a separate function. I think in this case, once the Handler is set, it's never unset, right? So either is is fine with me :) > > https://codereview.chromium.org/2528823002/diff/140001/ui/android/java/src/or... > File ui/android/java/src/org/chromium/ui/OverscrollRefreshHandler.java (right): > > https://codereview.chromium.org/2528823002/diff/140001/ui/android/java/src/or... > ui/android/java/src/org/chromium/ui/OverscrollRefreshHandler.java:12: public interface OverscrollRefreshHandler { > On 2016/12/05 19:53:56, rlanday wrote: > > On 2016/12/03 at 01:05:28, boliu wrote: > > > you can add @JNINamespace("ui"), then the native impl class can move into the > > ui namespace > > > > Isn't it already? The C++ OverscrollRefreshHandler class? > > Err... how did that compile? Can you double check the generated OverscrollRefreshHandler_jni.h file? how does it even know the native code is in the ui namespace..? > > https://codereview.chromium.org/2528823002/diff/140001/ui/android/overscroll_... > File ui/android/overscroll_refresh_unittest.cc (right): > > https://codereview.chromium.org/2528823002/diff/140001/ui/android/overscroll_... > ui/android/overscroll_refresh_unittest.cc:19: bool PullStart() { > On 2016/12/05 19:53:57, rlanday wrote: > > On 2016/12/03 01:05:28, boliu wrote: > > > why remove overrides? > > > > The override keyword can only be used with virtual methods. Since the > > OverscrollRefreshHandler class is becoming something that just forwards all the > > method calls to a Java implementation instead of being overridden itself, the > > methods are no longer virtual. > > Err, then are you sure this test still works? I think none of these methods will be called then, because test always calls things through the parent type? > > I think the parent needs to declare these as virtual, just because they are overridden in tests. So do that and add a comment. I think you're right, this test shouldn't be working, and indeed it seems that they're failing when I run them locally. I will make these methods virtual again...
There are unit test failures on some of the try bots, hopefully the changes I'm making to respond to your comments will fix them, otherwise I'll have to debug them tomorrow
https://codereview.chromium.org/2528823002/diff/140001/content/browser/androi... File content/browser/android/overscroll_controller_android.cc (right): https://codereview.chromium.org/2528823002/diff/140001/content/browser/androi... content/browser/android/overscroll_controller_android.cc:12: #include "content/browser/android/content_view_core_impl.h" On 2016/12/06 at 00:46:34, boliu wrote: > On 2016/12/05 19:53:56, rlanday wrote: > > On 2016/12/03 at 01:05:28, boliu wrote: > > > don't need this include > > > > It's needed for member access into blink::WebGestureEvent& > > then forward declare that as well, or include the WebGestureEvent header Ok, I will include the WebGestureEvent header (a forward declaration won't work) https://codereview.chromium.org/2528823002/diff/140001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/OverscrollRefreshHandler.java (right): https://codereview.chromium.org/2528823002/diff/140001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/OverscrollRefreshHandler.java:12: public interface OverscrollRefreshHandler { On 2016/12/06 at 00:46:34, boliu wrote: > On 2016/12/05 19:53:56, rlanday wrote: > > On 2016/12/03 at 01:05:28, boliu wrote: > > > you can add @JNINamespace("ui"), then the native impl class can move into the > > ui namespace > > > > Isn't it already? The C++ OverscrollRefreshHandler class? > > Err... how did that compile? Can you double check the generated OverscrollRefreshHandler_jni.h file? how does it even know the native code is in the ui namespace..? I checked the OverscrollRefreshHandler_jni.h file, it doesn't reference the ui namespace or the OverscrollRefreshHandler class. The Java file needs to know the namespace so it can call native methods, the jni.h file is used by the native code to call Java methods. If I recall correctly, I was getting errors about the Java file not being able to find the code in the ui package until I moved the Java file into the Java ui interface. Apparently it's smart enough to not need the JNINamespace annotation when the Java package matches the C++ namespace? https://codereview.chromium.org/2528823002/diff/160001/content/browser/androi... File content/browser/android/overscroll_controller_android.cc (right): https://codereview.chromium.org/2528823002/diff/160001/content/browser/androi... content/browser/android/overscroll_controller_android.cc:19: #include "ui/android/overscroll_refresh_handler.h" On 2016/12/06 at 00:58:16, boliu wrote: > actually this is just a pointer that's passed through, might not need the include at all Yeah, overscroll_refresh.h in the header was apparently including this file...that file can have a forward declaration...then we can include this header where it's actually needed in: content/browser/web_contents/web_contents_android.cc content/browser/web_contents/web_contents_view_android.cc ui/android/overscroll_refresh.cc ui/android/overscroll_refresh_unittest.cc https://codereview.chromium.org/2528823002/diff/160001/content/browser/androi... File content/browser/android/overscroll_controller_android.h (right): https://codereview.chromium.org/2528823002/diff/160001/content/browser/androi... content/browser/android/overscroll_controller_android.h:16: #include "ui/android/overscroll_refresh_handler.h" On 2016/12/06 at 00:58:16, boliu wrote: > I think forward declare this in header and then include in cc should be fine here Yep https://codereview.chromium.org/2528823002/diff/160001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2528823002/diff/160001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:81: #include "ui/android/overscroll_refresh_handler.h" On 2016/12/06 at 00:58:16, boliu wrote: > ditto, forward declare should be enough it seems If I take out the header it still compiles, must be getting either a forward declaration or the header from one of the other includes https://codereview.chromium.org/2528823002/diff/160001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1236: overscroll_controller_ = CreateOverscrollController( On 2016/12/06 at 00:58:16, boliu wrote: > hmm, DCHECK(!overscroll_controller_) first, to make sure we are not overwriting an existing one? Ok (would it be a problem if we were?) https://codereview.chromium.org/2528823002/diff/160001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.h (right): https://codereview.chromium.org/2528823002/diff/160001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.h:240: void OnSetOverscrollRefreshHandler(); On 2016/12/06 at 00:58:16, boliu wrote: > OverscrollRefreshHandlerSet? Ok https://codereview.chromium.org/2528823002/diff/160001/content/browser/web_co... File content/browser/web_contents/web_contents_view_android.cc (right): https://codereview.chromium.org/2528823002/diff/160001/content/browser/web_co... content/browser/web_contents/web_contents_view_android.cc:118: web_contents_->GetRenderWidgetHostView()); On 2016/12/06 at 00:58:16, boliu wrote: > need to update interstitial as well, see the ShowingInterstitialPage block in SetContentViewCore above Ah, ok https://codereview.chromium.org/2528823002/diff/160001/ui/android/overscroll_... File ui/android/overscroll_refresh.h (right): https://codereview.chromium.org/2528823002/diff/160001/ui/android/overscroll_... ui/android/overscroll_refresh.h:77: OverscrollRefreshHandler* handler_; On 2016/12/06 at 00:58:16, boliu wrote: > can stay const I think? Yep https://codereview.chromium.org/2528823002/diff/160001/ui/android/overscroll_... File ui/android/overscroll_refresh_handler.h (right): https://codereview.chromium.org/2528823002/diff/160001/ui/android/overscroll_... ui/android/overscroll_refresh_handler.h:35: }; On 2016/12/06 at 00:58:16, boliu wrote: > nit: empty line below Ok
The CQ bit was checked by rlanday@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...
Updated based on comments, hopefully the trybots all pass but there might be some more test failures to fix
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_...)
https://codereview.chromium.org/2528823002/diff/180001/content/browser/androi... File content/browser/android/overscroll_controller_android.cc (right): https://codereview.chromium.org/2528823002/diff/180001/content/browser/androi... content/browser/android/overscroll_controller_android.cc:108: DCHECK(compositor_); interesting, so looks like this DCHECK is failing on the test bots, so in RWHVA, need to check for null compositor as well https://codereview.chromium.org/2528823002/diff/180001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2528823002/diff/180001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:476: host_->delegate()->GetDelegateView()->GetOverscrollRefreshHandler(); looks like host_->delegate() may be null, so need to null check here and below And as stated elsewhere, also need to check that the compositor in CVC is non-null also content_view_core_ is guaranteed null here, so this will just crash immediately, so definitely don't need this block of code here? loads of null checks to add :p https://codereview.chromium.org/2528823002/diff/180001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1789: overscroll_controller_.reset(); probably this reset should move somewhere else as well? https://codereview.chromium.org/2528823002/diff/180001/ui/android/overscroll_... File ui/android/overscroll_refresh_unittest.cc (right): https://codereview.chromium.org/2528823002/diff/180001/ui/android/overscroll_... ui/android/overscroll_refresh_unittest.cc:17: : OverscrollRefreshHandler(base::android::JavaRef<jobject>()) {} nit: I *think* nullptr is good enough?
https://codereview.chromium.org/2528823002/diff/180001/content/browser/androi... File content/browser/android/overscroll_controller_android.cc (right): https://codereview.chromium.org/2528823002/diff/180001/content/browser/androi... content/browser/android/overscroll_controller_android.cc:108: DCHECK(compositor_); On 2016/12/06 at 16:06:45, boliu wrote: > interesting, so looks like this DCHECK is failing on the test bots, so in RWHVA, need to check for null compositor as well Ok, I'll look into this https://codereview.chromium.org/2528823002/diff/180001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2528823002/diff/180001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:476: host_->delegate()->GetDelegateView()->GetOverscrollRefreshHandler(); > also content_view_core_ is guaranteed null here, so this will just crash immediately, so definitely don't need this block of code here? I'm not sure what you mean here; this is right after content_view_core_ is set by SetContentViewCore, this block of code is needed or the OverscrollController won't get created if the OverscrollRefreshHandler had already been set on the WebContentsViewAndroid (I didn't have this block of code and I couldn't pull to refresh on my phone) I'll look into the null checks https://codereview.chromium.org/2528823002/diff/180001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1789: overscroll_controller_.reset(); On 2016/12/06 at 16:06:45, boliu wrote: > probably this reset should move somewhere else as well? Hmm I think we probably need to reset this when the OverscrollRefreshHandler is set instead? https://codereview.chromium.org/2528823002/diff/180001/ui/android/overscroll_... File ui/android/overscroll_refresh_unittest.cc (right): https://codereview.chromium.org/2528823002/diff/180001/ui/android/overscroll_... ui/android/overscroll_refresh_unittest.cc:17: : OverscrollRefreshHandler(base::android::JavaRef<jobject>()) {} On 2016/12/06 at 16:06:45, boliu wrote: > nit: I *think* nullptr is good enough? Ok I'll try that
https://codereview.chromium.org/2528823002/diff/180001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2528823002/diff/180001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:476: host_->delegate()->GetDelegateView()->GetOverscrollRefreshHandler(); On 2016/12/06 19:01:16, rlanday wrote: > > also content_view_core_ is guaranteed null here, so this will just crash > immediately, so definitely don't need this block of code here? > > I'm not sure what you mean here; this is right after content_view_core_ is set > by SetContentViewCore oh oops, my bad :p > this block of code is needed or the OverscrollController > won't get created if the OverscrollRefreshHandler had already been set on the > WebContentsViewAndroid (I didn't have this block of code and I couldn't pull to > refresh on my phone) > > I'll look into the null checks https://codereview.chromium.org/2528823002/diff/180001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1789: overscroll_controller_.reset(); On 2016/12/06 19:01:16, rlanday wrote: > On 2016/12/06 at 16:06:45, boliu wrote: > > probably this reset should move somewhere else as well? > > Hmm I think we probably need to reset this when the OverscrollRefreshHandler is > set instead? Hmm, if DelegateView is always valid for this lifetime of this object, then the reset probalby is not necessary anymore. I'm not sure if that's the case though.., I guess the question is did CVC used to go away after it's set before. It would be the same thing here I think.
https://codereview.chromium.org/2528823002/diff/180001/content/browser/androi... File content/browser/android/overscroll_controller_android.cc (right): https://codereview.chromium.org/2528823002/diff/180001/content/browser/androi... content/browser/android/overscroll_controller_android.cc:108: DCHECK(compositor_); On 2016/12/06 at 19:01:16, rlanday wrote: > On 2016/12/06 at 16:06:45, boliu wrote: > > interesting, so looks like this DCHECK is failing on the test bots, so in RWHVA, need to check for null compositor as well > > Ok, I'll look into this I'm actually not sure anything is hitting the DCHECK, I think I'm just copying the existing logic over...one of the test cases *was* running into a null pointer issue where you told me to add checks though https://codereview.chromium.org/2528823002/diff/180001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2528823002/diff/180001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1789: overscroll_controller_.reset(); On 2016/12/06 at 19:05:17, boliu wrote: > On 2016/12/06 19:01:16, rlanday wrote: > > On 2016/12/06 at 16:06:45, boliu wrote: > > > probably this reset should move somewhere else as well? > > > > Hmm I think we probably need to reset this when the OverscrollRefreshHandler is > > set instead? > > Hmm, if DelegateView is always valid for this lifetime of this object, then the reset probalby is not necessary anymore. I'm not sure if that's the case though.., I guess the question is did CVC used to go away after it's set before. It would be the same thing here I think. I agree, I am confused if this is something that can actually happen or not.
The CQ bit was checked by rlanday@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...
https://codereview.chromium.org/2528823002/diff/180001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2528823002/diff/180001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1789: overscroll_controller_.reset(); On 2016/12/06 21:26:28, rlanday wrote: > On 2016/12/06 at 19:05:17, boliu wrote: > > On 2016/12/06 19:01:16, rlanday wrote: > > > On 2016/12/06 at 16:06:45, boliu wrote: > > > > probably this reset should move somewhere else as well? > > > > > > Hmm I think we probably need to reset this when the OverscrollRefreshHandler > is > > > set instead? > > > > Hmm, if DelegateView is always valid for this lifetime of this object, then > the reset probalby is not necessary anymore. I'm not sure if that's the case > though.., I guess the question is did CVC used to go away after it's set before. > It would be the same thing here I think. > > I agree, I am confused if this is something that can actually happen or not. RenderWidgetHostViewAndroid::OnContentViewCoreDestroyed so move this reset there https://codereview.chromium.org/2528823002/diff/200001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2528823002/diff/200001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1220: overscroll_controller_.reset(); this can be set multiple times? did this fix a unit test or something? https://codereview.chromium.org/2528823002/diff/200001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1997: if (overscroll_controller_) { style: for if blocks where the both the condition and the body are single line, don't need braces, but do keep them on separate lines I guess that would make this method a bit less wordy.. (note for java, style rule is different, yay...) https://codereview.chromium.org/2528823002/diff/200001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:2007: if (!delegate_view) { can this happen? Pretty sure WebContentsImpl always has a DelegateView.. https://codereview.chromium.org/2528823002/diff/200001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:2019: content_view_core_->GetWindowAndroid()->GetCompositor(), also need to null check GetCompositor
https://codereview.chromium.org/2528823002/diff/200001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2528823002/diff/200001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:2019: content_view_core_->GetWindowAndroid()->GetCompositor(), On 2016/12/06 23:24:16, boliu wrote: > also need to null check GetCompositor actually, need to null check content_view_core_ as well, probably
It looks like there's one more test failure I need to debug
https://codereview.chromium.org/2528823002/diff/200001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2528823002/diff/200001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1220: overscroll_controller_.reset(); On 2016/12/06 at 23:24:16, boliu wrote: > this can be set multiple times? did this fix a unit test or something? We were discussing how in the current version of the code, ContentViewCore can be set multiple times. Since OverscrollRefreshHandler is currently part of ContentViewCore, that means I think the effect is being able to reset OverscrollRefreshHandler (it makes a new OverscrollController). I wasn't sure it's safe to prohibit something that I think is currently allowed. https://codereview.chromium.org/2528823002/diff/200001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1997: if (overscroll_controller_) { On 2016/12/06 at 23:24:16, boliu wrote: > style: for if blocks where the both the condition and the body are single line, don't need braces, but do keep them on separate lines > > I guess that would make this method a bit less wordy.. > > (note for java, style rule is different, yay...) Ok https://codereview.chromium.org/2528823002/diff/200001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:2007: if (!delegate_view) { On 2016/12/06 at 23:24:16, boliu wrote: > can this happen? Pretty sure WebContentsImpl always has a DelegateView.. This is actually what the test case was failing on: [INFO:render_widget_host_view_android.cc(475)] host_: 0xb3fe8880 [INFO:render_widget_host_view_android.cc(476)] host_->delegate()0xb3fc4340 [INFO:render_widget_host_view_android.cc(477)] host_->delegate()->GetDelegateView()0 It's possible there's always a delegate, is that check necessary? https://codereview.chromium.org/2528823002/diff/200001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:2019: content_view_core_->GetWindowAndroid()->GetCompositor(), On 2016/12/06 at 23:26:57, boliu wrote: > On 2016/12/06 23:24:16, boliu wrote: > > also need to null check GetCompositor > > actually, need to null check content_view_core_ as well, probably Hmm, this check doesn't already exist in the current version of the code, does it? Are you saying we need it in case the OverscrollRefreshHandler is set before the ContentViewCore? In that case, are there any other changes we need to make?
https://codereview.chromium.org/2528823002/diff/200001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2528823002/diff/200001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1220: overscroll_controller_.reset(); On 2016/12/07 00:07:48, rlanday wrote: > On 2016/12/06 at 23:24:16, boliu wrote: > > this can be set multiple times? did this fix a unit test or something? > > We were discussing how in the current version of the code, ContentViewCore can > be set multiple times. Since OverscrollRefreshHandler is currently part of > ContentViewCore, that means I think the effect is being able to reset > OverscrollRefreshHandler (it makes a new OverscrollController). I wasn't sure > it's safe to prohibit something that I think is currently allowed. Wording.. it's not CVC can be set multiple times, it's that CVC can be set only once, but that "one time" can be set at different points in the lifetime of RWHVA. (It can be set to null multiple times though, apparently) I think same is true for WCVA https://codereview.chromium.org/2528823002/diff/200001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:2007: if (!delegate_view) { On 2016/12/07 00:07:48, rlanday wrote: > On 2016/12/06 at 23:24:16, boliu wrote: > > can this happen? Pretty sure WebContentsImpl always has a DelegateView.. > > This is actually what the test case was failing on: > [INFO:render_widget_host_view_android.cc(475)] host_: 0xb3fe8880 > [INFO:render_widget_host_view_android.cc(476)] host_->delegate()0xb3fc4340 > [INFO:render_widget_host_view_android.cc(477)] > host_->delegate()->GetDelegateView()0 > > It's possible there's always a delegate, is that check necessary? Ok.. Do you have the full stack of when that happens. I'm interested. https://codereview.chromium.org/2528823002/diff/200001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:2019: content_view_core_->GetWindowAndroid()->GetCompositor(), On 2016/12/07 00:07:48, rlanday wrote: > On 2016/12/06 at 23:26:57, boliu wrote: > > On 2016/12/06 23:24:16, boliu wrote: > > > also need to null check GetCompositor > > > > actually, need to null check content_view_core_ as well, probably > > Hmm, this check doesn't already exist in the current version of the code, does > it? Are you saying we need it in case the OverscrollRefreshHandler is set before > the ContentViewCore? In that case, are there any other changes we need to make? Line 1820 on the left
https://codereview.chromium.org/2528823002/diff/200001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2528823002/diff/200001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:2019: content_view_core_->GetWindowAndroid()->GetCompositor(), On 2016/12/07 00:33:15, boliu wrote: > On 2016/12/07 00:07:48, rlanday wrote: > > On 2016/12/06 at 23:26:57, boliu wrote: > > > On 2016/12/06 23:24:16, boliu wrote: > > > > also need to null check GetCompositor > > > > > > actually, need to null check content_view_core_ as well, probably > > > > Hmm, this check doesn't already exist in the current version of the code, does > > it? Are you saying we need it in case the OverscrollRefreshHandler is set > before > > the ContentViewCore? In that case, are there any other changes we need to > make? > > Line 1820 on the left Oh wait you mean null check for CVC? Well, in theory, OnOverscrollRefreshHandlerSet can come before or after SetCVC(non-null CVC), right? I don't think we should assume which order they happen
https://codereview.chromium.org/2528823002/diff/200001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2528823002/diff/200001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:2007: if (!delegate_view) { > Ok.. Do you have the full stack of when that happens. I'm interested. I 8.385s Main signal 11 (SIGSEGV), code 1 (SEGV_MAPERR), fault addr 0x0 I 8.385s Main I 8.385s Main Stack Trace: I 8.385s Main RELADDR FUNCTION FILE:LINE I 8.385s Main 00db80d1 RenderWidgetHostViewAndroid /usr/local/google/home/rlanday/chromium/src/content/browser/renderer_host/render_widget_host_view_android.cc:476 I 8.385s Main 00b69e4a content::RenderWidgetHostTest_Background_Test::TestBody() /usr/local/google/home/rlanday/chromium/src/content/browser/renderer_host/render_widget_host_unittest.cc:878 I 8.385s Main 014dd742 void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) /usr/local/google/home/rlanday/chromium/src/testing/gtest/src/gtest-internal-inl.h:803 I 8.385s Main 014dd699 testing::Test::Run() /usr/local/google/home/rlanday/chromium/src/testing/gtest/src/gtest.cc:2474 I 8.386s Main 014de028 testing::TestInfo::Run() /usr/local/google/home/rlanday/chromium/src/testing/gtest/src/gtest.cc:2656 I 8.386s Main 014de63f testing::TestCase::Run() /usr/local/google/home/rlanday/chromium/src/testing/gtest/src/gtest.cc:2774 I 8.386s Main 014e4184 testing::internal::UnitTestImpl::RunAllTests() /usr/local/google/home/rlanday/chromium/src/testing/gtest/src/gtest.cc:4647 I 8.386s Main 014e3e97 bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) /usr/local/google/home/rlanday/chromium/src/testing/gtest/src/gtest-internal-inl.h:803 I 8.386s Main 014e3dd3 testing::UnitTest::Run() /usr/local/google/home/rlanday/chromium/src/testing/gtest/src/gtest.cc:4255 I 8.386s Main v------> RUN_ALL_TESTS() /usr/local/google/home/rlanday/chromium/src/testing/gtest/include/gtest/gtest.h:2237 I 8.386s Main 012a17bd base::TestSuite::Run() /usr/local/google/home/rlanday/chromium/src/base/test/test_suite.cc:271 I 8.386s Main 0126620c content::UnitTestTestSuite::Run() /usr/local/google/home/rlanday/chromium/src/content/public/test/unittest_test_suite.cc:45 I 8.386s Main v------> int base::internal::FunctorTraits<int (content::UnitTestTestSuite::*)(), void>::Invoke<content::UnitTestTestSuite*>(int (content::UnitTestTestSuite::*)(), content::UnitTestTestSuite*&&) /usr/local/google/home/rlanday/chromium/src/base/bind_internal.h:214 I 8.386s Main v------> int base::internal::InvokeHelper<false, int>::MakeItSo<int (content::UnitTestTestSuite::* const&)(), content::UnitTestTestSuite*>(int (content::UnitTestTestSuite::* const&)(), content::UnitTestTestSuite*&&) /usr/local/google/home/rlanday/chromium/src/base/bind_internal.h:285 I 8.386s Main v------> int base::internal::Invoker<base::internal::BindState<int (content::UnitTestTestSuite::*)(), base::internal::UnretainedWrapper<content::UnitTestTestSuite> >, int ()>::RunImpl<int (content::UnitTestTestSuite::* const&)(), std::__ndk1::tuple<base::internal::UnretainedWrapper<content::UnitTestTestSuite> > const&, 0u>(int (content::UnitTestTestSuite::* const&)(), std::__ndk1::tuple<base::internal::UnretainedWrapper<content::UnitTestTestSuite> > const&, base::IndexSequence<0u>) /usr/local/google/home/rlanday/chromium/src/base/bind_internal.h:361 I 8.386s Main 00f3e7de base::internal::Invoker<base::internal::BindState<int (content::UnitTestTestSuite::*)(), base::internal::UnretainedWrapper<content::UnitTestTestSuite> >, int ()>::Run(base::internal::BindStateBase*) /usr/local/google/home/rlanday/chromium/src/base/bind_internal.h:339 I 8.386s Main v------> base::internal::RunMixin<base::Callback<int (), (base::internal::CopyMode)1, (base::internal::RepeatMode)1> >::Run() const /usr/local/google/home/rlanday/chromium/src/base/callback.h:85 I 8.387s Main v------> base::(anonymous namespace)::LaunchUnitTestsInternal(base::Callback<int (), (base::internal::CopyMode)1, (base::internal::RepeatMode)1> const&, int, int, bool, base::Callback<void (), (base::internal::CopyMode)1, (base::internal::RepeatMode)1> const&) /usr/local/google/home/rlanday/chromium/src/base/test/launcher/unit_test_launcher.cc:187 I 8.387s Main 012b7e1a base::LaunchUnitTests(int, char**, base::Callback<int (), (base::internal::CopyMode)1, (base::internal::RepeatMode)1> const&) /usr/local/google/home/rlanday/chromium/src/base/test/launcher/unit_test_launcher.cc:453 I 8.387s Main 00f3e775 main /usr/local/google/home/rlanday/chromium/src/content/test/run_all_unittests.cc:22 I 8.387s Main v------> testing::android::RunTests(_JNIEnv*, base::android::JavaParamRef<_jobject*> const&, base::android::JavaParamRef<_jstring*> const&, base::android::JavaParamRef<_jstring*> const&, base::android::JavaParamRef<_jstring*> const&, base::android::JavaParamRef<_jobject*> const&, base::android::JavaParamRef<_jstring*> const&) /usr/local/google/home/rlanday/chromium/src/testing/android/native_test/native_test_launcher.cc:127 I 8.387s Main 01200705 Java_org_chromium_native_1test_NativeTest_nativeRunTests /usr/local/google/home/rlanday/chromium/src/out/Android86/gen/testing/android/native_test/native_test_jni_headers/testing/jni/NativeTest_jni.h:49 I 8.387s Main 00784a88 offset 0x4e1000) (void org.chromium.native_test.NativeTest.nativeRunTests(java.lang.String, java.lang.String, java.lang.String, android.content.Context, java.lang.String)+252 /data/app/org.chromium.native_test-1/oat/x86/base.odex I 8.387s Main 007858fd offset 0x4e1000) (void org.chromium.native_test.NativeTest.runTests(android.app.Activity)+257 /data/app/org.chromium.native_test-1/oat/x86/base.odex I 8.387s Main 00784960 offset 0x4e1000) (void org.chromium.native_test.NativeTest.access$000(org.chromium.native_test.NativeTest, android.app.Activity)+68 /data/app/org.chromium.native_test-1/oat/x86/base.odex I 8.387s Main 007845cd offset 0x4e1000) (void org.chromium.native_test.NativeTest$2.run()+113 /data/app/org.chromium.native_test-1/oat/x86/base.odex I 8.387s Main 7423bc3b offset 0x1eb2000 /data/dalvik-cache/x86/system@framework@boot.oat I 8.387s Main
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
https://codereview.chromium.org/2528823002/diff/200001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2528823002/diff/200001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:2007: if (!delegate_view) { meh, MockRenderWidgetHostDelegate doesn't have a DelegateView: https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_wid... Can you add a comment that this is for unit tests?
On 2016/12/07 at 00:50:19, boliu wrote: > https://codereview.chromium.org/2528823002/diff/200001/content/browser/render... > File content/browser/renderer_host/render_widget_host_view_android.cc (right): > > https://codereview.chromium.org/2528823002/diff/200001/content/browser/render... > content/browser/renderer_host/render_widget_host_view_android.cc:2007: if (!delegate_view) { > meh, MockRenderWidgetHostDelegate doesn't have a DelegateView: > https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_wid... > > Can you add a comment that this is for unit tests? Ok
On 2016/12/07 at 00:50:19, boliu wrote: > https://codereview.chromium.org/2528823002/diff/200001/content/browser/render... > File content/browser/renderer_host/render_widget_host_view_android.cc (right): > > https://codereview.chromium.org/2528823002/diff/200001/content/browser/render... > content/browser/renderer_host/render_widget_host_view_android.cc:2007: if (!delegate_view) { > meh, MockRenderWidgetHostDelegate doesn't have a DelegateView: > https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_wid... > > Can you add a comment that this is for unit tests? Ok
The CQ bit was checked by rlanday@chromium.org to run a CQ dry run
Updated agian, hopefully all the test cases pass now
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: This issue passed the CQ dry run.
https://codereview.chromium.org/2528823002/diff/220001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2528823002/diff/220001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1996: void RenderWidgetHostViewAndroid::CreateOverscrollControllerIfPossible() { also should call this in SetCVC when CVC is not null, matters for the case when the compositor has already been created when SetCVC is called https://codereview.chromium.org/2528823002/diff/220001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:2020: if (!window_android) this one can't happen
Updated with the latest requested changes https://codereview.chromium.org/2528823002/diff/220001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2528823002/diff/220001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1996: void RenderWidgetHostViewAndroid::CreateOverscrollControllerIfPossible() { On 2016/12/07 at 21:05:51, boliu wrote: > also should call this in SetCVC when CVC is not null, matters for the case when the compositor has already been created when SetCVC is called Ok https://codereview.chromium.org/2528823002/diff/220001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:2020: if (!window_android) On 2016/12/07 at 21:05:52, boliu wrote: > this one can't happen Oh, I saw all the "if (GetWindowAndroid())" checks in ContentViewCoreImpl and thought this might be necessary
ahh, forgot to publish... sorry https://codereview.chromium.org/2528823002/diff/220001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2528823002/diff/220001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:2020: if (!window_android) On 2016/12/07 22:13:39, rlanday wrote: > On 2016/12/07 at 21:05:52, boliu wrote: > > this one can't happen > > Oh, I saw all the "if (GetWindowAndroid())" checks in ContentViewCoreImpl and > thought this might be necessary Ohh, you are right. This can be null due to tab re-parenting, which is a user action. But if we ever hit it here, we'd actually leave things in a bad state, since we don't listen for WindowAndroid changing here. Keep this check for now, with a comment to fix this edge case when we get WindowAndroid from ViewAndroid..
The CQ bit was checked by rlanday@chromium.org to run a CQ dry run
rlanday@chromium.org changed reviewers: + tedchoc@chromium.orgr - tedchoc@chromium.org
Re-add WindowAndroid null check
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: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by rlanday@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
you are gonna need other owners, I don't own everything..
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
We now have a merge conflict with https://codereview.chromium.org/2524423002, I think I just need to add "is_showing_overscroll_glow_ = true" to CreateOverscrollControllerIfPossible() but I asked a question to clarify
rlanday@chromium.org changed required reviewers: + tedchoc@chromium.orgr
tedchoc, boliu said this looks good but I need your review for some files he doesn't own, I believe these: chrome/android/java/src/org/chromium/chrome/browser/SwipeRefreshHandler.java chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java ui/android/BUILD.gn ui/android/java/src/org/chromium/ui/OverscrollRefreshHandler.java ui/android/overscroll_refresh.cc ui/android/overscroll_refresh.h ui/android/overscroll_refresh_handler.cc ui/android/overscroll_refresh_handler.h ui/android/overscroll_refresh_unittest.cc https://codereview.chromium.org/2528823002/diff/280001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2528823002/diff/280001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:2044: is_showing_overscroll_glow_ = true; This is the new thing that was added in https://codereview.chromium.org/2524423002, for some reason they're only doing this in SetContentViewCore() and not OnAttachCompositor(), I think it's a bug but I asked there to be sure
https://codereview.chromium.org/2528823002/diff/280001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2528823002/diff/280001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:2044: is_showing_overscroll_glow_ = true; On 2016/12/08 19:29:14, rlanday wrote: > This is the new thing that was added in > https://codereview.chromium.org/2524423002, for some reason they're only doing > this in SetContentViewCore() and not OnAttachCompositor(), I think it's a bug > but I asked there to be sure lgtm
The CQ bit was checked by rlanday@chromium.org
The CQ bit was unchecked by rlanday@chromium.org
tedchoc@chromium.org changed reviewers: + tedchoc@chromium.org
lgtm .. some nits/questions but nothing blocking https://codereview.chromium.org/2528823002/diff/280001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/SwipeRefreshHandler.java (left): https://codereview.chromium.org/2528823002/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/SwipeRefreshHandler.java:114: mContentViewCore.setOverscrollRefreshHandler(null); we don't need to do webContents.setOverscrolllRefreshHandler(null) here? https://codereview.chromium.org/2528823002/diff/280001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/SwipeRefreshHandler.java (right): https://codereview.chromium.org/2528823002/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/SwipeRefreshHandler.java:60: public SwipeRefreshHandler(final Context context, Tab tab, WebContents webContents) { from an earlier comment, but you can just do tab.getWebContents to not pass in webContents. it's odd to do getContentViewCore() but not getWebContents() IMO (and if you needed it, the javadoc would need to be updated) https://codereview.chromium.org/2528823002/diff/280001/content/browser/render... File content/browser/renderer_host/render_view_host_delegate_view.h (right): https://codereview.chromium.org/2528823002/diff/280001/content/browser/render... content/browser/renderer_host/render_view_host_delegate_view.h:23: #if defined(OS_ANDROID) should these ifdefs go outside the namespace declaration for now? https://codereview.chromium.org/2528823002/diff/280001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2528823002/diff/280001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1220: void RenderWidgetHostViewAndroid::OnOverscrollRefreshHandlerSet() { I would probably use Available instead of Set, but that probably makes this far too long. https://codereview.chromium.org/2528823002/diff/280001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:2041: overscroll_controller_ = base::MakeUnique<OverscrollControllerAndroid>( I guess we could also have the RenderViewHostDelegateView expose a CreateOverscrollControllerAndroid or something that could encapsulate all of this compositor gathering and stuff if needed. If we wanted to get CVC out of this file entirely, that might be a way. Not needed now though.
https://codereview.chromium.org/2528823002/diff/280001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/SwipeRefreshHandler.java (left): https://codereview.chromium.org/2528823002/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/SwipeRefreshHandler.java:114: mContentViewCore.setOverscrollRefreshHandler(null); On 2016/12/13 at 21:09:48, Ted C wrote: > we don't need to do webContents.setOverscrolllRefreshHandler(null) here? The code as written does not support doing that...there are several places where we can call CreateOverscrollControllerIfPossible() (one of them being when setOverscrollRefreshHandler() is called), but we don't have code to destroy the controller if we unset the OverscrollRefreshHandler. I talked to boliu, he argues that since SwipeRefreshHandler is only ever destroyed when the CVC is about to be destroyed (inside Tab.destroyContentViewCore()), this doesn't really matter. https://codereview.chromium.org/2528823002/diff/280001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/SwipeRefreshHandler.java (right): https://codereview.chromium.org/2528823002/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/SwipeRefreshHandler.java:60: public SwipeRefreshHandler(final Context context, Tab tab, WebContents webContents) { On 2016/12/13 at 21:09:48, Ted C wrote: > from an earlier comment, but you can just do tab.getWebContents to not pass in webContents. > > it's odd to do getContentViewCore() but not getWebContents() IMO > > (and if you needed it, the javadoc would need to be updated) Ok https://codereview.chromium.org/2528823002/diff/280001/content/browser/render... File content/browser/renderer_host/render_view_host_delegate_view.h (right): https://codereview.chromium.org/2528823002/diff/280001/content/browser/render... content/browser/renderer_host/render_view_host_delegate_view.h:23: #if defined(OS_ANDROID) On 2016/12/13 at 21:09:48, Ted C wrote: > should these ifdefs go outside the namespace declaration for now? If you prefer, I think I found some other file where it was done like this https://codereview.chromium.org/2528823002/diff/280001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2528823002/diff/280001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1220: void RenderWidgetHostViewAndroid::OnOverscrollRefreshHandlerSet() { On 2016/12/13 at 21:09:48, Ted C wrote: > I would probably use Available instead of Set, but that probably makes this far too long. I think that fits https://codereview.chromium.org/2528823002/diff/280001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:2041: overscroll_controller_ = base::MakeUnique<OverscrollControllerAndroid>( On 2016/12/13 at 21:09:48, Ted C wrote: > I guess we could also have the RenderViewHostDelegateView expose a CreateOverscrollControllerAndroid or something that could encapsulate all of this compositor gathering and stuff if needed. > > If we wanted to get CVC out of this file entirely, that might be a way. > > Not needed now though. I added a note here about how we hope to someday get WindowAndroid from ViewAndroid instead of CVC, I think that would also eliminate the dependency?
The CQ bit was checked by rlanday@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from boliu@chromium.org, tedchoc@chromium.org Link to the patchset: https://codereview.chromium.org/2528823002/#ps300001 (title: "tedchoc's requested changes")
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
All required reviewers (with asterisk prefixes) have not yet approved this CL. No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
rlanday@chromium.org changed reviewers: - tedchoc@chromium.orgr
rlanday@chromium.org changed required reviewers: + tedchoc@chromium.org - tedchoc@chromium.orgr
The CQ bit was checked by rlanday@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 300001, "attempt_start_ts": 1481678454236360,
"parent_rev": "1abc2af8125705982a5b62844113cb0c7737a0c6", "commit_rev":
"8c6e2e7d2927888f42af2919023cc9da09792e5f"}
Message was sent while issue was closed.
Description was changed from ========== Separate SwipeRefreshHandler and ContentViewCore Currently Android's native ContentViewCore class implements ui::OverscrollRefreshHandler and handles swipe to refresh events. This change refactors this functionality out of ContentViewCore into a new native SwipeRefreshHandler class as part of a broader effort to eliminate ContentViewCore. BUG=627943 ========== to ========== Separate SwipeRefreshHandler and ContentViewCore Currently Android's native ContentViewCore class implements ui::OverscrollRefreshHandler and handles swipe to refresh events. This change refactors this functionality out of ContentViewCore into a new native SwipeRefreshHandler class as part of a broader effort to eliminate ContentViewCore. BUG=627943 Review-Url: https://codereview.chromium.org/2528823002 ==========
Message was sent while issue was closed.
Committed patchset #16 (id:300001)
Message was sent while issue was closed.
Description was changed from ========== Separate SwipeRefreshHandler and ContentViewCore Currently Android's native ContentViewCore class implements ui::OverscrollRefreshHandler and handles swipe to refresh events. This change refactors this functionality out of ContentViewCore into a new native SwipeRefreshHandler class as part of a broader effort to eliminate ContentViewCore. BUG=627943 Review-Url: https://codereview.chromium.org/2528823002 ========== to ========== Separate SwipeRefreshHandler and ContentViewCore Currently Android's native ContentViewCore class implements ui::OverscrollRefreshHandler and handles swipe to refresh events. This change refactors this functionality out of ContentViewCore into a new native SwipeRefreshHandler class as part of a broader effort to eliminate ContentViewCore. BUG=627943 Committed: https://crrev.com/7f2ec7f88816aa89f872f068881d117047049cfe Cr-Commit-Position: refs/heads/master@{#438397} ==========
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/7f2ec7f88816aa89f872f068881d117047049cfe Cr-Commit-Position: refs/heads/master@{#438397} |
