Bug 98270 - [GTK][WK2] Add API to retrieve a snapshot from a webview
: [GTK][WK2] Add API to retrieve a snapshot from a webview
Status: RESOLVED FIXED
: WebKit
WebKit Gtk
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2012-10-03 08:21 PST by
Modified: 2013-04-16 11:28 PST (History)


Attachments
First iteration of the patch (15.47 KB, patch)
2012-10-03 08:31 PST, Claudio Saavedra
mrobinson: review-
Review Patch | Details | Formatted Diff | Diff
[GTK][WK2] Add API to retrieve a snapshot from a webview https://bugs.webkit.org/show_bug.cgi?id=98270 (13.06 KB, patch)
2012-12-09 11:35 PST, Claudio Saavedra
no flags Review Patch | Details | Formatted Diff | Diff
[GTK][WK2] Add API to retrieve a snapshot from a webview https://bugs.webkit.org/show_bug.cgi?id=98270 (13.43 KB, patch)
2012-12-10 04:34 PST, Claudio Saavedra
no flags Review Patch | Details | Formatted Diff | Diff
[GTK][WK2] Add API to retrieve a snapshot from a webview https://bugs.webkit.org/show_bug.cgi?id=98270 (19.06 KB, patch)
2012-12-10 15:21 PST, Claudio Saavedra
gtk-ews: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
[GTK][WK2] Add API to retrieve a snapshot from a webview https://bugs.webkit.org/show_bug.cgi?id=98270 (19.17 KB, patch)
2012-12-11 01:53 PST, Claudio Saavedra
gtk-ews: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
[GTK][WK2] Add API to retrieve a snapshot from a webview https://bugs.webkit.org/show_bug.cgi?id=98270 (21.63 KB, patch)
2012-12-12 10:23 PST, Claudio Saavedra
no flags Review Patch | Details | Formatted Diff | Diff
Patch (24.81 KB, patch)
2013-01-18 07:32 PST, Claudio Saavedra
no flags Review Patch | Details | Formatted Diff | Diff
Patch (28.50 KB, patch)
2013-01-18 11:33 PST, Claudio Saavedra
no flags Review Patch | Details | Formatted Diff | Diff
Patch (28.02 KB, patch)
2013-03-07 01:54 PST, Claudio Saavedra
no flags Review Patch | Details | Formatted Diff | Diff
Patch (28.13 KB, patch)
2013-03-07 07:35 PST, Claudio Saavedra
no flags Review Patch | Details | Formatted Diff | Diff
Patch (28.16 KB, patch)
2013-03-07 09:24 PST, Claudio Saavedra
no flags Review Patch | Details | Formatted Diff | Diff
Patch (28.82 KB, patch)
2013-03-08 12:28 PST, Claudio Saavedra
no flags Review Patch | Details | Formatted Diff | Diff
Patch (29.89 KB, patch)
2013-03-12 02:42 PST, Claudio Saavedra
no flags Review Patch | Details | Formatted Diff | Diff
Patch (30.03 KB, patch)
2013-03-12 03:45 PST, Claudio Saavedra
no flags Review Patch | Details | Formatted Diff | Diff
Patch for landing (30.21 KB, patch)
2013-04-16 10:43 PST, Claudio Saavedra
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-10-03 08:21:20 PST
We already have the WK1 api for this, now let's add API to WK2.
------- Comment #1 From 2012-10-03 08:31:08 PST -------
Created an attachment (id=166901) [details]
First iteration of the patch
------- Comment #2 From 2012-10-03 08:34:41 PST -------
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
------- Comment #3 From 2012-10-03 08:35:06 PST -------
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 #4 From 2012-10-03 16:31:27 PST -------
(From update of attachment 166901 [details])
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 #5 From 2012-10-04 00:07:59 PST -------
(From update of attachment 166901 [details])
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
------- Comment #6 From 2012-12-09 11:35:39 PST -------
Created an attachment (id=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 #7 From 2012-12-09 11:48:45 PST -------
(From update of attachment 178430 [details])
This patch is simpler because some of the required stuff for IPC of images was added already in commit 0ee64d53.
------- Comment #8 From 2012-12-09 12:52:02 PST -------
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.
------- Comment #9 From 2012-12-10 04:34:02 PST -------
Created an attachment (id=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.
------- Comment #10 From 2012-12-10 04:39:03 PST -------
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.
------- Comment #11 From 2012-12-10 15:21:32 PST -------
Created an attachment (id=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 #12 From 2012-12-10 15:23:14 PST -------
(From update of attachment 178647 [details])
This patch:

- Adds a few tests.
- Adds a parameter for the snapshot options.
- Fixes a few corner cases.
------- Comment #13 From 2012-12-10 15:30:51 PST -------
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 #14 From 2012-12-10 17:43:43 PST -------
(From update of attachment 178647 [details])
Attachment 178647 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/15257378
------- Comment #15 From 2012-12-11 01:53:13 PST -------
Created an attachment (id=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 #16 From 2012-12-11 01:54:20 PST -------
(From update of attachment 178750 [details])
This fixes the above error and also simplifies the WebKitSnapshotOptions. Now we only expose WEBKIT_SNAPSHOT_INCLUDE_SELECTION_HIGHLIGHTING.
------- Comment #17 From 2012-12-11 01:56:57 PST -------
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 #18 From 2012-12-11 07:33:56 PST -------
(From update of attachment 178750 [details])
Attachment 178750 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/15257680
------- Comment #19 From 2012-12-12 10:23:00 PST -------
Created an attachment (id=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 #20 From 2012-12-12 10:28:48 PST -------
(From update of attachment 179075 [details])
- Renamed API.
- Added a method to take a snapshot of a specific region.
- More tests.
- Cats.
------- Comment #21 From 2012-12-12 10:32:38 PST -------
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 #22 From 2012-12-21 07:00:06 PST -------
(From update of attachment 179075 [details])
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
------- Comment #23 From 2013-01-18 03:52:28 PST -------
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.
------- Comment #24 From 2013-01-18 07:32:45 PST -------
Created an attachment (id=183454) [details]
Patch
------- Comment #25 From 2013-01-18 07:37:08 PST -------
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.
------- Comment #26 From 2013-01-18 08:02:05 PST -------
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.
------- Comment #27 From 2013-01-18 11:33:30 PST -------
Created an attachment (id=183518) [details]
Patch
------- Comment #28 From 2013-01-18 11:34:19 PST -------
(In reply to comment #27)
> Created an attachment (id=183518) [details] [details]
> Patch

I added a good bunch of extra test cases.
------- Comment #29 From 2013-01-18 12:25:39 PST -------
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.
------- Comment #30 From 2013-01-20 09:06:08 PST -------
(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.
------- Comment #31 From 2013-01-29 02:57:46 PST -------
(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.
------- Comment #32 From 2013-03-07 01:54:49 PST -------
Created an attachment (id=191951) [details]
Patch
------- Comment #33 From 2013-03-07 02:05:55 PST -------
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 #34 From 2013-03-07 05:55:28 PST -------
(From update of attachment 191951 [details])
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 #35 From 2013-03-07 07:27:23 PST -------
(From update of attachment 191951 [details])
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.
------- Comment #36 From 2013-03-07 07:35:36 PST -------
Created an attachment (id=191994) [details]
Patch
------- Comment #37 From 2013-03-07 07:41:58 PST -------
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 #38 From 2013-03-07 08:16:05 PST -------
(From update of attachment 191994 [details])
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.
------- Comment #39 From 2013-03-07 09:24:19 PST -------
Created an attachment (id=192018) [details]
Patch
------- Comment #40 From 2013-03-07 09:26:28 PST -------
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 #41 From 2013-03-08 09:42:03 PST -------
(From update of attachment 192018 [details])
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.
------- Comment #42 From 2013-03-08 12:28:02 PST -------
Created an attachment (id=192271) [details]
Patch
------- Comment #43 From 2013-03-08 12:31:23 PST -------
Thanks for all the reviews. I have added a new test to make sure that cancelling the asynchronous method works properly.
------- Comment #44 From 2013-03-11 04:23:50 PST -------
The patch looks good to me and works nicely, thanks! Adding WebKit2 owners to CC.
------- Comment #45 From 2013-03-12 01:22:12 PST -------
(From update of attachment 192271 [details])
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.
------- Comment #46 From 2013-03-12 02:42:00 PST -------
Created an attachment (id=192682) [details]
Patch
------- Comment #47 From 2013-03-12 02:53:59 PST -------
(From update of attachment 192682 [details])
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.
------- Comment #48 From 2013-03-12 03:45:38 PST -------
Created an attachment (id=192698) [details]
Patch
------- Comment #49 From 2013-03-12 03:49:57 PST -------
(From update of attachment 192698 [details])
Thanks!
------- Comment #50 From 2013-03-12 07:54:53 PST -------
(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.
------- Comment #51 From 2013-03-12 19:22:49 PST -------
(From update of attachment 192698 [details])
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
------- Comment #52 From 2013-03-13 01:13:25 PST -------
(In reply to comment #50)
> (From update of attachment 192271 [details] [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 #53 From 2013-03-13 01:15:21 PST -------
(From update of attachment 192698 [details])
That's a false-positive. Flipping again.
------- Comment #54 From 2013-04-16 09:29:53 PST -------
(From update of attachment 192698 [details])
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
------- Comment #55 From 2013-04-16 10:43:01 PST -------
Created an attachment (id=198345) [details]
Patch for landing
------- Comment #56 From 2013-04-16 11:28:26 PST -------
(From update of attachment 198345 [details])
Clearing flags on attachment: 198345

Committed r148526: <http://trac.webkit.org/changeset/148526>
------- Comment #57 From 2013-04-16 11:28:32 PST -------
All reviewed patches have been landed.  Closing bug.