DevTools: Small refactoring of InspectorView

Source/devtools/front_end/InspectorView.js

 /*
  * Copyright (C) 2011 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 309 matching lines @@
         if (!WebInspector.isWin() || (!this._openBracketIdentifiers[event.keyIdentifier] && !this._closeBracketIdentifiers[event.keyIdentifier])) {
             this._keyDownInternal(event);
             return;
         }
 
         this._keyDownTimer = setTimeout(this._keyDownInternal.bind(this, event), 0);
     },
 
     _keyDownInternal: function(event)
     {
-        if (this._openBracketIdentifiers[event.keyIdentifier]) {
-            var isRotateLeft = !event.shiftKey && !event.altKey;
-            if (isRotateLeft) {
-                var panelOrder = this._tabbedPane.allTabs();
-                var index = panelOrder.indexOf(this.currentPanel().name);
-                index = (index === 0) ? panelOrder.length - 1 : index - 1;
-                this.showPanel(panelOrder[index]);
-                event.consume(true);
-                return;
-            }
+        var direction = 0;
 
-            var isGoBack = event.altKey;
-            if (isGoBack && this._canGoBackInHistory()) {
-                this._goBackInHistory();
-                event.consume(true);
-            }
-            return;
-        }
+        if (this._openBracketIdentifiers[event.keyIdentifier])
+            direction = -1;
 
-        if (this._closeBracketIdentifiers[event.keyIdentifier]) {
-            var isRotateRight = !event.shiftKey && !event.altKey;
-            if (isRotateRight) {
-                var panelOrder = this._tabbedPane.allTabs();
-                var index = panelOrder.indexOf(this.currentPanel().name);
-                index = (index + 1) % panelOrder.length;
-                this.showPanel(panelOrder[index]);
-                event.consume(true);
-                return;
-            }
+        if (this._closeBracketIdentifiers[event.keyIdentifier])
+            direction = 1;
 
-            var isGoForward = event.altKey;
-            if (isGoForward && this._canGoForwardInHistory()) {
-                this._goForwardInHistory();
-                event.consume(true);
-            }
-            return;
+        if (direction) {
+            this._handlePanelChangeShortcut(event, direction);
+            this._handleHistoryMoveShortcut(event, direction);
         }
     },
 
-    _canGoBackInHistory: function()
+    _handleHistoryMoveShortcut: function(event, move)

[vsevik, 2014/03/04 17:29:56]

I would inline event handling and extract history/panel specific logic only.

[sergeyv, 2014/03/04 17:55:42]

Done.

     {
-        return this._historyIterator > 0;
+        if (event.handled || !event.altKey || !this._isValidMoveInHistory(move))
+            return;
+
+        this._moveInHistory(move);
+        event.consume(true);
     },
 
-    _goBackInHistory: function()
+    _handlePanelChangeShortcut: function(event, direction)

[vsevik, 2014/03/04 17:29:56]

Ditto.

[sergeyv, 2014/03/04 17:55:42]

Done.

+    {
+        if (event.handled || event.shiftKey || event.altKey)
+            return;
+
+        var panelOrder = this._tabbedPane.allTabs();
+        var index = panelOrder.indexOf(this.currentPanel().name);
+        index = (index + panelOrder.length + direction) % panelOrder.length;
+        this.showPanel(panelOrder[index]);
+        event.consume(true);
+    },
+
+    _isValidMoveInHistory: function(move)

[vsevik, 2014/03/04 17:29:56]

This could be inlined into moveInHistory

[sergeyv, 2014/03/04 17:55:42]

Done.

+    {
+        var newIndex = this._historyIterator + move;
+        return newIndex < this._history.length && newIndex >= 0;
+    },
+
+    _moveInHistory: function(move)
     {
         this._inHistory = true;
-        this.setCurrentPanel(WebInspector.panels[this._history[--this._historyIterator]]);
+        this._historyIterator = this._historyIterator + move;
+        this.setCurrentPanel(WebInspector.panels[this._history[this._historyIterator]]);
         delete this._inHistory;
-    },
 
-    _canGoForwardInHistory: function()
-    {
-        return this._historyIterator < this._history.length - 1;
-    },
-
-    _goForwardInHistory: function()
-    {
-        this._inHistory = true;
-        this.setCurrentPanel(WebInspector.panels[this._history[++this._historyIterator]]);
-        delete this._inHistory;
     },
 
     _pushToHistory: function(panelName)
     {
         if (this._inHistory)
             return;
 
         this._history.splice(this._historyIterator + 1, this._history.length - this._historyIterator - 1);
         if (!this._history.length || this._history[this._history.length - 1] !== panelName)
             this._history.push(panelName);
         this._historyIterator = this._history.length - 1;
     },
 
     onResize: function()
     {
         WebInspector.Dialog.modalHostRepositioned();
     },
 
     /**
      * @return {!Element}
      */
     topResizerElement: function()
     {
         return this._tabbedPane.headerElement();
     },
 
