1) HUD have a different light source than the rest of the UI. 2) The border radius is too big. 3) The “X” button is not vertically centered in the header. 4) Some of the shortcuts are too small, so you can’t see them.
Created attachment 138981 [details] Screenshot: before patch
Created attachment 138982 [details] Screenshot: after patch
Created attachment 138989 [details] Patch
Created attachment 138992 [details] Patch
Attachment 138992 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/ChangeLog:15: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Total errors found: 1 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 138989 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=138989&action=review > Source/WebCore/ChangeLog:8 > + No new tests. (OOPS!) You need to explain what has happened and why here (enumerate changes you've done briefly). > Source/WebCore/inspector/front-end/KeyboardShortcut.js:64 > + Left: { code: 37, name: "<Left>" }, // also NUM_WEST Make sure these are getting localized (get into English.lproj/localizedStrings.js) > Source/WebCore/inspector/front-end/ShortcutsScreen.js:82 > + var minMax = totalHeight; It is not clear what minMax stands for. > Source/WebCore/inspector/front-end/ShortcutsScreen.js:83 > + var minMaxIdx = -1; Please do not use abbreviations (should be Index) > Source/WebCore/inspector/front-end/ShortcutsScreen.js:86 > + for (idx = 0; idx < orderedSections.length; ++idx) { for (var i = 0; i < ... ++i) { } > Source/WebCore/inspector/front-end/ShortcutsScreen.js:-80 > - // This manual layout ugliness should be gone once WebKit implements The underlying issue it not fixed though - what if user resizes the screen? > Source/WebCore/inspector/front-end/ShortcutsScreen.js:106 > + for (idx = stopIndices[columnIndex - 1]; idx < stopIndices[columnIndex]; ++idx) { No need for { } around single line blocks.
Created attachment 138995 [details] Patch
Created attachment 139019 [details] Patch
Comment on attachment 138989 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=138989&action=review >> Source/WebCore/ChangeLog:8 >> + No new tests. (OOPS!) > > You need to explain what has happened and why here (enumerate changes you've done briefly). Done. >> Source/WebCore/inspector/front-end/KeyboardShortcut.js:64 >> + Left: { code: 37, name: "<Left>" }, // also NUM_WEST > > Make sure these are getting localized (get into English.lproj/localizedStrings.js) These strings are not localized. >> Source/WebCore/inspector/front-end/ShortcutsScreen.js:82 >> + var minMax = totalHeight; > > It is not clear what minMax stands for. Clarifying comment added. >> Source/WebCore/inspector/front-end/ShortcutsScreen.js:83 >> + var minMaxIdx = -1; > > Please do not use abbreviations (should be Index) Done >> Source/WebCore/inspector/front-end/ShortcutsScreen.js:86 >> + for (idx = 0; idx < orderedSections.length; ++idx) { > > for (var i = 0; i < ... ++i) { > } Done. >> Source/WebCore/inspector/front-end/ShortcutsScreen.js:-80 >> - // This manual layout ugliness should be gone once WebKit implements > > The underlying issue it not fixed though - what if user resizes the screen? I'm planning to turn 2 column table to 2 inlined divs in farther patches. So it will be 1 or 2 columned, depending container on width. >> Source/WebCore/inspector/front-end/ShortcutsScreen.js:106 >> + for (idx = stopIndices[columnIndex - 1]; idx < stopIndices[columnIndex]; ++idx) { > > No need for { } around single line blocks. Done.
Comment on attachment 139019 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=139019&action=review > Source/WebCore/inspector/front-end/KeyboardShortcut.js:64 > + Left: { code: 37, name: "<Left>" }, // also NUM_WEST Even though they are not currently localized, they should be (at least Left, Right, Up and Down) > Source/WebCore/inspector/front-end/ShortcutsScreen.js:58 > + * Renders sections to and appends them to the given node. We don't comment on code in WebKit unless necessary. > Source/WebCore/inspector/front-end/ShortcutsScreen.js:110 > +}; No ; after } > Source/WebCore/inspector/front-end/helpScreen.css:132 > + color: #DD0; We try to use rgb() notation for colors.
Created attachment 139353 [details] Patch
Comment on attachment 139019 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=139019&action=review >> Source/WebCore/inspector/front-end/KeyboardShortcut.js:64 >> + Left: { code: 37, name: "<Left>" }, // also NUM_WEST > > Even though they are not currently localized, they should be (at least Left, Right, Up and Down) Done >> Source/WebCore/inspector/front-end/ShortcutsScreen.js:58 >> + * Renders sections to and appends them to the given node. > > We don't comment on code in WebKit unless necessary. Removed >> Source/WebCore/inspector/front-end/ShortcutsScreen.js:110 >> +}; > > No ; after } Rolled back here and anywhere it was added. >> Source/WebCore/inspector/front-end/helpScreen.css:132 >> + color: #DD0; > > We try to use rgb() notation for colors. fixed
Comment on attachment 139353 [details] Patch Clearing flags on attachment: 139353 Committed r115568: <http://trac.webkit.org/changeset/115568>
All reviewed patches have been landed. Closing bug.