[Devtools] Seperate columns out of NetworkLogView into own file (issue 2118663002 by allada@chromium.org)

all...@chromium.org

unread,

Jun 30, 2016, 8:37:53 PM6/30/16

to dgo...@chromium.org, chromium...@chromium.org, caseq...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, apavlo...@chromium.org, devtools...@chromium.org, blink-...@chromium.org, sergey...@chromium.org, pfel...@chromium.org, kozyatins...@chromium.org

Reviewers: dgozman
CL: https://codereview.chromium.org/2118663002/

Message:
PTL

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

Base URL: https://chromium.googlesource.com/chromium/src.git@master

Affected files (+677, -527 lines):
M third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js
A third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js
M third_party/WebKit/Source/devtools/front_end/network/module.json

dgo...@chromium.org

unread,

Jun 30, 2016, 8:54:24 PM6/30/16

to all...@chromium.org, chromium...@chromium.org, caseq...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, apavlo...@chromium.org, devtools...@chromium.org, blink-...@chromium.org, sergey...@chromium.org, pfel...@chromium.org, kozyatins...@chromium.org

all...@google.com

unread,

Jun 30, 2016, 9:28:39 PM6/30/16

to all...@chromium.org, dgo...@chromium.org, chromium...@chromium.org, caseq...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, apavlo...@chromium.org, devtools...@chromium.org, blink-...@chromium.org, sergey...@chromium.org, pfel...@chromium.org, kozyatins...@chromium.org

PTL. After I will re-order the functions.

https://codereview.chromium.org/2118663002/diff/40001/third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js
File
third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js
(right):

https://codereview.chromium.org/2118663002/diff/40001/third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js#newcode54
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/Source/devtools/front_end/network/NetworkLogView.js#newcode254
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/Source/devtools/front_end/network/NetworkLogView.js#newcode313
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/Source/devtools/front_end/network/NetworkLogView.js#newcode370
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/Source/devtools/front_end/network/NetworkLogView.js#newcode1034
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/Source/devtools/front_end/network/NetworkLogView.js#newcode1037
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/Source/devtools/front_end/network/NetworkLogViewColumns.js
File
third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js
(right):

https://codereview.chromium.org/2118663002/diff/40001/third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js#newcode115
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/Source/devtools/front_end/network/NetworkLogViewColumns.js#newcode125
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/Source/devtools/front_end/network/NetworkLogViewColumns.js#newcode137
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/Source/devtools/front_end/network/NetworkLogViewColumns.js#newcode212
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/

dgo...@chromium.org

unread,

Jul 5, 2016, 2:12:31 PM7/5/16

to all...@chromium.org, all...@google.com, chromium...@chromium.org, caseq...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, apavlo...@chromium.org, devtools...@chromium.org, blink-...@chromium.org, sergey...@chromium.org, pfel...@chromium.org, kozyatins...@chromium.org

https://codereview.chromium.org/2118663002/diff/60001/third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js
File
third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js
(right):

https://codereview.chromium.org/2118663002/diff/60001/third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js#newcode316
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/Source/devtools/front_end/network/NetworkLogView.js#newcode838
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/Source/devtools/front_end/network/NetworkLogView.js#newcode1025
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/Source/devtools/front_end/network/NetworkLogViewColumns.js
File
third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js
(right):

https://codereview.chromium.org/2118663002/diff/60001/third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js#newcode105
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/Source/devtools/front_end/network/NetworkLogViewColumns.js#newcode132
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/Source/devtools/front_end/network/NetworkLogViewColumns.js#newcode612
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.

https://codereview.chromium.org/2118663002/

all...@chromium.org

unread,

Jul 6, 2016, 9:17:33 PM7/6/16

to dgo...@chromium.org, all...@google.com, chromium...@chromium.org, caseq...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, apavlo...@chromium.org, devtools...@chromium.org, blink-...@chromium.org, sergey...@chromium.org, pfel...@chromium.org, kozyatins...@chromium.org

PTL

https://codereview.chromium.org/2118663002/

dgo...@chromium.org

unread,

Jul 7, 2016, 12:21:24 PM7/7/16

to all...@chromium.org, all...@google.com, chromium...@chromium.org, caseq...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, apavlo...@chromium.org, devtools...@chromium.org, blink-...@chromium.org, sergey...@chromium.org, pfel...@chromium.org, kozyatins...@chromium.org

all...@chromium.org

unread,

Jul 7, 2016, 12:46:45 PM7/7/16

to dgo...@chromium.org, all...@google.com, chromium...@chromium.org, caseq...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, apavlo...@chromium.org, devtools...@chromium.org, blink-...@chromium.org, sergey...@chromium.org, pfel...@chromium.org, kozyatins...@chromium.org

done

https://codereview.chromium.org/2118663002/diff/120001/third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js
File
third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js
(right):

https://codereview.chromium.org/2118663002/diff/120001/third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js#newcode500
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/Source/devtools/front_end/network/NetworkLogViewColumns.js
File
third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js
(right):

https://codereview.chromium.org/2118663002/diff/120001/third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js#newcode116
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/Source/devtools/front_end/network/module.json
File third_party/WebKit/Source/devtools/front_end/network/module.json
(right):

https://codereview.chromium.org/2118663002/diff/120001/third_party/WebKit/Source/devtools/front_end/network/module.json#newcode110
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.

https://codereview.chromium.org/2118663002/

commit-bot@chromium.org via codereview.chromium.org

unread,

Jul 7, 2016, 3:06:19 PM7/7/16

to all...@chromium.org, dgo...@chromium.org, all...@google.com, commi...@chromium.org, chromium...@chromium.org, caseq...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, apavlo...@chromium.org, devtools...@chromium.org, blink-...@chromium.org, sergey...@chromium.org, pfel...@chromium.org, kozyatins...@chromium.org

CQ is trying da patch. Follow status at

https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2118663002/160001

https://codereview.chromium.org/2118663002/

commit-bot@chromium.org via codereview.chromium.org

unread,

Jul 7, 2016, 3:12:47 PM7/7/16

to all...@chromium.org, dgo...@chromium.org, all...@google.com, commi...@chromium.org, chromium...@chromium.org, caseq...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, apavlo...@chromium.org, devtools...@chromium.org, blink-...@chromium.org, sergey...@chromium.org, pfel...@chromium.org, kozyatins...@chromium.org

Committed patchset #5 (id:160001)

https://codereview.chromium.org/2118663002/

commit-bot@chromium.org via codereview.chromium.org

unread,

Jul 7, 2016, 3:12:52 PM7/7/16

to all...@chromium.org, dgo...@chromium.org, all...@google.com, commi...@chromium.org, chromium...@chromium.org, caseq...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, apavlo...@chromium.org, devtools...@chromium.org, blink-...@chromium.org, sergey...@chromium.org, pfel...@chromium.org, kozyatins...@chromium.org

CQ bit was unchecked.

https://codereview.chromium.org/2118663002/

commit-bot@chromium.org via codereview.chromium.org

unread,

Jul 7, 2016, 3:14:20 PM7/7/16

to all...@chromium.org, dgo...@chromium.org, all...@google.com, commi...@chromium.org, chromium...@chromium.org, caseq...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, apavlo...@chromium.org, devtools...@chromium.org, blink-...@chromium.org, sergey...@chromium.org, pfel...@chromium.org, kozyatins...@chromium.org

Patchset 5 (id:??) landed as
https://crrev.com/24f72580334bcbfe0fd4038dea65c2a1df910d31
Cr-Commit-Position: refs/heads/master@{#404203}

https://codereview.chromium.org/2118663002/

Reply all

Reply to author

Forward