Bug 17269 – Deobfuscate CanvasRenderingContext2D.cpp

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Oliver Hunt]

Our canvas implementation is currently an unholy conglomerate of ifdef's making it difficult to maintain, comprehend or look at.

This bug tracks progress towards removing this horrifying badness

[Oliver Hunt]

Created attachment 19029 [details]
Round 1 patch, add ImageData abstraction

Anyone have any thoughts on this?

[Eric Seidel (no email)]

The patch looks great.  I'm not sure if the free functions should start with a capital letter or not.  The style guidelines are vague on that point.

[Oliver Hunt]

Created attachment 19030 [details]
Minor tidy up as prep for next great leap for ((wo)?man|thing)kind

[Oliver Hunt]

Created attachment 19057 [details]
Setting up a pattern for stroke style is no longer ifdef hell

[Oliver Hunt]

Created attachment 19060 [details]
Final bit of logic to tidy up pattern painting

[Alp Toker]

(In reply to comment #1)
> Created an attachment (id=19029) [edit]
> Round 1 patch, add ImageData abstraction
> 
> Anyone have any thoughts on this? 
> 

platform/graphics/ImageBuffer already does nearly exactly what your newly added ImageData does. I think it'd be better to make ImageBuffer fit for the job and use it in canvas.

We already have 3/4 Image classes. I don't think we need another one.

[Oliver Hunt]

I'll try this again, i've said it plenty of times but for some reason people seem to ignore it:

The purpose of ImageDataRef is to provide a single typename i can use in code where the logic is otherwise identical.  Using a wrapper class requires either implementing otherwise unnecessary amounts of ref counting logic (mostly courtesy of Qt), or an extra heap allocation and indirection (which is what ImageBuffer would do).

Failing to do this would require:
#if PLATFORM(CG)
     CGImageRef
#elif PLATFORM(CAIRO)
    cairo_surface_t
#elif PLATFORM(QT)
    QImage
#else
    void*
#endif
     platformImage = canvas->createPlatformImage();
#if !PLATFORM(QT)
    if (!platformImage)
#else
    if (platformImage.IsNull())
#endif
        return; 

Note that the logic does not change, we just have half a dozen ifdef blocks to handle the type *name*

The only reason there is anything beyond the Retain/Release functions is because of Qt's desire to use smart pointers (effectively what QImage is) but without allowing them to be used as a pointer -- hence the damned ImageIsNull method.

[Darin Adler]

Comment on attachment 19029 [details]
Round 1 patch, add ImageData abstraction

I think this is OK as an incremental step, but the entire thing needs to be behind an abstraction later. I'm not sure this is helpful.

 67     ImageDataRef createPlatformImage() const;

Given the name of this, maybe the object should be PlatformImageRef.

 76 #if !PLATFORM(QT)
9177       m_platformImage(0)
9278     ,
9379 #endif

You could avoid this #if by writing:

    : m_platformImage()

It would initialize to 0 in the non-Qt case and call the default initializer in the Qt case.

I see no reason not to wrap these objects in a class so you don't need all the Qt strangeness and comments about not liking what Qt is doing. I don't think that adding a data type like this is a good as doing that.

r=me, despite my doubts about your approach here.

[Darin Adler]

Comment on attachment 19030 [details]
Minor tidy up as prep for next great leap for ((wo)?man|thing)kind

r=me

[Darin Adler]

Comment on attachment 19057 [details]
Setting up a pattern for stroke style is no longer ifdef hell

I'm not happy with the approach you're taking here. You're creating these lightweight abstractions on top of the platforms, rather than creating some real C++ objects that go along with GraphicsContext. I'd strongly prefer the latter.

These objects that you have to manually call retain and release on are not a good fit for WebCore's platform layer.

+        CGAffineTransformTranslate(CGAffineTransformScale((CGAffineTransform)transform, 1, -1), 0, -rect.size.height);

+    cairo_matrix_t cm = (cairo_matrix_t)m;

Please don't use C-style casts in new code.

r=me, but lets talk about finding a better design pattern for this work

[Darin Adler]

Comment on attachment 19060 [details]
Final bit of logic to tidy up pattern painting

r=me

[Oliver Hunt]

Comment on attachment 19029 [details]
Round 1 patch, add ImageData abstraction

fine, will redo

[Oliver Hunt]

Created attachment 19089 [details]
Different tack, lets fix HTMLCanvasElement first

[Alp Toker]

Comment on attachment 19089 [details]
Different tack, lets fix HTMLCanvasElement first


 cairo_surface_t* HTMLCanvasElement::createPlatformImage() const
 {
-    if (!m_surface)
+    if (!m_data)
         return 0;
 
     // Note that unlike CG, our returned image is not a copy or
     // copy-on-write, but the original. This is fine, since it is only
     // ever used as a source.
 
-    cairo_surface_flush(m_surface);
-    cairo_surface_reference(m_surface);
+    cairo_surface_flush(m_data->surface());
+    cairo_surface_reference(m_data->surface());
     return m_surface;
 }

^ Should be return m_data->surface();

Otherwise looks great.

[Oliver Hunt]

Created attachment 19098 [details]
Cleanup a de-ifdefify HTMLCanvasElement::paint

[Eric Seidel (no email)]

Comment on attachment 19098 [details]
Cleanup a de-ifdefify HTMLCanvasElement::paint

Looks straightforward.  Certainly isn't any worse than current code. :)

[Oliver Hunt]

Created attachment 19150 [details]
De-ifdefify drawing a canvas to a canvas

Comment on attachment 19150 [details]
De-ifdefify drawing a canvas to a canvas

Given that you are adding

+ImageBuffer* HTMLCanvasElement::buffer() const
+{
+    if (!m_createdDrawingContext)
+        createDrawingContext();
+    return m_data.get();
+}

I think you should change HTMLCanvasElement::drawingContext() to a simple "return buffer()->context();".

It seems odd that you null-check context in GraphicsContext::paintBuffer() but not in GraphicsContext::drawImage().

This is rather pointless:

+    CGContextRef platformContext = this->platformContext();

Finally,

+        // Slow path, boo!

We talked about how it can be made more efficient. You can add a FIXME in addition or instead of the above comment.

r=me

[Oliver Hunt]

Landed r30336

[Oliver Hunt]

Created attachment 19166 [details]
A tiny little tidy up

[Eric Seidel (no email)]

Comment on attachment 19166 [details]
A tiny little tidy up

You're fixing the CG stroke compile failure, but otherwise this looks fine.

[Oliver Hunt]

Comment on attachment 19089 [details]
Different tack, lets fix HTMLCanvasElement first

Landed

[Oliver Hunt]

Comment on attachment 19098 [details]
Cleanup a de-ifdefify HTMLCanvasElement::paint

Landed

[Oliver Hunt]

Comment on attachment 19150 [details]
De-ifdefify drawing a canvas to a canvas

Landed

[Oliver Hunt]

Comment on attachment 19166 [details]
A tiny little tidy up

Landed

[Eric Seidel (no email)]

I think this can be closed.  We'll open more bugs when we need more changes.