Bug 116017

Summary: Remove an overloaded strokeRect in <canvas>
Product: WebKit Reporter: Dongseong Hwang <dongseong.hwang>
Component: CanvasAssignee: Dongseong Hwang <dongseong.hwang>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, cdumez, commit-queue, dino, esprehn+autocc, simon.fraser
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
benjamin: review+, benjamin: commit-queue-
=Patch to Land none

Description Dongseong Hwang 2013-05-13 02:12:45 PDT
The canvas spec [1] does not define strokeRect with 5 arguments, so this issue
remains only strokeRect with 4 arguments.

[1] http://www.w3.org/TR/2dcontext2/
Comment 1 Dongseong Hwang 2013-05-13 02:13:26 PDT
FF and Opera have only strokeRect with 4 arguments. I checked it using
fast/canvas/canvas-strokeRect.html

strokeRect with 5 arguments was created when <canvas> was introduced in WebKit. 
Author: darin <darin@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Date:   Thu Mar 16 08:54:13 2006 +0000

    Oops, these files were supposed to go in.
    
    
    git-svn-id: http://svn.webkit.org/repository/webkit/trunk@13330
268f45cc-cd09-0410-ab3c-d52691b4dbfc
Comment 2 Dongseong Hwang 2013-05-13 02:13:52 PDT
This patch is backported from Blink : https://codereview.chromium.org/14972014/
Comment 3 Dongseong Hwang 2013-05-13 03:32:57 PDT
Created attachment 201541 [details]
Patch
Comment 4 Darin Adler 2013-05-13 08:44:02 PDT
Comment on attachment 201541 [details]
Patch

Is it really OK to remove this just because the standard chose not to include this? How long has WebKit had this last argument? Is there content out there that uses it?
Comment 5 Simon Fraser (smfr) 2013-05-13 10:56:08 PDT
Comment on attachment 201541 [details]
Patch

I'd be worried about compatibility issues with this change.
Comment 6 Dongseong Hwang 2013-05-13 11:58:34 PDT
Thank you for (In reply to comment #4)
> (From update of attachment 201541 [details])
> Is it really OK to remove this just because the standard chose not to include this? How long has WebKit had this last argument? Is there content out there that uses it?

It's really OK. Blink, IE, FF and Opera do not have strokeRect with 5 arguments. I checked it already.

The first <canvas> implementation strangely included this strokeRect api.

(In reply to comment #5)
> (From update of attachment 201541 [details])
> I'd be worried about compatibility issues with this change.

ditto. Now only WebKit HAS this api.
Comment 7 Benjamin Poulain 2013-05-15 01:33:41 PDT
Comment on attachment 201541 [details]
Patch

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

(In reply to comment #4)
> (From update of attachment 201541 [details])
> Is it really OK to remove this just because the standard chose not to include this? How long has WebKit had this last argument? Is there content out there that uses it?

I just checked, this version was there since upstreaming of WebKit.

I am also afraid of compatibility, especially with Dashboard. I check our widgets and none of them use that version of strokeRect().

But I think this API is bad and we should probably align with the good stuff here.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1117
> +    if (!(state().m_lineWidth >= 0))
>          return;

Can you please move this branch after "if (!state().m_invertibleCTM)"?
Comment 8 Dongseong Hwang 2013-05-15 08:53:13 PDT
Created attachment 201837 [details]
=Patch to Land
Comment 9 Dongseong Hwang 2013-05-15 08:56:52 PDT
(In reply to comment #7)
> I just checked, this version was there since upstreaming of WebKit.
> 
> I am also afraid of compatibility, especially with Dashboard. I check our widgets and none of them use that version of strokeRect().
> 
> But I think this API is bad and we should probably align with the good stuff here.
> 
> > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1117
> > +    if (!(state().m_lineWidth >= 0))
> >          return;
> 
> Can you please move this branch after "if (!state().m_invertibleCTM)"?

Yes, done.

Thank you for review! please cq+, if you checked enough.
Comment 10 WebKit Commit Bot 2013-05-15 12:36:36 PDT
Comment on attachment 201837 [details]
=Patch to Land

Clearing flags on attachment: 201837

Committed r150137: <http://trac.webkit.org/changeset/150137>
Comment 11 WebKit Commit Bot 2013-05-15 12:36:38 PDT
All reviewed patches have been landed.  Closing bug.