Make urlbar in custom tabs not focusable

Make urlbar in custom tabs not focusable

The urlbar in custom tabs is disabled and is not editable. However we
call urlbar.requestFocus in several cases, which causes a framework
crash. This CL added a flag mShouldHaveFocus in UrlBar and in onDraw()
we can choose not to set the urlbar to be focusable.

BUG=496418
Committed: https://crrev.com/890525d7cba5be038e640305f3199bac2bd964ef
Cr-Commit-Position: refs/heads/master@{#336641}

[Ian Wen, 2015-06-24 00:01:26]

ianwen@chromium.org changed reviewers:
+ tedchoc@chromium.org, yusufo@chromium.org

[Ian Wen, 2015-06-24 00:01:27]

ptal :)

[Ian Wen, 2015-06-24 00:19:06]

ianwen@chromium.org changed reviewers:
+ pedrosimonetti@chromium.org

[Yusuf, 2015-06-24 16:54:59]

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

https://codereview.chromium.org/1207523003/diff/1/chrome/android/java_staging...
chrome/android/java_staging/src/org/chromium/chrome/browser/omnibox/UrlBar.java:375:
public void setShouldHaveFocus(boolean shouldHaveFocus) {
rename to allowFocus() and mAllowFocus. 

inside the call itself do setFocusable(allowFocus) and
setFocusableInTouchMode(allowFocus) so we don't have to worry about timing.

Or if we don't want to set default, flip the logic and call it disallowFocus and
mDisallowFocus. and flip the boolean in all setFocusable calls.

https://codereview.chromium.org/1207523003/diff/1/chrome/android/java_staging...
chrome/android/java_staging/src/org/chromium/chrome/browser/omnibox/UrlBar.java:482:
if (!mFirstDrawComplete && mShouldHaveFocus) {
This will avoid firstdrawcomplete getting set to true as well.

Instead use the boolean in the setFocusable() calls below.
setFocusable(mAllowFocus);
setFocusableInTouchMode(mAllowFocus);

[Ian Wen, 2015-06-24 20:30:22]

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

https://codereview.chromium.org/1207523003/diff/1/chrome/android/java_staging...
chrome/android/java_staging/src/org/chromium/chrome/browser/omnibox/UrlBar.java:375:
public void setShouldHaveFocus(boolean shouldHaveFocus) {
On 2015/06/24 16:54:59, Yusuf wrote:
> rename to allowFocus() and mAllowFocus. 
> 
> inside the call itself do setFocusable(allowFocus) and
> setFocusableInTouchMode(allowFocus) so we don't have to worry about timing.
> 
> Or if we don't want to set default, flip the logic and call it disallowFocus
and
> mDisallowFocus. and flip the boolean in all setFocusable calls.

Done.

https://codereview.chromium.org/1207523003/diff/1/chrome/android/java_staging...
chrome/android/java_staging/src/org/chromium/chrome/browser/omnibox/UrlBar.java:482:
if (!mFirstDrawComplete && mShouldHaveFocus) {
On 2015/06/24 16:54:59, Yusuf wrote:
> This will avoid firstdrawcomplete getting set to true as well.
> 
> Instead use the boolean in the setFocusable() calls below.
> setFocusable(mAllowFocus);
> setFocusableInTouchMode(mAllowFocus);

Done.

[Ian Wen, 2015-06-29 18:06:35]

I double checked all places where we give the urlbar focus, and my conclusion is
that for custom tabs it is okay to make urlbar not focusable. There are some
implications after this change:

1. voice search in omnibox won't work properly if the urlbar is not focusable.
However in custom tab the urlbar is not searchable.
2. zero suggest won't work as the urlbar is never granted focus.
3. If a hardware keyboard user presses TAB key for multiple times and he reaches
the end of the webcontents, ChromeTab#takeFocus() will be called and normally
the urlbar will be focused afterwards. However in custom tab the urlbar is not
editable so the urlbar won't be focused.

In our codebase we never call urlbar#requestFocus() and then immediately do
something assuming that the urlbar now has focus. Instead we call
requestFocus(), and then in the focus change callback
(LocationBar#onUrlFocusChange) we do something.

The only exception of the above statement is the
LocationBarLayout#setSearchQuery(), which is called by voice search. Yet in
custom tabs omnibox voice search will not be triggered.

[Ted C, 2015-06-29 18:12:03]

lgtm and thanks for the investigation!

Also, we should really fix that ChromeTab#takeFocus method, but certainly not in
this patch

[Yusuf, 2015-06-29 18:12:14]

lgtm after the final nit

https://codereview.chromium.org/1207523003/diff/20001/chrome/android/java_sta...
File
chrome/android/java_staging/src/org/chromium/chrome/browser/omnibox/UrlBar.java
(right):

https://codereview.chromium.org/1207523003/diff/20001/chrome/android/java_sta...
chrome/android/java_staging/src/org/chromium/chrome/browser/omnibox/UrlBar.java:495:
if (mOmniboxLivenessListener != null && mAllowFocus) {
this is for user stats recording and testing purposes, so lets keep it called
and remove mAllowFocus

[Ian Wen, 2015-06-29 20:21:39]

https://codereview.chromium.org/1207523003/diff/20001/chrome/android/java_sta...
File
chrome/android/java_staging/src/org/chromium/chrome/browser/omnibox/UrlBar.java
(right):

https://codereview.chromium.org/1207523003/diff/20001/chrome/android/java_sta...
chrome/android/java_staging/src/org/chromium/chrome/browser/omnibox/UrlBar.java:495:
if (mOmniboxLivenessListener != null && mAllowFocus) {
On 2015/06/29 18:12:14, Yusuf wrote:
> this is for user stats recording and testing purposes, so lets keep it called
> and remove mAllowFocus

Done.

[Ian Wen, 2015-06-29 20:32:08]

The CQ bit was checked by ianwen@chromium.org

[Ian Wen, 2015-06-29 20:32:08]

The patchset sent to the CQ was uploaded after l-g-t-m from
tedchoc@chromium.org, yusufo@chromium.org
Link to the patchset: https://codereview.chromium.org/1207523003/#ps40001
(title: "one nit")

[commit-bot: I haz the power, 2015-06-29 20:34:45]

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

[commit-bot: I haz the power, 2015-06-29 21:13:32]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-06-29 21:14:48]

Patchset 3 (id:??) landed as
https://crrev.com/890525d7cba5be038e640305f3199bac2bd964ef
Cr-Commit-Position: refs/heads/master@{#336641}