Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2150713002:
[Devtools] Refactor NetworkLogViewColumns to be dynamic (Closed)
|
Created:
4 years, 5 months ago by allada Modified:
4 years, 4 months ago CC:
chromium-reviews, caseq+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, blink-reviews, sergeyv+blink_chromium.org, pfeldman, kozyatinskiy+blink_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Devtools] Refactor NetworkLogViewColumns to be dynamic
This patch is to make the columns in network panel
much more dynamic. This is in prep to allow custom
columns.
Related: https://codereview.chromium.org/2118663002/
BUG=540569
Committed: https://crrev.com/cc69422e7e1a446688a66bff3c6b4eb701462508
Cr-Commit-Position: refs/heads/master@{#409129}
Patch Set 1 : Revamp columns #Patch Set 2 : added settings #
Total comments: 46
Patch Set 3 : Changes #
Total comments: 28
Patch Set 4 : rebase #Patch Set 5 : Reviewer change requests #
Total comments: 2
Patch Set 6 : Changes #Patch Set 7 : Changes #
Messages
Total messages: 24 (12 generated)
Description was changed from ========== Revamp columns Changed supporting files BUG= ========== to ========== [Devtools] Refactor NetworkLogViewColumns to be dynamic This patch is to make the columns in network panel much more dynamic. This is in prep to allow custom columns. Related: https://codereview.chromium.org/2118663002/ BUG=540569 ==========
allada@chromium.org changed reviewers: + dgozman@chromium.org, lushnikov@chromium.org
PTL
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
The CQ bit was checked by allada@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2150713002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/common/Settings.js (right): https://codereview.chromium.org/2150713002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/common/Settings.js:724: var newSetting = WebInspector.settings.createSetting("networkLogColumns", {}) semicolons please! https://codereview.chromium.org/2150713002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js (left): https://codereview.chromium.org/2150713002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:328: this._networkLogView.dataGridSorted(); Where did this call go? https://codereview.chromium.org/2150713002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:344: this._networkLogView.dataGridSorted(); And this? https://codereview.chromium.org/2150713002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js (right): https://codereview.chromium.org/2150713002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:17: /** @type {!Array.<!Element>} */ Modern syntax requires omitting dot before "<". Please convert as you go. https://codereview.chromium.org/2150713002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:44: * subTitle: string, "subtitle" is widespread in our code. https://codereview.chromium.org/2150713002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:48: * nonSelectable: boolean, selectable? https://codereview.chromium.org/2150713002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:52: * sortConfig: (!WebInspector.NetworkLogViewColumns.SortConfig) No need in extra () https://codereview.chromium.org/2150713002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:55: WebInspector.NetworkLogViewColumns.ColumnDescriptor; I think we may avoid mentioning "column" twice. https://codereview.chromium.org/2150713002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:75: subTitle: null, null is not a {string} https://codereview.chromium.org/2150713002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:172: id: "setCookies", Shouldn't all these be lowercase (based on code in Settings.js) ? https://codereview.chromium.org/2150713002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:218: sortable: false, sortable: false, but with sortConfig? Let's remove sortable, and rely on the presence of sortConfig instead. https://codereview.chromium.org/2150713002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:335: WebInspector.NetworkLogViewColumns._orderrideableConfigs = ["visible", "title"]; I don't get the "orderrideable". Let's rename. https://codereview.chromium.org/2150713002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:352: WRONG!!!! It's wrong! https://codereview.chromium.org/2150713002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:367: this._columns.push(columnConfig); Create this._columns in this method. https://codereview.chromium.org/2150713002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:395: if (columnConfig.sortConfig && columnConfig.sortConfig.entries) { if (!...) continue; https://codereview.chromium.org/2150713002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:398: placeHolderOption.style.display = "none"; Toggle hidden class instead. https://codereview.chromium.org/2150713002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:422: if (columnConfig.sortConfig.entries) { if (!...) continue; https://codereview.chromium.org/2150713002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:427: entry.calculator = timeCalculator; This is weird. Let's use different fields for calculator vs calculatorType. You can also just have a map{type=>calculator} and bother updating all the columns at all. https://codereview.chromium.org/2150713002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:457: this._networkLogView.removeAllNodeHighlights(); You probably want to do this in _sortHandler as well. https://codereview.chromium.org/2150713002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:495: this._dataGrid.setColumnsVisiblity(visibleColumns) semicolons please! https://codereview.chromium.org/2150713002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:524: this._loadColumns(); Why load all the time? Let's do it once during initialization. https://codereview.chromium.org/2150713002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:583: stray blank line
Patchset #3 (id:80001) has been deleted
PTAL https://codereview.chromium.org/2150713002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/common/Settings.js (right): https://codereview.chromium.org/2150713002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/common/Settings.js:724: var newSetting = WebInspector.settings.createSetting("networkLogColumns", {}) On 2016/07/14 05:28:06, dgozman wrote: > semicolons please! Done. https://codereview.chromium.org/2150713002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js (left): https://codereview.chromium.org/2150713002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:328: this._networkLogView.dataGridSorted(); On 2016/07/14 05:28:06, dgozman wrote: > Where did this call go? Done. https://codereview.chromium.org/2150713002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:344: this._networkLogView.dataGridSorted(); On 2016/07/14 05:28:06, dgozman wrote: > And this? Done. https://codereview.chromium.org/2150713002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js (right): https://codereview.chromium.org/2150713002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:17: /** @type {!Array.<!Element>} */ On 2016/07/14 05:28:06, dgozman wrote: > Modern syntax requires omitting dot before "<". Please convert as you go. Done. https://codereview.chromium.org/2150713002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:44: * subTitle: string, On 2016/07/14 05:28:06, dgozman wrote: > "subtitle" is widespread in our code. Done. https://codereview.chromium.org/2150713002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:48: * nonSelectable: boolean, On 2016/07/14 05:28:06, dgozman wrote: > selectable? It is how WebInspector.DataGrid.ColumnDescriptor does it. Since this is a superset it needs to be the same. https://codereview.chromium.org/2150713002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:52: * sortConfig: (!WebInspector.NetworkLogViewColumns.SortConfig) On 2016/07/14 05:28:07, dgozman wrote: > No need in extra () Done. https://codereview.chromium.org/2150713002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:55: WebInspector.NetworkLogViewColumns.ColumnDescriptor; On 2016/07/14 05:28:06, dgozman wrote: > I think we may avoid mentioning "column" twice. Done. https://codereview.chromium.org/2150713002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:75: subTitle: null, On 2016/07/14 05:28:06, dgozman wrote: > null is not a {string} Done. https://codereview.chromium.org/2150713002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:172: id: "setCookies", On 2016/07/14 05:28:06, dgozman wrote: > Shouldn't all these be lowercase (based on code in Settings.js) ? Done. https://codereview.chromium.org/2150713002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:218: sortable: false, On 2016/07/14 05:28:07, dgozman wrote: > sortable: false, but with sortConfig? > Let's remove sortable, and rely on the presence of sortConfig instead. Because this is a superset of WebInspector.DataGrid.ColumnDescriptor we should keep it here. https://codereview.chromium.org/2150713002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:335: WebInspector.NetworkLogViewColumns._orderrideableConfigs = ["visible", "title"]; On 2016/07/14 05:28:07, dgozman wrote: > I don't get the "orderrideable". Let's rename. Done. https://codereview.chromium.org/2150713002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:352: WRONG!!!! On 2016/07/14 05:28:06, dgozman wrote: > It's wrong! Done. https://codereview.chromium.org/2150713002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:367: this._columns.push(columnConfig); On 2016/07/14 05:28:07, dgozman wrote: > Create this._columns in this method. Done. https://codereview.chromium.org/2150713002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:395: if (columnConfig.sortConfig && columnConfig.sortConfig.entries) { On 2016/07/14 05:28:07, dgozman wrote: > if (!...) continue; Done. https://codereview.chromium.org/2150713002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:398: placeHolderOption.style.display = "none"; On 2016/07/14 05:28:06, dgozman wrote: > Toggle hidden class instead. Done. https://codereview.chromium.org/2150713002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:422: if (columnConfig.sortConfig.entries) { On 2016/07/14 05:28:06, dgozman wrote: > if (!...) continue; Done. https://codereview.chromium.org/2150713002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:427: entry.calculator = timeCalculator; On 2016/07/14 05:28:06, dgozman wrote: > This is weird. Let's use different fields for calculator vs calculatorType. You > can also just have a map{type=>calculator} and bother updating all the columns > at all. Done. https://codereview.chromium.org/2150713002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:457: this._networkLogView.removeAllNodeHighlights(); On 2016/07/14 05:28:06, dgozman wrote: > You probably want to do this in _sortHandler as well. Done. https://codereview.chromium.org/2150713002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:495: this._dataGrid.setColumnsVisiblity(visibleColumns) On 2016/07/14 05:28:07, dgozman wrote: > semicolons please! Done. https://codereview.chromium.org/2150713002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:524: this._loadColumns(); On 2016/07/14 05:28:07, dgozman wrote: > Why load all the time? Let's do it once during initialization. loadColumns() re-syncs from the settings. I am following the same way it was doing it before. https://codereview.chromium.org/2150713002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:583: On 2016/07/14 05:28:07, dgozman wrote: > stray blank line Done.
https://codereview.chromium.org/2150713002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js (right): https://codereview.chromium.org/2150713002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:218: sortable: false, On 2016/07/14 22:27:08, Blaise wrote: > On 2016/07/14 05:28:07, dgozman wrote: > > sortable: false, but with sortConfig? > > Let's remove sortable, and rely on the presence of sortConfig instead. > > Because this is a superset of WebInspector.DataGrid.ColumnDescriptor we should > keep it here. We should either force JSCompiler to check this for us, or copy the fields explicitly. https://codereview.chromium.org/2150713002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:524: this._loadColumns(); On 2016/07/14 22:27:08, Blaise wrote: > On 2016/07/14 05:28:07, dgozman wrote: > > Why load all the time? Let's do it once during initialization. > > loadColumns() re-syncs from the settings. I am following the same way it was > doing it before. Well, I'm not sure that old way was good. Let's improve! https://codereview.chromium.org/2150713002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/common/Settings.js (right): https://codereview.chromium.org/2150713002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/common/Settings.js:702: _updateVersionFrom18To19: function() Blank line before function please. https://codereview.chromium.org/2150713002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/common/Settings.js:726: visibleColumnSettings.remove() semicolons please! https://codereview.chromium.org/2150713002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js (right): https://codereview.chromium.org/2150713002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:315: this._columns.sort(); Why sort? https://codereview.chromium.org/2150713002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js (left): https://codereview.chromium.org/2150713002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:257: this._networkLogView.switchViewMode(true); Where did this call go? https://codereview.chromium.org/2150713002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:327: this._timelineSortSelector.selectedIndex = 0; Where did this go? https://codereview.chromium.org/2150713002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:393: timelineSorting.selectedIndex = 1; Where did this go? https://codereview.chromium.org/2150713002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js (right): https://codereview.chromium.org/2150713002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:335: WebInspector.NetworkLogViewColumns._saveableConfigs = ["visible", "title"]; I think we'd better copy them explicitly. https://codereview.chromium.org/2150713002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:419: sort: function() sortByCurrentColumn https://codereview.chromium.org/2150713002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:470: if (this._columns[0].id !== "name") { Can we avoid this? If name is initially first and timeline last, and we always add columns right before the last, it should be fine. https://codereview.chromium.org/2150713002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:482: visibleColumns[columnConfig.id] = (columnConfig.visible || !columnConfig.hideable) Semicolons please! Can we fix this once and forever? https://codereview.chromium.org/2150713002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:579: columnConfigs.filter(columnConfig => !columnConfig.isResponseHeader) I'm pretty sure that replacing these two fancy functional sections with for-loop will be less and much more readable code. https://codereview.chromium.org/2150713002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:692: var calculator = this._calculatorsMap.get(WebInspector.NetworkLogViewColumns._calculatorTypes.Time);; extra semicolon!
PTL https://codereview.chromium.org/2150713002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/common/Settings.js (right): https://codereview.chromium.org/2150713002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/common/Settings.js:702: _updateVersionFrom18To19: function() On 2016/07/15 23:47:49, dgozman wrote: > Blank line before function please. Done. https://codereview.chromium.org/2150713002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/common/Settings.js:726: visibleColumnSettings.remove() On 2016/07/15 23:47:49, dgozman wrote: > semicolons please! Done. https://codereview.chromium.org/2150713002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js (right): https://codereview.chromium.org/2150713002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:315: this._columns.sort(); On 2016/07/15 23:47:49, dgozman wrote: > Why sort? This is sorting the data not the columns themselves. It is to ensure if the view is displayed with data in it, it will resort the data. Normally sort is called when data is received, but in the case that it has data already it must be sorted before displayed. https://codereview.chromium.org/2150713002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js (left): https://codereview.chromium.org/2150713002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:257: this._networkLogView.switchViewMode(true); On 2016/07/15 23:47:50, dgozman wrote: > Where did this call go? the needed function this was executing was _updateColumns(). By executing switchViewMode() it executes _updateColumns(). This function is being addressed on line 382 (few lines below). Also the _updateColumns() must be executed after the dom is attached to page, so it is done after asWidget().show(). https://codereview.chromium.org/2150713002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:327: this._timelineSortSelector.selectedIndex = 0; On 2016/07/15 23:47:50, dgozman wrote: > Where did this go? See comment below about selectedIndex https://codereview.chromium.org/2150713002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:393: timelineSorting.selectedIndex = 1; On 2016/07/15 23:47:50, dgozman wrote: > Where did this go? The way it works with the drop down timeline changed a lot. Now it has a hidden first item and hot swaps the last selected item's content into it's value then after we select the item it changes it then selects the first index 0 item. So now selectedIndex will always be 0 since the onchange event will set it to 0 when it is done handling the change. https://codereview.chromium.org/2150713002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js (right): https://codereview.chromium.org/2150713002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:335: WebInspector.NetworkLogViewColumns._saveableConfigs = ["visible", "title"]; On 2016/07/15 23:47:50, dgozman wrote: > I think we'd better copy them explicitly. I am fairly ok with doing so, but I am certain it would be put in this manner in the next patch (custom columns) since we will need to know what settings are savable. I am not a big fan of hard coding anything. https://codereview.chromium.org/2150713002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:419: sort: function() On 2016/07/15 23:47:50, dgozman wrote: > sortByCurrentColumn Done. https://codereview.chromium.org/2150713002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:470: if (this._columns[0].id !== "name") { On 2016/07/15 23:47:50, dgozman wrote: > Can we avoid this? If name is initially first and timeline last, and we always > add columns right before the last, it should be fine. I did this in prep to allow re-ordering of columns. Since the data is being loaded in whatever order it is in the json object ordering is (coming from Settings). Since this code works here like this and I don't think it's very ugly and if/when the reorder of the columns would not allow name and timeline to be changed I thought this was fine. https://codereview.chromium.org/2150713002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:482: visibleColumns[columnConfig.id] = (columnConfig.visible || !columnConfig.hideable) On 2016/07/15 23:47:50, dgozman wrote: > Semicolons please! Can we fix this once and forever? Done. https://codereview.chromium.org/2150713002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:579: columnConfigs.filter(columnConfig => !columnConfig.isResponseHeader) On 2016/07/15 23:47:50, dgozman wrote: > I'm pretty sure that replacing these two fancy functional sections with for-loop > will be less and much more readable code. Done. https://codereview.chromium.org/2150713002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:692: var calculator = this._calculatorsMap.get(WebInspector.NetworkLogViewColumns._calculatorTypes.Time);; On 2016/07/15 23:47:50, dgozman wrote: > extra semicolon! Done.
https://codereview.chromium.org/2150713002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js (right): https://codereview.chromium.org/2150713002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:335: WebInspector.NetworkLogViewColumns._saveableConfigs = ["visible", "title"]; On 2016/07/25 23:05:53, Blaise wrote: > On 2016/07/15 23:47:50, dgozman wrote: > > I think we'd better copy them explicitly. > > I am fairly ok with doing so, but I am certain it would be put in this manner in > the next patch (custom columns) since we will need to know what settings are > savable. I am not a big fan of hard coding anything. Let's do copyFromOneToAnother() with explicit assignments. https://codereview.chromium.org/2150713002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:470: if (this._columns[0].id !== "name") { On 2016/07/25 23:05:53, Blaise wrote: > On 2016/07/15 23:47:50, dgozman wrote: > > Can we avoid this? If name is initially first and timeline last, and we always > > add columns right before the last, it should be fine. > > I did this in prep to allow re-ordering of columns. Since the data is being > loaded in whatever order it is in the json object ordering is (coming from > Settings). Since this code works here like this and I don't think it's very ugly > and if/when the reorder of the columns would not allow name and timeline to be > changed I thought this was fine. Let's do isMutable(column). https://codereview.chromium.org/2150713002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js (right): https://codereview.chromium.org/2150713002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:364: var columnConfig = /** @type {!WebInspector.NetworkLogViewColumns.Descriptor} */ (Object.assign(/** @type {!Object} */ ({}), defaultColumnConfig, currentConfigColumn)); Let's construct it explicitly and rename nonSelectable?
PTL https://codereview.chromium.org/2150713002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js (right): https://codereview.chromium.org/2150713002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:335: WebInspector.NetworkLogViewColumns._saveableConfigs = ["visible", "title"]; On 2016/07/27 17:47:56, dgozman wrote: > On 2016/07/25 23:05:53, Blaise wrote: > > On 2016/07/15 23:47:50, dgozman wrote: > > > I think we'd better copy them explicitly. > > > > I am fairly ok with doing so, but I am certain it would be put in this manner > in > > the next patch (custom columns) since we will need to know what settings are > > savable. I am not a big fan of hard coding anything. > > Let's do copyFromOneToAnother() with explicit assignments. Decided to inline it since both functions are next to eachother and only used in this file. https://codereview.chromium.org/2150713002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:470: if (this._columns[0].id !== "name") { On 2016/07/27 17:47:56, dgozman wrote: > On 2016/07/25 23:05:53, Blaise wrote: > > On 2016/07/15 23:47:50, dgozman wrote: > > > Can we avoid this? If name is initially first and timeline last, and we > always > > > add columns right before the last, it should be fine. > > > > I did this in prep to allow re-ordering of columns. Since the data is being > > loaded in whatever order it is in the json object ordering is (coming from > > Settings). Since this code works here like this and I don't think it's very > ugly > > and if/when the reorder of the columns would not allow name and timeline to be > > changed I thought this was fine. > > Let's do isMutable(column). Since the next patch will be introducing custom columns I'll figure out how to handle this exception in that patch. https://codereview.chromium.org/2150713002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js (right): https://codereview.chromium.org/2150713002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:364: var columnConfig = /** @type {!WebInspector.NetworkLogViewColumns.Descriptor} */ (Object.assign(/** @type {!Object} */ ({}), defaultColumnConfig, currentConfigColumn)); On 2016/07/27 17:47:56, dgozman wrote: > Let's construct it explicitly and rename nonSelectable? I am not going to change nonSelectable because it will force everywhere that uses the DataGrid's Descriptor to set "selectable" to true. I think this is a bad idea. I also think setting ours to "selectable" seems wrong because the translation will be inversed and I think that will introduce more complexity.
PTL
lgtm
The CQ bit was checked by allada@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [Devtools] Refactor NetworkLogViewColumns to be dynamic This patch is to make the columns in network panel much more dynamic. This is in prep to allow custom columns. Related: https://codereview.chromium.org/2118663002/ BUG=540569 ========== to ========== [Devtools] Refactor NetworkLogViewColumns to be dynamic This patch is to make the columns in network panel much more dynamic. This is in prep to allow custom columns. Related: https://codereview.chromium.org/2118663002/ BUG=540569 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== [Devtools] Refactor NetworkLogViewColumns to be dynamic This patch is to make the columns in network panel much more dynamic. This is in prep to allow custom columns. Related: https://codereview.chromium.org/2118663002/ BUG=540569 ========== to ========== [Devtools] Refactor NetworkLogViewColumns to be dynamic This patch is to make the columns in network panel much more dynamic. This is in prep to allow custom columns. Related: https://codereview.chromium.org/2118663002/ BUG=540569 Committed: https://crrev.com/cc69422e7e1a446688a66bff3c6b4eb701462508 Cr-Commit-Position: refs/heads/master@{#409129} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/cc69422e7e1a446688a66bff3c6b4eb701462508 Cr-Commit-Position: refs/heads/master@{#409129} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
