1496990 - Computed view is not initialized when it is the default tab

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P3)

Description

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

STR:
1. Select the computed view.
2. Close the toolbox
3. Reopen toolbox
4. See that the computed view is not initialized

The case of this is that this.onSidebarSelect is not called when the computed view is added since it was moved with the rest of the event handlers. The rules view was immune to this change because I manually called getPanel.

Comment 1

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

Attachment: 1496990.patch

Comment 2

[Patrick Brosset <:pbro>]

Comment on attachment 9015037 [details] [diff] [review]
1496990.patch

Review of attachment 9015037 [details] [diff] [review]:
-----------------------------------------------------------------

Could you please add a test to avoid regressing this again? I'm surprised that try did not catch this by the way. We do have computed-view tests right? How come they didn't fail?

Comment 3

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

Attachment: 1496990.patch [1.0]

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c2dd39bde2cc
Move the onSidebarSelect event handler before adding the sidebar tabs. r=pbro

Comment 5

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/c2dd39bde2cc

Comment 6

[Alexandre Poirot [:ochameau]]

This patch regressed significantly lots of DAMP tests.
You may ignore the regression on close tests, but the >20% regression on open should be addressed.
If you don't have any obvious fix I would suggest backing out this patch.

== Change summary for alert #16698 (as of Wed, 10 Oct 2018 14:55:28 GMT) ==

Regressions:

 54%  damp simple.inspector.close.DAMP windows7-32 opt e10s stylo               22.11 -> 33.97
 49%  damp simple.inspector.close.DAMP windows10-64-qr opt e10s stylo           22.97 -> 34.19
 46%  damp simple.inspector.close.DAMP windows10-64 opt e10s stylo              24.02 -> 35.18
 44%  damp simple.inspector.close.DAMP windows7-32 pgo e10s stylo               20.28 -> 29.13
 37%  damp simple.inspector.close.DAMP linux64 pgo e10s stylo                   24.28 -> 33.25
 24%  damp simple.inspector.close.DAMP linux64 opt e10s stylo                   30.88 -> 38.18
 22%  damp simple.inspector.open.DAMP windows10-64-qr opt e10s stylo            269.35 -> 329.68
 22%  damp simple.inspector.open.DAMP windows7-32 opt e10s stylo                268.41 -> 326.82
 20%  damp simple.inspector.open.DAMP windows10-64 opt e10s stylo               277.34 -> 334.08
 19%  damp complicated.inspector.open.DAMP windows7-32 pgo e10s stylo           363.43 -> 434.27
 19%  damp simple.inspector.open.DAMP linux64 opt e10s stylo                    282.00 -> 336.53
 19%  damp complicated.inspector.close.DAMP windows7-32 opt e10s stylo          50.96 -> 60.74
 19%  damp simple.inspector.open.DAMP windows7-32 pgo e10s stylo                251.98 -> 298.75
 18%  damp complicated.inspector.open.DAMP windows10-64 opt e10s stylo          399.89 -> 473.43
 18%  damp simple.inspector.open.DAMP linux64-qr opt e10s stylo                 304.95 -> 359.70
 18%  damp complicated.inspector.open.DAMP windows10-64-qr opt e10s stylo       402.96 -> 474.53
 17%  damp complicated.inspector.close.DAMP windows7-32 pgo e10s stylo          49.73 -> 58.23
 17%  damp simple.inspector.open.DAMP linux64 pgo e10s stylo                    254.48 -> 297.93
 17%  damp complicated.inspector.close.DAMP windows10-64-qr opt e10s stylo      52.64 -> 61.49
 17%  damp complicated.inspector.close.DAMP linux64-qr opt e10s stylo           58.27 -> 67.99
 17%  damp complicated.inspector.open.DAMP windows7-32 opt e10s stylo           397.62 -> 463.89
 16%  damp complicated.inspector.close.DAMP windows10-64 opt e10s stylo         57.04 -> 66.40
 16%  damp complicated.inspector.open.DAMP linux64 pgo e10s stylo               359.89 -> 418.87
 16%  damp complicated.inspector.open.DAMP linux64 opt e10s stylo               395.22 -> 459.94
 16%  damp complicated.inspector.close.DAMP linux64 opt e10s stylo              58.39 -> 67.58
 15%  damp complicated.inspector.close.DAMP linux64 pgo e10s stylo              55.65 -> 63.75
 14%  damp cold.inspector.open.DAMP windows7-32 opt e10s stylo                  505.01 -> 573.21
 13%  damp simple.inspector.close.DAMP linux64-qr opt e10s stylo                35.38 -> 39.85
 13%  damp cold.inspector.open.DAMP linux64-qr opt e10s stylo                   538.25 -> 606.18
 12%  damp cold.inspector.open.DAMP windows10-64-qr opt e10s stylo              519.44 -> 583.46
 12%  damp complicated.inspector.open.DAMP linux64-qr opt e10s stylo            465.61 -> 522.74
 12%  damp cold.inspector.open.DAMP windows7-32 pgo e10s stylo                  470.59 -> 526.37
 11%  damp cold.inspector.open.DAMP linux64 opt e10s stylo                      533.59 -> 593.71
 11%  damp cold.inspector.open.DAMP windows10-64 opt e10s stylo                 551.08 -> 609.28
 10%  damp simple.inspector.reload.DAMP windows10-64-qr opt e10s stylo          85.59 -> 93.87
  9%  damp simple.inspector.reload.DAMP windows7-32 pgo e10s stylo              78.81 -> 86.04
  9%  damp inspector.layout.open windows7-32 opt e10s stylo                     406.22 -> 442.97
  9%  damp custom.inspector.manyrules.selectnode linux64-qr opt e10s stylo      874.91 -> 952.46
  9%  damp simple.inspector.reload.DAMP windows10-64 opt e10s stylo             89.41 -> 97.31
  9%  damp simple.inspector.reload.DAMP windows7-32 opt e10s stylo              86.66 -> 94.21
  8%  damp simple.inspector.reload.DAMP linux64 pgo e10s stylo                  84.11 -> 91.16
  8%  damp cold.inspector.open.DAMP linux64 pgo e10s stylo                      489.41 -> 528.48
  8%  damp simple.inspector.reload.DAMP linux64-qr opt e10s stylo               103.18 -> 111.04
  8%  damp custom.inspector.manyrules.selectnode linux64 pgo e10s stylo         758.37 -> 815.50
  7%  damp custom.inspector.manyrules.selectnode windows7-32 opt e10s stylo     857.21 -> 921.31
  7%  damp simple.inspector.reload.DAMP linux64 opt e10s stylo                  94.58 -> 101.13
  7%  damp custom.inspector.close.DAMP linux64 opt e10s stylo                   88.64 -> 94.64
  6%  damp custom.inspector.open.DAMP windows10-64 opt e10s stylo               1,104.08 -> 1,167.11
  6%  damp custom.inspector.open.DAMP windows10-64-qr opt e10s stylo            1,100.63 -> 1,162.31
  5%  damp custom.inspector.manyrules.selectnode linux64 opt e10s stylo         849.23 -> 895.75
  5%  damp custom.inspector.open.DAMP windows7-32 opt e10s stylo                1,068.59 -> 1,123.35
  5%  damp custom.inspector.manyrules.selectnode windows7-32 pgo e10s stylo     796.56 -> 837.23
  5%  damp custom.inspector.close.DAMP windows10-64 opt e10s stylo              93.01 -> 97.61
  5%  damp custom.inspector.open.DAMP linux64 pgo e10s stylo                    890.13 -> 932.88
  5%  damp custom.inspector.open.DAMP linux64-qr opt e10s stylo                 1,073.10 -> 1,124.23
  5%  damp custom.inspector.open.DAMP linux64 opt e10s stylo                    1,049.22 -> 1,098.36
  5%  damp custom.inspector.close.DAMP windows10-64-qr opt e10s stylo           92.14 -> 96.36
  5%  damp custom.inspector.close.DAMP windows7-32 pgo e10s stylo               81.49 -> 85.21
  4%  damp custom.inspector.close.DAMP windows7-32 opt e10s stylo               88.54 -> 92.46
  4%  damp custom.inspector.open.DAMP windows7-32 pgo e10s stylo                959.62 -> 1,000.40
  4%  damp complicated.inspector.reload.DAMP linux64-qr opt e10s stylo          1,288.22 -> 1,336.95
  3%  damp windows7-32 opt e10s stylo                                           230.64 -> 236.48
  2%  damp windows10-64 opt e10s stylo                                          237.17 -> 242.09

