Issue 701153002: DevTools: [Elements] Highlight DOM updates

Issue 701153002: DevTools: [Elements] Highlight DOM updates (Closed)

Created:
6 years, 1 month ago by apavlov

Modified:
6 years, 1 month ago

Reviewers:
vsevik, pfeldman

CC:
blink-reviews, caseq+blink_chromium.org, loislo+blink_chromium.org, eustas+blink_chromium.org, malch+blink_chromium.org, yurys+blink_chromium.org, lushnikov+blink_chromium.org, vsevik+blink_chromium.org, pfeldman+blink_chromium.org, paulirish+reviews_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, sergeyv+blink_chromium.org, aandrey+blink_chromium.org

Base URL:
svn://svn.chromium.org/blink/trunk

Project:
blink

Visibility:
Public.

More Reviews

Description

DevTools: [Elements] Highlight DOM updates R=vsevik BUG=424085 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=185115

Patch Set 1 #

Patch Set 2 : Don't access |document| directly #

Total comments: 6

Patch Set 3 : Address comments, add setting #

Total comments: 10

Patch Set 4 : Reworked the way titles are updated (through updateTitle() now) #

Total comments: 14

Patch Set 5 : Address more comments #

Patch Set 6 : Refactored UpdateInfo fill-in and their use when building titles #

Total comments: 16

Patch Set 7 : Address comments from vsevik #

Patch Set 8 : Fix tests (for ETO not wired to DOMModel) #

Created: 6 years, 1 month ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+511 lines, -37 lines)			Patch
M	LayoutTests/http/tests/inspector/elements-test.js	View	1 2 3 4	1 chunk	+39 lines, -0 lines	0 comments	Download
A	LayoutTests/inspector/elements/highlight-dom-updates.html	View	1 2 3 4	1 chunk	+170 lines, -0 lines	0 comments	Download
A	LayoutTests/inspector/elements/highlight-dom-updates-expected.txt	View	1 2 3 4	1 chunk	+46 lines, -0 lines	0 comments	Download
M	Source/devtools/front_end/common/Settings.js	View	1 2	1 chunk	+1 line, -0 lines	0 comments	Download
M	Source/devtools/front_end/elements/ElementsTreeOutline.js	View	1 2 3 4 5 6 7	19 chunks	+217 lines, -34 lines	0 comments	Download
M	Source/devtools/front_end/elements/elementsTreeOutline.css	View	1 2 3	1 chunk	+13 lines, -0 lines	0 comments	Download
M	Source/devtools/front_end/elements/module.json	View	1 2 3 4	1 chunk	+7 lines, -0 lines	0 comments	Download
M	Source/devtools/front_end/externs.js	View	1 2	4 chunks	+18 lines, -3 lines	0 comments	Download

Messages

