Make Textfield::GetTextInputClient() return non-null, even if readonly

Make Textfield::GetTextInputClient() return non-null, even if readonly

The TextInputClient is used to validate allowable operations in a
textfield (e.g. cut/copy/paste). On a readonly textfield, Copy should
work, but Cut/Paste should present to the user as being disabled.

Currently, a read-only textfield returns null from GetTextInputClient(),
making this validation impossible, except "inside" the textfield (e.g.
Textfield::ShowContextMenuForView(..)).

The logic originated in r80226, but doesn't seem to be for a specific
reason.

Note that TextInputClient::GetTextInputType() returns
TEXT_INPUT_TYPE_NONE for readonly textfields, and a test is updated to
check.

BUG=454353
Committed: https://crrev.com/eecac90df29818ec6e57344bf1663ba001f599e7
Cr-Commit-Position: refs/heads/master@{#317711}

[tapted, 2015-02-11 08:10:09]

tapted@chromium.org changed reviewers:
+ msw@chromium.org

[tapted, 2015-02-11 10:51:25]

Hi Michael, please take a look.

(for more context, this is split off from a mammoth "fix everything" CL in
http://crrev.com/901823002/ )

Thanks!

[msw, 2015-02-11 16:44:59]

msw@chromium.org changed reviewers:
+ suzhe@chromium.org

[msw, 2015-02-11 16:45:00]

lgtm with nits, but maybe wait for feedback from suzhe@.

https://codereview.chromium.org/919543003/diff/1/ui/views/controls/textfield/...
File ui/views/controls/textfield/textfield.cc (left):

https://codereview.chromium.org/919543003/diff/1/ui/views/controls/textfield/...
ui/views/controls/textfield/textfield.cc:703: return read_only_ ? NULL : this;
I didn't add this logic (as you note in the CL description), I merely ported the
logic from NativeTextfieldViews in that cited CL. +CC suzhe, since this actually
originated from Jame's input method integration in
https://codereview.chromium.org/6675005

https://codereview.chromium.org/919543003/diff/1/ui/views/controls/textfield/...
File ui/views/controls/textfield/textfield_unittest.cc (right):

https://codereview.chromium.org/919543003/diff/1/ui/views/controls/textfield/...
ui/views/controls/textfield/textfield_unittest.cc:1348:
textfield_->SetReadOnly(false);
nit: add EXPECT_TRUE(input_method_->text_input_type_changed()); after this.
maybe also EXPECT_EQ(text_input_client, textfield_->GetTextInputClient());

[tapted, 2015-02-11 23:07:28]

New patchsets have been uploaded after l-g-t-m from msw@chromium.org

[tapted, 2015-02-11 23:29:09]

suzhe - wdyt?

https://codereview.chromium.org/919543003/diff/1/ui/views/controls/textfield/...
File ui/views/controls/textfield/textfield.cc (left):

https://codereview.chromium.org/919543003/diff/1/ui/views/controls/textfield/...
ui/views/controls/textfield/textfield.cc:703: return read_only_ ? NULL : this;
On 2015/02/11 16:45:00, msw wrote:
> I didn't add this logic (as you note in the CL description), I merely ported
the
> logic from NativeTextfieldViews in that cited CL. +CC suzhe, since this
actually
> originated from Jame's input method integration in
> https://codereview.chromium.org/6675005

Thanks for the pointer! Updated the CL description. I had a dig through that
codereview too, and didn't see any red flags for this change, but I'll see what
suzhe thinks.

https://codereview.chromium.org/919543003/diff/1/ui/views/controls/textfield/...
File ui/views/controls/textfield/textfield_unittest.cc (right):

https://codereview.chromium.org/919543003/diff/1/ui/views/controls/textfield/...
ui/views/controls/textfield/textfield_unittest.cc:1348:
textfield_->SetReadOnly(false);
On 2015/02/11 16:45:00, msw wrote:
> nit: add EXPECT_TRUE(input_method_->text_input_type_changed()); after this.
> maybe also EXPECT_EQ(text_input_client, textfield_->GetTextInputClient());

Done.

[tapted, 2015-02-19 11:27:11]

Just an update on this. I raised this CL with James when we had a VC (just
before he left for a vacation!). He had seen it, and thinks it is safe so long
as GetTextInputType() is returning TEXT_INPUT_TYPE_NONE, which the test is
checking for.

But I'm in a paranoid mood, and there's no need to put this in just before
branch point. So I'll land this next week.

Thanks!

[tapted, 2015-02-24 00:29:44]

The CQ bit was checked by tapted@chromium.org

[commit-bot: I haz the power, 2015-02-24 00:32:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/919543003/20001

[commit-bot: I haz the power, 2015-02-24 01:06:06]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-02-24 01:06:39]

Patchset 2 (id:??) landed as
https://crrev.com/eecac90df29818ec6e57344bf1663ba001f599e7
Cr-Commit-Position: refs/heads/master@{#317711}