Bug 29493

Summary: [LTTF] Comprehensive testing of button rendering
Product: WebKit Reporter: Karen <karen+webkit>
Component: Tools / TestsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, dglazkov, eric, pam
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: OS X 10.5   
Attachments:
Description Flags
basic button layout test patch
none
fixed the changelog - basic button layout test for chromium.
none
basic button layout test patch
eric: review-
screen shot of the mac rendering, the buttons with text are ignoring the padding and the one with image are inconsistent.
none
tests basic buttons comprehensively. added expected files, fixed changelog.
dglazkov: review-
basic button layout test patch - fixed brace to be on new line
dglazkov: review-
basic button layout test patch - changed to have 4 space indents
dglazkov: review-
basic button layout test patch - changed to have 4 space indents none

Description Karen 2009-09-18 10:12:36 PDT
adding layout test for Chromium. This is a basic test that checks button rendering.
Comment 1 Karen 2009-09-18 10:13:16 PDT
Created attachment 39765 [details]
basic button layout test patch
Comment 2 Karen 2009-09-18 10:27:22 PDT
Created attachment 39766 [details]
fixed the changelog - basic button layout test for chromium.
Comment 3 Karen 2009-09-18 10:49:53 PDT
patch coming.
Comment 4 Karen 2009-09-18 12:10:17 PDT
Created attachment 39774 [details]
basic button layout test patch

there's a problem with the buttons not rendering the same on Mac vs PC. This patch just contains the HTML file. I will add the others as soon as I get information on what to do. please see bug for more details.
Comment 5 Karen 2009-09-18 12:11:00 PDT
Created attachment 39775 [details]
screen shot of the mac rendering, the buttons with text are ignoring the padding and the one with image are inconsistent.
Comment 6 Karen 2009-09-18 12:13:12 PDT
Hi,

This is a basic layout test chromium uses to check for proper button rendering. If you look at the results on a mac vs PC, you will see different results. The PC version renders both the image and text buttons with the proper padding. The Mac version doesn't add any padding to the text button and two of the paddings (10% and 2px) on the image button appear different than the rest. (see attached PNG file)

I included the html file in the patch and will wait for further instructions before I submit full patch with expected.txt, png, checksum files.

thank you.
Comment 7 Eric Seidel (no email) 2009-09-18 13:57:25 PDT
Comment on attachment 39774 [details]
basic button layout test patch

expected.png and expected.checksum are missing from your patch.

What is this test for?  Please explain in your ChangeLog.  What does this test cover that other tests do not already?
Comment 8 Eric Seidel (no email) 2009-09-18 13:58:09 PDT
(In reply to comment #7)
> (From update of attachment 39774 [details])
> expected.png and expected.checksum are missing from your patch.
> 
> What is this test for?  Please explain in your ChangeLog.  What does this test
> cover that other tests do not already?

Looks like you explained some of this in the bug, but the ChangeLog needs more of this info.
Comment 9 Karen 2009-09-22 10:41:34 PDT
Created attachment 39930 [details]
tests basic buttons comprehensively. added expected files, fixed changelog.

cleaned up patch. added comment to html, generated expected results and cleaned changelog.
Comment 10 Dimitri Glazkov (Google) 2009-09-23 15:34:49 PDT
Comment on attachment 39930 [details]
tests basic buttons comprehensively. added expected files, fixed changelog.

looks good, except for:

> +function printSize(tagname, cell) {

brace on new line for functions.
Comment 11 Karen 2009-09-23 15:43:25 PDT
Created attachment 40023 [details]
basic button layout test patch - fixed brace to be on new line

fixed function brace to be on a new line.
Comment 12 Dimitri Glazkov (Google) 2009-09-23 15:49:35 PDT
Comment on attachment 40023 [details]
basic button layout test patch - fixed brace to be on new line

I just realized that all indents are 2-space here :)
Comment 13 Karen 2009-09-23 16:11:40 PDT
Created attachment 40025 [details]
basic button layout test patch - changed to have 4 space indents 

changed the file to have 4-space indents
Comment 14 Dimitri Glazkov (Google) 2009-09-23 16:16:48 PDT
Comment on attachment 40025 [details]
basic button layout test patch - changed to have 4 space indents 


> +     return '<tr><td>' + (style || '(default)') + '</td>' +
> +    '<td><button ' + (style ? 'style="' + style + '"' : '') + '>' + fooImage + '</button></td>' +
> +    '<td></td>' +
> +    '<td><input type="button" value="foo" style="' + style + '"></td>' +
> +    '<td></td></tr>';

Extra 4 spaces. Sorry :)
Comment 15 Karen 2009-09-23 16:22:25 PDT
Created attachment 40026 [details]
basic button layout test patch - changed to have 4 space indents 

fixed indenting
Comment 16 Dimitri Glazkov (Google) 2009-09-23 16:23:42 PDT
Comment on attachment 40026 [details]
basic button layout test patch - changed to have 4 space indents 

r=me.
Comment 17 WebKit Commit Bot 2009-09-23 17:40:43 PDT
Comment on attachment 40026 [details]
basic button layout test patch - changed to have 4 space indents 

Clearing flags on attachment: 40026

Committed r48690: <http://trac.webkit.org/changeset/48690>
Comment 18 WebKit Commit Bot 2009-09-23 17:40:49 PDT
All reviewed patches have been landed.  Closing bug.
Comment 19 Eric Seidel (no email) 2009-09-23 18:15:15 PDT
It looks like the commit-queue failed to land the .png file due to bug 29622.  This is bad. :(   Re-opening.  I'll take care of fixing things.
Comment 20 WebKit Commit Bot 2009-09-23 19:17:59 PDT
Committed r48696: <http://trac.webkit.org/changeset/48696>
Comment 21 Eric Seidel (no email) 2009-09-23 19:19:38 PDT
Sorry, that wasn't actually the commit-queue making the commit.  I just happened to borrow the commit bot's checkout to do the commit. :(