Refactor autocomplete-specific logic into a separate class

Refactor autocomplete-specific logic into a separate class

Mostly just moving things around, but simplify call paths to
limitDisplayableLength().

Also, add a JUnit test to ensure that basic functions work.

BUG=722908
Review-Url: https://codereview.chromium.org/2885973002
Cr-Commit-Position: refs/heads/master@{#472681}
Committed: https://chromium.googlesource.com/chromium/src/+/5a89721d814b1dcb82de46a779b13e8e43209f23

[Changwan Ryu, 2017-05-16 21:43:40]

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

[commit-bot: I haz the power, 2017-05-16 21:45:44]

Dry run: CQ is trying da patch.

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

[Changwan Ryu, 2017-05-16 21:57:34]

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

[Changwan Ryu, 2017-05-16 21:58:13]

changwan@chromium.org changed reviewers:
+ tedchoc@chromium.org

[Changwan Ryu, 2017-05-16 21:58:14]

PTAL

[commit-bot: I haz the power, 2017-05-16 21:58:23]

Dry run: CQ is trying da patch.

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

[Changwan Ryu, 2017-05-16 22:49:29]

I'll fix build/test issues asap. Sorry about that!

[commit-bot: I haz the power, 2017-05-16 22:49:38]

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

[commit-bot: I haz the power, 2017-05-16 22:49:38]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[Changwan Ryu, 2017-05-17 00:26:36]

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

[commit-bot: I haz the power, 2017-05-17 00:27:38]

Dry run: CQ is trying da patch.

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

[Changwan Ryu, 2017-05-17 00:37:37]

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

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

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-17 04:04:29]

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

[commit-bot: I haz the power, 2017-05-17 04:04:30]

Dry run: This issue passed the CQ dry run.

[Ted C, 2017-05-17 18:29:33]

lgtm ... seems good overall to me

https://codereview.chromium.org/2885973002/diff/60001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/omnibox/UrlBar.java
(left):

https://codereview.chromium.org/2885973002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/UrlBar.java:316:
assert mUrlBarDelegate != null;
should we keep this but call super.setIgnore...

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

https://codereview.chromium.org/2885973002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/UrlBar.java:738:
public String getQueryText() {
Can we just convert all the callers to use getTextWithAutocomplete?

https://codereview.chromium.org/2885973002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/omnibox/view/AutocompleteEditText.java
(right):

https://codereview.chromium.org/2885973002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/view/AutocompleteEditText.java:5:
package org.chromium.chrome.browser.omnibox.view;
I wouldn't introduce a new package yet.  The omnibox package is primarily view
stuff right now.

https://codereview.chromium.org/2885973002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/view/AutocompleteEditText.java:34:
private static final boolean DEBUG = false;
should we make this protected and have UrlBar key off the same value?

https://codereview.chromium.org/2885973002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/view/AutocompleteEditText.java:546:
return mIsPastedText;
Does this need to be in this class?  Looks like it could be isolated entirely
within UrlBar?

https://codereview.chromium.org/2885973002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/view/AutocompleteEditText.java:565:
public void setTextFromAutocomplete(String text) {
seems like this should be protected.  We really don't want anyone to call this
from outside this or the child classes

maybe we should call this replaceAllTextFromAutocomplete?

[Changwan Ryu, 2017-05-17 20:18:57]

https://codereview.chromium.org/2885973002/diff/60001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/omnibox/UrlBar.java
(left):

https://codereview.chromium.org/2885973002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/UrlBar.java:316:
assert mUrlBarDelegate != null;
On 2017/05/17 18:29:32, Ted C wrote:
> should we keep this but call super.setIgnore...

Done

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

https://codereview.chromium.org/2885973002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/UrlBar.java:738:
public String getQueryText() {
On 2017/05/17 18:29:32, Ted C wrote:
> Can we just convert all the callers to use getTextWithAutocomplete?

Done.

https://codereview.chromium.org/2885973002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/omnibox/view/AutocompleteEditText.java
(right):

https://codereview.chromium.org/2885973002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/view/AutocompleteEditText.java:5:
package org.chromium.chrome.browser.omnibox.view;
On 2017/05/17 18:29:33, Ted C wrote:
> I wouldn't introduce a new package yet.  The omnibox package is primarily view
> stuff right now.

Done.

https://codereview.chromium.org/2885973002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/view/AutocompleteEditText.java:34:
private static final boolean DEBUG = false;
On 2017/05/17 18:29:33, Ted C wrote:
> should we make this protected and have UrlBar key off the same value?

At least for me, I prefer to keep DEBUG value separately for each file. Then I
can
keep a combination of them as a commit and rebase it as necessary. I looked up
and most of the files (although not many) have separate DEBUG values.

https://codereview.chromium.org/2885973002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/view/AutocompleteEditText.java:546:
return mIsPastedText;
On 2017/05/17 18:29:33, Ted C wrote:
> Does this need to be in this class?  Looks like it could be isolated entirely
> within UrlBar?

Done. Note that I overrode shouldAutocomplete() to check isPastedText() there.

https://codereview.chromium.org/2885973002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/view/AutocompleteEditText.java:565:
public void setTextFromAutocomplete(String text) {
On 2017/05/17 18:29:33, Ted C wrote:
> seems like this should be protected.  We really don't want anyone to call this
> from outside this or the child classes
> 
> maybe we should call this replaceAllTextFromAutocomplete?

Done, thanks for the naming suggestion!

[Changwan Ryu, 2017-05-17 20:28:41]

The CQ bit was checked by changwan@chromium.org

[Changwan Ryu, 2017-05-17 20:28:41]

The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org
Link to the patchset: https://codereview.chromium.org/2885973002/#ps80001
(title: "address tedchoc's comments")

[commit-bot: I haz the power, 2017-05-17 20:30:21]

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-17 20:51:03]

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

[commit-bot: I haz the power, 2017-05-17 20:51:03]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Changwan Ryu, 2017-05-17 22:06:17]

The CQ bit was checked by changwan@chromium.org

[commit-bot: I haz the power, 2017-05-17 22:07:51]

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-18 05:20:52]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1495058777553970,
"parent_rev": "5b84a98141df91643a05bcb8e81c9f0398e6ba81", "commit_rev":
"5a89721d814b1dcb82de46a779b13e8e43209f23"}

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

Description was changed from

==========
Refactor autocomplete-specific logic into a separate class

Mostly just moving things around, but simplify call paths to
limitDisplayableLength().

Also, add a JUnit test to ensure that basic functions work.

BUG=722908
==========

to

==========
Refactor autocomplete-specific logic into a separate class

Mostly just moving things around, but simplify call paths to
limitDisplayableLength().

Also, add a JUnit test to ensure that basic functions work.

BUG=722908

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

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

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