Bug 39212 – Window object should have a CanvasPixelArray

• Status: RESOLVED
• Resolution: INVALID
• Component: WebCore Misc.

[Andreas Kling]

Spec link:
http://www.whatwg.org/specs/web-apps/current-work/#canvaspixelarray

[Andreas Kling]

Created attachment 56233 [details]
Proposed patch, first stab

[WebKit Review Bot]

Attachment 56233 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/bindings/js/JSCanvasPixelArrayCustom.cpp:27:  You should add a blank line after implementation file's own header.  [build/include_order] [4]
Total errors found: 1 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Andreas Kling]

Created attachment 56234 [details]
Proposed patch, first stab

[Oliver Hunt]

Comment on attachment 56234 [details]
Proposed patch, first stab

This patch would destroy pixel array performance -- what is the bug you are seeing?

[Oliver Hunt]

On the assumption that you're wanting to make a CPA instance claim to be an instanceof CanvasPixelArray you'll just want to make the logic that creates a CPA specify use the prototype from the CanvasPixelArray constructor, and set the constructor property on the CPA appropriately.

That said, you clearly are not testing performance of your change -- even the most rudimentary tests would have shown a huge (if memory serves ~5-10x) regression in read/write access to the cpa data.

[Andreas Kling]

(In reply to comment #5)
> On the assumption that you're wanting to make a CPA instance claim to be an instanceof CanvasPixelArray you'll just want to make the logic that creates a CPA specify use the prototype from the CanvasPixelArray constructor, and set the constructor property on the CPA appropriately.

Your assumption is correct. I'll update the patch once I wrap my head around it.

> That said, you clearly are not testing performance of your change -- even the most rudimentary tests would have shown a huge (if memory serves ~5-10x) regression in read/write access to the cpa data.

Indeed true, this was merely a first (rather naive) shot at getting a "correct" implementation. :-)

[Andreas Kling]

Right. The claim-to-be-a-CanvasPixelArray part is already covered by finagling in JSImageDataCustom.cpp

The missing thing is a CanvasPixelArray constructor on the window object.

Tests failing because of this:
- canvas/philip/tests/2d.imageData.create1.type.html
- canvas/philip/tests/2d.imageData.create2.type.html
- canvas/philip/tests/2d.imageData.get.type.html

[Joshua Bell]

Some of these have recently started passing on chromium mac:

10.6 debug circa  http://trac.webkit.org/log/?verbose=on&rev=117893&stop_rev=117815 and http://build.chromium.org/f/chromium/perf/dashboard/ui/changelog.html?url=/trunk/src&mode=html&range=138206:138138

All mac variations circa http://trac.webkit.org/log/?verbose=on&rev=118542&stop_rev=118542 and http://build.chromium.org/f/chromium/perf/dashboard/ui/changelog.html?url=/trunk/src&mode=html&range=139078:139073

canvas/philip/tests/2d.imageData.create1.type.html,canvas/philip/tests/2d.imageData.create2.type.html,media/video-volume-slider.html,platform/chromium/virtual/gpu/canvas/philip/tests/2d.imageData.create1.type.html,platform/chromium/virtual/gpu/canvas/philip/tests/2d.imageData.create2.type.html

[Joshua Bell]

Whoops, that passing test list should be:

canvas/philip/tests/2d.imageData.create1.type.html
canvas/philip/tests/2d.imageData.create2.type.html
platform/chromium/virtual/gpu/canvas/philip/tests/2d.imageData.create1.type.html
platform/chromium/virtual/gpu/canvas/philip/tests/2d.imageData.create2.type.html

[Joshua Bell]

Ah, due to rebaseline from http://trac.webkit.org/changeset/117818/

That rebaseline is probably incorrect and should be removed.

[Thiago Marcos P. Santos]

CanvasPixelArray is deprecated. Layout tests will be updated at bug 94474.