Android UrlBar: Fix regression introduced by previous scrolling fix.

Android UrlBar: Fix regression introduced by previous scrolling fix.

Fixes regression introduced by:
https://codereview.chromium.org/2892203002

Still preserves the fix.

BUG=723100
TEST=MANUAL

Review-Url: https://codereview.chromium.org/2906793002
Cr-Commit-Position: refs/heads/master@{#474849}
Committed: https://chromium.googlesource.com/chromium/src/+/5f32dc08811c84362be404b8ce7a1d5b2100dd80

[tommycli, 2017-05-25 20:58:27]

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

[tommycli, 2017-05-25 20:59:17]

tommycli@chromium.org changed reviewers:
+ mgiuca@chromium.org, tedchoc@chromium.org

[tommycli, 2017-05-25 20:59:18]

tedchoc/mgiuca: PTAL

This seems to fix the regression introduced by previous patch without destroying
the fix.

In some senses this patch is a partial-revert.

Thanks!

[commit-bot: I haz the power, 2017-05-25 20:59:35]

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-25 22:47:31]

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

[commit-bot: I haz the power, 2017-05-25 22:47:32]

Dry run: This issue passed the CQ dry run.

[Ted C, 2017-05-25 23:41:17]

https://codereview.chromium.org/2906793002/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/omnibox/UrlBar.java
(right):

https://codereview.chromium.org/2906793002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/UrlBar.java:637:
setSelection(urlComponents.first.length());
would setSelection(0, urlComponents.first.length()) work or does that show the
selection bounding box?

I'm a bit worried that bringPointIntoView has different timing semantics than
setSelection, so this might set the selection but doing the small wiggle of the
omnibox might force it to be scrolled all the way to the front?

[tommycli, 2017-05-25 23:49:42]

https://codereview.chromium.org/2906793002/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/omnibox/UrlBar.java
(right):

https://codereview.chromium.org/2906793002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/UrlBar.java:637:
setSelection(urlComponents.first.length());
On 2017/05/25 23:41:17, Ted C wrote:
> would setSelection(0, urlComponents.first.length()) work or does that show the
> selection bounding box?
> 
> I'm a bit worried that bringPointIntoView has different timing semantics than
> setSelection, so this might set the selection but doing the small wiggle of
the
> omnibox might force it to be scrolled all the way to the front?

Hey, from what I was able to tell, setSelection(0, x) never had any different
from setSelection(x). I would be happy to change it to setSelection(0, x), but
it never had any benefits from what I could tell.

Hmm... I'm not sure about the wiggle comment... not sure how to interpret that.

But my reading of setSelection is that it enables the predraw listener, and then
calls bringPointIntoView on the next call to onPreDraw. So... it can only occur
after bringPointIntoView... it can't jump ahead of the previous
bringPointIntoView.

[Ted C, 2017-05-26 00:04:26]

On 2017/05/25 23:49:42, tommycli wrote:
>
https://codereview.chromium.org/2906793002/diff/1/chrome/android/java/src/org...
> File chrome/android/java/src/org/chromium/chrome/browser/omnibox/UrlBar.java
> (right):
> 
>
https://codereview.chromium.org/2906793002/diff/1/chrome/android/java/src/org...
> chrome/android/java/src/org/chromium/chrome/browser/omnibox/UrlBar.java:637:
> setSelection(urlComponents.first.length());
> On 2017/05/25 23:41:17, Ted C wrote:
> > would setSelection(0, urlComponents.first.length()) work or does that show
the
> > selection bounding box?
> > 
> > I'm a bit worried that bringPointIntoView has different timing semantics
than
> > setSelection, so this might set the selection but doing the small wiggle of
> the
> > omnibox might force it to be scrolled all the way to the front?
> 
> Hey, from what I was able to tell, setSelection(0, x) never had any different
> from setSelection(x). I would be happy to change it to setSelection(0, x), but
> it never had any benefits from what I could tell.
> 
> Hmm... I'm not sure about the wiggle comment... not sure how to interpret
that.

That was how I was seeing the quirks with the existing bringPointIIntoView
path.  For long searches, I would focus and then defocus the omnibox.  At
the end of the defocus animation, the text would be scrolled at the end.

