[omnibox] Break out SetCaretPos() method and enhance browser test

[omnibox] Break out SetCaretPos() method and enhance browser test

Instead of calling SetWindowTextAndCaretPos() twice, add a SetCaretPos()
method so that we can call it instead.

Also, enhance the portable browser tests to make sure the caret
position is restored, but also capped at the text length.

BUG=681740
Review-Url: https://codereview.chromium.org/2891653003
Cr-Commit-Position: refs/heads/master@{#475045}
Committed: https://chromium.googlesource.com/chromium/src/+/0daf68bf98988529e6fe65c29f3de08a497e5564

[Kevin Bailey, 2017-05-18 13:41:34]

Description was changed from

==========
[omnibox] Break out SetCaretPos() method and enhance browser test

Instead of calling SetWindowTextAndCaretPos() twice, add a SetCaretPos()
method so that we can call it instead.

Also, enhance the portable browser tests to make sure the caret
position is restored, but also capped at the text length.

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

to

==========
[omnibox] Break out SetCaretPos() method and enhance browser test

Instead of calling SetWindowTextAndCaretPos() twice, add a SetCaretPos()
method so that we can call it instead.

Also, enhance the portable browser tests to make sure the caret
position is restored, but also capped at the text length.

BUG=681740
==========

[Kevin Bailey, 2017-05-18 13:41:34]

krb@chromium.org changed reviewers:
+ justincohen@chromium.org, pkasting@chromium.org

[Kevin Bailey, 2017-05-18 13:42:17]

krb@chromium.org changed reviewers:
+ rohitrao@chromium.org
- justincohen@chromium.org

[Kevin Bailey, 2017-05-18 13:44:50]

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

[commit-bot: I haz the power, 2017-05-18 13:45:11]

Dry run: CQ is trying da patch.

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

[Kevin Bailey, 2017-05-18 13:47:39]

Rohit,

Could you look at the IOS files? 'SetWindowTextAndCaretPos()' doesn't even use
caret_pos, so wondering if I even have the right files?

All,

Also, I'm not seeing the Android equivalents, but 2 bots succeeded, so let me
know if I missed something.

[commit-bot: I haz the power, 2017-05-18 14:08:02]

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

[commit-bot: I haz the power, 2017-05-18 14:08:03]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[rohitrao (ping after 24h), 2017-05-18 14:35:33]

ios/ lgtm.  I don't believe we've ever supported setting the caret position on
iOS.

Could I please be cc'ed on the underlying bug?  I'm getting an error when I try
to click through to it.

[Kevin Bailey, 2017-05-18 15:00:23]

On 2017/05/18 14:35:33, rohitrao (ping after 24h) wrote:
> ios/ lgtm.  I don't believe we've ever supported setting the caret position on
> iOS.
> 
> Could I please be cc'ed on the underlying bug?  I'm getting an error when I
try
> to click through to it.

Done.

[Peter Kasting, 2017-05-18 18:38:35]

https://codereview.chromium.org/2891653003/diff/40001/chrome/browser/ui/omnib...
File chrome/browser/ui/omnibox/omnibox_view_browsertest.cc (right):

https://codereview.chromium.org/2891653003/diff/40001/chrome/browser/ui/omnib...
chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:864: // Add a small amount
of text.
Nit: "...to move the cursor to a nonzero value."  Then update the next comment
to "Check that RevertAll() resets the text and preserves the cursor position."

https://codereview.chromium.org/2891653003/diff/40001/chrome/browser/ui/omnib...
chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:877: // Check if cursor
gets reverted safely.
Nit: Maybe "Check that reverting clamps the cursor to the bounds of the new
text."

https://codereview.chromium.org/2891653003/diff/40001/chrome/browser/ui/omnib...
chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:885: // Add a small amount
of text.
Nit: "...to push the cursor past where the text end will be once we revert."

https://codereview.chromium.org/2891653003/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right):

https://codereview.chromium.org/2891653003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:433: size_t pos =
std::min(caret_pos, text().length());
Nit: Is this actually necessary, or will SelectRange() do this for you?

https://codereview.chromium.org/2891653003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:435: SelectRange(range);
Nit: Or just

  SelectRange(gfx::Range(pos, pos));

https://codereview.chromium.org/2891653003/diff/40001/components/omnibox/brow...
File components/omnibox/browser/omnibox_edit_model.cc (right):

https://codereview.chromium.org/2891653003/diff/40001/components/omnibox/brow...
components/omnibox/browser/omnibox_edit_model.cc:375:
view_->SetWindowTextAndCaretPos(permanent_text_, 0, false, true);
Nit: I didn't mention this before, but this deserves a comment about why we're
setting the caret pos twice.

https://codereview.chromium.org/2891653003/diff/40001/components/omnibox/brow...
components/omnibox/browser/omnibox_edit_model.cc:376:
view_->SetCaretPos(std::min(permanent_text_.length(), start));
Should we use SetCaretPos() first, then SetWindowTextAndCaretPos()?  Or will
that break the scrolling functionality we're trying to achieve?

I ask because if someone listening for change notifications reads the caret pos,
that would ensure they see the correct value.

https://codereview.chromium.org/2891653003/diff/40001/components/omnibox/brow...
File components/omnibox/browser/omnibox_view.h (right):

https://codereview.chromium.org/2891653003/diff/40001/components/omnibox/brow...
components/omnibox/browser/omnibox_view.h:108: // past the current text.
Nit: This last sentence isn't perfectly clear about whether requests to do this
will be clamped or no-oped, so I'd instead say "Clamps the requested caret
position to the length of the current text."

https://codereview.chromium.org/2891653003/diff/40001/ios/chrome/browser/ui/o...
File ios/chrome/browser/ui/omnibox/omnibox_view_ios.mm (right):

https://codereview.chromium.org/2891653003/diff/40001/ios/chrome/browser/ui/o...
ios/chrome/browser/ui/omnibox/omnibox_view_ios.mm:245: void
OmniboxViewIOS::SetCaretPos(size_t caret_pos) {}
Is this NOTIMPLEMENTED(), or is there a reason why on iOS it makes no logical
sense to set the caret position?  I'm worried about the lack of that causing
potential security/usability bugs on iOS.  It seems like maybe support for this
needs to be added (separately).

[Kevin Bailey, 2017-05-18 19:48:17]

https://codereview.chromium.org/2891653003/diff/40001/chrome/browser/ui/omnib...
File chrome/browser/ui/omnibox/omnibox_view_browsertest.cc (right):

https://codereview.chromium.org/2891653003/diff/40001/chrome/browser/ui/omnib...
chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:864: // Add a small amount
of text.
On 2017/05/18 18:38:34, Peter Kasting wrote:
> Nit: "...to move the cursor to a nonzero value."  Then update the next comment
> to "Check that RevertAll() resets the text and preserves the cursor position."

Done.

https://codereview.chromium.org/2891653003/diff/40001/chrome/browser/ui/omnib...
chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:877: // Check if cursor
gets reverted safely.
On 2017/05/18 18:38:34, Peter Kasting wrote:
> Nit: Maybe "Check that reverting clamps the cursor to the bounds of the new
> text."

Done.

https://codereview.chromium.org/2891653003/diff/40001/chrome/browser/ui/omnib...
chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:885: // Add a small amount
of text.
On 2017/05/18 18:38:34, Peter Kasting wrote:
> Nit: "...to push the cursor past where the text end will be once we revert."

Done.

https://codereview.chromium.org/2891653003/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right):

https://codereview.chromium.org/2891653003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:433: size_t pos =
std::min(caret_pos, text().length());
On 2017/05/18 18:38:34, Peter Kasting wrote:
> Nit: Is this actually necessary, or will SelectRange() do this for you?

It's not necessary. It's corrected way down in RenderText. (Unlike Mac's
SetSelectedRange() which demands a valid position.)

https://codereview.chromium.org/2891653003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:435: SelectRange(range);
On 2017/05/18 18:38:34, Peter Kasting wrote:
> Nit: Or just
> 
>   SelectRange(gfx::Range(pos, pos));

Done.

https://codereview.chromium.org/2891653003/diff/40001/components/omnibox/brow...
File components/omnibox/browser/omnibox_edit_model.cc (right):

https://codereview.chromium.org/2891653003/diff/40001/components/omnibox/brow...
components/omnibox/browser/omnibox_edit_model.cc:375:
view_->SetWindowTextAndCaretPos(permanent_text_, 0, false, true);
On 2017/05/18 18:38:34, Peter Kasting wrote:
> Nit: I didn't mention this before, but this deserves a comment about why we're
> setting the caret pos twice.

Done.

https://codereview.chromium.org/2891653003/diff/40001/components/omnibox/brow...
components/omnibox/browser/omnibox_edit_model.cc:376:
view_->SetCaretPos(std::min(permanent_text_.length(), start));
On 2017/05/18 18:38:34, Peter Kasting wrote:
> Should we use SetCaretPos() first, then SetWindowTextAndCaretPos()?  Or will
> that break the scrolling functionality we're trying to achieve?

Ya, RenderText apparently won't update with just a cursor change. So when we
update the text, the cursor must be homed.

> I ask because if someone listening for change notifications reads the caret
pos,
> that would ensure they see the correct value.

Recall, the notification was my concern too, but I've yet to find anything that
complains. iirc Textfield will bounce important changes back up to the
controller?

https://codereview.chromium.org/2891653003/diff/40001/components/omnibox/brow...
File components/omnibox/browser/omnibox_view.h (right):

https://codereview.chromium.org/2891653003/diff/40001/components/omnibox/brow...
components/omnibox/browser/omnibox_view.h:108: // past the current text.
On 2017/05/18 18:38:34, Peter Kasting wrote:
> Nit: This last sentence isn't perfectly clear about whether requests to do
this
> will be clamped or no-oped, so I'd instead say "Clamps the requested caret
> position to the length of the current text."

Done. ("Removes..." probably isn't necessary. Let me know if it clutters the
comment.)

https://codereview.chromium.org/2891653003/diff/40001/ios/chrome/browser/ui/o...
File ios/chrome/browser/ui/omnibox/omnibox_view_ios.mm (right):

https://codereview.chromium.org/2891653003/diff/40001/ios/chrome/browser/ui/o...
ios/chrome/browser/ui/omnibox/omnibox_view_ios.mm:245: void
OmniboxViewIOS::SetCaretPos(size_t caret_pos) {}
On 2017/05/18 18:38:34, Peter Kasting wrote:
> Is this NOTIMPLEMENTED(), or is there a reason why on iOS it makes no logical
> sense to set the caret position?  I'm worried about the lack of that causing
> potential security/usability bugs on iOS.  It seems like maybe support for
this
> needs to be added (separately).

I assume this is for Rohit? You saw his comment?

[Peter Kasting, 2017-05-18 19:50:10]

https://codereview.chromium.org/2891653003/diff/40001/ios/chrome/browser/ui/o...
File ios/chrome/browser/ui/omnibox/omnibox_view_ios.mm (right):

https://codereview.chromium.org/2891653003/diff/40001/ios/chrome/browser/ui/o...
ios/chrome/browser/ui/omnibox/omnibox_view_ios.mm:245: void
OmniboxViewIOS::SetCaretPos(size_t caret_pos) {}
On 2017/05/18 19:48:17, Kevin Bailey wrote:
> On 2017/05/18 18:38:34, Peter Kasting wrote:
> > Is this NOTIMPLEMENTED(), or is there a reason why on iOS it makes no
logical
> > sense to set the caret position?  I'm worried about the lack of that causing
> > potential security/usability bugs on iOS.  It seems like maybe support for
> this
> > needs to be added (separately).
> 
> I assume this is for Rohit? You saw his comment?

Yes.

[Kevin Bailey, 2017-05-23 13:56:50]

On 2017/05/18 19:50:10, Peter Kasting wrote:
>
https://codereview.chromium.org/2891653003/diff/40001/ios/chrome/browser/ui/o...
> File ios/chrome/browser/ui/omnibox/omnibox_view_ios.mm (right):
> 
>
https://codereview.chromium.org/2891653003/diff/40001/ios/chrome/browser/ui/o...
> ios/chrome/browser/ui/omnibox/omnibox_view_ios.mm:245: void
> OmniboxViewIOS::SetCaretPos(size_t caret_pos) {}
> On 2017/05/18 19:48:17, Kevin Bailey wrote:
> > On 2017/05/18 18:38:34, Peter Kasting wrote:
> > > Is this NOTIMPLEMENTED(), or is there a reason why on iOS it makes no
> logical
> > > sense to set the caret position?  I'm worried about the lack of that
causing
> > > potential security/usability bugs on iOS.  It seems like maybe support for
> > this
> > > needs to be added (separately).
> > 
> > I assume this is for Rohit? You saw his comment?
> 
> Yes.

ping? Rohit, I think Peter had a question for you?

[rohitrao (ping after 24h), 2017-05-23 14:02:40]

https://codereview.chromium.org/2891653003/diff/40001/ios/chrome/browser/ui/o...
File ios/chrome/browser/ui/omnibox/omnibox_view_ios.mm (right):

https://codereview.chromium.org/2891653003/diff/40001/ios/chrome/browser/ui/o...
ios/chrome/browser/ui/omnibox/omnibox_view_ios.mm:245: void
OmniboxViewIOS::SetCaretPos(size_t caret_pos) {}
On 2017/05/18 18:38:34, Peter Kasting wrote:
> Is this NOTIMPLEMENTED(), or is there a reason why on iOS it makes no logical
> sense to set the caret position?  I'm worried about the lack of that causing
> potential security/usability bugs on iOS.  It seems like maybe support for
this
> needs to be added (separately).

This used to be impossible with the APIs that iOS provided.  It's possible to
implement now, but "caret position" only has meaning when the omnibox is
focused.

Does anyone know what scenarios cause SetCaretPos() to be called?  I remember
this being used mostly for Instant, so at one point it never really got called
on iOS.

[Kevin Bailey, 2017-05-23 14:16:20]

https://codereview.chromium.org/2891653003/diff/40001/ios/chrome/browser/ui/o...
File ios/chrome/browser/ui/omnibox/omnibox_view_ios.mm (right):

https://codereview.chromium.org/2891653003/diff/40001/ios/chrome/browser/ui/o...
ios/chrome/browser/ui/omnibox/omnibox_view_ios.mm:245: void
OmniboxViewIOS::SetCaretPos(size_t caret_pos) {}
On 2017/05/23 14:02:39, rohitrao (ping after 24h) wrote:
> On 2017/05/18 18:38:34, Peter Kasting wrote:
> > Is this NOTIMPLEMENTED(), or is there a reason why on iOS it makes no
logical
> > sense to set the caret position?  I'm worried about the lack of that causing
> > potential security/usability bugs on iOS.  It seems like maybe support for
> this
> > needs to be added (separately).
> 
> This used to be impossible with the APIs that iOS provided.  It's possible to
> implement now, but "caret position" only has meaning when the omnibox is
> focused.
> 
> Does anyone know what scenarios cause SetCaretPos() to be called?  I remember
> this being used mostly for Instant, so at one point it never really got called
> on iOS.

SetWindowTextAndCaretPos() is called during most any change to the Omnibox,
except single character entry, which the Textfield takes care of itself.
SetCaretPos() is a new function that we just added for when we only need to
update the caret. Currently, it's only called from OmniboxEditModel::Revert().

[Peter Kasting, 2017-05-25 07:25:13]

Note: I'm behind on codereviews but will try and get to this today (Thursday May
25), after I get some sleep.

