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

• Status: RESOLVED
• Resolution: WONTFIX
• Component: Forms

[Kent Tamura]

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.

[Kent Tamura]

Created attachment 33710 [details]
Proposed patch

[Peter Kasting]

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

[Eric Seidel (no email)]

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]

(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]

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)]

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 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 on attachment 38718 [details]
Proposed patch (rev.2)

So all pixel test results should remain the same here?

[Eric Seidel (no email)]

(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]

This bug is obsolete.