[Webkit-unassigned] [Bug 23849] Keyboard shortcuts needed in Web Inspector

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=23849


timothy at hatcher.name changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #32002|review?(timothy at hatcher.name|review-
               Flag|)                           |




------- Comment #11 from timothy at hatcher.name  2009-06-29 09:58 PDT -------
(From update of attachment 32002)

> +    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!


-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.