[webkit-reviews] review denied: [Bug 38667] Web Inspector: add help on keyboard shortcuts : [Attachment 55415] patch

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Andrey Kosyakov
<caseq at chromium.org>'s request for review:
Bug 38667: Web Inspector: add help on keyboard shortcuts
https://bugs.webkit.org/show_bug.cgi?id=38667

Attachment 55415: patch
https://bugs.webkit.org/attachment.cgi?id=55415&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
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.