Bug 54080 – [GTK] Implement UpdateChunk, ChunkedUpdateDrawingArea/Proxy classes for WebKit2

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Alejandro G. Castro]

We have to add those classes for gtk webkit2 compilation.

[Alejandro G. Castro]

Created attachment 81796 [details]
Proposed patch

[Amruth Raj]

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.

[Alejandro G. Castro]

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

[Alejandro G. Castro]

Created attachment 81959 [details]
Proposed patch

[Martin Robinson]

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.

[Amruth Raj]

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.

[Martin Robinson]

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

[Alejandro G. Castro]

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

[Amruth Raj]

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

[Alejandro G. Castro]

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