Bug 111124 - RelevantRepaintedObjects heuristic should ensure there is some coverage in the bottom half of the relevant view rect
Summary: RelevantRepaintedObjects heuristic should ensure there is some coverage in th...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Beth Dakin
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2013-02-28 15:50 PST by Beth Dakin
Modified: 2013-02-28 16:44 PST (History)
4 users (show)

See Also:


Attachments
Patch (4.50 KB, patch)
2013-02-28 15:56 PST, Beth Dakin
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Beth Dakin 2013-02-28 15:50:16 PST
The RelevantRepaintedObjects heuristic should ensure there is some coverage in the bottom half of the relevant view rect. Making sure we have coverage in both halfs helps to prevent cases where we have a fully loaded menu bar or masthead with no content beneath that.

<rdar://problem/12257164>
Comment 1 Beth Dakin 2013-02-28 15:56:32 PST
Created attachment 190828 [details]
Patch
Comment 2 Tim Horton 2013-02-28 16:00:24 PST
Comment on attachment 190828 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=190828&action=review

> Source/WebCore/page/Page.cpp:1338
> +    // both halfs helps to prevent cases where we have a fully loaded menu bar or masthead with

halves.
Comment 3 Simon Fraser (smfr) 2013-02-28 16:13:09 PST
Comment on attachment 190828 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=190828&action=review

> Source/WebCore/ChangeLog:12
> +        We need two Regions now -- one for the top and another for the bottom. Make sure 
> +        we have at least half of our desired coverage in both.

This seems a bit western-centric. What about languages that might load a strip down one side first?

> Source/WebCore/page/Page.cpp:1348
> +    if (topRelevantRect.intersects(snappedPaintRect))
> +        m_topRelevantPaintedRegion.unite(snappedPaintRect);
> +    else
> +        m_bottomRelevantPaintedRegion.unite(snappedPaintRect);

Do you want to ignore coverage over the bottom rect if the paint rect happens to just touch the top rect?
Comment 4 Beth Dakin 2013-02-28 16:41:10 PST
(In reply to comment #3)
> (From update of attachment 190828 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=190828&action=review
> 
> > Source/WebCore/ChangeLog:12
> > +        We need two Regions now -- one for the top and another for the bottom. Make sure 
> > +        we have at least half of our desired coverage in both.
> 
> This seems a bit western-centric. What about languages that might load a strip down one side first?
> 

Hmm, good point. We should drum up some foreign language test cases and see if they would be improved by looking at left/right.

> > Source/WebCore/page/Page.cpp:1348
> > +    if (topRelevantRect.intersects(snappedPaintRect))
> > +        m_topRelevantPaintedRegion.unite(snappedPaintRect);
> > +    else
> > +        m_bottomRelevantPaintedRegion.unite(snappedPaintRect);
> 
> Do you want to ignore coverage over the bottom rect if the paint rect happens to just touch the top rect?

Hmm, good point. I changed this code to be more precise: 

    // If the rect straddles both Regions, split it appropriately.
    if (topRelevantRect.intersects(snappedPaintRect) && bottomRelevantRect.intersects(snappedPaintRect)) {
        IntRect topIntersection = snappedPaintRect;
        topIntersection.intersect(pixelSnappedIntRect(topRelevantRect));
        m_topRelevantPaintedRegion.unite(topIntersection);

        IntRect bottomIntersection = snappedPaintRect;
        bottomIntersection.intersect(pixelSnappedIntRect(bottomRelevantRect));
        m_bottomRelevantPaintedRegion.unite(bottomIntersection);
    } else if (topRelevantRect.intersects(snappedPaintRect))
        m_topRelevantPaintedRegion.unite(snappedPaintRect);
    else
        m_bottomRelevantPaintedRegion.unite(snappedPaintRect);
Comment 5 Beth Dakin 2013-02-28 16:44:31 PST
http://trac.webkit.org/changeset/144395