We already have the WK1 api for this, now let's add API to WK2.
Created attachment 166901 [details] First iteration of the patch
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Attachment 166901 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1 WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h" Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2706: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 1 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 166901 [details] First iteration of the patch View in context: https://bugs.webkit.org/attachment.cgi?id=166901&action=review Looks pretty good to me, but there's a memory leak. > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2630 > +static void didGetSnapshot(WKImageRef snapshot, WKErrorRef, void *context) Nit: The asterisk is in the wrong place here. > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2638 > + data->snapshot = WKImageCreateCairoSurface(snapshot); You probably want to use adoptRef here, since I'm assuming that WKImageCreateCairoSurface returns a new reference. > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2671 > + * Finishes an asynchronous opreation started with webkit_web_view_get_snapshot(). Nit: opreation -> operation > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2673 > + * Returns: a #cairo_surface_t with the retrieved snapshot It makes sense to include a transfer annotation here and mention it specifically in the documentation. Right now you aren't returning a new reference, so if the user doesn't ref the result immediately it will be freed. I imagine this didn't cause a crash for you because of the leak above. > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2678 > + g_return_val_if_fail(WEBKIT_IS_WEB_VIEW(webView), FALSE); > + g_return_val_if_fail(G_IS_ASYNC_RESULT(result), FALSE); Probably better to return 0 instead of FALSE here, just for the sake of correctness. They are the same thing in the end, but it looks a little funky. :) > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2684 > + return FALSE; Ditto. > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2688 > + GetSnapshotAsyncData* data = static_cast<GetSnapshotAsyncData*>(g_simple_async_result_get_op_res_gpointer(simple)); > + > + return data->snapshot.get(); This can just be: static_cast<GetSnapshotAsyncData*>(g_simple_async_result_get_op_res_gpointer(simple))->snapshot.get(); > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1269 > + RefPtr<WebImage> snapshotImage = scaledSnapshotWithOptions(contentRect, 1, SnapshotOptionsShareable | SnapshotOptionsExcludeSelectionHighlighting); It seems it could be useful to just expose scaledSnapshotWithOptions to the IPC layer. Wouldn't that make something like webkit_web_view_get_scaled_snapshot_with_options possible?
Comment on attachment 166901 [details] First iteration of the patch View in context: https://bugs.webkit.org/attachment.cgi?id=166901&action=review Looks great! good work :-) >> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2638 >> + data->snapshot = WKImageCreateCairoSurface(snapshot); > > You probably want to use adoptRef here, since I'm assuming that WKImageCreateCairoSurface returns a new reference. Don't use the C API, use toImpl(wkSnapshot)->bitmap()->createCairoSurface() that returns a PassRefPtr so you don't need to adopt it >> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2673 >> + * Returns: a #cairo_surface_t with the retrieved snapshot > > It makes sense to include a transfer annotation here and mention it specifically in the documentation. Right now you aren't returning a new reference, so if the user doesn't ref the result immediately it will be freed. I imagine this didn't cause a crash for you because of the leak above. We should return a reference and use transfer full > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2675 > +cairo_surface_t* webkit_web_view_get_snapshot_finish(WebKitWebView* webView, GAsyncResult* result, GError **error) GError **error -> GError** error
Created attachment 178430 [details] [GTK][WK2] Add API to retrieve a snapshot from a webview https://bugs.webkit.org/show_bug.cgi?id=98270 Reviewed by NOBODY (OOPS!). This adds the GTK+ API necessary to retrieve a snapshot from a webview asynchronously. * UIProcess/API/gtk/WebKitPrivate.h: Add WKImageCairo.h * UIProcess/API/gtk/WebKitWebView.cpp: (GetSnapshotAsyncData): (didGetSnapshot): (webkit_web_view_get_snapshot): (webkit_web_view_get_snapshot_finish): Add the public API. * UIProcess/API/gtk/WebKitWebView.h: Ditto. * UIProcess/API/gtk/docs/webkit2gtk-sections.txt: Add the new API to the docs. * UIProcess/WebPageProxy.cpp: (WebKit): (WebKit::WebPageProxy::getSnapshot): Add a proxy method to request a snapshot from the web process. * UIProcess/WebPageProxy.h: (WebKit): (WebPageProxy): Ditto. * WebProcess/WebPage/WebPage.cpp: (WebKit::WebPage::createSnapshotOfVisibleContent): Add a method to take a snapshot from the visible content of the web page, this method will send a message back to the UI process with the image handle. (WebKit): * WebProcess/WebPage/WebPage.h: Ditto. (WebPage): * WebProcess/WebPage/WebPage.messages.in: Add the message for CreateSnapshotOfVisibleContent.
Comment on attachment 178430 [details] [GTK][WK2] Add API to retrieve a snapshot from a webview https://bugs.webkit.org/show_bug.cgi?id=98270 This patch is simpler because some of the required stuff for IPC of images was added already in commit 0ee64d53.
Attachment 178430 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1 WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h" Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2832: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 1 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 178516 [details] [GTK][WK2] Add API to retrieve a snapshot from a webview https://bugs.webkit.org/show_bug.cgi?id=98270 Reviewed by NOBODY (OOPS!). This adds the GTK+ API necessary to retrieve a snapshot from a webview asynchronously. * UIProcess/API/gtk/WebKitWebView.cpp: (GetSnapshotAsyncData): (didGetSnapshot): (webkit_web_view_get_snapshot): (webkit_web_view_get_snapshot_finish): Add the public API. * UIProcess/API/gtk/WebKitWebView.h: Ditto. * UIProcess/API/gtk/docs/webkit2gtk-sections.txt: Add the new API to the docs. * UIProcess/WebPageProxy.cpp: (WebKit): (WebKit::WebPageProxy::getSnapshot): Add a proxy method to request a snapshot from the web process. * UIProcess/WebPageProxy.h: (WebKit): (WebPageProxy): Ditto. * WebProcess/WebPage/WebPage.cpp: (WebKit::WebPage::createSnapshotOfVisibleContent): Add a method to take a snapshot from the visible content of the web page, this method will send a message back to the UI process with the image handle. (WebKit): * WebProcess/WebPage/WebPage.h: Ditto. (WebPage): * WebProcess/WebPage/WebPage.messages.in: Add the message for CreateSnapshotOfVisibleContent.
Attachment 178516 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1 WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h" Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2834: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 1 in 9 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 178647 [details] [GTK][WK2] Add API to retrieve a snapshot from a webview https://bugs.webkit.org/show_bug.cgi?id=98270 Reviewed by NOBODY (OOPS!). This adds the GTK+ API necessary to retrieve a snapshot from a webview asynchronously. * UIProcess/API/gtk/WebKitError.cpp: (webkit_snapshot_error_quark): * UIProcess/API/gtk/WebKitError.h: Add snapshotting error handling. * UIProcess/API/gtk/WebKitWebView.cpp: (GetSnapshotAsyncData): (didGetSnapshot): (webkit_web_view_get_snapshot): (webkit_web_view_get_snapshot_finish): Add the public API. * UIProcess/API/gtk/WebKitWebView.h: Ditto. * UIProcess/API/gtk/docs/webkit2gtk-sections.txt: Add the new API to the docs. * UIProcess/API/gtk/tests/TestWebKitWebView.cpp: (testWebViewSnapshot): (beforeAll): Add tests for the snapshotting API. * UIProcess/WebPageProxy.cpp: (WebKit): (WebKit::WebPageProxy::getSnapshot): Add a proxy method to request a snapshot from the web process. * UIProcess/WebPageProxy.h: (WebKit): (WebPageProxy): Ditto. * WebProcess/WebPage/WebPage.cpp: (WebKit::WebPage::createSnapshotOfVisibleContent): Add a method to take a snapshot from the visible content of the web page, this method will send a message back to the UI process with the image handle. (WebKit): * WebProcess/WebPage/WebPage.h: Ditto. (WebPage): * WebProcess/WebPage/WebPage.messages.in: Add the message for CreateSnapshotOfVisibleContent.
Comment on attachment 178647 [details] [GTK][WK2] Add API to retrieve a snapshot from a webview https://bugs.webkit.org/show_bug.cgi?id=98270 This patch: - Adds a few tests. - Adds a parameter for the snapshot options. - Fixes a few corner cases.
Attachment 178647 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1 WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h" Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2837: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitError.h" Total errors found: 1 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 178647 [details] [GTK][WK2] Add API to retrieve a snapshot from a webview https://bugs.webkit.org/show_bug.cgi?id=98270 Attachment 178647 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/15257378
Created attachment 178750 [details] [GTK][WK2] Add API to retrieve a snapshot from a webview https://bugs.webkit.org/show_bug.cgi?id=98270 Reviewed by NOBODY (OOPS!). This adds the GTK+ API necessary to retrieve a snapshot from a webview asynchronously. * UIProcess/API/gtk/WebKitError.cpp: (webkit_snapshot_error_quark): * UIProcess/API/gtk/WebKitError.h: Add snapshotting error handling. * UIProcess/API/gtk/WebKitWebView.cpp: (GetSnapshotAsyncData): (didGetSnapshot): (webkit_web_view_get_snapshot): (webkit_web_view_get_snapshot_finish): Add the public API. * UIProcess/API/gtk/WebKitWebView.h: Ditto. * UIProcess/API/gtk/docs/webkit2gtk-sections.txt: Add the new API to the docs. * UIProcess/API/gtk/tests/TestWebKitWebView.cpp: (testWebViewSnapshot): (beforeAll): Add tests for the snapshotting API. * UIProcess/WebPageProxy.cpp: (WebKit): (WebKit::WebPageProxy::getSnapshot): Add a proxy method to request a snapshot from the web process. * UIProcess/WebPageProxy.h: (WebKit): (WebPageProxy): Ditto. * WebProcess/WebPage/WebPage.cpp: (WebKit::WebPage::createSnapshotOfVisibleContent): Add a method to take a snapshot from the visible content of the web page, this method will send a message back to the UI process with the image handle. (WebKit): * WebProcess/WebPage/WebPage.h: Ditto. (WebPage): * WebProcess/WebPage/WebPage.messages.in: Add the message for CreateSnapshotOfVisibleContent.
Comment on attachment 178750 [details] [GTK][WK2] Add API to retrieve a snapshot from a webview https://bugs.webkit.org/show_bug.cgi?id=98270 This fixes the above error and also simplifies the WebKitSnapshotOptions. Now we only expose WEBKIT_SNAPSHOT_INCLUDE_SELECTION_HIGHLIGHTING.
Attachment 178750 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1 WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h" Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2837: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitError.h" Total errors found: 1 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 178750 [details] [GTK][WK2] Add API to retrieve a snapshot from a webview https://bugs.webkit.org/show_bug.cgi?id=98270 Attachment 178750 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/15257680
Created attachment 179075 [details] [GTK][WK2] Add API to retrieve a snapshot from a webview https://bugs.webkit.org/show_bug.cgi?id=98270 Reviewed by NOBODY (OOPS!). This adds the GTK+ API necessary to retrieve a snapshot from a webview asynchronously. * UIProcess/API/gtk/WebKitError.cpp: (webkit_snapshot_error_quark): * UIProcess/API/gtk/WebKitError.h: Add snapshotting error handling. * UIProcess/API/gtk/WebKitWebView.cpp: (GetSnapshotAsyncData): (didGetSnapshot): (webkit_web_view_get_snapshot_of_visible_area): (webkit_web_view_get_snapshot): (webkit_web_view_get_snapshot_finish): Add the public API. * UIProcess/API/gtk/WebKitWebView.h: Ditto. * UIProcess/API/gtk/docs/webkit2gtk-sections.txt: Add the new API to the docs. * UIProcess/API/gtk/tests/TestWebKitWebView.cpp: (testWebViewSnapshot): (beforeAll): Add tests for the snapshotting API. * UIProcess/WebPageProxy.cpp: (WebKit): (WebKit::WebPageProxy::getSnapshot): Add a proxy method to request a snapshot from the web process. * UIProcess/WebPageProxy.h: (WebKit): (WebPageProxy): Ditto. * WebProcess/WebPage/WebPage.cpp: (WebKit::WebPage::createSnapshot): Add a method to take a snapshot for a given region in the WebPage, this method will send a message back to the UI process with the image handle. (WebKit): * WebProcess/WebPage/WebPage.h: Ditto. (WebPage): * WebProcess/WebPage/WebPage.messages.in: Add the message for CreateSnapshot.
Comment on attachment 179075 [details] [GTK][WK2] Add API to retrieve a snapshot from a webview https://bugs.webkit.org/show_bug.cgi?id=98270 - Renamed API. - Added a method to take a snapshot of a specific region. - More tests. - Cats.
Attachment 179075 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1 WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h" Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2905: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitError.h" Total errors found: 1 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 179075 [details] [GTK][WK2] Add API to retrieve a snapshot from a webview https://bugs.webkit.org/show_bug.cgi?id=98270 View in context: https://bugs.webkit.org/attachment.cgi?id=179075&action=review Sorry for the delay reviewing it :-( Looks mostly good, there are just a few minor issues and lack of more tests. > Source/WebKit2/UIProcess/API/gtk/WebKitError.h:141 > + WEBKIT_SNAPSHOT_ERROR_FAILED = 799 FAILED_TO_CREATE maybe? > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2879 > + **/ Use single * here. > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2883 > + webkit_web_view_get_snapshot(webView, rect, options, cancellable, callback, userData); Move the common code to a helper funtion and call it from here, but create a different async result, every async methods should have its corresponding _finish method. Add webkit_web_view_get_snapshot_of_visible_area_finish() > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2889 > + * @rect: a #GdkRectangle with the region to take the snapshot of I guess this is in document coordinates? > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2896 > + * Asynchronously retrieves a snapshot of the @web_view at @rect. If > + * @rect is empty, this function works exactly like Maybe the empty special case could return a snapshot of the whole page, since we already have a convenient variant to get the visible area, this would make easier to get the whole page without having to query its size first. > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2902 > +void webkit_web_view_get_snapshot(WebKitWebView* webView, GdkRectangle rect, WebKitSnapshotOptions options, GCancellable* cancellable, GAsyncReadyCallback callback, gpointer userData) The rectangle should be a pointer. > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2925 > + * Returns: (transfer full) : a #cairo_surface_t with the retrieved snapshot Extra space between ) and : > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2926 > + **/ Since * here too. > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:154 > + * @WEBKIT_SNAPSHOT_OPTIONS_INCLUDE_SELECTION_HIGHLIGHTING: Whether to include in the > + * snapshot the highlight of the selected content. Are you sure this is the selection and not the search results? I've tried the patch and I get the selection rendered in both cases. > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:1131 > + cairo_surface_destroy(surface); We need more tests, for example, we should tests the snapshot options, and check that the snapshot is different when there's something selected and the include selection option is used. There's code to compare pixbufs in search unit tests, comparing cairo surfaces should be easy too. > Source/WebKit2/UIProcess/WebPageProxy.h:764 > + void getSnapshot(PassRefPtr<ImageCallback>, WebCore::IntRect, SnapshotOptions); You should probably pass a const reference for the IntRect
I'm back working on this these days. > > > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2889 > > + * @rect: a #GdkRectangle with the region to take the snapshot of > > I guess this is in document coordinates? Yes, I'll add that to the docs. > > > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2896 > > + * Asynchronously retrieves a snapshot of the @web_view at @rect. If > > + * @rect is empty, this function works exactly like > > Maybe the empty special case could return a snapshot of the whole page, since we already have a convenient variant to get the visible area, this would make easier to get the whole page without having to query its size first. > Yes, that's doable. However I am starting to think that this way the API is a bit confusing. I'll add a patch here anyway followign what you suggest and we can discuss whether we can make it a bit nicer. > > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:154 > > + * @WEBKIT_SNAPSHOT_OPTIONS_INCLUDE_SELECTION_HIGHLIGHTING: Whether to include in the > > + * snapshot the highlight of the selected content. > > Are you sure this is the selection and not the search results? I've tried the patch and I get the selection rendered in both cases. Yes, I am sure. Actually it was due to a bug that I spotted now, but I tested this and it is indeed the selection. > > > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:1131 > > + cairo_surface_destroy(surface); > > We need more tests, for example, we should tests the snapshot options, and check that the snapshot is different when there's something selected and the include selection option is used. There's code to compare pixbufs in search unit tests, comparing cairo surfaces should be easy too. I'll check this after I post a new patch so that we can make a bit of progress in the review and API discussion while I add further tests.
Created attachment 183454 [details] Patch
So about the API. Right now we allow for the three following cases: 1. Make a snapshot of a particular region of the document. To do this, call webkit_webview_get_snapshot() with a non-NULL rectangle. 2. Make a snapshot of the whole document. To do this, call webkit_webview_get_snapshot() with a NULL rectangle. 3. Make a snapshot of the visible are of the document. To do this, call webkit_webview_get_snapshot_of_visible_area(). I have a feeling that this is very confusing. I feel inclined to have either only one method that can deal with the whole array of possibilities or to have one different method for each of the three cases above. But the special-casing of NULL for "full document" is rather weird to me.
Attachment 183454 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1 WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h" Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2999: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:3046: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitError.h" Total errors found: 2 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 183518 [details] Patch
(In reply to comment #27) > Created an attachment (id=183518) [details] > Patch I added a good bunch of extra test cases.
Attachment 183518 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1 WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h" Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2999: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:3046: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:1150: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitError.h" Total errors found: 3 in 15 files If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #25) > I have a feeling that this is very confusing. I feel inclined to have either only one method that can deal with the whole array of possibilities or to have one different method for each of the three cases above. But the special-casing of NULL for "full document" is rather weird to me. Your idea of having this as a single method sounds appealing. Perhaps use the SnapshotOptions enum to allow specifying which of these cases is wanted? You'd still need the rectangle argument, but it would only be taken into consideration if you request it through the options.
(In reply to comment #30) > (In reply to comment #25) > > I have a feeling that this is very confusing. I feel inclined to have either only one method that can deal with the whole array of possibilities or to have one different method for each of the three cases above. But the special-casing of NULL for "full document" is rather weird to me. > > Your idea of having this as a single method sounds appealing. Perhaps use the SnapshotOptions enum to allow specifying which of these cases is wanted? You'd still need the rectangle argument, but it would only be taken into consideration if you request it through the options. Or something in the middle, two methods, one receiving options and the other a rectangle: webkit_web_view_get_snapshot(view, options); webkit_web_view_get_snapshot_of_area(view, area); and options is an enum with document, visible_area options. I think having three different methods is the less confusing option, in any case, the advantage of having an enum is that it can be extended, but I would avoid having a parameter that is used depending on another.
Created attachment 191951 [details] Patch
Attachment 191951 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/Shared/ImageOptions.h', u'Source/WebKit2/UIProcess/API/gtk/WebKitError.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitError.h', u'Source/WebKit2/UIProcess/API/gtk/WebKitInjectedBundleClient.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h', u'Source/WebKit2/UIProcess/API/gtk/WebKitWebViewPrivate.h', u'Source/WebKit2/UIProcess/API/gtk/docs/webkit2gtk-sections.txt', u'Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp', u'Source/WebKit2/UIProcess/API/gtk/tests/WebViewTest.cpp', u'Source/WebKit2/UIProcess/API/gtk/tests/WebViewTest.h', u'Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebExtension.cpp']" exit_code: 1 WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h" Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:3000: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:1130: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitError.h" Total errors found: 2 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 191951 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=191951&action=review This looks pretty good! I have some comments though, see below. > Source/WebKit2/Shared/ImageOptions.h:46 > +enum SnapshotRegion { > + SnapshotRegionVisible = 0, > + SnapshotRegionFullDocument > +}; This is GTK+ specific in cross platform code. I think we can add this to a gtk+ specific private header. > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2936 > +void WebKitWebViewDidReceiveSnapshotOfVisibleArea(WebKitWebView* webView, uint64_t callbackID, WebKit::WebImage* webImage) First W should be lowercase. Also the OfVisibleArea sounds weird, since there are region options. webKitWebViewDidReceiveSnapshot() ? You don't need to use WebKit:: here in the cpp. > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2952 > + g_simple_async_result_complete(result.get()); You should remove the result from the map when the operation is done. You can also get the result from the map with take() instead of get() and then you don't need to remove it. > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2955 > + Nit: extra empty line here. > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2964 > + switch (region) { > + case WEBKIT_SNAPSHOT_REGION_VISIBLE: > + return SnapshotRegionVisible; > + case WEBKIT_SNAPSHOT_REGION_FULL_DOCUMENT: > + return SnapshotRegionFullDocument; > + } > + ASSERT_NOT_REACHED(); You can use COMPILE_ASSERT_MATCHING_ENUM to make sure the enum values are the same at compile time, and use the values directly without a conversion function. > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2993 > + * Remove this extra line. > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:3008 > + message.set(String::fromUTF8("CallbackID"), WebUInt64::create(GPOINTER_TO_UINT(result))); I think it would be easier and more consistent to use uint64_t directly. Define a function to get the callback, something like: uint64_t generateSnapshotCallbackID() { static uint64_t uniqueCallbackID = 1; return uniqueCallbackID++; } > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:3010 > + webView->priv->snapshotResultsMap.set(GPOINTER_TO_UINT(result), result); The result is leaked here, because the map takes a reference. So, you can use a GRefPtr adopting the ref and pass .get() to the map. > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:1129 > + && !g_strcmp0(const_cast<const char*>(reinterpret_cast<char*>(cairo_image_surface_get_data(s1))), I don't think g_strcmp0 is correct in this case, this is not NULL terminated strings, but data. You can probably use memcmp instead. > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:1136 > + cairo_surface_t* surface1; > + cairo_surface_t* surface2; Declare this when first used. > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:1141 > + surface1 = test->waitForSnapshot(WEBKIT_SNAPSHOT_REGION_VISIBLE, WEBKIT_SNAPSHOT_OPTIONS_NONE); Does this returns an error too? It seems that finish() only returns NULL in case of error. Is it considered an error to ask for the snapshot of a visible region when the visible region is empty? > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:1155 > + g_assert(cairo_image_surface_get_width(surface1) <= 50); > + g_assert(cairo_image_surface_get_height(surface1) <= 50); > + int width = cairo_image_surface_get_width(surface1); > + int height = cairo_image_surface_get_height(surface1); Why not moving the assert after getting widget/height? Use g_assert_cmpint to compare integers. > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:1163 > + g_assert(cairo_image_surface_get_width(surface1) == width); > + g_assert(cairo_image_surface_get_height(surface1) == height); Use g_asser_cmpint > Source/WebKit2/UIProcess/API/gtk/tests/WebViewTest.cpp:201 > +void WebViewTest::SelectAll() This should be selectAll() > Source/WebKit2/UIProcess/API/gtk/tests/WebViewTest.h:51 > + void SelectAll(); selectAll() > Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebExtension.cpp:106 > + if (frameView) { frameView is only used inside the if, you can do: if (WebCore::FrameView* frameView = page->mainFrameView()) {
Comment on attachment 191951 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=191951&action=review >> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2936 >> +void WebKitWebViewDidReceiveSnapshotOfVisibleArea(WebKitWebView* webView, uint64_t callbackID, WebKit::WebImage* webImage) > > First W should be lowercase. Also the OfVisibleArea sounds weird, since there are region options. webKitWebViewDidReceiveSnapshot() ? You don't need to use WebKit:: here in the cpp. Name is a left-over from the previous code. >> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:1141 >> + surface1 = test->waitForSnapshot(WEBKIT_SNAPSHOT_REGION_VISIBLE, WEBKIT_SNAPSHOT_OPTIONS_NONE); > > Does this returns an error too? It seems that finish() only returns NULL in case of error. Is it considered an error to ask for the snapshot of a visible region when the visible region is empty? Yes, we consider it an error too.
Created attachment 191994 [details] Patch
Attachment 191994 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/UIProcess/API/gtk/WebKitError.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitError.h', u'Source/WebKit2/UIProcess/API/gtk/WebKitInjectedBundleClient.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitPrivate.h', u'Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h', u'Source/WebKit2/UIProcess/API/gtk/WebKitWebViewPrivate.h', u'Source/WebKit2/UIProcess/API/gtk/docs/webkit2gtk-sections.txt', u'Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp', u'Source/WebKit2/UIProcess/API/gtk/tests/WebViewTest.cpp', u'Source/WebKit2/UIProcess/API/gtk/tests/WebViewTest.h', u'Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebExtension.cpp']" exit_code: 1 WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h" Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2996: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:1126: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:1130: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:1131: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitError.h" Total errors found: 4 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 191994 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=191994&action=review >> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2996 >> + reinterpret_cast<gpointer>(webkit_web_view_get_snapshot)); > > Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] You need to adopt the ref. Also fix the style issues, please > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:3019 > + * Returns: (transfer full): a #cairo_surface_t with the retrieved snapshot or %NULL in case of error > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:1123 > +static gboolean compare_cairo_surfaces(cairo_surface_t* s1, cairo_surface_t* s2) Sorry I didn't notice this before, this should be bool compareCairoSurfaces(). But compare functions usually return 0, <0 or >0. This just returns whether they are equal or not so I would use cairoSurfacesEqual() or something like that.
Created attachment 192018 [details] Patch
Attachment 192018 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/UIProcess/API/gtk/WebKitError.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitError.h', u'Source/WebKit2/UIProcess/API/gtk/WebKitInjectedBundleClient.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitPrivate.h', u'Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h', u'Source/WebKit2/UIProcess/API/gtk/WebKitWebViewPrivate.h', u'Source/WebKit2/UIProcess/API/gtk/docs/webkit2gtk-sections.txt', u'Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp', u'Source/WebKit2/UIProcess/API/gtk/tests/WebViewTest.cpp', u'Source/WebKit2/UIProcess/API/gtk/tests/WebViewTest.h', u'Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebExtension.cpp']" exit_code: 1 WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h" Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2996: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:1126: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:1130: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:1131: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitError.h" Total errors found: 4 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 192018 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=192018&action=review Looks sane to me. > Source/WebKit2/UIProcess/API/gtk/WebKitInjectedBundleClient.cpp:96 > + WebUInt64* callbackID = static_cast<WebUInt64*>(message.get(String::fromUTF8("CallbackID"))); > + WebImage* image = static_cast<WebImage*>(message.get(String::fromUTF8("Snapshot"))); You can just use the implicit String constructor here instead of String::fromUTF8 since the strings themselves are valid Latin1. For example: WebUInt64* callbackID = static_cast<WebUInt64*>(message.get("CallbackID")); if that doesn't work: WebUInt64* callbackID = static_cast<WebUInt64*>(message.get(String("CallbackID"))); > Source/WebKit2/UIProcess/API/gtk/WebKitPrivate.h:124 > + SnapshotRegionVisible = 0, I don't think you need = 0 here. > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2936 > +void WebKitWebViewDidReceiveSnapshot(WebKitWebView* webView, uint64_t callbackID, WebImage* webImage) Shoud be webkitWebViewDidReceiveSnapshot > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2945 > + RefPtr<ShareableBitmap> image = webImage->bitmap(); > + if (image) This can be one line. > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2958 > +static unsigned WebKitSnapshotOptionsToSnapshotOptions(WebKitSnapshotOptions options) Function names should start with a small letter so webkitSnapshotOptionsToSnapshotOptions. > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2969 > +static uint64_t > +generateSnapshotCallbackID(void) This should be one line and you don't need the void argument list. > Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebExtension.cpp:101 > +{ > + if (messageName == String::fromUTF8("GetSnapshot")) { > + SnapshotOptions snapshotOptions = static_cast<SnapshotOptions>(static_cast<WebUInt64*>(message.get(String::fromUTF8("SnapshotOptions")))->value()); > + uint64_t callbackID = static_cast<WebUInt64*>(message.get(String::fromUTF8("CallbackID")))->value(); > + SnapshotRegion region = static_cast<SnapshotRegion>(static_cast<WebUInt64*>(message.get(String::fromUTF8("SnapshotRegion")))->value()); Similar comments here about the strings as above.
Created attachment 192271 [details] Patch
Thanks for all the reviews. I have added a new test to make sure that cancelling the asynchronous method works properly.
The patch looks good to me and works nicely, thanks! Adding WebKit2 owners to CC.
Comment on attachment 192271 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=192271&action=review I've a few more really minor comments that could be improved before landing > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2988 > + **/ Nit: don't use double * here, it's only needed to start the comment block. This doesn't really matter, but it's consistent with all other api docs comments. > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:3018 > + **/ Ditto. > Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebExtension.cpp:23 > +#include "FrameView.h" Use #include <WebCore/Foo.h> to include webcore headers. > Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebExtension.cpp:99 > + SnapshotOptions snapshotOptions = static_cast<SnapshotOptions>(static_cast<WebUInt64*>(message.get(String::fromUTF8("SnapshotOptions")))->value()); You removed all the fromUTF8 except this one String::fromUTF8("SnapshotOptions"). > Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebExtension.cpp:149 > + ASSERT(WKGetTypeID(messageBody) == WKDictionaryGetTypeID()); > + webkitWebExtensionDidReceiveMessageToPage(WEBKIT_WEB_EXTENSION(clientInfo), toImpl(page), toImpl(name)->string(), *toImpl(static_cast<WKDictionaryRef>(messageBody))); Since these are messages to pages, they should be handled by WebKitWebPage. Add webkitWebPageDidReceiveMessage instead of webkitWebExtensionDidReceiveMessageToPage, and move the code there.
Created attachment 192682 [details] Patch
Comment on attachment 192682 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=192682&action=review > Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebExtension.cpp:111 > + webkitWebPageDidReceiveMessage(toImpl(page), toImpl(name)->string(), *toImpl(static_cast<WKDictionaryRef>(messageBody))); This should receive a WebKitWebPage that already has a pointer to the WebPage. Use extension->priv->pages.get(toImpl(page)); we could return early or even assert if this message if for a page we don't know about. I guess early return just in case the message is received after the web page is destroyed.
Created attachment 192698 [details] Patch
Comment on attachment 192698 [details] Patch Thanks!
Comment on attachment 192271 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=192271&action=review >> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2988 >> + **/ > > Nit: don't use double * here, it's only needed to start the comment block. This doesn't really matter, but it's consistent with all other api docs comments. If we're going to enforce this we really need to add a style checker rule.
Comment on attachment 192698 [details] Patch Attachment 192698 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-commit-queue.appspot.com/results/17213007 New failing tests: fast/css/sticky/inline-sticky.html
(In reply to comment #50) > (From update of attachment 192271 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=192271&action=review > > >> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2988 > >> + **/ > > > > Nit: don't use double * here, it's only needed to start the comment block. This doesn't really matter, but it's consistent with all other api docs comments. > > If we're going to enforce this we really need to add a style checker rule. Well, I'm not going to reject any patch because of this, but if I ask someone to change anything else in the patch, I'll suggest to use a single * for consistency. But I don't think this is that important.
Comment on attachment 192698 [details] Patch That's a false-positive. Flipping again.
Comment on attachment 192698 [details] Patch Rejecting attachment 192698 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-01', 'apply-attachment', '--no-update', '--non-interactive', 192698, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: e Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebPage.cpp Hunk #1 FAILED at 20. Hunk #2 succeeded at 199 (offset 72 lines). 1 out of 2 hunks FAILED -- saving rejects to file Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebPage.cpp.rej patching file Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebPagePrivate.h Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', 'Anders Carlsson']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Full output: http://webkit-queues.appspot.com/results/172060
Created attachment 198345 [details] Patch for landing
Comment on attachment 198345 [details] Patch for landing Clearing flags on attachment: 198345 Committed r148526: <http://trac.webkit.org/changeset/148526>
All reviewed patches have been landed. Closing bug.