Changed the constraints for hiding the top controls

Changed the constraints for hiding the top controls

Currently, for hiding the top controls we check if the IME is
hidden. But with that we cannot prevent the top controls from
hiding while using a physical keyboard and the input field is
focused by the js. Instead we should check if any input field
is focused in the content and disallow hiding of top controls.

BUG=504355
Committed: https://crrev.com/0e485126312877aa0843b938b242e78a22edee35
Cr-Commit-Position: refs/heads/master@{#337783}

[raghu, 2015-06-26 12:10:50]

r.ghatage@samsung.com changed reviewers:
+ ajith.v@samsung.com

[AKVT, 2015-06-26 12:22:09]

peer review looks good to me!

[raghu, 2015-06-26 12:27:25]

r.ghatage@samsung.com changed reviewers:
+ aurimas@chromium.org, jdduke@chromium.org, tedchoc@chromium.org
- ajith.v@samsung.com

[raghu, 2015-06-26 12:27:25]

Hi tedchoc, 
PTAL.

[jdduke (slow), 2015-06-26 15:03:08]

https://codereview.chromium.org/1209243003/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/Tab.java (right):

https://codereview.chromium.org/1209243003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/Tab.java:2476:
enableHidingTopControls &= !mContentViewCore.isFocusedNodeEditable();
With this change I think we still need a way to notify Tab when the editability
of the focused area changes (so that it can update the top controls
constraints).

[Ted C, 2015-06-26 16:49:53]

https://codereview.chromium.org/1209243003/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/Tab.java (right):

https://codereview.chromium.org/1209243003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/Tab.java:2476:
enableHidingTopControls &= !mContentViewCore.isFocusedNodeEditable();
On 2015/06/26 15:03:08, jdduke wrote:
> With this change I think we still need a way to notify Tab when the
editability
> of the focused area changes (so that it can update the top controls
> constraints).

Agreed.  And while this feature is woefully under tested, it would be good to
write some basic tests ensuring this works correctly.

[raghu, 2015-06-29 15:15:22]

On 2015/06/26 16:49:53, Ted C wrote:
>
https://codereview.chromium.org/1209243003/diff/1/chrome/android/java/src/org...
> File chrome/android/java/src/org/chromium/chrome/browser/Tab.java (right):
> 
>
https://codereview.chromium.org/1209243003/diff/1/chrome/android/java/src/org...
> chrome/android/java/src/org/chromium/chrome/browser/Tab.java:2476:
> enableHidingTopControls &= !mContentViewCore.isFocusedNodeEditable();
> On 2015/06/26 15:03:08, jdduke wrote:
> > With this change I think we still need a way to notify Tab when the
> editability
> > of the focused area changes (so that it can update the top controls
> > constraints).
> 
> Agreed.  And while this feature is woefully under tested, it would be good to
> write some basic tests ensuring this works correctly.

@Ted,
I have written a basic test to ensure that top controls don't hide if input
field is focused.
But I have some difficulty in getting the negative test working. i.e. after
loosing the focus on the input field we should be able to hide the top controls.
I tried clicking outside the input field to loose focus, but doesn't work.
Can you please suggest me.

[jdduke (slow), 2015-06-29 15:25:16]

https://codereview.chromium.org/1209243003/diff/20001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/Tab.java (right):

https://codereview.chromium.org/1209243003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/Tab.java:517: if
(getFullscreenManager() == null) return;
So, you'd probably need to change this client method to something like
|onFocusedNodeEditabilityChanged(bool editable)|, and plumb it through from
ContentViewCore.updateImeAdapter.

[raghu, 2015-06-30 12:38:34]

Done the changes as per the review comments. Got the test code working too.
PTAL.

https://codereview.chromium.org/1209243003/diff/20001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/Tab.java (right):

https://codereview.chromium.org/1209243003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/Tab.java:517: if
(getFullscreenManager() == null) return;
On 2015/06/29 15:25:16, jdduke wrote:
> So, you'd probably need to change this client method to something like
> |onFocusedNodeEditabilityChanged(bool editable)|, and plumb it through from
> ContentViewCore.updateImeAdapter.

Done.

[raghu, 2015-07-01 13:07:59]

jdduke@, PTAL

[jdduke (slow), 2015-07-01 16:02:04]

Trying to think of any cases where we we'd focus an editable node but wouldn't
want the topcontrols to show. What about if there's a hardware keyboard? Or
would we not get an "updateImeAdapter" in that case?

Otherwise this looks reasonable, I'll defer to aurimas@ and tedchoc@ for final
judgement.

https://codereview.chromium.org/1209243003/diff/40001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/Tab.java (right):

https://codereview.chromium.org/1209243003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/Tab.java:192: private
boolean mFocusedNodeEditable;
Do we need to cache this? Can we just pull it from ContentViewCore when
necessary?

https://codereview.chromium.org/1209243003/diff/40001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/1209243003/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:745:
public void onDismissInput() {
Can we then get rid of this delegate method entirely?

[raghu, 2015-07-01 17:16:40]

On 2015/07/01 16:02:04, jdduke wrote:
> Trying to think of any cases where we we'd focus an editable node but wouldn't
> want the topcontrols to show. What about if there's a hardware keyboard? Or
> would we not get an "updateImeAdapter" in that case?

|updateImeAdapter| does get called even when a hardware keyboard is there
and the topcontrols would hide in that case too. As per the discussion on the
bug
it seems to be the right behavior. will wait for aurimas@ and tedchoc@ to
confirm.

> 
> Otherwise this looks reasonable, I'll defer to aurimas@ and tedchoc@ for final
> judgement.
> 
>
https://codereview.chromium.org/1209243003/diff/40001/chrome/android/java/src...
> File chrome/android/java/src/org/chromium/chrome/browser/Tab.java (right):
> 
>
https://codereview.chromium.org/1209243003/diff/40001/chrome/android/java/src...
> chrome/android/java/src/org/chromium/chrome/browser/Tab.java:192: private
> boolean mFocusedNodeEditable;
> Do we need to cache this? Can we just pull it from ContentViewCore when
> necessary?
> 
>
https://codereview.chromium.org/1209243003/diff/40001/content/public/android/...
> File
>
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
> (right):
> 
>
https://codereview.chromium.org/1209243003/diff/40001/content/public/android/...
>
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:745:
> public void onDismissInput() {
> Can we then get rid of this delegate method entirely?

[raghu, 2015-07-01 17:16:57]

https://codereview.chromium.org/1209243003/diff/40001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/Tab.java (right):

https://codereview.chromium.org/1209243003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/Tab.java:192: private
boolean mFocusedNodeEditable;
On 2015/07/01 16:02:04, jdduke wrote:
> Do we need to cache this? Can we just pull it from ContentViewCore when
> necessary?

We can pull it from the |isFocusedNodeEditable()| method of contentviewcore as I
did in patch set 2. But now since we have a client callback coming to us with
this value, I cached it here. I shall remove it.

https://codereview.chromium.org/1209243003/diff/40001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/1209243003/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:745:
public void onDismissInput() {
On 2015/07/01 16:02:04, jdduke wrote:
> Can we then get rid of this delegate method entirely?

Acknowledged.

[raghu, 2015-07-01 21:54:21]

Jdduke@, PTAL

[raghu, 2015-07-01 21:55:52]

aurimas@ 
PTAL

[raghu, 2015-07-06 17:29:37]

jdduke@, can you please review my patch.

[jdduke (slow), 2015-07-06 18:03:56]

On 2015/07/06 17:29:37, raghu wrote:
> jdduke@, can you please review my patch.

lgtm, but please wait for tedchoc@ to approve.

[raghu, 2015-07-07 00:47:58]

On 2015/07/06 18:03:56, jdduke wrote:
> On 2015/07/06 17:29:37, raghu wrote:
> > jdduke@, can you please review my patch.
> 
> lgtm, but please wait for tedchoc@ to approve.

Thanks. 

tedchoc@, can you please review my patch.

[Ted C, 2015-07-07 16:46:33]

lgtm

https://codereview.chromium.org/1209243003/diff/60001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/fullscreen/FullscreenManagerTest.java
(right):

https://codereview.chromium.org/1209243003/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/fullscreen/FullscreenManagerTest.java:355:
singleClickView(tab.getView());
This line "could" end up causing issues if the click happens to land within the
bounds of the input text, but I think it is fine and we can monitor the test to
see if it flakes.

https://codereview.chromium.org/1209243003/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/fullscreen/FullscreenManagerTest.java:527:
return CriteriaHelper.pollForCriteria(new Criteria() {
use pollForUIThreadCriteria here instead.  then you can get rid of the
ThreadUtils block in isSatisified.

[raghu, 2015-07-07 22:24:15]

https://codereview.chromium.org/1209243003/diff/60001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/fullscreen/FullscreenManagerTest.java
(right):

https://codereview.chromium.org/1209243003/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/fullscreen/FullscreenManagerTest.java:527:
return CriteriaHelper.pollForCriteria(new Criteria() {
On 2015/07/07 16:46:33, Ted C wrote:
> use pollForUIThreadCriteria here instead.  then you can get rid of the
> ThreadUtils block in isSatisified.

Acknowledged.
Would you like me to do it for all the other methods in this file too?

[Ted C, 2015-07-07 22:26:50]

https://codereview.chromium.org/1209243003/diff/60001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/fullscreen/FullscreenManagerTest.java
(right):

https://codereview.chromium.org/1209243003/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/fullscreen/FullscreenManagerTest.java:527:
return CriteriaHelper.pollForCriteria(new Criteria() {
On 2015/07/07 22:24:15, raghu wrote:
> On 2015/07/07 16:46:33, Ted C wrote:
> > use pollForUIThreadCriteria here instead.  then you can get rid of the
> > ThreadUtils block in isSatisified.
> 
> Acknowledged.
> Would you like me to do it for all the other methods in this file too?

Nah, just this one.  The other ones should be done in a separate change to not
confuse the purpose of this one.

[raghu, 2015-07-08 07:27:08]

The CQ bit was checked by r.ghatage@samsung.com

[raghu, 2015-07-08 07:27:08]

The patchset sent to the CQ was uploaded after l-g-t-m from
tedchoc@chromium.org, jdduke@chromium.org
Link to the patchset: https://codereview.chromium.org/1209243003/#ps80001
(title: "Used pollForUIThreadCriteria instead of pollForCriteria in the test
code")

[commit-bot: I haz the power, 2015-07-08 07:27:24]

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

[commit-bot: I haz the power, 2015-07-08 08:26:44]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-07-08 08:27:31]

Patchset 5 (id:??) landed as
https://crrev.com/0e485126312877aa0843b938b242e78a22edee35
Cr-Commit-Position: refs/heads/master@{#337783}