Add autofill preview support for Textarea

Add autofill preview support for Textarea

BUG=314976
R=isherman@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=164683

[Ilya Sherman, 2013-12-29 03:15:07]

Thanks for the patch!  Adding tkent@ as a reviewer, as he is much more familiar
with this part of the codebase.

https://codereview.chromium.org/120443003/diff/1/Source/core/html/HTMLTextAre...
File Source/core/html/HTMLTextAreaElement.cpp (right):

https://codereview.chromium.org/120443003/diff/1/Source/core/html/HTMLTextAre...
Source/core/html/HTMLTextAreaElement.cpp:444:
setInnerTextValue(m_suggestedValue);
I'm wondering whether you need more of HTMLTextAreaElement::setValueCommon()
here.  In particular, why isn't updatePlaceholderVisibility(false); needed?

https://codereview.chromium.org/120443003/diff/1/Source/core/html/HTMLTextAre...
Source/core/html/HTMLTextAreaElement.cpp:449: 
nit: Please remove the second blank line -- there should only be a single blank
line between methods.

[ziran.sun, 2014-01-02 15:42:17]

Code updated as per comments. Please let me know if it's okay.

Many thanks

Ziran

[tkent, 2014-01-06 01:23:14]

Please add tests.  We should add textarea support to
Internals::setSuggestedValue, and test if HTMLTextAreaElement::value doesn't
updated by a suggested value, etc.

[ziran.sun, 2014-01-06 10:32:34]

On 2014/01/06 01:23:14, tkent wrote:
> Please add tests.  We should add textarea support to
> Internals::setSuggestedValue, and test if HTMLTextAreaElement::value doesn't
> updated by a suggested value, etc.

I take it's webkit_unit_tests that I need to run?

[ziran.sun, 2014-01-06 17:37:17]

On 2014/01/06 01:23:14, tkent wrote:
> Please add tests.  We should add textarea support to
> Internals::setSuggestedValue, and test if HTMLTextAreaElement::value doesn't
> updated by a suggested value, etc.

Test code added. Please review. Thanks very much!

[tkent, 2014-01-06 23:29:49]

https://codereview.chromium.org/120443003/diff/160001/Source/core/html/HTMLTe...
File Source/core/html/HTMLTextAreaElement.h (right):

https://codereview.chromium.org/120443003/diff/160001/Source/core/html/HTMLTe...
Source/core/html/HTMLTextAreaElement.h:52: // suggested values for autofill
nit: The first letter should be capital.
BTW, I don't think this comment is very helpful. We can omit it.

https://codereview.chromium.org/120443003/diff/160001/Source/core/html/HTMLTe...
Source/core/html/HTMLTextAreaElement.h:53: const String& suggestedValue() const;
Returning "const String&" here is dangerous. It should be "String".

https://codereview.chromium.org/120443003/diff/160001/Source/core/html/HTMLTe...
Source/core/html/HTMLTextAreaElement.h:132: 
nit: I don't think this blank line is needed.

https://codereview.chromium.org/120443003/diff/160001/Source/core/testing/Int...
File Source/core/testing/Internals.cpp (right):

https://codereview.chromium.org/120443003/diff/160001/Source/core/testing/Int...
Source/core/testing/Internals.cpp:1031: // FIXME: We should be using hasTagName
instead but Windows port doesn't link QualifiedNames properly.
What do you mean?  Why don't you use hasTagName(textareaTag) even though we use
hasTagName(inputTag) in the same function?

https://codereview.chromium.org/120443003/diff/160001/Source/core/testing/Int...
File Source/core/testing/Internals.h (right):

https://codereview.chromium.org/120443003/diff/160001/Source/core/testing/Int...
Source/core/testing/Internals.h:105: const AtomicString&
shadowPseudoId(Element*, ExceptionState&);
Please don't change unrelated code.

https://codereview.chromium.org/120443003/diff/160001/Source/web/WebTextAreaE...
File Source/web/WebTextAreaElement.cpp (right):

https://codereview.chromium.org/120443003/diff/160001/Source/web/WebTextAreaE...
Source/web/WebTextAreaElement.cpp:63: 
nit: Two blank lines are not needed. One blank line is enough.

[ziran.sun, 2014-01-07 17:18:25]

https://codereview.chromium.org/120443003/diff/160001/Source/core/html/HTMLTe...
File Source/core/html/HTMLTextAreaElement.h (right):

https://codereview.chromium.org/120443003/diff/160001/Source/core/html/HTMLTe...
Source/core/html/HTMLTextAreaElement.h:52: // suggested values for autofill
On 2014/01/06 23:29:49, tkent wrote:
> nit: The first letter should be capital.
> BTW, I don't think this comment is very helpful. We can omit it.

Done.

