Created attachment 78819 [details] [PATCH] Suggested solution

Comment on attachment 78819 [details] [PATCH] Suggested solution View in context: https://bugs.webkit.org/attachment.cgi?id=78819&action=review > Source/WebCore/inspector/front-end/CSSKeywordCompletions.js:2 > + * Copyright (C) 2010 Google Inc. All rights reserved. 2011 > Source/WebCore/inspector/front-end/StylesSidebarPane.js:1630 > + return { keyIdentifier: "___", handled: false }; ___ looks weird, can you use null or undefined instead? > Source/WebCore/inspector/front-end/TextPrompt.js:76 > + if (this._customKeyDownHandler) { Let's extract Up, Down and Tab handlers into their own methods to allow overriding them. r- for this. > Source/WebCore/inspector/front-end/TextPrompt.js:400 > + _tabKeyPressed: function(reverse) This method can be inlined.

Created attachment 78939 [details] [PATCH] Comments addressed Introduced a customKeyHandlers parameter into the TextPrompt ctor to pass in custom Up, Down, and Tab handlers.

Created attachment 78946 [details] [PATCH] Inherited TextPrompt to override key handlers

Comment on attachment 78946 [details] [PATCH] Inherited TextPrompt to override key handlers View in context: https://bugs.webkit.org/attachment.cgi?id=78946&action=review r- is for managing selection in the sidebar, rest seems fine. > Source/WebCore/inspector/front-end/CSSKeywordCompletions.js:45 > +WebInspector.CSSKeywordCompletions._colors = [ Nit: please compress to ~120 chars per line. > Source/WebCore/inspector/front-end/CSSKeywordCompletions.js:197 > +WebInspector.CSSKeywordCompletions._colorAwareProperties = [ ditto > Source/WebCore/inspector/front-end/CSSKeywordCompletions.js:224 > +WebInspector.CSSKeywordCompletions._propertyKeywordMap = { ditto > Source/WebCore/inspector/front-end/StylesSidebarPane.js:1690 > + finalSelectionRange.setStart(replacementTextNode, 0); I would expect all the selection management code to move to the prompt. Why is it still here? > Source/WebCore/inspector/front-end/StylesSidebarPane.js:1902 > + Nit: blank line.

(In reply to comment #5) > (From update of attachment 78946 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=78946&action=review > > r- is for managing selection in the sidebar, rest seems fine. > > > Source/WebCore/inspector/front-end/CSSKeywordCompletions.js:45 > > +WebInspector.CSSKeywordCompletions._colors = [ > > Nit: please compress to ~120 chars per line. Done. > > Source/WebCore/inspector/front-end/StylesSidebarPane.js:1690 > > + finalSelectionRange.setStart(replacementTextNode, 0); > > I would expect all the selection management code to move to the prompt. Why is it still here? I don't understand why you would expect this. I'd rather understand you expect all the _prompt_ management code move there. But scrolling numeric values is a whole different thing, isn't it? It does not match the TextPrompt responsibilities altogether. > > Source/WebCore/inspector/front-end/StylesSidebarPane.js:1902 > > + > > Nit: blank line. Done.

Created attachment 79146 [details] [PATCH] Comments from pfeldman addressed

Landed with minor fixes: http://trac.webkit.org/changeset/76116