Bug 54080

Created attachment 81796 [details] Proposed patch

Some comments from a non reviewer. View in context: https://bugs.webkit.org/attachment.cgi?id=81796&action=review > Source/WebKit2/Shared/gtk/UpdateChunk.cpp:57 > + encoder->encode(false); Can we send an empty m_rect instead of a separate variable to signify that SharedMemory creation failed? > Source/WebKit2/Shared/gtk/UpdateChunk.cpp:104 > + int stride = cairo_format_stride_for_width(CAIRO_FORMAT_ARGB32, m_rect.width()); This assumes that the bit depth is always 32 bit, can we provide an option to use a lower resolution for systems with less memory? > Source/WebKit2/UIProcess/gtk/ChunkedUpdateDrawingAreaProxyGtk.cpp:79 > + cairo_surface_t* pixmap(updateChunk->createImage()); The return pixmap's status has to be checked using cairo_surface_status(pixmap) before using. > Source/WebKit2/UIProcess/gtk/ChunkedUpdateDrawingAreaProxyGtk.cpp:86 > + The cairo_surface_t also has to be destroyed. > Source/WebKit2/WebProcess/WebPage/gtk/ChunkedUpdateDrawingAreaGtk.cpp:45 > + cairo_surface_t* image = updateChunk->createImage(); image has to be destroyed.

(In reply to comment #2) > Some comments from a non reviewer. Thanks very much for the review. > View in context: https://bugs.webkit.org/attachment.cgi?id=81796&action=review > > > Source/WebKit2/Shared/gtk/UpdateChunk.cpp:57 > > + encoder->encode(false); > > Can we send an empty m_rect instead of a separate variable to signify that SharedMemory creation failed? > I guess it is better to do this kind of changes with all the shared memory patch that you are doing? I did not check much the sharedmemory patch you sent, just fixes to make it work. > > Source/WebKit2/Shared/gtk/UpdateChunk.cpp:104 > > + int stride = cairo_format_stride_for_width(CAIRO_FORMAT_ARGB32, m_rect.width()); > > This assumes that the bit depth is always 32 bit, can we provide an option to use a lower resolution for systems with less memory? > Yep, it is something pendind, I was not sure if it makes sense to do it now or keep this patch more simple. I'll check it. > > Source/WebKit2/UIProcess/gtk/ChunkedUpdateDrawingAreaProxyGtk.cpp:79 > > + cairo_surface_t* pixmap(updateChunk->createImage()); > > The return pixmap's status has to be checked using cairo_surface_status(pixmap) before using. > Yep, I'll add this. > > Source/WebKit2/UIProcess/gtk/ChunkedUpdateDrawingAreaProxyGtk.cpp:86 > > + > > The cairo_surface_t also has to be destroyed. > > > Source/WebKit2/WebProcess/WebPage/gtk/ChunkedUpdateDrawingAreaGtk.cpp:45 > > + cairo_surface_t* image = updateChunk->createImage(); > > image has to be destroyed. Both fixed.

Created attachment 81959 [details] Proposed patch

Comment on attachment 81959 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=81959&action=review Looks good to me! I wouldn't mind Amruth taking another look at it too. > Source/WebKit2/Shared/gtk/UpdateChunk.cpp:112 > + return cairo_image_surface_create_for_data(reinterpret_cast<unsigned char*>(m_sharedMemory->data()), This should just be a static cast, I think. > Source/WebKit2/UIProcess/ChunkedUpdateDrawingAreaProxy.h:106 > + // BackingStore Please remove this comment.

The patch looks good to me except for the following usage: View in context: https://bugs.webkit.org/attachment.cgi?id=81959&action=review > Source/WebKit2/UIProcess/gtk/ChunkedUpdateDrawingAreaProxyGtk.cpp:80 > + RefPtr<cairo_surface_t> pixmap(updateChunk->createImage()); I am not sure if this is supported. cairo_destroy_surface has to be explicitly called, I think. > Source/WebKit2/WebProcess/WebPage/gtk/ChunkedUpdateDrawingAreaGtk.cpp:46 > + RefPtr<cairo_surface_t> image = updateChunk->createImage(); Same here. > Source/WebKit2/WebProcess/WebPage/gtk/ChunkedUpdateDrawingAreaGtk.cpp:47 > + RefPtr<cairo_t> cr = cairo_create(image.get()); And here. cairo_destroy has to be called.

(In reply to comment #6) > The patch looks good to me except for the following usage: > View in context: https://bugs.webkit.org/attachment.cgi?id=81959&action=review > > > Source/WebKit2/UIProcess/gtk/ChunkedUpdateDrawingAreaProxyGtk.cpp:80 > > + RefPtr<cairo_surface_t> pixmap(updateChunk->createImage()); > > I am not sure if this is supported. cairo_destroy_surface has to be explicitly called, I think. > > > Source/WebKit2/WebProcess/WebPage/gtk/ChunkedUpdateDrawingAreaGtk.cpp:46 > > + RefPtr<cairo_surface_t> image = updateChunk->createImage(); > > Same here. > > > Source/WebKit2/WebProcess/WebPage/gtk/ChunkedUpdateDrawingAreaGtk.cpp:47 > > + RefPtr<cairo_t> cr = cairo_create(image.get()); > > And here. cairo_destroy has to be called. RefPtrCairo defines some specializations for Cairo + RefPtrs, unless I'm missing something.

(In reply to comment #6) > The patch looks good to me except for the following usage: > View in context: https://bugs.webkit.org/attachment.cgi?id=81959&action=review > > > Source/WebKit2/UIProcess/gtk/ChunkedUpdateDrawingAreaProxyGtk.cpp:80 > > + RefPtr<cairo_surface_t> pixmap(updateChunk->createImage()); > > I am not sure if this is supported. cairo_destroy_surface has to be explicitly called, I think. > > > Source/WebKit2/WebProcess/WebPage/gtk/ChunkedUpdateDrawingAreaGtk.cpp:46 > > + RefPtr<cairo_surface_t> image = updateChunk->createImage(); > > Same here. > > > Source/WebKit2/WebProcess/WebPage/gtk/ChunkedUpdateDrawingAreaGtk.cpp:47 > > + RefPtr<cairo_t> cr = cairo_create(image.get()); > > And here. cairo_destroy has to be called. The destroy cairo functions are called, there is an specialization for those types. Check the RefPtrCairo.

Sorry, i haven't observed the specialization earlier. The patch looks good and thanks for correcting me.

Landed http://trac.webkit.org/changeset/78350