Total messages: 26 (9 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

apavlov

Screenshot: http://i.imgur.com/HTDLL5I.png - The highlights stick for 2 seconds, then disappear. - If an attr ...

6 years, 1 month ago (2014-11-05 15:24:55 UTC) #2

vsevik

https://chromiumcodereview.appspot.com/701153002/diff/20001/Source/devtools/front_end/elements/ElementsTreeOutline.js File Source/devtools/front_end/elements/ElementsTreeOutline.js (right): https://chromiumcodereview.appspot.com/701153002/diff/20001/Source/devtools/front_end/elements/ElementsTreeOutline.js#newcode500 Source/devtools/front_end/elements/ElementsTreeOutline.js:500: var treeElement = this.findTreeElement(/** @type {!WebInspector.DOMNode} */ (entry[0])); You ...

6 years, 1 month ago (2014-11-05 20:40:24 UTC) #3

apavlov

6 years, 1 month ago (2014-11-06 10:47:03 UTC) #4

pfeldman

https://chromiumcodereview.appspot.com/701153002/diff/40001/Source/devtools/front_end/elements/ElementsTreeOutline.js File Source/devtools/front_end/elements/ElementsTreeOutline.js (right): https://chromiumcodereview.appspot.com/701153002/diff/40001/Source/devtools/front_end/elements/ElementsTreeOutline.js#newcode498 Source/devtools/front_end/elements/ElementsTreeOutline.js:498: for (var node of modifiedAttributes.keys()) { I think this ...

6 years, 1 month ago (2014-11-06 13:57:28 UTC) #6

apavlov

6 years, 1 month ago (2014-11-07 08:31:42 UTC) #7

pfeldman

https://codereview.chromium.org/701153002/diff/60001/Source/devtools/front_end/elements/ElementsTreeOutline.js File Source/devtools/front_end/elements/ElementsTreeOutline.js (right): https://codereview.chromium.org/701153002/diff/60001/Source/devtools/front_end/elements/ElementsTreeOutline.js#newcode1335 Source/devtools/front_end/elements/ElementsTreeOutline.js:1335: var newElement = this.insertChildElement(child, treeChildIndex, false, !!childUpdates && childUpdates.isNodeInserted(child)); ...

6 years, 1 month ago (2014-11-07 12:54:16 UTC) #8

apavlov

https://chromiumcodereview.appspot.com/701153002/diff/60001/Source/devtools/front_end/elements/ElementsTreeOutline.js File Source/devtools/front_end/elements/ElementsTreeOutline.js (right): https://chromiumcodereview.appspot.com/701153002/diff/60001/Source/devtools/front_end/elements/ElementsTreeOutline.js#newcode1335 Source/devtools/front_end/elements/ElementsTreeOutline.js:1335: var newElement = this.insertChildElement(child, treeChildIndex, false, !!childUpdates && childUpdates.isNodeInserted(child)); ...

6 years, 1 month ago (2014-11-10 09:52:56 UTC) #9

https://chromiumcodereview.appspot.com/701153002/diff/60001/Source/devtools/f...
File Source/devtools/front_end/elements/ElementsTreeOutline.js (right):

https://chromiumcodereview.appspot.com/701153002/diff/60001/Source/devtools/f...
Source/devtools/front_end/elements/ElementsTreeOutline.js:1335: var newElement =
this.insertChildElement(child, treeChildIndex, false, !!childUpdates &&
childUpdates.isNodeInserted(child));
On 2014/11/07 12:54:15, pfeldman wrote:
> Can you do something like:
> 
> if (childUpdates.isNodeInserted())
>     newElement.listItemElement.mыrg(); ?

Done something along these lines with mыrging...

https://chromiumcodereview.appspot.com/701153002/diff/60001/Source/devtools/f...
Source/devtools/front_end/elements/ElementsTreeOutline.js:2852:
this._nodeModified(node,
WebInspector.ElementsTreeUpdater.Record.ChangeType.AttrModified,
event.data.name);
On 2014/11/07 12:54:15, pfeldman wrote:
> Lets inline record creation here.

This will result in one record per single DOM update event, which may cause
severe GC churns on highly dynamic pages (tens of thousands of short-lived
objects.)

https://chromiumcodereview.appspot.com/701153002/diff/60001/Source/devtools/f...
Source/devtools/front_end/elements/ElementsTreeOutline.js:2900:
this._parentNodeModified(node,
WebInspector.ElementsTreeUpdater.Record.ChangeType.ChildNodeCountUpdated);
On 2014/11/07 12:54:15, pfeldman wrote:
> Lets not use this signal.

Done.

https://chromiumcodereview.appspot.com/701153002/diff/60001/Source/devtools/f...
Source/devtools/front_end/elements/ElementsTreeOutline.js:2934:
nodeItem.updateTitle(false, highlightDOMUpdates ?
this._recentlyModifiedNodes.get(nodes[i]) : undefined);
On 2014/11/07 12:54:15, pfeldman wrote:
> updateTitle can reach out for it on its own

Done.

https://chromiumcodereview.appspot.com/701153002/diff/60001/Source/devtools/f...
Source/devtools/front_end/elements/ElementsTreeOutline.js:2941:
parentNodeItem.updateChildren(false, highlightDOMUpdates ?
this._recentlyModifiedParentNodes.get(parentNodes[i]) : undefined);
On 2014/11/07 12:54:16, pfeldman wrote:
> ditto

Done.

https://chromiumcodereview.appspot.com/701153002/diff/60001/Source/devtools/f...
Source/devtools/front_end/elements/ElementsTreeOutline.js:2975:
WebInspector.ElementsTreeUpdater.Record.prototype = {
On 2014/11/07 12:54:15, pfeldman wrote:
> Since you merge those, I'd call it UpdateInfo

Done.

https://chromiumcodereview.appspot.com/701153002/diff/60001/Source/devtools/f...
Source/devtools/front_end/elements/ElementsTreeOutline.js:2991: if
(!this.modifiedAttributes)
On 2014/11/07 12:54:15, pfeldman wrote:
> make them all private.

Done.

apavlov

Modified UpdateInfo construction as suggested by pfeldman.

6 years, 1 month ago (2014-11-10 15:22:24 UTC) #10

pfeldman

lgtm https://codereview.chromium.org/701153002/diff/100001/Source/devtools/front_end/elements/ElementsTreeOutline.js File Source/devtools/front_end/elements/ElementsTreeOutline.js (right): https://codereview.chromium.org/701153002/diff/100001/Source/devtools/front_end/elements/ElementsTreeOutline.js#newcode1384 Source/devtools/front_end/elements/ElementsTreeOutline.js:1384: this.updateTitle(false); revert https://codereview.chromium.org/701153002/diff/100001/Source/devtools/front_end/elements/ElementsTreeOutline.js#newcode2184 Source/devtools/front_end/elements/ElementsTreeOutline.js:2184: updateTitle: function(onlySearchQueryChanged, updates) revert ...

6 years, 1 month ago (2014-11-10 15:38:21 UTC) #11

vsevik

https://codereview.chromium.org/701153002/diff/100001/Source/devtools/front_end/elements/ElementsTreeOutline.js File Source/devtools/front_end/elements/ElementsTreeOutline.js (right): https://codereview.chromium.org/701153002/diff/100001/Source/devtools/front_end/elements/ElementsTreeOutline.js#newcode2976 Source/devtools/front_end/elements/ElementsTreeOutline.js:2976: var nodes = this._recentlyModifiedNodes.keysArray(); Let's switch to for-of while ...

6 years, 1 month ago (2014-11-10 16:00:51 UTC) #12

apavlov

https://codereview.chromium.org/701153002/diff/100001/Source/devtools/front_end/elements/ElementsTreeOutline.js File Source/devtools/front_end/elements/ElementsTreeOutline.js (right): https://codereview.chromium.org/701153002/diff/100001/Source/devtools/front_end/elements/ElementsTreeOutline.js#newcode1384 Source/devtools/front_end/elements/ElementsTreeOutline.js:1384: this.updateTitle(false); On 2014/11/10 15:38:20, pfeldman wrote: > revert Done. ...

6 years, 1 month ago (2014-11-10 16:07:37 UTC) #13