[Webkit-unassigned] [Bug 98270] [GTK][WK2] Add API to retrieve a snapshot from a webview

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=98270





--- Comment #41 from Martin Robinson <mrobinson at webkit.org>  2013-03-08 09:42:03 PST ---
(From update of attachment 192018)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.