Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
13704012:
Improve mobile device rotation behavior. (Closed)
|
Created:
7 years, 8 months ago by jdduke (slow) Modified:
7 years, 8 months ago CC:
blink-reviews Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionImprove mobile device rotation behavior.
Properly track viewport contents during resizes triggered by device rotation.
BUG=135962
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=148734
Patch Set 1 #
Total comments: 38
Patch Set 2 : Additional unit tests and general cleanup #Patch Set 3 : Rebase #
Total comments: 4
Patch Set 4 : Refactoring. #
Total comments: 10
Patch Set 5 : Code review updates #
Total comments: 4
Patch Set 6 : Logic cleanup #
Total comments: 6
Patch Set 7 : Fixing rebase and nits #
Total comments: 6
Patch Set 8 : Style cleanup #Patch Set 9 : Fixing test merge #Patch Set 10 : Rebase #Patch Set 11 : git cl upload with --similarity=70 #Messages
Total messages: 24 (0 generated)
Alex, any suggestions for additional reviewers?
Bunch of nits below, but at a high level, this needs more test coverage to test the different branches of logic. I'd like to see expectations for page scale factor, and another test for width="device-width" pages. (For OWNERS approval, you can add abarth@). https://codereview.chromium.org/13704012/diff/1/Source/WebKit/chromium/src/We... File Source/WebKit/chromium/src/WebViewImpl.cpp (right): https://codereview.chromium.org/13704012/diff/1/Source/WebKit/chromium/src/We... Source/WebKit/chromium/src/WebViewImpl.cpp:227: ASSERT(m_eventHandler); This type of assertion is not an idiom in WebKit, please remove. We'll just hit the null pointer dereference later anyway. https://codereview.chromium.org/13704012/diff/1/Source/WebKit/chromium/src/We... Source/WebKit/chromium/src/WebViewImpl.cpp:245: FloatSize anchorOffset = FloatSize(viewRect.size()); = viewRect.size() https://codereview.chromium.org/13704012/diff/1/Source/WebKit/chromium/src/We... Source/WebKit/chromium/src/WebViewImpl.cpp:254: m_anchorNodeBounds = node->Node::boundingBox(); node->boundingBox(). The Node:: you copied from somewhere was needed at one point to work around a bug, but no longer. https://codereview.chromium.org/13704012/diff/1/Source/WebKit/chromium/src/We... Source/WebKit/chromium/src/WebViewImpl.cpp:257: m_anchorInNodeCoords.scale(1.f / m_anchorNodeBounds.width(), 1.f / m_anchorNodeBounds.height()); Delete .f https://codereview.chromium.org/13704012/diff/1/Source/WebKit/chromium/src/We... Source/WebKit/chromium/src/WebViewImpl.cpp:260: // Note: No guarantees are made on the validity of the returned point. This comment is too scary and not very useful. Can you switch to saying "The returned point may not be clamped to document bounds" since I think that's what you mean? https://codereview.chromium.org/13704012/diff/1/Source/WebKit/chromium/src/We... Source/WebKit/chromium/src/WebViewImpl.cpp:268: const LayoutRect currentNodeBounds = m_anchorNode->Node::boundingBox(); m_anchorNode->boundingBox() https://codereview.chromium.org/13704012/diff/1/Source/WebKit/chromium/src/We... Source/WebKit/chromium/src/WebViewImpl.cpp:278: FloatSize anchorOffsetFromOrigin = FloatSize(currentViewSize); = currentViewSize; https://codereview.chromium.org/13704012/diff/1/Source/WebKit/chromium/src/We... Source/WebKit/chromium/src/WebViewImpl.cpp:285: ViewportAnchor& operator=(ViewportAnchor&); Not a common idiom to have a private operator=, please delete. https://codereview.chromium.org/13704012/diff/1/Source/WebKit/chromium/src/We... Source/WebKit/chromium/src/WebViewImpl.cpp:294: if (node && node->Node::pixelSnappedBoundingBox().contains(viewRect)) { node->boundingBox() ? https://codereview.chromium.org/13704012/diff/1/Source/WebKit/chromium/src/We... Source/WebKit/chromium/src/WebViewImpl.cpp:297: IntPoint shiftedPoint = point + pointOffset; Please delete this local variable. https://codereview.chromium.org/13704012/diff/1/Source/WebKit/chromium/src/We... Source/WebKit/chromium/src/WebViewImpl.cpp:301: while (node && node->Node::boundingBox().isEmpty()) Switch to node->boundingBox(). https://codereview.chromium.org/13704012/diff/1/Source/WebKit/chromium/src/We... Source/WebKit/chromium/src/WebViewImpl.cpp:1790: viewportAnchor.setAnchor(view->visibleContentRect(), FloatSize(0.5f, 0)); FloatSize(0.5f, 0) looks like a magic number, please put in a constant. https://codereview.chromium.org/13704012/diff/1/Source/WebKit/chromium/src/We... Source/WebKit/chromium/src/WebViewImpl.cpp:1811: // When the device rotates: This comment is out of date, could you delete/update? https://codereview.chromium.org/13704012/diff/1/Source/WebKit/chromium/src/We... Source/WebKit/chromium/src/WebViewImpl.cpp:1827: float scaleMultiplier = m_minimumPageScaleFactor == oldMinimumPageScaleFactor ? Too much for ternary operator, please make into an if statement. https://codereview.chromium.org/13704012/diff/1/Source/WebKit/chromium/tests/... File Source/WebKit/chromium/tests/WebFrameTest.cpp (right): https://codereview.chromium.org/13704012/diff/1/Source/WebKit/chromium/tests/... Source/WebKit/chromium/tests/WebFrameTest.cpp:581: relativeOffset.scale(1.f / rect.width(), 1.f / rect.height()); Delete .f https://codereview.chromium.org/13704012/diff/1/Source/WebKit/chromium/tests/... Source/WebKit/chromium/tests/WebFrameTest.cpp:594: m_webView->enableFixedLayoutMode(false); This is odd, on Android fixedLayoutMode setting is always true. This should be true. If it breaks your test, something is wrong with the test :). https://codereview.chromium.org/13704012/diff/1/Source/WebKit/chromium/tests/... Source/WebKit/chromium/tests/WebFrameTest.cpp:600: ASSERT_EQ(WebSize(), webViewImpl->mainFrame()->scrollOffset()); Delete this assert. https://codereview.chromium.org/13704012/diff/1/Source/WebKit/chromium/tests/... Source/WebKit/chromium/tests/WebFrameTest.cpp:608: ASSERT_EQ(scrollOffset, webViewImpl->mainFrame()->scrollOffset()); Delete this assert. https://codereview.chromium.org/13704012/diff/1/Source/WebKit/chromium/tests/... Source/WebKit/chromium/tests/WebFrameTest.cpp:620: = computeRelativeOffset(newAnchorPoint, anchorNode->Node::boundingBox()); anchorNode->boundingBox()
Adding some more tests for fixed layout and width=device-width, with pageScaleFactor checks. Thanks. https://codereview.chromium.org/13704012/diff/1/Source/WebKit/chromium/src/We... File Source/WebKit/chromium/src/WebViewImpl.cpp (right): https://codereview.chromium.org/13704012/diff/1/Source/WebKit/chromium/src/We... Source/WebKit/chromium/src/WebViewImpl.cpp:227: ASSERT(m_eventHandler); On 2013/04/08 04:24:52, aelias wrote: > This type of assertion is not an idiom in WebKit, please remove. We'll just hit > the null pointer dereference later anyway. Done. https://codereview.chromium.org/13704012/diff/1/Source/WebKit/chromium/src/We... Source/WebKit/chromium/src/WebViewImpl.cpp:245: FloatSize anchorOffset = FloatSize(viewRect.size()); On 2013/04/08 04:24:52, aelias wrote: > = viewRect.size() Done. https://codereview.chromium.org/13704012/diff/1/Source/WebKit/chromium/src/We... Source/WebKit/chromium/src/WebViewImpl.cpp:254: m_anchorNodeBounds = node->Node::boundingBox(); On 2013/04/08 04:24:52, aelias wrote: > node->boundingBox(). The Node:: you copied from somewhere was needed at one > point to work around a bug, but no longer. Done. https://codereview.chromium.org/13704012/diff/1/Source/WebKit/chromium/src/We... Source/WebKit/chromium/src/WebViewImpl.cpp:257: m_anchorInNodeCoords.scale(1.f / m_anchorNodeBounds.width(), 1.f / m_anchorNodeBounds.height()); On 2013/04/08 04:24:52, aelias wrote: > Delete .f Without the .f, int / LayoutUnit performs integer division!? Spent all morning looking for this bug =/ I'll use a FloatRect to avoid any confusion here... https://codereview.chromium.org/13704012/diff/1/Source/WebKit/chromium/src/We... Source/WebKit/chromium/src/WebViewImpl.cpp:260: // Note: No guarantees are made on the validity of the returned point. On 2013/04/08 04:24:52, aelias wrote: > This comment is too scary and not very useful. Can you switch to saying "The > returned point may not be clamped to document bounds" since I think that's what > you mean? Done. https://codereview.chromium.org/13704012/diff/1/Source/WebKit/chromium/src/We... Source/WebKit/chromium/src/WebViewImpl.cpp:268: const LayoutRect currentNodeBounds = m_anchorNode->Node::boundingBox(); On 2013/04/08 04:24:52, aelias wrote: > m_anchorNode->boundingBox() Done. https://codereview.chromium.org/13704012/diff/1/Source/WebKit/chromium/src/We... Source/WebKit/chromium/src/WebViewImpl.cpp:278: FloatSize anchorOffsetFromOrigin = FloatSize(currentViewSize); On 2013/04/08 04:24:52, aelias wrote: > = currentViewSize; Done. https://codereview.chromium.org/13704012/diff/1/Source/WebKit/chromium/src/We... Source/WebKit/chromium/src/WebViewImpl.cpp:285: ViewportAnchor& operator=(ViewportAnchor&); On 2013/04/08 04:24:52, aelias wrote: > Not a common idiom to have a private operator=, please delete. Done. https://codereview.chromium.org/13704012/diff/1/Source/WebKit/chromium/src/We... Source/WebKit/chromium/src/WebViewImpl.cpp:294: if (node && node->Node::pixelSnappedBoundingBox().contains(viewRect)) { On 2013/04/08 04:24:52, aelias wrote: > node->boundingBox() ? Done. https://codereview.chromium.org/13704012/diff/1/Source/WebKit/chromium/src/We... Source/WebKit/chromium/src/WebViewImpl.cpp:297: IntPoint shiftedPoint = point + pointOffset; On 2013/04/08 04:24:52, aelias wrote: > Please delete this local variable. Done. https://codereview.chromium.org/13704012/diff/1/Source/WebKit/chromium/src/We... Source/WebKit/chromium/src/WebViewImpl.cpp:301: while (node && node->Node::boundingBox().isEmpty()) On 2013/04/08 04:24:52, aelias wrote: > Switch to node->boundingBox(). Done. https://codereview.chromium.org/13704012/diff/1/Source/WebKit/chromium/src/We... Source/WebKit/chromium/src/WebViewImpl.cpp:1790: viewportAnchor.setAnchor(view->visibleContentRect(), FloatSize(0.5f, 0)); On 2013/04/08 04:24:52, aelias wrote: > FloatSize(0.5f, 0) looks like a magic number, please put in a constant. Done. https://codereview.chromium.org/13704012/diff/1/Source/WebKit/chromium/src/We... Source/WebKit/chromium/src/WebViewImpl.cpp:1790: viewportAnchor.setAnchor(view->visibleContentRect(), FloatSize(0.5f, 0)); On 2013/04/08 04:24:52, aelias wrote: > FloatSize(0.5f, 0) looks like a magic number, please put in a constant. Done. https://codereview.chromium.org/13704012/diff/1/Source/WebKit/chromium/src/We... Source/WebKit/chromium/src/WebViewImpl.cpp:1811: // When the device rotates: On 2013/04/08 04:24:52, aelias wrote: > This comment is out of date, could you delete/update? Done. https://codereview.chromium.org/13704012/diff/1/Source/WebKit/chromium/src/We... Source/WebKit/chromium/src/WebViewImpl.cpp:1827: float scaleMultiplier = m_minimumPageScaleFactor == oldMinimumPageScaleFactor ? On 2013/04/08 04:24:52, aelias wrote: > Too much for ternary operator, please make into an if statement. Done. https://codereview.chromium.org/13704012/diff/1/Source/WebKit/chromium/tests/... File Source/WebKit/chromium/tests/WebFrameTest.cpp (right): https://codereview.chromium.org/13704012/diff/1/Source/WebKit/chromium/tests/... Source/WebKit/chromium/tests/WebFrameTest.cpp:581: relativeOffset.scale(1.f / rect.width(), 1.f / rect.height()); On 2013/04/08 04:24:52, aelias wrote: > Delete .f Done. https://codereview.chromium.org/13704012/diff/1/Source/WebKit/chromium/tests/... Source/WebKit/chromium/tests/WebFrameTest.cpp:594: m_webView->enableFixedLayoutMode(false); On 2013/04/08 04:24:52, aelias wrote: > This is odd, on Android fixedLayoutMode setting is always true. This should be > true. If it breaks your test, something is wrong with the test :). Nothing broken, I was too lazy to check the default setting. https://codereview.chromium.org/13704012/diff/1/Source/WebKit/chromium/tests/... Source/WebKit/chromium/tests/WebFrameTest.cpp:600: ASSERT_EQ(WebSize(), webViewImpl->mainFrame()->scrollOffset()); On 2013/04/08 04:24:52, aelias wrote: > Delete this assert. Done. https://codereview.chromium.org/13704012/diff/1/Source/WebKit/chromium/tests/... Source/WebKit/chromium/tests/WebFrameTest.cpp:608: ASSERT_EQ(scrollOffset, webViewImpl->mainFrame()->scrollOffset()); On 2013/04/08 04:24:52, aelias wrote: > Delete this assert. Done.
Added unit tests for fixed width, fixed layout (desktop) and width=device-width (mobile), including checks on pageScaleFactor. All tests from WebFrameTest.* now passing; initial resize now properly respects setInitialPageScaleOverride. @abarth: Could you take a look at this for approval? Thanks.
@abarth: You're probably swamped, but would you mind taking a quick pass at this? Otherwise, would you know of another owner who could take a gander? Thanks.
I'm probably not the best reviewer for this CL. I'm happy to approve it if someone who knows the subject matter better reviews it for content. https://codereview.chromium.org/13704012/diff/12001/Source/WebKit/chromium/sr... File Source/WebKit/chromium/src/WebViewImpl.cpp (right): https://codereview.chromium.org/13704012/diff/12001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/WebViewImpl.cpp:222: class ViewportAnchor { This class should go in its own file. WebViewImpl has enough code in it. :) https://codereview.chromium.org/13704012/diff/12001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/WebViewImpl.cpp:224: ViewportAnchor(EventHandler* eventHandler) one-argument constructors should be marked explicit
@alias: I've expanded the unit tests to include checks on pageScaleFactor, as well as cases for fixed layout and width=device-width. @abarth gives approval granted somebody with relative familiarity to the code can sign off... I assume that is you? or should I grab somebody else? https://codereview.chromium.org/13704012/diff/12001/Source/WebKit/chromium/sr... File Source/WebKit/chromium/src/WebViewImpl.cpp (right): https://codereview.chromium.org/13704012/diff/12001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/WebViewImpl.cpp:222: class ViewportAnchor { On 2013/04/12 17:33:30, abarth wrote: > This class should go in its own file. WebViewImpl has enough code in it. :) Done. https://codereview.chromium.org/13704012/diff/12001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/WebViewImpl.cpp:224: ViewportAnchor(EventHandler* eventHandler) On 2013/04/12 17:33:30, abarth wrote: > one-argument constructors should be marked explicit Done.
On 2013/04/12 19:20:16, jdduke wrote: > @alias: I've expanded the unit tests to include checks on In which @alias aliases @aelias...
https://codereview.chromium.org/13704012/diff/17001/Source/WebKit/chromium/sr... File Source/WebKit/chromium/src/WebViewImpl.cpp (right): https://codereview.chromium.org/13704012/diff/17001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/WebViewImpl.cpp:1697: if (oldSize.width && oldContentsWidth && m_minimumPageScaleFactor != oldMinimumPageScaleFactor) Could you delete the "&& m_minimumPageScaleFactor != oldMinimumPageScaleFactor"? It seems that in this case, oldContentsWidth == contentSize.width() so you can safely do the division and get 1. https://codereview.chromium.org/13704012/diff/17001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/WebViewImpl.cpp:1700: scalingWidthRatio = fixedLayoutSize().width / (float) oldFixedLayoutWidth; Could you try deleting this else block and see if anything breaks? Contents width should be set to the greater of fixed layout width or greatest node width. So the fixedLayoutSize information is largely contained within contentsWidth anyway and I'm not sure this covers any other relevant case. https://codereview.chromium.org/13704012/diff/17001/Source/WebKit/chromium/te... File Source/WebKit/chromium/tests/WebFrameTest.cpp (right): https://codereview.chromium.org/13704012/diff/17001/Source/WebKit/chromium/te... Source/WebKit/chromium/tests/WebFrameTest.cpp:673: const bool useFixedLayout, Delete this https://codereview.chromium.org/13704012/diff/17001/Source/WebKit/chromium/te... Source/WebKit/chromium/tests/WebFrameTest.cpp:677: const float aspectRatio = (float)viewportSize.width / viewportSize.height; Nit: static_cast<float> https://codereview.chromium.org/13704012/diff/17001/Source/WebKit/chromium/te... Source/WebKit/chromium/tests/WebFrameTest.cpp:681: m_webView->enableFixedLayoutMode(useFixedLayout); This setting is always true on the Android platform (or, in theory, any mobile OS that would support device rotate). Please call enableFixedLayoutMode(true) here.
Alright, I think this is ready to go. https://codereview.chromium.org/13704012/diff/17001/Source/WebKit/chromium/sr... File Source/WebKit/chromium/src/WebViewImpl.cpp (right): https://codereview.chromium.org/13704012/diff/17001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/WebViewImpl.cpp:1697: if (oldSize.width && oldContentsWidth && m_minimumPageScaleFactor != oldMinimumPageScaleFactor) On 2013/04/15 18:25:39, aelias wrote: > Could you delete the "&& m_minimumPageScaleFactor != oldMinimumPageScaleFactor"? > It seems that in this case, oldContentsWidth == contentSize.width() so you can > safely do the division and get 1. Done. https://codereview.chromium.org/13704012/diff/17001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/WebViewImpl.cpp:1700: scalingWidthRatio = fixedLayoutSize().width / (float) oldFixedLayoutWidth; On 2013/04/15 18:25:39, aelias wrote: > Could you try deleting this else block and see if anything breaks? Contents > width should be set to the greater of fixed layout width or greatest node width. > So the fixedLayoutSize information is largely contained within contentsWidth > anyway and I'm not sure this covers any other relevant case. This initially caused some problems with fixed width pages, which is why I left it in. However, adding an additional check on "oldSize.width" when using the contentsSize.width() ratio fixes the problem. This is much cleaner. https://codereview.chromium.org/13704012/diff/17001/Source/WebKit/chromium/te... File Source/WebKit/chromium/tests/WebFrameTest.cpp (right): https://codereview.chromium.org/13704012/diff/17001/Source/WebKit/chromium/te... Source/WebKit/chromium/tests/WebFrameTest.cpp:673: const bool useFixedLayout, On 2013/04/15 18:25:39, aelias wrote: > Delete this Done. https://codereview.chromium.org/13704012/diff/17001/Source/WebKit/chromium/te... Source/WebKit/chromium/tests/WebFrameTest.cpp:677: const float aspectRatio = (float)viewportSize.width / viewportSize.height; On 2013/04/15 18:25:39, aelias wrote: > Nit: static_cast<float> Done. https://codereview.chromium.org/13704012/diff/17001/Source/WebKit/chromium/te... Source/WebKit/chromium/tests/WebFrameTest.cpp:681: m_webView->enableFixedLayoutMode(useFixedLayout); On 2013/04/15 18:25:39, aelias wrote: > This setting is always true on the Android platform (or, in theory, any mobile > OS that would support device rotate). Please call enableFixedLayoutMode(true) > here. Done.
https://codereview.chromium.org/13704012/diff/21001/Source/WebKit/chromium/sr... File Source/WebKit/chromium/src/WebViewImpl.cpp (right): https://codereview.chromium.org/13704012/diff/21001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/WebViewImpl.cpp:1684: if (settings()->viewportEnabled()) { Please change this to if (settings()->viewportEnabled() && oldSize.width && oldContentsWidth). We should just bail out on the whole logic if one of them is zero. https://codereview.chromium.org/13704012/diff/21001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/WebViewImpl.cpp:1689: float viewportWidthRatio = !oldSize.width ? 1 : newSize.width / (float) oldSize.width; And you can then delete both of the ternary operators here.
https://codereview.chromium.org/13704012/diff/21001/Source/WebKit/chromium/sr... File Source/WebKit/chromium/src/WebViewImpl.cpp (right): https://codereview.chromium.org/13704012/diff/21001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/WebViewImpl.cpp:1684: if (settings()->viewportEnabled()) { On 2013/04/16 16:35:42, aelias wrote: > Please change this to if (settings()->viewportEnabled() && oldSize.width && > oldContentsWidth). We should just bail out on the whole logic if one of them is > zero. There appear to be some assumptions that view->layout() is called if necessary, even when there's no re-scaling. After that, it's safe to bail and definitely cleaner. https://codereview.chromium.org/13704012/diff/21001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/WebViewImpl.cpp:1689: float viewportWidthRatio = !oldSize.width ? 1 : newSize.width / (float) oldSize.width; On 2013/04/16 16:35:42, aelias wrote: > And you can then delete both of the ternary operators here. Done.
lgtm https://codereview.chromium.org/13704012/diff/19002/Source/WebKit/chromium/sr... File Source/WebKit/chromium/src/WebViewImpl.cpp (right): https://codereview.chromium.org/13704012/diff/19002/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/WebViewImpl.cpp:1691: float viewportWidthRatio = newSize.width / (float) oldSize.width; Nit: static_cast<float> https://codereview.chromium.org/13704012/diff/19002/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/WebViewImpl.cpp:1692: float contentsWidthRatio = contentsSize().width() / (float) oldContentsWidth; static_cast<float> https://codereview.chromium.org/13704012/diff/19002/Source/WebKit/chromium/te... File Source/WebKit/chromium/tests/WebFrameTest.cpp (right): https://codereview.chromium.org/13704012/diff/19002/Source/WebKit/chromium/te... Source/WebKit/chromium/tests/WebFrameTest.cpp:707: const WebSize scrollOffset = WebSize(0, 200); nit: scrollOffset(0, 200). likewise below
https://codereview.chromium.org/13704012/diff/19002/Source/WebKit/chromium/sr... File Source/WebKit/chromium/src/WebViewImpl.cpp (right): https://codereview.chromium.org/13704012/diff/19002/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/WebViewImpl.cpp:1691: float viewportWidthRatio = newSize.width / (float) oldSize.width; On 2013/04/16 22:24:52, aelias wrote: > Nit: static_cast<float> Done. https://codereview.chromium.org/13704012/diff/19002/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/WebViewImpl.cpp:1692: float contentsWidthRatio = contentsSize().width() / (float) oldContentsWidth; On 2013/04/16 22:24:52, aelias wrote: > static_cast<float> Done. https://codereview.chromium.org/13704012/diff/19002/Source/WebKit/chromium/te... File Source/WebKit/chromium/tests/WebFrameTest.cpp (right): https://codereview.chromium.org/13704012/diff/19002/Source/WebKit/chromium/te... Source/WebKit/chromium/tests/WebFrameTest.cpp:707: const WebSize scrollOffset = WebSize(0, 200); On 2013/04/16 22:24:52, aelias wrote: > nit: scrollOffset(0, 200). likewise below Done.
LG. Adam, could you approve?
LGTM with some minor style nits. https://codereview.chromium.org/13704012/diff/33001/Source/WebKit/chromium/sr... File Source/WebKit/chromium/src/ViewportAnchor.h (right): https://codereview.chromium.org/13704012/diff/33001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/ViewportAnchor.h:56: class ViewportAnchor { This object seems to deal entirely in terms of WebCore types. Should it be in Core instead? https://codereview.chromium.org/13704012/diff/33001/Source/WebKit/chromium/sr... File Source/WebKit/chromium/src/WebViewImpl.cpp (right): https://codereview.chromium.org/13704012/diff/33001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/WebViewImpl.cpp:1667: if (shouldAnchorAndRescaleViewport) Multiline if statements request { } even if they contain only one statement. https://codereview.chromium.org/13704012/diff/33001/Source/WebKit/chromium/te... File Source/WebKit/chromium/tests/WebFrameTest.cpp (right): https://codereview.chromium.org/13704012/diff/33001/Source/WebKit/chromium/te... Source/WebKit/chromium/tests/WebFrameTest.cpp:638: void TestResizeYieldsCorrectScrollAndScale(const char* url, TestResizeYieldsCorrectScrollAndScale -> testResizeYieldsCorrectScrollAndScale
https://codereview.chromium.org/13704012/diff/33001/Source/WebKit/chromium/sr... File Source/WebKit/chromium/src/ViewportAnchor.h (right): https://codereview.chromium.org/13704012/diff/33001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/ViewportAnchor.h:56: class ViewportAnchor { On 2013/04/18 05:33:04, abarth wrote: > This object seems to deal entirely in terms of WebCore types. Should it be in > Core instead? That's hard for me to say. This was originally intended as an internal utility class to be used only during resize operations in WebViewImpl, which is why I'd prefer it live as close to that class as possible. Technically, there's likely no reason it couldn't exist in some folder within core. https://codereview.chromium.org/13704012/diff/33001/Source/WebKit/chromium/sr... File Source/WebKit/chromium/src/WebViewImpl.cpp (right): https://codereview.chromium.org/13704012/diff/33001/Source/WebKit/chromium/sr... Source/WebKit/chromium/src/WebViewImpl.cpp:1667: if (shouldAnchorAndRescaleViewport) On 2013/04/18 05:33:04, abarth wrote: > Multiline if statements request { } even if they contain only one statement. Done. https://codereview.chromium.org/13704012/diff/33001/Source/WebKit/chromium/te... File Source/WebKit/chromium/tests/WebFrameTest.cpp (right): https://codereview.chromium.org/13704012/diff/33001/Source/WebKit/chromium/te... Source/WebKit/chromium/tests/WebFrameTest.cpp:638: void TestResizeYieldsCorrectScrollAndScale(const char* url, On 2013/04/18 05:33:04, abarth wrote: > TestResizeYieldsCorrectScrollAndScale -> testResizeYieldsCorrectScrollAndScale Done.
Odd. The trybot targets compile just fine, but the triggered_tests are having trouble applying the patch... For whatever reason, ViewportAnchor.h is treated as a rename of WebUserGestureToken.h?
> For whatever reason, ViewportAnchor.h is treated > as a rename of WebUserGestureToken.h? Yeah, the rename finder is a bit too aggressive. I'm not sure how to tune that parameter.
On 2013/04/19 06:50:05, abarth wrote: > > For whatever reason, ViewportAnchor.h is treated > > as a rename of WebUserGestureToken.h? > > Yeah, the rename finder is a bit too aggressive. I'm not sure how to tune that > parameter. Ahh. git cl upload --similarity=X, where X is the desired similarity percentage. Seems to have done the trick.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jdduke@chromium.org/13704012/54001
Message was sent while issue was closed.
Change committed as 148734
Message was sent while issue was closed.
As the webapp might try to set the position itself, wouldn't it make more sense to first apply the position after the onresize handler was called?
Message was sent while issue was closed.
On 2013/04/25 12:37:37, kenneth.r.christiansen wrote: > As the webapp might try to set the position itself, wouldn't it make more sense > to first apply the position after the onresize handler was called? Where does that call take place? @aelias can probably speak to that better than I. |