https://codereview.chromium.org/120443003/diff/160001/Source/core/html/HTMLTe...
Source/core/html/HTMLTextAreaElement.h:53: const String& suggestedValue() const;
On 2014/01/06 23:29:49, tkent wrote:
> Returning "const String&" here is dangerous. It should be "String".

Done. In HTMLInputElement.h (L151), it also uses "const String&", should it also
be changed to "String"?

https://codereview.chromium.org/120443003/diff/160001/Source/core/html/HTMLTe...
Source/core/html/HTMLTextAreaElement.h:132: 
On 2014/01/06 23:29:49, tkent wrote:
> nit: I don't think this blank line is needed.

Done.

https://codereview.chromium.org/120443003/diff/160001/Source/core/testing/Int...
File Source/core/testing/Internals.cpp (right):

https://codereview.chromium.org/120443003/diff/160001/Source/core/testing/Int...
Source/core/testing/Internals.cpp:1031: // FIXME: We should be using hasTagName
instead but Windows port doesn't link QualifiedNames properly.
On 2014/01/06 23:29:49, tkent wrote:
> What do you mean?  Why don't you use hasTagName(textareaTag) even though we
use
> hasTagName(inputTag) in the same function?

I noticed this comment in Internals::wasLastChangeUserEdit() in this file (Line
993). I wonder if it would cause issue in Windows if I use
"hasTagName(textareaTag)"?

https://codereview.chromium.org/120443003/diff/160001/Source/core/testing/Int...
File Source/core/testing/Internals.h (right):

https://codereview.chromium.org/120443003/diff/160001/Source/core/testing/Int...
Source/core/testing/Internals.h:105: const AtomicString&
shadowPseudoId(Element*, ExceptionState&);
On 2014/01/06 23:29:49, tkent wrote:
> Please don't change unrelated code.

This and the rest of similar changes are updated after pulling latest codebase -
not my changes :).

https://codereview.chromium.org/120443003/diff/160001/Source/web/WebTextAreaE...
File Source/web/WebTextAreaElement.cpp (right):

https://codereview.chromium.org/120443003/diff/160001/Source/web/WebTextAreaE...
Source/web/WebTextAreaElement.cpp:63: 
On 2014/01/06 23:29:49, tkent wrote:
> nit: Two blank lines are not needed. One blank line is enough.

Done.

[tkent, 2014-01-08 00:59:37]

https://codereview.chromium.org/120443003/diff/160001/Source/core/testing/Int...
File Source/core/testing/Internals.cpp (right):

https://codereview.chromium.org/120443003/diff/160001/Source/core/testing/Int...
Source/core/testing/Internals.cpp:1031: // FIXME: We should be using hasTagName
instead but Windows port doesn't link QualifiedNames properly.
On 2014/01/07 17:18:26, ziran.sun wrote:
> On 2014/01/06 23:29:49, tkent wrote:
> > What do you mean?  Why don't you use hasTagName(textareaTag) even though we
> use
> > hasTagName(inputTag) in the same function?
> 
> I noticed this comment in Internals::wasLastChangeUserEdit() in this file
(Line
> 993). I wonder if it would cause issue in Windows if I use
> "hasTagName(textareaTag)"?   

I don't think we have such problem now.  Probably Windows means "WebKit Apple
Windows port"?  Commit-queue can confirm Windows build.  Please use
hasTagName(textareaTag).

[ziran.sun, 2014-01-08 10:45:53]

https://codereview.chromium.org/120443003/diff/160001/Source/core/testing/Int...
File Source/core/testing/Internals.cpp (right):

https://codereview.chromium.org/120443003/diff/160001/Source/core/testing/Int...
Source/core/testing/Internals.cpp:1031: // FIXME: We should be using hasTagName
instead but Windows port doesn't link QualifiedNames properly.
On 2014/01/08 00:59:38, tkent wrote:
> On 2014/01/07 17:18:26, ziran.sun wrote:
> > On 2014/01/06 23:29:49, tkent wrote:
> > > What do you mean?  Why don't you use hasTagName(textareaTag) even though
we
> > use
> > > hasTagName(inputTag) in the same function?
> > 
> > I noticed this comment in Internals::wasLastChangeUserEdit() in this file
> (Line
> > 993). I wonder if it would cause issue in Windows if I use
> > "hasTagName(textareaTag)"?   
> 
> I don't think we have such problem now.  Probably Windows means "WebKit Apple
> Windows port"?  Commit-queue can confirm Windows build.  Please use
> hasTagName(textareaTag).
> 

Done.

[tkent, 2014-01-08 12:33:06]

lgtm

[commit-bot: I haz the power, 2014-01-08 12:33:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ziran.sun@samsung.com/120443003/380001

[commit-bot: I haz the power, 2014-01-08 13:37:11]

Change committed as 164683