WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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-
Details
Formatted Diff
Diff
[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-
Details
Formatted Diff
Diff
[Gtk] Add WK1 API for snapshot retrieval
(4.25 KB, patch)
2012-07-27 07:13 PDT
,
Claudio Saavedra
no flags
Details
Formatted Diff
Diff
[Gtk] Add WK1 API for snapshot retrieval
(4.37 KB, patch)
2012-07-28 06:41 PDT
,
Claudio Saavedra
no flags
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
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.
Top of Page
Format For Printing
XML
Clone This Bug