Bug 98270 – [GTK][WK2] Add API to retrieve a snapshot from a webview

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Claudio Saavedra]

We already have the WK1 api for this, now let's add API to WK2.

[Claudio Saavedra]

Created attachment 166901 [details]
First iteration of the patch

[WebKit Review Bot]

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

[WebKit Review Bot]

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.

[Martin Robinson]

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?

[Carlos Garcia Campos]

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

[Claudio Saavedra]

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.

[Claudio Saavedra]

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.

[WebKit Review Bot]

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.

[Claudio Saavedra]

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.

[WebKit Review Bot]

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.

[Claudio Saavedra]

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.

[Claudio Saavedra]

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.

[WebKit Review Bot]

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.

[kov's GTK+ EWS bot]

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

[Claudio Saavedra]

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.

[Claudio Saavedra]

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.

[WebKit Review Bot]

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.

[kov's GTK+ EWS bot]

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

[Claudio Saavedra]

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.

[Claudio Saavedra]

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.

[WebKit Review Bot]

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.

[Carlos Garcia Campos]

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

[Claudio Saavedra]

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.

[Claudio Saavedra]

Created attachment 183454 [details]
Patch

[Claudio Saavedra]

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.

[WebKit Review Bot]

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.

[Claudio Saavedra]

Created attachment 183518 [details]
Patch

[Claudio Saavedra]

(In reply to comment #27)
> Created an attachment (id=183518) [details]
> Patch

I added a good bunch of extra test cases.

[WebKit Review Bot]

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.

[Gustavo Noronha (kov)]

(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.

[Carlos Garcia Campos]

(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.

[Claudio Saavedra]

Created attachment 191951 [details]
Patch

[WebKit Review Bot]

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.

[Carlos Garcia Campos]

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()) {

[Claudio Saavedra]

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.

[Claudio Saavedra]

Created attachment 191994 [details]
Patch

[WebKit Review Bot]

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.

[Carlos Garcia Campos]

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.

[Claudio Saavedra]

Created attachment 192018 [details]
Patch

[WebKit Review Bot]

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.

[Martin Robinson]

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.

[Claudio Saavedra]

Created attachment 192271 [details]
Patch

[Claudio Saavedra]

Thanks for all the reviews. I have added a new test to make sure that cancelling the asynchronous method works properly.

[Carlos Garcia Campos]

The patch looks good to me and works nicely, thanks! Adding WebKit2 owners to CC.

[Carlos Garcia Campos]

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.

[Claudio Saavedra]

Created attachment 192682 [details]
Patch

[Carlos Garcia Campos]

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.

[Claudio Saavedra]

Created attachment 192698 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 192698 [details]
Patch

Thanks!

[Martin Robinson]

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.

[Build Bot]

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

[Carlos Garcia Campos]

(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.

[Claudio Saavedra]

Comment on attachment 192698 [details]
Patch

That's a false-positive. Flipping again.

[WebKit Commit Bot]

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

[Claudio Saavedra]

Created attachment 198345 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 198345 [details]
Patch for landing

Clearing flags on attachment: 198345

Committed r148526: <http://trac.webkit.org/changeset/148526>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.