[webkit-reviews] review denied: [Bug 23849] Keyboard shortcuts needed in Web Inspector : [Attachment 32002] Add Firebug-compatible debugger and stack navigation shortcuts.

[bugzilla-daemon at webkit.org]

Timothy Hatcher <timothy at hatcher.name> has denied Yury Semikhatsky
<yurys at chromium.org>'s request for review:
Bug 23849: Keyboard shortcuts needed in Web Inspector
https://bugs.webkit.org/show_bug.cgi?id=23849

Attachment 32002: Add Firebug-compatible debugger and stack navigation
shortcuts.
https://bugs.webkit.org/attachment.cgi?id=32002&action=review

------- Additional Comments from Timothy Hatcher <timothy at hatcher.name>

> +    this.shortcuts_ = {};

Why does this end with an underscore? THis dosen't match any of our naming
conventions. Just remove the underscore or use a underscore prefix if the
property is "private".

> +    handleKeyEvent: function(event) {

Open curly brace should be on the next line.

> +	 var shortcut = WebInspector.KeyboardShortcut.makeKeyFromEvent(event);
> +	 var handler = this.shortcuts_[shortcut];
> +	 if (handler) {
> +	   handler(event);
> +	   event.preventDefault();
> +	   event.handled = true;
> +	 }
> +    },

Use 4 space indents.

> +    _selectNextCallFrameOnStack: function() {

Open curly brace should be on the next line.

> +    _selectPreviousCallFrameOnStack: function() {

Open curly brace should be on the next line.

> +    _selectedPlacardByIndex: function(index) {

Open curly brace should be on the next line.

> +    _selectedCallFrameIndex: function() {

Open curly brace should be on the next line.

> +	       if (placard.callFrame === this._selectedCallFrame) {
> +		   return i;
> +	       }

No need for the curly brances for one line.

> +WebInspector.KeyboardShortcut.Modifiers = {
> +    NONE: 0,
> +    SHIFT: 1,
> +    CTRL: 2,
> +    ALT: 4,
> +    META: 8
> +};

No need for the semicolon. I would also like to see these be CamelCase and
spelled out.

> +WebInspector.KeyboardShortcut.KeyCodes = {
> +    ESC: 27,
> +    SPACE: 32,
> +    PAGE_UP: 33,	// also NUM_NORTH_EAST
> +    PAGE_DOWN: 34,	// also NUM_SOUTH_EAST
> +    END: 35, 	// also NUM_SOUTH_WEST
> +    HOME: 36,	// also NUM_NORTH_WEST
> +    LEFT: 37,	// also NUM_WEST
> +    UP: 38,		// also NUM_NORTH
> +    RIGHT: 39,	// also NUM_EAST
> +    DOWN: 40,	// also NUM_SOUTH
> +    F1: 112,
> +    F2: 113,
> +    F3: 114,
> +    F4: 115,
> +    F5: 116,
> +    F6: 117,
> +    F7: 118,
> +    F8: 119,
> +    F9: 120,
> +    F10: 121,
> +    F11: 122,
> +    F12: 123,
> +    SEMICOLON: 186,
> +    COMMA: 188,
> +    PERIOD: 190,
> +    SLASH: 191,
> +    APOSTROPHE: 192,
> +    SINGLE_QUOTE: 222,
> +};

Same here. CamelCase and spelled out for the non function keys.

> +WebInspector.KeyboardShortcut.makeKey = function(keyCode, opt_modifiers) {

Open curly brace should be on the next line. The opt_modifiers parameter should
be spelled out and camelCase instead of an underscore.

> +    for (var i = 1; i < arguments.length; i++) {
> +	   modifiers |= arguments[i];
> +    }

No need for the curly brances for one line.

> +WebInspector.KeyboardShortcut.makeKeyFromCodeAndModifiers_ =
function(keyCode, modifiers) {
> +    return (keyCode & 255) | (modifiers << 8);
> +};

The underscore should be a prefix if this is a "private" function.

> +    this.shortcuts_ = {};

Use an underscore prefix or drop the underscore.

> +    handleKeyEvent: function(event) {

Open curly brace should be on the next line.

> +	 var shortcut = WebInspector.KeyboardShortcut.makeKeyFromEvent(event);
> +	 var handler = this.shortcuts_[shortcut];
> +	 if (handler) {
> +	   handler(event);
> +	   event.preventDefault();
> +	   event.handled = true;
> +	 } else {
> +	   this.sidebarPanes.callstack.handleKeyEvent(event);
> +	 }
> +    },

Use 4 space indent.

r- purely on code style. Functionally correct!