[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





--- Comment #12 from Andrey Kosyakov <caseq at chromium.org>  2010-05-11 07:05:07 PST ---
(In reply to comment #10)
> (From update of attachment 55415 [details])
> 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.
> 

This was just moved to upper level, and now I renamed HelpSection to ShortcutsSection to indicate these are shortcuts-specific.

> 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.

I suggest it's about time to start doing it -- added Element.prototype.createChild()
More on this topic: http://www.xs4all.nl/~hwiegman/jargon/html/C/COBOL-fingers.html

> 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

fixed.

> 
> 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.

As dicussed, inheriting from view doesn't see worth hassle. Besides, we don't really need to expose hide() and visible now.

> 
> 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.

We don't mean to expose order to client at the moment, it just accounts creation order (we rather store sections hashed by name rather than in array, so client code may address them by name) -- yet we need some determinate order.

> 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.
done.

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

Fixed (though often happened to be just once in this file)
> 
> 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.

It's now a map plus a static function.

> 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.

We now have shortcut-specific help section.

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

Moved help screen creation to inspector.js

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

Done.

> 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.

addRelatedKeys _is_ the convenience method, isn't it?

> 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.

Fixed.

> 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.

I suggest we don't specialize HelpScreen -- just make it generic enough to layout sections, then introduce various sections that support content-specific rendering (e.g. shortcuts, console etc).

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