[Webkit-unassigned] [Bug 38667] Web Inspector: add help on keyboard shortcuts

[bugzilla-daemon at webkit.org]

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


Pavel Feldman <pfeldman at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #55415|review?                     |review-
               Flag|                            |




--- Comment #10 from Pavel Feldman <pfeldman at chromium.org>  2010-05-10 04:17:58 PST ---
(From update of attachment 55415)
WebCore/ChangeLog:8
 +          No new tests. (OOPS!)
OOPS! (delete this line)

WebCore/inspector/front-end/ElementsPanel.js:119
 +      section.addRelatedKeys(keys, "Expand/collapse");
Nit: I thought we agreed on making section generic (with addRow) and having convenience utility methods for key combinations. These methods could reside in KeyboardShortcuts. My point was that we didn't need to create these structures for shortcuts only for the sake of the help screen - we could simply format a string here and pass it along.

WebCore/inspector/front-end/HelpScreen.js:9
 +      var upperWindow = this._makeChild(this._element, "div", "help-window-upper help-window");
We don't really do it in other places of the code, please be consistent.

WebCore/inspector/front-end/HelpScreen.js:18
 +      closeButton.onclick = this.hide.bind(this);
Please don't use dom0 handlers - add listeners instead.

WebCore/inspector/front-end/HelpScreen.js:19
 +      closeButton.innerHTML = "&#x2716;"
Please set a unicode textContent instead

WebCore/inspector/front-end/HelpScreen.js:26
 +      show: function()
View.js contains a generic show / hide functionality. You should inherit it should you need that.

WebCore/inspector/front-end/HelpScreen.js:52
 +          function compareSections(a, b) 
Approaches like this always concerned me. You either know the order upfront or you don't. Masking it by means of the 'order' comparator increases indirection, but does not really improve modularity.

WebCore/inspector/front-end/HelpScreen.js:64
 +          for (var section = 0; section < orderedSections.length; ++section) {
Coding this as two nested loops would look more natural.

WebCore/inspector/front-end/HelpScreen.js:111
 +  
nit: You often do blank lines before return that we are not used to.

WebCore/inspector/front-end/HelpScreen.js:117
 +  WebInspector.HelpTable = function()
This class looks like a map to me, not sure why you need a class. I thought I noted it already.

WebCore/inspector/front-end/HelpScreen.js:154
 +          this.addLine(this._renderSequence(keys,WebInspector.UIString("or")), description);
Help section should be shortcut agnostic. These convenience methods should be external to it.

WebCore/inspector/front-end/HelpScreen.js:178
 +  WebInspector.HelpController = function()
Not clear why you need this.


WebCore/inspector/front-end/HelpScreen.js:189
 +          if (this._helpScreen && this._helpScreen.shown) {
This should probably go to the inspector.js's documentKeyDown.

WebCore/inspector/front-end/StylesSidebarPane.js:98
 +      section.addRelatedKeys(keys, "Increment/decrement by 0.1");
Again, too much text meaning too little. Having a bunch of addRow calls using convenience formatters would be much simpler and more flexible.

WebCore/inspector/front-end/helpScreen.css:135
 +  .help-table > tr > td {
prefer explicit class names to complex css selectors for the sake of matching speed.

WebCore/inspector/front-end/inspector.js:445
 +      var shortcut = WebInspector.KeyboardShortcut;
It seems like you now have an abstract HelpScreen, but you don't have concrete ShortcutsHelpScreen inherited from it. The code below (general shortcuts on the all panels) should go there instead.

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