Bug 27800 - [HTML5][Forms] Part 4 of datalist&list: Move controlClip from RenderTextControlSingleLine to TextControlElements.cpp
: [HTML5][Forms] Part 4 of datalist&list: Move controlClip from RenderTextContr...
Status: RESOLVED WONTFIX
Product: WebKit
Classification: Unclassified
Component: Forms
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To: Nobody
:
Depends on:
Blocks: 27247
  Show dependency treegraph
 
Reported: 2009-07-29 03:46 PDT by Kent Tamura
Modified: 2012-04-05 07:03 PDT (History)
4 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Kent Tamura 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.
Comment 1 Kent Tamura 2009-07-29 04:00:24 PDT
Created attachment 33710 [details]
Proposed patch
Comment 2 Peter Kasting 2009-08-04 15:59:27 PDT
dhyatt was working on <datalist> support, he should be CCed on all bugs about it
Comment 3 Eric Seidel 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?
Comment 4 Kent Tamura 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
Comment 5 Kent Tamura 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.
Comment 6 Eric Seidel 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. :(
Comment 7 Eric Seidel 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. :)
Comment 8 Adele Peterson 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?
Comment 9 Eric Seidel 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.
Comment 10 Kent Tamura 2012-04-05 07:03:25 PDT
This bug is obsolete.