28703 – Refactoring placeholder-related code

Bug 28703 - Refactoring placeholder-related code

Summary: Refactoring placeholder-related code

Status:	RESOLVED FIXED

Alias:	None

Product:	WebKit
Classification:	Unclassified
Component:	Forms (show other bugs)
Version:	528+ (Nightly build)
Hardware:	All All

Importance:	P2 Normal
Assignee:	Nobody

URL:	https://bugs.webkit.org/show_bug.cgi?...
Keywords:

Depends on:
Blocks:	29782
	Show dependency tree / graph

Reported:	2009-08-24 23:23 PDT by Kent Tamura
Modified:	2009-09-27 09:31 PDT (History)
CC List:	5 users (show)

See Also:

Attachments
Proposed patch (22.57 KB, patch) 2009-08-24 23:39 PDT, Kent Tamura	no flags	Details \| Formatted Diff \| Diff
Proposed patch (rev.2) (24.36 KB, patch) 2009-08-26 22:43 PDT, Kent Tamura	no flags	Details \| Formatted Diff \| Diff
Proposed patch (rev.3) (24.69 KB, patch) 2009-09-15 20:02 PDT, Kent Tamura	eric: review+ commit-queue: commit-queue-	Details \| Formatted Diff \| Diff
Proposed patch (rev.4) (26.13 KB, patch) 2009-09-23 21:04 PDT, Kent Tamura	eric: review+ commit-queue: commit-queue-	Details \| Formatted Diff \| Diff
Proposed patch (rev.5) (26.30 KB, patch) 2009-09-23 22:53 PDT, Kent Tamura	eric: review+ commit-queue: commit-queue-	Details \| Formatted Diff \| Diff
Proposed patch (rev.6) (26.47 KB, patch) 2009-09-25 20:01 PDT, Kent Tamura	no flags	Details \| Formatted Diff \| Diff
Proposed patch (rev.7) (26.49 KB, patch) 2009-09-25 22:35 PDT, Kent Tamura	no flags	Details \| Formatted Diff \| Diff
Show Obsolete (6) View All Add an attachment (proposed patch, testcase, etc.)

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

Description Kent Tamura 2009-08-24 23:23:12 PDT

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

Comment 1 Kent Tamura 2009-08-24 23:39:34 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.

Comment 2 Eric Seidel (no email) 2009-08-25 00:34:16 PDT

Ideally adele should see this.

Comment 3 Adele Peterson 2009-08-25 11:02:09 PDT

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.

Comment 4 Kent Tamura 2009-08-26 18:56:54 PDT

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?

Comment 5 Adele Peterson 2009-08-26 21:07:30 PDT

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.

Comment 6 Kent Tamura 2009-08-26 22:43:45 PDT

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

Intoduces HTMLTextFormControlElement class.

Comment 7 Eric Seidel (no email) 2009-09-01 03:43:02 PDT

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

Comment 8 Kent Tamura 2009-09-15 20:02:44 PDT

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

Resolved conflict with today's WebKit source.

Comment 9 Eric Seidel (no email) 2009-09-23 10:39:50 PDT

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 10 WebKit Commit Bot 2009-09-23 17:15:11 PDT

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.

Comment 11 Kent Tamura 2009-09-23 21:04:12 PDT

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

Resolved a patch conflict.

Comment 12 Eric Seidel (no email) 2009-09-23 21:09:33 PDT

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

OK.

Comment 13 WebKit Commit Bot 2009-09-23 21:24:07 PDT

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.

Comment 14 Kent Tamura 2009-09-23 22:34:52 PDT

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

Comment 15 Eric Seidel (no email) 2009-09-23 22:40:25 PDT

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

Comment 16 Kent Tamura 2009-09-23 22:52:18 PDT

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

Comment 17 Kent Tamura 2009-09-23 22:53:12 PDT

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

Resolved another conflict.

Comment 18 Eric Seidel (no email) 2009-09-23 22:57:15 PDT

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

OK.

Comment 19 WebKit Commit Bot 2009-09-23 23:04:55 PDT

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)

Comment 20 Kent Tamura 2009-09-23 23:09:21 PDT

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

Comment 21 Kent Tamura 2009-09-24 01:14:38 PDT

(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 22 Shinichiro Hamaji 2009-09-24 01:29:44 PDT

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

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

Comment 23 WebKit Commit Bot 2009-09-24 03:33:45 PDT

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)

Comment 24 Kent Tamura 2009-09-24 04:42:14 PDT

(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 25 Kent Tamura 2009-09-25 16:54:32 PDT

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

I confirmed patching, building, testing with r48775.

Comment 26 Eric Seidel (no email) 2009-09-25 17:08:49 PDT

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

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

Comment 27 WebKit Commit Bot 2009-09-25 18:54:05 PDT

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)

Comment 28 Eric Seidel (no email) 2009-09-25 19:00:36 PDT

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

Comment 29 Kent Tamura 2009-09-25 19:51:17 PDT

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

Comment 30 Kent Tamura 2009-09-25 20:01:48 PDT

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

Add UNUSED_PARAM(inputElement) to build Release.

Comment 31 Darin Adler 2009-09-25 21:01:56 PDT

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.

Comment 32 Kent Tamura 2009-09-25 22:35:13 PDT

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

ASSERT + UNUSED_PARAM -> ASSERT_UNUSED

Comment 33 Kent Tamura 2009-09-25 22:35:50 PDT

(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 34 David Kilzer (:ddkilzer) 2009-09-26 10:02:01 PDT

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>

Comment 35 Eric Seidel (no email) 2009-09-26 11:04:02 PDT

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

Comment 36 WebKit Commit Bot 2009-09-26 11:56:13 PDT

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

Clearing flags on attachment: 40162

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

Comment 37 WebKit Commit Bot 2009-09-26 11:56:19 PDT

All reviewed patches have been landed.  Closing bug.