Remove shouldUseInputMethod

Remove shouldUseInputMethod

This method is only used in WebViewImpl so call isContentEditable directly, remove the InputType shouldUseInputMethod methods too.

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

[rwlbuis, 2014-07-21 21:59:00]

PTAL

[adamk, 2014-07-21 22:06:33]

https://codereview.chromium.org/408883003/diff/1/Source/core/dom/Element.cpp
File Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/408883003/diff/1/Source/core/dom/Element.cpp#...
Source/core/dom/Element.cpp:226: return isHTMLInputElement(this) ?
toHTMLInputElement(this)->shouldUseInputMethod() :
isContentEditable(UserSelectAllIsAlwaysNonEditable);
Is isHTMLInputElement(this) ever true? The only caller I could find of
Element::shouldUseInputMethod() is in WebViewImpl::textInputType, and there's a
whole separate section of that method dealing with HTMLInputElement.

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

https://codereview.chromium.org/408883003/diff/1/Source/core/html/HTMLTextAre...
Source/core/html/HTMLTextAreaElement.cpp:528: return true;
As far as I can tell this is only safe to remove because WebViewImpl never calls
it. Is that correct?

[rwlbuis, 2014-07-21 22:10:18]

On 2014/07/21 22:06:33, adamk wrote:
> https://codereview.chromium.org/408883003/diff/1/Source/core/dom/Element.cpp
> File Source/core/dom/Element.cpp (right):
> 
>
https://codereview.chromium.org/408883003/diff/1/Source/core/dom/Element.cpp#...
> Source/core/dom/Element.cpp:226: return isHTMLInputElement(this) ?
> toHTMLInputElement(this)->shouldUseInputMethod() :
> isContentEditable(UserSelectAllIsAlwaysNonEditable);
> Is isHTMLInputElement(this) ever true? The only caller I could find of
> Element::shouldUseInputMethod() is in WebViewImpl::textInputType, and there's
a
> whole separate section of that method dealing with HTMLInputElement.

Oh you are right, it seems that both HTMLTextAreaElement and HTMLInputElement's
are already handled before
shouldUseInputMethod is called, great catch!

>
https://codereview.chromium.org/408883003/diff/1/Source/core/html/HTMLTextAre...
> File Source/core/html/HTMLTextAreaElement.cpp (left):
> 
>
https://codereview.chromium.org/408883003/diff/1/Source/core/html/HTMLTextAre...
> Source/core/html/HTMLTextAreaElement.cpp:528: return true;
> As far as I can tell this is only safe to remove because WebViewImpl never
calls
> it. Is that correct?

Correct, see above. I'll make a new patch that should be even simpler.

[rwlbuis, 2014-07-22 00:12:31]

V2 patch should address the issues from the review.

[adamk, 2014-07-22 01:10:08]

https://codereview.chromium.org/408883003/diff/20001/Source/core/dom/Element.h
File Source/core/dom/Element.h (right):

https://codereview.chromium.org/408883003/diff/20001/Source/core/dom/Element....
Source/core/dom/Element.h:516: bool shouldUseInputMethod();
If the only caller is indeed WebViewImpl, I think you should just delete the
method and inline the contents into that one caller. It would actually make the
code clearer, given that the test is:

     if (element->shouldUseInputMethod())
         return WebTextInputTypeContentEditable;

https://codereview.chromium.org/408883003/diff/20001/Source/core/html/HTMLInp...
File Source/core/html/HTMLInputElement.h (right):

https://codereview.chromium.org/408883003/diff/20001/Source/core/html/HTMLInp...
Source/core/html/HTMLInputElement.h:272: bool shouldUseInputMethod();
Can't this method (and all the input type methods of the same name) just be
deleted? Again, I don't see any callers of shouldUseInputMethod() beyond that
one in WebViewImpl.

[rwlbuis, 2014-07-22 14:50:53]

On 2014/07/22 01:10:08, adamk wrote:
> https://codereview.chromium.org/408883003/diff/20001/Source/core/dom/Element.h
> File Source/core/dom/Element.h (right):
> 
>
https://codereview.chromium.org/408883003/diff/20001/Source/core/dom/Element....
> Source/core/dom/Element.h:516: bool shouldUseInputMethod();
> If the only caller is indeed WebViewImpl, I think you should just delete the
> method and inline the contents into that one caller. It would actually make
the
> code clearer, given that the test is:
> 
>      if (element->shouldUseInputMethod())
>          return WebTextInputTypeContentEditable;
> 
>
https://codereview.chromium.org/408883003/diff/20001/Source/core/html/HTMLInp...
> File Source/core/html/HTMLInputElement.h (right):
> 
>
https://codereview.chromium.org/408883003/diff/20001/Source/core/html/HTMLInp...
> Source/core/html/HTMLInputElement.h:272: bool shouldUseInputMethod();
> Can't this method (and all the input type methods of the same name) just be
> deleted? Again, I don't see any callers of shouldUseInputMethod() beyond that
> one in WebViewImpl.

You are right, I was a bit hasty with the patch, will clean it up and upload.

[rwlbuis, 2014-07-22 15:32:05]

PTAL, it turns out even more code can be removed.

[adamk, 2014-07-22 15:33:32]

lgtm (CC tkent FYI)

[rwlbuis, 2014-07-22 16:42:07]

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

[commit-bot: I haz the power, 2014-07-22 16:42:31]

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

[commit-bot: I haz the power, 2014-07-22 17:47:47]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[commit-bot: I haz the power, 2014-07-22 18:24:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-22 18:24:49]

Try jobs failed on following builders:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[rwlbuis, 2014-07-22 20:41:30]

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

[commit-bot: I haz the power, 2014-07-22 20:42:18]

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

[commit-bot: I haz the power, 2014-07-22 20:43:26]

Change committed as 178691