From crbug.com/128241: Repro Steps: 1. Visit webpage with text. 2. Pinch-zoom until things are large. 3. Pinch-zoom to make things a bit smaller, and hold the pinch. 4. Observe text become suddenly blurry as the contents are scaled wrong. 5. Release the pinch, and the text becomes clear at its current page scale. We should not be changing the contents scale during the pinch, and if we do want to change it, we are changing it incorrectly.
Created attachment 153578 [details] Patch
Comment on attachment 153578 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=153578&action=review > Source/WebKit/chromium/src/WebViewImpl.cpp:3562 > m_layerTreeView.setPageScaleFactorAndLimits(pageScaleFactor(), m_minimumPageScaleFactor, m_maximumPageScaleFactor); you probably want to remove this? And why WebView::max instead of m_max?
Created attachment 153590 [details] Patch
(In reply to comment #2) > (From update of attachment 153578 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=153578&action=review > > > Source/WebKit/chromium/src/WebViewImpl.cpp:3562 > > m_layerTreeView.setPageScaleFactorAndLimits(pageScaleFactor(), m_minimumPageScaleFactor, m_maximumPageScaleFactor); > > you probably want to remove this? > > And why WebView::max instead of m_max? Oops. Yes, I certainly want to move that. Copy pasta error while moving the change. I spoke with Alexandre, and he initially recommended WebView::maxPageScaleFactor. I suppose that m_maximumPageScaleFactor should work as well. Alexandre, can you comment on this?
m_maximumPageScaleFactor should be fine, it will be equal to WebView::maxPageScaleFactor. Actually, it would be better to keep the WebLayerTreeView arguments as is and just have above it: if (!m_webSettings->viewportEnabled()) m_minimumPageScaleFactor = 1; (Nit: WebKit style is to avoid ".0" and just write "1" even for floating point.)
Created attachment 153802 [details] Patch
Comment on attachment 153802 [details] Patch Attachment 153802 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13324269 New failing tests: compositing/rtl/rtl-absolute-overflow.html compositing/rtl/rtl-absolute-overflow-scrolled.html compositing/rtl/rtl-overflow-invalidation.html compositing/rtl/rtl-fixed-overflow.html compositing/overflow/overflow-scaled-descendant-overlapping.html compositing/rtl/rtl-fixed.html compositing/overflow/overflow-scroll.html compositing/rtl/rtl-absolute.html compositing/rtl/rtl-fixed-overflow-scrolled.html
Created attachment 153806 [details] Archive of layout-test-results from gce-cr-linux-08 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-08 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
(In reply to comment #7) > (From update of attachment 153802 [details]) > Attachment 153802 [details] did not pass chromium-ews (chromium-xvfb): > Output: http://queues.webkit.org/results/13324269 > > New failing tests: > compositing/rtl/rtl-absolute-overflow.html > compositing/rtl/rtl-absolute-overflow-scrolled.html > compositing/rtl/rtl-overflow-invalidation.html > compositing/rtl/rtl-fixed-overflow.html > compositing/overflow/overflow-scaled-descendant-overlapping.html > compositing/rtl/rtl-fixed.html > compositing/overflow/overflow-scroll.html > compositing/rtl/rtl-absolute.html > compositing/rtl/rtl-fixed-overflow-scrolled.html Can anyone remark on how the m_minimumPageScaleFactor setting can have an impact on the rtl overflow tests? Are these unrelated failures?
It's certainly possible for the RTL failures to be caused by this, as RTL relies on a special compositor feature (nonzero scrollOrigin passed to m_nonCompositedContentHost->setViewport) and the minimum page scale factor is passed to the compositor. Perhaps the layout tests have accidentally come to rely on <1 page scale factor. Adding enne@ who updated them last.
(In reply to comment #10) > It's certainly possible for the RTL failures to be caused by this, as RTL relies on a special compositor feature (nonzero scrollOrigin passed to m_nonCompositedContentHost->setViewport) and the minimum page scale factor is passed to the compositor. Perhaps the layout tests have accidentally come to rely on <1 page scale factor. Adding enne@ who updated them last. Looking at the tests, the two non-rtl tests look like actual scale changes. The RTL tests only look like scrollbar changes, in that the size of the scrollable region or the scroll offset has changed. The pixels inside the scrollbar are all correct and not incorrectly offset, so I don't think it's directly related to the code in NonCompositedContentHost. It's possible that CCScrollbarLayerImpl is doing the wrong thing with respect to calculating the total size and current position of the scrollbar. You could determine this by just adding a "return WebLayer()" to the top of createScrollbarLayer in ScrollingCoordinatorChromium. If that fixes these rtl tests, then CCScrollbarLayerImpl is busted. If it doesn't, then more investigation is needed into why things have changed.
Er, "pixels inside the scrollable area" not "pixels inside the scrollbar."
Created attachment 153860 [details] Patch
Comment on attachment 153860 [details] Patch Uploading a patch for EWS trials. I looked into the failures previously reported, and am unable to reproduce any of the rtl failures on a local (linux) build. The overflow failures are consistently reproducible, yet they are flaky. I applied the early return to createScrollBarLayer, and still observed the same compositing/overflow failures. The EWS run on this patch will hopefully show the same failures.
Comment on attachment 153860 [details] Patch Attachment 153860 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13328199 New failing tests: platform/chromium/compositing/scrollbars/custom-composited-different-track-parts.html transforms/3d/point-mapping/3d-point-mapping.html compositing/overflow/overflow-scaled-descendant-overlapping.html transforms/3d/point-mapping/3d-point-mapping-preserve-3d.html transforms/3d/point-mapping/3d-point-mapping-origins.html
Created attachment 153873 [details] Archive of layout-test-results from gce-cr-linux-05 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-05 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
(In reply to comment #14) > (From update of attachment 153860 [details]) > Uploading a patch for EWS trials. > > I looked into the failures previously reported, and am unable to reproduce any of the rtl failures on a local (linux) build. > > The overflow failures are consistently reproducible, yet they are flaky. Are they flaky or failing if you use --run-singly? Just curious if there's some state leaking from one test to another. For what it's worth, the flakiness dashboard doesn't have any results for these tests, which means that they haven't failed (even flakily) in a while on the bots.
(In reply to comment #17) > (In reply to comment #14) > > (From update of attachment 153860 [details] [details]) > > Uploading a patch for EWS trials. > > > > I looked into the failures previously reported, and am unable to reproduce any of the rtl failures on a local (linux) build. > > > > The overflow failures are consistently reproducible, yet they are flaky. > > Are they flaky or failing if you use --run-singly? Just curious if there's some state leaking from one test to another. They still fail, but are no longer flaky when individually run: twiz@twizt3500-l:~/mnt/src/src1/src/webkit/tools/layout_tests$ ./run_webkit_tests.py --run-singly compositing compositing/overflow/overflow-scaled-descendant-overlapping.html -> unexpected image mismatch compositing/geometry/fixed-position-transform-composited-page-scale.html -> unexpected image mismatch platform/chromium/compositing/scrollbars/custom-composited-different-track-parts.html -> unexpected image mismatch compositing/geometry/object-clip-rects-assertion.html -> unexpected text diff mismatch Retrying 4 unexpected failure(s) ... compositing/geometry/fixed-position-transform-composited-page-scale.html -> unexpected image mismatch compositing/geometry/object-clip-rects-assertion.html -> unexpected text diff mismatch compositing/overflow/overflow-scaled-descendant-overlapping.html -> unexpected image mismatch platform/chromium/compositing/scrollbars/custom-composited-different-track-parts.html -> unexpected image mismatch 305 tests ran as expected, 4 didn't: Regressions: Unexpected text diff mismatch : (1) compositing/geometry/object-clip-rects-assertion.html = TEXT Regressions: Unexpected image mismatch : (3) compositing/geometry/fixed-position-transform-composited-page-scale.html = IMAGE compositing/overflow/overflow-scaled-descendant-overlapping.html = IMAGE platform/chromium/compositing/scrollbars/custom-composited-different-track-parts.html = IMAGE > > For what it's worth, the flakiness dashboard doesn't have any results for these tests, which means that they haven't failed (even flakily) in a while on the bots. The drive to get pinch-zoom isn't as strong anymore, so I'm not even sure if this patch should be fixed. Does this change still apply, given the longer vision for pinch-zooming and the threaded compositor? WRT the failing tests above, is it worth opening a new bug to track these issues?
IMHO we might as well leave this until during/after the CLs to do pinch-zoom via compositor scale
(In reply to comment #18) > WRT the failing tests above, is it worth opening a new bug to track these issues? If they only fail with this patch, then I don't think so.