RESOLVED FIXED Bug 92261
[gtk] Add wk1 method for snapshotting a webview
https://bugs.webkit.org/show_bug.cgi?id=92261
Summary [gtk] Add wk1 method for snapshotting a webview
Claudio Saavedra
Reported 2012-07-25 09:23:48 PDT
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.
Attachments
2012-07-25 Claudio Saavedra <csaavedra@igalia.com> (5.53 KB, patch)
2012-07-25 09:43 PDT, Claudio Saavedra
cgarcia: review-
[Gtk] Add WK1 API for snapshot retrieval https://bugs.webkit.org/show_bug.cgi?id=92261 (4.15 KB, patch)
2012-07-27 06:57 PDT, Claudio Saavedra
gustavo: commit-queue-
[Gtk] Add WK1 API for snapshot retrieval (4.25 KB, patch)
2012-07-27 07:13 PDT, Claudio Saavedra
no flags
[Gtk] Add WK1 API for snapshot retrieval (4.37 KB, patch)
2012-07-28 06:41 PDT, Claudio Saavedra
no flags
Claudio Saavedra
Comment 1 2012-07-25 09:43:39 PDT
Created attachment 154377 [details] 2012-07-25 Claudio Saavedra <csaavedra@igalia.com> [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 2 2012-07-25 10:04:16 PDT
Comment on attachment 154377 [details] 2012-07-25 Claudio Saavedra <csaavedra@igalia.com> 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
Comment 3 2012-07-27 06:57:12 PDT
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 4 2012-07-27 07:06:25 PDT
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
Comment 5 2012-07-27 07:13:37 PDT
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 6 2012-07-27 07:27:21 PDT
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 7 2012-07-27 08:25:44 PDT
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 8 2012-07-27 08:28:25 PDT
Comment on attachment 154938 [details] [Gtk] Add WK1 API for snapshot retrieval Oops. Looks like this is missing a frame->view()->updateLayoutAndStyleIfNeededRecursive().
Claudio Saavedra
Comment 9 2012-07-28 06:41:15 PDT
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 10 2012-07-28 06:57:50 PDT
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 11 2012-07-28 07:04:12 PDT
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
Comment 12 2012-07-28 07:15:17 PDT
(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 13 2012-07-30 05:48:59 PDT
Comment on attachment 155128 [details] [Gtk] Add WK1 API for snapshot retrieval Thanks!
WebKit Review Bot
Comment 14 2012-07-30 05:49:20 PDT
Comment on attachment 155128 [details] [Gtk] Add WK1 API for snapshot retrieval Rejecting attachment 155128 [details] from commit-queue. csaavedra@igalia.com 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 15 2012-07-30 07:11:48 PDT
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
Comment 16 2012-07-30 07:11:53 PDT
All reviewed patches have been landed. Closing bug.
Gustavo Noronha (kov)
Comment 17 2012-08-23 08:11:49 PDT
> Because it doesn't always work (the widget needs to be mapped and visible, and this won't always hold true). Aha, thanks!
Note You need to log in before you can comment on or make changes to this bug.