[Peter Kasting, 2017-05-25 23:19:19]

LGTM.  I will leave it in Rohit's hands whether he thinks it's worth sticking a
NOTIMPLEMENTED() in the iOS Set[WindowTextAnd]CaretPos() functions.

https://codereview.chromium.org/2891653003/diff/60001/components/omnibox/brow...
File components/omnibox/browser/omnibox_edit_model.cc (right):

https://codereview.chromium.org/2891653003/diff/60001/components/omnibox/brow...
components/omnibox/browser/omnibox_edit_model.cc:375: // First home cursor, so
view of text is scrolled to left, then correct it.
Nit: This helps, but I'd probably expand the comment further to clarify what
would happen if you didn't do this.  (The scrolling behavior when the cursor
position is set is not immediately obvious.)

[rohitrao (ping after 24h), 2017-05-26 12:33:55]

On 2017/05/25 23:19:19, Peter Kasting wrote:
> LGTM.  I will leave it in Rohit's hands whether he thinks it's worth sticking
a
> NOTIMPLEMENTED() in the iOS Set[WindowTextAnd]CaretPos() functions.

I filed bug 726702 to implement those functions, so adding a reference to that
bug would be appreciated.  I'd rather not add NOTIMPLEMENTED(), to avoid adding
unnecessary logging.

