Bug 92261 – [gtk] Add wk1 method for snapshotting a webview

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Claudio Saavedra]

WK2 already has the infrastructure for this, but WKGTK+ API is missing. I am working on it. Meanwhile, I need the WK1 API for Ephy 3.6, since we will use wk1.

[Claudio Saavedra]

Created attachment 154377 [details]
2012-07-25  Claudio Saavedra  <[e-mail]>

[Gtk] Add WK1 API for snapshot retrieval
        https://bugs.webkit.org/show_bug.cgi?id=92261

        Reviewed by NOBODY (OOPS!).

        Added a method to ChromeClientGtk that paints the webview contents
        into a cairo_surface and add the public API to WebKitWebView for
        this.

        * WebCoreSupport/ChromeClientGtk.cpp:
        (WebKit::ChromeClient::paintWebViewInSurface): Paint
        the ChromeClient in a given cairo_surface.
        * WebCoreSupport/ChromeClientGtk.h:
        (ChromeClient): Ditto.
        * webkit/webkitwebview.cpp:
        (webkit_web_view_get_snapshot): New
        method to paint a webview snapshot.
        * webkit/webkitwebview.h: Ditto.

[Carlos Garcia Campos]

Comment on attachment 154377 [details]
2012-07-25  Claudio Saavedra  <[e-mail]>

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

Thanks for the patch, looks good in general, I have just a few comments.

> Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:531
> +    Frame* frame = core(m_webView)->mainFrame();

I think frame can be null here, check it and return early or add an assert if you are sure it can't be null.

> Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:532
> +    RefPtr<cairo_t> cr = cairo_create(surface);

You should adopt the ref and then you can remove the cairo_destroy. 

RefPtr<cairo_t> cr = adoptRef(cairo_create(surface));

> Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.h:167
> +        void paintWebViewInSurface(cairo_surface_t*, IntRect&);

Why do you need this method in the chrome client? It simply uses the frame, so I guess you can use it directly in the web view.

> Source/WebKit/gtk/webkit/webkitwebview.cpp:5263
> +cairo_surface_t*
> +webkit_web_view_get_snapshot(WebKitWebView *webView)

Please add gtk-doc comments. Remember to add Since: 1.10 tag and add the new symbol to the sections file.

> Source/WebKit/gtk/webkit/webkitwebview.cpp:5267
> +    GtkAllocation allocation;
> +    IntRect rect;
> +    cairo_surface_t* surface;

Declare these variables when you need them

> Source/WebKit/gtk/webkit/webkitwebview.cpp:5274
> +    surface = cairo_image_surface_create(CAIRO_FORMAT_RGB24,
> +                                         allocation.width,
> +                                         allocation.height);

This should be one line.

> Source/WebKit/gtk/webkit/webkitwebview.cpp:5275
> +    rect = IntRect(allocation.x, allocation.y, allocation.width, allocation.height);

I think you can do IntRect rect = allocation; or even directly call chromeClient->paintWebViewInSurface(surface, IntRect(allocation));

[Claudio Saavedra]

Created attachment 154935 [details]
        [Gtk] Add WK1 API for snapshot retrieval         https://bugs.webkit.org/show_bug.cgi?id=92261

Reviewed by NOBODY (OOPS!).

        Add API to WebKitWebView to retrieve a snapshot of its
        visible contents as a cairo_surface_t.

        * webkit/webkitwebview.cpp:
        (webkit_web_view_get_snapshot): New
        method to paint a webview snapshot.
        * webkit/webkitwebview.h: Ditto.

[Gustavo Noronha (kov)]

Comment on attachment 154935 [details]
        [Gtk] Add WK1 API for snapshot retrieval         https://bugs.webkit.org/show_bug.cgi?id=92261

Attachment 154935 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/13373391

[Claudio Saavedra]

Created attachment 154938 [details]
[Gtk] Add WK1 API for snapshot retrieval

https://bugs.webkit.org/show_bug.cgi?id=92261

Reviewed by NOBODY (OOPS!).

        Add API to WebKitWebView to retrieve a snapshot of its
        visible contents as a cairo_surface_t.

        * docs/webkitgtk-sections.txt: Add new symbols.
        * webkit/webkitwebview.cpp:
        (webkit_web_view_get_snapshot): New
        method to paint a webview snapshot.
        * webkit/webkitwebview.h: Ditto.

