Bug 155747 – Use Region instead of IntRect in PageClient and WebPageProxy setViewNeedsDisplay method

• Component: WebKit2
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

This way instead of calling setViewNeedsDisplay() for every rectangle in the damage area, we can build a region and call setViewNeedsDisplay() once. GTK+ has API to queue a redraw for a given region, so we also avoid scheduling multiple redraws in GTK+ port.

[Carlos Garcia Campos]

Created attachment 274648 [details]
Patch

[Darin Adler]

Comment on attachment 274648 [details]
Patch

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

> Source/WebKit2/UIProcess/API/gtk/PageClientImpl.cpp:73
> +    RefPtr<cairo_region_t> damageRegion = adoptRef(cairo_region_create());
> +    auto rects = region.rects();
> +    for (const auto& rect : rects) {
> +        cairo_rectangle_int_t damageRect = rect;
> +        cairo_region_union_rectangle(damageRegion.get(), &damageRect);
> +    }

This is “make a cairo_region_t out of a WebCore::Region” and it should do that as efficiently as possible. I think that belongs as a separate function, not nested inside PageClientImpl::setViewNeedsDisplay.

> Source/WebKit2/UIProcess/DrawingAreaProxyImpl.cpp:237
> +        damageRegion.unite(IntRect(IntPoint(), m_webPageProxy.viewSize()));

Should use the constructor here instead of unite.

> Source/WebKit2/UIProcess/efl/WebView.cpp:416
> +    auto rects = region.rects();
> +    for (const auto& rect : rects)
> +        m_client.viewNeedsDisplay(this, rect);

No reason for the local variable.

    for (auto& rect : region.rects())

[Carlos Garcia Campos]

Created attachment 274740 [details]
Patch for landing

[Carlos Garcia Campos]

Committed r198580: <http://trac.webkit.org/changeset/198580>

[Darin Adler]

Comment on attachment 274740 [details]
Patch for landing

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

> Source/WebKit2/UIProcess/API/gtk/PageClientImpl.cpp:70
> +    RefPtr<cairo_region_t> damageRegion = toCairoRegion(region);
> +    gtk_widget_queue_draw_region(m_viewWidget, damageRegion.get());

If you ever care to return here you could make this a 1-liner:

    gtk_widget_queue_draw_region(m_viewWidget, toCairoRegion(region).get());

That’s part of the magic of smart pointers!

[Carlos Garcia Campos]

(In reply to comment #5)
> Comment on attachment 274740 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=274740&action=review
> 
> > Source/WebKit2/UIProcess/API/gtk/PageClientImpl.cpp:70
> > +    RefPtr<cairo_region_t> damageRegion = toCairoRegion(region);
> > +    gtk_widget_queue_draw_region(m_viewWidget, damageRegion.get());
> 
> If you ever care to return here you could make this a 1-liner:
> 
>     gtk_widget_queue_draw_region(m_viewWidget, toCairoRegion(region).get());
> 
> That’s part of the magic of smart pointers!

Sure, done in r198775