Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2454913003:
MainFrame scrollbars should work with RFV instead of FV (Closed)
DescriptionMainFrame scrollbars should work with RFV instead of FV
This CL does the following things:
- FV::updateScrollbarGeometry and part of FV::computeScrollbarExistence is moved into
scrollbar manager.
- For MainFrame, the scroller that the scrollbars works with is changed to be RFV from FV
- To accommodate for this, a bunch of methods in Scrollbar that used to call into FV now
call into RFV, which in turn calls the corresponding methods on its layout viewport. An
exception to this is Tickmarks. Tickmarks for MainFrame are now stored in RFV.
- The isOverlayScrollbar method was needlessly implemented in FV. This CL removes this.
BUG=456861
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Committed: https://crrev.com/322db3d6f1f9eb4b9cd6a0054999718c24b1f076
Cr-Commit-Position: refs/heads/master@{#431683}
Patch Set 1 #Patch Set 2 : Add overrides in RFV #Patch Set 3 : add test + rebase #
Total comments: 6
Patch Set 4 : Remove needsScrollbar as the change broke tests #Patch Set 5 : rebase with dependency #Patch Set 6 : VV TODO comment #Patch Set 7 : rebase dependency #Patch Set 8 : Fix VisualViewportTest #
Total comments: 12
Patch Set 9 : Rebase and fix merge conflicts #
Total comments: 6
Patch Set 10 : rebase and Fix more tests #Patch Set 11 : Add TODOs #
Total comments: 11
Patch Set 12 : Address review comments #Patch Set 13 : Mark svg test as failing #Patch Set 14 : Remove traces from bad merge #Patch Set 15 : Make VisualViewportTest parameterized #Patch Set 16 : Fix failing test #
Total comments: 2
Patch Set 17 : nit #Patch Set 18 : rebase master #Patch Set 19 : Fix broken chromeos bot #Messages
Total messages: 84 (29 generated)
Description was changed from ========== MainFrame scrollbars should work with RFV instead of FV BUG=456861 ========== to ========== MainFrame scrollbars should work with RFV instead of FV BUG=456861 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Description was changed from ========== MainFrame scrollbars should work with RFV instead of FV BUG=456861 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== MainFrame scrollbars should work with RFV instead of FV This CL does the following things: - FV::updateScrollbarGeometry and part of FV::computeScrollbarExistence is moved into scrollbar manager. - For MainFrame, the scroller that the scrollbars works with is changed to be RFV from FV - To accommodate for this, a bunch of methods in Scrollbar that used to call into FV now call into RFV, which in turn calls the corresponding methods on its layout viewport. An exception to this is Tickmarks. Tickmarks for MainFrame are now stored in RFV. - The isOverlayScrollbar method was needlessly implemented in FV. This CL removes this. BUG=456861 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
ymalik@chromium.org changed reviewers: + bokan@chromium.org, skobes@chromium.org
Please hold off on the review until I fix the bots.
On 2016/10/31 15:19:14, ymalik wrote: > Please hold off on the review until I fix the bots. Have a few pending comments. https://codereview.chromium.org/2454913003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/RootFrameViewport.h (right): https://codereview.chromium.org/2454913003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/RootFrameViewport.h:98: // Scrollbar related I think we should have a separate interface from ScrollableArea for a ScrollbarController, ScrollableArea is too big and general. Might be easier in a follow up CL though. https://codereview.chromium.org/2454913003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/VisualViewport.cpp (right): https://codereview.chromium.org/2454913003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/VisualViewport.cpp:299: mainFrame()->view()->updateScrollbars(); VisualViewport shouldn't be talking to FrameView, this should happen though the RFV. There's already VisualViewport::notifyRootFrameViewport, just call into FrameView::updateScrollbars from RootFrameViewport::didUpdateVisualViewport https://codereview.chromium.org/2454913003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): https://codereview.chromium.org/2454913003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1747: DocumentMarker::TextMatch); Does this make the tickmarks work for root-layer-scrolling?
@bokan PTAL, I'm still trying to figure out the svg test failure. https://codereview.chromium.org/2454913003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/RootFrameViewport.h (right): https://codereview.chromium.org/2454913003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/RootFrameViewport.h:98: // Scrollbar related On 2016/10/31 15:25:09, bokan wrote: > I think we should have a separate interface from ScrollableArea for a > ScrollbarController, ScrollableArea is too big and general. Might be easier in a > follow up CL though. Agreed. Added a TODO for cleanup. https://codereview.chromium.org/2454913003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/VisualViewport.cpp (right): https://codereview.chromium.org/2454913003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/VisualViewport.cpp:299: mainFrame()->view()->updateScrollbars(); On 2016/10/31 15:25:09, bokan wrote: > VisualViewport shouldn't be talking to FrameView, this should happen though the > RFV. There's already VisualViewport::notifyRootFrameViewport, just call into > FrameView::updateScrollbars from RootFrameViewport::didUpdateVisualViewport Yes you're right. I have RFV::didUpdateVisualViewport calling updateScrollbars now. https://codereview.chromium.org/2454913003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): https://codereview.chromium.org/2454913003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1747: DocumentMarker::TextMatch); On 2016/10/31 15:25:09, bokan wrote: > Does this make the tickmarks work for root-layer-scrolling? Almost. I am not sure where the tickmarks should be stored for root-layer-scrolls. But currently, tickmarks are always stored in FV. This is the default behavior of FV when m_tickmarks is not set. I think this will be needed for root-layer-scrolling but not sure if its the only thing needed/ https://codereview.chromium.org/2454913003/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/transforms/selection-bounds-in-transformed-view.html (right): https://codereview.chromium.org/2454913003/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/transforms/selection-bounds-in-transformed-view.html:16: document.getElementById("result").innerText = internals.visualViewportScrollY() === 863 ? "PASS" : "FAIL (scrollTop:" + document.scrollingElement.scrollTop + ")"; This is expected because the calculation in scrollIntoView takes the horizontal scrollbar into account (which is only created after this patch). https://codereview.chromium.org/2454913003/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/paint/invalidation/window-resize-centered-inline-under-fixed-pos-expected.txt (right): https://codereview.chromium.org/2454913003/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/paint/invalidation/window-resize-centered-inline-under-fixed-pos-expected.txt:143: "reason": "forced by layout" This and other resize window tests are updated because of timing. When the window is resized, the visual viewport first reflects this new size. Because FV::computeScrollbarExistance works with the visual viewport's visibleContentRect now (see RFV::visibleContentRect), it sees the updated state before it did without this patch.
https://codereview.chromium.org/2454913003/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/transforms/selection-bounds-in-transformed-view.html (right): https://codereview.chromium.org/2454913003/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/transforms/selection-bounds-in-transformed-view.html:16: document.getElementById("result").innerText = internals.visualViewportScrollY() === 863 ? "PASS" : "FAIL (scrollTop:" + document.scrollingElement.scrollTop + ")"; On 2016/11/03 18:49:12, ymalik wrote: > This is expected because the calculation in scrollIntoView takes the horizontal > scrollbar into account (which is only created after this patch). Acknowledged. https://codereview.chromium.org/2454913003/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/paint/invalidation/window-resize-centered-inline-under-fixed-pos-expected.txt (right): https://codereview.chromium.org/2454913003/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/paint/invalidation/window-resize-centered-inline-under-fixed-pos-expected.txt:143: "reason": "forced by layout" On 2016/11/03 18:49:12, ymalik wrote: > This and other resize window tests are updated because of timing. When the > window is resized, the visual viewport first reflects this new size. Because > FV::computeScrollbarExistance works with the visual viewport's > visibleContentRect now (see RFV::visibleContentRect), it sees the updated state > before it did without this patch. Acknowledged. https://codereview.chromium.org/2454913003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2454913003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/FrameView.cpp:2442: if (m_frame->isMainFrame()) You'll need a similar check in getTickmarks. https://codereview.chromium.org/2454913003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/VisualViewport.cpp (right): https://codereview.chromium.org/2454913003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/VisualViewport.cpp:301: mainFrame()->view()->updateScrollbars(); We don't need this anymore since you added it to RFV::didUpdateVisualViewport https://codereview.chromium.org/2454913003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): https://codereview.chromium.org/2454913003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1778: tickmarks = frame->document()->markers().renderedRectsForMarkers( I think this should probably use the RFV markers if it's the main frame. Probably makes sense to just call frame->view()->getTickmarks and let it decide. https://codereview.chromium.org/2454913003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/tests/VisualViewportTest.cpp (right): https://codereview.chromium.org/2454913003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/web/tests/VisualViewportTest.cpp:313: if (RuntimeEnabledFeatures::rootLayerScrollingEnabled()) Run the test but change the expectations below based on bool rootLayerScrolls = GetParam().
@bokan, PTAL! https://codereview.chromium.org/2454913003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2454913003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/FrameView.cpp:2442: if (m_frame->isMainFrame()) On 2016/11/03 19:59:59, bokan wrote: > You'll need a similar check in getTickmarks. So RFV::getTickmarks calls layoutViewport().getTickmarks(). If we do the main frame check here, we will be stuck in an infinite loop. getTickmarks is only called from Scrollbar.cpp and this should just work since the scrollbar has the correct ScrollableArea. The reason we have to call layoutViewport().getTickmarks from RFV is because RFV doesn't have a reference to frame().document() and we can't return the default markers from it. The reason we have to save tickmarks in RFV (instead of just calling layoutViewport().getTickmarks()) is because root layer scrolls has PLSA as its layoutViewport. https://codereview.chromium.org/2454913003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/VisualViewport.cpp (right): https://codereview.chromium.org/2454913003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/VisualViewport.cpp:301: mainFrame()->view()->updateScrollbars(); On 2016/11/03 19:59:59, bokan wrote: > We don't need this anymore since you added it to RFV::didUpdateVisualViewport Oops, forgot to remove. Thanks. https://codereview.chromium.org/2454913003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): https://codereview.chromium.org/2454913003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1778: tickmarks = frame->document()->markers().renderedRectsForMarkers( On 2016/11/03 19:59:59, bokan wrote: > I think this should probably use the RFV markers if it's the main frame. > Probably makes sense to just call frame->view()->getTickmarks and let it decide. See comment above. This is only called from RFV and we will only get here for the default case (no custom tickmarks set). I agree its better to call frame()->view()->getTickmarks though. https://codereview.chromium.org/2454913003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/tests/VisualViewportTest.cpp (right): https://codereview.chromium.org/2454913003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/web/tests/VisualViewportTest.cpp:313: if (RuntimeEnabledFeatures::rootLayerScrollingEnabled()) On 2016/11/03 19:59:59, bokan wrote: > Run the test but change the expectations below based on bool rootLayerScrolls = > GetParam(). Good tip. Done. https://codereview.chromium.org/2454913003/diff/160001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/platform/linux/svg/custom/masking-clipping-hidpi-expected.txt (right): https://codereview.chromium.org/2454913003/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/platform/linux/svg/custom/masking-clipping-hidpi-expected.txt:3: layer at (0,0) size 1600x600 This test like the other window resize tests also fails because of timing. Before this CL, scrollbar existence was being computed incorrectly because of which we were setting the paint invalidation bit differently from what happens in this CL. I verified that the sizes and of the layers in this test are the same as they were before the CL and the size difference is just because this is an intermediate step.
@skobes, PTAL :)
https://codereview.chromium.org/2454913003/diff/160001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/platform/linux/svg/custom/masking-clipping-hidpi-expected.txt (right): https://codereview.chromium.org/2454913003/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/platform/linux/svg/custom/masking-clipping-hidpi-expected.txt:3: layer at (0,0) size 1600x600 On 2016/11/04 18:54:18, ymalik wrote: > This test like the other window resize tests also fails because of timing. > > Before this CL, scrollbar existence was being computed incorrectly because of > which we were setting the paint invalidation bit differently from what happens > in this CL. I verified that the sizes and of the layers in this test are the > same as they were before the CL and the size difference is just because this is > an intermediate step. Why was scrollbar existence computed incorrectly? The test sets a "BackingScaleFactor" not PageScaleFactor so the VisualViewport should be equal to the FrameView size, for which your CL shouldn't have any effect, right? Does the test not dump this text when notifyDone() is called? If not, are there any functions in testRunner/internals we can use to delay the dump until we're in a steady state? skobes@, any idea?
https://codereview.chromium.org/2454913003/diff/160001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/platform/linux/svg/custom/masking-clipping-hidpi-expected.txt (right): https://codereview.chromium.org/2454913003/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/platform/linux/svg/custom/masking-clipping-hidpi-expected.txt:3: layer at (0,0) size 1600x600 On 2016/11/04 19:23:59, bokan wrote: > On 2016/11/04 18:54:18, ymalik wrote: > > This test like the other window resize tests also fails because of timing. > > > > Before this CL, scrollbar existence was being computed incorrectly because of > > which we were setting the paint invalidation bit differently from what happens > > in this CL. I verified that the sizes and of the layers in this test are the > > same as they were before the CL and the size difference is just because this > is > > an intermediate step. > > Why was scrollbar existence computed incorrectly? The test sets a > "BackingScaleFactor" not PageScaleFactor so the VisualViewport should be equal > to the FrameView size, for which your CL shouldn't have any effect, right? > > Does the test not dump this text when notifyDone() is called? If not, are there > any functions in testRunner/internals we can use to delay the dump until we're > in a steady state? skobes@, any idea? I believe that the difference is caused by computeScrollbarExistance using the visual viewport size (RFV uses visualViewport::visibleContentRect) which is set right when BackingScaleFactor is changed.
https://codereview.chromium.org/2454913003/diff/160001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/platform/linux/svg/custom/masking-clipping-hidpi-expected.txt (right): https://codereview.chromium.org/2454913003/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/platform/linux/svg/custom/masking-clipping-hidpi-expected.txt:3: layer at (0,0) size 1600x600 On 2016/11/04 19:39:51, ymalik wrote: > On 2016/11/04 19:23:59, bokan wrote: > > On 2016/11/04 18:54:18, ymalik wrote: > > > This test like the other window resize tests also fails because of timing. > > > > > > Before this CL, scrollbar existence was being computed incorrectly because > of > > > which we were setting the paint invalidation bit differently from what > happens > > > in this CL. I verified that the sizes and of the layers in this test are the > > > same as they were before the CL and the size difference is just because this > > is > > > an intermediate step. > > > > Why was scrollbar existence computed incorrectly? The test sets a > > "BackingScaleFactor" not PageScaleFactor so the VisualViewport should be equal > > to the FrameView size, for which your CL shouldn't have any effect, right? > > > > Does the test not dump this text when notifyDone() is called? If not, are > there > > any functions in testRunner/internals we can use to delay the dump until we're > > in a steady state? skobes@, any idea? > > I believe that the difference is caused by computeScrollbarExistance using the > visual viewport size (RFV uses visualViewport::visibleContentRect) which is set > right when BackingScaleFactor is changed. In that case, the scrollbar existence was computed correctly before, no? Also, why does scrollbar existence affect the layer size here by so much?
https://codereview.chromium.org/2454913003/diff/160001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/platform/linux/svg/custom/masking-clipping-hidpi-expected.txt (right): https://codereview.chromium.org/2454913003/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/platform/linux/svg/custom/masking-clipping-hidpi-expected.txt:3: layer at (0,0) size 1600x600 On 2016/11/04 19:23:59, bokan wrote: > Does the test not dump this text when notifyDone() is called? If not, are there > any functions in testRunner/internals we can use to delay the dump until we're > in a steady state? skobes@, any idea? The test runner calls updateAllLifecyclePhases before dumping the layout tree, so everything ought to be up to date (if not, it's a bug).
@bokan/skobes, PTAL :) I keep running into merge conflicts and more failing tests. I'd like to land this asap and leave other non-major concerns for a TODO if possible :) https://codereview.chromium.org/2454913003/diff/160001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/platform/linux/svg/custom/masking-clipping-hidpi-expected.txt (right): https://codereview.chromium.org/2454913003/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/platform/linux/svg/custom/masking-clipping-hidpi-expected.txt:3: layer at (0,0) size 1600x600 On 2016/11/04 21:33:20, skobes wrote: > On 2016/11/04 19:23:59, bokan wrote: > > Does the test not dump this text when notifyDone() is called? If not, are > there > > any functions in testRunner/internals we can use to delay the dump until we're > > in a steady state? skobes@, any idea? > > The test runner calls updateAllLifecyclePhases before dumping the layout tree, > so everything ought to be up to date (if not, it's a bug). So I did a bit more investigation. My original comment about timing in this test doesn't make sense. Before this patch (where the scroller was FV and not RFV), here is roughly the sequence of events that took place when this test set the backing scale factor to 2: 1) The visual viewport dimensions are changed (800x600 -> 1600x1200) but the FV::frameRect dimensions are still (800x600) 2) We start doing layout and the LayoutSVGRoot object size (which is where the PaintLayer size comes from) is changed, but only in one dimension (800x600 -> 1600x600) 3) We get into FV::computeScrollbarExistence and determine that we need scrollbars (800x600 < 1600x1200), and layout the scrollbars (updating the sizes for the LayoutObjects to exclude the scrollbars).This sets the correct dimensions on LayoutSVGRoot(1600x600 -> 1600x1200) 4) We get into FV::computeScrollbarExistence again when the actual FV::frameRect changes after layout and determine that we don't actually need the scrollbars. With this patch, step 3) doesn't lay out the scrollbars because we use VV:visibleContentRect which is updated in the very beginning and we never end up correcting the size of the LayoutSVGRoot. I don't understand why step 2 doesn't compute the correct height, but it seems like a bug. This CL just causes this test to highlight it. We have two ways forward, 1) Just update the layout test and see if we get any complaints. I suspect we wont. and 2) Just don't make this change if the frame document is SVG and investigate it more. I don't want to special case so I am in favor of 1), especially because this looks broken anyway.
The CQ bit was checked by ymalik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/11/08 00:11:13, ymalik wrote: > @bokan/skobes, PTAL :) > > I keep running into merge conflicts and more failing tests. I'd like to land > this asap and leave other non-major concerns for a TODO if possible :) > > https://codereview.chromium.org/2454913003/diff/160001/third_party/WebKit/Lay... > File > third_party/WebKit/LayoutTests/platform/linux/svg/custom/masking-clipping-hidpi-expected.txt > (right): > > https://codereview.chromium.org/2454913003/diff/160001/third_party/WebKit/Lay... > third_party/WebKit/LayoutTests/platform/linux/svg/custom/masking-clipping-hidpi-expected.txt:3: > layer at (0,0) size 1600x600 > On 2016/11/04 21:33:20, skobes wrote: > > On 2016/11/04 19:23:59, bokan wrote: > > > Does the test not dump this text when notifyDone() is called? If not, are > > there > > > any functions in testRunner/internals we can use to delay the dump until > we're > > > in a steady state? skobes@, any idea? > > > > The test runner calls updateAllLifecyclePhases before dumping the layout tree, > > so everything ought to be up to date (if not, it's a bug). > > So I did a bit more investigation. My original comment about timing in this test > doesn't make sense. > > Before this patch (where the scroller was FV and not RFV), here is roughly the > sequence of events that took place when this test set the backing scale factor > to 2: > 1) The visual viewport dimensions are changed (800x600 -> 1600x1200) but the > FV::frameRect dimensions are still (800x600) > 2) We start doing layout and the LayoutSVGRoot object size (which is where the > PaintLayer size comes from) is changed, but only in one dimension (800x600 -> > 1600x600) > 3) We get into FV::computeScrollbarExistence and determine that we need > scrollbars (800x600 < 1600x1200), and layout the scrollbars (updating the sizes > for the LayoutObjects to exclude the scrollbars).This sets the correct > dimensions on LayoutSVGRoot(1600x600 -> 1600x1200) > 4) We get into FV::computeScrollbarExistence again when the actual FV::frameRect > changes after layout and determine that we don't actually need the scrollbars. > > With this patch, step 3) doesn't lay out the scrollbars because we use > VV:visibleContentRect which is updated in the very beginning and we never end up > correcting the size of the LayoutSVGRoot. > > I don't understand why step 2 doesn't compute the correct height, but it seems > like a bug. This CL just causes this test to highlight it. > > We have two ways forward, 1) Just update the layout test and see if we get any > complaints. I suspect we wont. and 2) Just don't make this change if the frame > document is SVG and investigate it more. > > I don't want to special case so I am in favor of 1), especially because this > looks broken anyway. Thanks for digging into this in detail. When I open that file in Chrome stable, making the window smaller than the contents doesn't make scrollbars appear. Do you know why the difference in the test? The resize from 600->1200 does sound a little fishy to me. It's possible that's a bug in layout code but I don't know enough about that to say - skobes@ can probably say more here. This does bring up another potential issue though. Since we now use the visual viewport rect to calculate scrollbar existence, if a previously unscrollable page is zoomed and we add a scrollbar, does that scrollbar affect layout? I wouldn't expect it to, it should take space away from the visual viewport so that the user can scroll all the way to the content extent, but I suspect this might not work that way. Could you test this? Also, what happens if we zoom in and do a layout, does the layout change to accomodate the scrollbar / psf?
On 2016/11/08 18:35:34, bokan wrote: > On 2016/11/08 00:11:13, ymalik wrote: > > @bokan/skobes, PTAL :) > > > > I keep running into merge conflicts and more failing tests. I'd like to land > > this asap and leave other non-major concerns for a TODO if possible :) > > > > > https://codereview.chromium.org/2454913003/diff/160001/third_party/WebKit/Lay... > > File > > > third_party/WebKit/LayoutTests/platform/linux/svg/custom/masking-clipping-hidpi-expected.txt > > (right): > > > > > https://codereview.chromium.org/2454913003/diff/160001/third_party/WebKit/Lay... > > > third_party/WebKit/LayoutTests/platform/linux/svg/custom/masking-clipping-hidpi-expected.txt:3: > > layer at (0,0) size 1600x600 > > On 2016/11/04 21:33:20, skobes wrote: > > > On 2016/11/04 19:23:59, bokan wrote: > > > > Does the test not dump this text when notifyDone() is called? If not, are > > > there > > > > any functions in testRunner/internals we can use to delay the dump until > > we're > > > > in a steady state? skobes@, any idea? > > > > > > The test runner calls updateAllLifecyclePhases before dumping the layout > tree, > > > so everything ought to be up to date (if not, it's a bug). > > > > So I did a bit more investigation. My original comment about timing in this > test > > doesn't make sense. > > > > Before this patch (where the scroller was FV and not RFV), here is roughly the > > sequence of events that took place when this test set the backing scale factor > > to 2: > > 1) The visual viewport dimensions are changed (800x600 -> 1600x1200) but the > > FV::frameRect dimensions are still (800x600) > > 2) We start doing layout and the LayoutSVGRoot object size (which is where the > > PaintLayer size comes from) is changed, but only in one dimension (800x600 -> > > 1600x600) > > 3) We get into FV::computeScrollbarExistence and determine that we need > > scrollbars (800x600 < 1600x1200), and layout the scrollbars (updating the > sizes > > for the LayoutObjects to exclude the scrollbars).This sets the correct > > dimensions on LayoutSVGRoot(1600x600 -> 1600x1200) > > 4) We get into FV::computeScrollbarExistence again when the actual > FV::frameRect > > changes after layout and determine that we don't actually need the scrollbars. > > > > With this patch, step 3) doesn't lay out the scrollbars because we use > > VV:visibleContentRect which is updated in the very beginning and we never end > up > > correcting the size of the LayoutSVGRoot. > > > > I don't understand why step 2 doesn't compute the correct height, but it seems > > like a bug. This CL just causes this test to highlight it. > > > > We have two ways forward, 1) Just update the layout test and see if we get any > > complaints. I suspect we wont. and 2) Just don't make this change if the frame > > document is SVG and investigate it more. > > > > I don't want to special case so I am in favor of 1), especially because this > > looks broken anyway. > > Thanks for digging into this in detail. > > When I open that file in Chrome stable, making the window smaller than the > contents doesn't make scrollbars appear. Do you know why the difference in the > test? For some reason, the size of the SVG elements doesn't add to the size of the document. The size of the document seems to be the same as the size of the scroller, so we never add scrollers. Perhaps another bug? Sorry to be clear, the test also doesn't show the scrollbar, but we think* that we need the scrollbar initially because the frame view rect is not assigned. > The resize from 600->1200 does sound a little fishy to me. It's possible that's > a bug in layout code but I don't know enough about that to say - skobes@ can > probably say more here. > > This does bring up another potential issue though. Since we now use the visual > viewport rect to calculate scrollbar existence, if a previously unscrollable > page is zoomed and we add a scrollbar, does that scrollbar affect layout? I > wouldn't expect it to, it should take space away from the visual viewport so > that the user can scroll all the way to the content extent, but I suspect this > might not work that way. Could you test this? Also, what happens if we zoom in > and do a layout, does the layout change to accomodate the scrollbar / psf? For non-overlay scrollbars, addition and removal of scrollbars does a layout on LayoutView and as a result, the layout objects and the layers reflect the new dimensions. In the case where the scrollbar is added when you pinch zoom in, the layout viewport is adjusted and you can scroll to see the entire content. This CL doesn't change any behavior regarding when the layout should/shouldn't happen. I don't entirely see the concern here.
I'm a bit confused by this patch. If the scrollbars "work with" RFV, why don't they belong to RFV? It seems like RFV should have its own scrollbars and its own ScrollbarManager instead of doing all this delegation with FV.
Also there are a bunch of changes to test baselines in addition to the LayoutSVGRoot sizing issue, can you comment on what causes those?
On 2016/11/09 00:52:55, skobes wrote: > I'm a bit confused by this patch. > > If the scrollbars "work with" RFV, why don't they belong to RFV? > > It seems like RFV should have its own scrollbars and its own ScrollbarManager > instead of doing all this delegation with FV. I think in the current world, RFV is really a "fake" scrollable area which generally represents the layout viewport, but has knowledge of the visual viewport to accommodate for scroll related things. It's true that the current flow of things is a bit confusing, but bokan@ has a plan to refactor the the scrollbar code. At a high level, all the scrollbar related logic should be abstracted out and put into scrollbar manager, and each ScrollableArea should contain its own scrollbar manager. The reason we can't have scrollbar manager in RFV is because we still need it in FV for iframes. This CL simply builds on the notion that a scrollbar is associated with a scroller, and that scroller should account for both the visual viewport and the layout viewport. This CL fixes the bug that unblocks MD scrollbars, and there's still a lot of refactoring that needs to be done.
On 2016/11/09 00:52:55, skobes wrote: > I'm a bit confused by this patch. > > If the scrollbars "work with" RFV, why don't they belong to RFV? > > It seems like RFV should have its own scrollbars and its own ScrollbarManager > instead of doing all this delegation with FV. I think in the current world, RFV is really a "fake" scrollable area which generally represents the layout viewport, but has knowledge of the visual viewport to accommodate for scroll related things. It's true that the current flow of things is a bit confusing, but bokan@ has a plan to refactor the the scrollbar code. At a high level, all the scrollbar related logic should be abstracted out and put into scrollbar manager, and each ScrollableArea should contain its own scrollbar manager. The reason we can't have scrollbar manager in RFV is because we still need it in FV for iframes. This CL simply builds on the notion that a scrollbar is associated with a scroller, and that scroller should account for both the visual viewport and the layout viewport. This CL fixes the bug that unblocks MD scrollbars, and there's still a lot of refactoring that needs to be done.
On 2016/11/09 00:54:42, skobes wrote: > Also there are a bunch of changes to test baselines in addition to the > LayoutSVGRoot sizing issue, can you comment on what causes those? PS 8 has some comments in the tests. PTAL :)
On 2016/11/09 01:30:08, ymalik wrote: > On 2016/11/09 00:52:55, skobes wrote: > > I'm a bit confused by this patch. > > > > If the scrollbars "work with" RFV, why don't they belong to RFV? > > > > It seems like RFV should have its own scrollbars and its own ScrollbarManager > > instead of doing all this delegation with FV. > > I think in the current world, RFV is really a "fake" scrollable area which > generally represents the layout viewport, but has knowledge of the visual > viewport to accommodate for scroll related things. My understanding is that RFV represents the composition of the layout and visual viewports, and that the goal of http://crbug.com/456861 is for the main frame scrollbars to reflect (in thumb metrics and existence determination) the reported scroll position and bounds of the RFV and not the FV. Is that accurate? > It's true that the current > flow of things is a bit confusing, but bokan@ has a plan to refactor the the > scrollbar code. What's the plan? > At a high level, all the scrollbar related logic should be abstracted out and > put into scrollbar manager, and each ScrollableArea should contain its own > scrollbar manager. Agreed. > The reason we can't have scrollbar manager in RFV is because we still need it in > FV for iframes. But if each ScrollableArea has its own scrollbar manager then RFV and FV would both have one, no? > This CL simply builds on the notion that a scrollbar is associated with a > scroller, and that scroller should account for both the visual viewport and the > layout viewport. I think the weirdness is in having two kinds of associations between scrollbars and scrollers - one for object ownership (this scrollbar object "belongs to" the FrameView) and one for position correlation (this scrollbar is sync'ed with the scroll position of the RootFrameViewport). I'm mainly wondering how hard it would be to make those associations match. > This CL fixes the bug that unblocks MD scrollbars, and there's still a lot of > refactoring that needs to be done. It would help if I understood the planned refactoring / intended end state better.
On 2016/11/09 02:13:01, skobes wrote: > On 2016/11/09 01:30:08, ymalik wrote: > > On 2016/11/09 00:52:55, skobes wrote: > > > I'm a bit confused by this patch. > > > > > > If the scrollbars "work with" RFV, why don't they belong to RFV? > > > > > > It seems like RFV should have its own scrollbars and its own > ScrollbarManager > > > instead of doing all this delegation with FV. > > > > I think in the current world, RFV is really a "fake" scrollable area which > > generally represents the layout viewport, but has knowledge of the visual > > viewport to accommodate for scroll related things. > > My understanding is that RFV represents the composition of the layout and visual > viewports, and that the goal of http://crbug.com/456861 is for the main frame > scrollbars to reflect (in thumb metrics and existence determination) the > reported scroll position and bounds of the RFV and not the FV. Is that > accurate? This is my understanding as well. I think Yash's line about it "generally representing the layout viewport" is more to do with the fact that RFV delegates most of the functions to the layout viewport. This is mostly for technical reasons and much to do with the fact that FrameView and ScrollableArea have been dumping grounds for functionality. Conceptually, RFV is the composition of layout/visual. > > > It's true that the current > > flow of things is a bit confusing, but bokan@ has a plan to refactor the the > > scrollbar code. > > What's the plan? I haven't put it into a longer more thought out doc yet, but here's a sketch that explains the relationships: https://drive.google.com/a/google.com/file/d/0B600QF7NoJmvdkMtbTN0V2JiRVU/vie... Basically, as you mention below, ownership and positional coordination should be tied together. Each* ScrollableArea has a ScrollbarManager (that will now own the Scrollbar). The ScrollableArea implements a ScrollbarContainer interface that ScrollbarManager uses to get information from the ScrollableArea to position, size and otherwise update the scrollbars. In this world, both RFV and the layout viewport FV/PLSA will have a ScrollbarManager. When a FV/PLSA is made the layout viewport, RFV will simply clear the ScrollbarManager on the layout viewport so that only RFV will handle scrollbars. WDYT? *I use ScrollableArea here but I think the derived classes should be the ones to hold a ScrollbarManager and implement ScrollbarContainer. For e.g. VisualViewport doesn't need Scrollbars.
On 2016/11/09 15:50:14, bokan wrote: > I haven't put it into a longer more thought out doc yet, but here's a sketch > that explains the relationships: > https://drive.google.com/a/google.com/file/d/0B600QF7NoJmvdkMtbTN0V2JiRVU/vie... > > Basically, as you mention below, ownership and positional coordination should be > tied together. Each* ScrollableArea has a ScrollbarManager (that will now own > the Scrollbar). The ScrollableArea implements a ScrollbarContainer interface > that ScrollbarManager uses to get information from the ScrollableArea to > position, size and otherwise update the scrollbars. > > In this world, both RFV and the layout viewport FV/PLSA will have a > ScrollbarManager. When a FV/PLSA is made the layout viewport, RFV will simply > clear the ScrollbarManager on the layout viewport so that only RFV will handle > scrollbars. > > WDYT? > > *I use ScrollableArea here but I think the derived classes should be the ones to > hold a ScrollbarManager and implement ScrollbarContainer. For e.g. > VisualViewport doesn't need Scrollbars. Thanks, that all sounds great. So eventually we would expect that - ScrollbarManager::setScroller will go away (it will only "use" the ScrollableArea from its ctor), and - RFV::horizontalScrollbar, RFV::verticalScrollbar, etc. will delegate to RFV's ScrollbarManager instead of the layout viewport? If so, I am fine with this change as an intermediate step on that path. But let's add comments noting that these are temporary hacks.
On 2016/11/09 18:04:25, skobes wrote: > On 2016/11/09 15:50:14, bokan wrote: > > I haven't put it into a longer more thought out doc yet, but here's a sketch > > that explains the relationships: > > > https://drive.google.com/a/google.com/file/d/0B600QF7NoJmvdkMtbTN0V2JiRVU/vie... > > > > Basically, as you mention below, ownership and positional coordination should > be > > tied together. Each* ScrollableArea has a ScrollbarManager (that will now own > > the Scrollbar). The ScrollableArea implements a ScrollbarContainer interface > > that ScrollbarManager uses to get information from the ScrollableArea to > > position, size and otherwise update the scrollbars. > > > > In this world, both RFV and the layout viewport FV/PLSA will have a > > ScrollbarManager. When a FV/PLSA is made the layout viewport, RFV will simply > > clear the ScrollbarManager on the layout viewport so that only RFV will handle > > scrollbars. > > > > WDYT? > > > > *I use ScrollableArea here but I think the derived classes should be the ones > to > > hold a ScrollbarManager and implement ScrollbarContainer. For e.g. > > VisualViewport doesn't need Scrollbars. > > Thanks, that all sounds great. So eventually we would expect that > > - ScrollbarManager::setScroller will go away (it will only "use" the > ScrollableArea from its ctor), and > - RFV::horizontalScrollbar, RFV::verticalScrollbar, etc. will delegate to RFV's > ScrollbarManager instead of the layout viewport? Correct > > If so, I am fine with this change as an intermediate step on that path. But > let's add comments noting that these are temporary hacks.
On 2016/11/08 21:48:17, ymalik wrote: > On 2016/11/08 18:35:34, bokan wrote: > > On 2016/11/08 00:11:13, ymalik wrote: > > > @bokan/skobes, PTAL :) > > > > > > I keep running into merge conflicts and more failing tests. I'd like to land > > > this asap and leave other non-major concerns for a TODO if possible :) > > > > > > > > > https://codereview.chromium.org/2454913003/diff/160001/third_party/WebKit/Lay... > > > File > > > > > > third_party/WebKit/LayoutTests/platform/linux/svg/custom/masking-clipping-hidpi-expected.txt > > > (right): > > > > > > > > > https://codereview.chromium.org/2454913003/diff/160001/third_party/WebKit/Lay... > > > > > > third_party/WebKit/LayoutTests/platform/linux/svg/custom/masking-clipping-hidpi-expected.txt:3: > > > layer at (0,0) size 1600x600 > > > On 2016/11/04 21:33:20, skobes wrote: > > > > On 2016/11/04 19:23:59, bokan wrote: > > > > > Does the test not dump this text when notifyDone() is called? If not, > are > > > > there > > > > > any functions in testRunner/internals we can use to delay the dump until > > > we're > > > > > in a steady state? skobes@, any idea? > > > > > > > > The test runner calls updateAllLifecyclePhases before dumping the layout > > tree, > > > > so everything ought to be up to date (if not, it's a bug). > > > > > > So I did a bit more investigation. My original comment about timing in this > > test > > > doesn't make sense. > > > > > > Before this patch (where the scroller was FV and not RFV), here is roughly > the > > > sequence of events that took place when this test set the backing scale > factor > > > to 2: > > > 1) The visual viewport dimensions are changed (800x600 -> 1600x1200) but the > > > FV::frameRect dimensions are still (800x600) > > > 2) We start doing layout and the LayoutSVGRoot object size (which is where > the > > > PaintLayer size comes from) is changed, but only in one dimension (800x600 > -> > > > 1600x600) > > > 3) We get into FV::computeScrollbarExistence and determine that we need > > > scrollbars (800x600 < 1600x1200), and layout the scrollbars (updating the > > sizes > > > for the LayoutObjects to exclude the scrollbars).This sets the correct > > > dimensions on LayoutSVGRoot(1600x600 -> 1600x1200) > > > 4) We get into FV::computeScrollbarExistence again when the actual > > FV::frameRect > > > changes after layout and determine that we don't actually need the > scrollbars. > > > > > > With this patch, step 3) doesn't lay out the scrollbars because we use > > > VV:visibleContentRect which is updated in the very beginning and we never > end > > up > > > correcting the size of the LayoutSVGRoot. > > > > > > I don't understand why step 2 doesn't compute the correct height, but it > seems > > > like a bug. This CL just causes this test to highlight it. > > > > > > We have two ways forward, 1) Just update the layout test and see if we get > any > > > complaints. I suspect we wont. and 2) Just don't make this change if the > frame > > > document is SVG and investigate it more. > > > > > > I don't want to special case so I am in favor of 1), especially because this > > > looks broken anyway. > > > > Thanks for digging into this in detail. > > > > When I open that file in Chrome stable, making the window smaller than the > > contents doesn't make scrollbars appear. Do you know why the difference in the > > test? > > For some reason, the size of the SVG elements doesn't add to the size of the > document. The size of the document seems to be the same as the size of the > scroller, so we never add scrollers. Perhaps another bug? > > Sorry to be clear, the test also doesn't show the scrollbar, but we think* that > we need the scrollbar initially because the frame view rect is not assigned. > > > The resize from 600->1200 does sound a little fishy to me. It's possible > that's > > a bug in layout code but I don't know enough about that to say - skobes@ can > > probably say more here. > > > > This does bring up another potential issue though. Since we now use the visual > > viewport rect to calculate scrollbar existence, if a previously unscrollable > > page is zoomed and we add a scrollbar, does that scrollbar affect layout? I > > wouldn't expect it to, it should take space away from the visual viewport so > > that the user can scroll all the way to the content extent, but I suspect this > > might not work that way. Could you test this? Also, what happens if we zoom in > > and do a layout, does the layout change to accomodate the scrollbar / psf? > > For non-overlay scrollbars, addition and removal of scrollbars does a layout on > LayoutView and as a result, the layout objects and the layers reflect the new > dimensions. In the case where the scrollbar is added when you pinch zoom in, the > layout viewport is adjusted and you can scroll to see the entire content. This > CL doesn't change any behavior regarding when the layout should/shouldn't > happen. I don't entirely see the concern here. When you say "the layout objects and the layers reflect the new dimensions" does that mean the dimensions of the scaled scrollbar? I'm worried that as you're pinch zooming you're constantly doing relayout because your scrollbar is changing width w.r.t the page due to zoom.
On 2016/11/09 20:22:58, bokan wrote: > On 2016/11/09 18:04:25, skobes wrote: > > On 2016/11/09 15:50:14, bokan wrote: > > > I haven't put it into a longer more thought out doc yet, but here's a sketch > > > that explains the relationships: > > > > > > https://drive.google.com/a/google.com/file/d/0B600QF7NoJmvdkMtbTN0V2JiRVU/vie... > > > > > > Basically, as you mention below, ownership and positional coordination > should > > be > > > tied together. Each* ScrollableArea has a ScrollbarManager (that will now > own > > > the Scrollbar). The ScrollableArea implements a ScrollbarContainer interface > > > that ScrollbarManager uses to get information from the ScrollableArea to > > > position, size and otherwise update the scrollbars. > > > > > > In this world, both RFV and the layout viewport FV/PLSA will have a > > > ScrollbarManager. When a FV/PLSA is made the layout viewport, RFV will > simply > > > clear the ScrollbarManager on the layout viewport so that only RFV will > handle > > > scrollbars. > > > > > > WDYT? > > > > > > *I use ScrollableArea here but I think the derived classes should be the > ones > > to > > > hold a ScrollbarManager and implement ScrollbarContainer. For e.g. > > > VisualViewport doesn't need Scrollbars. > > > > Thanks, that all sounds great. So eventually we would expect that > > > > - ScrollbarManager::setScroller will go away (it will only "use" the > > ScrollableArea from its ctor), and > > - RFV::horizontalScrollbar, RFV::verticalScrollbar, etc. will delegate to > RFV's > > ScrollbarManager instead of the layout viewport? > > Correct > > > > > If so, I am fine with this change as an intermediate step on that path. But > > let's add comments noting that these are temporary hacks. Added TODOs to reflect this.
On 2016/11/09 20:30:26, bokan wrote: > On 2016/11/08 21:48:17, ymalik wrote: > > On 2016/11/08 18:35:34, bokan wrote: > > > On 2016/11/08 00:11:13, ymalik wrote: > > > > @bokan/skobes, PTAL :) > > > > > > > > I keep running into merge conflicts and more failing tests. I'd like to > land > > > > this asap and leave other non-major concerns for a TODO if possible :) > > > > > > > > > > > > > > https://codereview.chromium.org/2454913003/diff/160001/third_party/WebKit/Lay... > > > > File > > > > > > > > > > third_party/WebKit/LayoutTests/platform/linux/svg/custom/masking-clipping-hidpi-expected.txt > > > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2454913003/diff/160001/third_party/WebKit/Lay... > > > > > > > > > > third_party/WebKit/LayoutTests/platform/linux/svg/custom/masking-clipping-hidpi-expected.txt:3: > > > > layer at (0,0) size 1600x600 > > > > On 2016/11/04 21:33:20, skobes wrote: > > > > > On 2016/11/04 19:23:59, bokan wrote: > > > > > > Does the test not dump this text when notifyDone() is called? If not, > > are > > > > > there > > > > > > any functions in testRunner/internals we can use to delay the dump > until > > > > we're > > > > > > in a steady state? skobes@, any idea? > > > > > > > > > > The test runner calls updateAllLifecyclePhases before dumping the layout > > > tree, > > > > > so everything ought to be up to date (if not, it's a bug). > > > > > > > > So I did a bit more investigation. My original comment about timing in > this > > > test > > > > doesn't make sense. > > > > > > > > Before this patch (where the scroller was FV and not RFV), here is roughly > > the > > > > sequence of events that took place when this test set the backing scale > > factor > > > > to 2: > > > > 1) The visual viewport dimensions are changed (800x600 -> 1600x1200) but > the > > > > FV::frameRect dimensions are still (800x600) > > > > 2) We start doing layout and the LayoutSVGRoot object size (which is where > > the > > > > PaintLayer size comes from) is changed, but only in one dimension (800x600 > > -> > > > > 1600x600) > > > > 3) We get into FV::computeScrollbarExistence and determine that we need > > > > scrollbars (800x600 < 1600x1200), and layout the scrollbars (updating the > > > sizes > > > > for the LayoutObjects to exclude the scrollbars).This sets the correct > > > > dimensions on LayoutSVGRoot(1600x600 -> 1600x1200) > > > > 4) We get into FV::computeScrollbarExistence again when the actual > > > FV::frameRect > > > > changes after layout and determine that we don't actually need the > > scrollbars. > > > > > > > > With this patch, step 3) doesn't lay out the scrollbars because we use > > > > VV:visibleContentRect which is updated in the very beginning and we never > > end > > > up > > > > correcting the size of the LayoutSVGRoot. > > > > > > > > I don't understand why step 2 doesn't compute the correct height, but it > > seems > > > > like a bug. This CL just causes this test to highlight it. > > > > > > > > We have two ways forward, 1) Just update the layout test and see if we get > > any > > > > complaints. I suspect we wont. and 2) Just don't make this change if the > > frame > > > > document is SVG and investigate it more. > > > > > > > > I don't want to special case so I am in favor of 1), especially because > this > > > > looks broken anyway. > > > > > > Thanks for digging into this in detail. > > > > > > When I open that file in Chrome stable, making the window smaller than the > > > contents doesn't make scrollbars appear. Do you know why the difference in > the > > > test? > > > > For some reason, the size of the SVG elements doesn't add to the size of the > > document. The size of the document seems to be the same as the size of the > > scroller, so we never add scrollers. Perhaps another bug? > > > > Sorry to be clear, the test also doesn't show the scrollbar, but we think* > that > > we need the scrollbar initially because the frame view rect is not assigned. > > > > > The resize from 600->1200 does sound a little fishy to me. It's possible > > that's > > > a bug in layout code but I don't know enough about that to say - skobes@ can > > > probably say more here. > > > > > > This does bring up another potential issue though. Since we now use the > visual > > > viewport rect to calculate scrollbar existence, if a previously unscrollable > > > page is zoomed and we add a scrollbar, does that scrollbar affect layout? I > > > wouldn't expect it to, it should take space away from the visual viewport so > > > that the user can scroll all the way to the content extent, but I suspect > this > > > might not work that way. Could you test this? Also, what happens if we zoom > in > > > and do a layout, does the layout change to accomodate the scrollbar / psf? > > > > For non-overlay scrollbars, addition and removal of scrollbars does a layout > on > > LayoutView and as a result, the layout objects and the layers reflect the new > > dimensions. In the case where the scrollbar is added when you pinch zoom in, > the > > layout viewport is adjusted and you can scroll to see the entire content. This > > CL doesn't change any behavior regarding when the layout should/shouldn't > > happen. I don't entirely see the concern here. > > When you say "the layout objects and the layers reflect the new dimensions" does > that mean the dimensions of the scaled scrollbar? That's right. > I'm worried that as you're > pinch zooming you're constantly doing relayout because your scrollbar is > changing width w.r.t the page due to zoom. We only do a relayout when the existence of the scrollbar changes. So if you're just zooming in, we don't constantly re-layout because the scrollbar dimensions are changing. I think this behavior makes sense, but I agree that the visual viewport should be what shrinks when scrollbar shrinks or shows. I suspect that that's a bigger change and will require a bunch of test fixes. Does this address your concern?
On 2016/11/09 20:52:29, ymalik wrote: > On 2016/11/09 20:30:26, bokan wrote: > > On 2016/11/08 21:48:17, ymalik wrote: > > > On 2016/11/08 18:35:34, bokan wrote: > > > > On 2016/11/08 00:11:13, ymalik wrote: > > > > > @bokan/skobes, PTAL :) > > > > > > > > > > I keep running into merge conflicts and more failing tests. I'd like to > > land > > > > > this asap and leave other non-major concerns for a TODO if possible :) > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2454913003/diff/160001/third_party/WebKit/Lay... > > > > > File > > > > > > > > > > > > > > > third_party/WebKit/LayoutTests/platform/linux/svg/custom/masking-clipping-hidpi-expected.txt > > > > > (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2454913003/diff/160001/third_party/WebKit/Lay... > > > > > > > > > > > > > > > third_party/WebKit/LayoutTests/platform/linux/svg/custom/masking-clipping-hidpi-expected.txt:3: > > > > > layer at (0,0) size 1600x600 > > > > > On 2016/11/04 21:33:20, skobes wrote: > > > > > > On 2016/11/04 19:23:59, bokan wrote: > > > > > > > Does the test not dump this text when notifyDone() is called? If > not, > > > are > > > > > > there > > > > > > > any functions in testRunner/internals we can use to delay the dump > > until > > > > > we're > > > > > > > in a steady state? skobes@, any idea? > > > > > > > > > > > > The test runner calls updateAllLifecyclePhases before dumping the > layout > > > > tree, > > > > > > so everything ought to be up to date (if not, it's a bug). > > > > > > > > > > So I did a bit more investigation. My original comment about timing in > > this > > > > test > > > > > doesn't make sense. > > > > > > > > > > Before this patch (where the scroller was FV and not RFV), here is > roughly > > > the > > > > > sequence of events that took place when this test set the backing scale > > > factor > > > > > to 2: > > > > > 1) The visual viewport dimensions are changed (800x600 -> 1600x1200) but > > the > > > > > FV::frameRect dimensions are still (800x600) > > > > > 2) We start doing layout and the LayoutSVGRoot object size (which is > where > > > the > > > > > PaintLayer size comes from) is changed, but only in one dimension > (800x600 > > > -> > > > > > 1600x600) > > > > > 3) We get into FV::computeScrollbarExistence and determine that we need > > > > > scrollbars (800x600 < 1600x1200), and layout the scrollbars (updating > the > > > > sizes > > > > > for the LayoutObjects to exclude the scrollbars).This sets the correct > > > > > dimensions on LayoutSVGRoot(1600x600 -> 1600x1200) > > > > > 4) We get into FV::computeScrollbarExistence again when the actual > > > > FV::frameRect > > > > > changes after layout and determine that we don't actually need the > > > scrollbars. > > > > > > > > > > With this patch, step 3) doesn't lay out the scrollbars because we use > > > > > VV:visibleContentRect which is updated in the very beginning and we > never > > > end > > > > up > > > > > correcting the size of the LayoutSVGRoot. > > > > > > > > > > I don't understand why step 2 doesn't compute the correct height, but it > > > seems > > > > > like a bug. This CL just causes this test to highlight it. > > > > > > > > > > We have two ways forward, 1) Just update the layout test and see if we > get > > > any > > > > > complaints. I suspect we wont. and 2) Just don't make this change if the > > > frame > > > > > document is SVG and investigate it more. > > > > > > > > > > I don't want to special case so I am in favor of 1), especially because > > this > > > > > looks broken anyway. > > > > > > > > Thanks for digging into this in detail. > > > > > > > > When I open that file in Chrome stable, making the window smaller than the > > > > contents doesn't make scrollbars appear. Do you know why the difference in > > the > > > > test? > > > > > > For some reason, the size of the SVG elements doesn't add to the size of the > > > document. The size of the document seems to be the same as the size of the > > > scroller, so we never add scrollers. Perhaps another bug? > > > > > > Sorry to be clear, the test also doesn't show the scrollbar, but we think* > > that > > > we need the scrollbar initially because the frame view rect is not assigned. > > > > > > > > The resize from 600->1200 does sound a little fishy to me. It's possible > > > that's > > > > a bug in layout code but I don't know enough about that to say - skobes@ > can > > > > probably say more here. > > > > > > > > This does bring up another potential issue though. Since we now use the > > visual > > > > viewport rect to calculate scrollbar existence, if a previously > unscrollable > > > > page is zoomed and we add a scrollbar, does that scrollbar affect layout? > I > > > > wouldn't expect it to, it should take space away from the visual viewport > so > > > > that the user can scroll all the way to the content extent, but I suspect > > this > > > > might not work that way. Could you test this? Also, what happens if we > zoom > > in > > > > and do a layout, does the layout change to accomodate the scrollbar / psf? > > > > > > For non-overlay scrollbars, addition and removal of scrollbars does a layout > > on > > > LayoutView and as a result, the layout objects and the layers reflect the > new > > > dimensions. In the case where the scrollbar is added when you pinch zoom in, > > the > > > layout viewport is adjusted and you can scroll to see the entire content. > This > > > CL doesn't change any behavior regarding when the layout should/shouldn't > > > happen. I don't entirely see the concern here. > > > > When you say "the layout objects and the layers reflect the new dimensions" > does > > that mean the dimensions of the scaled scrollbar? > > That's right. > > > I'm worried that as you're > > pinch zooming you're constantly doing relayout because your scrollbar is > > changing width w.r.t the page due to zoom. > > We only do a relayout when the existence of the scrollbar changes. So if you're > just zooming in, we don't constantly re-layout because the scrollbar dimensions > are changing. I think this behavior makes sense, but I agree that the visual > viewport should be what shrinks when scrollbar shrinks or shows. I suspect that > that's a bigger change and will require a bunch of test fixes. > > Does this address your concern? Almost :) Does it not cause layout by the "accident" that nothing on the page changed? Or is it because the LayoutView/Box is actually using the unscaled scrollbar dimension. Put another way, if you setNeedsLayout on each tick during a zoom, will we actually resize the page contents each tick? Temporarily, I'd be fine with landing in either case since I think we can rely on setNeedsLayout not being called but we should fix that shortly after. Both for performance and rationality reasons.
On 2016/11/09 21:07:01, bokan wrote: > On 2016/11/09 20:52:29, ymalik wrote: > > On 2016/11/09 20:30:26, bokan wrote: > > > On 2016/11/08 21:48:17, ymalik wrote: > > > > On 2016/11/08 18:35:34, bokan wrote: > > > > > On 2016/11/08 00:11:13, ymalik wrote: > > > > > > @bokan/skobes, PTAL :) > > > > > > > > > > > > I keep running into merge conflicts and more failing tests. I'd like > to > > > land > > > > > > this asap and leave other non-major concerns for a TODO if possible :) > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2454913003/diff/160001/third_party/WebKit/Lay... > > > > > > File > > > > > > > > > > > > > > > > > > > > > third_party/WebKit/LayoutTests/platform/linux/svg/custom/masking-clipping-hidpi-expected.txt > > > > > > (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2454913003/diff/160001/third_party/WebKit/Lay... > > > > > > > > > > > > > > > > > > > > > third_party/WebKit/LayoutTests/platform/linux/svg/custom/masking-clipping-hidpi-expected.txt:3: > > > > > > layer at (0,0) size 1600x600 > > > > > > On 2016/11/04 21:33:20, skobes wrote: > > > > > > > On 2016/11/04 19:23:59, bokan wrote: > > > > > > > > Does the test not dump this text when notifyDone() is called? If > > not, > > > > are > > > > > > > there > > > > > > > > any functions in testRunner/internals we can use to delay the dump > > > until > > > > > > we're > > > > > > > > in a steady state? skobes@, any idea? > > > > > > > > > > > > > > The test runner calls updateAllLifecyclePhases before dumping the > > layout > > > > > tree, > > > > > > > so everything ought to be up to date (if not, it's a bug). > > > > > > > > > > > > So I did a bit more investigation. My original comment about timing in > > > this > > > > > test > > > > > > doesn't make sense. > > > > > > > > > > > > Before this patch (where the scroller was FV and not RFV), here is > > roughly > > > > the > > > > > > sequence of events that took place when this test set the backing > scale > > > > factor > > > > > > to 2: > > > > > > 1) The visual viewport dimensions are changed (800x600 -> 1600x1200) > but > > > the > > > > > > FV::frameRect dimensions are still (800x600) > > > > > > 2) We start doing layout and the LayoutSVGRoot object size (which is > > where > > > > the > > > > > > PaintLayer size comes from) is changed, but only in one dimension > > (800x600 > > > > -> > > > > > > 1600x600) > > > > > > 3) We get into FV::computeScrollbarExistence and determine that we > need > > > > > > scrollbars (800x600 < 1600x1200), and layout the scrollbars (updating > > the > > > > > sizes > > > > > > for the LayoutObjects to exclude the scrollbars).This sets the correct > > > > > > dimensions on LayoutSVGRoot(1600x600 -> 1600x1200) > > > > > > 4) We get into FV::computeScrollbarExistence again when the actual > > > > > FV::frameRect > > > > > > changes after layout and determine that we don't actually need the > > > > scrollbars. > > > > > > > > > > > > With this patch, step 3) doesn't lay out the scrollbars because we use > > > > > > VV:visibleContentRect which is updated in the very beginning and we > > never > > > > end > > > > > up > > > > > > correcting the size of the LayoutSVGRoot. > > > > > > > > > > > > I don't understand why step 2 doesn't compute the correct height, but > it > > > > seems > > > > > > like a bug. This CL just causes this test to highlight it. > > > > > > > > > > > > We have two ways forward, 1) Just update the layout test and see if we > > get > > > > any > > > > > > complaints. I suspect we wont. and 2) Just don't make this change if > the > > > > frame > > > > > > document is SVG and investigate it more. > > > > > > > > > > > > I don't want to special case so I am in favor of 1), especially > because > > > this > > > > > > looks broken anyway. > > > > > > > > > > Thanks for digging into this in detail. > > > > > > > > > > When I open that file in Chrome stable, making the window smaller than > the > > > > > contents doesn't make scrollbars appear. Do you know why the difference > in > > > the > > > > > test? > > > > > > > > For some reason, the size of the SVG elements doesn't add to the size of > the > > > > document. The size of the document seems to be the same as the size of the > > > > scroller, so we never add scrollers. Perhaps another bug? > > > > > > > > Sorry to be clear, the test also doesn't show the scrollbar, but we think* > > > that > > > > we need the scrollbar initially because the frame view rect is not > assigned. > > > > > > > > > > > The resize from 600->1200 does sound a little fishy to me. It's possible > > > > that's > > > > > a bug in layout code but I don't know enough about that to say - skobes@ > > can > > > > > probably say more here. > > > > > > > > > > This does bring up another potential issue though. Since we now use the > > > visual > > > > > viewport rect to calculate scrollbar existence, if a previously > > unscrollable > > > > > page is zoomed and we add a scrollbar, does that scrollbar affect > layout? > > I > > > > > wouldn't expect it to, it should take space away from the visual > viewport > > so > > > > > that the user can scroll all the way to the content extent, but I > suspect > > > this > > > > > might not work that way. Could you test this? Also, what happens if we > > zoom > > > in > > > > > and do a layout, does the layout change to accomodate the scrollbar / > psf? > > > > > > > > For non-overlay scrollbars, addition and removal of scrollbars does a > layout > > > on > > > > LayoutView and as a result, the layout objects and the layers reflect the > > new > > > > dimensions. In the case where the scrollbar is added when you pinch zoom > in, > > > the > > > > layout viewport is adjusted and you can scroll to see the entire content. > > This > > > > CL doesn't change any behavior regarding when the layout should/shouldn't > > > > happen. I don't entirely see the concern here. > > > > > > When you say "the layout objects and the layers reflect the new dimensions" > > does > > > that mean the dimensions of the scaled scrollbar? > > > > That's right. > > > > > I'm worried that as you're > > > pinch zooming you're constantly doing relayout because your scrollbar is > > > changing width w.r.t the page due to zoom. > > > > We only do a relayout when the existence of the scrollbar changes. So if > you're > > just zooming in, we don't constantly re-layout because the scrollbar > dimensions > > are changing. I think this behavior makes sense, but I agree that the visual > > viewport should be what shrinks when scrollbar shrinks or shows. I suspect > that > > that's a bigger change and will require a bunch of test fixes. > > > > Does this address your concern? > > Almost :) > > Does it not cause layout by the "accident" that nothing on the page changed? Or > is it because the LayoutView/Box is actually using the unscaled scrollbar > dimension. Put another way, if you setNeedsLayout on each tick during a zoom, > will we actually resize the page contents each tick? It's sort of neither. The layout is caused by the fact that something on the page changed, i.e, the addition/removal of scrollbars. The LayoutView/Box is using the scrollbar dimension from the layout caused by the first time the scrollbar existence changed. Obviously this is non-ideal since it's inconsistent, and we should just not do a layout on the view and simply resize the visual viewport. This is probably less of a concern if we go with overlay scrollbars cross platform. As an aside discussion, why don't we just use the same creation logic as overlay scrollbars, with the difference that the scrollbar is always visible and styled differently? > > Temporarily, I'd be fine with landing in either case since I think we can rely > on setNeedsLayout not being called but we should fix that shortly after. Both > for performance and rationality reasons. Agreed.
On 2016/11/09 21:15:15, ymalik wrote: > On 2016/11/09 21:07:01, bokan wrote: > > On 2016/11/09 20:52:29, ymalik wrote: > > > On 2016/11/09 20:30:26, bokan wrote: > > > > On 2016/11/08 21:48:17, ymalik wrote: > > > > > On 2016/11/08 18:35:34, bokan wrote: > > > > > > On 2016/11/08 00:11:13, ymalik wrote: > > > > > > > @bokan/skobes, PTAL :) > > > > > > > > > > > > > > I keep running into merge conflicts and more failing tests. I'd like > > to > > > > land > > > > > > > this asap and leave other non-major concerns for a TODO if possible > :) > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2454913003/diff/160001/third_party/WebKit/Lay... > > > > > > > File > > > > > > > > > > > > > > > > > > > > > > > > > > > > third_party/WebKit/LayoutTests/platform/linux/svg/custom/masking-clipping-hidpi-expected.txt > > > > > > > (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2454913003/diff/160001/third_party/WebKit/Lay... > > > > > > > > > > > > > > > > > > > > > > > > > > > > third_party/WebKit/LayoutTests/platform/linux/svg/custom/masking-clipping-hidpi-expected.txt:3: > > > > > > > layer at (0,0) size 1600x600 > > > > > > > On 2016/11/04 21:33:20, skobes wrote: > > > > > > > > On 2016/11/04 19:23:59, bokan wrote: > > > > > > > > > Does the test not dump this text when notifyDone() is called? If > > > not, > > > > > are > > > > > > > > there > > > > > > > > > any functions in testRunner/internals we can use to delay the > dump > > > > until > > > > > > > we're > > > > > > > > > in a steady state? skobes@, any idea? > > > > > > > > > > > > > > > > The test runner calls updateAllLifecyclePhases before dumping the > > > layout > > > > > > tree, > > > > > > > > so everything ought to be up to date (if not, it's a bug). > > > > > > > > > > > > > > So I did a bit more investigation. My original comment about timing > in > > > > this > > > > > > test > > > > > > > doesn't make sense. > > > > > > > > > > > > > > Before this patch (where the scroller was FV and not RFV), here is > > > roughly > > > > > the > > > > > > > sequence of events that took place when this test set the backing > > scale > > > > > factor > > > > > > > to 2: > > > > > > > 1) The visual viewport dimensions are changed (800x600 -> 1600x1200) > > but > > > > the > > > > > > > FV::frameRect dimensions are still (800x600) > > > > > > > 2) We start doing layout and the LayoutSVGRoot object size (which is > > > where > > > > > the > > > > > > > PaintLayer size comes from) is changed, but only in one dimension > > > (800x600 > > > > > -> > > > > > > > 1600x600) > > > > > > > 3) We get into FV::computeScrollbarExistence and determine that we > > need > > > > > > > scrollbars (800x600 < 1600x1200), and layout the scrollbars > (updating > > > the > > > > > > sizes > > > > > > > for the LayoutObjects to exclude the scrollbars).This sets the > correct > > > > > > > dimensions on LayoutSVGRoot(1600x600 -> 1600x1200) > > > > > > > 4) We get into FV::computeScrollbarExistence again when the actual > > > > > > FV::frameRect > > > > > > > changes after layout and determine that we don't actually need the > > > > > scrollbars. > > > > > > > > > > > > > > With this patch, step 3) doesn't lay out the scrollbars because we > use > > > > > > > VV:visibleContentRect which is updated in the very beginning and we > > > never > > > > > end > > > > > > up > > > > > > > correcting the size of the LayoutSVGRoot. > > > > > > > > > > > > > > I don't understand why step 2 doesn't compute the correct height, > but > > it > > > > > seems > > > > > > > like a bug. This CL just causes this test to highlight it. > > > > > > > > > > > > > > We have two ways forward, 1) Just update the layout test and see if > we > > > get > > > > > any > > > > > > > complaints. I suspect we wont. and 2) Just don't make this change if > > the > > > > > frame > > > > > > > document is SVG and investigate it more. > > > > > > > > > > > > > > I don't want to special case so I am in favor of 1), especially > > because > > > > this > > > > > > > looks broken anyway. > > > > > > > > > > > > Thanks for digging into this in detail. > > > > > > > > > > > > When I open that file in Chrome stable, making the window smaller than > > the > > > > > > contents doesn't make scrollbars appear. Do you know why the > difference > > in > > > > the > > > > > > test? > > > > > > > > > > For some reason, the size of the SVG elements doesn't add to the size of > > the > > > > > document. The size of the document seems to be the same as the size of > the > > > > > scroller, so we never add scrollers. Perhaps another bug? > > > > > > > > > > Sorry to be clear, the test also doesn't show the scrollbar, but we > think* > > > > that > > > > > we need the scrollbar initially because the frame view rect is not > > assigned. > > > > > > > > > > > > > > The resize from 600->1200 does sound a little fishy to me. It's > possible > > > > > that's > > > > > > a bug in layout code but I don't know enough about that to say - > skobes@ > > > can > > > > > > probably say more here. > > > > > > > > > > > > This does bring up another potential issue though. Since we now use > the > > > > visual > > > > > > viewport rect to calculate scrollbar existence, if a previously > > > unscrollable > > > > > > page is zoomed and we add a scrollbar, does that scrollbar affect > > layout? > > > I > > > > > > wouldn't expect it to, it should take space away from the visual > > viewport > > > so > > > > > > that the user can scroll all the way to the content extent, but I > > suspect > > > > this > > > > > > might not work that way. Could you test this? Also, what happens if we > > > zoom > > > > in > > > > > > and do a layout, does the layout change to accomodate the scrollbar / > > psf? > > > > > > > > > > For non-overlay scrollbars, addition and removal of scrollbars does a > > layout > > > > on > > > > > LayoutView and as a result, the layout objects and the layers reflect > the > > > new > > > > > dimensions. In the case where the scrollbar is added when you pinch zoom > > in, > > > > the > > > > > layout viewport is adjusted and you can scroll to see the entire > content. > > > This > > > > > CL doesn't change any behavior regarding when the layout > should/shouldn't > > > > > happen. I don't entirely see the concern here. > > > > > > > > When you say "the layout objects and the layers reflect the new > dimensions" > > > does > > > > that mean the dimensions of the scaled scrollbar? > > > > > > That's right. > > > > > > > I'm worried that as you're > > > > pinch zooming you're constantly doing relayout because your scrollbar is > > > > changing width w.r.t the page due to zoom. > > > > > > We only do a relayout when the existence of the scrollbar changes. So if > > you're > > > just zooming in, we don't constantly re-layout because the scrollbar > > dimensions > > > are changing. I think this behavior makes sense, but I agree that the visual > > > viewport should be what shrinks when scrollbar shrinks or shows. I suspect > > that > > > that's a bigger change and will require a bunch of test fixes. > > > > > > Does this address your concern? > > > > Almost :) > > > > Does it not cause layout by the "accident" that nothing on the page changed? > Or > > is it because the LayoutView/Box is actually using the unscaled scrollbar > > dimension. Put another way, if you setNeedsLayout on each tick during a zoom, > > will we actually resize the page contents each tick? > > It's sort of neither. The layout is caused by the fact that something on the > page changed, i.e, the addition/removal of scrollbars. The LayoutView/Box is > using the scrollbar dimension from the layout caused by the first time the > scrollbar existence changed. The scrollbar width is cached across layouts rather than queried? Can you point me to where that happens? >Obviously this is non-ideal since it's > inconsistent, and we should just not do a layout on the view and simply resize > the visual viewport. I don't think it's that simple since there's web compat issues here. If you're fully zoomed out, a scrollbar must affect the layout size. Anyway, we can talk about how it should work in a separate discussion. > This is probably less of a concern if we go with overlay > scrollbars cross platform. I'm not sure this will ever happen on Windows and that's most of our desktop users so we can't rely on that. > As an aside discussion, why don't we just use the same creation logic as overlay > scrollbars, with the difference that the scrollbar is always visible and styled > differently? I'm not sure I follow. We do use the same logic for overlays. Or are you talking about the VisualViewport created overlays? Those are much simpler because they don't interact at all with layout, regular scrollbars do. > > > > > Temporarily, I'd be fine with landing in either case since I think we can rely > > on setNeedsLayout not being called but we should fix that shortly after. Both > > for performance and rationality reasons. > > Agreed. Ok, if Steve's fine with this it's lgtm from me.
https://codereview.chromium.org/2454913003/diff/160001/third_party/WebKit/Lay... > > > > > > > > File > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > third_party/WebKit/LayoutTests/platform/linux/svg/custom/masking-clipping-hidpi-expected.txt > > > > > > > > (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2454913003/diff/160001/third_party/WebKit/Lay... > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > third_party/WebKit/LayoutTests/platform/linux/svg/custom/masking-clipping-hidpi-expected.txt:3: > > > > > > > > layer at (0,0) size 1600x600 > > > > > > > > On 2016/11/04 21:33:20, skobes wrote: > > > > > > > > > On 2016/11/04 19:23:59, bokan wrote: > > > > > > > > > > Does the test not dump this text when notifyDone() is called? > If > > > > not, > > > > > > are > > > > > > > > > there > > > > > > > > > > any functions in testRunner/internals we can use to delay the > > dump > > > > > until > > > > > > > > we're > > > > > > > > > > in a steady state? skobes@, any idea? > > > > > > > > > > > > > > > > > > The test runner calls updateAllLifecyclePhases before dumping > the > > > > layout > > > > > > > tree, > > > > > > > > > so everything ought to be up to date (if not, it's a bug). > > > > > > > > > > > > > > > > So I did a bit more investigation. My original comment about > timing > > in > > > > > this > > > > > > > test > > > > > > > > doesn't make sense. > > > > > > > > > > > > > > > > Before this patch (where the scroller was FV and not RFV), here is > > > > roughly > > > > > > the > > > > > > > > sequence of events that took place when this test set the backing > > > scale > > > > > > factor > > > > > > > > to 2: > > > > > > > > 1) The visual viewport dimensions are changed (800x600 -> > 1600x1200) > > > but > > > > > the > > > > > > > > FV::frameRect dimensions are still (800x600) > > > > > > > > 2) We start doing layout and the LayoutSVGRoot object size (which > is > > > > where > > > > > > the > > > > > > > > PaintLayer size comes from) is changed, but only in one dimension > > > > (800x600 > > > > > > -> > > > > > > > > 1600x600) > > > > > > > > 3) We get into FV::computeScrollbarExistence and determine that we > > > need > > > > > > > > scrollbars (800x600 < 1600x1200), and layout the scrollbars > > (updating > > > > the > > > > > > > sizes > > > > > > > > for the LayoutObjects to exclude the scrollbars).This sets the > > correct > > > > > > > > dimensions on LayoutSVGRoot(1600x600 -> 1600x1200) > > > > > > > > 4) We get into FV::computeScrollbarExistence again when the actual > > > > > > > FV::frameRect > > > > > > > > changes after layout and determine that we don't actually need the > > > > > > scrollbars. > > > > > > > > > > > > > > > > With this patch, step 3) doesn't lay out the scrollbars because we > > use > > > > > > > > VV:visibleContentRect which is updated in the very beginning and > we > > > > never > > > > > > end > > > > > > > up > > > > > > > > correcting the size of the LayoutSVGRoot. > > > > > > > > > > > > > > > > I don't understand why step 2 doesn't compute the correct height, > > but > > > it > > > > > > seems > > > > > > > > like a bug. This CL just causes this test to highlight it. > > > > > > > > > > > > > > > > We have two ways forward, 1) Just update the layout test and see > if > > we > > > > get > > > > > > any > > > > > > > > complaints. I suspect we wont. and 2) Just don't make this change > if > > > the > > > > > > frame > > > > > > > > document is SVG and investigate it more. > > > > > > > > > > > > > > > > I don't want to special case so I am in favor of 1), especially > > > because > > > > > this > > > > > > > > looks broken anyway. > > > > > > > > > > > > > > Thanks for digging into this in detail. > > > > > > > > > > > > > > When I open that file in Chrome stable, making the window smaller > than > > > the > > > > > > > contents doesn't make scrollbars appear. Do you know why the > > difference > > > in > > > > > the > > > > > > > test? > > > > > > > > > > > > For some reason, the size of the SVG elements doesn't add to the size > of > > > the > > > > > > document. The size of the document seems to be the same as the size of > > the > > > > > > scroller, so we never add scrollers. Perhaps another bug? > > > > > > > > > > > > Sorry to be clear, the test also doesn't show the scrollbar, but we > > think* > > > > > that > > > > > > we need the scrollbar initially because the frame view rect is not > > > assigned. > > > > > > > > > > > > > > > > > The resize from 600->1200 does sound a little fishy to me. It's > > possible > > > > > > that's > > > > > > > a bug in layout code but I don't know enough about that to say - > > skobes@ > > > > can > > > > > > > probably say more here. > > > > > > > > > > > > > > This does bring up another potential issue though. Since we now use > > the > > > > > visual > > > > > > > viewport rect to calculate scrollbar existence, if a previously > > > > unscrollable > > > > > > > page is zoomed and we add a scrollbar, does that scrollbar affect > > > layout? > > > > I > > > > > > > wouldn't expect it to, it should take space away from the visual > > > viewport > > > > so > > > > > > > that the user can scroll all the way to the content extent, but I > > > suspect > > > > > this > > > > > > > might not work that way. Could you test this? Also, what happens if > we > > > > zoom > > > > > in > > > > > > > and do a layout, does the layout change to accomodate the scrollbar > / > > > psf? > > > > > > > > > > > > For non-overlay scrollbars, addition and removal of scrollbars does a > > > layout > > > > > on > > > > > > LayoutView and as a result, the layout objects and the layers reflect > > the > > > > new > > > > > > dimensions. In the case where the scrollbar is added when you pinch > zoom > > > in, > > > > > the > > > > > > layout viewport is adjusted and you can scroll to see the entire > > content. > > > > This > > > > > > CL doesn't change any behavior regarding when the layout > > should/shouldn't > > > > > > happen. I don't entirely see the concern here. > > > > > > > > > > When you say "the layout objects and the layers reflect the new > > dimensions" > > > > does > > > > > that mean the dimensions of the scaled scrollbar? > > > > > > > > That's right. > > > > > > > > > I'm worried that as you're > > > > > pinch zooming you're constantly doing relayout because your scrollbar is > > > > > changing width w.r.t the page due to zoom. > > > > > > > > We only do a relayout when the existence of the scrollbar changes. So if > > > you're > > > > just zooming in, we don't constantly re-layout because the scrollbar > > > dimensions > > > > are changing. I think this behavior makes sense, but I agree that the > visual > > > > viewport should be what shrinks when scrollbar shrinks or shows. I suspect > > > that > > > > that's a bigger change and will require a bunch of test fixes. > > > > > > > > Does this address your concern? > > > > > > Almost :) > > > > > > Does it not cause layout by the "accident" that nothing on the page changed? > > Or > > > is it because the LayoutView/Box is actually using the unscaled scrollbar > > > dimension. Put another way, if you setNeedsLayout on each tick during a > zoom, > > > will we actually resize the page contents each tick? > > > > It's sort of neither. The layout is caused by the fact that something on the > > page changed, i.e, the addition/removal of scrollbars. The LayoutView/Box is > > using the scrollbar dimension from the layout caused by the first time the > > scrollbar existence changed. > > The scrollbar width is cached across layouts rather than queried? Can you point > me to where that happens? Not quite cached. It's sort of an accident. This code calls contentResized if the scrollbar existence changes, which is what triggers the layout. https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/Fra... > >Obviously this is non-ideal since it's > > inconsistent, and we should just not do a layout on the view and simply resize > > the visual viewport. > > I don't think it's that simple since there's web compat issues here. If you're > fully zoomed out, a scrollbar must affect the layout size. Anyway, we can talk > about how it should work in a separate discussion. Ack > > This is probably less of a concern if we go with overlay > > scrollbars cross platform. > > I'm not sure this will ever happen on Windows and that's most of our desktop > users so we can't rely on that. Ack > > As an aside discussion, why don't we just use the same creation logic as > overlay > > scrollbars, with the difference that the scrollbar is always visible and > styled > > differently? > > I'm not sure I follow. We do use the same logic for overlays. Or are you talking > about the VisualViewport created overlays? Those are much simpler because they > don't interact at all with layout, regular scrollbars do. Lets talk about this in a separate discussion. > > > > > > > > Temporarily, I'd be fine with landing in either case since I think we can > rely > > > on setNeedsLayout not being called but we should fix that shortly after. > Both > > > for performance and rationality reasons. > > > > Agreed. > > Ok, if Steve's fine with this it's lgtm from me. @skobes, PTAL :)
> Not quite cached. It's sort of an accident. This code calls contentResized if > the scrollbar existence changes, which is what triggers the layout. > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/Fra... That's what kicks off the layout. But layout could occur for any other number of reasons (e.g. script reading a property). In that case, what value would layout use for the width of the scrollbar?
On 2016/11/09 21:33:05, bokan wrote: > That's what kicks off the layout. But layout could occur for any other number of > reasons (e.g. script reading a property). In that case, what value would layout > use for the width of the scrollbar? This bug is about pinch zoom, right? Pinch zoom doesn't change layout dimensions - the scrollbar should have the same width in LayoutUnits even if the ratio of LayoutUnit : DIP is changing. Or am I missing something?
On 2016/11/09 21:52:01, skobes wrote: > On 2016/11/09 21:33:05, bokan wrote: > > That's what kicks off the layout. But layout could occur for any other number > of > > reasons (e.g. script reading a property). In that case, what value would > layout > > use for the width of the scrollbar? > > This bug is about pinch zoom, right? Pinch zoom doesn't change layout > dimensions - the scrollbar should have the same width in LayoutUnits even if the > ratio of LayoutUnit : DIP is changing. Or am I missing something? Right, that's what I'm trying to confirm is still true after this patch. Though I may have gotten confused about what we're scaling, I'm guessing we just use Scrollbar's Widget rect so in that case we're fine.
On 2016/11/09 21:57:45, bokan wrote: > On 2016/11/09 21:52:01, skobes wrote: > > On 2016/11/09 21:33:05, bokan wrote: > > > That's what kicks off the layout. But layout could occur for any other > number > > of > > > reasons (e.g. script reading a property). In that case, what value would > > layout > > > use for the width of the scrollbar? > > > > This bug is about pinch zoom, right? Pinch zoom doesn't change layout > > dimensions - the scrollbar should have the same width in LayoutUnits even if > the > > ratio of LayoutUnit : DIP is changing. Or am I missing something? > > Right, that's what I'm trying to confirm is still true after this patch. Though > I may have gotten confused about what we're scaling, I'm guessing we just use > Scrollbar's Widget rect so in that case we're fine. That's correct, the layout dimensions aren't changed. @bokan, I thought you were asking if pinch zooming does a layout every time the page scale factor changes. The dimensions of LayoutView only changes when the existence of the scrollbar changes.
The CQ bit was checked by ymalik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2454913003/diff/200001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/platform/linux/svg/custom/masking-clipping-hidpi-expected.txt (right): https://codereview.chromium.org/2454913003/diff/200001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/platform/linux/svg/custom/masking-clipping-hidpi-expected.txt:7: LayoutSVGRect {rect} at (0,0) size 1600x600 [fill={[type=SOLID] [color=#FFFFFF]}] [x=0.00] [y=0.00] [width=800.00] [height=300.00] What was the outcome here? It seems wrong for the sizes to double on only one axis. If it's not easily fixable I'd rather we mark it failing and file a bug to follow up, instead of just updating the expectation. https://codereview.chromium.org/2454913003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2454913003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/FrameView.cpp:2463: // Tickmarks for main frame are stored in RFV. Why do we need to store tickmarks in RFV? Tickmarks aren't really a ScrollableArea concept, it seems like we can leave them on FrameView. https://codereview.chromium.org/2454913003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h (right): https://codereview.chromium.org/2454913003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h:340: void updateScrollbars() override{}; This method should just go away right? Once RFV owns its own scrollbars it can update them directly. https://codereview.chromium.org/2454913003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/tests/VisualViewportTest.cpp (right): https://codereview.chromium.org/2454913003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/web/tests/VisualViewportTest.cpp:312: // TODO(ymalik): With root layer scrolling enabled, the scroller returned If the test doesn't work with RLS, it shouldn't be parameterized. Don't fake it by checking the RLS setting.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
https://codereview.chromium.org/2454913003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/tests/VisualViewportTest.cpp (right): https://codereview.chromium.org/2454913003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/web/tests/VisualViewportTest.cpp:312: // TODO(ymalik): With root layer scrolling enabled, the scroller returned On 2016/11/09 23:18:25, skobes wrote: > If the test doesn't work with RLS, it shouldn't be parameterized. Don't fake it > by checking the RLS setting. I asked for this. The test should eventually work with RLS but doesn't yet. Checking for behavior with RLS will make sure we remember to update this when we fix RLS to work here as well (since the test will "break" if RLS is fixed).
https://codereview.chromium.org/2454913003/diff/200001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/platform/linux/svg/custom/masking-clipping-hidpi-expected.txt (right): https://codereview.chromium.org/2454913003/diff/200001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/platform/linux/svg/custom/masking-clipping-hidpi-expected.txt:7: LayoutSVGRect {rect} at (0,0) size 1600x600 [fill={[type=SOLID] [color=#FFFFFF]}] [x=0.00] [y=0.00] [width=800.00] [height=300.00] On 2016/11/09 23:18:24, skobes wrote: > What was the outcome here? > > It seems wrong for the sizes to double on only one axis. If it's not easily > fixable I'd rather we mark it failing and file a bug to follow up, instead of > just updating the expectation. Yes I agree that this is better. I can't figure out what's going on with the update only in one dimension and don't want to block this CL on it. Filed crbug.com/664206 for it and marked this test as failing. https://codereview.chromium.org/2454913003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2454913003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/FrameView.cpp:2463: // Tickmarks for main frame are stored in RFV. On 2016/11/09 23:18:25, skobes wrote: > Why do we need to store tickmarks in RFV? Tickmarks aren't really a > ScrollableArea concept, it seems like we can leave them on FrameView. I had to do this to make a test pass with root layer scrolling, but we don't actually need it. Why I had it: setTickmarks has callers that only call it on FV (frameView->setStickmarks) getTickmarks is currently only called from Scrollbar.cpp (it does m_scroller->getTickmarks()). In this CL, m_scroller can be RFV. Calling layoutViewport().getTickmarks() from RFV didn't work for root layer scrolling (and causes test failures). This is because setTickmarks would set it on FV, and getTickmarks would get called on RFV which would in turn call it on PLSA. So I had to store the tickmarks in RFV. I guess for now, calling FV's getTickmarks from PLSA makes the test pass. I have a todo there to figure out tickmarks for root layer scrolling. https://codereview.chromium.org/2454913003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h (right): https://codereview.chromium.org/2454913003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h:340: void updateScrollbars() override{}; On 2016/11/09 23:18:25, skobes wrote: > This method should just go away right? Once RFV owns its own scrollbars it can > update them directly. Yeah. I added a comment in the parent class and and removed this one. https://codereview.chromium.org/2454913003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/tests/VisualViewportTest.cpp (right): https://codereview.chromium.org/2454913003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/web/tests/VisualViewportTest.cpp:312: // TODO(ymalik): With root layer scrolling enabled, the scroller returned On 2016/11/10 15:16:57, bokan wrote: > On 2016/11/09 23:18:25, skobes wrote: > > If the test doesn't work with RLS, it shouldn't be parameterized. Don't fake > it > > by checking the RLS setting. > > I asked for this. The test should eventually work with RLS but doesn't yet. > Checking for behavior with RLS will make sure we remember to update this when we > fix RLS to work here as well (since the test will "break" if RLS is fixed). I agree with this. It will increase the chances that this gets fixed when RLS is enabled by default. @skobes, WDYT?
@skobes, PTAL :)
The CQ bit was checked by ymalik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/11/10 17:58:04, ymalik wrote: > On 2016/11/10 15:16:57, bokan wrote: > > I asked for this. The test should eventually work with RLS but doesn't yet. > > Checking for behavior with RLS will make sure we remember to update this when we > > fix RLS to work here as well (since the test will "break" if RLS is fixed). > > I agree with this. It will increase the chances that this gets fixed when RLS is > enabled by default. @skobes, WDYT? Sorry, I don't understand what you mean by "increase the chance this gets fixed". We use failing tests as an indicator of what work remains for RLS. If the code has a problem with RLS, the best way to make sure it gets fixed is to have a test that fails when RLS is on. If the test lies, it will have the opposite effect and be more likely to hide the issue.
On 2016/11/10 18:37:28, skobes wrote: > On 2016/11/10 17:58:04, ymalik wrote: > > On 2016/11/10 15:16:57, bokan wrote: > > > I asked for this. The test should eventually work with RLS but doesn't yet. > > > Checking for behavior with RLS will make sure we remember to update this > when we > > > fix RLS to work here as well (since the test will "break" if RLS is fixed). > > > > I agree with this. It will increase the chances that this gets fixed when RLS > is > > enabled by default. @skobes, WDYT? > > Sorry, I don't understand what you mean by "increase the chance this gets > fixed". > > We use failing tests as an indicator of what work remains for RLS. If the code > has a problem with RLS, the best way to make sure it gets fixed is to have a > test that fails when RLS is on. If the test lies, it will have the opposite > effect and be more likely to hide the issue. That's a good point. But if the test isn't parameterized then it doesn't fail either. Can we somehow mark it DISABLED but only for the RLS case?
https://codereview.chromium.org/2454913003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2454913003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/FrameView.cpp:2463: // Tickmarks for main frame are stored in RFV. On 2016/11/10 17:58:04, ymalik wrote: > In this CL, m_scroller can be RFV. Calling layoutViewport().getTickmarks() from > RFV didn't work for root layer scrolling (and causes test failures). Why can't RFV call getTickmarks() on the FV?
On 2016/11/10 18:41:10, bokan wrote: > On 2016/11/10 18:37:28, skobes wrote: > > On 2016/11/10 17:58:04, ymalik wrote: > > > On 2016/11/10 15:16:57, bokan wrote: > > > > I asked for this. The test should eventually work with RLS but doesn't > yet. > > > > Checking for behavior with RLS will make sure we remember to update this > > when we > > > > fix RLS to work here as well (since the test will "break" if RLS is > fixed). > > > > > > I agree with this. It will increase the chances that this gets fixed when > RLS > > is > > > enabled by default. @skobes, WDYT? > > > > Sorry, I don't understand what you mean by "increase the chance this gets > > fixed". > > > > We use failing tests as an indicator of what work remains for RLS. If the > code > > has a problem with RLS, the best way to make sure it gets fixed is to have a > > test that fails when RLS is on. If the test lies, it will have the opposite > > effect and be more likely to hide the issue. > > That's a good point. But if the test isn't parameterized then it doesn't fail > either. Can we somehow mark it DISABLED but only for the RLS case? NVM, if the test's not parameterized, turning RLS on by default causes it to fail. Disregard me :)
On 2016/11/10 18:41:10, bokan wrote: > That's a good point. But if the test isn't parameterized then it doesn't fail > either. Can we somehow mark it DISABLED but only for the RLS case? If the test isn't parameterized it will fail when RLS is made the default.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2016/11/10 18:42:40, skobes wrote: > On 2016/11/10 18:41:10, bokan wrote: > > That's a good point. But if the test isn't parameterized then it doesn't fail > > either. Can we somehow mark it DISABLED but only for the RLS case? > > If the test isn't parameterized it will fail when RLS is made the default. Yeah. Sorry I wasn't thinking.
@skobes, PTAL https://codereview.chromium.org/2454913003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2454913003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/FrameView.cpp:2463: // Tickmarks for main frame are stored in RFV. On 2016/11/10 18:41:28, skobes wrote: > On 2016/11/10 17:58:04, ymalik wrote: > > In this CL, m_scroller can be RFV. Calling layoutViewport().getTickmarks() > from > > RFV didn't work for root layer scrolling (and causes test failures). > > Why can't RFV call getTickmarks() on the FV? I believe RFV doesn't have access to FV. It relays all the calls through layoutViewport (so doesn't work for root layer scrolling). We can pass in FV, but I got rid of this bit and it looks cleaner now.
lgtm % nit https://codereview.chromium.org/2454913003/diff/290001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): https://codereview.chromium.org/2454913003/diff/290001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1769: if (LocalFrame* frame = box().frame()) I think you also need to check isRootLayer() here?
ymalik@chromium.org changed reviewers: + jbroman@chromium.org
+jbroman for Source/platform
https://codereview.chromium.org/2454913003/diff/290001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): https://codereview.chromium.org/2454913003/diff/290001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1769: if (LocalFrame* frame = box().frame()) On 2016/11/10 21:10:47, skobes wrote: > I think you also need to check isRootLayer() here? Yes. Done.
The CQ bit was checked by ymalik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
rs lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
ymalik@chromium.org changed reviewers: + sky@chromium.org
+sky for chrome/renderer
The CQ bit was checked by ymalik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by bokan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bokan@chromium.org, skobes@chromium.org, jbroman@chromium.org Link to the patchset: https://codereview.chromium.org/2454913003/#ps350001 (title: "Fix broken chromeos bot")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== MainFrame scrollbars should work with RFV instead of FV This CL does the following things: - FV::updateScrollbarGeometry and part of FV::computeScrollbarExistence is moved into scrollbar manager. - For MainFrame, the scroller that the scrollbars works with is changed to be RFV from FV - To accommodate for this, a bunch of methods in Scrollbar that used to call into FV now call into RFV, which in turn calls the corresponding methods on its layout viewport. An exception to this is Tickmarks. Tickmarks for MainFrame are now stored in RFV. - The isOverlayScrollbar method was needlessly implemented in FV. This CL removes this. BUG=456861 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== MainFrame scrollbars should work with RFV instead of FV This CL does the following things: - FV::updateScrollbarGeometry and part of FV::computeScrollbarExistence is moved into scrollbar manager. - For MainFrame, the scroller that the scrollbars works with is changed to be RFV from FV - To accommodate for this, a bunch of methods in Scrollbar that used to call into FV now call into RFV, which in turn calls the corresponding methods on its layout viewport. An exception to this is Tickmarks. Tickmarks for MainFrame are now stored in RFV. - The isOverlayScrollbar method was needlessly implemented in FV. This CL removes this. BUG=456861 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Message was sent while issue was closed.
Committed patchset #19 (id:350001)
Message was sent while issue was closed.
Description was changed from ========== MainFrame scrollbars should work with RFV instead of FV This CL does the following things: - FV::updateScrollbarGeometry and part of FV::computeScrollbarExistence is moved into scrollbar manager. - For MainFrame, the scroller that the scrollbars works with is changed to be RFV from FV - To accommodate for this, a bunch of methods in Scrollbar that used to call into FV now call into RFV, which in turn calls the corresponding methods on its layout viewport. An exception to this is Tickmarks. Tickmarks for MainFrame are now stored in RFV. - The isOverlayScrollbar method was needlessly implemented in FV. This CL removes this. BUG=456861 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== MainFrame scrollbars should work with RFV instead of FV This CL does the following things: - FV::updateScrollbarGeometry and part of FV::computeScrollbarExistence is moved into scrollbar manager. - For MainFrame, the scroller that the scrollbars works with is changed to be RFV from FV - To accommodate for this, a bunch of methods in Scrollbar that used to call into FV now call into RFV, which in turn calls the corresponding methods on its layout viewport. An exception to this is Tickmarks. Tickmarks for MainFrame are now stored in RFV. - The isOverlayScrollbar method was needlessly implemented in FV. This CL removes this. BUG=456861 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Committed: https://crrev.com/322db3d6f1f9eb4b9cd6a0054999718c24b1f076 Cr-Commit-Position: refs/heads/master@{#431683} ==========
Message was sent while issue was closed.
Patchset 19 (id:??) landed as https://crrev.com/322db3d6f1f9eb4b9cd6a0054999718c24b1f076 Cr-Commit-Position: refs/heads/master@{#431683}
Message was sent while issue was closed.
On 2016/11/11 23:54:34, commit-bot: I haz the power wrote: > Patchset 19 (id:??) landed as > https://crrev.com/322db3d6f1f9eb4b9cd6a0054999718c24b1f076 > Cr-Commit-Position: refs/heads/master@{#431683} This CL was reverted in https://codereview.chromium.org/2501493002/ because it resulted in a mac regression. The main problem is that ScrollAnimatorMac does both painting and and scroll animation, and now that the RFV is the scrollbar's scroller, we run into the situation where the painting and animation logic work with different instances of ScrollAnimationMac. For example, when we add a scrollbar, FrameView::ScrollbarManager calls m_scrollableArea->didAddScrollbar, which in turn calls ScrollAnimatorMac::didAddScrollbar and initializes some paint specific variables in RFV's instance of ScrollAnimatorMac. Then we have some painting code in RFV's version that depends on variables being set on the ScrollAnimatorMac, but RFV delegates its scroll animation to FV's ScrollAnimatorMac and not RFV's ScrollAnimatorMac. I spoke to @bokan offline and it seems like the bug that this CL fixes is not really a launch blocker for MD scrollbars. It has been around for a while before MD scrollbars, and its only marginally worse with MD scrollbars. Its also only a problem for cases which don't have scroll extent to begin with. This will be fixed by the suggested refactoring proposed in crbug.com/661236. Its probably not worthwhile to add more hacks to fix this for Mac, so lets keep this in CL reverted. |
