Make ScrollView::paint() clip by visibleContentRect
Created attachment 186524 [details] Patch
Comment on attachment 186524 [details] Patch Attachment 186524 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/16365611 New failing tests: fast/media/viewport-media-query.html
Comment on attachment 186524 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=186524&action=review I don't understand enough of this code, but is this going to affect the painting on WebPage::drawRect? What if I want to paint tiles that are outside the visible contents rect, can I still do it? My concern here is that on CoordinatedGraphics, there are heuristics to keep around tiles of non-visible areas (but still valid areas if you think in the frame itself) around the viewport so we can do scrolling on the UI while we get more tiles from uncovered areas in the meantime. > Source/WebCore/ChangeLog:9 > + are used, frameRect() and visibleContentRect(true).size() are What is the later on in the end? You should probably try to name it and put into a function so places where this is used will be more readable. > Source/WebCore/platform/ScrollView.cpp:1055 > + documentDirtyRect.intersect(IntRect(IntPoint(), visibleContentRect(false).size())); For example, here, what means this rect you're using? Using an auxiliary function would increase the legibility a lot.
This change is supposed to be a no-op for most platforms. This method was already clipping by the visible area, I just changed it to a slightly different definition of visible area. If you examine what visibleContentRect does, it ends being sized the same as the frameRect() in the typical case (since width() calls frameRect().width()). I'll edit for clarity.
Comment on attachment 186524 [details] Patch Attachment 186524 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16368512 New failing tests: css2.1/20110323/replaced-intrinsic-ratio-001.htm fast/frames/calculate-order.html editing/execCommand/paste-2.html editing/execCommand/find-after-replace.html editing/selection/iframe.html compositing/iframes/iframe-in-composited-layer.html editing/selection/drag-to-contenteditable-iframe.html editing/selection/4776665.html fast/dom/Window/open-existing-pop-up-blocking.html editing/selection/select-all-iframe.html fast/dom/HTMLDocument/frameless-location-bugzilla10837.html editing/pasteboard/drag-image-to-contenteditable-in-iframe.html fast/frames/calculate-percentage.html fast/frames/calculate-fixed.html http/tests/misc/frame-access-during-load.html editing/execCommand/paste-1.html editing/pasteboard/subframe-dragndrop-1.html fast/block/basic/013.html editing/selection/4975120.html compositing/iframes/iframe-copy-on-scroll.html fast/frames/calculate-round.html fast/frames/calculate-relative.html fast/block/positioning/window-height-change.html http/tests/navigation/javascriptlink-frames.html compositing/iframes/scroll-grandchild-iframe.html http/tests/misc/favicon-as-image.html http/tests/navigation/error404-subframeload.html fast/events/standalone-image-drag-to-editable.html
Created attachment 186549 [details] Patch Switch origin to location() and add named variables
Created attachment 186550 [details] Patch Fix scrollbar clip
Levi, could you take a look?
Comment on attachment 186550 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=186550&action=review > Source/WebCore/platform/ScrollView.cpp:1056 > + IntRect visibleAreaWithoutScrollbars(location(), visibleContentRect(false).size()); > + documentDirtyRect.intersect(visibleAreaWithoutScrollbars); Way cleaner, though I wish visibleContentRect took a named paramater instead of a boolean so this was clearer :-/
Comment on attachment 186550 [details] Patch Clearing flags on attachment: 186550 Committed r142045: <http://trac.webkit.org/changeset/142045>
All reviewed patches have been landed. Closing bug.
This has broken qt and efl ports https://bugs.webkit.org/show_bug.cgi?id=109185
Sorry about that. Looks like you landed the correct fix that works for all ports in the other bug, thanks.