DevTools: refactor Common.Renderer for DOM nodes

DevTools: refactor Common.Renderer for DOM nodes

Preparation for:
https://codereview.chromium.org/2714833002/

BUG=687641
Review-Url: https://codereview.chromium.org/2728543002
Cr-Commit-Position: refs/heads/master@{#458841}
Committed: https://chromium.googlesource.com/chromium/src/+/6acb535d91ce5fc01546dc63b07b736e919accbb

[chenwilliam, 2017-03-01 01:59:08]

chenwilliam@chromium.org changed reviewers:
+ dgozman@chromium.org, pfeldman@chromium.org

[chenwilliam, 2017-03-01 01:59:08]

ptal

https://codereview.chromium.org/2728543002/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/elements/module.json (right):

https://codereview.chromium.org/2728543002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/elements/module.json:24:
"SDK.DeferredDOMNode"
Don't know why SDK.DeferredDOMNode wasn't included before

[chenwilliam, 2017-03-01 02:05:23]

Description was changed from

==========
DevTools: refactor Common.Renderer -> Common.NodeRenderer

Preparation for:
https://codereview.chromium.org/2714833002/

BUG=687641
==========

to

==========
DevTools: refactor Common.Renderer for DOM nodes

Preparation for:
https://codereview.chromium.org/2714833002/

BUG=687641
==========

[chenwilliam, 2017-03-01 02:12:00]

Sorry! I made a mistake in my earlier patchset. ptal.

[pfeldman, 2017-03-01 02:37:45]

https://codereview.chromium.org/2728543002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/audits/AuditExtensionCategory.js
(right):

https://codereview.chromium.org/2728543002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/audits/AuditExtensionCategory.js:218:
var domModel = SDK.DOMModel.fromTarget(remoteObject.target());
if (remoteObject.isNode()) {
   ...
} else {
   ... still render it as an object though.
}

you can try using async/await.

https://codereview.chromium.org/2728543002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js
(right):

https://codereview.chromium.org/2728543002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:616:
result.appendChild(this._formatParameterAsObject(remoteObject, false));
don't use arrow function here and above, leave them the way they were or inline
them?

https://codereview.chromium.org/2728543002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:624:
domModel.pushObjectAsNodeToFrontend(remoteObject, onNodeResolved);
isNode() helps!

[chenwilliam, 2017-03-02 00:06:55]

ptal

https://codereview.chromium.org/2728543002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/audits/AuditExtensionCategory.js
(right):

https://codereview.chromium.org/2728543002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/audits/AuditExtensionCategory.js:218:
var domModel = SDK.DOMModel.fromTarget(remoteObject.target());
On 2017/03/01 02:37:44, pfeldman wrote:
> if (remoteObject.isNode()) {
>    ...
> } else {
>    ... still render it as an object though.
> }
> 
> you can try using async/await.

Done. If we use async await in our frontend code, I think this means we'll need
to rethink our backwards compatibility test suite right?

https://codereview.chromium.org/2728543002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js
(right):

https://codereview.chromium.org/2728543002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:616:
result.appendChild(this._formatParameterAsObject(remoteObject, false));
On 2017/03/01 02:37:44, pfeldman wrote:
> don't use arrow function here and above, leave them the way they were or
inline
> them?

Done.

https://codereview.chromium.org/2728543002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:624:
domModel.pushObjectAsNodeToFrontend(remoteObject, onNodeResolved);
On 2017/03/01 02:37:44, pfeldman wrote:
> isNode() helps!

Done.

[pfeldman, 2017-03-02 22:04:51]

https://codereview.chromium.org/2728543002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/audits/AuditExtensionCategory.js
(right):

https://codereview.chromium.org/2728543002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/audits/AuditExtensionCategory.js:220:
domModel.pushObjectAsNodeToFrontend(remoteObject, onNodeResolved);
(If only this returned a promise, this whole thing could have been async and
nice, but oh well)

https://codereview.chromium.org/2728543002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/audits/AuditExtensionCategory.js:222:
var section = new ObjectUI.ObjectPropertiesSection(remoteObject);
Why aren't you using renderer here? I was hoping that you resolve object to a
node first and then lookup renderer. Same for the snippet below.

https://codereview.chromium.org/2728543002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/audits/AuditExtensionCategory.js:237:
remoteObject.release.call(remoteObject);
remoteObject.release();

https://codereview.chromium.org/2728543002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js
(right):

https://codereview.chromium.org/2728543002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:629:
result.appendChild(this._formatParameterAsObject(remoteObject, false));
Don't use silent catch, it is better to break than to swallow exception.

[chenwilliam, 2017-03-13 23:25:26]

ptal

https://codereview.chromium.org/2728543002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/audits/AuditExtensionCategory.js
(right):

https://codereview.chromium.org/2728543002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/audits/AuditExtensionCategory.js:222:
var section = new ObjectUI.ObjectPropertiesSection(remoteObject);
On 2017/03/02 22:04:51, pfeldman wrote:
> Why aren't you using renderer here? I was hoping that you resolve object to a
> node first and then lookup renderer. Same for the snippet below.

I'm implementing renderer for ObjectUI in the f/u patch
(https://codereview.chromium.org/2714833002/). Not sure I follow your comment -
I'm resolving the object to a DOM node and then looking up the renderer.

https://codereview.chromium.org/2728543002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/audits/AuditExtensionCategory.js:237:
remoteObject.release.call(remoteObject);
On 2017/03/02 22:04:51, pfeldman wrote:
> remoteObject.release();

Done.

https://codereview.chromium.org/2728543002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js
(right):

https://codereview.chromium.org/2728543002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:629:
result.appendChild(this._formatParameterAsObject(remoteObject, false));
On 2017/03/02 22:04:51, pfeldman wrote:
> Don't use silent catch, it is better to break than to swallow exception.

Done.

[dgozman, 2017-03-16 23:07:36]

https://codereview.chromium.org/2728543002/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/audits/AuditExtensionCategory.js
(right):

https://codereview.chromium.org/2728543002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/audits/AuditExtensionCategory.js:220:
domModel.pushObjectAsNodeToFrontend(remoteObject, onNodeResolved);
While you are here, let's promisify this.

https://codereview.chromium.org/2728543002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/audits/AuditExtensionCategory.js:221:
} else {
Let's remove the else branch. This code assumed it's a node.

https://codereview.chromium.org/2728543002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js
(right):

https://codereview.chromium.org/2728543002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:607:
if (remoteObject.isNode()) {
- let's change the callsite to check for remoteObject.isNode() itself

[chenwilliam, 2017-03-21 22:40:28]

ptal

https://codereview.chromium.org/2728543002/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/audits/AuditExtensionCategory.js
(right):

https://codereview.chromium.org/2728543002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/audits/AuditExtensionCategory.js:220:
domModel.pushObjectAsNodeToFrontend(remoteObject, onNodeResolved);
On 2017/03/16 23:07:36, dgozman wrote:
> While you are here, let's promisify this.

Done.

https://codereview.chromium.org/2728543002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/audits/AuditExtensionCategory.js:221:
} else {
On 2017/03/16 23:07:36, dgozman wrote:
> Let's remove the else branch. This code assumed it's a node.

Done.

https://codereview.chromium.org/2728543002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js
(right):

https://codereview.chromium.org/2728543002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:607:
if (remoteObject.isNode()) {
On 2017/03/16 23:07:36, dgozman wrote:
> - let's change the callsite to check for remoteObject.isNode() itself

Done.

[dgozman, 2017-03-21 23:00:49]

https://codereview.chromium.org/2728543002/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/devtools/front_end/audits/AuditExtensionCategory.js
(right):

https://codereview.chromium.org/2728543002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/audits/AuditExtensionCategory.js:222:
var node = await domModel.pushObjectAsNodeToFrontend(remoteObject);
if (!domModel) return;

https://codereview.chromium.org/2728543002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js
(right):

https://codereview.chromium.org/2728543002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:608:
domModel.pushObjectAsNodeToFrontend(remoteObject).then(onNodeResolved.bind(this));
if (!domModel) return result;

https://codereview.chromium.org/2728543002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:614:
function onNodeResolved(node) {
Make it an arrow function?

https://codereview.chromium.org/2728543002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:616:
result.appendChild(this._formatParameterAsObject(remoteObject, false));
Just return here.

https://codereview.chromium.org/2728543002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/sdk/DOMModel.js (right):

https://codereview.chromium.org/2728543002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sdk/DOMModel.js:1855:
this.pushNodeToFrontend(/** @type {string} */ (object.objectId), node =>
fulfill(node));
nit: can just pass fulfill as a last parameter.

[chenwilliam, 2017-03-22 00:30:39]

ptal

https://codereview.chromium.org/2728543002/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/devtools/front_end/audits/AuditExtensionCategory.js
(right):

https://codereview.chromium.org/2728543002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/audits/AuditExtensionCategory.js:222:
var node = await domModel.pushObjectAsNodeToFrontend(remoteObject);
On 2017/03/21 23:00:49, dgozman wrote:
> if (!domModel) return;

Done.

https://codereview.chromium.org/2728543002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js
(right):

https://codereview.chromium.org/2728543002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:608:
domModel.pushObjectAsNodeToFrontend(remoteObject).then(onNodeResolved.bind(this));
On 2017/03/21 23:00:49, dgozman wrote:
> if (!domModel) return result;

Done.

https://codereview.chromium.org/2728543002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:614:
function onNodeResolved(node) {
On 2017/03/21 23:00:49, dgozman wrote:
> Make it an arrow function?

Done.

https://codereview.chromium.org/2728543002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:616:
result.appendChild(this._formatParameterAsObject(remoteObject, false));
On 2017/03/21 23:00:49, dgozman wrote:
> Just return here.

Done.

https://codereview.chromium.org/2728543002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/sdk/DOMModel.js (right):

https://codereview.chromium.org/2728543002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sdk/DOMModel.js:1855:
this.pushNodeToFrontend(/** @type {string} */ (object.objectId), node =>
fulfill(node));
On 2017/03/21 23:00:49, dgozman wrote:
> nit: can just pass fulfill as a last parameter.

Done.

[dgozman, 2017-03-22 00:35:52]

https://codereview.chromium.org/2728543002/diff/140001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/devtools/front_end/audits/AuditExtensionCategory.js
(right):

https://codereview.chromium.org/2728543002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/audits/AuditExtensionCategory.js:229:
remoteObject.release();
We have to always release the object.

https://codereview.chromium.org/2728543002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/audits/AuditExtensionCategory.js:235:
parentElement.appendChild(element);
nit: inline it?

[chenwilliam, 2017-03-22 00:56:23]

Patchset #9 (id:160001) has been deleted

[chenwilliam, 2017-03-22 01:04:26]

ptal

https://codereview.chromium.org/2728543002/diff/140001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/devtools/front_end/audits/AuditExtensionCategory.js
(right):

https://codereview.chromium.org/2728543002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/audits/AuditExtensionCategory.js:229:
remoteObject.release();
On 2017/03/22 00:35:51, dgozman wrote:
> We have to always release the object.

Done.

https://codereview.chromium.org/2728543002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/audits/AuditExtensionCategory.js:235:
parentElement.appendChild(element);
On 2017/03/22 00:35:51, dgozman wrote:
> nit: inline it?

Done.

[dgozman, 2017-03-22 01:09:01]

lgtm

https://codereview.chromium.org/2728543002/diff/180001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/devtools/front_end/audits/AuditExtensionCategory.js
(right):

https://codereview.chromium.org/2728543002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/audits/AuditExtensionCategory.js:212:
this.evaluate(expression, evaluateOptions, onEvaluate);
this.evaluate(expression, evaluateOptions, async remoteObject => {
  await append(remoteObject);
  remoteObject.release();
});

[chenwilliam, 2017-03-22 01:14:07]

The CQ bit was checked by chenwilliam@chromium.org

[chenwilliam, 2017-03-22 01:14:08]

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/2728543002/#ps200001
(title: "fixup")

[commit-bot: I haz the power, 2017-03-22 01:15:09]

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-03-22 03:17:34]

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

[commit-bot: I haz the power, 2017-03-22 03:17:36]

Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_TIMED_OUT, build hasn't
started yet, builder probably lacks capacity)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_TIMED_OUT,
build hasn't started yet, builder probably lacks capacity)

[chenwilliam, 2017-03-22 19:22:16]

The CQ bit was checked by chenwilliam@chromium.org

[commit-bot: I haz the power, 2017-03-22 19:22:52]

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-03-22 19:35:06]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1490210536146340,
"parent_rev": "5de449fdaae6cb58ce02aed512f383857ef6bcfe", "commit_rev":
"6acb535d91ce5fc01546dc63b07b736e919accbb"}

[commit-bot: I haz the power, 2017-03-22 19:35:47]

Description was changed from

==========
DevTools: refactor Common.Renderer for DOM nodes

Preparation for:
https://codereview.chromium.org/2714833002/

BUG=687641
==========

to

==========
DevTools: refactor Common.Renderer for DOM nodes

Preparation for:
https://codereview.chromium.org/2714833002/

BUG=687641

Review-Url: https://codereview.chromium.org/2728543002
Cr-Commit-Position: refs/heads/master@{#458841}
Committed:
https://chromium.googlesource.com/chromium/src/+/6acb535d91ce5fc01546dc63b07b...
==========

[commit-bot: I haz the power, 2017-03-22 19:35:48]

Committed patchset #10 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/6acb535d91ce5fc01546dc63b07b...