Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
895543005:
Refactor GestureNavigation to eliminate code redundancy (Closed)
DescriptionRefactor GestureNavigation to eliminate code redundancy
Right now, Gesture Navigation has two very distinct phases: the initial
one, where the web contents window receives events and is moved, and the
follow up navigation, where the page still hasn't loaded and the user
scrolls on the overlay. The meat of this patch is to unify as much of
the logic as possible without sacrificing features. A new class is
introduced, OverscrollWindowAnimation, and the responsibility of the
existing OverscrollNavigationOverlay is steered into performing the
actual navigation.
TEST=Overscroll*
BUG=467692, 464532, 420121
Committed: https://crrev.com/5384f002f839439ef666ed9688069e42ca5ccdca
Cr-Commit-Position: refs/heads/master@{#324275}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Following suggestions by Mikhail, rewritten OWA to use a delegate interface (to be implemented) #
Total comments: 18
Patch Set 3 : Checked comments #
Total comments: 16
Patch Set 4 : New design with window|wrapper in OWA #
Total comments: 64
Patch Set 5 : Addressed Mikhail's comments #
Total comments: 40
Patch Set 6 : Changed code to only have one OWA instance, addressed comments. #Patch Set 7 : Fixed some bugs related to back and forth 2nd case navigation #
Total comments: 18
Patch Set 8 : Fixed nits, brought back the dismiss layer. #Patch Set 9 : Removed unnecessary file. #
Total comments: 2
Patch Set 10 : Removed unused include #Patch Set 11 : Added tests for ONO #
Total comments: 1
Patch Set 12 : Modified patch to work with windows. #
Total comments: 2
Patch Set 13 : Added OWA test, fixed touchpad issues by setting capture on the event window #Patch Set 14 : Removed unnecessary code #Patch Set 15 : Brought back gesture cancellation by mouse and linted code. #
Total comments: 46
Patch Set 16 : Simple OWD tests, better ONO tests. #
Total comments: 36
Patch Set 17 : Addressed comments #Patch Set 18 : Last round of comments. #
Total comments: 36
Patch Set 19 : Addressed comments, ran git cl format #Patch Set 20 : Hopefully fixed trybots #Patch Set 21 : Added exports, fixed BUILD.gn files #Patch Set 22 : Fixed compile error #Patch Set 23 : Added another export #Patch Set 24 : Renamed method that was clashing with some Win macro #
Total comments: 17
Patch Set 25 : Fixed nits #
Total comments: 26
Patch Set 26 : Addressed Sadrul's comments #Patch Set 27 : Moved SetBounds from OWA to ONO #Patch Set 28 : Rebase #Patch Set 29 : Removed unused function declaration #Patch Set 30 : Moved bounds setting away from OWA #Patch Set 31 : Changed bounds setting responsibility #
Total comments: 4
Patch Set 32 : Fixed nit #Messages
Total messages: 69 (15 generated)
nsatragno@chromium.org changed reviewers: + mfomitchev@chromium.org
nsatragno@chromium.org changed required reviewers: + mfomitchev@chromium.org
Mikhail, Here is the WIP patch you requested. Please give it a look and tell me what you think.
https://codereview.chromium.org/895543005/diff/1/content/browser/web_contents... File content/browser/web_contents/aura/overscroll_navigation_overlay.cc (right): https://codereview.chromium.org/895543005/diff/1/content/browser/web_contents... content/browser/web_contents/aura/overscroll_navigation_overlay.cc:259: wcva_->DismissOverlay(); As implemented, OverscrollWindowAnimation::DismissOverlay() (which ends up getting called here) is not really a directive to dismiss - it is a notification that the page finished loading/painting, and then OverscrollWindowAnimation (OWA) really decides when to dismiss based on gesture_completed_, etc. This will become more complicated once we add support for the slider scenario. I am not sure this will work well. What I envisioned instead was that OWA would use a combination of mechanisms similar to WindowSlider::Delegate callbacks and IsSlideInProgress() to keep ONO up to date with its state, and then all the logic for dismissing would be in ONO. Perhaps there's a way to make this work differently - it's hard to tell now because the slider use case is not implemented. I am skeptical though - with the approach I am suggesting OWA is clearly only focused on animation/UI input processing, while as implemented it would also have to contain the logic for layer/window management and timely dismissal based on load/repaint state - this just seems like unnecessarily mixing two separate roles in one class. - If we take the approach I am suggesting, we should probably also keep the DismissAnimator in ONO. https://codereview.chromium.org/895543005/diff/1/content/browser/web_contents... content/browser/web_contents/aura/overscroll_navigation_overlay.cc:322: overlay_dismiss_layer_ = layer.Pass(); Make sure this logic is preserved.
https://codereview.chromium.org/895543005/diff/1/content/browser/web_contents... File content/browser/web_contents/aura/overscroll_window_animation.cc (right): https://codereview.chromium.org/895543005/diff/1/content/browser/web_contents... content/browser/web_contents/aura/overscroll_window_animation.cc:127: contents->parent()->StackChildBelow(contents, overscroll_window_.get()); Similar to the previous comments - this logic should probably be kept out of OWA. Perhaps we could move it to ONO? ONO is already responsible for this sort of management for the slider case, so can it also take on the responsibility for the overlay window case? OWA would notify ONO through WindowSlider::Delegate callbacks (perhaps rename the interface to OWA::Delegate). https://codereview.chromium.org/895543005/diff/1/content/browser/web_contents... content/browser/web_contents/aura/overscroll_window_animation.cc:201: OverscrollWindowDelegate* overscroll_delegate = new OverscrollWindowDelegate( I am not sure this setup code belongs in OWA either.. Basically, I think we shouldn't have any code in OWA which makes a distinction between the overlay window use case vs. the slider use case. It should just be working with windows/layers, it shouldn't know/care what kind of a window/layer it is. If for some reason we need this distinction, I think we shouldn't minimize the number of places where we do that. https://codereview.chromium.org/895543005/diff/1/content/browser/web_contents... content/browser/web_contents/aura/overscroll_window_animation.cc:234: ui::Layer* parent = overscroll_window_->layer()->parent(); Same here
+ CC tdanderson@
Mikhail, can you please take a look at overscroll_window_animation.[h|cc] and share your thoughts on the delegate definition? Thanks!
https://codereview.chromium.org/895543005/diff/20001/content/browser/web_cont... File content/browser/web_contents/aura/overscroll_window_animation.cc (right): https://codereview.chromium.org/895543005/diff/20001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_window_animation.cc:17: animation_completed_(true) { It's a bit weird initialized completed to true given that nothing completed. Perhaps change to animation_active_ and inverse the usage? https://codereview.chromium.org/895543005/diff/20001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_window_animation.cc:36: void OverscrollWindowAnimation::AbortAllAnimations() { This method is called only once. Consider getting rid of it https://codereview.chromium.org/895543005/diff/20001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_window_animation.cc:40: bool OverscrollWindowAnimation::IsSlideInProgress() const { Lets call this something else. Perhaps IsAnimationInProgress? https://codereview.chromium.org/895543005/diff/20001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_window_animation.cc:79: AbortAllAnimations(); Seems like an overkill to call this every time. We only need to call this if there is an active animation and we are starting a new one (i.e. direction_ is not NONE) https://codereview.chromium.org/895543005/diff/20001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_window_animation.cc:80: direction_ = delegate_->StartNavigation(new_mode); StartNavigation(OVERSCROLL_NONE) sounds a bit misleading.. https://codereview.chromium.org/895543005/diff/20001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_window_animation.cc:92: void OverscrollWindowAnimation::StartAnimating() { I don't see much reason for the existence of this method.. https://codereview.chromium.org/895543005/diff/20001/content/browser/web_cont... File content/browser/web_contents/aura/overscroll_window_animation.h (right): https://codereview.chromium.org/895543005/diff/20001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_window_animation.h:47: virtual void OnAnimationCompleted() = 0; OnOverscrollCompleted Maybe OnOverscrollCompleted(scoped_ptr<ui::Layer> layer) (or OverscrollLayerWrapper instead of the Layer) https://codereview.chromium.org/895543005/diff/20001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_window_animation.h:98: bool overscroll_cancelled_; DOesn't seem like you need this https://codereview.chromium.org/895543005/diff/20001/content/browser/web_cont... File content/browser/web_contents/aura/overscroll_window_delegate.cc (right): https://codereview.chromium.org/895543005/diff/20001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_window_delegate.cc:83: // TODO dafuq this does? Thresholds on how much you need to drag before the overscroll engages are defined by start_threshold_touchpad_ and start_threshold_touchscreen_. This just sets the appropriate value depending on whether you use touchscreen on touchpad.
Mikhail, can you give a new look at OWA and the new OverscrollLayerWrapper? See if we're in the right track. Thanks! https://codereview.chromium.org/895543005/diff/20001/content/browser/web_cont... File content/browser/web_contents/aura/overscroll_window_animation.cc (right): https://codereview.chromium.org/895543005/diff/20001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_window_animation.cc:17: animation_completed_(true) { On 2015/02/19 19:04:59, mfomitchev wrote: > It's a bit weird initialized completed to true given that nothing completed. > Perhaps change to animation_active_ and inverse the usage? Done. https://codereview.chromium.org/895543005/diff/20001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_window_animation.cc:36: void OverscrollWindowAnimation::AbortAllAnimations() { On 2015/02/19 19:04:59, mfomitchev wrote: > This method is called only once. Consider getting rid of it Done. https://codereview.chromium.org/895543005/diff/20001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_window_animation.cc:40: bool OverscrollWindowAnimation::IsSlideInProgress() const { On 2015/02/19 19:04:59, mfomitchev wrote: > Lets call this something else. Perhaps IsAnimationInProgress? Done. https://codereview.chromium.org/895543005/diff/20001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_window_animation.cc:79: AbortAllAnimations(); On 2015/02/19 19:04:59, mfomitchev wrote: > Seems like an overkill to call this every time. We only need to call this if > there is an active animation and we are starting a new one (i.e. direction_ is > not NONE) Done. https://codereview.chromium.org/895543005/diff/20001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_window_animation.cc:80: direction_ = delegate_->StartNavigation(new_mode); On 2015/02/19 19:04:59, mfomitchev wrote: > StartNavigation(OVERSCROLL_NONE) sounds a bit misleading.. This should no longer be the case after changing the delegate interface as we discussed. https://codereview.chromium.org/895543005/diff/20001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_window_animation.cc:92: void OverscrollWindowAnimation::StartAnimating() { On 2015/02/19 19:04:59, mfomitchev wrote: > I don't see much reason for the existence of this method.. Removed. https://codereview.chromium.org/895543005/diff/20001/content/browser/web_cont... File content/browser/web_contents/aura/overscroll_window_animation.h (right): https://codereview.chromium.org/895543005/diff/20001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_window_animation.h:47: virtual void OnAnimationCompleted() = 0; On 2015/02/19 19:04:59, mfomitchev wrote: > OnOverscrollCompleted > Maybe OnOverscrollCompleted(scoped_ptr<ui::Layer> layer) (or > OverscrollLayerWrapper instead of the Layer) I think passing the layer instead of the wrapper makes more sense for this case. But as we discussed, it might be OK to not pass the layer. I'll look into this again as I code more of ONO. https://codereview.chromium.org/895543005/diff/20001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_window_animation.h:98: bool overscroll_cancelled_; On 2015/02/19 19:04:59, mfomitchev wrote: > DOesn't seem like you need this Removed. https://codereview.chromium.org/895543005/diff/20001/content/browser/web_cont... File content/browser/web_contents/aura/overscroll_window_delegate.cc (right): https://codereview.chromium.org/895543005/diff/20001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_window_delegate.cc:83: // TODO dafuq this does? On 2015/02/19 19:05:00, mfomitchev wrote: > Thresholds on how much you need to drag before the overscroll engages are > defined by start_threshold_touchpad_ and start_threshold_touchscreen_. This just > sets the appropriate value depending on whether you use touchscreen on touchpad. Acknowledged.
https://codereview.chromium.org/895543005/diff/40001/content/browser/web_cont... File content/browser/web_contents/aura/overscroll_layer_wrapper.h (right): https://codereview.chromium.org/895543005/diff/40001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_layer_wrapper.h:29: class OverscrollLayerWrapper { Perhaps we don't need this - perhaps OWA could always work with a layer and we would only create a window in ONO when OWA's animation finishes https://codereview.chromium.org/895543005/diff/40001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_layer_wrapper.h:38: void WrapWindow(aura::Window* window); Just put these into constr https://codereview.chromium.org/895543005/diff/40001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_layer_wrapper.h:55: ui::Layer* layer_; Perhaps make it own window/layer and have ONO/WCVA/etc own the wrapper https://codereview.chromium.org/895543005/diff/40001/content/browser/web_cont... File content/browser/web_contents/aura/overscroll_window_animation.cc (right): https://codereview.chromium.org/895543005/diff/40001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_window_animation.cc:55: gfx::Vector2dF OverscrollWindowAnimation::GetTranslationForOverscroll( any reason this doesn't just return an int/float? https://codereview.chromium.org/895543005/diff/40001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_window_animation.cc:60: return gfx::Vector2dF(std::max(-bounds_width, delta_x), 0); confirm if delta_x is always positive https://codereview.chromium.org/895543005/diff/40001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_window_animation.cc:101: layer_wrapper_ = direction_ == FORWARD ? delegate_->CreateFrontLayer() Leak old layer if you change the direction from East to West? https://codereview.chromium.org/895543005/diff/40001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_window_animation.cc:123: direction_ == FORWARD ? -content_width RTL? https://codereview.chromium.org/895543005/diff/40001/content/browser/web_cont... File content/browser/web_contents/aura/overscroll_window_animation.h (right): https://codereview.chromium.org/895543005/diff/40001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_window_animation.h:42: virtual void OnOverscrollCompleted() = 0; consider passing the scoped ptr to the wrapper here https://codereview.chromium.org/895543005/diff/40001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_window_animation.h:92: bool animation_active_; nit: may be a bit ambiguous: active_ instead of animation_active_?
https://codereview.chromium.org/895543005/diff/60001/content/browser/web_cont... File content/browser/web_contents/aura/overscroll_navigation_overlay.cc (right): https://codereview.chromium.org/895543005/diff/60001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_navigation_overlay.cc:206: if (direction_ == OverscrollWindowAnimation::FORWARD) I think somewhere here we should have LTR/RTL logic? https://codereview.chromium.org/895543005/diff/60001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_navigation_overlay.cc:215: scoped_ptr<OverscrollLayerWrapper> OverscrollNavigationOverlay::CreateLayer() { Method needs to be renamed - we are creating a wrapper (around window or layer), not a layer https://codereview.chromium.org/895543005/diff/60001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_navigation_overlay.cc:222: SetupOverlayWindow(); SetupOverlayWindow() should not inititalize window_. At this point ONO doesn't own the overlay window - it is creating it for OWA, so it shouldn't set window_ to the overlay window, even intermittently for the duration of CreateLayer() - it just makes the state/ownership confusing. window_ should only be set to the overlay window in the "slide state". https://codereview.chromium.org/895543005/diff/60001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_navigation_overlay.cc:257: if (!observing_ && !loading_complete_) { This seems like the only place we use observing_. So if we add a separate callback for cancelled overscroll - we should be able to get rid of observing_. https://codereview.chromium.org/895543005/diff/60001/content/browser/web_cont... File content/browser/web_contents/aura/overscroll_navigation_overlay.h (right): https://codereview.chromium.org/895543005/diff/60001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_navigation_overlay.h:110: // The window shown on top of the live window for the animations. This comment is confusing: - live window is a concept within OWA, not ONO - window_ IS actually the live window for the slider case https://codereview.chromium.org/895543005/diff/60001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_navigation_overlay.h:117: bool observing_; I don't think we need observing_ - see my comment below. Also why remove received_paint_update? Generally, I think we should try to keep changes unrelated to "slider/overlay -> OWA consolidation" out of this CL unless there's a good reason. Just to reduce the number of moving parts. https://codereview.chromium.org/895543005/diff/60001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_navigation_overlay.h:125: OverscrollWindowAnimation* owa_; Consider if ONO should own a separate instance of OWA. set_live_window would be easier. https://codereview.chromium.org/895543005/diff/60001/content/browser/web_cont... File content/browser/web_contents/aura/overscroll_window_animation.cc (right): https://codereview.chromium.org/895543005/diff/60001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_window_animation.cc:32: live_window_(nullptr), Don't allow this or add null checks elsewhere (or DCHECK if method can't be called with null live_window) https://codereview.chromium.org/895543005/diff/60001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_window_animation.cc:33: delegate_(nullptr), Same thing here https://codereview.chromium.org/895543005/diff/60001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_window_animation.cc:79: void OverscrollWindowAnimation::OnImplicitAnimationsCompleted() { Failure vs Success https://codereview.chromium.org/895543005/diff/60001/content/browser/web_cont... File content/browser/web_contents/aura/overscroll_window_animation.h (right): https://codereview.chromium.org/895543005/diff/60001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_window_animation.h:32: enum Direction { FORWARD, BACKWARD, NONE }; In WindowSlider Direction was SLIDE_UNKNOWN, SLIDE_BACK, SLIDE_FRONT. Note that BACK/FRONT means BEHIND/IN FRONT, not BACKWARDS/FORWARD. I think we should either use the same semantics or perhaps LEFT/RIGHT. BACKWARDS/FORWARD doesn't really make sense here - it only makes sense in the context of browser history navigation, but OWA shouldn't be tied to that necessarily. https://codereview.chromium.org/895543005/diff/60001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_window_animation.h:47: virtual void OnOverscrollCompleted( Don't fire Completed() on failures - add another method for failure like WindowSlider.Delegate::XYZReset(). https://codereview.chromium.org/895543005/diff/60001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_window_animation.h:65: live_window_ = live_window; DCHECK(live_window) https://codereview.chromium.org/895543005/diff/60001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_window_animation.h:101: // Live window holding the currently active web contents. not web contents for slider case https://codereview.chromium.org/895543005/diff/60001/content/browser/web_cont... File content/browser/web_contents/web_contents_view_aura.cc (right): https://codereview.chromium.org/895543005/diff/60001/content/browser/web_cont... content/browser/web_contents/web_contents_view_aura.cc:1132: overscroll_window_animation_->set_live_window(GetContentNativeView()); Calling this every time the mode changes seems excessive
https://codereview.chromium.org/895543005/diff/60001/content/browser/web_cont... File content/browser/web_contents/aura/overscroll_navigation_overlay.cc (right): https://codereview.chromium.org/895543005/diff/60001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_navigation_overlay.cc:176: window_->layer()->SetLayerBrightness(-0.1f); It doesn't seem like we are animating the brightness anymore, so I think we can just get rid of this https://codereview.chromium.org/895543005/diff/60001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_navigation_overlay.cc:278: web_contents_->GetContentNativeView()->SetTransform(gfx::Transform()); These three instructions should only be done for the overlay case (layer_wrapper->has_window()), but not the slider case. Take a look at the old OnWindowSlideCompleted() implementation to see what should be done for the slider case https://codereview.chromium.org/895543005/diff/60001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_navigation_overlay.cc:314: void OverscrollNavigationOverlay::StopObservingIfDone() { Order of the methods should be the same as in .h file. It's also best not to change the order unless there's a good reason - makes it easier to see the diff when reviewing. https://codereview.chromium.org/895543005/diff/60001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_navigation_overlay.cc:324: owa_->set_live_window(web_contents_window_); It's pretty unfortunate that ONO has to do this - it should have no business setting up the relationship between web_contents_window_ and owa. Unfortunately I don't see a good way for WCVA to do this. Having two separate instances of OWA would solve this though. Is there any good reason not to do it? https://codereview.chromium.org/895543005/diff/60001/content/browser/web_cont... File content/browser/web_contents/aura/overscroll_window_animation.cc (right): https://codereview.chromium.org/895543005/diff/60001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_window_animation.cc:152: ui::Layer* OverscrollWindowAnimation::GetAnimatedLayer() { Perhaps call this GetFrontLayer() - this way we won't have to change this when both layers are animated. And it will also reinforce that it's always the front layer that is getting animated. https://codereview.chromium.org/895543005/diff/60001/content/browser/web_cont... File content/browser/web_contents/aura/overscroll_window_animation.h (right): https://codereview.chromium.org/895543005/diff/60001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_window_animation.h:46: // dismiss the overlay layer, or wait for the page to load first. We shouldn't reference page load in OWA's comments. OWA shouldn't know anything about page load and navigation. It is just moving windows/layers around. https://codereview.chromium.org/895543005/diff/60001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_window_animation.h:52: virtual void OnOverscrollCompleting() = 0; Completing() should be declared before Completed() IMO https://codereview.chromium.org/895543005/diff/60001/content/browser/web_cont... File content/browser/web_contents/web_contents_view_aura.cc (left): https://codereview.chromium.org/895543005/diff/60001/content/browser/web_cont... content/browser/web_contents/web_contents_view_aura.cc:1372: UMA_HISTOGRAM_ENUMERATION("Overscroll.Started", new_mode, OVERSCROLL_COUNT); Looks like we lost this histogram https://codereview.chromium.org/895543005/diff/60001/content/browser/web_cont... File content/browser/web_contents/web_contents_view_aura.cc (right): https://codereview.chromium.org/895543005/diff/60001/content/browser/web_cont... content/browser/web_contents/web_contents_view_aura.cc:732: if (!overscroll_window_animation_) { Can we do the same thing for overscroll_window_animation_ that we are doing for the overlay? E.g. if value == 0, it should be rest, etc. https://codereview.chromium.org/895543005/diff/60001/content/browser/web_cont... content/browser/web_contents/web_contents_view_aura.cc:964: navigation_overlay_.reset(); reset OWA here as well https://codereview.chromium.org/895543005/diff/60001/content/browser/web_cont... content/browser/web_contents/web_contents_view_aura.cc:968: if (!overscroll_window_animation_) { remove this if https://codereview.chromium.org/895543005/diff/60001/content/browser/web_cont... content/browser/web_contents/web_contents_view_aura.cc:972: navigation_overlay_.reset( Create OWA here as well https://codereview.chromium.org/895543005/diff/60001/content/browser/web_cont... content/browser/web_contents/web_contents_view_aura.cc:1103: // Only horizontal overscrolls participate in the navigation gesture. shouldn't this be inside else like it was before? If not - we should remove the comment https://codereview.chromium.org/895543005/diff/60001/content/browser/web_cont... content/browser/web_contents/web_contents_view_aura.cc:1104: if (!overscroll_window_animation_) Is this even possible? Previous flow ddidn't account for this case it seems
I updated the code to fix the comments. I also changed the architecture of the code a little to use two instances of OverscrollWindowAnimation as was suggested, to avoid having to set the live window all the time. However, I did have to pass the WebContents for the first overscroll case. See what you think of this implementation. I also found the bug with OWD not cancelling the overscroll gesture - I was trying to store a float in an int. Cheers! https://codereview.chromium.org/895543005/diff/40001/content/browser/web_cont... File content/browser/web_contents/aura/overscroll_layer_wrapper.h (right): https://codereview.chromium.org/895543005/diff/40001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_layer_wrapper.h:29: class OverscrollLayerWrapper { On 2015/02/27 21:33:42, mfomitchev wrote: > Perhaps we don't need this - perhaps OWA could always work with a layer and we > would only create a window in ONO when OWA's animation finishes Ack, for now we decided to keep the wrapper. https://codereview.chromium.org/895543005/diff/40001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_layer_wrapper.h:38: void WrapWindow(aura::Window* window); On 2015/02/27 21:33:42, mfomitchev wrote: > Just put these into constr Done. https://codereview.chromium.org/895543005/diff/40001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_layer_wrapper.h:55: ui::Layer* layer_; On 2015/02/27 21:33:42, mfomitchev wrote: > Perhaps make it own window/layer and have ONO/WCVA/etc own the wrapper I would love to, but by doing that we would lose the ability to wrap the web contents window. https://codereview.chromium.org/895543005/diff/40001/content/browser/web_cont... File content/browser/web_contents/aura/overscroll_window_animation.cc (right): https://codereview.chromium.org/895543005/diff/40001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_window_animation.cc:55: gfx::Vector2dF OverscrollWindowAnimation::GetTranslationForOverscroll( On 2015/02/27 21:33:42, mfomitchev wrote: > any reason this doesn't just return an int/float? None, changed the code. https://codereview.chromium.org/895543005/diff/40001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_window_animation.cc:60: return gfx::Vector2dF(std::max(-bounds_width, delta_x), 0); On 2015/02/27 21:33:42, mfomitchev wrote: > confirm if delta_x is always positive It isn't, and it shouldn't. However I do realize now that this code will most likely not work for RTL. https://codereview.chromium.org/895543005/diff/40001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_window_animation.cc:101: layer_wrapper_ = direction_ == FORWARD ? delegate_->CreateFrontLayer() On 2015/02/27 21:33:42, mfomitchev wrote: > Leak old layer if you change the direction from East to West? After some analysis it seems we don't. However, this code is bound to change from offline discussions. https://codereview.chromium.org/895543005/diff/40001/content/browser/web_cont... File content/browser/web_contents/aura/overscroll_window_animation.h (right): https://codereview.chromium.org/895543005/diff/40001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_window_animation.h:92: bool animation_active_; On 2015/02/27 21:33:42, mfomitchev wrote: > nit: may be a bit ambiguous: active_ instead of animation_active_? Done. https://codereview.chromium.org/895543005/diff/60001/content/browser/web_cont... File content/browser/web_contents/aura/overscroll_navigation_overlay.cc (right): https://codereview.chromium.org/895543005/diff/60001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_navigation_overlay.cc:176: window_->layer()->SetLayerBrightness(-0.1f); On 2015/03/06 01:36:43, mfomitchev wrote: > It doesn't seem like we are animating the brightness anymore, so I think we can > just get rid of this Done. https://codereview.chromium.org/895543005/diff/60001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_navigation_overlay.cc:206: if (direction_ == OverscrollWindowAnimation::FORWARD) On 2015/03/05 23:37:06, mfomitchev wrote: > I think somewhere here we should have LTR/RTL logic? FORWARD and BACKWARD refer to history direction, not screen direction, so RTL should have already been taken care of where this is set. Regardless of direction, if we're going FORWARD we want to stack above the existing layer, and vice versa. https://codereview.chromium.org/895543005/diff/60001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_navigation_overlay.cc:215: scoped_ptr<OverscrollLayerWrapper> OverscrollNavigationOverlay::CreateLayer() { On 2015/03/05 23:37:05, mfomitchev wrote: > Method needs to be renamed - we are creating a wrapper (around window or layer), > not a layer Agreed, had a TODO on the .h file. https://codereview.chromium.org/895543005/diff/60001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_navigation_overlay.cc:222: SetupOverlayWindow(); On 2015/03/05 23:37:06, mfomitchev wrote: > SetupOverlayWindow() should not inititalize window_. At this point ONO doesn't > own the overlay window - it is creating it for OWA, so it shouldn't set window_ > to the overlay window, even intermittently for the duration of CreateLayer() - > it just makes the state/ownership confusing. window_ should only be set to the > overlay window in the "slide state". Done. https://codereview.chromium.org/895543005/diff/60001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_navigation_overlay.cc:257: if (!observing_ && !loading_complete_) { On 2015/03/05 23:37:05, mfomitchev wrote: > This seems like the only place we use observing_. So if we add a separate > callback for cancelled overscroll - we should be able to get rid of observing_. Done. https://codereview.chromium.org/895543005/diff/60001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_navigation_overlay.cc:278: web_contents_->GetContentNativeView()->SetTransform(gfx::Transform()); On 2015/03/06 01:36:43, mfomitchev wrote: > These three instructions should only be done for the overlay case > (layer_wrapper->has_window()), but not the slider case. Done. > Take a look at the old > OnWindowSlideCompleted() implementation to see what should be done for the > slider case Agreed. This was not working at all. https://codereview.chromium.org/895543005/diff/60001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_navigation_overlay.cc:314: void OverscrollNavigationOverlay::StopObservingIfDone() { On 2015/03/06 01:36:43, mfomitchev wrote: > Order of the methods should be the same as in .h file. It's also best not to > change the order unless there's a good reason - makes it easier to see the diff > when reviewing. Done. I also rearranged some of the methods in a way that IMHO makes more sense. https://codereview.chromium.org/895543005/diff/60001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_navigation_overlay.cc:324: owa_->set_live_window(web_contents_window_); On 2015/03/06 01:36:43, mfomitchev wrote: > It's pretty unfortunate that ONO has to do this - it should have no business > setting up the relationship between web_contents_window_ and owa. Unfortunately > I don't see a good way for WCVA to do this. > Having two separate instances of OWA would solve this though. Yes > Is there any good reason not to do it? No. Done! https://codereview.chromium.org/895543005/diff/60001/content/browser/web_cont... File content/browser/web_contents/aura/overscroll_navigation_overlay.h (right): https://codereview.chromium.org/895543005/diff/60001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_navigation_overlay.h:110: // The window shown on top of the live window for the animations. On 2015/03/05 23:37:06, mfomitchev wrote: > This comment is confusing: > - live window is a concept within OWA, not ONO > - window_ IS actually the live window for the slider case Agreed, hopefully the new comment is better. https://codereview.chromium.org/895543005/diff/60001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_navigation_overlay.h:117: bool observing_; On 2015/03/05 23:37:06, mfomitchev wrote: > I don't think we need observing_ - see my comment below. Also why remove > received_paint_update? Generally, I think we should try to keep changes > unrelated to "slider/overlay -> OWA consolidation" out of this CL unless there's > a good reason. Just to reduce the number of moving parts. Brought back. The reason why it was removed is because |loading_complete_| was used in the exact same context, i.e. it was irrelevant. https://codereview.chromium.org/895543005/diff/60001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_navigation_overlay.h:125: OverscrollWindowAnimation* owa_; On 2015/03/05 23:37:06, mfomitchev wrote: > Consider if ONO should own a separate instance of OWA. set_live_window would be > easier. Yeah, this makes a lot of sense and will save many calls to set_live_window on WCVA. https://codereview.chromium.org/895543005/diff/60001/content/browser/web_cont... File content/browser/web_contents/aura/overscroll_window_animation.cc (right): https://codereview.chromium.org/895543005/diff/60001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_window_animation.cc:32: live_window_(nullptr), On 2015/03/05 23:37:06, mfomitchev wrote: > Don't allow this or add null checks elsewhere (or DCHECK if method can't be > called with null live_window) Done. https://codereview.chromium.org/895543005/diff/60001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_window_animation.cc:33: delegate_(nullptr), On 2015/03/05 23:37:06, mfomitchev wrote: > Same thing here Done. https://codereview.chromium.org/895543005/diff/60001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_window_animation.cc:79: void OverscrollWindowAnimation::OnImplicitAnimationsCompleted() { On 2015/03/05 23:37:06, mfomitchev wrote: > Failure vs Success Done. https://codereview.chromium.org/895543005/diff/60001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_window_animation.cc:152: ui::Layer* OverscrollWindowAnimation::GetAnimatedLayer() { On 2015/03/06 01:36:43, mfomitchev wrote: > Perhaps call this GetFrontLayer() - this way we won't have to change this when > both layers are animated. And it will also reinforce that it's always the front > layer that is getting animated. Done. https://codereview.chromium.org/895543005/diff/60001/content/browser/web_cont... File content/browser/web_contents/aura/overscroll_window_animation.h (right): https://codereview.chromium.org/895543005/diff/60001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_window_animation.h:32: enum Direction { FORWARD, BACKWARD, NONE }; On 2015/03/05 23:37:06, mfomitchev wrote: > In WindowSlider Direction was SLIDE_UNKNOWN, SLIDE_BACK, SLIDE_FRONT. Note that > BACK/FRONT means BEHIND/IN FRONT, not BACKWARDS/FORWARD. I think we should > either use the same semantics or perhaps LEFT/RIGHT. BACKWARDS/FORWARD doesn't > really make sense here - it only makes sense in the context of browser history > navigation, but OWA shouldn't be tied to that necessarily. In this case, FORWARD == FRONT and BACKWARD == BACK. I found using the FRONT and BACK, as in the where the layer is stacked, was not intuitive for ONO, and I especially wanted to avoid having two different enums for the direction. However, due to the fact that we know have two callbacks for Create*LayerWrapper, having two enums make more sense. See how you find the new implementation. https://codereview.chromium.org/895543005/diff/60001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_window_animation.h:46: // dismiss the overlay layer, or wait for the page to load first. On 2015/03/06 01:36:43, mfomitchev wrote: > We shouldn't reference page load in OWA's comments. OWA shouldn't know anything > about page load and navigation. It is just moving windows/layers around. Done. https://codereview.chromium.org/895543005/diff/60001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_window_animation.h:47: virtual void OnOverscrollCompleted( On 2015/03/05 23:37:06, mfomitchev wrote: > Don't fire Completed() on failures - add another method for failure like > WindowSlider.Delegate::XYZReset(). Done. https://codereview.chromium.org/895543005/diff/60001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_window_animation.h:52: virtual void OnOverscrollCompleting() = 0; On 2015/03/06 01:36:43, mfomitchev wrote: > Completing() should be declared before Completed() IMO Done. https://codereview.chromium.org/895543005/diff/60001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_window_animation.h:65: live_window_ = live_window; On 2015/03/05 23:37:06, mfomitchev wrote: > DCHECK(live_window) Done. https://codereview.chromium.org/895543005/diff/60001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_window_animation.h:101: // Live window holding the currently active web contents. On 2015/03/05 23:37:06, mfomitchev wrote: > not web contents for slider case Done. https://codereview.chromium.org/895543005/diff/60001/content/browser/web_cont... File content/browser/web_contents/web_contents_view_aura.cc (left): https://codereview.chromium.org/895543005/diff/60001/content/browser/web_cont... content/browser/web_contents/web_contents_view_aura.cc:1372: UMA_HISTOGRAM_ENUMERATION("Overscroll.Started", new_mode, OVERSCROLL_COUNT); On 2015/03/06 01:36:43, mfomitchev wrote: > Looks like we lost this histogram Brought back https://codereview.chromium.org/895543005/diff/60001/content/browser/web_cont... File content/browser/web_contents/web_contents_view_aura.cc (right): https://codereview.chromium.org/895543005/diff/60001/content/browser/web_cont... content/browser/web_contents/web_contents_view_aura.cc:732: if (!overscroll_window_animation_) { On 2015/03/06 01:36:43, mfomitchev wrote: > Can we do the same thing for overscroll_window_animation_ that we are doing for > the overlay? E.g. if value == 0, it should be rest, etc. Done. https://codereview.chromium.org/895543005/diff/60001/content/browser/web_cont... content/browser/web_contents/web_contents_view_aura.cc:964: navigation_overlay_.reset(); On 2015/03/06 01:36:43, mfomitchev wrote: > reset OWA here as well Done. https://codereview.chromium.org/895543005/diff/60001/content/browser/web_cont... content/browser/web_contents/web_contents_view_aura.cc:968: if (!overscroll_window_animation_) { On 2015/03/06 01:36:43, mfomitchev wrote: > remove this if Done. https://codereview.chromium.org/895543005/diff/60001/content/browser/web_cont... content/browser/web_contents/web_contents_view_aura.cc:972: navigation_overlay_.reset( On 2015/03/06 01:36:43, mfomitchev wrote: > Create OWA here as well I rewrote this. OWA is now created on overscroll update if it did not exist. https://codereview.chromium.org/895543005/diff/60001/content/browser/web_cont... content/browser/web_contents/web_contents_view_aura.cc:1103: // Only horizontal overscrolls participate in the navigation gesture. On 2015/03/06 01:36:43, mfomitchev wrote: > shouldn't this be inside else like it was before? If not - we should remove the > comment We have a return on the other if, no need for an else. https://codereview.chromium.org/895543005/diff/60001/content/browser/web_cont... content/browser/web_contents/web_contents_view_aura.cc:1104: if (!overscroll_window_animation_) On 2015/03/06 01:36:43, mfomitchev wrote: > Is this even possible? Previous flow ddidn't account for this case it seems TBH I don't know as I didn't look much at the code for the Simple GestureNav. Let's try removing it. https://codereview.chromium.org/895543005/diff/60001/content/browser/web_cont... content/browser/web_contents/web_contents_view_aura.cc:1132: overscroll_window_animation_->set_live_window(GetContentNativeView()); On 2015/03/05 23:37:06, mfomitchev wrote: > Calling this every time the mode changes seems excessive Yes, but it was necessary. I changed it so that we can use the web_contents which is not ideal but at least now we don't crash if the ContentNativeView changes.
https://codereview.chromium.org/895543005/diff/60001/content/browser/web_cont... File content/browser/web_contents/aura/overscroll_navigation_overlay.cc (right): https://codereview.chromium.org/895543005/diff/60001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_navigation_overlay.cc:206: if (direction_ == OverscrollWindowAnimation::FORWARD) Ok, cool https://codereview.chromium.org/895543005/diff/60001/content/browser/web_cont... File content/browser/web_contents/aura/overscroll_navigation_overlay.h (right): https://codereview.chromium.org/895543005/diff/60001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_navigation_overlay.h:117: bool observing_; On 2015/03/09 15:54:52, Nicolás wrote: > On 2015/03/05 23:37:06, mfomitchev wrote: > > I don't think we need observing_ - see my comment below. Also why remove > > received_paint_update? Generally, I think we should try to keep changes > > unrelated to "slider/overlay -> OWA consolidation" out of this CL unless > there's > > a good reason. Just to reduce the number of moving parts. > > Brought back. The reason why it was removed is because |loading_complete_| was > used in the exact same context, i.e. it was irrelevant. Ah, I see. https://codereview.chromium.org/895543005/diff/60001/content/browser/web_cont... File content/browser/web_contents/web_contents_view_aura.cc (right): https://codereview.chromium.org/895543005/diff/60001/content/browser/web_cont... content/browser/web_contents/web_contents_view_aura.cc:972: navigation_overlay_.reset( On 2015/03/09 15:54:53, Nicolás wrote: > On 2015/03/06 01:36:43, mfomitchev wrote: > > Create OWA here as well > > I rewrote this. OWA is now created on overscroll update if it did not exist. Hmm.. I don't really like that we are initializing everything in advance but we delay initializing OWA until an overscroll happens. I think it would be better if we were consistent. https://codereview.chromium.org/895543005/diff/60001/content/browser/web_cont... content/browser/web_contents/web_contents_view_aura.cc:1104: if (!overscroll_window_animation_) On 2015/03/09 15:54:53, Nicolás wrote: > On 2015/03/06 01:36:43, mfomitchev wrote: > > Is this even possible? Previous flow ddidn't account for this case it seems > > TBH I don't know as I didn't look much at the code for the Simple GestureNav. > Let's try removing it. I think overscroll events won't go to WCVA in the "simple gesture nav" case https://codereview.chromium.org/895543005/diff/80001/content/browser/web_cont... File content/browser/web_contents/aura/overscroll_navigation_overlay.cc (right): https://codereview.chromium.org/895543005/diff/80001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_navigation_overlay.cc:152: received_paint_update_ = false; overlay_dismiss_layer_.reset(); https://codereview.chromium.org/895543005/diff/80001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_navigation_overlay.cc:169: !(loading_complete_ || received_paint_update_) || Can you please copy the comments for loading_complete_, received_paint_update_ from the old implementation? https://codereview.chromium.org/895543005/diff/80001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_navigation_overlay.cc:189: if (direction_ == FORWARD) { It's not very logical IMO that stacking is done in CreateSlideLayer for the layer while it's done right here in CreateLayerWrapper for the window. We should be consistent about that. It might even make sense to get rid of CreateSlideLayer and CreateOverlayWindow altogether and just do everything in CreateLayerWrapper. https://codereview.chromium.org/895543005/diff/80001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_navigation_overlay.cc:212: owa_->set_overlay_window(window.get()); It would be better to do this in OnOverscrollCompleted() - this is where we transition from the overscroll state to the slider state, so I think this is where it would make sense to set up that up. https://codereview.chromium.org/895543005/diff/80001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_navigation_overlay.cc:237: return; I don't think you can get here if !window_ https://codereview.chromium.org/895543005/diff/80001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_navigation_overlay.cc:240: window_.reset(); We should probably also reset the overlay window on OWA. set_overlay_window(NULL)? It would probably also make sense to do set_overlay_window(NULL) and window_.reset() outside of FadeOutOverscrollWindow() - perhaps just after calling it. https://codereview.chromium.org/895543005/diff/80001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_navigation_overlay.cc:248: contents->layer()->SetLayerBrightness(0.f); Why are we fiddling with the brightness of web_contents_ layer here? If we get rid of this, we probably don't really need FadeOutOverscrollWindow()? https://codereview.chromium.org/895543005/diff/80001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_navigation_overlay.cc:250: (new OverlayDismissAnimator(dismiss_layer_.Pass()))->Animate(); keep comment https://codereview.chromium.org/895543005/diff/80001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_navigation_overlay.cc:272: return nullptr; Not sure if the convention is to return a scoped_ptr wrapping nullptr in these cases instead - can you check how it's done in similar cases? https://codereview.chromium.org/895543005/diff/80001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_navigation_overlay.cc:293: web_contents_->GetController().GoBack(); NOTREACHED() for direction == NONE https://codereview.chromium.org/895543005/diff/80001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_navigation_overlay.cc:300: if (layer_wrapper->has_window()) { I think checking if (!window_) would be better to determine the state. And then do DCHECK(layer_wrapper->has_window()) https://codereview.chromium.org/895543005/diff/80001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_navigation_overlay.cc:309: dismiss_layer_ = layer_wrapper->AcquireLayer(); Move this to after schedulepaint and add back the comment https://codereview.chromium.org/895543005/diff/80001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_navigation_overlay.cc:310: // If this overscroll was started too quickly, there is a chance we've Huh? We shouldn't be dismissing the window while OWA is active, and it just ceases to be active when it calls OnOverscrollCompleted, so it seems window_ should always be true here.. (if so - DCHECK) https://codereview.chromium.org/895543005/diff/80001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_navigation_overlay.cc:320: StopObservingIfDone(); We should be resetting direction_ to NONE when the animation completes. So here and in OnOverscrollAborted(). https://codereview.chromium.org/895543005/diff/80001/content/browser/web_cont... File content/browser/web_contents/aura/overscroll_navigation_overlay.h (right): https://codereview.chromium.org/895543005/diff/80001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_navigation_overlay.h:37: enum Direction { FORWARD, BACK, NONE }; Maybe call this NavigastionDirection, and the one in OWA - SlideDirection to reduce the risk of confusion https://codereview.chromium.org/895543005/diff/80001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_navigation_overlay.h:107: // delegate that processes overscroll events right after a successfull I don't think you need to talk about the delegate here - it's irrelevant in the context of ONO. I.e. we can change the way events are delivered to/processed in OWA, and ideally it shouldn't change ONO (including the comments in ONO) in any way. https://codereview.chromium.org/895543005/diff/80001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_navigation_overlay.h:123: // We own the OWA instance that handles layer animations for the case where an ..for the case where an overscroll gesture occurs while an overscroll overlay is displayed waiting for the page to load before it is dismissed. https://codereview.chromium.org/895543005/diff/80001/content/browser/web_cont... File content/browser/web_contents/aura/overscroll_window_animation.cc (right): https://codereview.chromium.org/895543005/diff/80001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_window_animation.cc:162: if (web_contents_) OWA shouldn't know about web_contents https://codereview.chromium.org/895543005/diff/80001/content/browser/web_cont... File content/browser/web_contents/web_contents_view_aura.cc (right): https://codereview.chromium.org/895543005/diff/80001/content/browser/web_cont... content/browser/web_contents/web_contents_view_aura.cc:725: navigation_overlay_.reset(); overscroll_window_animation_.reset()
Mikhail, please see the new patch. Cheers! https://codereview.chromium.org/895543005/diff/60001/content/browser/web_cont... File content/browser/web_contents/web_contents_view_aura.cc (right): https://codereview.chromium.org/895543005/diff/60001/content/browser/web_cont... content/browser/web_contents/web_contents_view_aura.cc:972: navigation_overlay_.reset( On 2015/03/10 19:25:59, mfomitchev wrote: > On 2015/03/09 15:54:53, Nicolás wrote: > > On 2015/03/06 01:36:43, mfomitchev wrote: > > > Create OWA here as well > > > > I rewrote this. OWA is now created on overscroll update if it did not exist. > > Hmm.. I don't really like that we are initializing everything in advance but we > delay initializing OWA until an overscroll happens. I think it would be better > if we were consistent. Done. https://codereview.chromium.org/895543005/diff/60001/content/browser/web_cont... content/browser/web_contents/web_contents_view_aura.cc:1104: if (!overscroll_window_animation_) On 2015/03/10 19:25:59, mfomitchev wrote: > On 2015/03/09 15:54:53, Nicolás wrote: > > On 2015/03/06 01:36:43, mfomitchev wrote: > > > Is this even possible? Previous flow ddidn't account for this case it seems > > > > TBH I don't know as I didn't look much at the code for the Simple GestureNav. > > Let's try removing it. > > I think overscroll events won't go to WCVA in the "simple gesture nav" case Acknowledged. https://codereview.chromium.org/895543005/diff/80001/content/browser/web_cont... File content/browser/web_contents/aura/overscroll_navigation_overlay.cc (right): https://codereview.chromium.org/895543005/diff/80001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_navigation_overlay.cc:152: received_paint_update_ = false; On 2015/03/10 19:25:59, mfomitchev wrote: > overlay_dismiss_layer_.reset(); Done. https://codereview.chromium.org/895543005/diff/80001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_navigation_overlay.cc:169: !(loading_complete_ || received_paint_update_) || On 2015/03/10 19:25:59, mfomitchev wrote: > Can you please copy the comments for loading_complete_, received_paint_update_ > from the old implementation? Done. https://codereview.chromium.org/895543005/diff/80001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_navigation_overlay.cc:189: if (direction_ == FORWARD) { On 2015/03/10 19:25:59, mfomitchev wrote: > It's not very logical IMO that stacking is done in CreateSlideLayer for the > layer while it's done right here in CreateLayerWrapper for the window. We should > be consistent about that. > > It might even make sense to get rid of CreateSlideLayer and CreateOverlayWindow > altogether and just do everything in CreateLayerWrapper. Done. https://codereview.chromium.org/895543005/diff/80001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_navigation_overlay.cc:212: owa_->set_overlay_window(window.get()); On 2015/03/10 19:25:59, mfomitchev wrote: > It would be better to do this in OnOverscrollCompleted() - this is where we > transition from the overscroll state to the slider state, so I think this is > where it would make sense to set up that up. That function was deleted so now we handle it differently. https://codereview.chromium.org/895543005/diff/80001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_navigation_overlay.cc:237: return; On 2015/03/10 19:25:59, mfomitchev wrote: > I don't think you can get here if !window_ Right! https://codereview.chromium.org/895543005/diff/80001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_navigation_overlay.cc:240: window_.reset(); On 2015/03/10 19:25:59, mfomitchev wrote: > We should probably also reset the overlay window on OWA. > set_overlay_window(NULL)? It would probably also make sense to do > set_overlay_window(NULL) and window_.reset() outside of > FadeOutOverscrollWindow() - perhaps just after calling it. Done. https://codereview.chromium.org/895543005/diff/80001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_navigation_overlay.cc:248: contents->layer()->SetLayerBrightness(0.f); On 2015/03/10 19:25:59, mfomitchev wrote: > Why are we fiddling with the brightness of web_contents_ layer here? If we get > rid of this, we probably don't really need FadeOutOverscrollWindow()? Got rid of this. https://codereview.chromium.org/895543005/diff/80001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_navigation_overlay.cc:250: (new OverlayDismissAnimator(dismiss_layer_.Pass()))->Animate(); On 2015/03/10 19:25:59, mfomitchev wrote: > keep comment Done. https://codereview.chromium.org/895543005/diff/80001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_navigation_overlay.cc:272: return nullptr; On 2015/03/10 19:25:59, mfomitchev wrote: > Not sure if the convention is to return a scoped_ptr wrapping nullptr in these > cases instead - can you check how it's done in similar cases? I don't know what the convention usually is, but I think in general if the constructor is not annotated by the explicit keyword, you can use the implicit version. Also, we are still returning a scoped_ptr. Adding explicit to the scoped_ptr constructor and trying to compile yielded some results where this idiom is used: https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/discar... https://code.google.com/p/chromium/codesearch#chromium/src/ipc/mojo/ipc_chann... https://code.google.com/p/chromium/codesearch#chromium/src/net/disk_cache/mem... https://code.google.com/p/chromium/codesearch#chromium/src/ui/gfx/geometry/r_... https://code.google.com/p/chromium/codesearch#chromium/src/net/base/sdch_mana... https://codereview.chromium.org/895543005/diff/80001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_navigation_overlay.cc:293: web_contents_->GetController().GoBack(); On 2015/03/10 19:25:59, mfomitchev wrote: > NOTREACHED() for direction == NONE Done. https://codereview.chromium.org/895543005/diff/80001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_navigation_overlay.cc:300: if (layer_wrapper->has_window()) { On 2015/03/10 19:26:00, mfomitchev wrote: > I think checking if (!window_) would be better to determine the state. And then > do DCHECK(layer_wrapper->has_window()) Done, but I removed has_window() https://codereview.chromium.org/895543005/diff/80001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_navigation_overlay.cc:309: dismiss_layer_ = layer_wrapper->AcquireLayer(); On 2015/03/10 19:25:59, mfomitchev wrote: > Move this to after schedulepaint and add back the comment Done. https://codereview.chromium.org/895543005/diff/80001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_navigation_overlay.cc:310: // If this overscroll was started too quickly, there is a chance we've On 2015/03/10 19:25:59, mfomitchev wrote: > Huh? We shouldn't be dismissing the window while OWA is active, and it just > ceases to be active when it calls OnOverscrollCompleted, so it seems window_ > should always be true here.. (if so - DCHECK) Removed. https://codereview.chromium.org/895543005/diff/80001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_navigation_overlay.cc:320: StopObservingIfDone(); On 2015/03/10 19:25:59, mfomitchev wrote: > We should be resetting direction_ to NONE when the animation completes. So here > and in OnOverscrollAborted(). Done. https://codereview.chromium.org/895543005/diff/80001/content/browser/web_cont... File content/browser/web_contents/aura/overscroll_navigation_overlay.h (right): https://codereview.chromium.org/895543005/diff/80001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_navigation_overlay.h:37: enum Direction { FORWARD, BACK, NONE }; On 2015/03/10 19:26:00, mfomitchev wrote: > Maybe call this NavigastionDirection, and the one in OWA - SlideDirection to > reduce the risk of confusion Done. https://codereview.chromium.org/895543005/diff/80001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_navigation_overlay.h:107: // delegate that processes overscroll events right after a successfull On 2015/03/10 19:26:00, mfomitchev wrote: > I don't think you need to talk about the delegate here - it's irrelevant in the > context of ONO. I.e. we can change the way events are delivered to/processed in > OWA, and ideally it shouldn't change ONO (including the comments in ONO) in any > way. Done. https://codereview.chromium.org/895543005/diff/80001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_navigation_overlay.h:123: // We own the OWA instance that handles layer animations for the case where an On 2015/03/10 19:26:00, mfomitchev wrote: > ..for the case where an overscroll gesture occurs while an overscroll overlay is > displayed waiting for the page to load before it is dismissed. Changed the comment. https://codereview.chromium.org/895543005/diff/80001/content/browser/web_cont... File content/browser/web_contents/aura/overscroll_window_animation.cc (right): https://codereview.chromium.org/895543005/diff/80001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_window_animation.cc:162: if (web_contents_) On 2015/03/10 19:26:00, mfomitchev wrote: > OWA shouldn't know about web_contents Modified so it doesn't have to. https://codereview.chromium.org/895543005/diff/80001/content/browser/web_cont... File content/browser/web_contents/web_contents_view_aura.cc (right): https://codereview.chromium.org/895543005/diff/80001/content/browser/web_cont... content/browser/web_contents/web_contents_view_aura.cc:725: navigation_overlay_.reset(); On 2015/03/10 19:26:00, mfomitchev wrote: > overscroll_window_animation_.reset() Not needed anymore.
This looks great! My comments are pretty minor. I think we can add Sadrul after you address the remaining comments unless new major issues come up. https://codereview.chromium.org/895543005/diff/80001/content/browser/web_cont... File content/browser/web_contents/aura/overscroll_navigation_overlay.h (right): https://codereview.chromium.org/895543005/diff/80001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_navigation_overlay.h:134: scoped_ptr<ui::Layer> dismiss_layer_; We should confirm if this is not needed before removing it. The question we need to ask is if we have layer1, change it's contents, schedule a repaint, and then remove layer2 which is on top of layer1 - are we guaranteed that we won't see layer1 with the old contents. https://codereview.chromium.org/895543005/diff/120001/content/browser/web_con... File content/browser/web_contents/aura/overscroll_navigation_overlay.cc (right): https://codereview.chromium.org/895543005/diff/120001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay.cc:185: if (window_) { Add some comments for the two cases https://codereview.chromium.org/895543005/diff/120001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay.cc:217: return scoped_ptr<SlidableWrapper>( So we don't need ONO to own the overlay window (window_) from the beginning of the animation anymore? I remember we discussed it, but not sure if we still need it with a single OWA. https://codereview.chromium.org/895543005/diff/120001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay.cc:260: if (direction_ == FORWARD && web_contents_->GetController().CanGoForward()) What happens if we do GoForward()/Back without checking CanGoFOrmard/Back? https://codereview.chromium.org/895543005/diff/120001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay.cc:270: window_ = wrapper->AcquireWindow(); DCHECK wrapper->has_window() https://codereview.chromium.org/895543005/diff/120001/content/browser/web_con... File content/browser/web_contents/aura/overscroll_window_animation.cc (right): https://codereview.chromium.org/895543005/diff/120001/content/browser/web_con... content/browser/web_contents/aura/overscroll_window_animation.cc:92: // TODO(nsatragno): in this case, show a feedback animation. I think we can get rid of this TODO? https://codereview.chromium.org/895543005/diff/120001/content/browser/web_con... content/browser/web_contents/aura/overscroll_window_animation.cc:98: GetFrontLayer()->GetAnimator()->AbortAllAnimations(); So I think we need to do StopAnimating() instead of Abort.. We need the animations to be completed, not cancelled. https://codereview.chromium.org/895543005/diff/120001/content/browser/web_con... content/browser/web_contents/aura/overscroll_window_animation.cc:108: // Make sure the live window is in its default position. Don't you also need to abort all animations on it? https://codereview.chromium.org/895543005/diff/120001/content/browser/web_con... File content/browser/web_contents/aura/overscroll_window_animation.h (right): https://codereview.chromium.org/895543005/diff/120001/content/browser/web_con... content/browser/web_contents/aura/overscroll_window_animation.h:48: virtual aura::Window* GetLiveWindow() const = 0; We should really find another name for this. This window is not really "live" in the slider case. Perhaps we could call it GetTopWindow or GetDisplayedWindow. I am not in love with either of those names, but I think they would be more descriptive/accurate. https://codereview.chromium.org/895543005/diff/120001/content/browser/web_con... File content/browser/web_contents/web_contents_view_aura.cc (right): https://codereview.chromium.org/895543005/diff/120001/content/browser/web_con... content/browser/web_contents/web_contents_view_aura.cc:741: overscroll_window_animation_ = navigation_overlay_->owa(); I think we need to handle the possibility of overscroll_window_animation_ being null (e.g. because overscroll was disabled)
nsatragno@chromium.org changed reviewers: + sadrul@chromium.org
nsatragno@chromium.org changed required reviewers: + sadrul@chromium.org
+sadrul@ Fixed the nits, brought back the dismiss layer, which is resetted on the creation of a new layer to avoid having it hide those. Sadrul, can you give this patch a review? Tests are on the way. Thanks! https://codereview.chromium.org/895543005/diff/80001/content/browser/web_cont... File content/browser/web_contents/aura/overscroll_navigation_overlay.h (right): https://codereview.chromium.org/895543005/diff/80001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_navigation_overlay.h:134: scoped_ptr<ui::Layer> dismiss_layer_; On 2015/03/16 22:28:01, mfomitchev wrote: > We should confirm if this is not needed before removing it. The question we need > to ask is if we have layer1, change it's contents, schedule a repaint, and then > remove layer2 which is on top of layer1 - are we guaranteed that we won't see > layer1 with the old contents. Ah, I saw the flickering we discussed today. I'll try to make it work by resetting the dismiss layer when a new layer is created. https://codereview.chromium.org/895543005/diff/120001/content/browser/web_con... File content/browser/web_contents/aura/overscroll_navigation_overlay.cc (right): https://codereview.chromium.org/895543005/diff/120001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay.cc:185: if (window_) { On 2015/03/16 22:28:01, mfomitchev wrote: > Add some comments for the two cases Done. https://codereview.chromium.org/895543005/diff/120001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay.cc:217: return scoped_ptr<SlidableWrapper>( On 2015/03/16 22:28:01, mfomitchev wrote: > So we don't need ONO to own the overlay window (window_) from the beginning of > the animation anymore? I remember we discussed it, but not sure if we still need > it with a single OWA. Right, we don't need that anymore with this design. https://codereview.chromium.org/895543005/diff/120001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay.cc:260: if (direction_ == FORWARD && web_contents_->GetController().CanGoForward()) On 2015/03/16 22:28:01, mfomitchev wrote: > What happens if we do GoForward()/Back without checking CanGoFOrmard/Back? BOOM. There's a DCHECK in place to prevent that from happening, I guess in release we would have undefined behaviour by trying to access an out of bounds index in the history array. Added comment. https://codereview.chromium.org/895543005/diff/120001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay.cc:270: window_ = wrapper->AcquireWindow(); On 2015/03/16 22:28:01, mfomitchev wrote: > DCHECK wrapper->has_window() Done. https://codereview.chromium.org/895543005/diff/120001/content/browser/web_con... File content/browser/web_contents/aura/overscroll_window_animation.cc (right): https://codereview.chromium.org/895543005/diff/120001/content/browser/web_con... content/browser/web_contents/aura/overscroll_window_animation.cc:92: // TODO(nsatragno): in this case, show a feedback animation. On 2015/03/16 22:28:02, mfomitchev wrote: > I think we can get rid of this TODO? Done. https://codereview.chromium.org/895543005/diff/120001/content/browser/web_con... content/browser/web_contents/aura/overscroll_window_animation.cc:98: GetFrontLayer()->GetAnimator()->AbortAllAnimations(); On 2015/03/16 22:28:02, mfomitchev wrote: > So I think we need to do StopAnimating() instead of Abort.. We need the > animations to be completed, not cancelled. Done. https://codereview.chromium.org/895543005/diff/120001/content/browser/web_con... content/browser/web_contents/aura/overscroll_window_animation.cc:108: // Make sure the live window is in its default position. On 2015/03/16 22:28:02, mfomitchev wrote: > Don't you also need to abort all animations on it? Ah, I guess yes - if something else transforms the window we might be in trouble if we don't. https://codereview.chromium.org/895543005/diff/120001/content/browser/web_con... File content/browser/web_contents/aura/overscroll_window_animation.h (right): https://codereview.chromium.org/895543005/diff/120001/content/browser/web_con... content/browser/web_contents/aura/overscroll_window_animation.h:48: virtual aura::Window* GetLiveWindow() const = 0; On 2015/03/16 22:28:02, mfomitchev wrote: > We should really find another name for this. This window is not really "live" in > the slider case. Perhaps we could call it GetTopWindow or GetDisplayedWindow. I > am not in love with either of those names, but I think they would be more > descriptive/accurate. GetTargetWindow? As the window is the one being overscrolled... https://codereview.chromium.org/895543005/diff/120001/content/browser/web_con... File content/browser/web_contents/web_contents_view_aura.cc (right): https://codereview.chromium.org/895543005/diff/120001/content/browser/web_con... content/browser/web_contents/web_contents_view_aura.cc:741: overscroll_window_animation_ = navigation_overlay_->owa(); On 2015/03/16 22:28:02, mfomitchev wrote: > I think we need to handle the possibility of overscroll_window_animation_ being > null (e.g. because overscroll was disabled) If the overscroll is disabled, navigation_overlay_ would be null and *this would't be handling the overscroll events, so I don't think we need to check for now.
danakj@chromium.org changed reviewers: + danakj@chromium.org
https://codereview.chromium.org/895543005/diff/160001/content/browser/web_con... File content/browser/web_contents/aura/overscroll_navigation_overlay.cc (right): https://codereview.chromium.org/895543005/diff/160001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay.cc:18: #include "ui/aura_extra/image_window_delegate.h" Unused? Is the ImageWindowDelegate used anywhere else? I was just trying to understand what it's used for and this was the only use I see, which Sadrul pointed me to. Can we remove the ImageWindowDelegate entirely?
https://codereview.chromium.org/895543005/diff/160001/content/browser/web_con... File content/browser/web_contents/aura/overscroll_navigation_overlay.cc (right): https://codereview.chromium.org/895543005/diff/160001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay.cc:18: #include "ui/aura_extra/image_window_delegate.h" On 2015/03/17 21:42:53, danakj wrote: > Unused? Is the ImageWindowDelegate used anywhere else? I was just trying to > understand what it's used for and this was the only use I see, which Sadrul > pointed me to. Can we remove the ImageWindowDelegate entirely? We are inheriting from it in OverscrollWindowDelegate (see content/browser/web_contents/aura/overscroll_window_delegate.h in this patch). We were using it in this file before, and I forgot to remove the include. Do you think we should keep it in case someone else needs it or just pull down its code to OverscrollWindowDelegate, effectively removing it?
On 2015/03/17 21:50:47, Nicolás wrote: > https://codereview.chromium.org/895543005/diff/160001/content/browser/web_con... > File content/browser/web_contents/aura/overscroll_navigation_overlay.cc (right): > > https://codereview.chromium.org/895543005/diff/160001/content/browser/web_con... > content/browser/web_contents/aura/overscroll_navigation_overlay.cc:18: #include > "ui/aura_extra/image_window_delegate.h" > On 2015/03/17 21:42:53, danakj wrote: > > Unused? Is the ImageWindowDelegate used anywhere else? I was just trying to > > understand what it's used for and this was the only use I see, which Sadrul > > pointed me to. Can we remove the ImageWindowDelegate entirely? > > We are inheriting from it in OverscrollWindowDelegate (see > content/browser/web_contents/aura/overscroll_window_delegate.h in this patch). Ah! Okay. > We were using it in this file before, and I forgot to remove the include. Do you > think we should keep it in case someone else needs it or just pull down its code Drop the include here if nothing is using that type in this file. IncludeWhatYouUse. > to OverscrollWindowDelegate, effectively removing it? As an outsider without much familiarity (listen to sadrul instead of me for anything), I would like us to have an ImageWindowDelegate OR an ImageLayerDelegate, not both. They are doing the same thing-ish, so I hope there's no reason to have both. WRT picking one, the former seems like the better level to be implementing stuff... but I feel like I don't have the full picture in my head.
On 2015/03/17 21:58:45, danakj wrote: > On 2015/03/17 21:50:47, Nicolás wrote: > > > https://codereview.chromium.org/895543005/diff/160001/content/browser/web_con... > > File content/browser/web_contents/aura/overscroll_navigation_overlay.cc > (right): > > > > > https://codereview.chromium.org/895543005/diff/160001/content/browser/web_con... > > content/browser/web_contents/aura/overscroll_navigation_overlay.cc:18: > #include > > "ui/aura_extra/image_window_delegate.h" > > On 2015/03/17 21:42:53, danakj wrote: > > > Unused? Is the ImageWindowDelegate used anywhere else? I was just trying to > > > understand what it's used for and this was the only use I see, which Sadrul > > > pointed me to. Can we remove the ImageWindowDelegate entirely? > > > > We are inheriting from it in OverscrollWindowDelegate (see > > content/browser/web_contents/aura/overscroll_window_delegate.h in this patch). > > Ah! Okay. > > > We were using it in this file before, and I forgot to remove the include. Do > you > > think we should keep it in case someone else needs it or just pull down its > code > > Drop the include here if nothing is using that type in this file. > IncludeWhatYouUse. > Done. > > to OverscrollWindowDelegate, effectively removing it? > > As an outsider without much familiarity (listen to sadrul instead of me for > anything), I would like us to have an ImageWindowDelegate OR an > ImageLayerDelegate, not both. They are doing the same thing-ish, so I hope > there's no reason to have both. > For this implementation we need a custom delegate installed in the overlay window that handles some UI events and shows a picture. We also show layers with pictures, thus the need for ImageLayerDelegate. If we can somehow also use the ImageLayerDelegate to paint the image shown on the window, having OverscrollWindowDelegate only managing the events, I'd be more than happy to remove ImageWindowDelegate. > WRT picking one, the former seems like the better level to be implementing > stuff... but I feel like I don't have the full picture in my head.
On Tue, Mar 17, 2015 at 3:15 PM, <nsatragno@chromium.org> wrote: > On 2015/03/17 21:58:45, danakj wrote: > >> On 2015/03/17 21:50:47, Nicolás wrote: >> > >> > > https://codereview.chromium.org/895543005/diff/160001/ > content/browser/web_contents/aura/overscroll_navigation_overlay.cc > >> > File content/browser/web_contents/aura/overscroll_navigation_overlay.cc >> (right): >> > >> > >> > > https://codereview.chromium.org/895543005/diff/160001/ > content/browser/web_contents/aura/overscroll_navigation_ > overlay.cc#newcode18 > >> > content/browser/web_contents/aura/overscroll_navigation_overlay.cc:18: >> #include >> > "ui/aura_extra/image_window_delegate.h" >> > On 2015/03/17 21:42:53, danakj wrote: >> > > Unused? Is the ImageWindowDelegate used anywhere else? I was just >> trying >> > to > >> > > understand what it's used for and this was the only use I see, which >> > Sadrul > >> > > pointed me to. Can we remove the ImageWindowDelegate entirely? >> > >> > We are inheriting from it in OverscrollWindowDelegate (see >> > content/browser/web_contents/aura/overscroll_window_delegate.h in this >> > patch). > > Ah! Okay. >> > > > We were using it in this file before, and I forgot to remove the >> include. Do >> you >> > think we should keep it in case someone else needs it or just pull down >> its >> code >> > > Drop the include here if nothing is using that type in this file. >> IncludeWhatYouUse. >> > > > Done. > > > to OverscrollWindowDelegate, effectively removing it? >> > > As an outsider without much familiarity (listen to sadrul instead of me >> for >> anything), I would like us to have an ImageWindowDelegate OR an >> ImageLayerDelegate, not both. They are doing the same thing-ish, so I hope >> there's no reason to have both. >> > > > For this implementation we need a custom delegate installed in the overlay > window that handles some UI events and shows a picture. We also show layers > with pictures, thus the need for ImageLayerDelegate. If we can somehow also > use the ImageLayerDelegate to paint the image shown on the window, having > OverscrollWindowDelegate only managing the events, I'd be more than happy > to remove ImageWindowDelegate. Alternatively, maybe we could have aura::Windows instead of using ui::Layers directly, and that way the ImageWindowDelegate would do that painting instead of an ImageLayerDelegate. Is there anything you can think of that would be a problem for that? > > > WRT picking one, the former seems like the better level to be implementing >> stuff... but I feel like I don't have the full picture in my head. >> > > > https://codereview.chromium.org/895543005/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/03/17 22:17:06, danakj wrote: > On Tue, Mar 17, 2015 at 3:15 PM, <mailto:nsatragno@chromium.org> wrote: > > > On 2015/03/17 21:58:45, danakj wrote: > > > >> On 2015/03/17 21:50:47, Nicolás wrote: > >> > > >> > > > > https://codereview.chromium.org/895543005/diff/160001/ > > content/browser/web_contents/aura/overscroll_navigation_overlay.cc > > > >> > File content/browser/web_contents/aura/overscroll_navigation_overlay.cc > >> (right): > >> > > >> > > >> > > > > https://codereview.chromium.org/895543005/diff/160001/ > > content/browser/web_contents/aura/overscroll_navigation_ > > overlay.cc#newcode18 > > > >> > content/browser/web_contents/aura/overscroll_navigation_overlay.cc:18: > >> #include > >> > "ui/aura_extra/image_window_delegate.h" > >> > On 2015/03/17 21:42:53, danakj wrote: > >> > > Unused? Is the ImageWindowDelegate used anywhere else? I was just > >> trying > >> > > to > > > >> > > understand what it's used for and this was the only use I see, which > >> > > Sadrul > > > >> > > pointed me to. Can we remove the ImageWindowDelegate entirely? > >> > > >> > We are inheriting from it in OverscrollWindowDelegate (see > >> > content/browser/web_contents/aura/overscroll_window_delegate.h in this > >> > > patch). > > > > Ah! Okay. > >> > > > > > We were using it in this file before, and I forgot to remove the > >> include. Do > >> you > >> > think we should keep it in case someone else needs it or just pull down > >> its > >> code > >> > > > > Drop the include here if nothing is using that type in this file. > >> IncludeWhatYouUse. > >> > > > > > > Done. > > > > > to OverscrollWindowDelegate, effectively removing it? > >> > > > > As an outsider without much familiarity (listen to sadrul instead of me > >> for > >> anything), I would like us to have an ImageWindowDelegate OR an > >> ImageLayerDelegate, not both. They are doing the same thing-ish, so I hope > >> there's no reason to have both. > >> > > > > > > For this implementation we need a custom delegate installed in the overlay > > window that handles some UI events and shows a picture. We also show layers > > with pictures, thus the need for ImageLayerDelegate. If we can somehow also > > use the ImageLayerDelegate to paint the image shown on the window, having > > OverscrollWindowDelegate only managing the events, I'd be more than happy > > to remove ImageWindowDelegate. > > > Alternatively, maybe we could have aura::Windows instead of using > ui::Layers directly, and that way the ImageWindowDelegate would do that > painting instead of an ImageLayerDelegate. Is there anything you can think > of that would be a problem for that? Yeah, using Window directly if possible would be better. I believe the WIP implementation of the new touch-selection-controller also uses the IWD.
mfomitchev@, please give the ONO tests an initial review. sadrul@, danakj@, I'll start working on making the code use windows instead of layers. It seems that would simplify the logic substantially. Do you think the rest of the code looks good? Thanks! https://codereview.chromium.org/895543005/diff/200001/content/browser/web_con... File content/browser/web_contents/aura/overscroll_navigation_overlay.cc (right): https://codereview.chromium.org/895543005/diff/200001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay.cc:180: window_.reset(); This was causing use-after-free, ASAN got it.
Lo and behold, layers are no more! This patchset removes layers from the overscroll navigation and substitutes them with gluten-free windows. We don't change the screenshot in the overlay window anymore, instead we set window_ in ONO to the latest window that has been created. This way, we don't need to keep track of the dismiss window or have two separate slide cases in ONO. Last but not least, the dreaded ImageLayerDelegate and SlidableWrapper have been vanquished from the lands of Chrome. Naturally, this patch needs a new review for ONO and OWA. Cheers!
:D Amazing :) Should we remove those layer delegates then? If they aren't quite unused yet then nvm. I'll let sadrul do the reviewering :) On Wed, Mar 18, 2015 at 2:58 PM, <nsatragno@chromium.org> wrote: > Lo and behold, layers are no more! This patchset removes layers from the > overscroll navigation and substitutes them with gluten-free windows. We > don't > change the screenshot in the overlay window anymore, instead we set > window_ in > ONO to the latest window that has been created. This way, we don't need to > keep > track of the dismiss window or have two separate slide cases in ONO. Last > but > not least, the dreaded ImageLayerDelegate and SlidableWrapper have been > vanquished from the lands of Chrome. > > Naturally, this patch needs a new review for ONO and OWA. > > Cheers! > > https://codereview.chromium.org/895543005/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/895543005/diff/220001/content/browser/web_con... File content/browser/web_contents/aura/overscroll_navigation_overlay.cc (right): https://codereview.chromium.org/895543005/diff/220001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay.cc:209: window_ = window.Pass(); I think this can mess up the event targeting, since this destroys the old window. Try starting a new slide when there is a completing slide animation in progress. I'd expect the scroll gesture to get targeted to ONO's window_. The begin of the overscroll will call StopAnimating(), which will trigger OnOverscrollCompleted(), which will desroy the old ONO's window here. But I think the scroll updates will still be targeted to that destroyed window... So at best that scroll will be ignored. At worst we will have something more unpleasant
https://codereview.chromium.org/895543005/diff/220001/content/browser/web_con... File content/browser/web_contents/aura/overscroll_navigation_overlay.cc (right): https://codereview.chromium.org/895543005/diff/220001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay.cc:209: window_ = window.Pass(); On 2015/03/19 03:00:15, mfomitchev wrote: > I think this can mess up the event targeting, since this destroys the old > window. > Try starting a new slide when there is a completing slide animation in progress. It seems to work well. > I'd expect the scroll gesture to get targeted to ONO's window_. The begin of the > overscroll will call StopAnimating(), which will trigger > OnOverscrollCompleted(), which will desroy the old ONO's window here. But I > think the scroll updates will still be targeted to that destroyed window... IMHO this would indicate a failure in the targeting mechanism, so stuff like close buttons or gestures to close would never work properly if they destroy the window that is hosting them. > So > at best that scroll will be ignored. At worst we will have something more > unpleasant
I fixed a bug with the trackpad that made it impossible to overscroll past the overlay window by setting the capture on the window that is currently handling the events. I also added OWA tests. OWD tests are on the way.
General comment: A lot of method names, variable names, and comments are out of date or confusing. I think the readability of the code could be improved a lot if we spend some time on this. I made some specific comments, but I didn't comment on everything. Can you please read through the code, especially in OWA and ONO and make sure all the names and comments are good and accurate? Try to approach this as an "outsider" - would the names and comments make sense to you and help your understanding if you were looking at this code for the first time and didn't have prior context? We've been using a lot of jargon, like "slider window", "live window" but this won't make much sense to other people who look at the code. https://codereview.chromium.org/895543005/diff/280001/content/browser/web_con... File content/browser/web_contents/aura/overscroll_navigation_overlay.cc (right): https://codereview.chromium.org/895543005/diff/280001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay.cc:93: window_(nullptr), no need to init scoped_ptr to null https://codereview.chromium.org/895543005/diff/280001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay.cc:121: // If a slide is in progress, then do not destroy the window or the slide. "slide" is not clear https://codereview.chromium.org/895543005/diff/280001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay.cc:122: if (!window_ || !(loading_complete_ || received_paint_update_) || Do need both !window_ and owa_->is_active()? https://codereview.chromium.org/895543005/diff/280001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay.cc:145: aura::Window* target_window = GetTargetWindow(); event_window? top_window? owner_window? https://codereview.chromium.org/895543005/diff/280001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay.cc:146: if (target_window) { Let's find a way to get rid of this if as we discussed. For now maybe add a TODO. https://codereview.chromium.org/895543005/diff/280001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay.cc:151: target_window->SetCapture(); A short comment here would be nice https://codereview.chromium.org/895543005/diff/280001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay.cc:201: if (direction_ == FORWARD && web_contents_->GetController().CanGoForward()) We should probably change this logic, since it looks like we won't be able to fix the screenshot issue in time (is there a crbug for that btw?) Lets do that, add a TODO, and reference the crbug in a TODO. https://codereview.chromium.org/895543005/diff/280001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay.cc:215: web_contents_->GetContentNativeView()->SetTransform(gfx::Transform()); Can we avoid doing this for the slider case? Perhaps we could just do that on the GetTargetWindow() (before we assign window.Pass to window_ though, otherwise it won't work) https://codereview.chromium.org/895543005/diff/280001/content/browser/web_con... File content/browser/web_contents/aura/overscroll_navigation_overlay.h (right): https://codereview.chromium.org/895543005/diff/280001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay.h:64: // Creates the overscroll window if it does not exist or a slide window if it It's not clear from this comment what is the "overscroll window" or "slide window" and these concepts are not established anywhere. Maybe just say it's created above or below the current top-level window (web_contents or overlay) depending on the navigation direction. https://codereview.chromium.org/895543005/diff/280001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay.h:68: // Returns the screenshot for the given direction. "screenshot for direction" is not clear enough https://codereview.chromium.org/895543005/diff/280001/content/browser/web_con... File content/browser/web_contents/aura/overscroll_window_animation.h (right): https://codereview.chromium.org/895543005/diff/280001/content/browser/web_con... content/browser/web_contents/aura/overscroll_window_animation.h:47: virtual aura::Window* GetTargetWindow() const = 0; "Target window" doesn't really explain what this window is. Nor does the comment really. Try to explain it from the POV of OWA. Perhaps GetMainWindow() or GetOwnerWindow(). The role of this window and window_ can be establishedin the comments to the OWA class itself if that makes sense
https://codereview.chromium.org/895543005/diff/280001/content/browser/web_con... File content/browser/web_contents/aura/overscroll_navigation_overlay_unittest.cc (right): https://codereview.chromium.org/895543005/diff/280001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay_unittest.cc:149: EXPECT_NE(GetOverlay()->direction_, OverscrollNavigationOverlay::NONE); Why don't we test that the direction is BACK instead of testing it's not NONE? Applies to other tests as well. https://codereview.chromium.org/895543005/diff/280001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay_unittest.cc:153: EXPECT_FALSE(contents()->cross_navigation_pending()); Also confirm direction is NONE? https://codereview.chromium.org/895543005/diff/280001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay_unittest.cc:156: // Tests that if a second navigation is aborted before, the first one still "a second navigation is aborted before" is not clear https://codereview.chromium.org/895543005/diff/280001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay_unittest.cc:210: PerformBackNavigationViaSliderCallbacks(); Do you think we still need the multinavigation tests? It was a different flow for the second navigation before since the slider callbacks were only used for the second navigation. Now it's the same callbacks, so I am not sure these tests are still useful. https://codereview.chromium.org/895543005/diff/280001/content/browser/web_con... File content/browser/web_contents/aura/overscroll_window_animation.cc (right): https://codereview.chromium.org/895543005/diff/280001/content/browser/web_con... content/browser/web_contents/aura/overscroll_window_animation.cc:31: : window_(nullptr), no need to init scoped ptr to null https://codereview.chromium.org/895543005/diff/280001/content/browser/web_con... content/browser/web_contents/aura/overscroll_window_animation.cc:110: delegate_->GetTargetWindow()->layer()->GetAnimator()->AbortAllAnimations(); Is this a left over from the time when we had two OWAs? Also, shouldn't we be calling StopAnimation here as well instead of Abort? Also, if we need to stop/abort, we should probably do this before setting the transform/bounds. https://codereview.chromium.org/895543005/diff/280001/content/browser/web_con... File content/browser/web_contents/aura/overscroll_window_animation.h (right): https://codereview.chromium.org/895543005/diff/280001/content/browser/web_con... content/browser/web_contents/aura/overscroll_window_animation.h:27: // Manages the animations during an overscroll navigation by handling overscroll We probably shouldn't be talking about "navigation" in the context of this class. This class is all about animations in response to overscroll events. It doesn't have to know or care that these animations are happening as the visual candy associated with navigations. More generally, when designing a class, we should try to establish as "broad" role for it as possible. This way we minimize the change that when outside components change we will have to change the class. Thus when we write comments (or name class members) we should also look at it from the view of the "broad" role of the class. It's tempting to comment in terms of the specific use case that the class is being used for, but generally speaking that should be avoided. https://codereview.chromium.org/895543005/diff/280001/content/browser/web_con... File content/browser/web_contents/aura/overscroll_window_animation_unittest.cc (right): https://codereview.chromium.org/895543005/diff/280001/content/browser/web_con... content/browser/web_contents/aura/overscroll_window_animation_unittest.cc:5: #include "content/browser/web_contents/aura/overscroll_window_animation.h" I think we need a test for overscroll_window_delegate as well. A lot of the things tested in old WS tests should probably be tested there, li8ke interop of gestures and key events or mouse events. https://codereview.chromium.org/895543005/diff/280001/content/browser/web_con... content/browser/web_contents/aura/overscroll_window_animation_unittest.cc:7: #include "testing/gtest/include/gtest/gtest.h" The old window slider tests have three tests which we don't seem to be replicating: OwnerWindowChangesDuringWindowSlide, OwnerIsDestroyedOnSliderDestroy, OwnerIsDestroyedOnSlideComplete. I am not sure if all of them are needed: OWA doesn't really have the "owner" window, since it is operating a window, not a layer. However it would be good to understand what specific conditions were those tests designed for. @sadrul, perhaps you can comment on this? https://codereview.chromium.org/895543005/diff/280001/content/browser/web_con... content/browser/web_contents/aura/overscroll_window_animation_unittest.cc:120: TEST_F(OverscrollWindowAnimationTest, BasicOverscroll) { what about north/south overscroll? https://codereview.chromium.org/895543005/diff/280001/content/browser/web_con... content/browser/web_contents/aura/overscroll_window_animation_unittest.cc:157: // Tests starting an overscroll gesture when navigation is impossible. "when navigation is impossible" -> "when xyz window can't be created". https://codereview.chromium.org/895543005/diff/280001/content/browser/web_con... File content/browser/web_contents/web_contents_view_aura.h (right): https://codereview.chromium.org/895543005/diff/280001/content/browser/web_con... content/browser/web_contents/web_contents_view_aura.h:200: // Receives overscroll events and manages the overscroll animation. Owned by navigation_overlay_
https://codereview.chromium.org/895543005/diff/300001/content/browser/web_con... File content/browser/web_contents/aura/overscroll_navigation_overlay_unittest.cc (right): https://codereview.chromium.org/895543005/diff/300001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay_unittest.cc:29: class OverscrollTestWebContents : public TestWebContents { Why not just add a SetContentsNativeView method to TestWebContents? https://codereview.chromium.org/895543005/diff/300001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay_unittest.cc:183: GetOverlay()->window_->delegate())->has_image()); Verify contents()->GetURL()? https://codereview.chromium.org/895543005/diff/300001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay_unittest.cc:192: GetOverlay()->window_->delegate())->has_image()); Verify contents()->GetURL()? https://codereview.chromium.org/895543005/diff/300001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay_unittest.cc:198: EXPECT_EQ(GetOverlay()->CreateFrontWindow(), nullptr); Verify contents()->GetURL()? https://codereview.chromium.org/895543005/diff/300001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay_unittest.cc:209: EXPECT_FALSE(contents()->cross_navigation_pending()); Verify contents()->GetURL()? https://codereview.chromium.org/895543005/diff/300001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay_unittest.cc:339: // cancel the first one. The first event that cancels the animation will go to So this comment is incorrect, right? The events go to the new window, not the old overlay window from the beginning.. https://codereview.chromium.org/895543005/diff/300001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay_unittest.cc:343: gfx::Point(10, 10), Can we make scroll gesture begin on the old overlay window? When we pass 10, I think it is positioned on the new window.. https://codereview.chromium.org/895543005/diff/300001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay_unittest.cc:359: EXPECT_EQ(contents()->GetURL(), first_); We start with fourth_ and do two back gestures, shouldn't we be at second_ now? https://codereview.chromium.org/895543005/diff/300001/content/browser/web_con... File content/browser/web_contents/aura/overscroll_window_delegate_unittest.cc (right): https://codereview.chromium.org/895543005/diff/300001/content/browser/web_con... content/browser/web_contents/aura/overscroll_window_delegate_unittest.cc:117: gfx::Point(20 + touch_complete_threshold(), 10), 10 + touch_complete_threshold() + 1? https://codereview.chromium.org/895543005/diff/300001/content/browser/web_con... content/browser/web_contents/aura/overscroll_window_delegate_unittest.cc:121: EXPECT_EQ(current_mode(), OVERSCROLL_EAST); SO by the end of the gesture I'd think the mode would be changed back to NONE, no? Perhaps we need to check the mode mid-gesture? https://codereview.chromium.org/895543005/diff/300001/content/browser/web_con... content/browser/web_contents/aura/overscroll_window_delegate_unittest.cc:171: gfx::Point(10 + touch_start_threshold() + 10, 10), 10 + touch_start_threshold() + 1?
@mfomitchev, please see the the updated patch. Thanks! https://codereview.chromium.org/895543005/diff/280001/content/browser/web_con... File content/browser/web_contents/aura/overscroll_navigation_overlay.cc (right): https://codereview.chromium.org/895543005/diff/280001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay.cc:93: window_(nullptr), On 2015/03/26 02:41:38, mfomitchev wrote: > no need to init scoped_ptr to null Removed. https://codereview.chromium.org/895543005/diff/280001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay.cc:121: // If a slide is in progress, then do not destroy the window or the slide. On 2015/03/26 02:41:39, mfomitchev wrote: > "slide" is not clear Done. https://codereview.chromium.org/895543005/diff/280001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay.cc:122: if (!window_ || !(loading_complete_ || received_paint_update_) || On 2015/03/26 02:41:38, mfomitchev wrote: > Do need both !window_ and owa_->is_active()? Yes. For the first overscroll case, window_ == nullptr && owa_->is_active() after OnOverscrollCompleting() but before OnOverscrollCompleted() => we want to return early (no need to ) If the first overscroll is aborted, window_ == nullptr && !owa_->is_active() => we want to return early. If the first overscroll completes successfully and another one is started before the page loads, window_ != nullptr && owa_->is_active => return early. For all the successful overscroll cases where we want to dismiss the overlay, we hold a reference to the window_ and owa is not active. https://codereview.chromium.org/895543005/diff/280001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay.cc:145: aura::Window* target_window = GetTargetWindow(); On 2015/03/26 02:41:39, mfomitchev wrote: > event_window? top_window? owner_window? Event window sgtm https://codereview.chromium.org/895543005/diff/280001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay.cc:146: if (target_window) { On 2015/03/26 02:41:38, mfomitchev wrote: > Let's find a way to get rid of this if as we discussed. For now maybe add a > TODO. Found and done https://codereview.chromium.org/895543005/diff/280001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay.cc:151: target_window->SetCapture(); On 2015/03/26 02:41:39, mfomitchev wrote: > A short comment here would be nice Done. https://codereview.chromium.org/895543005/diff/280001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay.cc:201: if (direction_ == FORWARD && web_contents_->GetController().CanGoForward()) On 2015/03/26 02:41:38, mfomitchev wrote: > We should probably change this logic, since it looks like we won't be able to > fix the screenshot issue in time (is there a crbug for that btw?) Lets do that, > add a TODO, and reference the crbug in a TODO. Looks like we were able to fix it by avoiding clipping. I'll double check with vollick@ though. https://codereview.chromium.org/895543005/diff/280001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay.cc:215: web_contents_->GetContentNativeView()->SetTransform(gfx::Transform()); On 2015/03/26 02:41:39, mfomitchev wrote: > Can we avoid doing this for the slider case? Perhaps we could just do that on > the GetTargetWindow() (before we assign window.Pass to window_ though, otherwise > it won't work) Done. https://codereview.chromium.org/895543005/diff/280001/content/browser/web_con... File content/browser/web_contents/aura/overscroll_navigation_overlay.h (right): https://codereview.chromium.org/895543005/diff/280001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay.h:64: // Creates the overscroll window if it does not exist or a slide window if it On 2015/03/26 02:41:39, mfomitchev wrote: > It's not clear from this comment what is the "overscroll window" or "slide > window" and these concepts are not established anywhere. Maybe just say it's > created above or below the current top-level window (web_contents or overlay) > depending on the navigation direction. Done. https://codereview.chromium.org/895543005/diff/280001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay.h:68: // Returns the screenshot for the given direction. On 2015/03/26 02:41:39, mfomitchev wrote: > "screenshot for direction" is not clear enough Done. https://codereview.chromium.org/895543005/diff/280001/content/browser/web_con... File content/browser/web_contents/aura/overscroll_navigation_overlay_unittest.cc (right): https://codereview.chromium.org/895543005/diff/280001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay_unittest.cc:149: EXPECT_NE(GetOverlay()->direction_, OverscrollNavigationOverlay::NONE); On 2015/03/26 15:36:00, mfomitchev wrote: > Why don't we test that the direction is BACK instead of testing it's not NONE? > Applies to other tests as well. Done. https://codereview.chromium.org/895543005/diff/280001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay_unittest.cc:153: EXPECT_FALSE(contents()->cross_navigation_pending()); On 2015/03/26 15:36:00, mfomitchev wrote: > Also confirm direction is NONE? Done. https://codereview.chromium.org/895543005/diff/280001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay_unittest.cc:156: // Tests that if a second navigation is aborted before, the first one still On 2015/03/26 15:36:00, mfomitchev wrote: > "a second navigation is aborted before" is not clear Done. https://codereview.chromium.org/895543005/diff/280001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay_unittest.cc:210: PerformBackNavigationViaSliderCallbacks(); On 2015/03/26 15:36:00, mfomitchev wrote: > Do you think we still need the multinavigation tests? It was a different flow > for the second navigation before since the slider callbacks were only used for > the second navigation. Now it's the same callbacks, so I am not sure these tests > are still useful. Yeah... it does not make that much sense to have these. The codepath is the same. Removed. https://codereview.chromium.org/895543005/diff/280001/content/browser/web_con... File content/browser/web_contents/aura/overscroll_window_animation.cc (right): https://codereview.chromium.org/895543005/diff/280001/content/browser/web_con... content/browser/web_contents/aura/overscroll_window_animation.cc:31: : window_(nullptr), On 2015/03/26 15:36:00, mfomitchev wrote: > no need to init scoped ptr to null Done. https://codereview.chromium.org/895543005/diff/280001/content/browser/web_con... content/browser/web_contents/aura/overscroll_window_animation.cc:110: delegate_->GetTargetWindow()->layer()->GetAnimator()->AbortAllAnimations(); On 2015/03/26 15:36:00, mfomitchev wrote: > Is this a left over from the time when we had two OWAs? Also, shouldn't we be > calling StopAnimation here as well instead of Abort? Also, if we need to > stop/abort, we should probably do this before setting the transform/bounds. We still need to make sure the bounds are OK, but there was no need to abort the animations. https://codereview.chromium.org/895543005/diff/280001/content/browser/web_con... File content/browser/web_contents/aura/overscroll_window_animation.h (right): https://codereview.chromium.org/895543005/diff/280001/content/browser/web_con... content/browser/web_contents/aura/overscroll_window_animation.h:27: // Manages the animations during an overscroll navigation by handling overscroll On 2015/03/26 15:36:00, mfomitchev wrote: > We probably shouldn't be talking about "navigation" in the context of this > class. This class is all about animations in response to overscroll events. It > doesn't have to know or care that these animations are happening as the visual > candy associated with navigations. > More generally, when designing a class, we should try to establish as "broad" > role for it as possible. This way we minimize the change that when outside > components change we will have to change the class. Thus when we write comments > (or name class members) we should also look at it from the view of the "broad" > role of the class. It's tempting to comment in terms of the specific use case > that the class is being used for, but generally speaking that should be avoided. Changed the comment completely. Check it out. https://codereview.chromium.org/895543005/diff/280001/content/browser/web_con... content/browser/web_contents/aura/overscroll_window_animation.h:47: virtual aura::Window* GetTargetWindow() const = 0; On 2015/03/26 02:41:39, mfomitchev wrote: > "Target window" doesn't really explain what this window is. Nor does the comment > really. Try to explain it from the POV of OWA. Perhaps GetMainWindow() or > GetOwnerWindow(). The role of this window and window_ can be establishedin the > comments to the OWA class itself if that makes sense Changed to GetMainWindow. https://codereview.chromium.org/895543005/diff/280001/content/browser/web_con... File content/browser/web_contents/aura/overscroll_window_animation_unittest.cc (right): https://codereview.chromium.org/895543005/diff/280001/content/browser/web_con... content/browser/web_contents/aura/overscroll_window_animation_unittest.cc:5: #include "content/browser/web_contents/aura/overscroll_window_animation.h" On 2015/03/26 15:36:00, mfomitchev wrote: > I think we need a test for overscroll_window_delegate as well. A lot of the > things tested in old WS tests should probably be tested there, li8ke interop of > gestures and key events or mouse events. Done. https://codereview.chromium.org/895543005/diff/280001/content/browser/web_con... content/browser/web_contents/aura/overscroll_window_animation_unittest.cc:7: #include "testing/gtest/include/gtest/gtest.h" On 2015/03/26 15:36:01, mfomitchev wrote: > The old window slider tests have three tests which we don't seem to be > replicating: OwnerWindowChangesDuringWindowSlide, This should not be a problem IMHO, if the contents window changes in ONO, then we'll just get a new one. > OwnerIsDestroyedOnSliderDestroy, OwnerIsDestroyedOnSlideComplete. These are more interesting, I don't know what would happen here. > > I am not sure if all of them are needed: OWA doesn't really have the "owner" > window, since it is operating a window, not a layer. However it would be good to > understand what specific conditions were those tests designed for. @sadrul, > perhaps you can comment on this? Waiting for sadrul@ input. https://codereview.chromium.org/895543005/diff/280001/content/browser/web_con... content/browser/web_contents/aura/overscroll_window_animation_unittest.cc:120: TEST_F(OverscrollWindowAnimationTest, BasicOverscroll) { On 2015/03/26 15:36:00, mfomitchev wrote: > what about north/south overscroll? We don't support it in OWD as we didn't support it on WindowSlider. I think it doesn't make sense to support them for this case until we find a reason to have them, or when we merge OWD and OC. https://codereview.chromium.org/895543005/diff/280001/content/browser/web_con... content/browser/web_contents/aura/overscroll_window_animation_unittest.cc:157: // Tests starting an overscroll gesture when navigation is impossible. On 2015/03/26 15:36:00, mfomitchev wrote: > "when navigation is impossible" -> "when xyz window can't be created". Done. https://codereview.chromium.org/895543005/diff/280001/content/browser/web_con... File content/browser/web_contents/web_contents_view_aura.h (right): https://codereview.chromium.org/895543005/diff/280001/content/browser/web_con... content/browser/web_contents/web_contents_view_aura.h:200: // Receives overscroll events and manages the overscroll animation. On 2015/03/26 15:36:01, mfomitchev wrote: > Owned by navigation_overlay_ Done. https://codereview.chromium.org/895543005/diff/300001/content/browser/web_con... File content/browser/web_contents/aura/overscroll_navigation_overlay_unittest.cc (right): https://codereview.chromium.org/895543005/diff/300001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay_unittest.cc:29: class OverscrollTestWebContents : public TestWebContents { On 2015/03/27 00:14:05, mfomitchev wrote: > Why not just add a SetContentsNativeView method to TestWebContents? This was a good suggestion, but I had to add another modification to it, and I would rather have them isolated for this test only. https://codereview.chromium.org/895543005/diff/300001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay_unittest.cc:183: GetOverlay()->window_->delegate())->has_image()); On 2015/03/27 00:14:05, mfomitchev wrote: > Verify contents()->GetURL()? I don't think we need to re-test this (we do it later), as the point of the test is to make sure ONO works correctly with / without screenshot. https://codereview.chromium.org/895543005/diff/300001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay_unittest.cc:192: GetOverlay()->window_->delegate())->has_image()); On 2015/03/27 00:14:05, mfomitchev wrote: > Verify contents()->GetURL()? Ditto https://codereview.chromium.org/895543005/diff/300001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay_unittest.cc:198: EXPECT_EQ(GetOverlay()->CreateFrontWindow(), nullptr); On 2015/03/27 00:14:05, mfomitchev wrote: > Verify contents()->GetURL()? Ditto. And in this case we are not navigating. https://codereview.chromium.org/895543005/diff/300001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay_unittest.cc:209: EXPECT_FALSE(contents()->cross_navigation_pending()); On 2015/03/27 00:14:05, mfomitchev wrote: > Verify contents()->GetURL()? I really don't think we need to test this - it means we have to commit the navigation in the test side, when cross_navigation_pending() should do the trick. https://codereview.chromium.org/895543005/diff/300001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay_unittest.cc:339: // cancel the first one. The first event that cancels the animation will go to On 2015/03/27 00:14:05, mfomitchev wrote: > So this comment is incorrect, right? The events go to the new window, not the > old overlay window from the beginning.. Right, fixed comment. https://codereview.chromium.org/895543005/diff/300001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay_unittest.cc:343: gfx::Point(10, 10), On 2015/03/27 00:14:05, mfomitchev wrote: > Can we make scroll gesture begin on the old overlay window? When we pass 10, I > think it is positioned on the new window.. I noticed this, and modified it. https://codereview.chromium.org/895543005/diff/300001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay_unittest.cc:359: EXPECT_EQ(contents()->GetURL(), first_); On 2015/03/27 00:14:05, mfomitchev wrote: > We start with fourth_ and do two back gestures, shouldn't we be at second_ now? We did three gestures, L307, L323 and L342. https://codereview.chromium.org/895543005/diff/300001/content/browser/web_con... File content/browser/web_contents/aura/overscroll_window_animation.cc (right): https://codereview.chromium.org/895543005/diff/300001/content/browser/web_con... content/browser/web_contents/aura/overscroll_window_animation.cc:79: delegate_->OnOverscrollAborted(); I made sure that aborting the overscroll during the second overscroll case allows starting a new gesture if the page hasn't loaded yet. https://codereview.chromium.org/895543005/diff/300001/content/browser/web_con... File content/browser/web_contents/aura/overscroll_window_delegate_unittest.cc (right): https://codereview.chromium.org/895543005/diff/300001/content/browser/web_con... content/browser/web_contents/aura/overscroll_window_delegate_unittest.cc:117: gfx::Point(20 + touch_complete_threshold(), 10), On 2015/03/27 00:14:05, mfomitchev wrote: > 10 + touch_complete_threshold() + 1? By using 10, we add 1 per motion, which I thought was clearer. I also replaced "10 + 10" with "20", it didn't add much to readability and made the line longer. https://codereview.chromium.org/895543005/diff/300001/content/browser/web_con... content/browser/web_contents/aura/overscroll_window_delegate_unittest.cc:121: EXPECT_EQ(current_mode(), OVERSCROLL_EAST); On 2015/03/27 00:14:05, mfomitchev wrote: > SO by the end of the gesture I'd think the mode would be changed back to NONE, > no? > Perhaps we need to check the mode mid-gesture? Yes, the mode in OWD is changed to none. However, it is the delegate responsibility to change its mode to NONE when the overscroll completes (and OnOverscrollComplete is fired). This is why we check for OVERSCROLL_EAST and overscroll_completed here. I'll add another test to verify the internal OverscrollMode status. https://codereview.chromium.org/895543005/diff/300001/content/browser/web_con... content/browser/web_contents/aura/overscroll_window_delegate_unittest.cc:171: gfx::Point(10 + touch_start_threshold() + 10, 10), On 2015/03/27 00:14:05, mfomitchev wrote: > 10 + touch_start_threshold() + 1? Same comment as above, I think it's cleaner (especially for debugging purposes) to have a +1 per step.
https://codereview.chromium.org/895543005/diff/340001/content/browser/web_con... File content/browser/web_contents/aura/overscroll_navigation_overlay.cc (right): https://codereview.chromium.org/895543005/diff/340001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay.cc:126: web_contents_->GetNativeView()->layer()->SetMasksToBounds(true); What if it was already false when we called OnOverscrollCompleting()? Also, I wonder how bad would it be if ONO is destroyed between OnOverscrollCompleting and StopObservingIfDone. A good way to do this would be use a pattern similar to RAII here: create an class which sets MasksToBounds to false in its constructor and saves the old values. In the destructor set them back to the old values if and only if the old values were "true". Then add a member var to ONO which would be a scoped_ptr to the instance of that class. Init it in OnOverscrollCompleting(), reset it in StopObservingIfDone(). The advantage is that if ONO is destroyed for whatever reason while this is active - the flags would be reset. https://codereview.chromium.org/895543005/diff/340001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay.cc:196: GetMainWindow()->ReleaseCapture(); Using RAII-like pattern for this is probably an overkill, but perhaps call GetMainWindow()->ReleaseCapture() in the destructor if OWA is active? https://codereview.chromium.org/895543005/diff/340001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay.cc:202: // their parents coordinates. Avoid clipping of the screenshot caused by the contents window being moved outside of the screen bounds. https://codereview.chromium.org/895543005/diff/340001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay.cc:219: // Reset the position of the main window. |main_window| will be invalid No need to say "|main_window| will be invalid after this block." https://codereview.chromium.org/895543005/diff/340001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay.cc:223: main_window->SetBounds(gfx::Rect(web_contents_window_->bounds().size())); Why do we need to set the bounds here? We don't ever change the bounds, do we? Perhaps this should be just GetMainWindow()->SetTransform(gfx::Transform())? https://codereview.chromium.org/895543005/diff/340001/content/browser/web_con... File content/browser/web_contents/aura/overscroll_navigation_overlay.h (right): https://codereview.chromium.org/895543005/diff/340001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay.h:61: // an overscroll slide isn't in progress and either the page has been painted I don't think "overscroll slide" hasn't been established as a term in this class. Either explain what it is or just say "if there's no active overscroll window animation". https://codereview.chromium.org/895543005/diff/340001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay.h:66: // the current overscroll |direction_|. Transfers ownership to the caller. NO need to talk about the ownership transfer. scoped_ptr makes the ownership clear. https://codereview.chromium.org/895543005/diff/340001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay.h:93: scoped_ptr<aura::Window> window_; Now this is actually a bit misleading since this is null in the "first case" and is only set to the overlay window in the "second case". To explain this field we need to explain these two cases somewhere in this .h. Perhaps right here or in the class comments. https://codereview.chromium.org/895543005/diff/340001/content/browser/web_con... File content/browser/web_contents/aura/overscroll_window_animation.cc (right): https://codereview.chromium.org/895543005/diff/340001/content/browser/web_con... content/browser/web_contents/aura/overscroll_window_animation.cc:77: delegate_->OnOverscrollAborted(); Set overscroll_cancelled_ to false here? https://codereview.chromium.org/895543005/diff/340001/content/browser/web_con... File content/browser/web_contents/aura/overscroll_window_animation.h (right): https://codereview.chromium.org/895543005/diff/340001/content/browser/web_con... content/browser/web_contents/aura/overscroll_window_animation.h:36: // stays behind while the slide window one moves in from the right. SLIDE_BACK stays still, while the slide window moves on top of it, entering from the right. https://codereview.chromium.org/895543005/diff/340001/content/browser/web_con... content/browser/web_contents/aura/overscroll_window_animation.h:38: // window on the back. SLIDE_NONE means we are not animating yet. Left and in the back, which stays still. https://codereview.chromium.org/895543005/diff/340001/content/browser/web_con... content/browser/web_contents/aura/overscroll_window_animation.h:39: // right are reversed for users of RTL languages, but stack order remains Directions are reversed for RTL languages,.. https://codereview.chromium.org/895543005/diff/340001/content/browser/web_con... content/browser/web_contents/aura/overscroll_window_animation.h:62: // ownership is transferred. "The slide window's ownership is transferred" -> "|window| points to the slide window". Ownership is implicit from scoped_ptr. https://codereview.chromium.org/895543005/diff/340001/content/browser/web_con... content/browser/web_contents/aura/overscroll_window_animation.h:84: void OnOverscrollModeChange(OverscrollMode old_mode, Leave no space between overridden methods https://codereview.chromium.org/895543005/diff/340001/content/browser/web_con... content/browser/web_contents/aura/overscroll_window_animation.h:117: // Indicates if the current animation has been cancelled. Pick either "aborted" or "cancelled and stick to that within this class. Right now you use both. Also - this is only true while the "abort" animation is progress? If so - say that.
https://codereview.chromium.org/895543005/diff/300001/content/browser/web_con... File content/browser/web_contents/aura/overscroll_navigation_overlay_unittest.cc (right): https://codereview.chromium.org/895543005/diff/300001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay_unittest.cc:29: class OverscrollTestWebContents : public TestWebContents { What is the reason we need to set the "fake" native view? Is it something unique to our tests or an issue with the default behavior that could cause problems to other tests as well? If the latter - why not also add SetNativeView to TestWebContents? https://codereview.chromium.org/895543005/diff/300001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay_unittest.cc:183: GetOverlay()->window_->delegate())->has_image()); On 2015/03/27 17:52:36, Nicolás wrote: > On 2015/03/27 00:14:05, mfomitchev wrote: > > Verify contents()->GetURL()? > > I don't think we need to re-test this (we do it later), as the point of the test > is to make sure ONO works correctly with / without screenshot. Ok, well, maybe put the check for cross_navigation_pending() after Completing() is called into PerformBackNavigationViaSliderCallbacks(). This is the first, most basic test, and requesting navigation in response to the callback is part of ONO's most basic functionality. So it seems like that should be tested here. https://codereview.chromium.org/895543005/diff/300001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay_unittest.cc:192: GetOverlay()->window_->delegate())->has_image()); On 2015/03/27 17:52:36, Nicolás wrote: > On 2015/03/27 00:14:05, mfomitchev wrote: > > Verify contents()->GetURL()? > > Ditto same as above https://codereview.chromium.org/895543005/diff/300001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay_unittest.cc:209: EXPECT_FALSE(contents()->cross_navigation_pending()); On 2015/03/27 17:52:36, Nicolás wrote: > On 2015/03/27 00:14:05, mfomitchev wrote: > > Verify contents()->GetURL()? > > I really don't think we need to test this - it means we have to commit the > navigation in the test side, when cross_navigation_pending() should do the > trick. Acknowledged. https://codereview.chromium.org/895543005/diff/300001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay_unittest.cc:359: EXPECT_EQ(contents()->GetURL(), first_); On 2015/03/27 17:52:36, Nicolás wrote: > On 2015/03/27 00:14:05, mfomitchev wrote: > > We start with fourth_ and do two back gestures, shouldn't we be at second_ > now? > > We did three gestures, L307, L323 and L342. Acknowledged. https://codereview.chromium.org/895543005/diff/300001/content/browser/web_con... File content/browser/web_contents/aura/overscroll_window_delegate_unittest.cc (right): https://codereview.chromium.org/895543005/diff/300001/content/browser/web_con... content/browser/web_contents/aura/overscroll_window_delegate_unittest.cc:117: gfx::Point(20 + touch_complete_threshold(), 10), On 2015/03/27 17:52:37, Nicolás wrote: > On 2015/03/27 00:14:05, mfomitchev wrote: > > 10 + touch_complete_threshold() + 1? > > By using 10, we add 1 per motion, which I thought was clearer. I also replaced > "10 + 10" with "20", it didn't add much to readability and made the line longer. We want to ensure that if we are over the threshold - the overscroll is started. The number of steps in the gesture is completely irrelevant. It could be 1 or 1000, I don 't think that should impact the behavior in any way. https://codereview.chromium.org/895543005/diff/300001/content/browser/web_con... content/browser/web_contents/aura/overscroll_window_delegate_unittest.cc:121: EXPECT_EQ(current_mode(), OVERSCROLL_EAST); On 2015/03/27 17:52:37, Nicolás wrote: > On 2015/03/27 00:14:05, mfomitchev wrote: > > SO by the end of the gesture I'd think the mode would be changed back to NONE, > > no? > > Perhaps we need to check the mode mid-gesture? > > Yes, the mode in OWD is changed to none. However, it is the delegate > responsibility to change its mode to NONE when the overscroll completes (and > OnOverscrollComplete is fired). This is why we check for OVERSCROLL_EAST and > overscroll_completed here. I'll add another test to verify the internal > OverscrollMode status. Acknowledged. https://codereview.chromium.org/895543005/diff/300001/content/browser/web_con... content/browser/web_contents/aura/overscroll_window_delegate_unittest.cc:171: gfx::Point(10 + touch_start_threshold() + 10, 10), ditto https://codereview.chromium.org/895543005/diff/340001/content/browser/web_con... File content/browser/web_contents/aura/overscroll_navigation_overlay_unittest.cc (right): https://codereview.chromium.org/895543005/diff/340001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay_unittest.cc:215: EXPECT_EQ(GetOverlay()->direction_, OverscrollNavigationOverlay::BACK); Might actually want to add this check to PerformBackNavigationViaSliderCallbacks() as well https://codereview.chromium.org/895543005/diff/340001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay_unittest.cc:309: // the slide window, which will be turned into the new overlay window which will be used as the overlay window when the new overscroll starts https://codereview.chromium.org/895543005/diff/340001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay_unittest.cc:312: gfx::Point(550, 10), Avoid using "magic numbers" like this in tests. This applies to the previous gesture as well, where we should probably use something like 10+threshold+1. For this case I think it is a bigger problem - looks like you are somehow relying on window being a certain width, and it's not clear immediately where is that even declared. Best to use the width and the threshold here as well. BTW, we should probably also DCHECK that threshold+1 is going to leave us within the bounds.
@mfomitchev, please see the reviewed patch. Thanks! https://codereview.chromium.org/895543005/diff/300001/content/browser/web_con... File content/browser/web_contents/aura/overscroll_navigation_overlay_unittest.cc (right): https://codereview.chromium.org/895543005/diff/300001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay_unittest.cc:29: class OverscrollTestWebContents : public TestWebContents { On 2015/03/31 22:16:31, mfomitchev wrote: > What is the reason we need to set the "fake" native view? Is it something unique > to our tests or an issue with the default behavior that could cause problems to > other tests as well? If the latter - why not also add SetNativeView to > TestWebContents? It's unique to our case AFAICT. We need a window with a parent (for the clipping code), and the test infrastructure provides a window without a parent. I can't find a subclass of TestWebContents in CS, so I assume other tests do not need it. https://codereview.chromium.org/895543005/diff/300001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay_unittest.cc:183: GetOverlay()->window_->delegate())->has_image()); On 2015/03/31 22:16:31, mfomitchev wrote: > On 2015/03/27 17:52:36, Nicolás wrote: > > On 2015/03/27 00:14:05, mfomitchev wrote: > > > Verify contents()->GetURL()? > > > > I don't think we need to re-test this (we do it later), as the point of the > test > > is to make sure ONO works correctly with / without screenshot. > > Ok, well, maybe put the check for cross_navigation_pending() after Completing() > is called into PerformBackNavigationViaSliderCallbacks(). Done. > This is the first, > most basic test, and requesting navigation in response to the callback is part > of ONO's most basic functionality. So it seems like that should be tested here. So are you OK with testing cross_navigation_pending() in PerformBackBavigationViaSliderCallbacks()? https://codereview.chromium.org/895543005/diff/300001/content/browser/web_con... File content/browser/web_contents/aura/overscroll_window_delegate_unittest.cc (right): https://codereview.chromium.org/895543005/diff/300001/content/browser/web_con... content/browser/web_contents/aura/overscroll_window_delegate_unittest.cc:117: gfx::Point(20 + touch_complete_threshold(), 10), On 2015/03/31 22:16:31, mfomitchev wrote: > On 2015/03/27 17:52:37, Nicolás wrote: > > On 2015/03/27 00:14:05, mfomitchev wrote: > > > 10 + touch_complete_threshold() + 1? > > > > By using 10, we add 1 per motion, which I thought was clearer. I also replaced > > "10 + 10" with "20", it didn't add much to readability and made the line > longer. > > We want to ensure that if we are over the threshold - the overscroll is started. > The number of steps in the gesture is completely irrelevant. It could be 1 or > 1000, I don 't think that should impact the behavior in any way. OK, modified test accordingly. https://codereview.chromium.org/895543005/diff/300001/content/browser/web_con... content/browser/web_contents/aura/overscroll_window_delegate_unittest.cc:171: gfx::Point(10 + touch_start_threshold() + 10, 10), On 2015/03/31 22:16:31, mfomitchev wrote: > ditto Done. https://codereview.chromium.org/895543005/diff/340001/content/browser/web_con... File content/browser/web_contents/aura/overscroll_navigation_overlay.cc (right): https://codereview.chromium.org/895543005/diff/340001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay.cc:126: web_contents_->GetNativeView()->layer()->SetMasksToBounds(true); On 2015/03/31 21:04:06, mfomitchev wrote: > What if it was already false when we called OnOverscrollCompleting()? Also, I > wonder how bad would it be if ONO is destroyed between OnOverscrollCompleting > and StopObservingIfDone. > A good way to do this would be use a pattern similar to RAII here: create an > class which sets MasksToBounds to false in its constructor and saves the old > values. In the destructor set them back to the old values if and only if the old > values were "true". Then add a member var to ONO which would be a scoped_ptr to > the instance of that class. Init it in OnOverscrollCompleting(), reset it in > StopObservingIfDone(). The advantage is that if ONO is destroyed for whatever > reason while this is active - the flags would be reset. SGTM, done. https://codereview.chromium.org/895543005/diff/340001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay.cc:196: GetMainWindow()->ReleaseCapture(); On 2015/03/31 21:04:06, mfomitchev wrote: > Using RAII-like pattern for this is probably an overkill, but perhaps call > GetMainWindow()->ReleaseCapture() in the destructor if OWA is active? Done. https://codereview.chromium.org/895543005/diff/340001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay.cc:202: // their parents coordinates. On 2015/03/31 21:04:06, mfomitchev wrote: > Avoid clipping of the screenshot caused by the contents window being moved > outside of the screen bounds. Done. https://codereview.chromium.org/895543005/diff/340001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay.cc:219: // Reset the position of the main window. |main_window| will be invalid On 2015/03/31 21:04:06, mfomitchev wrote: > No need to say "|main_window| will be invalid after this block." Done. https://codereview.chromium.org/895543005/diff/340001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay.cc:223: main_window->SetBounds(gfx::Rect(web_contents_window_->bounds().size())); On 2015/03/31 21:04:05, mfomitchev wrote: > Why do we need to set the bounds here? We don't ever change the bounds, do we? > Perhaps this should be just > GetMainWindow()->SetTransform(gfx::Transform())? Done. https://codereview.chromium.org/895543005/diff/340001/content/browser/web_con... File content/browser/web_contents/aura/overscroll_navigation_overlay.h (right): https://codereview.chromium.org/895543005/diff/340001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay.h:61: // an overscroll slide isn't in progress and either the page has been painted On 2015/03/31 21:04:06, mfomitchev wrote: > I don't think "overscroll slide" hasn't been established as a term in this > class. Either explain what it is or just say "if there's no active overscroll > window animation". Done. https://codereview.chromium.org/895543005/diff/340001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay.h:66: // the current overscroll |direction_|. Transfers ownership to the caller. On 2015/03/31 21:04:06, mfomitchev wrote: > NO need to talk about the ownership transfer. scoped_ptr makes the ownership > clear. Removed. https://codereview.chromium.org/895543005/diff/340001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay.h:93: scoped_ptr<aura::Window> window_; On 2015/03/31 21:04:06, mfomitchev wrote: > Now this is actually a bit misleading since this is null in the "first case" and > is only set to the overlay window in the "second case". To explain this field we > need to explain these two cases somewhere in this .h. Perhaps right here or in > the class comments. Explained up there in the class comments. https://codereview.chromium.org/895543005/diff/340001/content/browser/web_con... File content/browser/web_contents/aura/overscroll_window_animation.cc (right): https://codereview.chromium.org/895543005/diff/340001/content/browser/web_con... content/browser/web_contents/aura/overscroll_window_animation.cc:77: delegate_->OnOverscrollAborted(); On 2015/03/31 21:04:07, mfomitchev wrote: > Set overscroll_cancelled_ to false here? Done. https://codereview.chromium.org/895543005/diff/340001/content/browser/web_con... File content/browser/web_contents/aura/overscroll_window_animation.h (right): https://codereview.chromium.org/895543005/diff/340001/content/browser/web_con... content/browser/web_contents/aura/overscroll_window_animation.h:36: // stays behind while the slide window one moves in from the right. SLIDE_BACK On 2015/03/31 21:04:07, mfomitchev wrote: > stays still, while the slide window moves on top of it, entering from the right. Done. https://codereview.chromium.org/895543005/diff/340001/content/browser/web_con... content/browser/web_contents/aura/overscroll_window_animation.h:38: // window on the back. SLIDE_NONE means we are not animating yet. Left and On 2015/03/31 21:04:07, mfomitchev wrote: > in the back, which stays still. Done. https://codereview.chromium.org/895543005/diff/340001/content/browser/web_con... content/browser/web_contents/aura/overscroll_window_animation.h:39: // right are reversed for users of RTL languages, but stack order remains On 2015/03/31 21:04:08, mfomitchev wrote: > Directions are reversed for RTL languages,.. Done. https://codereview.chromium.org/895543005/diff/340001/content/browser/web_con... content/browser/web_contents/aura/overscroll_window_animation.h:62: // ownership is transferred. On 2015/03/31 21:04:08, mfomitchev wrote: > "The slide window's ownership is transferred" -> "|window| points to the slide > window". Ownership is implicit from scoped_ptr. Done. https://codereview.chromium.org/895543005/diff/340001/content/browser/web_con... content/browser/web_contents/aura/overscroll_window_animation.h:84: void OnOverscrollModeChange(OverscrollMode old_mode, On 2015/03/31 21:04:07, mfomitchev wrote: > Leave no space between overridden methods Done. https://codereview.chromium.org/895543005/diff/340001/content/browser/web_con... content/browser/web_contents/aura/overscroll_window_animation.h:117: // Indicates if the current animation has been cancelled. On 2015/03/31 21:04:07, mfomitchev wrote: > Pick either "aborted" or "cancelled and stick to that within this class. Right > now you use both. Also - this is only true while the "abort" animation is > progress? If so - say that. s/abort/cancel for the overscroll related functions.
@mfomitchev, just fixed the stuff I had left. I also attempted fixing the trybot failures - I can't repro some of them locally. https://codereview.chromium.org/895543005/diff/340001/content/browser/web_con... File content/browser/web_contents/aura/overscroll_navigation_overlay_unittest.cc (right): https://codereview.chromium.org/895543005/diff/340001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay_unittest.cc:215: EXPECT_EQ(GetOverlay()->direction_, OverscrollNavigationOverlay::BACK); On 2015/03/31 22:16:31, mfomitchev wrote: > Might actually want to add this check to > PerformBackNavigationViaSliderCallbacks() as well Done. https://codereview.chromium.org/895543005/diff/340001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay_unittest.cc:309: // the slide window, which will be turned into the new overlay window On 2015/03/31 22:16:31, mfomitchev wrote: > which will be used as the overlay window when the new overscroll starts Done. https://codereview.chromium.org/895543005/diff/340001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay_unittest.cc:312: gfx::Point(550, 10), On 2015/03/31 22:16:31, mfomitchev wrote: > Avoid using "magic numbers" like this in tests. This applies to the previous > gesture as well, where we should probably use something like 10+threshold+1. > > For this case I think it is a bigger problem - looks like you are somehow > relying on window being a certain width, and it's not clear immediately where is > that even declared. Best to use the width and the threshold here as well. > > BTW, we should probably also DCHECK that threshold+1 is going to leave us within > the bounds. Done.
LGTM with nits!
All comments are pretty nitty https://codereview.chromium.org/895543005/diff/300001/content/browser/web_con... File content/browser/web_contents/aura/overscroll_navigation_overlay_unittest.cc (right): https://codereview.chromium.org/895543005/diff/300001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay_unittest.cc:29: class OverscrollTestWebContents : public TestWebContents { On 2015/04/01 18:10:00, Nicolás wrote: > On 2015/03/31 22:16:31, mfomitchev wrote: > > What is the reason we need to set the "fake" native view? Is it something > unique > > to our tests or an issue with the default behavior that could cause problems > to > > other tests as well? If the latter - why not also add SetNativeView to > > TestWebContents? > > It's unique to our case AFAICT. We need a window with a parent (for the clipping > code), and the test infrastructure provides a window without a parent. I can't > find a subclass of TestWebContents in CS, so I assume other tests do not need > it. Gotcha. https://codereview.chromium.org/895543005/diff/460001/content/browser/web_con... File content/browser/web_contents/aura/overscroll_navigation_overlay.h (right): https://codereview.chromium.org/895543005/diff/460001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay.h:28: // of the page until the page has completed loading and painting. Say here that this is what |window_| is, since you are using |window_| further down in this comment. https://codereview.chromium.org/895543005/diff/460001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay.h:29: // There are two overscroll cases, for the first one the main window is the live "live content window" -> web contents window https://codereview.chromium.org/895543005/diff/460001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay.h:31: // completes, |window_| is set to the slide window returned by "slide window" is not an established concept here. Lets say "overscroll window" or just "window". https://codereview.chromium.org/895543005/diff/460001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay.h:100: // handles overscroll events during the second case. "second overscroll case" https://codereview.chromium.org/895543005/diff/460001/content/browser/web_con... File content/browser/web_contents/aura/overscroll_navigation_overlay_unittest.cc (right): https://codereview.chromium.org/895543005/diff/460001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay_unittest.cc:103: if (window) { Can we add checks for !window case as well? https://codereview.chromium.org/895543005/diff/460001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay_unittest.cc:136: browser_context(), SiteInstance::Create(browser_context()), Nit: I am surprised git cl format did this. I thought the chromium style was all params on one line or one param per line. The old way was certainly more readable. https://codereview.chromium.org/895543005/diff/460001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay_unittest.cc:298: base::TimeDelta::FromMilliseconds(10), 10); nit: last 10 on a separate line https://codereview.chromium.org/895543005/diff/460001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay_unittest.cc:303: // The overlay window should now be being animated to the edge of the screen. We've removed animator->Step(), so there will be no movement in the animation? But we do want the window to move a bit, don't we? https://codereview.chromium.org/895543005/diff/460001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay_unittest.cc:310: int second_overscroll_start_distance = overscroll_complete_distance + 1; A comment explaining why we chose such a value for second_overscroll_start_distance would be nice. It's pretty obscure..
nsatragno@chromium.org changed reviewers: + sky@chromium.org
nsatragno@chromium.org changed required reviewers: + sky@chromium.org
@mfomitchev, done with the nits. @sadrul, please review content/browser/web_contents/web_contents_view_aura.(cc|h) and content/browser/renderer_host/overscroll_controller_delegate.h. @sky, please review content/browser/BUILD.gn and content/test/BUILD.gn Thanks! https://codereview.chromium.org/895543005/diff/460001/content/browser/web_con... File content/browser/web_contents/aura/overscroll_navigation_overlay.h (right): https://codereview.chromium.org/895543005/diff/460001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay.h:28: // of the page until the page has completed loading and painting. On 2015/04/02 22:50:36, mfomitchev wrote: > Say here that this is what |window_| is, since you are using |window_| further > down in this comment. Done. https://codereview.chromium.org/895543005/diff/460001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay.h:29: // There are two overscroll cases, for the first one the main window is the live On 2015/04/02 22:50:36, mfomitchev wrote: > "live content window" -> web contents window Done. https://codereview.chromium.org/895543005/diff/460001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay.h:31: // completes, |window_| is set to the slide window returned by On 2015/04/02 22:50:36, mfomitchev wrote: > "slide window" is not an established concept here. Lets say "overscroll window" > or just "window". Done. https://codereview.chromium.org/895543005/diff/460001/content/browser/web_con... File content/browser/web_contents/aura/overscroll_navigation_overlay_unittest.cc (right): https://codereview.chromium.org/895543005/diff/460001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay_unittest.cc:103: if (window) { On 2015/04/02 22:50:37, mfomitchev wrote: > Can we add checks for !window case as well? Done. https://codereview.chromium.org/895543005/diff/460001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay_unittest.cc:136: browser_context(), SiteInstance::Create(browser_context()), On 2015/04/02 22:50:37, mfomitchev wrote: > Nit: I am surprised git cl format did this. I thought the chromium style was all > params on one line or one param per line. The old way was certainly more > readable. Done. https://codereview.chromium.org/895543005/diff/460001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay_unittest.cc:298: base::TimeDelta::FromMilliseconds(10), 10); On 2015/04/02 22:50:37, mfomitchev wrote: > nit: last 10 on a separate line Done. https://codereview.chromium.org/895543005/diff/460001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay_unittest.cc:303: // The overlay window should now be being animated to the edge of the screen. On 2015/04/02 22:50:37, mfomitchev wrote: > We've removed animator->Step(), so there will be no movement in the animation? > But we do want the window to move a bit, don't we? TBH it really is not necessary and only complicates the math for the next gesture a little. Putting the animation "on hold" is enough. https://codereview.chromium.org/895543005/diff/460001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay_unittest.cc:310: int second_overscroll_start_distance = overscroll_complete_distance + 1; On 2015/04/02 22:50:37, mfomitchev wrote: > A comment explaining why we chose such a value for > second_overscroll_start_distance would be nice. It's pretty obscure.. If it was hard to write, it must be hard to read! Just kidding, added some ascii art for it.
https://codereview.chromium.org/895543005/diff/480001/content/browser/web_con... File content/browser/web_contents/aura/overscroll_navigation_overlay.h (right): https://codereview.chromium.org/895543005/diff/480001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay.h:47: OverscrollWindowAnimation* owa() { return owa_.get(); } Can you expose this as OverscrollControllerDelegate? e.g. OverscrollControllerDelegate* relay_delegate() https://codereview.chromium.org/895543005/diff/480001/content/browser/web_con... File content/browser/web_contents/aura/overscroll_navigation_overlay_unittest.cc (right): https://codereview.chromium.org/895543005/diff/480001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay_unittest.cc:118: const GURL fourth_; These should be private (with public accessors if need be). https://codereview.chromium.org/895543005/diff/480001/content/browser/web_con... File content/browser/web_contents/aura/overscroll_window_animation.h (right): https://codereview.chromium.org/895543005/diff/480001/content/browser/web_con... content/browser/web_contents/aura/overscroll_window_animation.h:48: // The following two functions create a slide window. This comment isn't necessary. https://codereview.chromium.org/895543005/diff/480001/content/browser/web_con... content/browser/web_contents/aura/overscroll_window_animation.h:53: // this window. Not clear what 'We' here means. Either 'The delegate does not own this window' or 'The caller does not own the window'. https://codereview.chromium.org/895543005/diff/480001/content/browser/web_con... content/browser/web_contents/aura/overscroll_window_animation.h:61: virtual void OnOverscrollCompleted(scoped_ptr<aura::Window> window) = 0; What is |window|? https://codereview.chromium.org/895543005/diff/480001/content/browser/web_con... content/browser/web_contents/aura/overscroll_window_animation.h:75: // Overridden from OverscrollControllerDelegate: // OverscrollControllerDelegate: (like below for ImplicitAnimationObserver) https://codereview.chromium.org/895543005/diff/480001/content/browser/web_con... content/browser/web_contents/aura/overscroll_window_animation.h:94: ui::Layer* GetFrontLayer() const; Document ownership of the returned layer. If the caller owns this layer, then return a scoped_ptr<> https://codereview.chromium.org/895543005/diff/480001/content/browser/web_con... content/browser/web_contents/aura/overscroll_window_animation.h:114: bool overscroll_cancelled_; DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/895543005/diff/480001/content/browser/web_con... File content/browser/web_contents/aura/overscroll_window_delegate.h (right): https://codereview.chromium.org/895543005/diff/480001/content/browser/web_con... content/browser/web_contents/aura/overscroll_window_delegate.h:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. I believe we do not do (c) anymore. https://codereview.chromium.org/895543005/diff/480001/content/browser/web_con... File content/browser/web_contents/web_contents_view_aura.cc (right): https://codereview.chromium.org/895543005/diff/480001/content/browser/web_con... content/browser/web_contents/web_contents_view_aura.cc:1199: overscroll_window_animation_ = nullptr; Instead of keeping this pointer |overscroll_window_animation_| in WCVA, access it through navigation_overlay_ when needed.
nsatragno@chromium.org changed reviewers: + avi@chromium.org
nsatragno@chromium.org changed required reviewers: + avi@chromium.org
sadrul@, I addressed the comments. Please give the patch a new look. I just realized sky@ does not own content/browser/BUILD.gn :S @avi, would you please review content/browser/BUILD.gn? Thanks! https://codereview.chromium.org/895543005/diff/480001/content/browser/web_con... File content/browser/web_contents/aura/overscroll_navigation_overlay.h (right): https://codereview.chromium.org/895543005/diff/480001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay.h:47: OverscrollWindowAnimation* owa() { return owa_.get(); } On 2015/04/07 17:10:08, sadrul wrote: > Can you expose this as OverscrollControllerDelegate? e.g. > > OverscrollControllerDelegate* relay_delegate() Done. https://codereview.chromium.org/895543005/diff/480001/content/browser/web_con... File content/browser/web_contents/aura/overscroll_navigation_overlay_unittest.cc (right): https://codereview.chromium.org/895543005/diff/480001/content/browser/web_con... content/browser/web_contents/aura/overscroll_navigation_overlay_unittest.cc:118: const GURL fourth_; On 2015/04/07 17:10:08, sadrul wrote: > These should be private (with public accessors if need be). Done. https://codereview.chromium.org/895543005/diff/480001/content/browser/web_con... File content/browser/web_contents/aura/overscroll_window_animation.h (right): https://codereview.chromium.org/895543005/diff/480001/content/browser/web_con... content/browser/web_contents/aura/overscroll_window_animation.h:48: // The following two functions create a slide window. On 2015/04/07 17:10:08, sadrul wrote: > This comment isn't necessary. Removed. https://codereview.chromium.org/895543005/diff/480001/content/browser/web_con... content/browser/web_contents/aura/overscroll_window_animation.h:53: // this window. On 2015/04/07 17:10:08, sadrul wrote: > Not clear what 'We' here means. Either 'The delegate does not own this window' > or 'The caller does not own the window'. Good call. s/ to The delegate does not own this window https://codereview.chromium.org/895543005/diff/480001/content/browser/web_con... content/browser/web_contents/aura/overscroll_window_animation.h:61: virtual void OnOverscrollCompleted(scoped_ptr<aura::Window> window) = 0; On 2015/04/07 17:10:08, sadrul wrote: > What is |window|? Added "The slide window is transferred to the delegate." https://codereview.chromium.org/895543005/diff/480001/content/browser/web_con... content/browser/web_contents/aura/overscroll_window_animation.h:75: // Overridden from OverscrollControllerDelegate: On 2015/04/07 17:10:08, sadrul wrote: > // OverscrollControllerDelegate: > > (like below for ImplicitAnimationObserver) Done. https://codereview.chromium.org/895543005/diff/480001/content/browser/web_con... content/browser/web_contents/aura/overscroll_window_animation.h:94: ui::Layer* GetFrontLayer() const; On 2015/04/07 17:10:08, sadrul wrote: > Document ownership of the returned layer. If the caller owns this layer, then > return a scoped_ptr<> The caller does not own it, made it clear in the comment. https://codereview.chromium.org/895543005/diff/480001/content/browser/web_con... content/browser/web_contents/aura/overscroll_window_animation.h:114: bool overscroll_cancelled_; On 2015/04/07 17:10:08, sadrul wrote: > DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/895543005/diff/480001/content/browser/web_con... File content/browser/web_contents/aura/overscroll_window_delegate.h (right): https://codereview.chromium.org/895543005/diff/480001/content/browser/web_con... content/browser/web_contents/aura/overscroll_window_delegate.h:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. On 2015/04/07 17:10:08, sadrul wrote: > I believe we do not do (c) anymore. Removed all of these in the new code. https://codereview.chromium.org/895543005/diff/480001/content/browser/web_con... File content/browser/web_contents/web_contents_view_aura.cc (right): https://codereview.chromium.org/895543005/diff/480001/content/browser/web_con... content/browser/web_contents/web_contents_view_aura.cc:1199: overscroll_window_animation_ = nullptr; On 2015/04/07 17:10:08, sadrul wrote: > Instead of keeping this pointer |overscroll_window_animation_| in WCVA, access > it through navigation_overlay_ when needed. Done.
content/browser/BUILD.gn LGTM
https://codereview.chromium.org/895543005/diff/480001/content/browser/web_con... File content/browser/web_contents/aura/overscroll_window_animation.cc (right): https://codereview.chromium.org/895543005/diff/480001/content/browser/web_con... content/browser/web_contents/aura/overscroll_window_animation.cc:92: CancelAnimation(); I think this method name is a bit confusing - we are not actually cancelling the animation, we are cancelling the overscroll and starting the animation. Perhaps call this CancelOverscroll() or CancelSlide()? https://codereview.chromium.org/895543005/diff/480001/content/browser/web_con... content/browser/web_contents/aura/overscroll_window_animation.cc:95: if (slide_window_) Use is_active() instead? https://codereview.chromium.org/895543005/diff/480001/content/browser/web_con... content/browser/web_contents/aura/overscroll_window_animation.cc:113: slide_window_->SetBounds(bounds); Ugh, just noticed this. Why are we setting the bounds instead of setting the transform? I remember I asked a similar question in ONO when we were resetting the bounds to default, and we got rid of that code..
lgtm
@mfomitchev done with the nits. https://codereview.chromium.org/895543005/diff/480001/content/browser/web_con... File content/browser/web_contents/aura/overscroll_window_animation.cc (right): https://codereview.chromium.org/895543005/diff/480001/content/browser/web_con... content/browser/web_contents/aura/overscroll_window_animation.cc:92: CancelAnimation(); On 2015/04/07 21:07:38, mfomitchev wrote: > I think this method name is a bit confusing - we are not actually cancelling the > animation, we are cancelling the overscroll and starting the animation. Perhaps > call this CancelOverscroll() or CancelSlide()? Renamed to CancelSlide. https://codereview.chromium.org/895543005/diff/480001/content/browser/web_con... content/browser/web_contents/aura/overscroll_window_animation.cc:95: if (slide_window_) On 2015/04/07 21:07:38, mfomitchev wrote: > Use is_active() instead? Done. https://codereview.chromium.org/895543005/diff/480001/content/browser/web_con... content/browser/web_contents/aura/overscroll_window_animation.cc:113: slide_window_->SetBounds(bounds); On 2015/04/07 21:07:38, mfomitchev wrote: > Ugh, just noticed this. Why are we setting the bounds instead of setting the > transform? To simplify the transform logic - especially for parallax scrolling. > I remember I asked a similar question in ONO when we were resetting > the bounds to default, and we got rid of that code.. I modified the code as we discussed offline to make sure we do not set bounds on the content window, and most of the bounds setting occurs before aura::Window::Show().
The other GN file LGTM too.
https://codereview.chromium.org/895543005/diff/600001/content/browser/web_con... File content/browser/web_contents/aura/overscroll_window_animation.cc (right): https://codereview.chromium.org/895543005/diff/600001/content/browser/web_con... content/browser/web_contents/aura/overscroll_window_animation.cc:98: gfx::Rect bounds = gfx::Rect(GetVisibleBounds().size()); maybe call this slide_window_bounds? https://codereview.chromium.org/895543005/diff/600001/content/browser/web_con... File content/browser/web_contents/aura/overscroll_window_animation.h (right): https://codereview.chromium.org/895543005/diff/600001/content/browser/web_con... content/browser/web_contents/aura/overscroll_window_animation.h:49: const gfx::Rect& bounds) = 0; Please comment what bounds are
Done, thanks to everyone, will CQ. https://codereview.chromium.org/895543005/diff/600001/content/browser/web_con... File content/browser/web_contents/aura/overscroll_window_animation.cc (right): https://codereview.chromium.org/895543005/diff/600001/content/browser/web_con... content/browser/web_contents/aura/overscroll_window_animation.cc:98: gfx::Rect bounds = gfx::Rect(GetVisibleBounds().size()); On 2015/04/08 17:33:16, mfomitchev wrote: > maybe call this slide_window_bounds? Done. https://codereview.chromium.org/895543005/diff/600001/content/browser/web_con... File content/browser/web_contents/aura/overscroll_window_animation.h (right): https://codereview.chromium.org/895543005/diff/600001/content/browser/web_con... content/browser/web_contents/aura/overscroll_window_animation.h:49: const gfx::Rect& bounds) = 0; On 2015/04/08 17:33:16, mfomitchev wrote: > Please comment what bounds are Done.
The CQ bit was checked by nsatragno@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mfomitchev@chromium.org, sadrul@chromium.org, avi@chromium.org Link to the patchset: https://codereview.chromium.org/895543005/#ps620001 (title: "Fixed nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/895543005/620001
The CQ bit was unchecked by commit-bot@chromium.org
All required reviewers (with asterisk prefixes) have not yet approved this CL. No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an LGTM 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.
nsatragno@chromium.org changed reviewers: - sky@chromium.org
nsatragno@chromium.org changed required reviewers: - sky@chromium.org
Removed sky@ as required reviewer since avi@ LGTM'd missing BUILD.gn file.
The CQ bit was checked by nsatragno@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/895543005/620001
Message was sent while issue was closed.
Committed patchset #32 (id:620001)
Message was sent while issue was closed.
Patchset 32 (id:??) landed as https://crrev.com/5384f002f839439ef666ed9688069e42ca5ccdca Cr-Commit-Position: refs/heads/master@{#324275}
Message was sent while issue was closed.
A revert of this CL (patchset #32 id:620001) has been created in https://codereview.chromium.org/1076743003/ by engedy@chromium.org. The reason for reverting is: After this CL, content browsertest WebContentsViewAuraTest.RepeatedQuickOverscrollGestures started failing with segmentation fault on almost every run on Linux ChromiumOS Tests (dbg)(1). See: http://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%2.... |
