Unnecessarily cursor appears in omnibox after clicking inside the 'Search box'

Remove the unnecessarily cursor appeared in omnibox after clicking inside the 'Search box' 

BUG=696894
Review-Url: https://codereview.chromium.org/2728433002
Cr-Commit-Position: refs/heads/master@{#454148}
Committed: https://chromium.googlesource.com/chromium/src/+/413ab9af7d2755e3287b07b08c21fc3be062103c

[yiyix, 2017-02-28 21:58:15]

Description was changed from

==========
fix

BUG=
==========

to

==========
Unnecessarily cursor appears in omnibox after clicking inside the 'Search box' 

BUG=696894
==========

[yiyix, 2017-02-28 21:59:09]

Description was changed from

==========
Unnecessarily cursor appears in omnibox after clicking inside the 'Search box' 

BUG=696894
==========

to

==========
Remove the unnecessarily cursor appears in omnibox after clicking inside the
'Search box' 

BUG=696894
==========

[yiyix, 2017-02-28 21:59:09]

yiyix@chromium.org changed reviewers:
+ sadrul@chromium.org

[yiyix, 2017-02-28 21:59:27]

Description was changed from

==========
Remove the unnecessarily cursor appears in omnibox after clicking inside the
'Search box' 

BUG=696894
==========

to

==========
Remove the unnecessarily cursor appeared in omnibox after clicking inside the
'Search box' 

BUG=696894
==========

[yiyix, 2017-02-28 21:59:50]

@sadrul, could you please take a look at this change? Thank you very much

[sadrul, 2017-03-01 15:33:28]

looks good! Is it possible to write a test for this?

[yiyix, 2017-03-01 20:34:30]

On 2017/03/01 15:33:28, sadrul wrote:
> looks good! Is it possible to write a test for this?

I added some tests for this bug. Could you please take another look? Thank you.

[sadrul, 2017-03-01 20:51:24]

I think what you have is good. Maybe it could be a little bit cleaner? Can you
try out the change I suggested below and see if that works?

Thanks for the test!

https://codereview.chromium.org/2728433002/diff/40001/ui/views/controls/textf...
File ui/views/controls/textfield/textfield.cc (right):

https://codereview.chromium.org/2728433002/diff/40001/ui/views/controls/textf...
ui/views/controls/textfield/textfield.cc:984: if (ShouldShowCursor()) {
Can you look at whether ShouldShowCursor() should check
render_text->cursor_enabled() instead?

https://codereview.chromium.org/2728433002/diff/40001/ui/views/controls/textf...
ui/views/controls/textfield/textfield.cc:2093: !cursor_view_.visible());
If cursor is disabled in RenderText, is there a reason to run the blink-timer at
all?

[yiyix, 2017-03-01 22:14:26]

The CQ bit was checked by yiyix@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-01 22:14:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yiyix, 2017-03-01 22:18:58]

@sadrul, could you please check this version again? Thank you very much.

https://codereview.chromium.org/2728433002/diff/40001/ui/views/controls/textf...
File ui/views/controls/textfield/textfield.cc (right):

https://codereview.chromium.org/2728433002/diff/40001/ui/views/controls/textf...
ui/views/controls/textfield/textfield.cc:984: if (ShouldShowCursor()) {
On 2017/03/01 20:51:24, sadrul wrote:
> Can you look at whether ShouldShowCursor() should check
> render_text->cursor_enabled() instead?

ShouldShowCursor() does not check render_text->cursor_enabled(). Yes, I agree
that this check should be done in ShouldShowCursor(). Thank you for catching
this.

https://codereview.chromium.org/2728433002/diff/40001/ui/views/controls/textf...
ui/views/controls/textfield/textfield.cc:2093: !cursor_view_.visible());
On 2017/03/01 20:51:24, sadrul wrote:
> If cursor is disabled in RenderText, is there a reason to run the blink-timer
at
> all?

After the modification, this function is only called if ShouldBlinkCursor()
returns true. No need to check GetRenderText()->cursor_enabled() in this
function.

[commit-bot: I haz the power, 2017-03-01 23:21:31]

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

[commit-bot: I haz the power, 2017-03-01 23:21:32]

Dry run: This issue passed the CQ dry run.

[sadrul, 2017-03-01 23:59:00]

Thanks! lgtm

[yiyix, 2017-03-02 01:50:51]

The CQ bit was checked by yiyix@chromium.org

[commit-bot: I haz the power, 2017-03-02 01:51:52]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-02 02:00:27]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1488419451834610,
"parent_rev": "7f57605b9ac7dd12449a7fc6494a74786959785b", "commit_rev":
"413ab9af7d2755e3287b07b08c21fc3be062103c"}

[commit-bot: I haz the power, 2017-03-02 02:01:06]

Description was changed from

==========
Remove the unnecessarily cursor appeared in omnibox after clicking inside the
'Search box' 

BUG=696894
==========

to

==========
Remove the unnecessarily cursor appeared in omnibox after clicking inside the
'Search box' 

BUG=696894

Review-Url: https://codereview.chromium.org/2728433002
Cr-Commit-Position: refs/heads/master@{#454148}
Committed:
https://chromium.googlesource.com/chromium/src/+/413ab9af7d2755e3287b07b08c21...
==========

[commit-bot: I haz the power, 2017-03-02 02:01:08]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/413ab9af7d2755e3287b07b08c21...