Bug 38667 - Web Inspector: add help on keyboard shortcuts
Summary: Web Inspector: add help on keyboard shortcuts
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 39064
Blocks:
  Show dependency treegraph
 
Reported: 2010-05-06 11:18 PDT by Andrey Kosyakov
Modified: 2010-05-14 05:36 PDT (History)
9 users (show)

See Also:


Attachments
screenshot (120.02 KB, image/png)
2010-05-06 11:26 PDT, Andrey Kosyakov
no flags Details
screenshot (a slightly better one) (116.87 KB, image/png)
2010-05-06 11:50 PDT, Andrey Kosyakov
no flags Details
screenshot (docked mode, scroll-bar, clipping) (64.14 KB, image/png)
2010-05-06 12:30 PDT, Andrey Kosyakov
no flags Details
draft patch (still needs a bit of facelifting on mac, perhaps) (35.89 KB, patch)
2010-05-06 12:45 PDT, Andrey Kosyakov
pfeldman: review-
Details | Formatted Diff | Diff
patch (34.66 KB, patch)
2010-05-07 13:32 PDT, Andrey Kosyakov
pfeldman: review-
Details | Formatted Diff | Diff
patch (35.07 KB, patch)
2010-05-11 06:50 PDT, Andrey Kosyakov
pfeldman: review-
Details | Formatted Diff | Diff
patch (39.58 KB, patch)
2010-05-11 08:49 PDT, Andrey Kosyakov
no flags Details | Formatted Diff | Diff
patch (37.55 KB, patch)
2010-05-12 05:13 PDT, Andrey Kosyakov
pfeldman: review+
Details | Formatted Diff | Diff
patch (100.72 KB, patch)
2010-05-12 11:13 PDT, Andrey Kosyakov
pfeldman: review+
Details | Formatted Diff | Diff
patch (picked nits, moved WebInspector.UIString to be used directly with string literals) (100.87 KB, patch)
2010-05-13 03:24 PDT, Andrey Kosyakov
pfeldman: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andrey Kosyakov 2010-05-06 11:18:40 PDT
Display help on keyboard shortcuts upon '?' (or F1/Cmd-Shift-? while in console)
Comment 1 Andrey Kosyakov 2010-05-06 11:26:46 PDT
Created attachment 55272 [details]
screenshot
Comment 2 Andrey Kosyakov 2010-05-06 11:50:29 PDT
Created attachment 55281 [details]
screenshot (a slightly better one)
Comment 3 Patrick Mueller 2010-05-06 11:55:16 PDT
Looks nice.  Though "?" would be nice, it obviously won't work anywhere that
otherwise takes keyboard input and has focus.  Maybe that's ok though.

How well does this "shrink", if the window width is small-ish?

I'm thinking also providing a "visual" link.  Maybe a question mark glyph on a
keyboard icon, to the left of the search bar.
Comment 4 Andrey Kosyakov 2010-05-06 12:30:13 PDT
Created attachment 55284 [details]
screenshot (docked mode, scroll-bar, clipping)

