Bug 155747 - Use Region instead of IntRect in PageClient and WebPageProxy setViewNeedsDisplay method
Summary: Use Region instead of IntRect in PageClient and WebPageProxy setViewNeedsDisp...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on:
Blocks:
 
Reported: 2016-03-22 06:38 PDT by Carlos Garcia Campos
Modified: 2016-03-29 00:27 PDT (History)
3 users (show)

See Also:


Attachments
Patch (11.21 KB, patch)
2016-03-22 06:42 PDT, Carlos Garcia Campos
darin: review+
Details | Formatted Diff | Diff
Patch for landing (13.57 KB, patch)
2016-03-23 01:23 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2016-03-22 06:38:59 PDT
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.
Comment 1 Carlos Garcia Campos 2016-03-22 06:42:51 PDT
Created attachment 274648 [details]
Patch
Comment 2 Darin Adler 2016-03-22 09:34:56 PDT
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())
Comment 3 Carlos Garcia Campos 2016-03-23 01:23:10 PDT
Created attachment 274740 [details]
Patch for landing
Comment 4 Carlos Garcia Campos 2016-03-23 01:52:12 PDT
Committed r198580: <http://trac.webkit.org/changeset/198580>
Comment 5 Darin Adler 2016-03-23 08:21:54 PDT
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!
Comment 6 Carlos Garcia Campos 2016-03-29 00:27:35 PDT
(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