[Carlos Garcia Campos]

Comment on attachment 154938 [details]
[Gtk] Add WK1 API for snapshot retrieval

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

API looks good to me, I just wonder whether it would be useful to pass the clip rect. In any case we need the approval from another reviewer for this.

> Source/WebKit/gtk/webkit/webkitwebview.cpp:5264
> + * webkit_web_view_get_snapshot:
> + * @web_view: a #WebKitWebView

I wonder if we could pass a rectangle to the method, or even having two methods and this one could call the one taking a rectangle with the view allocation.

> Source/WebKit/gtk/webkit/webkitwebview.cpp:5270
> + * Since: 1.9.5

Use stable version numbers in Since tags, and 1.9.5 was already released :-) Use 1.10 in this case, please.

> Source/WebKit/gtk/webkit/webkitwebview.cpp:5275
> +    GtkAllocation allocation;

Move this right before gtk_widget_get_allocation()

> Source/WebKit/gtk/webkit/webkitwebview.cpp:5277
> +    g_return_val_if_fail(WEBKIT_IS_WEB_VIEW(webView), NULL);

Use 0 instead of NULL.

> Source/WebKit/gtk/webkit/webkitwebview.cpp:5281
> +        return NULL;

Ditto.

[Martin Robinson]

Comment on attachment 154938 [details]
[Gtk] Add WK1 API for snapshot retrieval

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

Looks good to me apart from two minor changes!

> Source/WebKit/gtk/webkit/webkitwebview.cpp:5273
> +webkit_web_view_get_snapshot(WebKitWebView *webView)

The asterisk should be up against the type here.

> Source/WebKit/gtk/webkit/webkitwebview.cpp:5284
> +    cairo_surface_t* surface = cairo_image_surface_create(CAIRO_FORMAT_RGB24, allocation.width, allocation.height);

Changing this to CAIRO_FORMAT_ARGB32 and adding a clearRect (see the call in ChromeClient) below would ensure that transparent backgrounds work.

[Martin Robinson]

Comment on attachment 154938 [details]
[Gtk] Add WK1 API for snapshot retrieval

Oops. Looks like this is missing a frame->view()->updateLayoutAndStyleIfNeededRecursive().

[Claudio Saavedra]

Created attachment 155128 [details]
[Gtk] Add WK1 API for snapshot retrieval

https://bugs.webkit.org/show_bug.cgi?id=92261

Reviewed by NOBODY (OOPS!).

        Add API to WebKitWebView to retrieve a snapshot of its
        visible contents as a cairo_surface_t.

        * docs/webkitgtk-sections.txt: Add new symbols.
        * webkit/webkitwebview.cpp:
        (webkit_web_view_get_snapshot): New
        method to paint a webview snapshot.
        * webkit/webkitwebview.h: Ditto.

[Gustavo Noronha (kov)]

Comment on attachment 155128 [details]
[Gtk] Add WK1 API for snapshot retrieval

Why not use gtk_widget_draw()? Is it because of the need to apply the device scale factor?

[WebKit Review Bot]

Comment on attachment 154938 [details]
[Gtk] Add WK1 API for snapshot retrieval

Cleared Martin Robinson's review+ from obsolete attachment 154938 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Claudio Saavedra]

(In reply to comment #10)
> (From update of attachment 155128 [details])
> Why not use gtk_widget_draw()? Is it because of the need to apply the device scale factor?

Because it doesn't always work (the widget needs to be mapped and visible, and this won't always hold true).

[Claudio Saavedra]

Comment on attachment 155128 [details]
[Gtk] Add WK1 API for snapshot retrieval

Thanks!

[WebKit Review Bot]

Comment on attachment 155128 [details]
[Gtk] Add WK1 API for snapshot retrieval

Rejecting attachment 155128 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[WebKit Review Bot]

Comment on attachment 155128 [details]
[Gtk] Add WK1 API for snapshot retrieval

Clearing flags on attachment: 155128

Committed r124023: <http://trac.webkit.org/changeset/124023>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Gustavo Noronha (kov)]

> Because it doesn't always work (the widget needs to be mapped and visible, and this won't always hold true).

Aha, thanks!