[webkit-reviews] review denied: [Bug 84235] Web Inspector: Does not have search navigation button for going through matches in either direction (prev, next) : [Attachment 137908] Patch

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied sam <dsam2912 at gmail.com>'s
request for review:
Bug 84235: Web Inspector: Does not have search navigation button for going
through matches in either direction (prev, next)
https://bugs.webkit.org/show_bug.cgi?id=84235

Attachment 137908: Patch
https://bugs.webkit.org/attachment.cgi?id=137908&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=137908&action=review


> Source/WebCore/WebCore.gypi:6555
> +	       'inspector/front-end/Images/searchBtnBck.png',

We try to not abbreviate in WebKit. searchButtonBack, etc.

> Source/WebCore/inspector/front-end/SearchController.js:40
> +    this._searchControlBoxElement =
document.getElementById("toolbar-search-nav-ctrl");

-navigation-control

> Source/WebCore/inspector/front-end/SearchController.js:258
> +    _onNextBtnSearch: function(event)

No abbreviations please.

> Source/WebCore/inspector/front-end/SearchController.js:264
> +    _onPrevBtnSearch: function(event)

ditto

> Source/WebCore/inspector/front-end/SearchController.js:336
> +    _createSearchNavBtn: function(direction) {

ditto. Also, you should place { on the next line.

> Source/WebCore/inspector/front-end/SearchController.js:340
> +	   switch(direction) {

Do not offset case from switch, use 4 characters for indentation within case
blocks.

> Source/WebCore/inspector/front-end/SearchController.js:342
> +			  searchNavControlElement.setAttribute("title" ,
"Search Previous");

You should use WebInspector.UIString("Search Previous") and add the string
itself into localizedStrings.js (WebCore/English.lproj).

> Source/WebCore/inspector/front-end/SearchController.js:359
> +    _populateSearchNavButtons: function() {

{ on the next line

> Source/WebCore/inspector/front-end/inspector.css:279
> +    background-image: url('Images/searchBtnBck.png');

Sorry for not being clear in my previous review. I don't think images will make
it better - they are still not in line with the theme the OS might use for the
inspector. We try to only use thin borders and/or icons with (semi-)transparent
background when we make new controls.