Bug 116017 – Remove an overloaded strokeRect in <canvas>

• Status: RESOLVED
• Resolution: FIXED
• Component: Canvas

[Dongseong Hwang]

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/

[Dongseong Hwang]

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

[Dongseong Hwang]

This patch is backported from Blink : https://codereview.chromium.org/14972014/

[Dongseong Hwang]

Created attachment 201541 [details]
Patch

[Darin Adler]

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?

[Simon Fraser (smfr)]

Comment on attachment 201541 [details]
Patch

I'd be worried about compatibility issues with this change.

[Dongseong Hwang]

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.

[Benjamin Poulain]

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)"?

[Dongseong Hwang]

Created attachment 201837 [details]
=Patch to Land

[Dongseong Hwang]

(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.

[WebKit Commit Bot]

Comment on attachment 201837 [details]
=Patch to Land

Clearing flags on attachment: 201837

Committed r150137: <http://trac.webkit.org/changeset/150137>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.