If you scrubbed/wiggled the omnibox (like the very, very beginning of a 
toolbar side swipe), you'd see the text jump to the position you'd expect.

If you do the same path and you don't see any jumps with this approach, then
that seems good to me.

> 
> But my reading of setSelection is that it enables the predraw listener, and
then
> calls bringPointIntoView on the next call to onPreDraw. So... it can only
occur
> after bringPointIntoView... it can't jump ahead of the previous
> bringPointIntoView.

sgtm and lgtm

[tommycli, 2017-05-26 00:09:32]

On 2017/05/26 00:04:26, Ted C wrote:
> On 2017/05/25 23:49:42, tommycli wrote:
> >
>
https://codereview.chromium.org/2906793002/diff/1/chrome/android/java/src/org...
> > File chrome/android/java/src/org/chromium/chrome/browser/omnibox/UrlBar.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/2906793002/diff/1/chrome/android/java/src/org...
> > chrome/android/java/src/org/chromium/chrome/browser/omnibox/UrlBar.java:637:
> > setSelection(urlComponents.first.length());
> > On 2017/05/25 23:41:17, Ted C wrote:
> > > would setSelection(0, urlComponents.first.length()) work or does that show
> the
> > > selection bounding box?
> > > 
> > > I'm a bit worried that bringPointIntoView has different timing semantics
> than
> > > setSelection, so this might set the selection but doing the small wiggle
of
> > the
> > > omnibox might force it to be scrolled all the way to the front?
> > 
> > Hey, from what I was able to tell, setSelection(0, x) never had any
different
> > from setSelection(x). I would be happy to change it to setSelection(0, x),
but
> > it never had any benefits from what I could tell.
> > 
> > Hmm... I'm not sure about the wiggle comment... not sure how to interpret
> that.
> 
> That was how I was seeing the quirks with the existing bringPointIIntoView
> path.  For long searches, I would focus and then defocus the omnibox.  At
> the end of the defocus animation, the text would be scrolled at the end.
> 
> If you scrubbed/wiggled the omnibox (like the very, very beginning of a 
> toolbar side swipe), you'd see the text jump to the position you'd expect.
> 
> If you do the same path and you don't see any jumps with this approach, then
> that seems good to me.
> 
> > 
> > But my reading of setSelection is that it enables the predraw listener, and
> then
> > calls bringPointIntoView on the next call to onPreDraw. So... it can only
> occur
> > after bringPointIntoView... it can't jump ahead of the previous
> > bringPointIntoView.
> 
> sgtm and lgtm

Hey -- yes. I was able to reproduce the symptoms you saw -- including the 
on-blur issue, as well as the jump to the proper position during a wiggle.

I can confirm that this CL seems to fix these issues. I'll send it in, thanks!

[tommycli, 2017-05-26 00:09:38]

The CQ bit was checked by tommycli@chromium.org

[commit-bot: I haz the power, 2017-05-26 00:10:38]

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 00:16:09]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1495757378323630, "parent_rev":
"36dfaf4598059fdb20b71e81264f795c3781282f", "commit_rev":
"5f32dc08811c84362be404b8ce7a1d5b2100dd80"}

[commit-bot: I haz the power, 2017-05-26 00:16:22]

Description was changed from

==========
Android UrlBar: Fix regression introduced by previous scrolling fix.

Fixes regression introduced by:
https://codereview.chromium.org/2892203002

Still preserves the fix.

BUG=723100
TEST=MANUAL
==========

to

==========
Android UrlBar: Fix regression introduced by previous scrolling fix.

Fixes regression introduced by:
https://codereview.chromium.org/2892203002

Still preserves the fix.

BUG=723100
TEST=MANUAL

Review-Url: https://codereview.chromium.org/2906793002
Cr-Commit-Position: refs/heads/master@{#474849}
Committed:
https://chromium.googlesource.com/chromium/src/+/5f32dc08811c84362be404b8ce7a...
==========

[commit-bot: I haz the power, 2017-05-26 00:16:23]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/5f32dc08811c84362be404b8ce7a...