Bug 51809 – [Chromium] Autofill should work with HTML5 form elements

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Naoki Takano]

Created attachment 77791 [details]
first pach but does not include ChangeLog

Add email, month and telephone input type to enable setSuggestedValue().

[Ilya Sherman]

https://bugs.webkit.org/show_bug.cgi?id=51791 eliminates the deprecatedInputType() function, so this will need to be updated correspondingly.

[Naoki Takano]

Created attachment 78208 [details]
Second patch according to WebKit change.

While waiting for review, the patch becomes obsolete.
So I made new one.

[Ilya Sherman]

Comment on attachment 78208 [details]
Second patch according to WebKit change.

I am not a reviewer, but this looks good to me.

[Eric Seidel (no email)]

Comment on attachment 78208 [details]
Second patch according to WebKit change.

Is this testable?

[Naoki Takano]

I think it's hard to test only in WebKit.
With chrome, I wrote the test, but the review is on going.

[Naoki Takano]

Is there anybody who can review my patch officially?

[Tony Chang]

Comment on attachment 78208 [details]
Second patch according to WebKit change.

This patch is missing a ChangeLog.

[Naoki Takano]

Ok, I'll try to make it.

[Kent Tamura]

Comment on attachment 78208 [details]
Second patch according to WebKit change.

Why you enable suggestion for email, month, tel, and not color, time, date, datetime, datetime-local, time, url, week?
IMO we should change InputType::canSetSuggestedValue() so that it just returns isTextField().

[Naoki Takano]

Kent,

Thank you for your comment.

