Bug 79705 – didNewFirstVisuallyNonEmptyLayout should be enhanced to look at size instead of a raw tally

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit API

[Beth Dakin]

didNewFirstVisuallyNonEmptyLayout currently works just by tallying relevant replanted objects until they reach a certain threshold. Instead, we should look at the size of the repainted objects, and we should also consider objects that are relevant but unpainted (such as images that are still loading)

<rdar://problem/10821314>

[Beth Dakin]

Created attachment 129106 [details]
Patch

[WebKit Review Bot]

Comment on attachment 129106 [details]
Patch

Attachment 129106 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11643251

New failing tests:
fast/gradients/generated-gradients.html
fast/canvas/webgl/css-webkit-canvas.html
fast/canvas/canvas-as-image.html
fast/canvas/webgl/css-webkit-canvas-repaint.html
fast/canvas/canvas-as-image-incremental-repaint.html

[Beth Dakin]

Created attachment 129125 [details]
Attempt to fix tests

[WebKit Review Bot]

Comment on attachment 129125 [details]
Attempt to fix tests

Attachment 129125 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11640424

New failing tests:
fast/gradients/generated-gradients.html
fast/canvas/webgl/css-webkit-canvas.html
fast/canvas/canvas-as-image.html
fast/canvas/webgl/css-webkit-canvas-repaint.html
fast/canvas/canvas-as-image-incremental-repaint.html

[Dave Hyatt]

Comment on attachment 129125 [details]
Attempt to fix tests

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

Looks mostly ok. My one high level objection is that I don't like putting stuff in the line box list class.

> Source/WebCore/page/Page.h:427
> +        int m_relevantPaintedArea;
> +        int m_relevantUnpaintedArea;

These can be unsigned right?

> Source/WebCore/rendering/InlineTextBox.cpp:499
> +    root()->rendererLineBoxes()->addRelevantRepaintedInlineBox(this, IntRect(adjustedPaintOffset.x(), adjustedPaintOffset.y(), logicalWidth(), logicalHeight()));

I don't really get why you don't just put the function right in InlineTextBox. Why is it over in the line box list?

> Source/WebCore/rendering/RenderLineBoxList.cpp:276
> +    if (m_relevantPaintedArea) {
> +        if (Frame* frame = renderer->frame()) {
> +            if (Page* page = frame->page()) {
> +                page->addRelevantRepaintedObject(renderer, m_relevantPaintedArea);
> +                // m_relevantPaintedArea can be re-set once it's been reported to the Page.
> +                m_relevantPaintedArea = 0;
> +            }
> +        }
> +    }

Feels weird that this is here...

> Source/WebCore/rendering/RenderLineBoxList.cpp:428
> +void RenderLineBoxList::addRelevantRepaintedInlineBox(InlineBox* box, const IntRect& lineBoxPaintRect)
> +{
> +    RenderObject* renderer = box->renderer();
> +    if (!renderer)
> +        return;
> +
> +    // If the Page is not currently counting repainted objects, then we shouldn't either.
> +    if (Frame* frame = renderer->frame()) {
> +        if (Page* page = frame->page()) {
> +            if (!page->isCountingRelevantRepaintedObjects())
> +                return;
> +        }
> +    }
> +
> +    // InlineBoxes are only relevant if they are being painted within the viewRect().
> +    if (RenderView* view = renderer->view()) {
> +        if (!lineBoxPaintRect.intersects(pixelSnappedIntRect(view->viewRect())))
> +            return;
> +    }
> +
> +    m_relevantPaintedArea += (lineBoxPaintRect.width() * lineBoxPaintRect.height());
> +}
> +

Can't this just be over in InlineTextBox?

> Source/WebCore/rendering/RenderLineBoxList.h:42
> +        , m_relevantPaintedArea(0)

I don't think the line box list is the appropriate place to track this sort of thing.

> Source/WebCore/rendering/RenderRegion.cpp:145
> +    if (!m_flowThread || !isValid()) {
> +        if (Frame* frame = this->frame()) {
> +            if (Page* page = frame->page())
> +                page->addRelevantUnpaintedObject(this, visualOverflowRect());
> +        }
>          return;
> +    }

Don't patch RenderRegion. Its contents will paint and be tracked on their own.

[Beth Dakin]

Working on  a patch to address hyatt's comments now.

[Beth Dakin]

Created attachment 129347 [details]
Patch that uses Regions

[WebKit Review Bot]

Comment on attachment 129347 [details]
Patch that uses Regions

Attachment 129347 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11703024

New failing tests:
fast/gradients/generated-gradients.html
fast/canvas/webgl/css-webkit-canvas.html
fast/canvas/canvas-as-image.html
fast/canvas/webgl/css-webkit-canvas-repaint.html
fast/canvas/canvas-as-image-incremental-repaint.html

[Dave Hyatt]

Comment on attachment 129347 [details]
Patch that uses Regions

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

Let's limit the adding of unpainted and repainted objects to the foreground paint phase. Otherwise this is going to be called too much.

There seems to be an issue with unpainted objects that overlap with other unpainted objects. It looks like when an unpainted object becomes painted, you subtract out the object from the unpainted area. However by doing so you could run into a paint order issue where a previous unpainted object had added itself already. So for example you have unpainted object A and unpainted object B that use the same area. B is ready and so it removes itself from the unpainted region, but if it does so after A painted, then you now mistakenly believe the area consumed by A is ready.

The easy way to fix the above issue is to stop tracking changes to unpainted objects and trying to preserve state across multiple paints. You should simply be clearing the regions on every paint. Just start over every time. Then you're never looking at "mutations." Every paint just computes the regions from scratch all over again, and so your data will always be correct.

> Source/WebCore/page/Page.cpp:1061
> +// FIXME: We should potentially move this to Region.h if we think it would be valuable elsewhere.
> +static int totalAreaForRegion(Region region)
> +{
> +    Vector<IntRect> rects = region.rects();
> +    size_t size = rects.size();
> +    int totalArea = 0;
> +
> +    for (size_t i = 0; i < size; ++i) {
> +        IntRect rect = rects[i];
> +        totalArea += (rect.width() * rect.height());
> +    }
> +
> +    return totalArea;
> +}

Put it on region! :)

[Beth Dakin]

Created attachment 129510 [details]
Patch that addresses some comments

This patch moves the totalArea function into the Region class and also only does any adding/removing of relevant objects during the foreground paint phase. I did not address Hyatt's concern about overlapping objects. We think it will be uncommon for replaced elements to overlap on a page, and we fear that Hyatt's solution of doing a fake paint could have a negative performance impact.

[Dave Hyatt]

Comment on attachment 129510 [details]
Patch that addresses some comments

Make totalArea() return an unsigned.

r=me

[Beth Dakin]

Committed change with http://trac.webkit.org/changeset/109273