| Summary: | Refactoring placeholder-related code | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Kent Tamura <tkent> | ||||||||||||||||
| Component: | Forms | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||
| Severity: | Normal | CC: | adele, commit-queue, darin, ddkilzer, eric | ||||||||||||||||
| Priority: | P2 | ||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||
| Hardware: | All | ||||||||||||||||||
| OS: | All | ||||||||||||||||||
| URL: | https://bugs.webkit.org/show_bug.cgi?id=21248 | ||||||||||||||||||
| Bug Depends on: | |||||||||||||||||||
| Bug Blocks: | 29782 | ||||||||||||||||||
| Attachments: |
|
||||||||||||||||||
|
Description
Kent Tamura
2009-08-24 23:23:12 PDT
Created attachment 38532 [details]
Proposed patch
No new tests because this patch is just a refactoring.
I confirmed run-webkit-tests passed on Mac.
Ideally adele should see this. Comment on attachment 38532 [details]
Proposed patch
I wish HTMLFormControlElementWithState were a more descriptive name. Its not obvious why this stuff goes in there and not in the HTMLFormControlElement.
HTMLFormControlElementWithState is used by HTMLSelectElement too, which has no placeholder. So we had better - move the placeholder methods to HTMLFormControlElement, or - introduce another subclass. HTMLFormControlElementWithPlaceholder? Which do you like? I think I'd like another subclass. The common thread is that these are both text controls, so the subclass name should say something about that. I think its likely that there are other bits of code we'll want to share between all text controls in the future. In fact, there may be other bits of duplicated code already. Created attachment 38657 [details]
Proposed patch (rev.2)
Intoduces HTMLTextFormControlElement class.
Alternatively we could share via composition instead of inheritance. (This looks OK though.) I think this patch is best reviewed by Adele. Created attachment 39631 [details]
Proposed patch (rev.3)
Resolved conflict with today's WebKit source.
Comment on attachment 39631 [details]
Proposed patch (rev.3)
No comment from Adele in a month. Looks like you're just moving code, and that's OK with me. r+
Comment on attachment 39631 [details] Proposed patch (rev.3) Rejecting patch 39631 from commit-queue. Patch https://bugs.webkit.org/attachment.cgi?id=39631 from bug 28703 failed to download and apply. Created attachment 40043 [details]
Proposed patch (rev.4)
Resolved a patch conflict.
Comment on attachment 40043 [details]
Proposed patch (rev.4)
OK.
Comment on attachment 40043 [details] Proposed patch (rev.4) Rejecting patch 40043 from commit-queue. Patch https://bugs.webkit.org/attachment.cgi?id=40043 from bug 28703 failed to download and apply. > Rejecting patch 40043 from commit-queue.
>
> Patch https://bugs.webkit.org/attachment.cgi?id=40043 from bug 28703 failed to
> download and apply.
I have no idea about the failure.
I need to fix bugzilla-tool to post failure messages for patch application failures too. patching file WebCore/html/HTMLTextAreaElement.h Hunk #1 succeeded at 31 with fuzz 1 (offset 1 line). Hunk #2 FAILED at 95. 1 out of 2 hunks FAILED -- saving rejects to file WebCore/html/HTMLTextAreaElement.h.rej (In reply to comment #15) > patching file WebCore/html/HTMLTextAreaElement.h > Hunk #1 succeeded at 31 with fuzz 1 (offset 1 line). > Hunk #2 FAILED at 95. > 1 out of 2 hunks FAILED -- saving rejects to file > WebCore/html/HTMLTextAreaElement.h.rej Ah, it conflicted with my latest landed patch :-) Thank you for the information. I'll update the patch soon. Created attachment 40046 [details]
Proposed patch (rev.5)
Resolved another conflict.
Comment on attachment 40046 [details]
Proposed patch (rev.5)
OK.
Comment on attachment 40046 [details]
Proposed patch (rev.5)
Rejecting patch 40046 from commit-queue.
Failed to run "WebKitTools/Scripts/build-webkit" exit_code: 1
Last 500 characters of output:
compilers.gcc.4_2
Distributed-CompileC /Users/eseidel/Projects/build/WebCore.build/Release/WebCore.build/Objects-normal/i386/InputElement.o /Users/eseidel/Projects/CommitQueue/WebCore/dom/InputElement.cpp normal i386 c++ com.apple.compilers.gcc.4_2
Distributed-CompileC /Users/eseidel/Projects/build/WebCore.build/Release/WebCore.build/Objects-normal/i386/XMLHttpRequest.o /Users/eseidel/Projects/CommitQueue/WebCore/xml/XMLHttpRequest.cpp normal i386 c++ com.apple.compilers.gcc.4_2
(3 failures)
(In reply to comment #19) > (From update of attachment 40046 [details]) > Rejecting patch 40046 from commit-queue. > > Failed to run "WebKitTools/Scripts/build-webkit" exit_code: 1 > Last 500 characters of output: > compilers.gcc.4_2 > Distributed-CompileC > /Users/eseidel/Projects/build/WebCore.build/Release/WebCore.build/Objects-normal/i386/InputElement.o > /Users/eseidel/Projects/CommitQueue/WebCore/dom/InputElement.cpp normal i386 > c++ com.apple.compilers.gcc.4_2 > Distributed-CompileC > /Users/eseidel/Projects/build/WebCore.build/Release/WebCore.build/Objects-normal/i386/XMLHttpRequest.o > /Users/eseidel/Projects/CommitQueue/WebCore/xml/XMLHttpRequest.cpp normal i386 > c++ com.apple.compilers.gcc.4_2 > (3 failures) Oops, a huge patch modifying many files was committed some minutes ago.... (In reply to comment #20) > (In reply to comment #19) > > (From update of attachment 40046 [details] [details]) > > Rejecting patch 40046 from commit-queue. > > > > Failed to run "WebKitTools/Scripts/build-webkit" exit_code: 1 > > Oops, a huge patch modifying many files was committed some minutes ago.... It was caused by r48701 build break. I think It's safe to commit this patch now. Comment on attachment 40046 [details]
Proposed patch (rev.5)
Adding cq+ per tkent's request and eric's review+ .
Comment on attachment 40046 [details]
Proposed patch (rev.5)
Rejecting patch 40046 from commit-queue.
Failed to run "WebKitTools/Scripts/build-webkit" exit_code: 1
Last 500 characters of output:
ts/build/Release/DerivedSources/WebCore/JSCanvasRenderingContext2D.cpp -o /Users/eseidel/Projects/build/WebCore.build/Release/WebCore.build/Objects-normal/i386/JSCanvasRenderingContext2D.o
** BUILD FAILED **
The following build commands failed:
WebCore:
Distributed-CompileC /Users/eseidel/Projects/build/WebCore.build/Release/WebCore.build/Objects-normal/i386/InputElement.o /Users/eseidel/Projects/CommitQueue/WebCore/dom/InputElement.cpp normal i386 c++ com.apple.compilers.gcc.4_2
(1 failure)
(In reply to comment #23) > (From update of attachment 40046 [details]) > Rejecting patch 40046 from commit-queue. > > Failed to run "WebKitTools/Scripts/build-webkit" exit_code: 1 > Last 500 characters of output: > ts/build/Release/DerivedSources/WebCore/JSCanvasRenderingContext2D.cpp -o > /Users/eseidel/Projects/build/WebCore.build/Release/WebCore.build/Objects-normal/i386/JSCanvasRenderingContext2D.o > ** BUILD FAILED ** > > The following build commands failed: > WebCore: > Distributed-CompileC > /Users/eseidel/Projects/build/WebCore.build/Release/WebCore.build/Objects-normal/i386/InputElement.o > /Users/eseidel/Projects/CommitQueue/WebCore/dom/InputElement.cpp normal i386 > c++ com.apple.compilers.gcc.4_2 > (1 failure) I couldn't reproduce the build problem. WebKit r48714, the latest revision for now, and svn-apply for the download patch 40046 were successfully built. Comment on attachment 40046 [details] Proposed patch (rev.5) I confirmed patching, building, testing with r48775. Comment on attachment 40046 [details]
Proposed patch (rev.5)
OK. We'll try again. I probably had something screwed up on that machine.
Comment on attachment 40046 [details]
Proposed patch (rev.5)
Rejecting patch 40046 from commit-queue.
Failed to run "WebKitTools/Scripts/build-webkit" exit_code: 1
Last 500 characters of output:
itQueue/WebCore/bindings/js/JSCanvasRenderingContext2DCustom.cpp -o /Users/eseidel/Projects/build/WebCore.build/Release/WebCore.build/Objects-normal/i386/JSCanvasRenderingContext2DCustom.o
** BUILD FAILED **
The following build commands failed:
WebCore:
Distributed-CompileC /Users/eseidel/Projects/build/WebCore.build/Release/WebCore.build/Objects-normal/i386/InputElement.o /Users/eseidel/Projects/CommitQueue/WebCore/dom/InputElement.cpp normal i386 c++ com.apple.compilers.gcc.4_2
(1 failure)
Are you sure you tried building Release? It's possible that your patch works in Debug mode but not Release... (In reply to comment #28) > Are you sure you tried building Release? It's possible that your patch works > in Debug mode but not Release... Oh I didn't check Release. >> WebCore/dom/InputElement.cpp:119: warning: unused parameter ‘inputElement’ This is the error with --release. Created attachment 40160 [details]
Proposed patch (rev.6)
Add UNUSED_PARAM(inputElement) to build Release.
Comment on attachment 40160 [details] Proposed patch (rev.6) > // Renderer and our event handler are responsible for sanitizing values. > ASSERT(value == inputElement->sanitizeValue(value) || inputElement->sanitizeValue(value).isEmpty()); > - > - if (inputElement->isTextField()) > - updatePlaceholderVisibility(inputElement, element); > + UNUSED_PARAM(inputElement); Instead of UNUSED_PARAM you should be using ASSERT_UNUSED, which is the assert macro that identifies an argument used only for the assertion and not in release versions. ASSERT_UNUSED(inputElement, value == inputElement->sanitizeValue(value) || inputElement->sanitizeValue(value).isEmpty()); That's how you do it. Created attachment 40162 [details]
Proposed patch (rev.7)
ASSERT + UNUSED_PARAM -> ASSERT_UNUSED
(In reply to comment #31) > Instead of UNUSED_PARAM you should be using ASSERT_UNUSED, which is the assert > macro that identifies an argument used only for the assertion and not in > release versions. > > ASSERT_UNUSED(inputElement, value == inputElement->sanitizeValue(value) || > inputElement->sanitizeValue(value).isEmpty()); Thank you. I changed so. Comment on attachment 40162 [details] Proposed patch (rev.7) r=me Interdiff is cool! <https://bugs.webkit.org/attachment.cgi?oldid=40160&action=interdiff&newid=40162&headers=1> I did not realize bugzilla could do that. :) Interdiff is one of the things I miss about rietveld. Comment on attachment 40162 [details] Proposed patch (rev.7) Clearing flags on attachment: 40162 Committed r48792: <http://trac.webkit.org/changeset/48792> All reviewed patches have been landed. Closing bug. |