Bug 17486 - Support HTML5 Canvas.getImageData API
Summary: Support HTML5 Canvas.getImageData API
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Oliver Hunt
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-02-21 21:34 PST by Oliver Hunt
Modified: 2008-02-22 17:17 PST (History)
3 users (show)

See Also:


Attachments
Woo! getImageData support! (76.92 KB, patch)
2008-02-21 21:39 PST, Oliver Hunt
sam: review-
Details | Formatted Diff | Diff
Address weinig's commentary (77.92 KB, patch)
2008-02-22 01:49 PST, Oliver Hunt
sam: review-
Details | Formatted Diff | Diff
getImageData Redux (84.82 KB, patch)
2008-02-22 15:59 PST, Oliver Hunt
sam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Oliver Hunt 2008-02-21 21:34:44 PST
See HTML5 getImageData spec
Comment 1 Oliver Hunt 2008-02-21 21:39:13 PST
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
Comment 2 Dave Hyatt 2008-02-21 23:13:00 PST
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.
Comment 3 Sam Weinig 2008-02-21 23:41:11 PST
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.
Comment 4 Oliver Hunt 2008-02-22 01:49:15 PST
Created attachment 19276 [details]
Address weinig's commentary 

Hopefully i have sufficiently appeased the weinig
Comment 5 Sam Weinig 2008-02-22 13:24:39 PST
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.
Comment 6 Oliver Hunt 2008-02-22 15:59:21 PST
Created attachment 19283 [details]
getImageData Redux

Now we report the extra memory usage to the collectorator and look pretty and stuff
Comment 7 Sam Weinig 2008-02-22 16:07:17 PST
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.
Comment 8 Oliver Hunt 2008-02-22 17:17:49 PST
Landed r30506