DevTools: clean up ObjectPropertiesSection.createValueElement

DevTools: clean up ObjectPropertiesSection.createValueElement

BUG=405845
Review-Url: https://codereview.chromium.org/2606733003
Cr-Commit-Position: refs/heads/master@{#443155}
Committed: https://chromium.googlesource.com/chromium/src/+/30b506970f7903f7434dad86568fc90633dadce1

[luoe, 2016-12-27 22:30:13]

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

[commit-bot: I haz the power, 2016-12-27 22:30:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[luoe, 2016-12-27 22:37:12]

Description was changed from

==========
DevTools: clean up ObjectPropertiesSection

BUG=405845
==========

to

==========
DevTools: clean up ObjectPropertiesSection.createValueElement

BUG=405845
==========

[luoe, 2016-12-27 22:37:41]

luoe@chromium.org changed reviewers:
+ dgozman@chromium.org, lushnikov@chromium.org

[luoe, 2016-12-27 22:37:41]

Please take a look

[dgozman, 2016-12-28 00:17:44]

https://codereview.chromium.org/2606733003/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js
(right):

https://codereview.chromium.org/2606733003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js:244:
var description = value.description || '';
Let's not do || ''

https://codereview.chromium.org/2606733003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js:248:
var isNumberWithExponent = type === 'number' && description.indexOf('e') !== -1;
Let's structure it differently:

var valueElement = null;

if (...)
  valueElement = createInternalLocationElement(...);
else if (...)
  valueElement = createStringElement(...);
...


if (!wasThrown)
  return valueElement;

wrap with exception and return

https://codereview.chromium.org/2606733003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js:264:
valueElement.addEventListener('click', mouseClick, false);
Inline the listeners.

https://codereview.chromium.org/2606733003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js:279:
valueElement.title = description;
Let's move these into the cases.

https://codereview.chromium.org/2606733003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js:282:
if (wasThrown) {
Let's do at the start:
if (wasThrown) {
  valueElement.createTextChild('[Exception: ');
  valueElement = valueElement.createChild('span', 'value');
  valueElement.parentElement.createTextChild(']');
}

Let's add tests for 'throw function(){}', 'throw 123', 'throw "123"', 'throw new
Error("hi")'

[luoe, 2017-01-03 21:52:31]

Please take another look.

After this CL, I plan to create the object preview experiment.

https://codereview.chromium.org/2606733003/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js
(right):

https://codereview.chromium.org/2606733003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js:248:
var isNumberWithExponent = type === 'number' && description.indexOf('e') !== -1;
On 2016/12/28 00:17:44, dgozman wrote:
> Let's structure it differently:
> 
> var valueElement = null;
> 
> if (...)
>   valueElement = createInternalLocationElement(...);
> else if (...)
>   valueElement = createStringElement(...);
> ...
> 
> 
> if (!wasThrown)
>   return valueElement;
> 
> wrap with exception and return
> 

Done.

https://codereview.chromium.org/2606733003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js:264:
valueElement.addEventListener('click', mouseClick, false);
On 2016/12/28 00:17:44, dgozman wrote:
> Inline the listeners.

Done.

https://codereview.chromium.org/2606733003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js:279:
valueElement.title = description;
On 2016/12/28 00:17:44, dgozman wrote:
> Let's move these into the cases.

Done.

https://codereview.chromium.org/2606733003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js:282:
if (wasThrown) {
On 2016/12/28 00:17:44, dgozman wrote:
> Let's do at the start:
> if (wasThrown) {
>   valueElement.createTextChild('[Exception: ');
>   valueElement = valueElement.createChild('span', 'value');
>   valueElement.parentElement.createTextChild(']');
> }
> 
> Let's add tests for 'throw function(){}', 'throw 123', 'throw "123"', 'throw
new
> Error("hi")'

Done.

[dgozman, 2017-01-09 23:58:52]

lgtm

https://codereview.chromium.org/2606733003/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js
(right):

https://codereview.chromium.org/2606733003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js:254:
valueElement.title = description;
Let's set title inside of respective functions.

https://codereview.chromium.org/2606733003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js:268:
valueElement.title = description;
description || ''

https://codereview.chromium.org/2606733003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js:278:
valueElement.classList.add('value');
Don't you need this in case of wasThrown?

https://codereview.chromium.org/2606733003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js:312:
valueElement.addEventListener('mouseleave', SDK.DOMModel.hideDOMNodeHighlight,
false);
() => SDK.DOMModel.hideDOMNodeHighlight()

[luoe, 2017-01-10 22:05:09]

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

[commit-bot: I haz the power, 2017-01-10 22:05:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[luoe, 2017-01-10 22:22:11]

ac.  Thanks for the review

https://codereview.chromium.org/2606733003/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js
(right):

https://codereview.chromium.org/2606733003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js:254:
valueElement.title = description;
On 2017/01/09 23:58:52, dgozman wrote:
> Let's set title inside of respective functions.

Done.

https://codereview.chromium.org/2606733003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js:268:
valueElement.title = description;
On 2017/01/09 23:58:52, dgozman wrote:
> description || ''

Done.

https://codereview.chromium.org/2606733003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js:278:
valueElement.classList.add('value');
It's covered in line 272 on the wrapper.

https://codereview.chromium.org/2606733003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js:312:
valueElement.addEventListener('mouseleave', SDK.DOMModel.hideDOMNodeHighlight,
false);
On 2017/01/09 23:58:52, dgozman wrote:
> () => SDK.DOMModel.hideDOMNodeHighlight()

Done.

[commit-bot: I haz the power, 2017-01-10 23:41:58]

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

[commit-bot: I haz the power, 2017-01-10 23:41:59]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[luoe, 2017-01-11 19:49:12]

The CQ bit was checked by luoe@chromium.org

[luoe, 2017-01-11 19:49:13]

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/2606733003/#ps120001
(title: "fix test")

[commit-bot: I haz the power, 2017-01-11 19:50:07]

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-01-11 20:06:17]

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

[commit-bot: I haz the power, 2017-01-11 20:06:18]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[luoe, 2017-01-11 21:26:06]

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

[commit-bot: I haz the power, 2017-01-11 21:27:13]

Dry run: 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-01-11 23:09:15]

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

[commit-bot: I haz the power, 2017-01-11 23:09:18]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[luoe, 2017-01-12 02:12:58]

The CQ bit was checked by luoe@chromium.org

[luoe, 2017-01-12 02:12:59]

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/2606733003/#ps140001
(title: "actually fix test")

[commit-bot: I haz the power, 2017-01-12 02:13:39]

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-01-12 02:24:09]

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

[commit-bot: I haz the power, 2017-01-12 02:24:10]

Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)

[luoe, 2017-01-12 04:03:50]

The CQ bit was checked by luoe@chromium.org

[commit-bot: I haz the power, 2017-01-12 04:04:08]

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-01-12 04:50:23]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1484193830730670,
"parent_rev": "80fccc61857e261e52bed6c44a6d4f0ec97aff72", "commit_rev":
"30b506970f7903f7434dad86568fc90633dadce1"}

[commit-bot: I haz the power, 2017-01-12 04:51:30]

Description was changed from

==========
DevTools: clean up ObjectPropertiesSection.createValueElement

BUG=405845
==========

to

==========
DevTools: clean up ObjectPropertiesSection.createValueElement

BUG=405845

Review-Url: https://codereview.chromium.org/2606733003
Cr-Commit-Position: refs/heads/master@{#443155}
Committed:
https://chromium.googlesource.com/chromium/src/+/30b506970f7903f7434dad86568f...
==========

[commit-bot: I haz the power, 2017-01-12 04:51:31]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/30b506970f7903f7434dad86568f...