[webkit-reviews] review denied: [Bug 92261] [gtk] Add wk1 method for snapshotting a webview : [Attachment 154377] 2012-07-25 Claudio Saavedra <csaavedra at igalia.com>

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has denied Claudio Saavedra
<csaavedra at igalia.com>'s request for review:
Bug 92261: [gtk] Add wk1 method for snapshotting a webview
https://bugs.webkit.org/show_bug.cgi?id=92261

Attachment 154377: 2012-07-25  Claudio Saavedra  <csaavedra at igalia.com>
https://bugs.webkit.org/attachment.cgi?id=154377&action=review

------- Additional Comments from Carlos Garcia Campos <cgarcia at igalia.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=154377&action=review


Thanks for the patch, looks good in general, I have just a few comments.

> Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:531
> +    Frame* frame = core(m_webView)->mainFrame();

I think frame can be null here, check it and return early or add an assert if
you are sure it can't be null.

> Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:532
> +    RefPtr<cairo_t> cr = cairo_create(surface);

You should adopt the ref and then you can remove the cairo_destroy. 

RefPtr<cairo_t> cr = adoptRef(cairo_create(surface));

> Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.h:167
> +	   void paintWebViewInSurface(cairo_surface_t*, IntRect&);

Why do you need this method in the chrome client? It simply uses the frame, so
I guess you can use it directly in the web view.

> Source/WebKit/gtk/webkit/webkitwebview.cpp:5263
> +cairo_surface_t*
> +webkit_web_view_get_snapshot(WebKitWebView *webView)

Please add gtk-doc comments. Remember to add Since: 1.10 tag and add the new
symbol to the sections file.

> Source/WebKit/gtk/webkit/webkitwebview.cpp:5267
> +    GtkAllocation allocation;
> +    IntRect rect;
> +    cairo_surface_t* surface;

Declare these variables when you need them

> Source/WebKit/gtk/webkit/webkitwebview.cpp:5274
> +    surface = cairo_image_surface_create(CAIRO_FORMAT_RGB24,
> +					    allocation.width,
> +					    allocation.height);

This should be one line.

> Source/WebKit/gtk/webkit/webkitwebview.cpp:5275
> +    rect = IntRect(allocation.x, allocation.y, allocation.width,
allocation.height);

I think you can do IntRect rect = allocation; or even directly call
chromeClient->paintWebViewInSurface(surface, IntRect(allocation));