Bug 32081 – repaint events from outside the viewport aren't received

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Rafael Antognolli]

Repaint events are clipped with the viewport area to prevent those that are fully outside the screen of triggering some rendering. This optimization prevents one of implementing backing stores that keeps some content of the page in a cache, because this cache should be marked as dirty when a repaint is called. Since the repaints are being clipped, no dirty area is marked.

[Rafael Antognolli]

Created attachment 44171 [details]
Patch adding a new paintEntireContents property.

This new property asks WebCore to request repaints even if they are out
of the screen. This is useful if implementing a backing store which
already has some area of the page rendered, but needs to know that
something has changed and so is now "dirty".

[Simon Fraser (smfr)]

Comment on attachment 44171 [details]
Patch adding a new paintEntireContents property.

Is m_paintEntireContents ever going to change for a given platform? In other words, could you just have a virtual method, bool paintsEntireContents() const, that returns false by default, and you override for platforms that need it?

> bool m_paintEntireContents; // should this be m_hasBacking ?

I like m_paintEntireContents.

[Kenneth Rohde Christiansen]

(In reply to comment #2)
> (From update of attachment 44171 [details])
> Is m_paintEntireContents ever going to change for a given platform? In other
> words, could you just have a virtual method, bool paintsEntireContents() const,
> that returns false by default, and you override for platforms that need it?
> 
> > bool m_paintEntireContents; // should this be m_hasBacking ?
> 
> I like m_paintEntireContents.

For Qt, we have two views, one canvas based and one widget based. It might make sense only implementing tiling for the canvas based one, or at least make it a configuration.

[Rafael Antognolli]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 44171 [details] [details])
> > Is m_paintEntireContents ever going to change for a given platform? In other
> > words, could you just have a virtual method, bool paintsEntireContents() const,
> > that returns false by default, and you override for platforms that need it?
> > 
> > > bool m_paintEntireContents; // should this be m_hasBacking ?
> > 
> > I like m_paintEntireContents.
> 
> For Qt, we have two views, one canvas based and one widget based. It might make
> sense only implementing tiling for the canvas based one, or at least make it a
> configuration.

For EFL we also have two views, and one of them uses a tiled backing store, the other one doesn't. So it would be good to have something configurable. But anyway, we can clip the repaint request on the single backing store later, if necessary...

[Kenneth Rohde Christiansen]

At least you should remove this comment

// should this be m_hasBacking ?

Can you update the patch?

[Rafael Antognolli]

Created attachment 44184 [details]
Patch adding a new paintEntireContents property.

Patch updated!

[Kenneth Rohde Christiansen]

Please remove or replace

+        No new tests. (OOPS!)

with

No change in behavior, so no new tests added.

[Kenneth Rohde Christiansen]

Another thing, shouldn't it be called paintEntireContent and not paintEntireContents - notice the trailing s.

[Simon Fraser (smfr)]

It should be paintsEntireContents. It's descriptive, not imperative.

[Rafael Antognolli]

Created attachment 44205 [details]
Patch adding a new paintsEntireContents property.

ok, updating property to paintsEntireContents and fixed the Changelog message of no tests added.

[Simon Fraser (smfr)]

Comment on attachment 44205 [details]
Patch adding a new paintsEntireContents property.

> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog

> [phone]  Rafael Antognolli  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        repaint events from outside the viewport aren't received
> +        https://bugs.webkit.org/show_bug.cgi?id=32081
> +

This Changelog entry needs to explain the justification for the change in more detail.
What does it mean for a view to paint its entire contents? Why does it need to be settable?

> diff --git a/WebCore/page/FrameView.cpp b/WebCore/page/FrameView.cpp

> +void ScrollView::setPaintsEntireContents(bool paint)
> +{
> +    m_paintsEntireContents = paint;
> +}

Parameter name could be improved: paintsEntireContents would be fine.

> +    if (!m_paintsEntireContents)

I have a slight preference for calling paintsEntireContents() here (it's inline, so no extra overhead),
so this code matches the code in FrameView.

> diff --git a/WebCore/platform/ScrollView.h b/WebCore/platform/ScrollView.h
> index dbbbff1..e269678 100644
> --- a/WebCore/platform/ScrollView.h
> +++ b/WebCore/platform/ScrollView.h
> @@ -92,6 +92,9 @@ public:
>      virtual void setCanHaveScrollbars(bool);
>      bool canHaveScrollbars() const { return horizontalScrollbarMode() != ScrollbarAlwaysOff || verticalScrollbarMode() != ScrollbarAlwaysOff; }
>  
> +    bool paintsEntireContents() const { return m_paintsEntireContents; }
> +    void setPaintsEntireContents(bool flag);

You can omit the parameter name here in the header. I'd like to see a comment here explaining the purpose of the 'paintsEntireContents' behavior.

[Simon Fraser (smfr)]

Comment on attachment 44205 [details]
Patch adding a new paintsEntireContents property.

Patch needs revision before commit, so commit-queue-

[Kenneth Rohde Christiansen]

Landed in r51636