(In reply to comment #9)
> (From update of attachment 78208 [details])
> Why you enable suggestion for email, month, tel, and not color, time, date, datetime, datetime-local, time, url, week?
> IMO we should change InputType::canSetSuggestedValue() so that it just returns isTextField().
I see, so do you mean to implement returning TexField() as default implementation?
And should we include all text input that could be suggested, not only email, month and tel?

Not only this change, but we might need to implement to pass  dom/InputElement::toInputElement() function as API to chrome.
See http://codereview.chromium.org/6033010/ discussion.

Thanks,

[Naoki Takano]

Kent,

Could you response my question?

Thanks,

[Kent Tamura]

(In reply to comment #10)
> > IMO we should change InputType::canSetSuggestedValue() so that it just returns isTextField().
> I see, so do you mean to implement returning TexField() as default implementation?

I have changed my mind about this point.  isTextField() may be wrong.  canSetSuggestedValue() should be removed.

Suggestion/Autofill feature depends on browser implementations. Supported input types also can depend on browsers.  We should not have Chromium-dependent behavior in WebCore/html/.

> And should we include all text input that could be suggested, not only email, month and tel?

I don't know.
Autofill-team, what do you think?

[Naoki Takano]

(In reply to comment #12)
> (In reply to comment #10)
> > > IMO we should change InputType::canSetSuggestedValue() so that it just returns isTextField().
> > I see, so do you mean to implement returning TexField() as default implementation?
> 
> I have changed my mind about this point.  isTextField() may be wrong.  canSetSuggestedValue() should be removed.
> 
> Suggestion/Autofill feature depends on browser implementations. Supported input types also can depend on browsers.  We should not have Chromium-dependent behavior in WebCore/html/.
I see, but, right now, canSetSuggestedValue() reject month, tel and email as you know.
So should I delete canSetSuggestedValue() check itself?

> > And should we include all text input that could be suggested, not only email, month and tel?
> 
> I don't know.
> Autofill-team, what do you think?
Ilya, do you have any idea?

[Ilya Sherman]

(In reply to comment #12)
> (In reply to comment #10)
> > > IMO we should change InputType::canSetSuggestedValue() so that it just returns isTextField().
> > I see, so do you mean to implement returning TexField() as default implementation?
> 
> I have changed my mind about this point.  isTextField() may be wrong.  canSetSuggestedValue() should be removed.
> 
> Suggestion/Autofill feature depends on browser implementations. Supported input types also can depend on browsers.  We should not have Chromium-dependent behavior in WebCore/html/.

I think canSetSuggestedValue() answers a question that is relevant to the rendering engine, not the browser: Does this element have a way to display a suggested value (one that is not accessible to JavaScript but viewable by the user)?  The answer to this question should be independent of whether specific browsers ever actually set suggested values.

Also, I believe that both Chromium and Safari use this functionality, so this isn't Chromium-specific behavior ;)

> > And should we include all text input that could be suggested, not only email, month and tel?
> 
> I don't know.
> Autofill-team, what do you think?

If displaying the suggested value requires no extra code on the WebKit side, I think it's reasonable to allow all text inputs to have suggested values.  I would expect this to be true of all text input types, though I could easily be overlooking a reason why that wouldn't be the case.

[David Holloway]

I agree with what Ilya said.

[Naoki Takano]

Ok, So I will change canSetSuggestedValue() to return TexField() as default implementation.

(In reply to comment #15)
> I agree with what Ilya said.

[Naoki Takano]

Created attachment 79555 [details]
Add toWebInputElement().

Added toWebInputElement() to support safe cast from WebNode to WebInputElement.

[Naoki Takano]

Darin,

Could you review my patch related to WebInputElement()?

[WebKit Review Bot]

Attachment 79555 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebKit/chromium/src/WebInputElement.cpp:163:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Source/WebKit/chromium/src/WebInputElement.cpp:165:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebKit/chromium/src/WebInputElement.cpp:167:  Local variables should never be PassRefPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
Source/WebKit/chromium/src/WebInputElement.cpp:169:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebKit/chromium/src/WebInputElement.cpp:171:  input_element is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit/chromium/src/WebInputElement.cpp:173:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebKit/chromium/src/WebInputElement.cpp:178:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Source/WebKit/chromium/src/WebInputElement.cpp:180:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebKit/chromium/src/WebInputElement.cpp:182:  Local variables should never be PassRefPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
Source/WebKit/chromium/src/WebInputElement.cpp:184:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebKit/chromium/src/WebInputElement.cpp:186:  input_element is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit/chromium/src/WebInputElement.cpp:188:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 12 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ilya Sherman]

Comment on attachment 79555 [details]
Add toWebInputElement().

View in context: https://bugs.webkit.org/attachment.cgi?id=79555&action=review

> Source/WebCore/html/InputType.cpp:470
>  bool InputType::canSetSuggestedValue()
>  {
> -    return false;
> +    return isTextField();
>  }

You should probably also remove the override for canSetSuggestedValue() in TextInputType.{cpp,h}

> WebKit/chromium/ChangeLog:1
> [phone]  Naoki Takano  <[e-mail]>

This file now lives at Source/WebKit/chromium/ChangeLog -- you'll need to merge with head.

> WebKit/chromium/public/WebInputElement.h:84
> +        WEBKIT_API static const WebInputElement* toWebInputElement(const WebNode*);
> +        WEBKIT_API static WebInputElement* toWebInputElement(WebNode*);

I think it might be better for these functions to more closely parallel toInputElement and take a WebElement parameter rather than a WebNode parameter.

> WebKit/chromium/src/WebInputElement.cpp:163
> +const WebInputElement* WebInputElement::toWebInputElement(const WebNode* webNode) {

If we want a const version of toWebInputElement, we should probably also add a const version of toInputElement (unless other reviewers think that's a bad idea).

> WebKit/chromium/src/WebInputElement.cpp:168
> +    if (!node->isElementNode())

You can just call webNode->isElementNode() here (or drop this if-stmt entirely if you change the parameter to be a WebElement).

> WebKit/chromium/src/WebInputElement.cpp:171
> +    InputElement* input_element = toInputElement(static_cast<Element*>(node.get()));

You can then call constUnwrap<Element>(webNode) here.

> WebKit/chromium/src/WebInputElement.cpp:173
> +    if (!input_element)
> +        return NULL;

I think you also want "&& input_element->isHTMLElement()" here.

> WebKit/chromium/src/WebInputElement.cpp:190
> +    if (!webNode)
> +        return NULL;
> +
> +    PassRefPtr<Node> node = *webNode;
> +    if (!node->isElementNode())
> +        return NULL;
> +
> +    InputElement* input_element = toInputElement(static_cast<Element*>(node.get()));
> +    if (!input_element)
> +        return NULL;
> +
> +    return static_cast<WebInputElement*>(webNode);

It might be nice to implement this as

return const_cast<WebInputElement*>(toWebInputElement(static_cast<const WebNode*>(webNode)));

[Kent Tamura]

Comment on attachment 79555 [details]
Add toWebInputElement().

View in context: https://bugs.webkit.org/attachment.cgi?id=79555&action=review

> Source/WebCore/html/InputType.cpp:469
>  {
> -    return false;
> +    return isTextField();

Please add a comment about what type should return true.

InputType::canSetSuggestedValue() should return false, and TextFieldInputType::canSetSuggestedValue() should return true. isTextField() is a virtual function and it costs a little bit.

> WebKit/chromium/ChangeLog:13
> +        [Chromium] Autofill should work with HTML5 form elements
> +        https://bugs.webkit.org/show_bug.cgi?id=51809
> +        http://crbug.com/65654
> +
> +        No new tests, because this fix is for Chromium project and hard to test only in WebKit project.
> +
> +        * public/WebInputElement.h: Added toWebInputElement() declarations.
> +        * src/WebInputElement.cpp:
> +        (WebKit::WebInputElement::toWebInputElement): Implemented two convert functions to cast (const or non-const) WebNode* to (const or non-const) WebInputElement*.

Please explain why we need toWebInputElement().

> WebKit/chromium/src/WebInputElement.cpp:167
> +    PassRefPtr<Node> node = *webNode;

We don't need to use PassRefPtr<>.

[Naoki Takano]

Created attachment 79694 [details]
Change return values and removed const version of toWebInputElement().

Please review it.

[WebKit Review Bot]

Attachment 79694 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebKit/chromium/src/WebInputElement.cpp:163:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Source/WebKit/chromium/src/WebInputElement.cpp:164:  input_element is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit/chromium/src/WebInputElement.cpp:166:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebKit/chromium/src/WebInputElement.cpp:168:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 4 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Naoki Takano]

Thanks, Ilya.

I already uploaded a patch, but I wrote some comment.

(In reply to comment #20)
> (From update of attachment 79555 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=79555&action=review
> 
> > Source/WebCore/html/InputType.cpp:470
> >  bool InputType::canSetSuggestedValue()
> >  {
> > -    return false;
> > +    return isTextField();
> >  }
> 
> You should probably also remove the override for canSetSuggestedValue() in TextInputType.{cpp,h}
done.

> > WebKit/chromium/ChangeLog:1
> > [phone]  Naoki Takano  <[e-mail]>
> 
> This file now lives at Source/WebKit/chromium/ChangeLog -- you'll need to merge with head.
> 
> > WebKit/chromium/public/WebInputElement.h:84
> > +        WEBKIT_API static const WebInputElement* toWebInputElement(const WebNode*);
> > +        WEBKIT_API static WebInputElement* toWebInputElement(WebNode*);
> 
> I think it might be better for these functions to more closely parallel toInputElement and take a WebElement parameter rather than a WebNode parameter.
done.

> > WebKit/chromium/src/WebInputElement.cpp:163
> > +const WebInputElement* WebInputElement::toWebInputElement(const WebNode* webNode) {
> 
> If we want a const version of toWebInputElement, we should probably also add a const version of toInputElement (unless other reviewers think that's a bad idea).
Ok, actually in the new patch, I deleted const version, and I used const_cast<> in Chrome source.

> > WebKit/chromium/src/WebInputElement.cpp:168
> > +    if (!node->isElementNode())
> 
> You can just call webNode->isElementNode() here (or drop this if-stmt entirely if you change the parameter to be a WebElement).
done.

> > WebKit/chromium/src/WebInputElement.cpp:171
> > +    InputElement* input_element = toInputElement(static_cast<Element*>(node.get()));
> 
> You can then call constUnwrap<Element>(webNode) here.
done.

> > WebKit/chromium/src/WebInputElement.cpp:173
> > +    if (!input_element)
> > +        return NULL;
> 
> I think you also want "&& input_element->isHTMLElement()" here.
I cannot understand why I had to add  "&& input_element->isHTMLElement()" .
Isn't "if (!input_element && input_element->isHTMLElement())" strange?

> > WebKit/chromium/src/WebInputElement.cpp:190
> > +    if (!webNode)
> > +        return NULL;
> > +
> > +    PassRefPtr<Node> node = *webNode;
> > +    if (!node->isElementNode())
> > +        return NULL;
> > +
> > +    InputElement* input_element = toInputElement(static_cast<Element*>(node.get()));
> > +    if (!input_element)
> > +        return NULL;
> > +
> > +    return static_cast<WebInputElement*>(webNode);
> 
> It might be nice to implement this as
> 
> return const_cast<WebInputElement*>(toWebInputElement(static_cast<const WebNode*>(webNode)));
I deleted const version, so not needed.

[Naoki Takano]

Created attachment 79698 [details]
Fix format errors.

Oops, fix format errors.

[Naoki Takano]

Thanks, Kent.

I added comment and changed ChangeLog.

Please review it.

Thanks,

[Kent Tamura]

Comment on attachment 79698 [details]
Fix format errors.

View in context: https://bugs.webkit.org/attachment.cgi?id=79698&action=review

> Source/WebCore/html/TextFieldInputType.cpp:57
> +    // All text field input like email, month, tel, and not color, time, date, datetime, datetime-local, time, url, week can be completed.

Please remove the comment.  This helps nothing.

I meant we should have a comment on InputType::canSetSuggestedValue() (not TextFieldInputType::canSetSuggetedValue()) so that one can understand how to decide whether an input type should return true for canSetSuggestedValue().  The answer is Ilya's comment:

> I think canSetSuggestedValue() answers a question that is relevant to the rendering engine, not the browser: Does this element have a way to display a suggested value (one that is not accessible to JavaScript but viewable by the user)?  The answer to this question should be independent of whether specific browsers ever actually set suggested values.

The comment would be...
    // Should return true if the corresponding renderer for a type can display a suggested value.

> Source/WebKit/chromium/src/WebInputElement.cpp:169
> +        return 0;
> +
> +    return static_cast<WebInputElement*>(webElement);

We had better have
  ASSERT(inputElement->isHTMLElement());
here.

InputElement can be one of HTMLInputElement or WMLInputElement, and Chromium doesn't support WML for now.

[Kent Tamura]

Comment on attachment 79555 [details]
Add toWebInputElement().

View in context: https://bugs.webkit.org/attachment.cgi?id=79555&action=review

>>> WebKit/chromium/src/WebInputElement.cpp:163
>>> +const WebInputElement* WebInputElement::toWebInputElement(const WebNode* webNode) {
>> 
>> If we want a const version of toWebInputElement, we should probably also add a const version of toInputElement (unless other reviewers think that's a bad idea).
> 
> Ok, actually in the new patch, I deleted const version, and I used const_cast<> in Chrome source.

const_cast<> in Chrome sounds worse.  We had better introducing toWebInputElement const version.

>>> WebKit/chromium/src/WebInputElement.cpp:173
>>> +        return NULL;
>> 
>> I think you also want "&& input_element->isHTMLElement()" here.
> 
> I cannot understand why I had to add  "&& input_element->isHTMLElement()" .
> Isn't "if (!input_element && input_element->isHTMLElement())" strange?

It's not strange.  But I prefer ASSERT() as I wrote in another comment.

[Naoki Takano]

(In reply to comment #28)
> (From update of attachment 79555 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=79555&action=review
> 
> >>> WebKit/chromium/src/WebInputElement.cpp:163
> >>> +const WebInputElement* WebInputElement::toWebInputElement(const WebNode* webNode) {
> >> 
> >> If we want a const version of toWebInputElement, we should probably also add a const version of toInputElement (unless other reviewers think that's a bad idea).
> > 
> > Ok, actually in the new patch, I deleted const version, and I used const_cast<> in Chrome source.
> 
> const_cast<> in Chrome sounds worse.  We had better introducing toWebInputElement const version.
Yes, I thought so.

But const_cast<> is already in use in chrome/renderer/form_manager.cc where we want to use toWebInputElement().

What do you think?

> >>> WebKit/chromium/src/WebInputElement.cpp:173
> >>> +        return NULL;
> >> 
> >> I think you also want "&& input_element->isHTMLElement()" here.
> > 
> > I cannot understand why I had to add  "&& input_element->isHTMLElement()" .
> > Isn't "if (!input_element && input_element->isHTMLElement())" strange?
> 
> It's not strange.  But I prefer ASSERT() as I wrote in another comment.
Really?

if input_element == NULL, "if (!input_element && input_element->isHTMLElement())" throws an exception, right?

Or it should be 
if (!input_element)
    return NULL;
if (!input_elementisHTMLElement())
    return NULL;
Isn't it?

But I agree we should add ASSERT(input_element->isHTMLElement()) here.

[Kent Tamura]

Comment on attachment 79555 [details]
Add toWebInputElement().

View in context: https://bugs.webkit.org/attachment.cgi?id=79555&action=review

>>>>> WebKit/chromium/src/WebInputElement.cpp:163
>>>>> +const WebInputElement* WebInputElement::toWebInputElement(const WebNode* webNode) {
>>>> 
>>>> If we want a const version of toWebInputElement, we should probably also add a const version of toInputElement (unless other reviewers think that's a bad idea).
>>> 
>>> Ok, actually in the new patch, I deleted const version, and I used const_cast<> in Chrome source.
>> 
>> const_cast<> in Chrome sounds worse.  We had better introducing toWebInputElement const version.
> 
> Yes, I thought so.
> 
> But const_cast<> is already in use in chrome/renderer/form_manager.cc where we want to use toWebInputElement().
> 
> What do you think?

We should eliminate const_cast<> in form_manager.cc later.

>>>>> WebKit/chromium/src/WebInputElement.cpp:173
>>>>> +        return NULL;
>>>> 
>>>> I think you also want "&& input_element->isHTMLElement()" here.
>>> 
>>> I cannot understand why I had to add  "&& input_element->isHTMLElement()" .
>>> Isn't "if (!input_element && input_element->isHTMLElement())" strange?
>> 
>> It's not strange.  But I prefer ASSERT() as I wrote in another comment.
> 
> Really?
> 
> if input_element == NULL, "if (!input_element && input_element->isHTMLElement())" throws an exception, right?
> 
> Or it should be 
> if (!input_element)
>     return NULL;
> if (!input_elementisHTMLElement())
>     return NULL;
> Isn't it?
> 
> But I agree we should add ASSERT(input_element->isHTMLElement()) here.

Ah, right.
Ilya should have wanted to write
  || input_element->isHTMLElement()
:-)

[Naoki Takano]

(In reply to comment #30)
> (From update of attachment 79555 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=79555&action=review

> We should eliminate const_cast<> in form_manager.cc later.
Ok, that makes sense.
I will add const version.

> Ah, right.
> Ilya should have wanted to write
>   || input_element->isHTMLElement()
> :-)
I'm glad to here that;-)

[Naoki Takano]

Created attachment 79704 [details]
Added const version of toInputElement().

Added const version.

Please review it.

[Kent Tamura]

(In reply to comment #27)
> Please remove the comment.  This helps nothing.
> 
> I meant we should have a comment on InputType::canSetSuggestedValue() (not TextFieldInputType::canSetSuggetedValue()) so that one can understand how to decide whether an input type should return true for canSetSuggestedValue().  The answer is Ilya's comment:
> 
> > I think canSetSuggestedValue() answers a question that is relevant to the rendering engine, not the browser: Does this element have a way to display a suggested value (one that is not accessible to JavaScript but viewable by the user)?  The answer to this question should be independent of whether specific browsers ever actually set suggested values.
> 
> The comment would be...
>     // Should return true if the corresponding renderer for a type can display a suggested value.

Takano-san, you missed this comment?

[Eric Seidel (no email)]

Comment on attachment 79704 [details]
Added const version of toInputElement().

View in context: https://bugs.webkit.org/attachment.cgi?id=79704&action=review

> Source/WebCore/dom/InputElement.cpp:292
> +    return const_cast<InputElement*>(toInputElement(static_cast<const Element*>(element)));

Why all the const casting?

> Source/WebCore/dom/InputElement.cpp:295
> +const InputElement* toInputElement(const Element* element)

We don't ever have const pointers for Elements.  They don't make sense.  The dom is always mutable (from refcounting if nothing else.  Why is this needed?

> Source/WebKit/chromium/src/WebInputElement.cpp:163
> +const WebInputElement* WebInputElement::toWebInputElement(const WebElement* webElement)

Again, const makes no sense here.  At least for WebCore classes.

[Naoki Takano]

Eric,

Thank you for your review.
But the reason why I added const version of toInputElement() is what Ilya suggested me.

As I wrote here, Chrome project requires const version of toWebInputElenet() and he suggested me as following,

>> WebKit/chromium/src/WebInputElement.cpp:163
>> +const WebInputElement* WebInputElement::toWebInputElement(const WebNode* webNode) {
>
>If we want a const version of toWebInputElement, we should probably also add a const version of toInputElement (unless other reviewers think that's a bad idea).

That's why I added const version.

> > Source/WebCore/dom/InputElement.cpp:292
> > +    return const_cast<InputElement*>(toInputElement(static_cast<const Element*>(element)));
> 
> Why all the const casting?
Also this way how he suggested as following,

> It might be nice to implement this as
>
> return const_cast<WebInputElement*>(toWebInputElement(static_cast<const WebNode*>(webNode)));
So I mimic it for toInputElement().

I don't care whether we should add const version of toInputElement() and I also understand we don't need it in dom area, but I want to have consistent reviewer's comment.

Could you give me your thought, Ilya?

If you are Ok, I'll delete const version of toInputElement() and remain toWebInputElement().

Thanks,

(In reply to comment #34)
> (From update of attachment 79704 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=79704&action=review
> 
> > Source/WebCore/dom/InputElement.cpp:292
> > +    return const_cast<InputElement*>(toInputElement(static_cast<const Element*>(element)));
> 
> Why all the const casting?
> 
> > Source/WebCore/dom/InputElement.cpp:295
> > +const InputElement* toInputElement(const Element* element)
> 
> We don't ever have const pointers for Elements.  They don't make sense.  The dom is always mutable (from refcounting if nothing else.  Why is this needed?
> 
> > Source/WebKit/chromium/src/WebInputElement.cpp:163
> > +const WebInputElement* WebInputElement::toWebInputElement(const WebElement* webElement)
> 
> Again, const makes no sense here.  At least for WebCore classes.

[Naoki Takano]

(In reply to comment #33)
> (In reply to comment #27)
> > Please remove the comment.  This helps nothing.
> > 
> > I meant we should have a comment on InputType::canSetSuggestedValue() (not TextFieldInputType::canSetSuggetedValue()) so that one can understand how to decide whether an input type should return true for canSetSuggestedValue().  The answer is Ilya's comment:
> > 
> > > I think canSetSuggestedValue() answers a question that is relevant to the rendering engine, not the browser: Does this element have a way to display a suggested value (one that is not accessible to JavaScript but viewable by the user)?  The answer to this question should be independent of whether specific browsers ever actually set suggested values.
> > 
> > The comment would be...
> >     // Should return true if the corresponding renderer for a type can display a suggested value.
> 
> Takano-san, you missed this comment?
Oops, I'll add it...

Thanks,

[Ilya Sherman]

(In reply to comment #30)
> Ilya should have wanted to write
>   || input_element->isHTMLElement()
> :-)

Yeah, that was a typo -- my bad :)


(In reply to comment #35)
> >> WebKit/chromium/src/WebInputElement.cpp:163
> >> +const WebInputElement* WebInputElement::toWebInputElement(const WebNode* webNode) {
> >
> >If we want a const version of toWebInputElement, we should probably also add a const version of toInputElement (unless other reviewers think that's a bad idea).

I'll happily defer to Eric, who knows the WebKit code far better than I do.

[Naoki Takano]

Created attachment 79858 [details]
Removed toInputElement() const version.

Please review it.

[Eric Seidel (no email)]

Comment on attachment 79858 [details]
Removed toInputElement() const version.

View in context: https://bugs.webkit.org/attachment.cgi?id=79858&action=review

> Source/WebCore/ChangeLog:9
> +        No new tests, because this fix is for Chromium project and hard to test only in WebKit project.

Why is this on the WebCore classes if this information is only used by Chromium?  Why wouldn't this information be carried on the Web* chromium API wrappers if this  is chromium-only information?

> Source/WebKit/chromium/src/WebInputElement.cpp:165
> +    const InputElement* inputElement = toInputElement(const_cast<Element*>(webElement->constUnwrap<Element>()));

WebElement should have a non-const WebCore::Element* accessor which does this const cast for you.  That doesn't have to be part of this change, but again, const Element* makes no sense in WebCore, so it makes little sense for the Chromium webkit API to expose such. :)

> Source/WebKit/chromium/src/WebInputElement.cpp:169
> +    ASSERT(inputElement->isHTMLElement());

Why is this ASSERT valid?  Do we have non-HTML input elements?  Are those only WML types?

[Ilya Sherman]

Comment on attachment 79858 [details]
Removed toInputElement() const version.

View in context: https://bugs.webkit.org/attachment.cgi?id=79858&action=review

>> Source/WebCore/ChangeLog:9
>> +        No new tests, because this fix is for Chromium project and hard to test only in WebKit project.
> 
> Why is this on the WebCore classes if this information is only used by Chromium?  Why wouldn't this information be carried on the Web* chromium API wrappers if this  is chromium-only information?

I'm pretty sure that canSetSuggestedValue() is used by Safari's autofill implementation as well as for Chromium's.  However, it's not used by WebKit directly, which is a large part of what makes it hard to test purely in WebKit.

AFAIK the WebKit layout test framework doesn't have any support for testing autofill-related functions, so there are currently no tests for any of this code.  If adding test support for this is a simple change (pointers welcome), we should probably add that to this patch.  Otherwise, I think it might be better to add proper testing for all the WebCore autofill functions in a separate patch.  I'd be happy to take that on if someone who knows what they're doing could volunteer to mentor me a bit =)

>> Source/WebKit/chromium/src/WebInputElement.cpp:169
>> +    ASSERT(inputElement->isHTMLElement());
> 
> Why is this ASSERT valid?  Do we have non-HTML input elements?  Are those only WML types?

Yeah, toInputElement() returns either an HTML input element or a WML input element, but we expect to never see WML types here.

[Naoki Takano]

Eric,

Thank you for your comment.

> > Source/WebKit/chromium/src/WebInputElement.cpp:165
> > +    const InputElement* inputElement = toInputElement(const_cast<Element*>(webElement->constUnwrap<Element>()));
> 
> WebElement should have a non-const WebCore::Element* accessor which does this const cast for you.  That doesn't have to be part of this change, but again, const Element* makes no sense in WebCore, so it makes little sense for the Chromium webkit API to expose such. :)
Ok,

But, as I already wrote here

>>>> +const WebInputElement* WebInputElement::toWebInputElement(const WebNode* webNode) {
>>> 
>>> If we want a const version of toWebInputElement, we should probably also add a const version of toInputElement (unless other reviewers think that's a bad idea).
>> 
>> Ok, actually in the new patch, I deleted const version, and I used const_cast<> in Chrome source.

Kent replies as following,
>const_cast<> in Chrome sounds worse.  We had better introducing toWebInputElement const version.

Again, of course, we should not use const_cast<>, but I really need const_cast<> at WebKit or Chrome, we have to choose. I'm not sure about the const_cast<> policy between them, but could you please decide, Kent and Eric?

[Kent Tamura]

Comment on attachment 79858 [details]
Removed toInputElement() const version.

View in context: https://bugs.webkit.org/attachment.cgi?id=79858&action=review

> Source/WebCore/html/TextFieldInputType.cpp:57
> +    // Should return true if the corresponding renderer for a type can display a suggested value.

Please move this comment to InputType::canSetSuggestedValue().  This comment would be a guide for one who implements canSetSuggestedValue() for other types.

[Naoki Takano]

Sorry for my misunderstanding... I'll revise it.

How about const_cast<> problem.
Could you give me your thought?

Thanks,

(In reply to comment #42)
> (From update of attachment 79858 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=79858&action=review
> 
> > Source/WebCore/html/TextFieldInputType.cpp:57
> > +    // Should return true if the corresponding renderer for a type can display a suggested value.
> 
> Please move this comment to InputType::canSetSuggestedValue().  This comment would be a guide for one who implements canSetSuggestedValue() for other types.

[Kent Tamura]

(In reply to comment #43)
> How about const_cast<> problem.
> Could you give me your thought?

My opinion is that the latest patch is ok about const_cast<>.

[Naoki Takano]

Ok,

If so, could you convince Eric that we really need const version of toWebInputElement()?

(In reply to comment #44)
> (In reply to comment #43)
> > How about const_cast<> problem.
> > Could you give me your thought?
> 
> My opinion is that the latest patch is ok about const_cast<>.

[Eric Seidel (no email)]

I don't need to be convinced about anything in WebKIt.  That's out of my domain of knowledge.  Darin Fisher is the only one to convince there.

I'm was just saying contst makes no sense in WebCore (and generally no sense with ref-counted objects).

[Naoki Takano]

Created attachment 80010 [details]
Change comment.

Please review it.

I want to finalize WebKit side patch.

[Kent Tamura]

(In reply to comment #47)
> Created an attachment (id=80010) [details]

I'm ok with the patch.
However, we need an approval by Darin Fisher for changes to WebKit/chromium/public/.  Please wait for his comment.

[Naoki Takano]

Ok,

So how can I contact him?
I believe his e-mail already is included in the e-mail list.

> Darin,
If you read this entry, could you review the last patch, especially for const_cast<>?

Thanks,

(In reply to comment #48)
> (In reply to comment #47)
> > Created an attachment (id=80010) [details] [details]
> 
> I'm ok with the patch.
> However, we need an approval by Darin Fisher for changes to WebKit/chromium/public/.  Please wait for his comment.

[Darin Fisher (:fishd, Google)]

Comment on attachment 80010 [details]
Change comment.

View in context: https://bugs.webkit.org/attachment.cgi?id=80010&action=review

Sorry for the review latency on my part...

> Source/WebCore/html/InputType.cpp:482
> +    // Should return true if the corresponding renderer for a type can display a suggested value.

I can't tell if this comment is meant to be a FIXME, or if it is really intended to be
in the header file.  It seems more like the latter.

> Source/WebKit/chromium/public/WebInputElement.h:83
> +        WEBKIT_API static WebInputElement* toWebInputElement(WebElement*);

Just like the WebCore equivalent, I think this should be a global function instead of
a class member function.

Also, I find it odd that we only have this kind of conversion function for WebInputEvent.
I think we should have this as a convention for all subclasses of WebNode, and we should
fixup existing Chromium code to make use of these "downcast" functions.

[Naoki Takano]

Darin,

Yeah... I should ask you first more precisely...

Before I revise again, I want to make sure.

> > Source/WebCore/html/InputType.cpp:482
> > +    // Should return true if the corresponding renderer for a type can display a suggested value.
> 
> I can't tell if this comment is meant to be a FIXME, or if it is really intended to be
> in the header file.  It seems more like the latter.
So, I have to move this comment to header file. Is this correct?

> > Source/WebKit/chromium/public/WebInputElement.h:83
> > +        WEBKIT_API static WebInputElement* toWebInputElement(WebElement*);
> 
> Just like the WebCore equivalent, I think this should be a global function instead of
> a class member function.
> 
> Also, I find it odd that we only have this kind of conversion function for WebInputEvent.
> I think we should have this as a convention for all subclasses of WebNode, and we should
> fixup existing Chromium code to make use of these "downcast" functions.
Ok, I got your point.

I have to move 
> > +        WEBKIT_API static WebInputElement* toWebInputElement(WebElement*);
to global function.

But do we have to do all "downcast" fixup right now?
I think it's a huge change for chromium source code change.

I want to suggest once we close this bug, and after that, should we fixup existing code?
We have to enumerate the whole subclasses of WebNode first, right?

So for this bug, I should
1, move the comment to header file.
2, move the declaration to global.
Are they OK?

BTW, what do you think of const_cast<> problem. Could you give us your thought?

Thanks,

[Darin Fisher (:fishd, Google)]

(In reply to comment #51)
> But do we have to do all "downcast" fixup right now?
> I think it's a huge change for chromium source code change.
> 
> I want to suggest once we close this bug, and after that, should we fixup existing code?
> We have to enumerate the whole subclasses of WebNode first, right?
> 
> So for this bug, I should
> 1, move the comment to header file.
> 2, move the declaration to global.
> Are they OK?

Yes, I like this plan.  I did not mean that you should include all of this fixup in this change.  Sorry, for not being clear.  I just want to see a commitment to keep our code consistent.


> BTW, what do you think of const_cast<> problem. Could you give us your thought?

Having two versions of each downcast function seems fine.  Avoiding the need to const_cast in Chromium code is helpful.  I'd probably standardize on just having the non-const version as the DLL entry point w/ the const version provided inline:

WEBKIT_API WebInputElement* toWebInputElement(WebElement*);

inline const WebInputElement* toWebInputElement(const WebElement* element)
{
    return toWebInputElement(const_cast<WebElement*>(element));
}

[Ilya Sherman]

(In reply to comment #52)
>  I'd probably standardize on just having the non-const version as the DLL entry point w/ the const version provided inline:
> 
> WEBKIT_API WebInputElement* toWebInputElement(WebElement*);
> 
> inline const WebInputElement* toWebInputElement(const WebElement* element)
> {
>     return toWebInputElement(const_cast<WebElement*>(element));
> }


This implementation is semantically odd -- it's assuming that the non-const version of toWebInputElement will always preserve logical constness of the parameter, without any help from the compiler for future changes.  For this reason, I'd prefer the const version as the DLL entry point, but I'll leave it to your discretion.

[Naoki Takano]

Created attachment 80538 [details]
Change comment and const version.

This entry needs my perseverance;-)

I wish this is the last attempt...

Please review it.

[Darin Fisher (:fishd, Google)]

(In reply to comment #53)
> (In reply to comment #52)
> >  I'd probably standardize on just having the non-const version as the DLL entry point w/ the const version provided inline:
> > 
> > WEBKIT_API WebInputElement* toWebInputElement(WebElement*);
> > 
> > inline const WebInputElement* toWebInputElement(const WebElement* element)
> > {
> >     return toWebInputElement(const_cast<WebElement*>(element));
> > }
> 
> 
> This implementation is semantically odd -- it's assuming that the non-const version of toWebInputElement will always preserve logical constness of the parameter, without any help from the compiler for future changes.  For this reason, I'd prefer the const version as the DLL entry point, but I'll leave it to your discretion.

That's a fine point.  However, my suggestion was based on the fact that the corresponding WebCore equivalent is non-const, so writing the implementation that calls the WebCore equivalent will not require a const_cast.

[Darin Fisher (:fishd, Google)]

Comment on attachment 80538 [details]
Change comment and const version.

View in context: https://bugs.webkit.org/attachment.cgi?id=80538&action=review

R=me, but CQ- because of the indentation nit.

> Source/WebKit/chromium/public/WebInputElement.h:96
> +          return toWebInputElement(const_cast<WebElement*>(element));

nit: indentation should be 4 spaces

> Source/WebKit/chromium/src/WebInputElement.cpp:182
> +    ASSERT(inputElement->isHTMLElement());

nit: I'm not so sure if this assertion is appropriate, but since we don't
make a big distinction between HTML and non-HTML documents in the WebKit
API, I guess it is OK here until we find that we trip over it.

[Naoki Takano]

Created attachment 80542 [details]
Change indent.

Please review again.

[Eric Seidel (no email)]

Comment on attachment 80538 [details]
Change comment and const version.

Cleared Darin Fisher's review+ from obsolete attachment 80538 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[WebKit Commit Bot]

Comment on attachment 80542 [details]
Change indent.

Clearing flags on attachment: 80542

Committed r77228: <http://trac.webkit.org/changeset/77228>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Yury Semikhatsky]

http://trac.webkit.org/changeset/77228 broke Chromium Debug compilation: 
http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Linux%20Builder%20(dbg)/builds/1186/steps/compile/logs/stdio

I've committed a build fix http://trac.webkit.org/changeset/77232. Please make sure that it's correct.

[Naoki Takano]

For me, LGTM.

Tamaru-san,

Do you have any comment?

(In reply to comment #61)
> http://trac.webkit.org/changeset/77228 broke Chromium Debug compilation: 
> http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Linux%20Builder%20(dbg)/builds/1186/steps/compile/logs/stdio
> 
> I've committed a build fix http://trac.webkit.org/changeset/77232. Please make sure that it's correct.

[Kent Tamura]

(In reply to comment #62)
> For me, LGTM.
> 
> Tamaru-san,
> 
> Do you have any comment?

No problem.  Yury and I talked about it before the commit.