1556255 - Convert the ElementEditor markup into a React component

Status: NEW

(DevTools :: Inspector, task, P3)

Comment 1

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Bug 1556255 - Part 1: Convert the ElementEditor markup into a React component. r=jdescottes

Comment 2

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Bug 1556255 - Part 2: Fix tests so it will pass with the new ElementNode component. r=jdescottes

Comment 3

[Gabriel Luong [:gl] (ΦωΦ)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=26482670915c533f4a9f00e67d2a849bd160e467

Comment 4

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Bug 1556255 - Part 3: Avoid calling setState for the ElementNode immediately after render. r=jdescottes

Comment 5

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Bug 1556255 - Part 4: Stop calling React.unmountComponentAtNode on destroy for the ElementEditor and TextEditor. r=jdescottes

This fixes inspector.close performance regressions.

Comment 6

[Gabriel Luong [:gl] (ΦωΦ)]

This is the latest damp results with Part 3 and 4. https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&newProject=try&newRevision=3866fa42102047f57c72b1657274f9d8aefc4976&originalSignature=1759151&newSignature=1759151&framework=12&originalRevision=56bfa95adf27e77089d7bfa5dab023246d71931a

I am mostly seeing regression on custom tests so I am wondering what those tests are doing. Everything else seems to be within reasonable perf. Part 4 fixes the close time performance regressions by not calling React.unmountComponentAtNode on destroy.

Comment 7

[Gabriel Luong [:gl] (ΦωΦ)]

try test results https://treeherder.mozilla.org/#/jobs?repo=try&revision=3866fa42102047f57c72b1657274f9d8aefc4976

Comment 8

[Julian Descottes [:jdescottes]]

(In reply to Gabriel [:gl] (ΦωΦ) from comment #6)

This is the latest damp results with Part 3 and 4. https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&newProject=try&newRevision=3866fa42102047f57c72b1657274f9d8aefc4976&originalSignature=1759151&newSignature=1759151&framework=12&originalRevision=56bfa95adf27e77089d7bfa5dab023246d71931a

I am mostly seeing regression on custom tests so I am wondering what those tests are doing.

They try to open the inspector on a page identical to this one https://amber-mask.glitch.me/

A lot of attributes on a few elements. A lot of elements overall, but most are not rendered.
Actually not a huge markup view to display before you start expanding things. I guess the patch is stressing the attribute creation codepath so this test is heavily impacted.

Comment 9

[Gabriel Luong [:gl] (ΦωΦ)]

Nicholas assisted me on profiling the attribute performance issues and will be spinning up some new bugs to address some performance issues overall. We found a couple of things to address:

• parseAttribute performance
• scrollIntoViewIfNeeded performance
• Lazify the rendering of the attributes by only rendering X number of attributes when the NodeAttribute is initially mounted and then render the rest on the next tick. This will mean some shifting.

Since we are going to regress anyways, we are looking at other things we can shave.

Comment 10

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Bug 1556255 - Part 4: Only render 10 attributes on initial render and render the rest on the next tick. r=nchevobbe

Comment 11

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Bug 1556255 - Part 5: Sort the attributes in the ElementEditor. r=nchevobbe

Comment 12

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Bug 1556255 - Part 6: Don't truncate the string unless the value's length is greater than COLLAPSE_DATA_URL_LENGTH. r=nchevobbe

Comment 13

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Bug 1556255 - Part 7: Remove the ref to the attribute list. r=nchevobbe