[webkit-reviews] review denied: [Bug 38667] Web Inspector: add help on keyboard shortcuts : [Attachment 55285] draft patch (still needs a bit of facelifting on mac, perhaps)

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu May 6 23:42:48 PDT 2010


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 55285: draft patch (still needs a bit of facelifting on mac,
perhaps)
https://bugs.webkit.org/attachment.cgi?id=55285&action=review

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


More information about the webkit-reviews mailing list