[Webkit-unassigned] [Bug 92261] [gtk] Add wk1 method for snapshotting a webview

[bugzilla-daemon at webkit.org]

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


Carlos Garcia Campos <cgarcia at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #154377|review?                     |review-
               Flag|                            |




--- Comment #2 from Carlos Garcia Campos <cgarcia at igalia.com>  2012-07-25 10:04:16 PST ---
(From update of attachment 154377)
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));

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