Created attachment 156327 [details] Screenshot In network tab search is replaced with filtering
Created attachment 156328 [details] Patch
Comment on attachment 156328 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=156328&action=review Please provide a ChangeLog and fix style. > Source/WebCore/WebCore.gypi:11 > + 'Modules/geolocation/Geoposition.h', Why did it move? > Source/WebCore/WebCore.vcproj/WebCore.vcproj:2520 > + Please remove these extra lines > Source/WebCore/inspector/front-end/FilterController.js:2 > + * Copyright (C) 2006, 2007, 2008 Apple Inc. All rights reserved. Copyright (C) 2012 only. Please use Google copyright. See SettingsScreen.js for an example. > Source/WebCore/inspector/front-end/FilterController.js:33 > +WebInspector.FilterController = function() { Wrong indent. > Source/WebCore/inspector/front-end/FilterController.js:34 > + this._element = document.createElement("table"); I don't think you need to use table for such a simple component. > Source/WebCore/inspector/front-end/FilterController.js:39 > + this._secondRowElement = this._element.createChild("tr", "hidden"); Please remove this second row element, you don't need it here. > Source/WebCore/inspector/front-end/FilterController.js:47 > + this._filterInputElement.addEventListener("mousedown", null, false); What's the point? > Source/WebCore/inspector/front-end/FilterController.js:60 > + Remove this line > Source/WebCore/inspector/front-end/FilterController.js:70 > + One line between methods, please. > Source/WebCore/inspector/front-end/FilterController.js:79 > + _onKeyDown : function(event) { No space before : > Source/WebCore/inspector/front-end/FilterController.js:83 > + WebInspector.setCurrentFocusElement(WebInspector.previousFocusElement()); I don't think you saved previous focus element, so it makes no sense to set it back. You should save it in showFilterField > Source/WebCore/inspector/front-end/FilterController.js:100 > + showFilterField : function() { Ditto > Source/WebCore/inspector/front-end/FilterController.js:111 > + if (event.keyIdentifier === "U+0046") { What is that? Please add an inline comment. > Source/WebCore/inspector/front-end/NetworkPanel.js:51 > + this._isRequestVisible = new Map(); This field needs a better name. I would actually call it _filteredOutRequests and store only requests that were filtered out. > Source/WebCore/inspector/front-end/NetworkPanel.js:441 > + _filterByContent: function(content){ Weird indent (4 spaces indent in webkit) > Source/WebCore/inspector/front-end/NetworkPanel.js:444 > + node.element.removeStyleClass("hidden"); I wouldn't use global hidden class here since it says nothing about the reason why the row is not visible. I would add .network-log-grid.data-grid tr.filtered-out instead > Source/WebCore/inspector/front-end/NetworkPanel.js:446 > + if (node.element.children[0].innerText.indexOf(content)==-1){ Please add spaces near ==. Also we use === in WebInspector. > Source/WebCore/inspector/front-end/NetworkPanel.js:1143 > +// performSearch: function(searchQuery) Please remove commented out code. > Source/WebCore/inspector/front-end/SearchController.js:158 > + if (typeof WebInspector.inspectorView.currentPanel().performSearch !== "function") Weird indent
Created attachment 156384 [details] Patch
Comment on attachment 156384 [details] Patch Please make sure you address all the review comments. Also please rebaseline your patch so that bots could try it.
Created attachment 156677 [details] Patch
Comment on attachment 156677 [details] Patch Please make sure you address all the review comments.
Created attachment 156685 [details] Patch
Comment on attachment 156685 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=156685&action=review > Source/WebCore/ChangeLog:3 > + A new feature added: filtering in the network panel. This line should be equal to the bug summary. > Source/WebCore/ChangeLog:5 > + Reviewed by NOBODY (OOPS!). Please add a blank line before this line. > Source/WebCore/ChangeLog:6 > + Please add a change description here. See previous changes's ChangeLogs for the reference > Source/WebCore/inspector/front-end/FilterController.js:5 > + * modification, are permitted provided that the following conditions are met: * Please fix formatting of the license text, there should be new lines before '*'. It should look the same as in the other files. > Source/WebCore/inspector/front-end/FilterController.js:33 > + this._element.cellSpacing = 0; Please remove this. > Source/WebCore/inspector/front-end/FilterController.js:35 > + // Column 1 Please remove these comments. > Source/WebCore/inspector/front-end/NetworkPanel.js:-754 > - Please revert this change. > Source/WebCore/inspector/front-end/NetworkPanel.js:1757 > + if (!this._parentView._hiddenCategories.all && !this._parentView._filteredOutRequests.get(this._request)) It is enough to look at _filteredOutRequests once. if (this._parentView._filteredOutRequests.get(this._request)) return true; if (!this._parentView._hiddenCategories.all) return false; return this._request.type.name() in this._parentView._hiddenCategories;
Created attachment 156696 [details] Patch
Comment on attachment 156696 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=156696&action=review > Source/WebCore/ChangeLog:11 > + No new tests (OOPS!). Please remove this line (and 1 blank line). > Source/WebCore/inspector/front-end/FilterController.js:50 > + cancelFilter: function() { Here and everywhere: we put open bracket { to the next line after the function name. > Source/WebCore/inspector/front-end/FilterController.js:60 > + _performFilter: function(query) { Please add compiler annotations to this function. > Source/WebCore/inspector/front-end/NetworkPanel.js:443 > + for(var i = 0; i < this._dataGrid.rootNode().children.length; ++i){ Please add space after for and a space before bracket. > Source/WebCore/inspector/front-end/NetworkPanel.js:447 > + if (node.element.children[0].innerText.indexOf(content) === -1){ Please use createPlainTextSearchRegex like it is done in performSearch. Also a space before bracket please. > Source/WebCore/inspector/front-end/NetworkPanel.js:1175 > + performFilter: function(query) { Please add compiler annotations to this function. > Source/WebCore/inspector/front-end/NetworkPanel.js:1442 > + performFilter: function(query){ Please add compiler annotations to this function.
Created attachment 156897 [details] Patch
Comment on attachment 156897 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=156897&action=review > Source/WebCore/inspector/front-end/FilterController.js:38 > + this._filterControlElement = this._element.createChild("span", "toolbar-filter-control"); This element is redundant. > Source/WebCore/inspector/front-end/FilterController.js:59 > + this._performFilter(''); Please use double quotes. > Source/WebCore/inspector/front-end/FilterController.js:60 > + this._filterInputElement.value = ''; Please use double quotes. > Source/WebCore/inspector/front-end/FilterController.js:63 > + _performFilter: function(query) Compiler annotations please. > Source/WebCore/inspector/front-end/FilterController.js:66 > + if (typeof currentPanel.performFilter === 'function') Please use double quotes. > Source/WebCore/inspector/front-end/FilterController.js:78 > + return false; return; > Source/WebCore/inspector/front-end/FilterController.js:81 > + if (isEnterKey(event)) { No need to handle enter since we perform filtering on input event anyway. > Source/WebCore/inspector/front-end/FilterController.js:104 > + var isMac = WebInspector.isMac(); isMac is used only once, please inline. > Source/WebCore/inspector/front-end/FilterController.js:107 > + var isFindKey = event.metaKey && !event.ctrlKey && !event.altKey && !event.shiftKey; isFilterKey > Source/WebCore/inspector/front-end/NetworkPanel.js:447 > + this._filteredOutRequests.put(this._requests[i], true); Just clear the map before the loop and fill it with non matching requests below. Otherwise it looks confusing: you remove "hidden" style and mark request as filtered out at the same time. > Source/WebCore/inspector/front-end/NetworkPanel.js:448 > + if (!_match) _match is never defined and is probably redundant. Also wrong indent here and below. > Source/WebCore/inspector/front-end/NetworkPanel.js:450 > + node.element.addStyleClass("hidden"); I wouldn't use global hidden class here since it says nothing about the reason why the row is not visible. I would add .network-log-grid.data-grid tr.filtered-out instead > Source/WebCore/inspector/front-end/NetworkPanel.js:1174 > + this.performFilter(''); Please use double quotes. > Source/WebCore/inspector/front-end/NetworkPanel.js:1181 > + this._filterByContent(query); What is the difference between performFilter and filterByContent? There is no need to extract one from another. > Source/WebCore/inspector/front-end/NetworkPanel.js:1450 > + performFilter: function(query){ Compiler annotations please.
Created attachment 156911 [details] Patch
Comment on attachment 156911 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=156911&action=review > Source/WebCore/inspector/front-end/NetworkPanel.js:-1554 > - // of a specific event. If startAtZero is set, then this is useless, and we Why did this change? Please revert weird formatting changes.
Created attachment 156920 [details] Patch
Comment on attachment 156920 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=156920&action=review > Source/WebCore/inspector/front-end/NetworkPanel.js:1169 > + node.element.removeStyleClass("filtered-out"); I'd move this below: if (node._request.displayName.match(filterRegExp)) node.element.removeStyleClass("filtered-out"); else { node.element.addStyleClass("filtered-out"); this._filteredOutRequests.put(this._requests[i], true); } > Source/WebCore/inspector/front-end/NetworkPanel.js:1445 > + performFilter: function(query){ Annotations are still missing.
Comment on attachment 156920 [details] Patch As discussed offline let's get rid of FilterController and implement filtering as a checkbox on search pane.
Created attachment 158281 [details] Patch
Created attachment 158293 [details] Patch
Comment on attachment 158293 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158293&action=review Considering the scope of this patch the search controller change looks too complicated to me. I think this complexity is the result of hiding inspector footer and showing again each time the checkbox state is changed. We should update already shown footer instead. > Source/WebCore/inspector/front-end/InspectorView.js:85 > + WebInspector.searchController.panelChanged(); cancelSearch call should be enough.
Created attachment 158338 [details] Patch
Created attachment 158360 [details] Patch
Comment on attachment 158360 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158360&action=review > Source/WebCore/ChangeLog:8 > + New feature: there are both search bar and filter bar in network tabw. One can swich them using checkbox "filtering". Please fix typos. > Source/WebCore/inspector/front-end/SearchController.js:46 > + this._firstRowElement.createChild("td"); Why is this needed? > Source/WebCore/inspector/front-end/SearchController.js:57 > + Extra blank line. > Source/WebCore/inspector/front-end/SearchController.js:74 > + this._filterInputElement.addEventListener("keydown", this._onKeyDown.bind(this), true); keydown handler has some search specific logic that should be treated carefully. > Source/WebCore/inspector/front-end/SearchController.js:79 > + this._secondRowElement.createChild("td"); Why is this needed? > Source/WebCore/inspector/front-end/SearchController.js:125 > + Extra blank line. > Source/WebCore/inspector/front-end/SearchController.js:136 > + this._searchOrFilterQuery = ""; unused > Source/WebCore/inspector/front-end/SearchController.js:255 > + this._performSearch(this._searchInputElement.value); Please remove this line, we don't want to change search behavior. > Source/WebCore/inspector/front-end/SearchController.js:432 > + this._performSearch(this._filterInputElement.value, false, false); Please use this._searchInputElement for consistency.
Created attachment 158532 [details] Patch
Comment on attachment 158532 [details] Patch As discussed please add match highlighting for filtering. Please attach a screenshot for the new implementation also.
Created attachment 158779 [details] Patch
Comment on attachment 158779 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158779&action=review > Source/WebCore/inspector/front-end/NetworkPanel.js:1072 > + this._highlightMatchedRequests([], false, ""); "" doesn't look like a regexp to me. Please annotate new methods to make sure we catch such errors. > Source/WebCore/inspector/front-end/NetworkPanel.js:1106 > + if (regExp.test("")) I don't think this is needed. > Source/WebCore/inspector/front-end/NetworkPanel.js:1155 > + this._toggleLargerRequests(); This should be in highlight... method because it's needed both for search and filtering.
Created attachment 158792 [details] Patch
Created attachment 158807 [details] Screenshot
Created attachment 158808 [details] Screenshot
Comment on attachment 158792 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158792&action=review > Source/WebCore/inspector/front-end/NetworkPanel.js:1072 > + this._highlightMatchedRequests([], false, createPlainTextSearchRegex("")); Let's always use ...ForSearch method for search. > Source/WebCore/inspector/front-end/NetworkPanel.js:1136 > + _highlightNthMatchedRequestForSearch: function(matchedRequestIndex, reveal, regExp) Please remove regExp parameter here, for search it is always this._searchRegExp
Created attachment 158841 [details] Patch
Created attachment 158848 [details] Patch
Created attachment 159048 [details] Patch
Created attachment 159057 [details] Patch
Comment on attachment 159057 [details] Patch Clearing flags on attachment: 159057 Committed r125879: <http://trac.webkit.org/changeset/125879>
All reviewed patches have been landed. Closing bug.