Bug 27800

Summary:

[HTML5][Forms] Part 4 of datalist&list: Move controlClip from RenderTextControlSingleLine to TextControlElements.cpp

Product:

WebKit

Reporter:

Kent Tamura <tkent>

Component:

Forms

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED WONTFIX

Severity:

Normal

CC:

adele, dwim79, eric, hyatt

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Bug Depends on:

Bug Blocks:

27247

Attachments:

Description	Flags
Proposed patch	eric: review-
Proposed patch (rev.2)	eric: review-

Kent Tamura

Reported 2009-07-29 03:46:34 PDT

For https://bugs.webkit.org/show_bug.cgi?id=27247 , I'd like to render a button in the border-box of the input element. The content-box clipRect in RenderTextControlSingleLine prevents it. So I'd like to move clipRect to RenderTextControlSingleLine::m_innerBlock.

Attachments
Proposed patch (12.24 KB, patch) 2009-07-29 04:00 PDT, Kent Tamura	eric: review-	Details Formatted Diff Diff
Proposed patch (rev.2) (58.40 KB, patch) 2009-08-27 23:55 PDT, Kent Tamura	eric: review-	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Kent Tamura

Comment 1 2009-07-29 04:00:24 PDT

Created attachment 33710 [details] Proposed patch

Peter Kasting

Comment 2 2009-08-04 15:59:27 PDT

dhyatt was working on <datalist> support, he should be CCed on all bugs about it

Eric Seidel (no email)

Comment 3 2009-08-07 13:53:52 PDT

Comment on attachment 33710 [details] Proposed patch Adele Peterson is your best reviewer. Please contact her directly. This should be guarded by ENABLE_DATALIST, no?

Kent Tamura

Comment 4 2009-08-09 18:17:17 PDT

(In reply to comment #3) > This should be guarded by ENABLE_DATALIST, no? No. This patch is a refactoring and doesn't add new behavior though it is required to make a UI of https://bugs.webkit.org/show_bug.cgi?id=27247

Kent Tamura

Comment 5 2009-08-27 23:55:34 PDT

Created attachment 38718 [details] Proposed patch (rev.2) * Update test files The code is not guarded by ENABLE(DATALIST) because this patch doesn't make any user-visible changes. This patch can be applied without the part 3 patch.

Eric Seidel (no email)

Comment 6 2009-09-23 10:22:38 PDT

I don't really understand what this does. Please email Adele and ask her for a quick review, or at least a "this generally looks OK" comment from her in the bug. This has been up for review for 3 weeks now. :(

Eric Seidel (no email)

Comment 7 2009-09-23 10:26:36 PDT

Comment on attachment 38718 [details] Proposed patch (rev.2) Actually, the change looks fine to me, but the ChangeLog is really missing explanation as to what you're doing here. Please post a new patch with explanation in the Changelog as to *why* you're making this change, and little snippets next to the individual files for the tricky bits of the change. Clear changelogs tend to improve review speeds. http://webkit.org/coding/contributing.html#changelogs Talks about good ChangeLogs and even points to an example. Strongly suggested reading for new(-ish) contributors. r- for the unclear ChangeLog, but in general this looks fine. Again, would be good to get a "yeah this looks sane" from someone who works on forms, but if they're not answering your email (assuming you've sent one?), then there is only so much we can do. :)

Adele Peterson

Comment 8 2009-09-23 10:29:34 PDT

Comment on attachment 38718 [details] Proposed patch (rev.2) So all pixel test results should remain the same here?

Eric Seidel (no email)

Comment 9 2009-09-23 11:06:00 PDT

(In reply to comment #8) > (From update of attachment 38718 [details]) > So all pixel test results should remain the same here? That was my understanding of the patch, although tkent could comment better.

Kent Tamura

Comment 10 2012-04-05 07:03:25 PDT

This bug is obsolete.

Note You need to log in before you can comment on or make changes to this bug.