[webkit-reviews] review granted: [Bug 32081] repaint events from outside the viewport aren't received : [Attachment 44205] Patch adding a new paintsEntireContents property.

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted  review:
Bug 32081: repaint events from outside the viewport aren't received
https://bugs.webkit.org/show_bug.cgi?id=32081

Attachment 44205: Patch adding a new paintsEntireContents property.
https://bugs.webkit.org/attachment.cgi?id=44205&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog

> +2009-12-02  Rafael Antognolli  <antognolli at profusion.mobi>
> +
> +	   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.