+    _createImagedCounterElementIfNeeded: function(count, id, styleName)
+    {
+        if (count) {

[vsevik, 2014/03/04 17:29:56]

if (!count)
    return

[sergeyv, 2014/03/04 17:55:42]

Done.

+            var imageElement = this._errorWarningCountElement.createChild("div", styleName);
+            var counterElement = this._errorWarningCountElement.createChild("span");
+            counterElement.id = id;
+            counterElement.textContent = count;
+        }
+    },
+
     /**
      * @param {number} errors
      * @param {number} warnings
      */
     setErrorAndWarningCounts: function(errors, warnings)
     {
-        if (!errors && !warnings) {
-            this._errorWarningCountElement.classList.add("hidden");
-            this._tabbedPane.headerResized();
-            return;
-        }
-
-        this._errorWarningCountElement.classList.remove("hidden");
+        this._errorWarningCountElement.enableStyleClass("hidden", !errors && !warnings);

[vsevik, 2014/03/04 17:29:56]

classList.toggle

[sergeyv, 2014/03/04 17:55:42]

Done.

         this._errorWarningCountElement.removeChildren();
 
-        if (errors) {
-            var errorImageElement = this._errorWarningCountElement.createChild("div", "error-icon-small");
-            var errorElement = this._errorWarningCountElement.createChild("span");
-            errorElement.id = "error-count";
-            errorElement.textContent = errors;
-        }
+        this._createImagedCounterElementIfNeeded(errors, "error-count", "error-icon-small");
+        this._createImagedCounterElementIfNeeded(warnings, "warning-count", "warning-icon-small");
 
-        if (warnings) {
-            var warningsImageElement = this._errorWarningCountElement.createChild("div", "warning-icon-small");
-            var warningsElement = this._errorWarningCountElement.createChild("span");
-            warningsElement.id = "warning-count";
-            warningsElement.textContent = warnings;
-        }
-
-        if (errors) {
-            if (warnings) {
-                if (errors == 1) {
-                    if (warnings == 1)
-                        this._errorWarningCountElement.title = WebInspector.UIString("%d error, %d warning", errors, warnings);
-                    else
-                        this._errorWarningCountElement.title = WebInspector.UIString("%d error, %d warnings", errors, warnings);
-                } else if (warnings == 1)
-                    this._errorWarningCountElement.title = WebInspector.UIString("%d errors, %d warning", errors, warnings);
-                else
-                    this._errorWarningCountElement.title = WebInspector.UIString("%d errors, %d warnings", errors, warnings);
-            } else if (errors == 1)
-                this._errorWarningCountElement.title = WebInspector.UIString("%d error", errors);
-            else
-                this._errorWarningCountElement.title = WebInspector.UIString("%d errors", errors);
-        } else if (warnings == 1)
-            this._errorWarningCountElement.title = WebInspector.UIString("%d warning", warnings);
-        else if (warnings)
-            this._errorWarningCountElement.title = WebInspector.UIString("%d warnings", warnings);
-        else
-            this._errorWarningCountElement.title = null;
-
+        var errorString = errors ?  WebInspector.UIString("%d error%s", errors, errors > 1 ? "s" : "") : "";
+        var warningString = warnings ?  WebInspector.UIString("%d warning%s", warnings, warnings > 1 ? "s" : "") : "";
+        var commaString = errors & warnings ? ", " : "";

[vsevik, 2014/03/04 17:29:56]

did you mean &&?

[sergeyv, 2014/03/04 17:55:42]

Done.

+        this._errorWarningCountElement.title = errorString + commaString + warningString;
         this._tabbedPane.headerResized();
     },
 
     __proto__: WebInspector.View.prototype
 };
 
 /**
  * @type {!WebInspector.InspectorView}
  */
 WebInspector.inspectorView;
 
 /**
  * @constructor
  * @extends {WebInspector.View}
  */
 WebInspector.RootView = function()
 {
     WebInspector.View.call(this);
     this.markAsRoot();
     this.element.classList.add("fill", "root-view");
     this.element.setAttribute("spellcheck", false);
     window.addEventListener("resize", this.doResize.bind(this), true);
 };
 
 WebInspector.RootView.prototype = {
     __proto__: WebInspector.View.prototype
 };