[Kevin Bailey, 2017-05-26 13:58:06]

rohitrao wrote:
> I filed bug 726702 to implement those functions, so adding a reference to that
> bug would be appreciated.  I'd rather not add NOTIMPLEMENTED(), to avoid
adding
> unnecessary logging.

Done.

https://codereview.chromium.org/2891653003/diff/60001/components/omnibox/brow...
File components/omnibox/browser/omnibox_edit_model.cc (right):

https://codereview.chromium.org/2891653003/diff/60001/components/omnibox/brow...
components/omnibox/browser/omnibox_edit_model.cc:375: // First home cursor, so
view of text is scrolled to left, then correct it.
On 2017/05/25 23:19:19, Peter Kasting wrote:
> Nit: This helps, but I'd probably expand the comment further to clarify what
> would happen if you didn't do this.  (The scrolling behavior when the cursor
> position is set is not immediately obvious.)

Done.

[Kevin Bailey, 2017-05-26 13:58:13]

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

[commit-bot: I haz the power, 2017-05-26 13:58:19]

Dry run: 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-05-26 14:45:32]

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

[commit-bot: I haz the power, 2017-05-26 14:45:33]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Kevin Bailey, 2017-05-26 15:44:34]

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

[commit-bot: I haz the power, 2017-05-26 15:45:02]

