[DevTools] Set row background instead of the table

[DevTools] Set row background instead of the table

BUG=695197
Review-Url: https://codereview.chromium.org/2894083002
Cr-Commit-Position: refs/heads/master@{#474845}
Committed: https://chromium.googlesource.com/chromium/src/+/35a7e7d29a36ec864267db7c057aa12aaa69b7ce

[eostroukhov, 2017-05-18 22:31:18]

eostroukhov@chromium.org changed reviewers:
+ allada@chromium.org, alph@chromium.org, dgozman@chromium.org

[dgozman, 2017-05-19 19:27:42]

https://codereview.chromium.org/2894083002/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/data_grid/DataGrid.js (right):

https://codereview.chromium.org/2894083002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/data_grid/DataGrid.js:1943:
this.updateStripesBelowThisNode(previousNode ? !previousNode.isStriped() :
false);
Won't this make it O(n^2)?

https://codereview.chromium.org/2894083002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/data_grid/DataGrid.js:1967:
function next(node) {
JSDoc

[eostroukhov, 2017-05-20 00:31:45]

Thanks for the comments, addressed them. Please take another look.

https://codereview.chromium.org/2894083002/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/data_grid/DataGrid.js (right):

https://codereview.chromium.org/2894083002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/data_grid/DataGrid.js:1943:
this.updateStripesBelowThisNode(previousNode ? !previousNode.isStriped() :
false);
On 2017/05/19 19:27:42, dgozman wrote:
> Won't this make it O(n^2)?

I redid this code.

https://codereview.chromium.org/2894083002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/data_grid/DataGrid.js:1967:
function next(node) {
On 2017/05/19 19:27:42, dgozman wrote:
> JSDoc

Done.

[dgozman, 2017-05-22 18:45:27]

https://codereview.chromium.org/2894083002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/data_grid/DataGrid.js (right):

https://codereview.chromium.org/2894083002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/data_grid/DataGrid.js:1276:
this._isStriped = true;
Please add a test for this functionality, e.g. dump it in data-grid.html test.

https://codereview.chromium.org/2894083002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/data_grid/DataGrid.js:1732:
this._expanded = false;
update here as well.

https://codereview.chromium.org/2894083002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/data_grid/DataGrid.js:1744: }
And here?

https://codereview.chromium.org/2894083002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/data_grid/dataGrid.css
(right):

https://codereview.chromium.org/2894083002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/data_grid/dataGrid.css:69:
/*background-image: linear-gradient(to bottom, transparent, transparent 50%,
hsla(214, 100%, 40%, 0.05) 50%, hsla(214, 100%, 40%, 0.05));*/
Commented code.

[allada, 2017-05-22 19:10:29]

https://codereview.chromium.org/2894083002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/data_grid/DataGrid.js (right):

https://codereview.chromium.org/2894083002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/data_grid/DataGrid.js:1965: return
node.traverseNextNode(true, null, false);
Can we inline this?

https://codereview.chromium.org/2894083002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/data_grid/DataGrid.js:1998:
this.element().classList.toggle('data-grid-odd-row', isStriped);
Can we try something like this instead, so we don't force a node creation when
it may never be inserted into the dom:

if (this._element)
  this._element.classList.toggle('data-grid-odd-row', isStriped);

https://codereview.chromium.org/2894083002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/data_grid/ViewportDataGrid.js
(right):

https://codereview.chromium.org/2894083002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/data_grid/ViewportDataGrid.js:190:
if (visibleNodes[0]) {
nit: If length = 0 then v8 will deop, this should fix it.

if (visibleNodes.length) {

https://codereview.chromium.org/2894083002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/data_grid/ViewportDataGrid.js:192:
visibleNodes[0].updateStripesBelowThisNode();
Lets move this into Datagrid.Datagrid instead of the node to set an anchor node
for striping. This way we don't end up in an O(n^2) problem.

https://codereview.chromium.org/2894083002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/data_grid/dataGrid.css
(right):

https://codereview.chromium.org/2894083002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/data_grid/dataGrid.css:69:
/*background-image: linear-gradient(to bottom, transparent, transparent 50%,
hsla(214, 100%, 40%, 0.05) 50%, hsla(214, 100%, 40%, 0.05));*/
Remove this.

https://codereview.chromium.org/2894083002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/data_grid/dataGrid.css:79:
background-color: hsla(214, 100%, 40%, 0.05);
Is there a reason we are using alpha channel instead of just finding the
non-alpha color?

https://codereview.chromium.org/2894083002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/network/networkLogView.css
(right):

https://codereview.chromium.org/2894083002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/network/networkLogView.css:55:
.network-log-grid.data-grid .data-grid-odd-row {
Do we need this any more? I think the reason we were using table.data selector
was because dataGrid.css sets the background color to white or something. Maybe
we can just kill this rule?

[alph, 2017-05-22 19:11:46]

https://codereview.chromium.org/2894083002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/data_grid/DataGrid.js (right):

https://codereview.chromium.org/2894083002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/data_grid/DataGrid.js:1964:
function next(node) {
annotate plz

https://codereview.chromium.org/2894083002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/network/networkLogView.css
(right):

https://codereview.chromium.org/2894083002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/network/networkLogView.css:56:
background-color: transparent;
I'd rather put a 'striped-mode' class onto the data-grid that would enable the
striped mode.

[eostroukhov, 2017-05-24 18:03:08]

The CQ bit was checked by eostroukhov@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-24 18:04:19]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[eostroukhov, 2017-05-24 18:04:46]

Please take another look. I redid the implementation with what I believe is
easier/more maintainable.

Striped mode is now an opt-in as many clients already were removing the
background.

[commit-bot: I haz the power, 2017-05-24 20:28:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-24 20:28:42]

Dry run: This issue passed the CQ dry run.

[dgozman, 2017-05-25 00:12:04]

Please add a test. Code lgtm

https://codereview.chromium.org/2894083002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/data_grid/DataGrid.js (right):

https://codereview.chromium.org/2894083002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/data_grid/DataGrid.js:1301:
this._element = createElement('tr');
createElementWithClass('tr', '...');

[eostroukhov, 2017-05-25 21:38:23]

The CQ bit was checked by eostroukhov@chromium.org

[eostroukhov, 2017-05-25 21:38:23]

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/2894083002/#ps60001
(title: "Tests were added")

[eostroukhov, 2017-05-25 21:38:27]

The CQ bit was unchecked by eostroukhov@chromium.org

[eostroukhov, 2017-05-25 21:44:23]

The CQ bit was checked by eostroukhov@chromium.org

[eostroukhov, 2017-05-25 21:44:24]

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/2894083002/#ps80001
(title: "Fixed a comment")

[commit-bot: I haz the power, 2017-05-25 21:45:25]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-25 23:44:39]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1495748663716680,
"parent_rev": "05dd8e15fe84d5e8ec203f3f007d6a3e2d657cbf", "commit_rev":
"35a7e7d29a36ec864267db7c057aa12aaa69b7ce"}

[commit-bot: I haz the power, 2017-05-25 23:44:50]

Description was changed from

==========
[DevTools] Set row background instead of the table

BUG=695197
==========

to

==========
[DevTools] Set row background instead of the table

BUG=695197

Review-Url: https://codereview.chromium.org/2894083002
Cr-Commit-Position: refs/heads/master@{#474845}
Committed:
https://chromium.googlesource.com/chromium/src/+/35a7e7d29a36ec864267db7c057a...
==========

[commit-bot: I haz the power, 2017-05-25 23:44:52]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/35a7e7d29a36ec864267db7c057a...