1168125 - Cleanup performance xul and css

Status: RESOLVED FIXED

(DevTools :: Performance Tools (Profiler/Timeline), defect)

Description

[Victor Porof [:vporof][:vp]]

We have a lot of misplaced or dead code, and the xul file is terribly hard to follow now.

CONSOLIDATE.

THIS IS CAPTAIN PLANET.

Comment 1

[Victor Porof [:vporof][:vp]]

Attachment: v1

Groups things logically together, both in the XUL and CSS file. Adds a few more comments. Removed dead CSS code.

The collapsible markers patch is very close I promise and it's beautiful.

Comment 2

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8610144 [details] [diff] [review]
v1

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

If console recording messages are still fine, r+, but i think there'll be a problem with it

::: browser/devtools/performance/performance.xul
@@ +162,5 @@
>            </hbox>
>          </hbox>
> +
> +        <!-- "Console" notice, shown when a console recording is in progress -->
> +        <vbox id="console-recording-notice"

This should be a sibling of the other recording notices (in progress, loading, etc), no? How do we display the overview graphs at the same time with console recordings then?

Comment 3

[Victor Porof [:vporof][:vp]]

Whoops, fixing.

Comment 4

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/bfc1a9444c76

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/be8c492b2431

Comment 6

[Brian Grinstead [:bgrins]]

Attachment: panel-clipped.png

I'm seeing a regression with these patches applied in which the Performance panel is clipped off.  I haven't tracked it down further, but it goes away when I back them out locally

Comment 7

[Brian Grinstead [:bgrins]]

It seems to be caused by be8c492b2431

Comment 8

[Victor Porof [:vporof][:vp]]

Huuuuh.

Comment 9

[Victor Porof [:vporof][:vp]]

I don't see that.

Comment 10

[Brian Grinstead [:bgrins]]

(In reply to Victor Porof [:vporof][:vp] from comment #9)
> I don't see that.

Try running non-e10s - I can reproduce on a clean profile if that is switched off.

Comment 11

[Victor Porof [:vporof][:vp]]

http://www.reactiongifs.com/wp-content/uploads/2013/06/mad-at-theinternet.gif

Fixing.

Comment 12

[Victor Porof [:vporof][:vp]]

Attachment: e10somg.patch

It seems like this isn't needed anymore? Fixes things for me and the overview graphs are still resizable.

Comment 13

[Brian Grinstead [:bgrins]]

Comment on attachment 8611019 [details] [diff] [review]
e10somg.patch

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

Fixes the problem for me.  Somehow the label is now wrapping normally now (it wasn't before)

Comment 14

[Victor Porof [:vporof][:vp]]

http://www.reactiongifs.com/r/jck.gif

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/67e5dc593871

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/f24a70954568
https://hg.mozilla.org/integration/fx-team/rev/37227fbe07f7
https://hg.mozilla.org/integration/fx-team/rev/c3594996ddb6
https://hg.mozilla.org/integration/fx-team/rev/37d2acb3c1f1

Comment 17

[Jordan Santell [:jsantell] [@jsantell]]

Patches landed in comment #16 are for bug 1152421

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/bfc1a9444c76
https://hg.mozilla.org/mozilla-central/rev/be8c492b2431
https://hg.mozilla.org/mozilla-central/rev/67e5dc593871

Comment 19

[Ryan VanderMeulen [:RyanVM]]

browser_timeline-waterfall-sidebar.js has been hitting frequent e10s timeouts since this landed, and now it's basically impossible to backout due to everything that's landed on top of it. I'm disabling the test for now until it can be fixed.

4718 INFO TEST-UNEXPECTED-FAIL | browser/devtools/performance/test/browser_timeline-waterfall-sidebar.js | This test exceeded the timeout threshold. It should be rewritten or split up. If that's not possible, use requestLongerTimeout(N), but only as a last resort. - expected PASS

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f24a70954568
https://hg.mozilla.org/mozilla-central/rev/37227fbe07f7
https://hg.mozilla.org/mozilla-central/rev/c3594996ddb6
https://hg.mozilla.org/mozilla-central/rev/37d2acb3c1f1

Comment 21

[Andrei Vaida [:avaida]]

Setting [qe-verify-] since this is already covered by automated testing.