Bug 29502 - basic inputs layout test for chromium
Summary: basic inputs layout test for chromium
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-09-18 13:14 PDT by Karen
Modified: 2009-09-23 20:03 PDT (History)
3 users (show)

See Also:


Attachments
basic inputs layout test patch (89.26 KB, text/plain)
2009-09-18 13:26 PDT, Karen
no flags Details
basic inputs layout test patch - fixed patch. (86.39 KB, text/plain)
2009-09-18 13:54 PDT, Karen
eric: review-
Details
basic inputs layout test patch - added more info to changelog. (86.51 KB, text/plain)
2009-09-18 14:51 PDT, Karen
eric: review-
Details
basic inputs layout test patch - fixed tabs in changelog, capitalized. (86.49 KB, text/plain)
2009-09-18 15:05 PDT, Karen
no flags Details
fixed tabs in changelog. (86.49 KB, text/plain)
2009-09-18 15:12 PDT, Karen
eric: review-
Details
basic inputs layout test patch - fixed space in changelog and removed period. (86.49 KB, patch)
2009-09-18 15:34 PDT, Karen
dglazkov: review-
Details | Formatted Diff | Diff
basic inputs layout test patch - fixed indents and changelog (86.53 KB, patch)
2009-09-23 16:37 PDT, Karen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Karen 2009-09-18 13:14:58 PDT
This tests basic inputs for chromium to make sure they render properly.
Comment 1 Karen 2009-09-18 13:26:43 PDT
Created attachment 39784 [details]
basic inputs layout test patch
Comment 2 Pam Greene (IRC:pamg) 2009-09-18 13:36:29 PDT
A test for any WebKit port, really. It happens to have been written for Chromium, but any new port might find it useful.
Comment 3 Karen 2009-09-18 13:54:44 PDT
Created attachment 39787 [details]
basic inputs layout test patch - fixed patch.
Comment 4 Eric Seidel (no email) 2009-09-18 14:00:48 PDT
Comment on attachment 39787 [details]
basic inputs layout test patch - fixed patch.

The tab in the ChangeLog will make this unlandable.  (We have a pre-commit hook which rejects commits with tabs in them.)

The ChangeLog shoudl also explain more what you're doing here.  Why this test, and what does it test that other tests don't?
Comment 5 Karen 2009-09-18 14:51:36 PDT
Created attachment 39790 [details]
basic inputs layout test patch - added more info to changelog.

i added more info to the changelog. this patch is for a layout test to thoroughly test input fields. text and password, enabled and disabled.
Comment 6 Eric Seidel (no email) 2009-09-18 14:56:28 PDT
Comment on attachment 39790 [details]
basic inputs layout test patch - added more info to changelog.

This also has tabs in the ChangeLog, which will make it impossible to land automatically. :(

Missing a couple capital letters at the start of your sentences.

prepare-ChangeLog (the ChangeLog template generation tool) takes a --bug argument which automatically inserts the bug url in the expected format in case you need it in the future.  Generally the URL goes on its own line.

So asside from the ChagneLog nits, this looks fine.  but r- for the tabs.
Comment 7 Pam Greene (IRC:pamg) 2009-09-18 14:57:57 PDT
Any test containing an input field implicitly depends on these features, but it's difficult for a new port to isolate differences. It is helpful to have some tests that are intended only to document and verify the sizes and behavior of basic input fields.
Comment 8 Karen 2009-09-18 15:05:14 PDT
Created attachment 39794 [details]
basic inputs layout test patch - fixed tabs in changelog, capitalized.
Comment 9 Karen 2009-09-18 15:12:26 PDT
Created attachment 39797 [details]
fixed tabs in changelog.
Comment 10 Eric Seidel (no email) 2009-09-18 15:17:17 PDT
Comment on attachment 39797 [details]
fixed tabs in changelog.

Now you have an extra space at the top of your ChangeLog file. :(

You might find http://webkit.org/coding/contributing.html#changelogs of interest if you haven't already seen it.

Otherwise this looks fine.  I would r+ this, except you're not a committer so it has to be perfect for the commit bot to land this for you. :(

While we're at it, "For chomium" is not a sentence, I assume you added an extra period by mistake:
This tests all types of inputs text and password, both enabled and disabled, the existing tests weren't thorough enough. For chromium.
Comment 11 Karen 2009-09-18 15:34:01 PDT
Created attachment 39800 [details]
basic inputs layout test patch - fixed space in changelog and removed period.
Comment 12 Dimitri Glazkov (Google) 2009-09-23 15:42:58 PDT
Comment on attachment 39800 [details]
basic inputs layout test patch - fixed space in changelog and removed period.

> +  border:1px solid red;
> +  margin:10px;

4-space indent.

> +<div>
> +  a<input value="foobarbazfoobarbazfoobarbaz" type="text" />text
> +  <input value="foo" type="text" DISABLED />b
> +  a<input value="foo" type="password" />password
> +  <input value="foo" type="password" DISABLED />b
> +</div>

ditto here and on.
Comment 13 Karen 2009-09-23 16:37:15 PDT
Created attachment 40028 [details]
basic inputs layout test patch - fixed indents and changelog

basic inputs layout test patch - fixed indents and changelog
Comment 14 Eric Seidel (no email) 2009-09-23 17:24:18 PDT
Comment on attachment 40028 [details]
basic inputs layout test patch - fixed indents and changelog

LGTM.  I'm assuming you wanted this commit-queued since this won't break the canary (except in missing test results, which theoretically shouldn't be missing anyway).
Comment 15 WebKit Commit Bot 2009-09-23 20:03:07 PDT
Comment on attachment 40028 [details]
basic inputs layout test patch - fixed indents and changelog

Clearing flags on attachment: 40028

Committed r48699: <http://trac.webkit.org/changeset/48699>
Comment 16 WebKit Commit Bot 2009-09-23 20:03:12 PDT
All reviewed patches have been landed.  Closing bug.