Use WebInputElement.isCheckbox() and isRadioButton() API being exposed by https://bugs.webkit.org/s…

Use WebInputElement.isCheckbox() and isRadioButton() API being exposed by https://bugs.webkit.org/show_bug.cgi?id=108031

BUG=157636
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=179491

[Raman Kakilate, 2013-01-28 19:05:45]

Will only submit once the webkit change is in and is available on chromium.
Wanted to get the required LGTMs early to avoid the delays.

[Albert Bodenhamer, 2013-01-28 19:16:27]

https://codereview.chromium.org/12084024/diff/1/chrome/renderer/autofill/form...
File chrome/renderer/autofill/form_autofill_util.cc (right):

https://codereview.chromium.org/12084024/diff/1/chrome/renderer/autofill/form...
chrome/renderer/autofill/form_autofill_util.cc:573: return
element.formControlType() == kSelectOne;
This could benefit from the same sort of optimization, but it's much less
obvious how to do the plumbing for it.  Maybe add a TODO here to look into it?

https://codereview.chromium.org/12084024/diff/1/chrome/renderer/autofill/form...
chrome/renderer/autofill/form_autofill_util.cc:584: return IsTextInput(element)
|| IsCheckableElement(element);
This CL is likely to fix the performance regression, but it MIGHT still be
better to put uncommenting your code into another CL so we have more control if
we've still missed it somehow.

[Raman Kakilate, 2013-01-28 20:02:59]

https://codereview.chromium.org/12084024/diff/1/chrome/renderer/autofill/form...
File chrome/renderer/autofill/form_autofill_util.cc (right):

https://codereview.chromium.org/12084024/diff/1/chrome/renderer/autofill/form...
chrome/renderer/autofill/form_autofill_util.cc:573: return
element.formControlType() == kSelectOne;
On 2013/01/28 19:16:28, Albert Bodenhamer wrote:
> This could benefit from the same sort of optimization, but it's much less
> obvious how to do the plumbing for it.  Maybe add a TODO here to look into it?

Created a issue and referred in the TODO.

https://codereview.chromium.org/12084024/diff/1/chrome/renderer/autofill/form...
chrome/renderer/autofill/form_autofill_util.cc:584: return IsTextInput(element)
|| IsCheckableElement(element);
On 2013/01/28 19:16:28, Albert Bodenhamer wrote:
> This CL is likely to fix the performance regression, but it MIGHT still be
> better to put uncommenting your code into another CL so we have more control
if
> we've still missed it somehow.

All the test data file changes in this CL depend on this line change itself. If
I leave this commented out, then the only change left would be
IsCheckableElement() function definition.

I think having them in single CL is easier.

[Albert Bodenhamer, 2013-01-28 20:04:14]

LGTM


On Mon, Jan 28, 2013 at 12:03 PM, <ramankk@chromium.org> wrote:

>
> https://codereview.chromium.**org/12084024/diff/1/chrome/**
>
renderer/autofill/form_**autofill_util.cc<https://codereview.chromium.org/12084024/diff/1/chrome/renderer/autofill/form_autofill_util.cc>
> File chrome/renderer/autofill/form_**autofill_util.cc (right):
>
> https://codereview.chromium.**org/12084024/diff/1/chrome/**
>
renderer/autofill/form_**autofill_util.cc#newcode573<https://codereview.chromium.org/12084024/diff/1/chrome/renderer/autofill/form_autofill_util.cc#newcode573>
> chrome/renderer/autofill/form_**autofill_util.cc:573: return
> element.formControlType() == kSelectOne;
> On 2013/01/28 19:16:28, Albert Bodenhamer wrote:
>
>> This could benefit from the same sort of optimization, but it's much
>>
> less
>
>> obvious how to do the plumbing for it.  Maybe add a TODO here to look
>>
> into it?
>
> Created a issue and referred in the TODO.
>
>
> https://codereview.chromium.**org/12084024/diff/1/chrome/**
>
renderer/autofill/form_**autofill_util.cc#newcode584<https://codereview.chromium.org/12084024/diff/1/chrome/renderer/autofill/form_autofill_util.cc#newcode584>
> chrome/renderer/autofill/form_**autofill_util.cc:584: return
> IsTextInput(element) || IsCheckableElement(element);
> On 2013/01/28 19:16:28, Albert Bodenhamer wrote:
>
>> This CL is likely to fix the performance regression, but it MIGHT
>>
> still be
>
>> better to put uncommenting your code into another CL so we have more
>>
> control if
>
>> we've still missed it somehow.
>>
>
> All the test data file changes in this CL depend on this line change
> itself. If I leave this commented out, then the only change left would
> be IsCheckableElement() function definition.
>
> I think having them in single CL is easier.
>
>
https://codereview.chromium.**org/12084024/<https://codereview.chromium.org/1...
>



-- 
Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com>
org

[Ilya Sherman, 2013-01-28 21:14:53]

LGTM, thanks.

https://codereview.chromium.org/12084024/diff/4001/chrome/renderer/autofill/f...
File chrome/renderer/autofill/form_autofill_util.cc (right):

https://codereview.chromium.org/12084024/diff/4001/chrome/renderer/autofill/f...
chrome/renderer/autofill/form_autofill_util.cc:573: // TODO: Fix issue 172665 by
avoiding string comparision.
TODO's should have names associated with them, e.g. "TODO(ramankk):".  If you
don't want to tag this as a TODO for yourself, since you've filed a bug for
this, IMO it's fine to omit the TODO entirely.

[Raman Kakilate, 2013-01-29 21:49:27]

https://codereview.chromium.org/12084024/diff/4001/chrome/renderer/autofill/f...
File chrome/renderer/autofill/form_autofill_util.cc (right):

https://codereview.chromium.org/12084024/diff/4001/chrome/renderer/autofill/f...
chrome/renderer/autofill/form_autofill_util.cc:573: // TODO: Fix issue 172665 by
avoiding string comparision.
On 2013/01/28 21:14:53, Ilya Sherman wrote:
> TODO's should have names associated with them, e.g. "TODO(ramankk):".  If you
> don't want to tag this as a TODO for yourself, since you've filed a bug for
> this, IMO it's fine to omit the TODO entirely.

Removed the comment as issue is filed.

[commit-bot: I haz the power, 2013-01-29 22:30:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ramankk@chromium.org/12084024/2003

[commit-bot: I haz the power, 2013-01-30 01:13:40]

Change committed as 179491