Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
556703005:
Initial draft - modify ViewportAnchor to know about both inner and outer viewports. (Closed)
|
Created:
6 years, 3 months ago by Tima Vaisburd Modified:
6 years, 2 months ago Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Project:
blink Visibility:
Public. |
DescriptionFix pinch virtual viewport position after resize.
Associate the viewport anchor with the inner viewport.
Adjust the inner and outer viewport positions after resize
such that they both remain in their allowed range and the
inner viewport origin scales proportionally within the
outer viewport.
As a small cleanup, made the method ScrollView::scrollTo() protected.
BUG=364108
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=182365
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=182727
Patch Set 1 #
Total comments: 16
Patch Set 2 : ViewportAnchor passes the inner viewport offset relative to outer viewport; fixed bugs #
Total comments: 19
Patch Set 3 : Accounted for inner and outer viewport constraints #
Total comments: 6
Patch Set 4 : Pass the frame by reference. #Patch Set 5 : Added unit tests. #
Total comments: 9
Patch Set 6 : Changed unit tests, added missing PinchViewport.h change #Patch Set 7 : Redid unit tests with actual scaling. #
Total comments: 14
Patch Set 8 : Fixed spaces and removed extra comments. #
Total comments: 6
Patch Set 9 : Changed the parameter name frameView -> scrollView #Patch Set 10 : Passes output params by reference in ViewportAnchor::computeOrigins() #
Total comments: 1
Patch Set 11 : Passes output param by reference in moveToEncloseRect() and moveIntoRect() #Patch Set 12 : Reapplying patch set 11 after a mac test #
Messages
Total messages: 61 (11 generated)
timav@google.com changed reviewers: + aelias@chromium.org, bokan@chromium.org
timav@google.com changed reviewers: + timav@google.com
Please review the first draft
The semantics of this look fine. Comments below are mainly for more clarity/consistency in the coordinate system being used. https://codereview.chromium.org/556703005/diff/1/Source/web/ViewportAnchor.cpp File Source/web/ViewportAnchor.cpp (right): https://codereview.chromium.org/556703005/diff/1/Source/web/ViewportAnchor.cp... Source/web/ViewportAnchor.cpp:118: IntPoint ViewportAnchor::computeInnerViewportOrigin(const IntSize& innerSize) const Could you make this a FloatPoint instead, and likewise use FloatPoint everywhere related to inner viewport? The only offset that really needs to be integers here is what goes into FrameView::scrollTo. https://codereview.chromium.org/556703005/diff/1/Source/web/ViewportAnchor.h File Source/web/ViewportAnchor.h (right): https://codereview.chromium.org/556703005/diff/1/Source/web/ViewportAnchor.h#... Source/web/ViewportAnchor.h:63: IntPoint computeOuterViewportOrigin(const IntPoint& innerOrigin, const IntSize& outerSize) const; How about merging these two functions into: void computeOrigins(const IntSize& innerSize, const IntSize& outerSize, IntSize* outerScrollOffset, FloatSize pinchViewportOffset); (pinchViewportOffset would be the vector from outer -> inner, i.e. exactly the value needed in the PinchViewport class.) https://codereview.chromium.org/556703005/diff/1/Source/web/ViewportAnchor.h#... Source/web/ViewportAnchor.h:75: FloatSize m_outerInInnerCoords; Please rename this to m_normalizedPinchViewportOffset and invert the sign (currently it's always negative or zero which seems weird, please make it always nonnegative instead). https://codereview.chromium.org/556703005/diff/1/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/556703005/diff/1/Source/web/WebViewImpl.cpp#n... Source/web/WebViewImpl.cpp:1730: IntRect WebViewImpl::outerViewportRectInDocument() const This method doesn't seem to save much code duplication so please delete it. https://codereview.chromium.org/556703005/diff/1/Source/web/WebViewImpl.cpp#n... Source/web/WebViewImpl.cpp:2950: const IntPoint & innerViewportOrigin, Please make these arguments: const IntPoint& outerViewportOffset, const FloatPoint& pinchViewportOffset then you don't need to do any conversions at all in this method.
Looks good overall. Left a few comments in addition to what Alex said. One case I noticed breaks is when in portrait, zoomed in, and scrolled to the end horizontally. I do a rotation to landscape. I'm no longer at the maximum scroll offset. You'll also want to add some tests when you're ready. Have a look at Source/web/tests/PinchViewportTest.cpp, you can add some similar tests there. https://codereview.chromium.org/556703005/diff/1/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/556703005/diff/1/Source/web/WebViewImpl.cpp#n... Source/web/WebViewImpl.cpp:1693: performResize(); Why did this get moved? TextAutosizer::DeferUpdatePageInfo sets/unsets some global state in its constructor/destructor so this is not equivalent. https://codereview.chromium.org/556703005/diff/1/Source/web/WebViewImpl.cpp#n... Source/web/WebViewImpl.cpp:1706: IntPoint innerOrigin = viewportAnchor.computeInnerViewportOrigin(innerViewportSize); Can you please rename as follows (here and other places in this CL): innerOrigin -> pinchViewportOrigin outerOrigin -> mainFrameOrigin On CC they're referred to by inner/outer, in Blink they're pinchViewport and mainFrame. Hopefully we'll soon converge and rename to one set everywhere but for now we should at least maintain consistency within local context. https://codereview.chromium.org/556703005/diff/1/Source/web/WebViewImpl.cpp#n... Source/web/WebViewImpl.cpp:2970: view->scrollTo(toIntSize(outerViewportOrigin)); Use WebViewImpl::updateMainFrameScrollPosition(IntPoint, false) instead. Or view->setScrollOffset(). ScrollView::scrollTo has a comment not to call from outside setScrollOffset and looks like it doesn't clamp to scroll bounds (try going landscape to portrait when fully vertically scrolled). Something, something, footguns...
On 2014/09/10 15:03:59, bokan wrote: > Looks good overall. Left a few comments in addition to what Alex said. > > One case I noticed breaks is when in portrait, zoomed in, and scrolled to the > end horizontally. I do a rotation to landscape. I'm no longer at the maximum > scroll offset. > > You'll also want to add some tests when you're ready. Have a look at > Source/web/tests/PinchViewportTest.cpp, you can add some similar tests there. > > https://codereview.chromium.org/556703005/diff/1/Source/web/WebViewImpl.cpp > File Source/web/WebViewImpl.cpp (right): > > https://codereview.chromium.org/556703005/diff/1/Source/web/WebViewImpl.cpp#n... > Source/web/WebViewImpl.cpp:1693: performResize(); > Why did this get moved? TextAutosizer::DeferUpdatePageInfo sets/unsets some > global state in its constructor/destructor so this is not equivalent. > > https://codereview.chromium.org/556703005/diff/1/Source/web/WebViewImpl.cpp#n... > Source/web/WebViewImpl.cpp:1706: IntPoint innerOrigin = > viewportAnchor.computeInnerViewportOrigin(innerViewportSize); > Can you please rename as follows (here and other places in this CL): > > innerOrigin -> pinchViewportOrigin > outerOrigin -> mainFrameOrigin > > On CC they're referred to by inner/outer, in Blink they're pinchViewport and > mainFrame. Hopefully we'll soon converge and rename to one set everywhere but > for now we should at least maintain consistency within local context. > > https://codereview.chromium.org/556703005/diff/1/Source/web/WebViewImpl.cpp#n... > Source/web/WebViewImpl.cpp:2970: view->scrollTo(toIntSize(outerViewportOrigin)); > Use WebViewImpl::updateMainFrameScrollPosition(IntPoint, false) instead. Or > view->setScrollOffset(). ScrollView::scrollTo has a comment not to call from > outside setScrollOffset and looks like it doesn't clamp to scroll bounds (try > going landscape to portrait when fully vertically scrolled). Something, > something, footguns... When scrolled to the right end the rotation does not work as expected. I'll take a look. Thank you for pointing this out.
https://codereview.chromium.org/556703005/diff/1/Source/web/ViewportAnchor.cpp File Source/web/ViewportAnchor.cpp (right): https://codereview.chromium.org/556703005/diff/1/Source/web/ViewportAnchor.cp... Source/web/ViewportAnchor.cpp:118: IntPoint ViewportAnchor::computeInnerViewportOrigin(const IntSize& innerSize) const On 2014/09/10 01:46:24, aelias wrote: > Could you make this a FloatPoint instead, and likewise use FloatPoint everywhere > related to inner viewport? The only offset that really needs to be integers > here is what goes into FrameView::scrollTo. Yes, this was one of my questions, should everything be Float. https://codereview.chromium.org/556703005/diff/1/Source/web/ViewportAnchor.h File Source/web/ViewportAnchor.h (right): https://codereview.chromium.org/556703005/diff/1/Source/web/ViewportAnchor.h#... Source/web/ViewportAnchor.h:63: IntPoint computeOuterViewportOrigin(const IntPoint& innerOrigin, const IntSize& outerSize) const; On 2014/09/10 01:46:24, aelias wrote: > How about merging these two functions into: > > void computeOrigins(const IntSize& innerSize, const IntSize& outerSize, IntSize* > outerScrollOffset, FloatSize pinchViewportOffset); > > (pinchViewportOffset would be the vector from outer -> inner, i.e. exactly the > value needed in the PinchViewport class.) Will do. https://codereview.chromium.org/556703005/diff/1/Source/web/ViewportAnchor.h#... Source/web/ViewportAnchor.h:75: FloatSize m_outerInInnerCoords; On 2014/09/10 01:46:24, aelias wrote: > Please rename this to m_normalizedPinchViewportOffset and invert the sign > (currently it's always negative or zero which seems weird, please make it always > nonnegative instead). Yes. The sign was positive in the beginning, but I decided to avoid multiplying by -1.0. Was not thinking straight. https://codereview.chromium.org/556703005/diff/1/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/556703005/diff/1/Source/web/WebViewImpl.cpp#n... Source/web/WebViewImpl.cpp:1693: performResize(); On 2014/09/10 15:03:59, bokan wrote: > Why did this get moved? TextAutosizer::DeferUpdatePageInfo sets/unsets some > global state in its constructor/destructor so this is not equivalent. Oops. Thank you, I did not catch that. Will undo. https://codereview.chromium.org/556703005/diff/1/Source/web/WebViewImpl.cpp#n... Source/web/WebViewImpl.cpp:1730: IntRect WebViewImpl::outerViewportRectInDocument() const On 2014/09/10 01:46:25, aelias wrote: > This method doesn't seem to save much code duplication so please delete it. Acknowledged. https://codereview.chromium.org/556703005/diff/1/Source/web/WebViewImpl.cpp#n... Source/web/WebViewImpl.cpp:2950: const IntPoint & innerViewportOrigin, On 2014/09/10 01:46:25, aelias wrote: > Please make these arguments: > const IntPoint& outerViewportOffset, > const FloatPoint& pinchViewportOffset > > then you don't need to do any conversions at all in this method. Acknowledged. https://codereview.chromium.org/556703005/diff/1/Source/web/WebViewImpl.cpp#n... Source/web/WebViewImpl.cpp:2970: view->scrollTo(toIntSize(outerViewportOrigin)); On 2014/09/10 15:03:58, bokan wrote: > Use WebViewImpl::updateMainFrameScrollPosition(IntPoint, false) instead. Or > view->setScrollOffset(). ScrollView::scrollTo has a comment not to call from > outside setScrollOffset and looks like it doesn't clamp to scroll bounds (try > going landscape to portrait when fully vertically scrolled). Something, > something, footguns... Yes, I missed that comment. The ScrollView::scrollTo() is public though, from the comment I would assume it should have been protected... The view->setScrollOffset() seems simpler, I will use it unless there is a compelling reason to use WebViewImpl::updateMainFrameScrollPosition()
https://codereview.chromium.org/556703005/diff/1/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/556703005/diff/1/Source/web/WebViewImpl.cpp#n... Source/web/WebViewImpl.cpp:2970: view->scrollTo(toIntSize(outerViewportOrigin)); On 2014/09/10 18:22:40, timav wrote: > On 2014/09/10 15:03:58, bokan wrote: > > Use WebViewImpl::updateMainFrameScrollPosition(IntPoint, false) instead. Or > > view->setScrollOffset(). ScrollView::scrollTo has a comment not to call from > > outside setScrollOffset and looks like it doesn't clamp to scroll bounds (try > > going landscape to portrait when fully vertically scrolled). Something, > > something, footguns... > > Yes, I missed that comment. The ScrollView::scrollTo() is public though, from > the comment I would assume it should have been protected... > The view->setScrollOffset() seems simpler, I will use it unless there is a > compelling reason to use WebViewImpl::updateMainFrameScrollPosition() Yup, highly easy to misuse. If you don't mind, go ahead and make that protected in ScrollView (and FrameView). Thanks.
Hi, I tried to address all the issues except for the tests. The names in the ViewportAnchor class are still mostly "inner" and "outer", I think this make it easier to think about what it is doing, but if you feel strongly, I will rename them. I changed the order of setting scale and location in WebViewImpl::scrollAndRescaleViewports() and it fixed one bug where location was erroneously clamped. I also put a check that the newly computed scale is in bounds. https://codereview.chromium.org/556703005/diff/20001/Source/web/ViewportAncho... File Source/web/ViewportAnchor.cpp (right): https://codereview.chromium.org/556703005/diff/20001/Source/web/ViewportAncho... Source/web/ViewportAnchor.cpp:150: moveToEncloseRect(&outerRect, FloatRect(innerOrigin, FloatSize(innerSize))); At first I thought this move is necessary, but now the whole idea of this move seems incorrect to me. Please confirm that we are never going to scroll the outer viewport horizontally and I will remove it.
Sorry for delay. Functionally, looked good when I tested, thanks! I just have a few minor concerns about edge cases and clamping. https://codereview.chromium.org/556703005/diff/20001/Source/web/ViewportAncho... File Source/web/ViewportAnchor.cpp (right): https://codereview.chromium.org/556703005/diff/20001/Source/web/ViewportAncho... Source/web/ViewportAnchor.cpp:150: moveToEncloseRect(&outerRect, FloatRect(innerOrigin, FloatSize(innerSize))); On 2014/09/11 21:52:05, timav wrote: > At first I thought this move is necessary, but now the whole idea of this move > seems incorrect to me. Please confirm that we are never going to scroll the > outer viewport horizontally and I will remove it. In general, the outer viewport can be scrolled horizontally and vertically, but I'm not sure why you think the horizontal case is special; they should be symmetrical. I do think you may have edge cases where the innerOrigin - absPinchViewportOffset ends up with the outer viewport not bounding the inner so I expect you need this. That said, you're relying on the returned pinchViewportOffset to be clamped by the caller for this to work. I think a better way to do this would be to replace moveToEncloseRect with something like clampPinchViewportOffset(absPinchViewportOffset, innerSize, outerSize) to ensure the inner viewport offset is valid and do that before setting the outerOrigin. https://codereview.chromium.org/556703005/diff/20001/Source/web/ViewportAnchor.h File Source/web/ViewportAnchor.h (right): https://codereview.chromium.org/556703005/diff/20001/Source/web/ViewportAncho... Source/web/ViewportAnchor.h:59: void setAnchor(const IntRect& outerViewRect, const IntRect& innerViewRect, const FloatSize& anchorInViewCoords); The inner rect has scale applied to it so it should be a FloatRect (here and other uses of Inner size/position) https://codereview.chromium.org/556703005/diff/20001/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/556703005/diff/20001/Source/web/WebViewImpl.c... Source/web/WebViewImpl.cpp:1683: localFrameRootTemporary()->frameView()->visibleContentRect(), Just use the local 'view' as before. https://codereview.chromium.org/556703005/diff/20001/Source/web/WebViewImpl.c... Source/web/WebViewImpl.cpp:1710: IntSize mainFrameSize = localFrameRootTemporary()->frameView()->visibleContentRect().size(); ditto here, localFrameRootTemporary()->frameView() --> view https://codereview.chromium.org/556703005/diff/20001/Source/web/WebViewImpl.c... Source/web/WebViewImpl.cpp:2942: // main frame scroll position and pinch viewport scale. The inner viewport is always set relative to the outer viewport so it doesn't matter if you set the inner or outer first. Any time the outer viewport moves, the inner viewport implicitly moves with it (it maintains its relative offset to the outer viewport). However, anytime we scale or move the inner viewport, or resize the outer, we clamp to make sure it remains bounded by the outer, so in general we do need to set the scale before the inner offset. Though if the offset was calculated to take the final scale into account (which I think you do now) that shouldn't matter either. Additionally, we have to consider clamping of outer viewport here. If mainFrameOrigin causes the outer viewport to exceed the document bounds, it'll be clamped so the inner viewport wont end up at the desired offset. You'll need to check the difference between mainFrameOrigin and view->scrollOffset() after setting it and apply the difference to the inner viewport.
https://codereview.chromium.org/556703005/diff/20001/Source/web/ViewportAncho... File Source/web/ViewportAnchor.cpp (right): https://codereview.chromium.org/556703005/diff/20001/Source/web/ViewportAncho... Source/web/ViewportAnchor.cpp:150: moveToEncloseRect(&outerRect, FloatRect(innerOrigin, FloatSize(innerSize))); On 2014/09/12 20:53:07, bokan wrote: > On 2014/09/11 21:52:05, timav wrote: > > At first I thought this move is necessary, but now the whole idea of this move > > seems incorrect to me. Please confirm that we are never going to scroll the > > outer viewport horizontally and I will remove it. > > In general, the outer viewport can be scrolled horizontally and vertically, but > I'm not sure why you think the horizontal case is special; they should be > symmetrical. I do think you may have edge cases where the innerOrigin - > absPinchViewportOffset ends up with the outer viewport not bounding the inner so > I expect you need this. > > That said, you're relying on the returned pinchViewportOffset to be clamped by > the caller for this to work. I think a better way to do this would be to replace > moveToEncloseRect with something like > clampPinchViewportOffset(absPinchViewportOffset, innerSize, outerSize) to ensure > the inner viewport offset is valid and do that before setting the outerOrigin. David, my idea here was different (of course if I get you right): I did not want to clamp inner viewport into some boundaries after calculating its proper position in the document. On the contrary, I wanted to ensure that the outer viewport covers the inner by moving the outer (effectively modifying the relative position of the inner within the outer). The thing is, I never saw it working in practice: I could never made the inner viewport protrude down through the outer viewport lower edge for vertical scrolls; in case of extreme horizontal scrolls I did have this situation and the outer viewport did move to x > 0, but later the ScrollView::setScrollOffset() returned it back to 0. I realized that honestly I would need to take into account the outer viewport constraints by ScrollView::adjustScrollPositionWithinRange(), but then I thought that there are cases where anchor cannot be satisfied and so inner viewport will be clamped, then what's wrong on relying the caller to clamp it (as you say)? This is why I now think that the whole moving idea *in this place in code* is incorrect. https://codereview.chromium.org/556703005/diff/20001/Source/web/ViewportAnchor.h File Source/web/ViewportAnchor.h (right): https://codereview.chromium.org/556703005/diff/20001/Source/web/ViewportAncho... Source/web/ViewportAnchor.h:59: void setAnchor(const IntRect& outerViewRect, const IntRect& innerViewRect, const FloatSize& anchorInViewCoords); On 2014/09/12 20:53:07, bokan wrote: > The inner rect has scale applied to it so it should be a FloatRect (here and > other uses of Inner size/position) Acknowledged. https://codereview.chromium.org/556703005/diff/20001/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/556703005/diff/20001/Source/web/WebViewImpl.c... Source/web/WebViewImpl.cpp:1683: localFrameRootTemporary()->frameView()->visibleContentRect(), On 2014/09/12 20:53:07, bokan wrote: > Just use the local 'view' as before. Acknowledged. https://codereview.chromium.org/556703005/diff/20001/Source/web/WebViewImpl.c... Source/web/WebViewImpl.cpp:1710: IntSize mainFrameSize = localFrameRootTemporary()->frameView()->visibleContentRect().size(); On 2014/09/12 20:53:07, bokan wrote: > ditto here, localFrameRootTemporary()->frameView() --> view Acknowledged. https://codereview.chromium.org/556703005/diff/20001/Source/web/WebViewImpl.c... Source/web/WebViewImpl.cpp:2942: // main frame scroll position and pinch viewport scale. On 2014/09/12 20:53:07, bokan wrote: > The inner viewport is always set relative to the outer viewport so it doesn't > matter if you set the inner or outer first. Any time the outer viewport moves, > the inner viewport implicitly moves with it (it maintains its relative offset to > the outer viewport). I think you are right for the outer viewport, i.e. ScrollView. > However, anytime we scale or move the inner viewport, or > resize the outer, we clamp to make sure it remains bounded by the outer, so in > general we do need to set the scale before the inner offset. Though if the > offset was calculated to take the final scale into account (which I think you do > now) that shouldn't matter either. The scale affects the value of PinchViewport::maximumScrollPosition(): the larger the scale, the smaller is the visibleRect() and the more you are allowed to scroll. In my experiments the scale went approx 2.5 -> 4.0. When the proper scale was not set before setLocation(), the latter was erroneously clamped. This was the bug I was referring to. > Additionally, we have to consider clamping of outer viewport here. If > mainFrameOrigin causes the outer viewport to exceed the document bounds, it'll > be clamped so the inner viewport wont end up at the desired offset. You'll need > to check the difference between mainFrameOrigin and view->scrollOffset() after > setting it and apply the difference to the inner viewport. Yes, I can imagine that view->setScrollOffset() is clamped and implicitly pulls the inner viewport, but maybe the inner could have stayed where it was in the doc (i.e. still within the outer). I was tempted to do this correction in ViewportAchor::computeOrigins(), see my comment for this method.
https://codereview.chromium.org/556703005/diff/20001/Source/web/ViewportAncho... File Source/web/ViewportAnchor.cpp (right): https://codereview.chromium.org/556703005/diff/20001/Source/web/ViewportAncho... Source/web/ViewportAnchor.cpp:150: moveToEncloseRect(&outerRect, FloatRect(innerOrigin, FloatSize(innerSize))); On 2014/09/12 23:33:35, timav wrote: > On 2014/09/12 20:53:07, bokan wrote: > > On 2014/09/11 21:52:05, timav wrote: > > > At first I thought this move is necessary, but now the whole idea of this > move > > > seems incorrect to me. Please confirm that we are never going to scroll the > > > outer viewport horizontally and I will remove it. > > > > In general, the outer viewport can be scrolled horizontally and vertically, > but > > I'm not sure why you think the horizontal case is special; they should be > > symmetrical. I do think you may have edge cases where the innerOrigin - > > absPinchViewportOffset ends up with the outer viewport not bounding the inner > so > > I expect you need this. > > > > That said, you're relying on the returned pinchViewportOffset to be clamped by > > the caller for this to work. I think a better way to do this would be to > replace > > moveToEncloseRect with something like > > clampPinchViewportOffset(absPinchViewportOffset, innerSize, outerSize) to > ensure > > the inner viewport offset is valid and do that before setting the outerOrigin. > > David, my idea here was different (of course if I get you right): I did not want > to clamp inner viewport into some boundaries after calculating its proper > position in the document. On the contrary, I wanted to ensure that the outer > viewport covers the inner by moving the outer (effectively modifying the > relative position of the inner within the outer). > > The thing is, I never saw it working in practice: I could never made the inner > viewport protrude down through the outer viewport lower edge for vertical > scrolls; in case of extreme horizontal scrolls I did have this situation and the > outer viewport did move to x > 0, but later the ScrollView::setScrollOffset() > returned it back to 0. > > I realized that honestly I would need to take into account the outer viewport > constraints by ScrollView::adjustScrollPositionWithinRange(), but then I thought > that there are cases where anchor cannot be satisfied and so inner viewport will > be clamped, then what's wrong on relying the caller to clamp it (as you say)? > > This is why I now think that the whole moving idea *in this place in code* is > incorrect. Correction: I actually wanted to say that this would be code complication that won't give us much (but the code place is right), but of course I'd like to hear your opinion.
https://codereview.chromium.org/556703005/diff/20001/Source/web/ViewportAncho... File Source/web/ViewportAnchor.cpp (right): https://codereview.chromium.org/556703005/diff/20001/Source/web/ViewportAncho... Source/web/ViewportAnchor.cpp:150: moveToEncloseRect(&outerRect, FloatRect(innerOrigin, FloatSize(innerSize))); On 2014/09/12 23:33:35, timav wrote: > On 2014/09/12 20:53:07, bokan wrote: > > On 2014/09/11 21:52:05, timav wrote: > > > At first I thought this move is necessary, but now the whole idea of this > move > > > seems incorrect to me. Please confirm that we are never going to scroll the > > > outer viewport horizontally and I will remove it. > > > > In general, the outer viewport can be scrolled horizontally and vertically, > but > > I'm not sure why you think the horizontal case is special; they should be > > symmetrical. I do think you may have edge cases where the innerOrigin - > > absPinchViewportOffset ends up with the outer viewport not bounding the inner > so > > I expect you need this. > > > > That said, you're relying on the returned pinchViewportOffset to be clamped by > > the caller for this to work. I think a better way to do this would be to > replace > > moveToEncloseRect with something like > > clampPinchViewportOffset(absPinchViewportOffset, innerSize, outerSize) to > ensure > > the inner viewport offset is valid and do that before setting the outerOrigin. > > David, my idea here was different (of course if I get you right): I did not want > to clamp inner viewport into some boundaries after calculating its proper > position in the document. On the contrary, I wanted to ensure that the outer > viewport covers the inner by moving the outer (effectively modifying the > relative position of the inner within the outer). > > The thing is, I never saw it working in practice: I could never made the inner > viewport protrude down through the outer viewport lower edge for vertical > scrolls; in case of extreme horizontal scrolls I did have this situation and the > outer viewport did move to x > 0, but later the ScrollView::setScrollOffset() > returned it back to 0. > > I realized that honestly I would need to take into account the outer viewport > constraints by ScrollView::adjustScrollPositionWithinRange(), but then I thought > that there are cases where anchor cannot be satisfied and so inner viewport will > be clamped, then what's wrong on relying the caller to clamp it (as you say)? > > This is why I now think that the whole moving idea *in this place in code* is > incorrect. Correction 2: I did not realize that if I move the outer rectangle I need to move the inner one in the opposite direction do it will stay in the same place in doc. This is still to precisely clamping, but now I'd just like to apologize for the noise. I'll provide another version and the we'll see whether we need it or not. Sorry.
https://codereview.chromium.org/556703005/diff/20001/Source/web/ViewportAncho... File Source/web/ViewportAnchor.cpp (right): https://codereview.chromium.org/556703005/diff/20001/Source/web/ViewportAncho... Source/web/ViewportAnchor.cpp:150: moveToEncloseRect(&outerRect, FloatRect(innerOrigin, FloatSize(innerSize))); On 2014/09/12 23:33:35, timav wrote: > David, my idea here was different (of course if I get you right): I did not want > to clamp inner viewport into some boundaries after calculating its proper > position in the document. On the contrary, I wanted to ensure that the outer > viewport covers the inner by moving the outer (effectively modifying the > relative position of the inner within the outer). Right, the end result is the same though, we want the inner viewport to be bounded by the outer since otherwise there'll be a clamp. > The thing is, I never saw it working in practice: I could never made the inner > viewport protrude down through the outer viewport lower edge for vertical > scrolls; in case of extreme horizontal scrolls I did have this situation and the > outer viewport did move to x > 0, but later the ScrollView::setScrollOffset() > returned it back to 0. In most cases the outer viewport won't be horizontally scrollable, so that's why you see it return to 0. However, when the page sets an explicit minimum zoom there may be horizontal scrolling of the outer viewport so I think this is necessary. I still think it can be caused in both directions due to symmetry. > > I realized that honestly I would need to take into account the outer viewport > constraints by ScrollView::adjustScrollPositionWithinRange(), but then I thought > that there are cases where anchor cannot be satisfied and so inner viewport will > be clamped, then what's wrong on relying the caller to clamp it (as you say)? The issue is that any clamping on the outer viewport has to be matched by moving the inner viewport in the opposite direction. This is non-obvious and easy to get wrong so I'd rather we do all that in this class; the offsets returned from here should be the final positions of the viewports. In light of this, you'll need an additional rect in the form of the outer viewport's content rect. I think it may make more sense to pass in the FrameView itself (in the form of a ScrollableArea ref). IMO the steps should looks something like: 1) Clamp absPinchViewportOffset so that: absPinchViewportOffset + innerViewportSize <= outerViewportSize 2) Position outer viewport outerRect = innerOrigin - absPinchViewportOffset 3) Clamp outer viewport so that: outerRect.location + outerRect.size <= contentRect.size 4) Apply outer viewport clamp amount in opposite direction to absPinchViewportOffset. You may wish to ASSERT that: outerRect.location + absPinchViewportOffset == innerOrigin But feel free to do it as you see fit, I'll leave the details to you :) https://codereview.chromium.org/556703005/diff/20001/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/556703005/diff/20001/Source/web/WebViewImpl.c... Source/web/WebViewImpl.cpp:2942: // main frame scroll position and pinch viewport scale. On 2014/09/12 23:33:35, timav wrote: > > The scale affects the value of PinchViewport::maximumScrollPosition(): the > larger the scale, the smaller is the visibleRect() and the more you are allowed > to scroll. In my experiments the scale went approx 2.5 -> 4.0. When the proper > scale was not set before setLocation(), the latter was erroneously clamped. This > was the bug I was referring to. You're right, I misspoke, the scale needs to be applied before the offset for this reason. > > Yes, I can imagine that view->setScrollOffset() is clamped and implicitly pulls > the inner viewport, but maybe the inner could have stayed where it was in the > doc (i.e. still within the outer). > I was tempted to do this correction in ViewportAchor::computeOrigins(), see my > comment for this method. Yes, I think it's best to do all the corrections in ViewportAnchor. See my comments there.
https://codereview.chromium.org/556703005/diff/20001/Source/web/ViewportAncho... File Source/web/ViewportAnchor.cpp (right): https://codereview.chromium.org/556703005/diff/20001/Source/web/ViewportAncho... Source/web/ViewportAnchor.cpp:150: moveToEncloseRect(&outerRect, FloatRect(innerOrigin, FloatSize(innerSize))); On 2014/09/15 15:36:35, bokan wrote: > On 2014/09/12 23:33:35, timav wrote: > > David, my idea here was different (of course if I get you right): I did not > want > > to clamp inner viewport into some boundaries after calculating its proper > > position in the document. On the contrary, I wanted to ensure that the outer > > viewport covers the inner by moving the outer (effectively modifying the > > relative position of the inner within the outer). > > Right, the end result is the same though, we want the inner viewport to be > bounded by the outer since otherwise there'll be a clamp. > > > The thing is, I never saw it working in practice: I could never made the inner > > viewport protrude down through the outer viewport lower edge for vertical > > scrolls; in case of extreme horizontal scrolls I did have this situation and > the > > outer viewport did move to x > 0, but later the ScrollView::setScrollOffset() > > returned it back to 0. > > In most cases the outer viewport won't be horizontally scrollable, so that's > why you see it return to 0. However, when the page sets an explicit minimum > zoom there may be horizontal scrolling of the outer viewport so I think this > is necessary. I still think it can be caused in both directions due to symmetry. > > > > > > I realized that honestly I would need to take into account the outer viewport > > constraints by ScrollView::adjustScrollPositionWithinRange(), but then I > thought > > that there are cases where anchor cannot be satisfied and so inner viewport > will > > be clamped, then what's wrong on relying the caller to clamp it (as you say)? > > The issue is that any clamping on the outer viewport has to be matched by moving > the inner viewport in the opposite direction. Yes, absolutely. I realized it after I submitted my last lengthy comment. > This is non-obvious and easy to > get wrong so I'd rather we do all that in this class; the offsets returned > from here should be the final positions of the viewports. > > In light of this, you'll need an additional rect in the form of the outer > viewport's content rect. I think it may make more sense to pass in the > FrameView itself (in the form of a ScrollableArea ref). IMO the steps should > looks something like: > > 1) Clamp absPinchViewportOffset so that: > absPinchViewportOffset + innerViewportSize <= outerViewportSize > > 2) Position outer viewport > outerRect = innerOrigin - absPinchViewportOffset > > 3) Clamp outer viewport so that: > outerRect.location + outerRect.size <= contentRect.size > > 4) Apply outer viewport clamp amount in opposite direction to > absPinchViewportOffset. You may wish to ASSERT that: > > outerRect.location + absPinchViewportOffset == innerOrigin > > But feel free to do it as you see fit, I'll leave the details to you :) I passed the view as a const ScrollView * since I use adjustScrollPositionWithinRange(). The rest seems to be more or less staightforward, I get the locations of both viewports in the reference frame of the document, then the pinchViewportOffset is the difference between them. Please review the next patch. https://codereview.chromium.org/556703005/diff/20001/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/556703005/diff/20001/Source/web/WebViewImpl.c... Source/web/WebViewImpl.cpp:1683: localFrameRootTemporary()->frameView()->visibleContentRect(), On 2014/09/12 23:33:35, timav wrote: > On 2014/09/12 20:53:07, bokan wrote: > > Just use the local 'view' as before. > > Acknowledged. Done. https://codereview.chromium.org/556703005/diff/20001/Source/web/WebViewImpl.c... Source/web/WebViewImpl.cpp:1710: IntSize mainFrameSize = localFrameRootTemporary()->frameView()->visibleContentRect().size(); On 2014/09/12 23:33:35, timav wrote: > On 2014/09/12 20:53:07, bokan wrote: > > ditto here, localFrameRootTemporary()->frameView() --> view > > Acknowledged. Done. https://codereview.chromium.org/556703005/diff/20001/Source/web/WebViewImpl.c... Source/web/WebViewImpl.cpp:2942: // main frame scroll position and pinch viewport scale. On 2014/09/15 15:36:35, bokan wrote: > On 2014/09/12 23:33:35, timav wrote: > > > > The scale affects the value of PinchViewport::maximumScrollPosition(): the > > larger the scale, the smaller is the visibleRect() and the more you are > allowed > > to scroll. In my experiments the scale went approx 2.5 -> 4.0. When the proper > > scale was not set before setLocation(), the latter was erroneously clamped. > This > > was the bug I was referring to. > > You're right, I misspoke, the scale needs to be applied before the offset for > this reason. > > > > > Yes, I can imagine that view->setScrollOffset() is clamped and implicitly > pulls > > the inner viewport, but maybe the inner could have stayed where it was in the > > doc (i.e. still within the outer). > > I was tempted to do this correction in ViewportAchor::computeOrigins(), see my > > comment for this method. > > Yes, I think it's best to do all the corrections in ViewportAnchor. See my > comments there. > Done.
lgtm modulo comments https://codereview.chromium.org/556703005/diff/40001/Source/web/ViewportAncho... File Source/web/ViewportAnchor.cpp (right): https://codereview.chromium.org/556703005/diff/40001/Source/web/ViewportAncho... Source/web/ViewportAnchor.cpp:94: // PinchViewport::maximimScrollPosition() does the same. nit: typo on maximim -> maximum https://codereview.chromium.org/556703005/diff/40001/Source/web/ViewportAncho... Source/web/ViewportAnchor.cpp:156: const IntSize& outerSize, const FloatSize& innerSize, You can get outerSize from frameView. Remove the outerSize param. Also, pass frameView by const reference rather than pointer and remove the ASSERT below. https://codereview.chromium.org/556703005/diff/40001/Source/web/ViewportAncho... Source/web/ViewportAnchor.cpp:182: *pinchViewportOffset = FloatPoint(innerRect.location() - FloatPoint(outerRect.location())); You can remove the outer-most FloatPoint(...)
And tests, of course :)
Working on a test now. https://codereview.chromium.org/556703005/diff/40001/Source/web/ViewportAncho... File Source/web/ViewportAnchor.cpp (right): https://codereview.chromium.org/556703005/diff/40001/Source/web/ViewportAncho... Source/web/ViewportAnchor.cpp:94: // PinchViewport::maximimScrollPosition() does the same. On 2014/09/15 21:02:20, bokan wrote: > nit: typo on maximim -> maximum Done. https://codereview.chromium.org/556703005/diff/40001/Source/web/ViewportAncho... Source/web/ViewportAnchor.cpp:156: const IntSize& outerSize, const FloatSize& innerSize, On 2014/09/15 21:02:21, bokan wrote: > You can get outerSize from frameView. Remove the outerSize param. Also, pass > frameView by const reference rather than pointer and remove the ASSERT below. Done. https://codereview.chromium.org/556703005/diff/40001/Source/web/ViewportAncho... Source/web/ViewportAnchor.cpp:182: *pinchViewportOffset = FloatPoint(innerRect.location() - FloatPoint(outerRect.location())); On 2014/09/15 21:02:20, bokan wrote: > You can remove the outer-most FloatPoint(...) FloatPoint - FloatPoint = FloatSize, but pinchViewportOffset is a point, so outer-most cast is needed. But the second cast can be removed, indeed, since there is an implicit conversion from IntPoint for FloatPoint.
Submitted the unit tests, please review.
tests lgtm. https://codereview.chromium.org/556703005/diff/80001/Source/web/tests/PinchVi... File Source/web/tests/PinchViewportTest.cpp (right): https://codereview.chromium.org/556703005/diff/80001/Source/web/tests/PinchVi... Source/web/tests/PinchViewportTest.cpp:162: settings->setMainFrameResizesAreOrientationChanges(true); Does anything break if you just add setMainFrameResizesAreOrientationChanges in configureAndroidSettings and use that? We should use that to get as close to the Android config as possible. https://codereview.chromium.org/556703005/diff/80001/Source/web/tests/PinchVi... Source/web/tests/PinchViewportTest.cpp:225: --------------------- --------------------- ASCII art is super helpful, nice! The anchor should be at x: 0.5 though right? https://codereview.chromium.org/556703005/diff/80001/Source/web/tests/PinchVi... Source/web/tests/PinchViewportTest.cpp:229: initializeWithDesktopSettings(resizesAreOrientationChanges); use initializeWithAndroidSettings() and move the mainFrameResizesAreOrientationChanges in there. https://codereview.chromium.org/556703005/diff/80001/Source/web/tests/PinchVi... Source/web/tests/PinchViewportTest.cpp:282: webViewImpl()->layout(); you shouldn't need this layout (there's a layout call inside resize() if viewport is enabled).
https://codereview.chromium.org/556703005/diff/80001/Source/web/tests/PinchVi... File Source/web/tests/PinchViewportTest.cpp (right): https://codereview.chromium.org/556703005/diff/80001/Source/web/tests/PinchVi... Source/web/tests/PinchViewportTest.cpp:162: settings->setMainFrameResizesAreOrientationChanges(true); On 2014/09/16 19:46:48, bokan wrote: > Does anything break if you just add setMainFrameResizesAreOrientationChanges in > configureAndroidSettings and use that? We should use that to get as close to the > Android config as possible. Nothing breaks in the existing tests, but for my own ones I had to add a line webViewImpl()->setPageScaleFactorLimits(1, 4); https://codereview.chromium.org/556703005/diff/80001/Source/web/tests/PinchVi... Source/web/tests/PinchViewportTest.cpp:225: --------------------- --------------------- On 2014/09/16 19:46:48, bokan wrote: > ASCII art is super helpful, nice! The anchor should be at x: 0.5 though right? Right, and I keep forgetting about that. I can move the "o". On the other hand, I was useful for me to mark the inner origin in the document that we are going to approximately preserve. For horizontal scroll case I tried to show that we can't do it due to constrains. https://codereview.chromium.org/556703005/diff/80001/Source/web/tests/PinchVi... Source/web/tests/PinchViewportTest.cpp:229: initializeWithDesktopSettings(resizesAreOrientationChanges); On 2014/09/16 19:46:48, bokan wrote: > use initializeWithAndroidSettings() and move the > mainFrameResizesAreOrientationChanges in there. Done. https://codereview.chromium.org/556703005/diff/80001/Source/web/tests/PinchVi... Source/web/tests/PinchViewportTest.cpp:282: webViewImpl()->layout(); On 2014/09/16 19:46:48, bokan wrote: > you shouldn't need this layout (there's a layout call inside resize() if > viewport is enabled). Done.
https://codereview.chromium.org/556703005/diff/80001/Source/web/tests/PinchVi... File Source/web/tests/PinchViewportTest.cpp (right): https://codereview.chromium.org/556703005/diff/80001/Source/web/tests/PinchVi... Source/web/tests/PinchViewportTest.cpp:162: settings->setMainFrameResizesAreOrientationChanges(true); On 2014/09/16 21:44:51, timav wrote: > On 2014/09/16 19:46:48, bokan wrote: > > Does anything break if you just add setMainFrameResizesAreOrientationChanges > in > > configureAndroidSettings and use that? We should use that to get as close to > the > > Android config as possible. > > Nothing breaks in the existing tests, but for my own ones I had to add a line > webViewImpl()->setPageScaleFactorLimits(1, 4); Hmm...you shouldn't need that. What was the problem that was happening? The layout() in resize() should trigger refreshPageScaleFactorAfterLayout() which calls: updatePageDefinedViewportConstraints(mainFrameImpl()->frame()->document()->viewportDescription()); m_pageScaleConstraintsSet.computeFinalConstraints(); That should set up the Android default limits.
Please review another patch. Thank you, --Tima
lgtm with style nits https://codereview.chromium.org/556703005/diff/120001/Source/web/ViewportAnch... File Source/web/ViewportAnchor.cpp (right): https://codereview.chromium.org/556703005/diff/120001/Source/web/ViewportAnch... Source/web/ViewportAnchor.cpp:72: void moveToEncloseRect(IntRect * outer, const FloatRect & inner) nit: IntRect*, FloatRect& https://codereview.chromium.org/556703005/diff/120001/Source/web/ViewportAnch... Source/web/ViewportAnchor.cpp:86: void moveIntoRect(FloatRect * inner, const IntRect & outer) nit: FloatRect*, IntRect& https://codereview.chromium.org/556703005/diff/120001/Source/web/ViewportAnch... Source/web/ViewportAnchor.cpp:167: // Create the outer and inner rectangles that represent the viewports. nit: all five comments here are a bit too micro and no-added-information, please delete them. I prefer comments to explain higher-level concepts or nonobvious properties, for this we can let the variable/method names speak for themselves https://codereview.chromium.org/556703005/diff/120001/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (left): https://codereview.chromium.org/556703005/diff/120001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:2963: nit: unnecessary newline https://codereview.chromium.org/556703005/diff/120001/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/556703005/diff/120001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:1717: FloatSize pinchViewportSize = FloatSize(newSize); nit: second FloatSize() shouldn't be necessary, please delete it. initialization "=" is shorthand for constructor invocation, if I recall my C++ correctly. https://codereview.chromium.org/556703005/diff/120001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:2942: const IntPoint & mainFrameOrigin, nit: "IntPoint& mainFrameOrigin" (no space before) https://codereview.chromium.org/556703005/diff/120001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:2964: // Set outer viewport position. Please delete the three comments starting here. But it's fine to keep the "Order is important" comment.
Another round. https://codereview.chromium.org/556703005/diff/120001/Source/web/ViewportAnch... File Source/web/ViewportAnchor.cpp (right): https://codereview.chromium.org/556703005/diff/120001/Source/web/ViewportAnch... Source/web/ViewportAnchor.cpp:72: void moveToEncloseRect(IntRect * outer, const FloatRect & inner) On 2014/09/17 05:16:36, aelias wrote: > nit: IntRect*, FloatRect& Done. https://codereview.chromium.org/556703005/diff/120001/Source/web/ViewportAnch... Source/web/ViewportAnchor.cpp:86: void moveIntoRect(FloatRect * inner, const IntRect & outer) On 2014/09/17 05:16:36, aelias wrote: > nit: FloatRect*, IntRect& Done. https://codereview.chromium.org/556703005/diff/120001/Source/web/ViewportAnch... Source/web/ViewportAnchor.cpp:167: // Create the outer and inner rectangles that represent the viewports. On 2014/09/17 05:16:36, aelias wrote: > nit: all five comments here are a bit too micro and no-added-information, please > delete them. I prefer comments to explain higher-level concepts or nonobvious > properties, for this we can let the variable/method names speak for themselves Done. https://codereview.chromium.org/556703005/diff/120001/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (left): https://codereview.chromium.org/556703005/diff/120001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:2963: On 2014/09/17 05:16:36, aelias wrote: > nit: unnecessary newline Newline removed in the new version. https://codereview.chromium.org/556703005/diff/120001/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/556703005/diff/120001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:1717: FloatSize pinchViewportSize = FloatSize(newSize); On 2014/09/17 05:16:36, aelias wrote: > nit: second FloatSize() shouldn't be necessary, please delete it. > initialization "=" is shorthand for constructor invocation, if I recall my C++ > correctly. The cast is required to convert WebSize -> IntSize -> FloatSize, I guess C++ does implicit conversions only if there is a direct path from A to B. IntSize() works here too, but compiler complains if there is no cast at all. https://codereview.chromium.org/556703005/diff/120001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:2942: const IntPoint & mainFrameOrigin, On 2014/09/17 05:16:36, aelias wrote: > nit: "IntPoint& mainFrameOrigin" (no space before) Done. https://codereview.chromium.org/556703005/diff/120001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:2964: // Set outer viewport position. On 2014/09/17 05:16:36, aelias wrote: > Please delete the three comments starting here. But it's fine to keep the > "Order is important" comment. Done.
The CQ bit was checked by timav@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/556703005/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: blink_presubmit on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/1...)
FYI: You still need an OWNER for Source/core and Source/platform
timav@google.com changed reviewers: + leviw@chromium.org
Hi Levi, I need a reviewer who owns the directories Source/core and Source/platform. Could you, please, take a look at this change? Thank you, --Tima
Typo in description: "ancor" https://codereview.chromium.org/556703005/diff/140001/Source/web/ViewportAnch... File Source/web/ViewportAnchor.cpp (right): https://codereview.chromium.org/556703005/diff/140001/Source/web/ViewportAnch... Source/web/ViewportAnchor.cpp:156: IntPoint* mainFrameOffset, FloatPoint* pinchViewportOffset) const FrameView is another class apart from ScrollView. Don't confuse them here. https://codereview.chromium.org/556703005/diff/140001/Source/web/ViewportAnch... File Source/web/ViewportAnchor.h (right): https://codereview.chromium.org/556703005/diff/140001/Source/web/ViewportAnch... Source/web/ViewportAnchor.h:62: void computeOrigins(const ScrollView& frameView, const FloatSize& innerSize, ScrollView& frameView? Really you should leave that identifier out of the header. https://codereview.chromium.org/556703005/diff/140001/Source/web/ViewportAnch... Source/web/ViewportAnchor.h:63: IntPoint* mainFrameOffset, FloatPoint* pinchViewportOffset) const; These appear to never be null. Why are you passing them as pointers instead of references?
Is this still an "initial draft"?
Updated the description - please review again. https://codereview.chromium.org/556703005/diff/140001/Source/web/ViewportAnch... File Source/web/ViewportAnchor.cpp (right): https://codereview.chromium.org/556703005/diff/140001/Source/web/ViewportAnch... Source/web/ViewportAnchor.cpp:156: IntPoint* mainFrameOffset, FloatPoint* pinchViewportOffset) const On 2014/09/18 18:49:54, leviw wrote: > FrameView is another class apart from ScrollView. Don't confuse them here. Done. https://codereview.chromium.org/556703005/diff/140001/Source/web/ViewportAnch... File Source/web/ViewportAnchor.h (right): https://codereview.chromium.org/556703005/diff/140001/Source/web/ViewportAnch... Source/web/ViewportAnchor.h:62: void computeOrigins(const ScrollView& frameView, const FloatSize& innerSize, On 2014/09/18 18:49:54, leviw wrote: > ScrollView& frameView? Really you should leave that identifier out of the > header. Done. https://codereview.chromium.org/556703005/diff/140001/Source/web/ViewportAnch... Source/web/ViewportAnchor.h:63: IntPoint* mainFrameOffset, FloatPoint* pinchViewportOffset) const; On 2014/09/18 18:49:54, leviw wrote: > These appear to never be null. Why are you passing them as pointers instead of > references? I'm passing pointers because I think it would better indicate to a caller that they are output parameters. f() { ... g(a, &b); ... } versus f() { ... g(a, b); ... } I think the first variant better describes my intention to modify 'b'. Of course if there is a WebKit policy to always use references in such cases or if you strongly prefer the second way I'll change the code.
On 2014/09/19 at 17:38:43, timav wrote: > Updated the description - please review again. > > https://codereview.chromium.org/556703005/diff/140001/Source/web/ViewportAnch... > File Source/web/ViewportAnchor.cpp (right): > > https://codereview.chromium.org/556703005/diff/140001/Source/web/ViewportAnch... > Source/web/ViewportAnchor.cpp:156: IntPoint* mainFrameOffset, FloatPoint* pinchViewportOffset) const > On 2014/09/18 18:49:54, leviw wrote: > > FrameView is another class apart from ScrollView. Don't confuse them here. > > Done. > > https://codereview.chromium.org/556703005/diff/140001/Source/web/ViewportAnch... > File Source/web/ViewportAnchor.h (right): > > https://codereview.chromium.org/556703005/diff/140001/Source/web/ViewportAnch... > Source/web/ViewportAnchor.h:62: void computeOrigins(const ScrollView& frameView, const FloatSize& innerSize, > On 2014/09/18 18:49:54, leviw wrote: > > ScrollView& frameView? Really you should leave that identifier out of the > > header. > > Done. > > https://codereview.chromium.org/556703005/diff/140001/Source/web/ViewportAnch... > Source/web/ViewportAnchor.h:63: IntPoint* mainFrameOffset, FloatPoint* pinchViewportOffset) const; > On 2014/09/18 18:49:54, leviw wrote: > > These appear to never be null. Why are you passing them as pointers instead of > > references? > > I'm passing pointers because I think it would better indicate to a caller that they are output parameters. > f() { ... g(a, &b); ... } > versus > f() { ... g(a, b); ... } > I think the first variant better describes my intention to modify 'b'. > > Of course if there is a WebKit policy to always use references in such cases or if you strongly prefer the second way I'll change the code. We use references for out-parameters in Blink: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/YrWxDDGyDMc
On 2014/09/19 18:43:04, leviw wrote: > On 2014/09/19 at 17:38:43, timav wrote: > > Updated the description - please review again. > > > > > https://codereview.chromium.org/556703005/diff/140001/Source/web/ViewportAnch... > > File Source/web/ViewportAnchor.cpp (right): > > > > > https://codereview.chromium.org/556703005/diff/140001/Source/web/ViewportAnch... > > Source/web/ViewportAnchor.cpp:156: IntPoint* mainFrameOffset, FloatPoint* > pinchViewportOffset) const > > On 2014/09/18 18:49:54, leviw wrote: > > > FrameView is another class apart from ScrollView. Don't confuse them here. > > > > Done. > > > > > https://codereview.chromium.org/556703005/diff/140001/Source/web/ViewportAnch... > > File Source/web/ViewportAnchor.h (right): > > > > > https://codereview.chromium.org/556703005/diff/140001/Source/web/ViewportAnch... > > Source/web/ViewportAnchor.h:62: void computeOrigins(const ScrollView& > frameView, const FloatSize& innerSize, > > On 2014/09/18 18:49:54, leviw wrote: > > > ScrollView& frameView? Really you should leave that identifier out of the > > > header. > > > > Done. > > > > > https://codereview.chromium.org/556703005/diff/140001/Source/web/ViewportAnch... > > Source/web/ViewportAnchor.h:63: IntPoint* mainFrameOffset, FloatPoint* > pinchViewportOffset) const; > > On 2014/09/18 18:49:54, leviw wrote: > > > These appear to never be null. Why are you passing them as pointers instead > of > > > references? > > > > I'm passing pointers because I think it would better indicate to a caller that > they are output parameters. > > f() { ... g(a, &b); ... } > > versus > > f() { ... g(a, b); ... } > > I think the first variant better describes my intention to modify 'b'. > > > > Of course if there is a WebKit policy to always use references in such cases > or if you strongly prefer the second way I'll change the code. > > We use references for out-parameters in Blink: > https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/YrWxDDGyDMc Thank you for the link. I changed the code.
Passed by reference.
lgtm with one more nit. https://codereview.chromium.org/556703005/diff/180001/Source/web/ViewportAnch... File Source/web/ViewportAnchor.cpp (right): https://codereview.chromium.org/556703005/diff/180001/Source/web/ViewportAnch... Source/web/ViewportAnchor.cpp:72: void moveToEncloseRect(IntRect* outer, const FloatRect& inner) You're actually doing the same thing here. At a minimum, this and moveIntoRect should be marked static.
On 2014/09/19 19:03:06, leviw wrote: > lgtm with one more nit. > > https://codereview.chromium.org/556703005/diff/180001/Source/web/ViewportAnch... > File Source/web/ViewportAnchor.cpp (right): > > https://codereview.chromium.org/556703005/diff/180001/Source/web/ViewportAnch... > Source/web/ViewportAnchor.cpp:72: void moveToEncloseRect(IntRect* outer, const > FloatRect& inner) > You're actually doing the same thing here. > > At a minimum, this and moveIntoRect should be marked static. They're in an anonymous namespace, IIRC that's equivalent in C++ as making it static.
On 2014/09/19 at 19:14:45, bokan wrote: > On 2014/09/19 19:03:06, leviw wrote: > > lgtm with one more nit. > > > > https://codereview.chromium.org/556703005/diff/180001/Source/web/ViewportAnch... > > File Source/web/ViewportAnchor.cpp (right): > > > > https://codereview.chromium.org/556703005/diff/180001/Source/web/ViewportAnch... > > Source/web/ViewportAnchor.cpp:72: void moveToEncloseRect(IntRect* outer, const > > FloatRect& inner) > > You're actually doing the same thing here. > > > > At a minimum, this and moveIntoRect should be marked static. > > They're in an anonymous namespace, IIRC that's equivalent in C++ as making it static. It's more explicit and consistent with other Blink code.
On 2014/09/19 20:11:16, leviw wrote: > On 2014/09/19 at 19:14:45, bokan wrote: > > On 2014/09/19 19:03:06, leviw wrote: > > > lgtm with one more nit. > > > > > > > https://codereview.chromium.org/556703005/diff/180001/Source/web/ViewportAnch... > > > File Source/web/ViewportAnchor.cpp (right): > > > > > > > https://codereview.chromium.org/556703005/diff/180001/Source/web/ViewportAnch... > > > Source/web/ViewportAnchor.cpp:72: void moveToEncloseRect(IntRect* outer, > const > > > FloatRect& inner) > > > You're actually doing the same thing here. Yes, you are right, I have changed pointers to references there, too. > > > > > > At a minimum, this and moveIntoRect should be marked static. > > > > They're in an anonymous namespace, IIRC that's equivalent in C++ as making it > static. > > It's more explicit and consistent with other Blink code. Here I'm a little bit confused: anonymous namespace seems to be used in blink pretty widely: the command "find . -name *.cpp | xargs grep 'namespace {$' | wc -l" returns 363. If I exclude tests like this: "find . -name *.cpp | grep -v [tT]est | xargs grep 'namespace {$' | wc -l" it returns 168. I'd like to know more what can be wrong with them. I'll try to submit the latest patch meanwhile, do stop me if you find it wrong.
The CQ bit was checked by timav@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/556703005/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)
The CQ bit was checked by timav@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/556703005/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)
The CQ bit was checked by timav@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/556703005/200001
Message was sent while issue was closed.
Committed patchset #11 (id:200001) as 182365
Message was sent while issue was closed.
A revert of this CL (patchset #11 id:200001) has been created in https://codereview.chromium.org/597113002/ by ager@chromium.org. The reason for reverting is: The new test is failing on mac. Reverting for now so it can be investigated. [ FAILED ] PinchViewportTest.TestResizeAfterHorizontalScroll (39 ms) [1471/1471] PinchViewportTest.TestResizeAfterHorizontalScroll (39 ms) Retrying 2 tests (retry #3) [ RUN ] PinchViewportTest.TestResizeAfterVerticalScroll ../../third_party/WebKit/Source/web/tests/PinchViewportTest.cpp:251: Failure Value of: (pinchViewport.visibleRect().size()).width() Actual: 45.94595 Expected: (FloatSize(50, 25)).width() Which is: 50 ../../third_party/WebKit/Source/web/tests/PinchViewportTest.cpp:251: Failure Value of: (pinchViewport.visibleRect().size()).height() Actual: 22.972975 Expected: (FloatSize(50, 25)).height() Which is: 25 ../../third_party/WebKit/Source/web/tests/PinchViewportTest.cpp:253: Failure Value of: (frame()->view()->scrollPosition()).y() Actual: 638 Expected: (IntPoint(0, 625)).y() Which is: 625 ../../third_party/WebKit/Source/web/tests/PinchViewportTest.cpp:254: Failure Value of: (pinchViewport.location()).y() Actual: 62 Expected: (FloatPoint(0, 75)).y() Which is: 75.
Message was sent while issue was closed.
Sorry about that. I don't think this one needs to be reverted. I think it was the other one causing the issue. I'm reapplying this one to see how that goes.
Message was sent while issue was closed.
On 2014/09/24 11:15:06, Mads Ager (chromium) wrote: > Sorry about that. I don't think this one needs to be reverted. I think it was > the other one causing the issue. I'm reapplying this one to see how that goes. And I pulled it out again. It still causes webkit_unit_test failures on mac. :( Dreadfully sorry about the mess.
Message was sent while issue was closed.
On 2014/09/24 11:50:13, Mads Ager (chromium) wrote: > On 2014/09/24 11:15:06, Mads Ager (chromium) wrote: > > Sorry about that. I don't think this one needs to be reverted. I think it was > > the other one causing the issue. I'm reapplying this one to see how that goes. > > And I pulled it out again. It still causes webkit_unit_test failures on mac. :( > > Dreadfully sorry about the mess. I ran the weblit_unit_tests on mac and could not reproduce the failure. Reapplying in a hope that that failure was transient. If it persists, I will disable the test on mac.
The CQ bit was checked by timav@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/556703005/220001
Message was sent while issue was closed.
Committed patchset #12 (id:220001) as 182727 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
