[Devtools] Changed ViewportDataGrid to use DataGrid for dom operations

[Devtools] Changed ViewportDataGrid to use DataGrid for dom operations

This patch makes ViewportDataGrid send dom operations to DataGrid
instead of doing them directly. It also ensures DataGrid's DOM
operations can be intercepted by ViewportDataGrid to process later.

R=caseq
BUG=None

[allada, 2017-03-14 00:58:09]

Patchset #2 (id:20001) has been deleted

[allada, 2017-03-14 00:58:14]

Patchset #2 (id:40001) has been deleted

[allada, 2017-03-14 00:58:20]

Patchset #2 (id:60001) has been deleted

[allada, 2017-03-14 00:58:25]

Patchset #2 (id:80001) has been deleted

[allada, 2017-03-14 00:58:30]

Patchset #1 (id:1) has been deleted

[allada, 2017-03-14 00:58:36]

PTL

[caseq, 2017-03-17 18:38:27]

caseq@chromium.org changed reviewers:
+ dgozman@chromium.org

[caseq, 2017-03-18 00:37:48]

https://codereview.chromium.org/2747893005/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/data_grid/DataGrid.js (right):

https://codereview.chromium.org/2747893005/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/data_grid/DataGrid.js:1617: if
(!this.children.length)
let's nuke this check, it's redundant.

https://codereview.chromium.org/2747893005/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/data_grid/DataGrid.js:1624: do {
for (var current = child; current; current = current.traverseNextNode(false,
child, true))?

Doing an extra loop condition check if perhaps fine here :-)

https://codereview.chromium.org/2747893005/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/data_grid/DataGrid.js:1637:
updateDOMStructure(previousParent) {
This one along with _isNodeParentOutOfSyncWithDOM are a bit sketchy. Perhaps
split it in two, one for insertion and one for removal, and thus be more
explicit about the expected state of the DOM in each case?

https://codereview.chromium.org/2747893005/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/data_grid/ViewportDataGrid.js
(right):

https://codereview.chromium.org/2747893005/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/data_grid/ViewportDataGrid.js:325:
updateDOMStructure(previousParent) {
This interface looks a bit strange to me. Should we just have something as
DataGridNode.willDetach() instead?

[allada, 2017-03-18 01:26:03]

PTaL - LMK if questions.

https://codereview.chromium.org/2747893005/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/data_grid/DataGrid.js (right):

https://codereview.chromium.org/2747893005/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/data_grid/DataGrid.js:1617: if
(!this.children.length)
On 2017/03/18 00:37:48, caseq wrote:
> let's nuke this check, it's redundant.

Done.

https://codereview.chromium.org/2747893005/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/data_grid/DataGrid.js:1624: do {
On 2017/03/18 00:37:48, caseq wrote:
> for (var current = child; current; current = current.traverseNextNode(false,
> child, true))?
> 
> Doing an extra loop condition check if perhaps fine here :-)

Done.

https://codereview.chromium.org/2747893005/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/data_grid/DataGrid.js:1637:
updateDOMStructure(previousParent) {
On 2017/03/18 00:37:48, caseq wrote:
> This one along with _isNodeParentOutOfSyncWithDOM are a bit sketchy. Perhaps
> split it in two, one for insertion and one for removal, and thus be more
> explicit about the expected state of the DOM in each case?

I did a mock this way, but I opted to put them together because it made it much
easier to understand from the ViewportDataGrid side of things. Since devtools
pretty much only uses ViewportDataGrid optimizing for what is best for the
viewport mode seemed like a better approach. Otherwise I'd need to have the
viewport figure out if it is in-sync, attached to dom, or removed from dom.

https://codereview.chromium.org/2747893005/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/data_grid/ViewportDataGrid.js
(right):

https://codereview.chromium.org/2747893005/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/data_grid/ViewportDataGrid.js:325:
updateDOMStructure(previousParent) {
On 2017/03/18 00:37:48, caseq wrote:
> This interface looks a bit strange to me. Should we just have something as
> DataGridNode.willDetach() instead?

The problem is that willDetach does not describe if it's being detached from DOM
or from the grid itself.

We want the DOM to be always updated immediately for DataGrid but easy for
ViewPortDataGrid to capture requests to update the node and schedule them later.
I found this was a very unintrusive way to do it.

[dgozman, 2017-03-20 17:58:37]

I think this patch changes too much logic at once, and since DataGrid is used
extensively through the frontend we have to be sure no regressions slip in.

Can we split it up into smaller chunks or cover it all with tests beforehand?
Better description would help as well.

[allada, 2017-03-22 22:38:53]

PTaL

[dgozman, 2017-03-23 20:57:43]

https://codereview.chromium.org/2747893005/diff/140001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/inspector-unit/viewport-datagrid-items-expandable-attached-to-dom-expected.txt
(right):

https://codereview.chromium.org/2747893005/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/inspector-unit/viewport-datagrid-items-expandable-attached-to-dom-expected.txt:49:
a0
Shouldn't this be depth:1 ?

https://codereview.chromium.org/2747893005/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/data_grid/DataGrid.js (right):

https://codereview.chromium.org/2747893005/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/data_grid/DataGrid.js:1634:
updateDOMStructure(previousParent) {
if (!this.dataGrid || (this.parent && !this.parent.expanded))
  this.detachFromDOM();
else
  this.attachToDOM();

https://codereview.chromium.org/2747893005/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/data_grid/DataGrid.js:1752: if
(this._attachedToDOM) {
Should we check for _isNodeParentOutOfSyncWithDOM here? Or just call
this.attachToDOM() to check it for us?
I'm worried that attaching children before attaching parent properly may be
wrong.

https://codereview.chromium.org/2747893005/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/data_grid/DataGrid.js:1921:
_isNodeParentOutOfSyncWithDOM() {
When does this happen? Could you please describe a specific scenario? Maybe
worth a comment?

https://codereview.chromium.org/2747893005/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/data_grid/DataGrid.js:1967: delete
this._depth;
I think _depth is not related to DOM at all, and should be updated in other
places.

[dgozman, 2017-07-15 05:49:07]

Description was changed from

==========
[Devtools] Changed ViewportDataGrid to use DataGrid for dom operations

This patch makes ViewportDataGrid send dom operations to DataGrid
instead of doing them directly. It also ensures DataGrid's DOM
operations can be intercepted by ViewportDataGrid to process later.

R=caseq
BUG=None
==========

to

==========
[Devtools] Changed ViewportDataGrid to use DataGrid for dom operations

This patch makes ViewportDataGrid send dom operations to DataGrid
instead of doing them directly. It also ensures DataGrid's DOM
operations can be intercepted by ViewportDataGrid to process later.

R=caseq
BUG=None
==========

[dgozman, 2017-07-15 05:49:07]

dgozman@chromium.org changed reviewers:
- dgozman@chromium.org