Bug 43341

Summary: Canvas is not reset when setting canvas.width
Product: WebKit Reporter: lvanbass+webkitbugzilla
Component: Layout and RenderingAssignee: Matthew Delaney <mdelaney7>
Status: RESOLVED FIXED    
Severity: Major CC: commit-queue, darin, lvanbass+webkitbugzilla, mdelaney7
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
URL: http://codingwithattitude.net/gml/gml-drawer.html
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description lvanbass+webkitbugzilla 2010-08-02 05:56:23 PDT
1) Open this page : http://codingwithattitude.net/gml/gml-drawer.html and draw lines on the black canvas (this uses regular mousedown, mousemove and mouseup events)

2) Click on the "clear" action.

Expected result:
The canvas should be reset by setting its width to the original width

Observed result:
Nothing. Subsequently drawing on the surface adds a black square covering the old drawing under the new drawing.

Platform:
WebKit-r64451 nightly on Windows XP
Comment 1 Matthew Delaney 2010-08-03 21:11:53 PDT
Created attachment 63409 [details]
Patch
Comment 2 Darin Adler 2010-08-03 22:53:20 PDT
Comment on attachment 63409 [details]
Patch

Bug fixes need a test case. Could you make a test for this one please?
Comment 3 Darin Adler 2010-08-03 22:54:02 PDT
Comment on attachment 63409 [details]
Patch

review- due to lack of regression test

Fix itself looks good, although I might move the initialization of hadImageBuffer up even higher.
Comment 4 Matthew Delaney 2010-08-04 19:26:36 PDT
Created attachment 63534 [details]
Patch
Comment 5 Darin Adler 2010-08-05 07:35:10 PDT
Comment on attachment 63534 [details]
Patch

> -    bool ok;
> +    bool ok, hadImageBuffer = hasCreatedImageBuffer();

The two booleans should be defined on separate lines. We don't do two on one line like this in WebKit.

> Index: LayoutTests/fast/repaint/setWidthResetAfterForcedRender.html

While this does involve "repainting", I think this is a canvas test and should be in "fast/canvas".

Patch is OK as is, so review+ but I'm not setting commit-queue on this so you have a chance to decide whether to make some additional refinements before landing this.
Comment 6 Matthew Delaney 2010-08-05 11:48:32 PDT
Created attachment 63611 [details]
Patch
Comment 7 WebKit Commit Bot 2010-08-06 02:32:23 PDT
Comment on attachment 63611 [details]
Patch

Rejecting patch 63611 from commit-queue.

Failed to run "[u'git', u'svn', u'dcommit']" exit_code: 1
Last 500 characters of output:
e/ChangeLog
	M	WebCore/html/HTMLCanvasElement.cpp
A repository hook failed: MERGE request failed on '/repository/webkit/trunk': Commit blocked by pre-commit hook (exit code 1) with output:

    The following files contain tab characters:

        trunk/LayoutTests/fast/canvas/setWidthResetAfterForcedRender.html

    Please use spaces instead to indent.
    If you must commit a file with tabs, use svn propset to set the "allow-tabs" property.
 at /usr/local/git/libexec/git-core/git-svn line 572


Full output: http://queues.webkit.org/results/3576985
Comment 8 Matthew Delaney 2010-08-10 11:49:01 PDT
Created attachment 64035 [details]
Patch
Comment 9 Matthew Delaney 2010-08-10 12:18:52 PDT
Hopefully this one makes the commit bot happy :-)
Comment 10 Kenneth Rohde Christiansen 2010-08-14 08:05:41 PDT
Comment on attachment 64035 [details]
Patch

WebCore/ChangeLog:10
 +          hasCreatedImageBuffer up *before* the call to setSurface since setSurface
I guess this comment would be nice in the code as well.
Comment 11 WebKit Commit Bot 2010-08-14 08:37:06 PDT
Comment on attachment 64035 [details]
Patch

Clearing flags on attachment: 64035

Committed r65367: <http://trac.webkit.org/changeset/65367>
Comment 12 WebKit Commit Bot 2010-08-14 08:37:11 PDT
All reviewed patches have been landed.  Closing bug.