Create a new custom class that handles keyboard shortcuts for Custom Tab

Create a new custom class that handles keyboard shortcuts for Custom Tab

This CL opens some methods inside of KeyboardShortcuts for reusability.
Also a new class, CustoMTabKeyboardShortcuts is created to handle the
small subset of shortcuts that custom tab supports.

BUG=527196
Committed: https://crrev.com/730fcbedc16dbea063e664d04f64736189d6fc7b
Cr-Commit-Position: refs/heads/master@{#350955}

[Ian Wen, 2015-09-22 22:19:04]

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

[Ian Wen, 2015-09-22 22:19:04]

PTAL

[Yusuf, 2015-09-24 05:26:26]

less code duplication pls?

See crbug.com/513770 where newt described a few alternatives. AFAICT this uses
neither and essentially copies over all the logic from KeyboardShortcuts.

I am not feeling strongly about any in particular, but I think this patch should
at least move in the direction of one of them. Maybe start with adding a common
class with shared implementations for some key strokes at least?

[newt (away), 2015-09-24 20:07:40]

On 2015/09/24 05:26:26, Yusuf wrote:
> less code duplication pls?
> 
> See crbug.com/513770 where newt described a few alternatives. AFAICT this uses
> neither and essentially copies over all the logic from KeyboardShortcuts.
> 
> I am not feeling strongly about any in particular, but I think this patch
should
> at least move in the direction of one of them. Maybe start with adding a
common
> class with shared implementations for some key strokes at least?

Indeed. Otherwise this class will quickly get out of sync with
KeyboardShortcuts. (For example, as soon as Ted's Ctrl+Shift+R CL lands:
https://codereview.chromium.org/1347543002/)

I think the "mode" idea that I mentioned in
https://code.google.com/p/chromium/issues/detail?id=513770#c4 might be the best
option. Or, just adding another boolean parameter or two to onKeyDown().

[Ian Wen, 2015-09-24 22:06:19]

Adopted a simpler approach and removed duplicated code. PTAL. :)

[newt (away), 2015-09-25 20:56:01]

Remember to update the commit message. Also a few comments inline.

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

https://codereview.chromium.org/1365453004/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/KeyboardShortcuts.java:140:
String url = keyCode == KeyEvent.KEYCODE_B
Don't you need to replace lines 140-146 with
"activity.onMenuOrKeyboardAction(R.id.all_bookmarks_menu_id or
R.id.history_menu_id)"? to prevent these shortcuts from functioning in
CustomTabActivity?

Also, now that these two keyboard shortcuts only apply when
"isCurrentTabVisible" is true, you should move the handling for these shortcuts
inside the big "if (isCurrentTabVisible)" statement starting on line 154.

https://codereview.chromium.org/1365453004/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java
(right):

https://codereview.chromium.org/1365453004/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:422:
} else if (id == R.id.focus_url_bar) {
I'd group together all the disabled keyboard shortcuts and add a single comment
explaining that returning true effectively disables these shortcuts.

  // Disable blah blah
  if (id == R.id.all_bookmarks_menu_id
          || id == R.id.bookmark_this_page_id
          || ...) {
      return true;
  }

[Ian Wen, 2015-09-25 23:15:23]

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

https://codereview.chromium.org/1365453004/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/KeyboardShortcuts.java:140:
String url = keyCode == KeyEvent.KEYCODE_B
On 2015/09/25 20:56:00, newt wrote:
> Don't you need to replace lines 140-146 with
> "activity.onMenuOrKeyboardAction(R.id.all_bookmarks_menu_id or
> R.id.history_menu_id)"? to prevent these shortcuts from functioning in
> CustomTabActivity?
> 
> Also, now that these two keyboard shortcuts only apply when
> "isCurrentTabVisible" is true, you should move the handling for these
shortcuts
> inside the big "if (isCurrentTabVisible)" statement starting on line 154.

Done. Moved this piece of code downwards.

https://codereview.chromium.org/1365453004/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java
(right):

https://codereview.chromium.org/1365453004/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:422:
} else if (id == R.id.focus_url_bar) {
On 2015/09/25 20:56:00, newt wrote:
> I'd group together all the disabled keyboard shortcuts and add a single
comment
> explaining that returning true effectively disables these shortcuts.
> 
>   // Disable blah blah
>   if (id == R.id.all_bookmarks_menu_id
>           || id == R.id.bookmark_this_page_id
>           || ...) {
>       return true;
>   }

Done.

[newt (away), 2015-09-25 23:19:32]

lgtm

[Ian Wen, 2015-09-25 23:20:12]

The CQ bit was checked by ianwen@chromium.org

[commit-bot: I haz the power, 2015-09-25 23:20:53]

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

[commit-bot: I haz the power, 2015-09-26 00:07:55]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-09-26 00:08:42]

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