(In reply to comment #3)
> Looks nice.  Though "?" would be nice, it obviously won't work anywhere that
> otherwise takes keyboard input and has focus.  Maybe that's ok though.

That's why we also check F1 on Win and Cmd-Shift-? on Mac. I'm not particularly sure if the latter is obvious enough (though this appears to be default help shortcut for the platform), so suggestions on other possible hotkeys are much appreciated.

> How well does this "shrink", if the window width is small-ish?

It scrolls vertically and is clipped in the unlikely case it doesn't fit horizontally. Also, in docked mode, it's just full screen with caption off, to save precious estate -- please find screenshot attached.
Comment 5 Andrey Kosyakov 2010-05-06 12:45:39 PDT
Created attachment 55285 [details]
draft patch (still needs a bit of facelifting on mac, perhaps)
Comment 6 WebKit Review Bot 2010-05-06 12:53:38 PDT
Attachment 55285 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/inspector/front-end/shortcutKeysPopover.css:1:  One or more unexpected \r (^M) found; better to use only a \n  [whitespace/carriage_return] [1]
Suppressing further [whitespace/carriage_return] reports for this file.
WebCore/inspector/front-end/shortcutKeysPopover.css:72:  Line contains tab character.  [whitespace/tab] [5]
WebCore/inspector/front-end/ShortcutKeysPopover.js:1:  One or more unexpected \r (^M) found; better to use only a \n  [whitespace/carriage_return] [1]
Suppressing further [whitespace/carriage_return] reports for this file.
Total errors found: 346 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Pavel Feldman 2010-05-06 23:42:49 PDT
Comment on attachment 55285 [details]
draft patch (still needs a bit of facelifting on mac, perhaps)

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!
Comment 8 Andrey Kosyakov 2010-05-07 13:32:08 PDT
Created attachment 55415 [details]
patch

-- Generalized naming (HelpScreen etc)
-- Generalized "key" column handling (lower level accepts html, shortcut rendering is now in HelpSection's helper methods)
-- Create HelpScreen & load CSS lazily, 
-- Decoupled from inspector.js where possible (key handling etc)
-- Better fonts for Mac
Comment 9 Andrey Kosyakov 2010-05-07 13:51:58 PDT
(In reply to comment #7)
> (From update of attachment 55285 [details])
> ChangeLog is missing
> 
> You should modify visual studio project and WebCore.qrc to make Windows & Qt
> happy.

I just skipped it for the draft to save time (Qt was fine, though). Fixed it now.

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

As agreed offline, lower level now exposes generic API that accepts arbitrary HTML as "keys", higher level provides specific utility methods to decorate shortcuts and their combinations. Also, this will complicate localization and won't allow us to render special strings (like '+' here) distinguishably.

> 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")

This won't allow for platform-specific key names (e.g. Shift vs. ⇑)

> section.addRow("Up / Down", "Next / previous line")

This complicates localization (if we ever want one for key names).

> section.addRow("Alt + N / Alt + P", ...

And this does not account for shortcuts varying by platform (e.g. "Ctrl + [" vs. "Cmd + [")

> Rationale: there is no need to deal with shortcut objects if they are not used
> in the code explicitly.

This also allows us to keep consistent key names. And then, it would be good idea to migrate all shortcuts to be configured with the help of WebInspector.KeyboardShortcut anyway.

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

Fixed.

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

Fixed for class members, not done for local vars for sake of brevity and readability.

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

I'd rather render table only once, as it's expensive.

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

Fixed.

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

Exactly. Status may change as a result of click on close button, which the upper level won't know.

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

It almost is, except for section() helper.

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

Done.

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

As discussed -- added generic method + specific helpers.

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

Fixed.

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

Fixed.

> 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

Fixed.

> WebCore/inspector/front-end/shortcutKeysPopover.css:1
>  +  #shortcuts-window {
> No need
> 
> WebCore/inspector/front-end/shortcutKeysPopover.css:11
>  +      -webkit-border-radius: 8px;
> border-radius?

Fixed.

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

This is to override more generic style (help-window now)

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

Wouldn't work -- we have size: 90% in another style we use.

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

Fixed.

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

Done.
Comment 10 Pavel Feldman 2010-05-10 04:17:58 PDT
Comment on attachment 55415 [details]
patch

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.
Comment 11 Andrey Kosyakov 2010-05-11 06:50:41 PDT
Created attachment 55699 [details]
patch
Comment 12 Andrey Kosyakov 2010-05-11 07:05:07 PDT
(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).
Comment 13 Pavel Feldman 2010-05-11 07:14:14 PDT
Comment on attachment 55699 [details]
patch

Almost there...

WebCore/inspector/front-end/HelpScreen.js:1
 +  WebInspector.HelpScreen = function(sections, nColumns)
You need copyright here.

WebCore/inspector/front-end/HelpScreen.js:97
 +  WebInspector.shortcutsHelpSection = function(name)
It'd be great to have this on some singleton class corresponding to keyboard shortcuts screen. That class would also have methods for showing / hiding it.

WebCore/inspector/front-end/HelpScreen.js:105
 +  WebInspector.ShortcutsSection = function(name)
You want this in a separate file (ShortcutsScreen.js probably).
Comment 14 Andrey Kosyakov 2010-05-11 08:49:03 PDT
Created attachment 55706 [details]
patch
Comment 15 Andrey Kosyakov 2010-05-12 05:13:24 PDT
Created attachment 55834 [details]
patch

- Moved multi-column layout logic into ShortcutsHelp
- Misc cleanup
Comment 16 WebKit Review Bot 2010-05-12 05:15:51 PDT
Attachment 55834 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/inspector/front-end/ShortcutsHelp.js:51:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 17 Pavel Feldman 2010-05-12 05:36:31 PDT
Comment on attachment 55834 [details]
patch

r+ with comments, please fix prior to landing.

WebCore/inspector/front-end/ConsoleView.js:104
 +      var shortcut = WebInspector.KeyboardShortcut;
extract method?

WebCore/inspector/front-end/ElementsPanel.js:110
 +      var shortcut = WebInspector.KeyboardShortcut;
extract method + fill style shortcuts from here as well or call explicit method on styles sidebar.

WebCore/inspector/front-end/ScriptsPanel.js:178
 +      section.addAlternateKeys([ shortcut1.name, shortcut2.name ], "Continue");
consider extracting.

WebCore/inspector/front-end/ShortcutsHelp.js:101
 +      get height()
_ is private to a compilation unit, not a class.

WebCore/inspector/front-end/ShortcutsHelp.js:51
 +  	this._helpScreen.show();
tab?
Comment 18 Andrey Kosyakov 2010-05-12 11:13:42 PDT
Created attachment 55873 [details]
patch

- extracted shortcut registration to separated methods
- close only on certain keys, fix keyboard scrolling
- added localizedStrings
- fixed some bugs
Comment 19 Pavel Feldman 2010-05-12 11:29:59 PDT
Comment on attachment 55873 [details]
patch

I'll land it.
Comment 20 Joseph Pecoraro 2010-05-12 11:40:05 PDT
Please don't land the patch with an empty ChangeLog like this. I really think ChangeLogs deserve better treatment.
Comment 21 Joseph Pecoraro 2010-05-12 11:58:01 PDT
Comment on attachment 55873 [details]
patch

> +++ WebCore/ChangeLog	(working copy)

Could really use some high level descriptions here!


> +    closeButton.innerText = "\u2716";

Could use a comment to clarify what this character is.


> +        // this manual layout ugliness should be gone once WebKit implements
> +        // pagination hints for CSS columns (break-inside etc)

// Comments should be like sentences.


Arg I have to run so I couldn't get any more details. Nothing major though.
UIString being hidden deep down below was a bit confusing though.
Comment 22 Andrey Kosyakov 2010-05-13 03:24:50 PDT
Created attachment 55959 [details]
patch (picked nits, moved WebInspector.UIString to be used directly with string literals)
Comment 23 Pavel Feldman 2010-05-13 06:43:02 PDT
Landed with keycodes fixed.

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/English.lproj/localizedStrings.js
	M	WebCore/WebCore.gypi
	M	WebCore/WebCore.vcproj/WebCore.vcproj
	M	WebCore/inspector/front-end/CallStackSidebarPane.js
	M	WebCore/inspector/front-end/ConsoleView.js
	M	WebCore/inspector/front-end/ElementsPanel.js
	M	WebCore/inspector/front-end/ElementsTreeOutline.js
	A	WebCore/inspector/front-end/HelpScreen.js
	M	WebCore/inspector/front-end/KeyboardShortcut.js
	M	WebCore/inspector/front-end/ScriptsPanel.js
	A	WebCore/inspector/front-end/ShortcutsHelp.js
	M	WebCore/inspector/front-end/SidebarPane.js
	M	WebCore/inspector/front-end/StylesSidebarPane.js
	M	WebCore/inspector/front-end/TextViewer.js
	M	WebCore/inspector/front-end/WebKit.qrc
	A	WebCore/inspector/front-end/helpScreen.css
	M	WebCore/inspector/front-end/inspector.html
	M	WebCore/inspector/front-end/inspector.js
	M	WebCore/inspector/front-end/utilities.js
Committed r59360
Comment 24 Pavel Feldman 2010-05-13 07:47:42 PDT
Reverted due to Chromium concatenated js problems: too many characters for windows command line :)
Comment 25 Pavel Feldman 2010-05-14 05:36:44 PDT
Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/English.lproj/localizedStrings.js
	M	WebCore/WebCore.gypi
	M	WebCore/WebCore.vcproj/WebCore.vcproj
	M	WebCore/inspector/front-end/CallStackSidebarPane.js
	M	WebCore/inspector/front-end/ConsoleView.js
	M	WebCore/inspector/front-end/ElementsPanel.js
	M	WebCore/inspector/front-end/ElementsTreeOutline.js
	A	WebCore/inspector/front-end/HelpScreen.js
	M	WebCore/inspector/front-end/KeyboardShortcut.js
	M	WebCore/inspector/front-end/ScriptsPanel.js
	A	WebCore/inspector/front-end/ShortcutsHelp.js
	M	WebCore/inspector/front-end/SidebarPane.js
	M	WebCore/inspector/front-end/StylesSidebarPane.js
	M	WebCore/inspector/front-end/TextViewer.js
	M	WebCore/inspector/front-end/WebKit.qrc
	A	WebCore/inspector/front-end/helpScreen.css
	M	WebCore/inspector/front-end/inspector.html
	M	WebCore/inspector/front-end/inspector.js
	M	WebCore/inspector/front-end/utilities.js
Committed r59468