Bug 34074 - Convert textarea-rows-cols.html to dumpAsText()
Summary: Convert textarea-rows-cols.html to dumpAsText()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-01-25 00:48 PST by Kent Tamura
Modified: 2010-02-01 21:10 PST (History)
4 users (show)

See Also:


Attachments
Patch (96.54 KB, patch)
2010-01-25 00:50 PST, Kent Tamura
no flags Details | Formatted Diff | Diff
Proposed patch (rev.2) (3.24 KB, patch)
2010-01-25 22:01 PST, Kent Tamura
no flags Details | Formatted Diff | Diff
Go back to rev1 (3.08 KB, patch)
2010-01-27 08:01 PST, Shinichiro Hamaji
no flags Details | Formatted Diff | Diff
ChangeLog entry was wrong... (3.11 KB, patch)
2010-01-27 08:02 PST, Shinichiro Hamaji
no flags Details | Formatted Diff | Diff
ChangeLog was wrong again (3.11 KB, patch)
2010-02-01 15:46 PST, Shinichiro Hamaji
dimich: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kent Tamura 2010-01-25 00:48:44 PST
Convert textarea-rows-cols.html to dumpAsText()
Comment 1 Kent Tamura 2010-01-25 00:50:40 PST
Created attachment 47324 [details]
Patch
Comment 2 Shinichiro Hamaji 2010-01-25 21:07:59 PST
Comment on attachment 47324 [details]
Patch

Looks good, but I feel the new test lacks information about scrollbar for the cols=1 case. It would be good to check if scrollHeight > defaultScrollHeight and scrollWidth < defaultScrollWidth ?
Comment 3 Kent Tamura 2010-01-25 22:01:51 PST
Created attachment 47387 [details]
Proposed patch (rev.2)
Comment 4 Kent Tamura 2010-01-25 22:02:39 PST
(In reply to comment #2)
> (From update of attachment 47324 [details])
> Looks good, but I feel the new test lacks information about scrollbar for the
> cols=1 case. It would be good to check if scrollHeight > defaultScrollHeight
> and scrollWidth < defaultScrollWidth ?

Thank you for the comment.
I added these checks.
Comment 5 Shinichiro Hamaji 2010-01-25 22:17:27 PST
Comment on attachment 47387 [details]
Proposed patch (rev.2)

Thanks for the fix! r+ assuming you'll address the comment below and combine patch v1 and v2 into a single patch.

> -shouldBeTrue('defaultHeight > 0');
> -shouldBeTrue('defaultWidth > 0');

I guess we don't need to remove them?
Comment 6 Kent Tamura 2010-01-25 22:30:41 PST
(In reply to comment #5)
> (From update of attachment 47387 [details])
> Thanks for the fix! r+ assuming you'll address the comment below and combine
> patch v1 and v2 into a single patch.

Wow, It seems I have missed --amend.  I'll commit combined patch manually.

> > -shouldBeTrue('defaultHeight > 0');
> > -shouldBeTrue('defaultWidth > 0');
> 
> I guess we don't need to remove them?

Other assertions contain them implicitly. We may have them for readability, of course. I'll revive them.
Comment 7 Kent Tamura 2010-01-26 01:44:00 PST
Landed as r53844 <http://trac.webkit.org/changeset/53844>
Comment 9 Eric Seidel (no email) 2010-01-27 03:20:41 PST
Looks like it's been broken for a while.  I suspect this change.
Comment 10 Shinichiro Hamaji 2010-01-27 08:00:07 PST
(In reply to comment #9)
> Looks like it's been broken for a while.  I suspect this change.

Yes, this happened due to my suggestion, sorry. It seems Windows Safari doesn't show scrollbars for this case. I think the reason of this difference is the size of fonts so we can just remove checks for scrollbars.
Comment 11 Shinichiro Hamaji 2010-01-27 08:01:06 PST
Created attachment 47536 [details]
Go back to rev1
Comment 12 Shinichiro Hamaji 2010-01-27 08:02:28 PST
Created attachment 47537 [details]
ChangeLog entry was wrong...
Comment 13 Eric Seidel (no email) 2010-02-01 15:36:19 PST
Comment on attachment 47537 [details]
ChangeLog entry was wrong...

ChangeLog looks wrong.
Comment 14 Shinichiro Hamaji 2010-02-01 15:46:43 PST
Created attachment 47875 [details]
ChangeLog was wrong again
Comment 15 Shinichiro Hamaji 2010-02-01 15:47:18 PST
> ChangeLog looks wrong.

Oops sorry.
Comment 16 Dmitry Titov 2010-02-01 18:22:11 PST
Comment on attachment 47875 [details]
ChangeLog was wrong again

Looks good. r=me.
Comment 17 Shinichiro Hamaji 2010-02-01 21:10:58 PST
Committed r54201: <http://trac.webkit.org/changeset/54201>