[webkit-reviews] review requested: [Bug 127938] Web Inspector: Inspect Element sometimes does not select the right DOM Node : [Attachment 231603] [PaTCH] Proposed Fix: Flag to allow _restoreSelectedNodeAfterUpdate REV1

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has asked	for review:
Bug 127938: Web Inspector: Inspect Element sometimes does not select the right
DOM Node
https://bugs.webkit.org/show_bug.cgi?id=127938

Attachment 231603: [PaTCH] Proposed Fix: Flag to allow
_restoreSelectedNodeAfterUpdate REV1
https://bugs.webkit.org/attachment.cgi?id=231603&action=review

------- Additional Comments from Joseph Pecoraro <joepeck at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=231603&action=review


> Source/WebInspectorUI/UserInterface/Controllers/DOMTreeManager.js:39
> +    this._isRestoreSelectedNodeAllowed = true;

How about, "_selectedNodeRestorationAllowed"? WebKit style is typically to
avoid "is" on booleans.

> Source/WebInspectorUI/UserInterface/Controllers/DOMTreeManager.js:269
> +    get isRestoreSelectedNodeAllowed() {
> +	   return this._isRestoreSelectedNodeAllowed;
> +    },

Style: "{" on newline.

> Source/WebInspectorUI/UserInterface/Views/FrameDOMTreeContentView.js:75
> +	   if (WebInspector.domTreeManager.isRestoreSelectedNodeAllowed)
> +	       this._restoreSelectedNodeAfterUpdate(this._domTree.frame.url,
rootDOMNode.body || rootDOMNode.documentElement);

This seems like the wrong point to check the boolean state.

    - _restoreSelectedNodeAfterUpdate is called in multiple places. Do we want
to respect this state in all places it is called?
    - _restoreSelectedNodeAfterUpdate may be called when this state is true,
but run its async operation when this state is false, it should check this
state again before actually changing the selected node.

It seems like inside _restoreSelectedNodeAfterUpdate is where we should be
checking this "is restoration allowed" state, so we never do a restoration when
we are not allowed.