WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(6.57 KB, patch)
2013-02-04 22:44 PST
,
Alexandre Elias
no flags
Details
Formatted Diff
Diff
Patch
(6.57 KB, patch)
2013-02-04 22:45 PST
,
Alexandre Elias
no flags
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Alexandre Elias
Comment 1
2013-02-04 19:02:55 PST
Created
attachment 186524
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug