Remove form control related virtual methods from Element

Remove form control related virtual methods from Element

As the FIXME says, these are only used by form controls. That is, setChangedSinceLastFormControlChangeEvent
can be removed from Element as-is. The other two can also be removed but only after adding some cast code
to Document.setFocusedElement.

Based on http://trac.webkit.org/changeset/150711 by Andreas Kling.

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

[rwlbuis, 2014-01-30 00:58:30]

I did a first patch for this, removing three virtuals. Then I checked WebKit and
discovered there is one more virtual to remove. So in the end my patch is very
similar to the WebKit one.

[Inactive, 2014-01-30 01:21:33]

lgtm

[commit-bot: I haz the power, 2014-01-30 01:21:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rob.buis@samsung.com/137833018/40001

[tkent, 2014-01-30 01:38:50]

https://codereview.chromium.org/137833018/diff/40001/Source/core/dom/Document...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/137833018/diff/40001/Source/core/dom/Document...
Source/core/dom/Document.cpp:3366: if
(oldFocusedElement->isFormControlElement()) {
I don't like this change.
It increased virtual calls, and the function needs to know the special
requirement of HTMLFormControlElement.

Alternative idea is to move this code into
HTMLTextFormControlElement::setFocus().

[Inactive, 2014-01-30 01:40:29]

On 2014/01/30 01:38:50, tkent wrote:
>
https://codereview.chromium.org/137833018/diff/40001/Source/core/dom/Document...
> File Source/core/dom/Document.cpp (right):
> 
>
https://codereview.chromium.org/137833018/diff/40001/Source/core/dom/Document...
> Source/core/dom/Document.cpp:3366: if
> (oldFocusedElement->isFormControlElement()) {
> I don't like this change.
> It increased virtual calls, and the function needs to know the special
> requirement of HTMLFormControlElement.
> 
> Alternative idea is to move this code into
> HTMLTextFormControlElement::setFocus().

tkent, can you uncheck the commit box? I don't have the rights to do so.

[commit-bot: I haz the power, 2014-01-30 01:45:18]

CQ bit was unchecked on CL. Ignoring.

[Inactive, 2014-01-30 01:57:22]

https://codereview.chromium.org/137833018/diff/40001/Source/core/dom/Document...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/137833018/diff/40001/Source/core/dom/Document...
Source/core/dom/Document.cpp:3366: if
(oldFocusedElement->isFormControlElement()) {
On 2014/01/30 01:38:50, tkent wrote:
> I don't like this change.
> It increased virtual calls, and the function needs to know the special
> requirement of HTMLFormControlElement.
> 
> Alternative idea is to move this code into
> HTMLTextFormControlElement::setFocus().
> 

Did it really increase virtual calls? Sure isFormControlElement() is a new
virtual call but wasChangedSinceLastFormControlChangeEvent() is no longer one.
So it looks like it evens out.

I do agree with your second point though. The HTMLFormControlElement special
handling is now more explicit. Overriding Node::setFocus(bool) in
HTMLFormControlElement and moving the code there does sound like a better idea.
It seems setFocus() is only called from this method.

[rwlbuis, 2014-01-30 20:16:08]

The new patch now implements the setFocus logic in HTMLFormControlElement, which
does seem to give much cleaner code in Document.cpp.

[tkent, 2014-01-31 00:09:21]

lgtm

This looks nice.  Thanks!

[commit-bot: I haz the power, 2014-01-31 00:09:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rob.buis@samsung.com/137833018/60001

[commit-bot: I haz the power, 2014-01-31 03:27:20]

Retried try job too often on linux_blink for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

[commit-bot: I haz the power, 2014-01-31 03:27:20]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-01-31 03:27:23]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-01-31 03:28:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rob.buis@samsung.com/137833018/60001

[commit-bot: I haz the power, 2014-01-31 07:43:03]

Retried try job too often on win_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...

[commit-bot: I haz the power, 2014-01-31 07:43:08]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-01-31 07:43:11]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-01-31 08:06:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rob.buis@samsung.com/137833018/60001

[commit-bot: I haz the power, 2014-01-31 11:32:40]

Retried try job too often on win_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...

[commit-bot: I haz the power, 2014-01-31 11:32:42]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-01-31 11:32:49]

CQ bit was unchecked on CL. Ignoring.

[rwlbuis, 2014-01-31 13:48:56]

The CQ bit was checked by rob.buis@samsung.com

[commit-bot: I haz the power, 2014-01-31 13:51:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rob.buis@samsung.com/137833018/60001

[commit-bot: I haz the power, 2014-01-31 14:32:49]

Change committed as 166216

[commit-bot: I haz the power, 2014-01-31 14:32:51]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-01-31 14:32:52]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-01-31 14:32:54]

CQ bit was unchecked on CL. Ignoring.