[Android] Support unfocusable container views.

[Android] Support unfocusable container views.

If the ContentViewCore container view is not focusable (which can
happen when it's emebedded in an app via android webview), we
were clearing the RWHVA notion of focus when the RenderView was
swapped out. As the view is never focusable again, chromium never
considers this view focused and as such, text selection did not
function.

We support this type of container view by always treating it focused
from Chromium's point of view.

BUG=430859
(internal b/18066784)

Committed: https://crrev.com/9074d9fc9119ef4d2223f6c993d1a1b67a0bea7c
Cr-Commit-Position: refs/heads/master@{#303071}

[benm (inactive), 2014-11-05 20:20:45]

benm@chromium.org changed reviewers:
+ jdduke@chromium.org

[benm (inactive), 2014-11-05 20:20:45]

ptal!

[jdduke (slow), 2014-11-05 21:20:31]

https://codereview.chromium.org/701233002/diff/1/content/browser/android/cont...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/701233002/diff/1/content/browser/android/cont...
content/browser/android/content_view_core_impl.cc:851: LOG(WARN) << "Unfocusable
container view, treating as always focused.";
Hmm, we know that this condition exists, will the warning be anything more than
logspam at this point? I'd almost prefer a short comment explaining when this
condition might exist, and if we have follow-up plans then a link to the crbug.

Also, I wonder if it would make sense to instead embed this logic in
ContentViewCore.hasFocus()? Then other |HasFocus()| queries via RWHVAndroid
should just work.

[benm (inactive), 2014-11-06 10:48:24]

On 2014/11/05 21:20:31, jdduke wrote:
>
https://codereview.chromium.org/701233002/diff/1/content/browser/android/cont...
> File content/browser/android/content_view_core_impl.cc (right):
> 
>
https://codereview.chromium.org/701233002/diff/1/content/browser/android/cont...
> content/browser/android/content_view_core_impl.cc:851: LOG(WARN) <<
"Unfocusable
> container view, treating as always focused.";
> Hmm, we know that this condition exists, will the warning be anything more
than
> logspam at this point? I'd almost prefer a short comment explaining when this
> condition might exist, and if we have follow-up plans then a link to the
crbug.
> 
> Also, I wonder if it would make sense to instead embed this logic in
> ContentViewCore.hasFocus()? Then other |HasFocus()| queries via RWHVAndroid
> should just work.

Yep, that sounds reasonable and will be a much smaller change :-)

[jdduke (slow), 2014-11-06 16:34:49]

lgtm with one potential change.

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

https://codereview.chromium.org/701233002/diff/40001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2367:
public boolean hasFocus() {
I *think* we can make this private, at least, grepping doesn't show any usage
and a few local builds (including downstream) seemed to check out...

[benm (inactive), 2014-11-06 17:55:58]

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

https://codereview.chromium.org/701233002/diff/40001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2367:
public boolean hasFocus() {
On 2014/11/06 16:34:49, jdduke wrote:
> I *think* we can make this private, at least, grepping doesn't show any usage
> and a few local builds (including downstream) seemed to check out...

Done.

[benm (inactive), 2014-11-06 18:01:58]

The CQ bit was checked by benm@chromium.org

[commit-bot: I haz the power, 2014-11-06 18:03:06]

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

[commit-bot: I haz the power, 2014-11-06 19:35:40]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2014-11-06 19:36:29]

Patchset 4 (id:??) landed as
https://crrev.com/9074d9fc9119ef4d2223f6c993d1a1b67a0bea7c
Cr-Commit-Position: refs/heads/master@{#303071}

[Nicolas Zea, 2014-11-06 22:34:52]

On 2014/11/06 19:36:29, I haz the power (commit-bot) wrote:
> Patchset 4 (id:??) landed as
> https://crrev.com/9074d9fc9119ef4d2223f6c993d1a1b67a0bea7c
> Cr-Commit-Position: refs/heads/master@{#303071}

Looks like this introduces flake into content_browsertests' WebRTCBrowserTest.*
(see the android_dbg_tests_recipe trybot result in the last patchset). Also see
buildbot failure is at
http://build.chromium.org/p/chromium.linux/buildstatus?builder=Android%20Test....
Reverting.

[Nicolas Zea, 2014-11-06 22:37:35]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/712433002/ by zea@chromium.org.

The reason for reverting is: Introduced flake into WebRTCBrowserTest.*
http://build.chromium.org/p/chromium.linux/buildstatus?builder=Android%20Test...

Example:
Value of: ExecuteScriptAndExtractString(shell()->web_contents(), javascript,
&result)
  Actual: false
Expected: true
From javascript: ../../content/test/webrtc_content_browsertest_base.cc:71:
Failure
Failed
[INFO:CONSOLE(883)] "Receiving remote stream...", source:
http://127.0.0.1:50076/media/peerconnection-call.html (883)
[INFO:CONSOLE(859)] "Receiving answer...", source:
http://127.0.0.1:50076/media/peerconnection-call.html (859)
[  FAILED  ] WebRtcBrowserTest.AddTwoMediaStreamsToOnePC, where TypeParam =  and
GetParam() =  (2180 ms).

[Nicolas Zea, 2014-11-06 22:55:50]

Some more investigation revealed that the flakiness pre-existed this patch. I'll
reland.

[benm (inactive), 2014-11-07 08:46:54]

OK. Thanks for relanding. Can you link to the reland cl so that I can be
sure it makes m40.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[benm (inactive), 2014-11-07 08:48:22]

Ah, I see it further up my inbox. Thanks :-)

On Fri, 7 Nov 2014 08:46 Ben Murdoch <benm@chromium.org> wrote:

> OK. Thanks for relanding. Can you link to the reland cl so that I can be
> sure it makes m40.
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.