Bug 108888 – Make ScrollView::paint() clip by visibleContentRect

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alexandre Elias]

Make ScrollView::paint() clip by visibleContentRect

[Alexandre Elias]

Created attachment 186524 [details]
Patch

[Build Bot]

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 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]

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 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]

Created attachment 186549 [details]
Patch

Switch origin to location() and add named variables

[Alexandre Elias]

Created attachment 186550 [details]
Patch

Fix scrollbar clip

[Alexandre Elias]

Levi, could you take a look?

[Levi Weintraub]

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 on attachment 186550 [details]
Patch

Clearing flags on attachment: 186550

Committed r142045: <http://trac.webkit.org/changeset/142045>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Mikhail Pozdnyakov]

This has broken qt and efl ports
https://bugs.webkit.org/show_bug.cgi?id=109185

[Alexandre Elias]

Sorry about that.  Looks like you landed the correct fix that works for all ports in the other bug, thanks.