Improvements:

  7%  damp console.typing linux64 pgo e10s stylo                                327.55 -> 303.83
  5%  damp custom.inspector.collapseall.manychildren linux64 pgo e10s stylo     1.87 -> 1.77
  4%  damp custom.inspector.expandall.manychildren linux64 pgo e10s stylo       154.99 -> 148.06

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=16698

Comment 7

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

I have confirmed that this bug is causing the regression. This is what the DAMP looks like reverting some of the changes. https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=cd051a68a9a9a36e3d1bd7f37b7d16512b9365be&newProject=try&newRevision=e3c944474bb5ad2c797355ac18e295bfea70a0bf&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&filter=inspector&framework=12

Comment 8

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

So, we can't back out this patch. Looking at the DAMP tests - what was happening before was that we weren't initializing the computed view and the performance improvements was because the computed view was not initializing because the sidebar select event was not on. So, that performance improvement was really an error.

Comment 9

[Alexandre Poirot [:ochameau]]

Is it a fix against a particular patch that landed recently?
I imagine it comes from one patch of bug 1494162, do you know which one?
An equivalent "improvement" has been landed in this pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=04baba568b9ac18b7202ba6abe2fffd23cd32f39&tochange=301f1711aab2fabca34a6c80d6bf4f42779b0c01

It would be great to update bugzilla to state that dependency.

Comment 10

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

Yes, this would be Part 29 of Bug 1494162.