[Android] Fix refresh keyboard shortcut behavior.

[Android] Fix refresh keyboard shortcut behavior.

This ensures the omnibox suggestions are dismissed, and it either
keeps the focus of the omnibox if on the NTP or focuses the page (to
keep it consistent with desktop).

BUG=527041
Committed: https://crrev.com/051086f3c3dbd8d3acc70e4161fd5834ff1e7567
Cr-Commit-Position: refs/heads/master@{#353102}

[Ted C, 2015-09-14 23:43:11]

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

[Ted C, 2015-09-14 23:43:11]

PTAL

[newt (away), 2015-09-24 03:27:55]

A belated review... better late than never, right?

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

https://codereview.chromium.org/1347543002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/KeyboardShortcuts.java:245:
&& tab.getWebContents().focusLocationBarByDefault()) {
Why jump through the web contents and web contents delegate to decide whether
the location bar should be focused by default? Why not just use
NewTabPage.isNTPUrl(tab.getUrl()) or tab.isNativePage()?

[Ted C, 2015-09-24 21:24:34]

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

https://codereview.chromium.org/1347543002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/KeyboardShortcuts.java:245:
&& tab.getWebContents().focusLocationBarByDefault()) {
On 2015/09/24 03:27:55, newt wrote:
> Why jump through the web contents and web contents delegate to decide whether
> the location bar should be focused by default? Why not just use
> NewTabPage.isNTPUrl(tab.getUrl()) or tab.isNativePage()?

This was to be consistent w/ desktop, which uses the same logic.

[newt (away), 2015-09-25 17:17:18]

On 2015/09/24 21:24:34, Ted C wrote:
>
https://codereview.chromium.org/1347543002/diff/1/chrome/android/java/src/org...
> File
chrome/android/java/src/org/chromium/chrome/browser/KeyboardShortcuts.java
> (right):
> 
>
https://codereview.chromium.org/1347543002/diff/1/chrome/android/java/src/org...
>
chrome/android/java/src/org/chromium/chrome/browser/KeyboardShortcuts.java:245:
> && tab.getWebContents().focusLocationBarByDefault()) {
> On 2015/09/24 03:27:55, newt wrote:
> > Why jump through the web contents and web contents delegate to decide
whether
> > the location bar should be focused by default? Why not just use
> > NewTabPage.isNTPUrl(tab.getUrl()) or tab.isNativePage()?
> 
> This was to be consistent w/ desktop, which uses the same logic.

But we're not actually sharing code and it complicates things considerably. Does
the consistency have a benefit here? I'm inclined to go with the simpler
approach.

[Ted C, 2015-09-25 17:43:26]

On 2015/09/25 17:17:18, newt wrote:
> On 2015/09/24 21:24:34, Ted C wrote:
> >
>
https://codereview.chromium.org/1347543002/diff/1/chrome/android/java/src/org...
> > File
> chrome/android/java/src/org/chromium/chrome/browser/KeyboardShortcuts.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/1347543002/diff/1/chrome/android/java/src/org...
> >
>
chrome/android/java/src/org/chromium/chrome/browser/KeyboardShortcuts.java:245:
> > && tab.getWebContents().focusLocationBarByDefault()) {
> > On 2015/09/24 03:27:55, newt wrote:
> > > Why jump through the web contents and web contents delegate to decide
> whether
> > > the location bar should be focused by default? Why not just use
> > > NewTabPage.isNTPUrl(tab.getUrl()) or tab.isNativePage()?
> > 
> > This was to be consistent w/ desktop, which uses the same logic.
> 
> But we're not actually sharing code and it complicates things considerably.
Does
> the consistency have a benefit here? I'm inclined to go with the simpler
> approach.

If you want to fix the behavior on all platforms, you can look in the same
general place (i.e. the delegate).  If we do it in android UI code, then
you have to just start searching around an hope you find the right place.

Keeping things consistent across ports is a very good thing in my opinion.

[newt (away), 2015-09-25 18:37:53]

On 2015/09/25 17:43:26, Ted C wrote:
> On 2015/09/25 17:17:18, newt wrote:
> > On 2015/09/24 21:24:34, Ted C wrote:
> > >
> >
>
https://codereview.chromium.org/1347543002/diff/1/chrome/android/java/src/org...
> > > File
> > chrome/android/java/src/org/chromium/chrome/browser/KeyboardShortcuts.java
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1347543002/diff/1/chrome/android/java/src/org...
> > >
> >
>
chrome/android/java/src/org/chromium/chrome/browser/KeyboardShortcuts.java:245:
> > > && tab.getWebContents().focusLocationBarByDefault()) {
> > > On 2015/09/24 03:27:55, newt wrote:
> > > > Why jump through the web contents and web contents delegate to decide
> > whether
> > > > the location bar should be focused by default? Why not just use
> > > > NewTabPage.isNTPUrl(tab.getUrl()) or tab.isNativePage()?
> > > 
> > > This was to be consistent w/ desktop, which uses the same logic.
> > 
> > But we're not actually sharing code and it complicates things considerably.
> Does
> > the consistency have a benefit here? I'm inclined to go with the simpler
> > approach.
> 
> If you want to fix the behavior on all platforms, you can look in the same
> general place (i.e. the delegate).  If we do it in android UI code, then
> you have to just start searching around an hope you find the right place.
> 
> Keeping things consistent across ports is a very good thing in my opinion.

Agreed in general about keeping things consistent, but I'm not convinced that it
makes the code simpler or more maintainable here. I'd be more swayed if there
was a plan to somehow share a significant portion of the keyboard code (but I'm
not sure that's feasible).

Anyway, lgtm either way.

[Ted C, 2015-10-08 17:10:13]

The CQ bit was checked by tedchoc@chromium.org

[Ted C, 2015-10-08 17:10:14]

The patchset sent to the CQ was uploaded after l-g-t-m from newt@chromium.org
Link to the patchset: https://codereview.chromium.org/1347543002/#ps20001
(title: "Rebase")

[commit-bot: I haz the power, 2015-10-08 17:13:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1347543002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1347543002/20001

[commit-bot: I haz the power, 2015-10-08 18:40:15]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-10-08 18:41:16]

Patchset 2 (id:??) landed as
https://crrev.com/051086f3c3dbd8d3acc70e4161fd5834ff1e7567
Cr-Commit-Position: refs/heads/master@{#353102}