Summary: | Remove an overloaded strokeRect in <canvas> | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dongseong Hwang <dongseong.hwang> | ||||||
Component: | Canvas | Assignee: | 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
Dongseong Hwang
2013-05-13 02:12:45 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 This patch is backported from Blink : https://codereview.chromium.org/14972014/ Created attachment 201541 [details]
Patch
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 on attachment 201541 [details]
Patch
I'd be worried about compatibility issues with this change.
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 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)"? Created attachment 201837 [details]
=Patch to Land
(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 on attachment 201837 [details] =Patch to Land Clearing flags on attachment: 201837 Committed r150137: <http://trac.webkit.org/changeset/150137> All reviewed patches have been landed. Closing bug. |