DevTools: Forward whitelisted unhandled shortcuts from inspected page into DevTools frontend

DevTools: Forward whitelisted unhandled shortcuts from inspected page into DevTools frontend

This will allow to freeze the inspected page,
so that the focus loss will not alter its state.

This is the Chromium part of the change.

R=pfeldman
BUG=160510
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=262492

[apavlov, 2014-04-04 15:01:16]

jochen@chromium.org: Please review changes in browser.cc

tsepez@chromium.org: Please review changes in
devtools_embedder_message_dispatcher.h/cc

[Tom Sepez, 2014-04-07 18:01:13]

https://codereview.chromium.org/225973003/diff/1/chrome/browser/devtools/devt...
File chrome/browser/devtools/devtools_window.cc (right):

https://codereview.chromium.org/225973003/diff/1/chrome/browser/devtools/devt...
chrome/browser/devtools/devtools_window.cc:1237: static inline int
hashKeyCodeAndModifiers(int keyCode, int modifiers) {
nit: this isn't really a "hash" per-se, in that it encompasses all the
information needed to make the test later on without the possibility of
collision. Maybe combineKeyCodeAndModifiers()?

https://codereview.chromium.org/225973003/diff/1/chrome/browser/devtools/devt...
chrome/browser/devtools/devtools_window.cc:1241: void
DevToolsWindow::SetWhitelistedShortcuts(const std::string& message) {
You'll probably want to check here that the keycode the devtools is asking to be
whitelisted is sane, just because we can.  In other words, it should have
modifiers or some such.  It might be bad if the devtools could steal every
keystroke, even for things like normal letters and numbers, no?

https://codereview.chromium.org/225973003/diff/1/chrome/browser/devtools/devt...
chrome/browser/devtools/devtools_window.cc:1573: static inline int
windowsVirtualKeyCodeWithoutLocation(int keycode)
nit:  if you want to call this windows... I'd expect to see an #ifdef windows
around it. But clearly this benign in all cases, even on linux, and the name
means that only windows is (persumably dumb enough) to care about which ctrl key
we pressed (so as to enable insane different behaviours for each key as
desired). So the limited bloat probably outweighs the ugliness of the #ifdefs. 
I'd call it virtualKeyCodeWithoutLocation() though; this would be fine without
any ifdef's.

[apavlov, 2014-04-08 10:11:31]

Thanks for the useful comments, Tom!

https://codereview.chromium.org/225973003/diff/1/chrome/browser/devtools/devt...
File chrome/browser/devtools/devtools_window.cc (right):

https://codereview.chromium.org/225973003/diff/1/chrome/browser/devtools/devt...
chrome/browser/devtools/devtools_window.cc:1237: static inline int
hashKeyCodeAndModifiers(int keyCode, int modifiers) {
On 2014/04/07 18:01:14, Tom Sepez wrote:
> nit: this isn't really a "hash" per-se, in that it encompasses all the
> information needed to make the test later on without the possibility of
> collision. Maybe combineKeyCodeAndModifiers()?

Done.

https://codereview.chromium.org/225973003/diff/1/chrome/browser/devtools/devt...
chrome/browser/devtools/devtools_window.cc:1241: void
DevToolsWindow::SetWhitelistedShortcuts(const std::string& message) {
On 2014/04/07 18:01:14, Tom Sepez wrote:
> You'll probably want to check here that the keycode the devtools is asking to
be
> whitelisted is sane, just because we can.  In other words, it should have
> modifiers or some such.  It might be bad if the devtools could steal every
> keystroke, even for things like normal letters and numbers, no?

Good point. For now, let's allow keys with modifiers and F-keys. Sounds good?

https://codereview.chromium.org/225973003/diff/1/chrome/browser/devtools/devt...
chrome/browser/devtools/devtools_window.cc:1573: static inline int
windowsVirtualKeyCodeWithoutLocation(int keycode)
On 2014/04/07 18:01:14, Tom Sepez wrote:
> nit:  if you want to call this windows... I'd expect to see an #ifdef windows
> around it. But clearly this benign in all cases, even on linux, and the name
> means that only windows is (persumably dumb enough) to care about which ctrl
key
> we pressed (so as to enable insane different behaviours for each key as
> desired). So the limited bloat probably outweighs the ugliness of the #ifdefs.

> I'd call it virtualKeyCodeWithoutLocation() though; this would be fine without
> any ifdef's.

Yeah, I just wanted to make it consistent with the Blink code (where I sto..
borrowed this function from.) But guess just having a comment referring to the
origin will be fine.

[pfeldman, 2014-04-08 12:01:20]

lgtm with nits.

https://codereview.chromium.org/225973003/diff/20001/chrome/browser/devtools/...
File chrome/browser/devtools/devtools_window.cc (right):

https://codereview.chromium.org/225973003/diff/20001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_window.cc:1238: static inline int
combineKeyCodeAndModifiers(int key_code, int modifiers) {
Lets move it all into a separate class? Could be local.

https://codereview.chromium.org/225973003/diff/20001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_window.cc:1268: whitelisted_keys_.push_back(
Could you use a set?

https://codereview.chromium.org/225973003/diff/20001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_window.cc:1609: event_type = "keydown";
lets extract some constants.

https://codereview.chromium.org/225973003/diff/20001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_window.cc:1620: if (std::find(
lets use a set.

https://codereview.chromium.org/225973003/diff/20001/chrome/browser/ui/browse...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/225973003/diff/20001/chrome/browser/ui/browse...
chrome/browser/ui/browser.cc:1153:
DevToolsWindow::GetInstanceForInspectedRenderViewHost(
Lets introduce GetInstanceForInspectedWebContents

[apavlov, 2014-04-08 13:28:12]

https://codereview.chromium.org/225973003/diff/20001/chrome/browser/devtools/...
File chrome/browser/devtools/devtools_window.cc (right):

https://codereview.chromium.org/225973003/diff/20001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_window.cc:1238: static inline int
combineKeyCodeAndModifiers(int key_code, int modifiers) {
On 2014/04/08 12:01:20, pfeldman wrote:
> Lets move it all into a separate class? Could be local.

Done.

https://codereview.chromium.org/225973003/diff/20001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_window.cc:1268: whitelisted_keys_.push_back(
On 2014/04/08 12:01:20, pfeldman wrote:
> Could you use a set?

Done.

https://codereview.chromium.org/225973003/diff/20001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_window.cc:1609: event_type = "keydown";
On 2014/04/08 12:01:20, pfeldman wrote:
> lets extract some constants.

Done.

https://codereview.chromium.org/225973003/diff/20001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_window.cc:1620: if (std::find(
On 2014/04/08 12:01:20, pfeldman wrote:
> lets use a set.

Done.

https://codereview.chromium.org/225973003/diff/20001/chrome/browser/ui/browse...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/225973003/diff/20001/chrome/browser/ui/browse...
chrome/browser/ui/browser.cc:1153:
DevToolsWindow::GetInstanceForInspectedRenderViewHost(
On 2014/04/08 12:01:20, pfeldman wrote:
> Lets introduce GetInstanceForInspectedWebContents

Done.

[Tom Sepez, 2014-04-08 17:27:39]

LGTM.

[jochen (gone - plz use gerrit), 2014-04-08 17:31:16]

lgtm

[apavlov, 2014-04-08 17:34:01]

The CQ bit was checked by apavlov@chromium.org

[commit-bot: I haz the power, 2014-04-08 17:34:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/apavlov@chromium.org/225973003/60001

[commit-bot: I haz the power, 2014-04-08 20:04:14]

Change committed as 262492