Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2118663002:
[Devtools] Seperate columns out of NetworkLogView into own file (Closed)
|
Created:
4 years, 5 months ago by allada Modified:
4 years, 5 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] Seperate columns out of NetworkLogView into own file
NetworkLogView is a bit bloated. This patch is to seperate column
specific stuff out of NetworkLogView.js into it's file.
BUG=624988
Committed: https://crrev.com/24f72580334bcbfe0fd4038dea65c2a1df910d31
Cr-Commit-Position: refs/heads/master@{#404203}
Patch Set 1 : Cleanup NetworkLogView #
Total comments: 20
Patch Set 2 : Revisions #
Total comments: 12
Patch Set 3 : Cleanup NetworkLogView #
Total comments: 6
Patch Set 4 : rebase #Patch Set 5 : Cleanup NetworkLogView #
Messages
Total messages: 25 (11 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
allada@chromium.org changed reviewers: + dgozman@chromium.org
PTL
https://codereview.chromium.org/2118663002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js (right): https://codereview.chromium.org/2118663002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:54: this._columnsView = new WebInspector.NetworkLogViewColumns(this); this._columns https://codereview.chromium.org/2118663002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:254: this._columnsView.setupColumns(); Let's combine initializeView with setupColumns? https://codereview.chromium.org/2118663002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:313: this._columnsView.setDataGrid(this._dataGrid); DataGrid was created by columns, no need to set it. https://codereview.chromium.org/2118663002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:370: _createCalculators: function() Let's inline this function. https://codereview.chromium.org/2118663002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:1034: this.removeAllNodeHighlights(); Just call this one directly instead of onWillSort. https://codereview.chromium.org/2118663002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:1037: onSorted: function() dataGridSorted https://codereview.chromium.org/2118663002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js (right): https://codereview.chromium.org/2118663002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:115: this._createTimelineGrid(); Let's inline this into createDataGrid. https://codereview.chromium.org/2118663002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:125: initializeView: function() And combine with this. https://codereview.chromium.org/2118663002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:137: return this._dataGrid style: semicolon https://codereview.chromium.org/2118663002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:212: setupColumns: function() And also combine with this.
allada@google.com changed reviewers: + allada@google.com
PTL. After I will re-order the functions. https://codereview.chromium.org/2118663002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js (right): https://codereview.chromium.org/2118663002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:54: this._columnsView = new WebInspector.NetworkLogViewColumns(this); On 2016/07/01 00:54:23, dgozman wrote: > this._columns Done. https://codereview.chromium.org/2118663002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:254: this._columnsView.setupColumns(); On 2016/07/01 00:54:23, dgozman wrote: > Let's combine initializeView with setupColumns? Done. https://codereview.chromium.org/2118663002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:313: this._columnsView.setDataGrid(this._dataGrid); On 2016/07/01 00:54:23, dgozman wrote: > DataGrid was created by columns, no need to set it. Done. https://codereview.chromium.org/2118663002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:370: _createCalculators: function() On 2016/07/01 00:54:23, dgozman wrote: > Let's inline this function. Done. https://codereview.chromium.org/2118663002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:1034: this.removeAllNodeHighlights(); On 2016/07/01 00:54:23, dgozman wrote: > Just call this one directly instead of onWillSort. Done. https://codereview.chromium.org/2118663002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:1037: onSorted: function() On 2016/07/01 00:54:23, dgozman wrote: > dataGridSorted Done. https://codereview.chromium.org/2118663002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js (right): https://codereview.chromium.org/2118663002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:115: this._createTimelineGrid(); On 2016/07/01 00:54:24, dgozman wrote: > Let's inline this into createDataGrid. Done. https://codereview.chromium.org/2118663002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:125: initializeView: function() On 2016/07/01 00:54:24, dgozman wrote: > And combine with this. Done. https://codereview.chromium.org/2118663002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:137: return this._dataGrid On 2016/07/01 00:54:23, dgozman wrote: > style: semicolon Done. https://codereview.chromium.org/2118663002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:212: setupColumns: function() On 2016/07/01 00:54:23, dgozman wrote: > And also combine with this. Done.
https://codereview.chromium.org/2118663002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js (right): https://codereview.chromium.org/2118663002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:316: this._columns.setupGrid(); Can we move things around to avoid this extra method? Ideally, columns will create data grid in constructor, expose a getter for it, and that's it. https://codereview.chromium.org/2118663002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:838: if (this._columns.contextMenu(event)) Should columns add a context menu handler themselves? https://codereview.chromium.org/2118663002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:1025: dataGridSorted: function() Let's call this |highlightSearchResults| or similar. https://codereview.chromium.org/2118663002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js (right): https://codereview.chromium.org/2118663002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:105: this._popoverHelper.hidePopover(); Let's expose hidePopover() public method. This will allow to remove willHide and reset, as we already expose removeEventDividers and updateDividersIfNeeded. https://codereview.chromium.org/2118663002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:132: var timeCalculator = this._networkLogView.timeCalculator(); Let's pass these two as parameters and remove getters. https://codereview.chromium.org/2118663002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:612: rowSizeUpdated: function(isLargeRows) - rename to setLargeRows - even better, we should listen for a setting (it's a setting, right) in this class and avoid exposing this method.
Patchset #3 (id:80001) has been deleted
Patchset #3 (id:100001) has been deleted
PTL
lgtm https://codereview.chromium.org/2118663002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js (right): https://codereview.chromium.org/2118663002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:500: durationCalculator: function() Let's remove this. https://codereview.chromium.org/2118663002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js (right): https://codereview.chromium.org/2118663002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:116: createGrid: function(timeCalculator, durationCalculator) Annotate the parameters please. https://codereview.chromium.org/2118663002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/network/module.json (right): https://codereview.chromium.org/2118663002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/module.json:110: "NetworkLogViewColumns.js", Also add this file to devtools.gypi.
comments https://codereview.chromium.org/2118663002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js (right): https://codereview.chromium.org/2118663002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:316: this._columns.setupGrid(); On 2016/07/05 18:12:30, dgozman wrote: > Can we move things around to avoid this extra method? Ideally, columns will > create data grid in constructor, expose a getter for it, and that's it. Done. https://codereview.chromium.org/2118663002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:838: if (this._columns.contextMenu(event)) On 2016/07/05 18:12:30, dgozman wrote: > Should columns add a context menu handler themselves? On the surface this looks easy, but digging deeper it's a lot of work. Added a todo instead. https://codereview.chromium.org/2118663002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:1025: dataGridSorted: function() On 2016/07/05 18:12:30, dgozman wrote: > Let's call this |highlightSearchResults| or similar. Acknowledged. https://codereview.chromium.org/2118663002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js (right): https://codereview.chromium.org/2118663002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:105: this._popoverHelper.hidePopover(); On 2016/07/05 18:12:31, dgozman wrote: > Let's expose hidePopover() public method. This will allow to remove willHide and > reset, as we already expose removeEventDividers and updateDividersIfNeeded. As per our talk, we should try and keep these separate. https://codereview.chromium.org/2118663002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:132: var timeCalculator = this._networkLogView.timeCalculator(); On 2016/07/05 18:12:30, dgozman wrote: > Let's pass these two as parameters and remove getters. Done. https://codereview.chromium.org/2118663002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:612: rowSizeUpdated: function(isLargeRows) On 2016/07/05 18:12:30, dgozman wrote: > - rename to setLargeRows > - even better, we should listen for a setting (it's a setting, right) in this > class and avoid exposing this method. Done.
done https://codereview.chromium.org/2118663002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js (right): https://codereview.chromium.org/2118663002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:500: durationCalculator: function() On 2016/07/07 16:21:24, dgozman wrote: > Let's remove this. Done. https://codereview.chromium.org/2118663002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js (right): https://codereview.chromium.org/2118663002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:116: createGrid: function(timeCalculator, durationCalculator) On 2016/07/07 16:21:24, dgozman wrote: > Annotate the parameters please. Done. https://codereview.chromium.org/2118663002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/network/module.json (right): https://codereview.chromium.org/2118663002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/module.json:110: "NetworkLogViewColumns.js", On 2016/07/07 16:21:24, dgozman wrote: > Also add this file to devtools.gypi. Done.
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.
The CQ bit was checked by allada@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dgozman@chromium.org Link to the patchset: https://codereview.chromium.org/2118663002/#ps160001 (title: "Cleanup NetworkLogView")
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.
Committed patchset #5 (id:160001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== [Devtools] Seperate columns out of NetworkLogView into own file NetworkLogView is a bit bloated. This patch is to seperate column specific stuff out of NetworkLogView.js into it's file. BUG=624988 ========== to ========== [Devtools] Seperate columns out of NetworkLogView into own file NetworkLogView is a bit bloated. This patch is to seperate column specific stuff out of NetworkLogView.js into it's file. BUG=624988 Committed: https://crrev.com/24f72580334bcbfe0fd4038dea65c2a1df910d31 Cr-Commit-Position: refs/heads/master@{#404203} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/24f72580334bcbfe0fd4038dea65c2a1df910d31 Cr-Commit-Position: refs/heads/master@{#404203} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
