[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 #55285|review?                     |review-
               Flag|                            |




--- Comment #7 from Pavel Feldman <pfeldman at chromium.org>  2010-05-06 23:42:49 PST ---
(From update of attachment 55285)
ChangeLog is missing

You should modify visual studio project and WebCore.qrc to make Windows & Qt
happy.

WebCore/WebCore.xcodeproj/project.pbxproj:904
 +          49E6045411933E4900425C05 /* ShortcutKeysPopover.js in Sources */ =
{isa = PBXBuildFile; fileRef = 49E6045211933E4900425C05 /*
ShortcutKeysPopover.js */; };
No need to modify xproject


WebCore/WebCore.xcodeproj/project.pbxproj:10488
 +                  49E6045211933E4900425C05 /* ShortcutKeysPopover.js */,
ditto

WebCore/WebCore.xcodeproj/project.pbxproj:21201
 +                  49E6045411933E4900425C05 /* ShortcutKeysPopover.js in
Sources */,
ditto

WebCore/inspector/front-end/ConsoleView.js:107
 +      section.addAlternateKeys(keys, "Clear Console");
I don't think we need such a strict API. You could just have an addRow method
receiving two strings: first is non-localizable shortcut name name and second
is localizable description. That way you would have:
section.addRow(keys.join("/"), "Clear Console") here.
Rationale: sometimes you need to do things like 'Ctrl + ("A" - "Z")' and such.
Why limiting ourselves artificially.

WebCore/inspector/front-end/ConsoleView.js:110
 +          shortcut.shortcutToString(shortcut.Keys.Tab),
I think this is overcomplication. I'd replace code below with:
section.addRow("Tab / Shift + Tab", "Next / previous suggestion")
section.addRow("Up / Down", "Next / previous line")
section.addRow("Alt + N / Alt + P", ...
Rationale: there is no need to deal with shortcut objects if they are not used
in the code explicitly.

WebCore/inspector/front-end/ShortcutKeysPopover.js:3
 +      this._element = document.getElementById("shortcuts-window");
You should create these elements dynamically in the code and add to body
element. No need to touch html.

WebCore/inspector/front-end/ShortcutKeysPopover.js:4
 +      this._table = document.getElementById("shortcuts-table");
All element objects should have Element suffix. this._tableElement in this
case.

WebCore/inspector/front-end/ShortcutKeysPopover.js:21
 +          this._element.style.visibility = "visible";
You'll be simply adding it here, not unhiding.

WebCore/inspector/front-end/ShortcutKeysPopover.js:18
 +              this._buildShortcutsTable(this._sections, 2);
If this method was creating the table object itself, you would not need the
ugly needBuild flag.

WebCore/inspector/front-end/ShortcutKeysPopover.js:31
 +      get shown()
No one should know your status. Why is it here? Cancel key events?

WebCore/inspector/front-end/ShortcutKeysPopover.js:132
 +  WebInspector.KeyboardShortcutsTable = function()
The size of this class suggests that it is a map, not a class.

WebCore/inspector/front-end/ShortcutKeysPopover.js:151
 +      this.order = ++WebInspector.KeyboardShortcutsSection._seqNo;
_sequenceNumber

WebCore/inspector/front-end/ShortcutKeysPopover.js:156
 +  WebInspector.KeyboardShortcutsSection.prototype = {
As mentioned above, please keep it simple and not distinguish between related /
alternate / general keys. It simplifies both client and panel code + adds
flexibility.

WebCore/inspector/front-end/inspector.html:146
 +      <div id="shortcuts-window">
As stated above, no need to add this.

WebCore/inspector/front-end/inspector.js:538
 +      this._shortcutsPopover = new
WebInspector.ShortcutKeysPopover(WebInspector.keyboardShortcuts.sections);
This guy could be created lazily. No need to pay for its creation at startup.

WebCore/inspector/front-end/inspector.js:707
 +          if (event.keyIdentifier === "U+001B") {
Please add comment explaining the character

WebCore/inspector/front-end/inspector.js:713
 +      var helpKey = WebInspector.isMac() ? "U+003F" : "U+00BF";
ditto

WebCore/inspector/front-end/shortcutKeysPopover.css:1
 +  #shortcuts-window {
No need

WebCore/inspector/front-end/shortcutKeysPopover.css:11
 +      -webkit-border-radius: 8px;
border-radius?

WebCore/inspector/front-end/shortcutKeysPopover.css:15
 +      border-bottom-left-radius: 0;
This is strange - you don't set radius to non-0, but you zero it for bottom
parts.

WebCore/inspector/front-end/shortcutKeysPopover.css:41
 +      z-index: 1000;
I don't think you need this

WebCore/inspector/front-end/shortcutKeysPopover.css:48
 +      width: 100%;
right: 0

WebCore/inspector/front-end/shortcutKeysPopover.css:53
 +      font-family: Helvetica, Arial, sans-serif;
You should not define this. It'll pick default sans serif defined for body.

WebCore/inspector/front-end/shortcutKeysPopover.css:1
 +  #shortcuts-window {
This component and a style should be generalized as a help screen, not
necessarily related to popover. I think we should have similar one for console
API and such. It is great you have a dedicated file for it, not that great
there are so many styles here. Let us at least load this stylesheet on demand
not to harm the startup time!

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