Dry run: 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-05-26 16:10:34]

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

[commit-bot: I haz the power, 2017-05-26 16:10:35]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Kevin Bailey, 2017-05-26 16:57:21]

The CQ bit was checked by krb@chromium.org

[Kevin Bailey, 2017-05-26 16:57:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
rohitrao@chromium.org, pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2891653003/#ps100001
(title: "Fixed range on Cocoa")

[commit-bot: I haz the power, 2017-05-26 16:57:45]

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-05-26 17:31:04]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1495817841726470,
"parent_rev": "1a1392222a9b97ffbaa77f3beee093fc81e655ff", "commit_rev":
"99d5520f68cd2b08d948db029de1b06fb1bf760c"}

[commit-bot: I haz the power, 2017-05-26 17:31:21]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1495817841726470,
"parent_rev": "802480762c4dcb5e61bcafc21edf49002ca003f8", "commit_rev":
"0daf68bf98988529e6fe65c29f3de08a497e5564"}

[commit-bot: I haz the power, 2017-05-26 17:31:32]

Description was changed from

==========
[omnibox] Break out SetCaretPos() method and enhance browser test

Instead of calling SetWindowTextAndCaretPos() twice, add a SetCaretPos()
method so that we can call it instead.

Also, enhance the portable browser tests to make sure the caret
position is restored, but also capped at the text length.

BUG=681740
==========

to

==========
[omnibox] Break out SetCaretPos() method and enhance browser test

Instead of calling SetWindowTextAndCaretPos() twice, add a SetCaretPos()
method so that we can call it instead.

Also, enhance the portable browser tests to make sure the caret
position is restored, but also capped at the text length.

BUG=681740

Review-Url: https://codereview.chromium.org/2891653003
Cr-Commit-Position: refs/heads/master@{#475045}
Committed:
https://chromium.googlesource.com/chromium/src/+/0daf68bf98988529e6fe65c29f3d...
==========

[commit-bot: I haz the power, 2017-05-26 17:31:34]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/0daf68bf98988529e6fe65c29f3d...