Bug 28703 – Refactoring placeholder-related code

• Component: Forms
• Status: RESOLVED FIXED

[Kent Tamura]

As Eric wrote in bug#21248, we should unify placeholder-related code in InputElement and HTMLTextAreaElement.

[Kent Tamura]

Created attachment 38532 [details]
Proposed patch

No new tests because this patch is just a refactoring.
I confirmed run-webkit-tests passed on Mac.

[Eric Seidel (no email)]

Ideally adele should see this.

[Adele Peterson]

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.

[Kent Tamura]

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?

[Adele Peterson]

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.

[Kent Tamura]

Created attachment 38657 [details]
Proposed patch (rev.2)

Intoduces HTMLTextFormControlElement class.

[Eric Seidel (no email)]

Alternatively we could share via composition instead of inheritance.  (This looks OK though.)  I think this patch is best reviewed by Adele.

[Kent Tamura]

Created attachment 39631 [details]
Proposed patch (rev.3)

Resolved conflict with today's WebKit source.

[Eric Seidel (no email)]

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+

[WebKit Commit Bot]

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.

[Kent Tamura]

Created attachment 40043 [details]
Proposed patch (rev.4)

Resolved a patch conflict.

[Eric Seidel (no email)]

Comment on attachment 40043 [details]
Proposed patch (rev.4)

OK.

[WebKit Commit Bot]

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.

[Kent Tamura]

> 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.

[Eric Seidel (no email)]

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

[Kent Tamura]

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

[Kent Tamura]

Created attachment 40046 [details]
Proposed patch (rev.5)

Resolved another conflict.

[Eric Seidel (no email)]

Comment on attachment 40046 [details]
Proposed patch (rev.5)

OK.

[WebKit Commit Bot]

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)

[Kent Tamura]

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

[Kent Tamura]

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

[Shinichiro Hamaji]

Comment on attachment 40046 [details]
Proposed patch (rev.5)

Adding cq+ per tkent's request and eric's review+ .

[WebKit Commit Bot]

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)

[Kent Tamura]

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

[Kent Tamura]

Comment on attachment 40046 [details]
Proposed patch (rev.5)

I confirmed patching, building, testing with r48775.

[Eric Seidel (no email)]

Comment on attachment 40046 [details]
Proposed patch (rev.5)

OK.  We'll try again.  I probably had something screwed up on that machine.

[WebKit Commit Bot]

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)

[Eric Seidel (no email)]

Are you sure you tried building Release?  It's possible that your patch works in Debug mode but not Release...

[Kent Tamura]

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

[Kent Tamura]

Created attachment 40160 [details]
Proposed patch (rev.6)

Add UNUSED_PARAM(inputElement) to build Release.

[Darin Adler]

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.

[Kent Tamura]

Created attachment 40162 [details]
Proposed patch (rev.7)

ASSERT + UNUSED_PARAM -> ASSERT_UNUSED

[Kent Tamura]

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

[David Kilzer (:ddkilzer)]

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>

[Eric Seidel (no email)]

I did not realize bugzilla could do that. :)  Interdiff is one of the things I miss about rietveld.

[WebKit Commit Bot]

Comment on attachment 40162 [details]
Proposed patch (rev.7)

Clearing flags on attachment: 40162

Committed r48792: <http://trac.webkit.org/changeset/48792>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.