Bug 17486 – Support HTML5 Canvas.getImageData API

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Oliver Hunt]

See HTML5 getImageData spec

[Oliver Hunt]

Created attachment 19272 [details]
Woo! getImageData support!

In my defence layout tests and copyrights on new files make this patch look bigger than it is :-O

[Dave Hyatt]

Comment on attachment 19272 [details]
Woo! getImageData support!

The canvas parts look good to me.  I'd do something besides just returning 0 with no FIXME Or notimplemented in the ports impls though.

I'd also leak test and make sure everything gets cleaned up.

Maybe Sam could comment on the CodeGenerator changes, since I don't really understand any of that code.

[Sam Weinig]

As mentioned in IRC, I think the logic in the JSCanvasPixelArray indexGetter and indexSetter should be in the CanvasPixelArray class, not in the JS wrapper.  (Also, JSCanvasPixelArray::indexGetter should follow idiom and do a static_cast to get this without at type check.)

You should start your classes out with a ref-count of 1 by not explicitly calling RefCounted in the constructor.  This also requires changing your create functions to use adoptRef.

[Oliver Hunt]

Created attachment 19276 [details]
Address weinig's commentary 

Hopefully i have sufficiently appeased the weinig

[Sam Weinig]

Comment on attachment 19276 [details]
Address weinig's commentary 

For the indexGetter method, please look at the existing indexGetters for how to implement getting the index from the slot.  I also think you move details into the CanvasPixelArray object so that you don't access the vector directly in the JS wrapper.

+    double pvalue = value->toNumber(exec);
pvalue!  Really!

+CanvasPixelArray::CanvasPixelArray(unsigned size)
+    : RefCounted<CanvasPixelArray>()
No need to explicitly call RefCounted here.

+        void set(unsigned index, double value) {
+            if (!(value > 0))
brace should go on it's own line.  Does this need to be inlined?

+        CanvasPixelArray(unsigned size);
New line after this.

+ImageData::ImageData(unsigned width, unsigned height)
+    : RefCounted<ImageData>()
+    , m_width(width)
+    , m_height(height)
+    , m_data(CanvasPixelArray::create(width * height * 4))
+{
+    
+}
No need to explicitly call RefCounted.  Extra newline in the body.

[Oliver Hunt]

Created attachment 19283 [details]
getImageData Redux

Now we report the extra memory usage to the collectorator and look pretty and stuff

[Sam Weinig]

Comment on attachment 19283 [details]
getImageData Redux

r+ but please 
+    else
+        return jsUndefined();
remove this else and
+    JSCanvasPixelArray* ret = static_cast<JSCanvasPixelArray*>(ScriptInterpreter::getDOMObject(pixels));
remove this cast and
remove the implementation of putImageData.

[Oliver Hunt]

Landed r30506