Bug 225263 - Update operations in CanvasImageData.idl to use long rather than float as per spec
Summary: Update operations in CanvasImageData.idl to use long rather than float as per...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sam Weinig
URL:
Keywords: InRadar
Depends on:
Blocks: 225140
  Show dependency treegraph
 
Reported: 2021-04-30 18:41 PDT by Sam Weinig
Modified: 2021-05-01 13:42 PDT (History)
9 users (show)

See Also:


Attachments
Patch (7.64 KB, patch)
2021-04-30 18:42 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (43.64 KB, patch)
2021-05-01 09:15 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2021-04-30 18:41:34 PDT
Update operations in CanvasImageData.idl to use long rather than float as per spec
Comment 1 Sam Weinig 2021-04-30 18:42:45 PDT
Created attachment 427480 [details]
Patch
Comment 2 Sam Weinig 2021-05-01 09:15:39 PDT
Created attachment 427498 [details]
Patch
Comment 3 Simon Fraser (smfr) 2021-05-01 10:27:08 PDT
Comment on attachment 427498 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=427498&action=review

I'm a bit nervous about compat fallout from this one.

> Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:2172
> +    IntSize size { std::abs(sw), std::abs(sh) };

Is this what the spec says; that negative values become positive? Are there tests for it?

> Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:2196
> +    IntRect imageDataRect { sx, sy, sw, sh };

Presumably the getImageData() call will fail if this rect ends up being outside the backing store rect? What if it's half outside?
Comment 4 Sam Weinig 2021-05-01 10:59:51 PDT
(In reply to Simon Fraser (smfr) from comment #3)
> Comment on attachment 427498 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=427498&action=review
> 
> I'm a bit nervous about compat fallout from this one.

Anything you would like me to do to make you feel less nervous? All the other engines, according to WPT and my testing have this new behavior.

> 
> > Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:2172
> > +    IntSize size { std::abs(sw), std::abs(sh) };
> 
> Is this what the spec says; that negative values become positive? Are there
> tests for it?

Yes (it's also not new in the change, I just added some braces). The spec says:

"When the createImageData() method is invoked with two numeric arguments sw and sh, it must create an ImageData object, with parameter pixelsPerRow set to the absolute magnitude of sw, and parameter rows set to the absolute magnitude of sh."

And yes, we have tests with negatives for these values. 

> 
> > Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:2196
> > +    IntRect imageDataRect { sx, sy, sw, sh };
> 
> Presumably the getImageData() call will fail if this rect ends up being
> outside the backing store rect? What if it's half outside?

This is also not new behavior but no, getImageData() returns transparent black pixels for any pixel outside the backing store. We also have testing of this.
Comment 5 EWS 2021-05-01 13:41:04 PDT
Committed r276877 (237223@main): <https://commits.webkit.org/237223@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 427498 [details].
Comment 6 Radar WebKit Bug Importer 2021-05-01 13:42:15 PDT
<rdar://problem/77421906>