RESOLVED FIXED 108888
Make ScrollView::paint() clip by visibleContentRect
https://bugs.webkit.org/show_bug.cgi?id=108888
Summary Make ScrollView::paint() clip by visibleContentRect
Alexandre Elias
Reported 2013-02-04 18:59:30 PST
Make ScrollView::paint() clip by visibleContentRect
Attachments
Patch (6.45 KB, patch)
2013-02-04 19:02 PST, Alexandre Elias
no flags
Patch (6.57 KB, patch)
2013-02-04 22:44 PST, Alexandre Elias
no flags
Patch (6.57 KB, patch)
2013-02-04 22:45 PST, Alexandre Elias
no flags
Alexandre Elias
Comment 1 2013-02-04 19:02:55 PST
Build Bot
Comment 2 2013-02-04 19:26:38 PST
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
Rafael Brandao
Comment 3 2013-02-04 19:36:55 PST
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.
Alexandre Elias
Comment 4 2013-02-04 19:43:56 PST
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.
WebKit Review Bot
Comment 5 2013-02-04 21:19:02 PST
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
Alexandre Elias
Comment 6 2013-02-04 22:44:31 PST
Created attachment 186549 [details] Patch Switch origin to location() and add named variables
Alexandre Elias
Comment 7 2013-02-04 22:45:56 PST
Created attachment 186550 [details] Patch Fix scrollbar clip
Alexandre Elias
Comment 8 2013-02-05 11:35:49 PST
Levi, could you take a look?
Levi Weintraub
Comment 9 2013-02-06 15:39:41 PST
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 :-/
WebKit Review Bot
Comment 10 2013-02-06 15:48:53 PST
Comment on attachment 186550 [details] Patch Clearing flags on attachment: 186550 Committed r142045: <http://trac.webkit.org/changeset/142045>
WebKit Review Bot
Comment 11 2013-02-06 15:48:57 PST
All reviewed patches have been landed. Closing bug.
Mikhail Pozdnyakov
Comment 12 2013-02-07 06:36:01 PST
This has broken qt and efl ports https://bugs.webkit.org/show_bug.cgi?id=109185
Alexandre Elias
Comment 13 2013-02-07 11:05:11 PST
Sorry about that. Looks like you landed the correct fix that works for all ports in the other bug, thanks.
Note You need to log in before you can comment on or make changes to this bug.