All users were logged out of Bugzilla on October 13th, 2018

about:performance should be on pause by default

RESOLVED FIXED in Firefox 41

Status

()

RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: Yoric, Assigned: wrahman0)

Tracking

unspecified
mozilla41
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(1 attachment, 4 obsolete attachments)

At the moment, about:performance is pretty useless. One of the reasons is that it changes so fast that it's quite hard to copy&paste or use it to request assistance.

We should change its behavior as follows:
- by default, display a snapshot of an interval of ~1 second sampled while opening the page;
- a "play/pause" button lets users decide when they want a live view.
(Assignee)

Comment 1

4 years ago
What would this feature require? Can I have an estimation of the difficulty of this bug?
Flags: needinfo?(dteller)
Well, this would require changing the source code of https://dxr.mozilla.org/mozilla-central/source/toolkit/components/aboutperformance/content/aboutPerformance.js and https://dxr.mozilla.org/mozilla-central/source/toolkit/components/aboutperformance/content/aboutPerformance.xhtml.

We would need to add a button to the .xhtml, and make it control the `setInterval` time the .js.

All in all, this should be pretty easy for someone comfortable with setTimeout and setInterval.
Flags: needinfo?(dteller)
(Assignee)

Comment 3

4 years ago
Awesome, I would like to give this a shot. I will let you know about my progress.

Comment 4

4 years ago
David, you asked only for a "play/pause" button.

I think it would be even more useful if the tool would offer multiple update interval options, like Sysinternals Process Explorer does:

-------------
* 0.5 seconds
* 1 second
* 2 seconds
* 5 seconds
* 10 seconds
-------------
* Paused
-------------

Agree?
Flags: needinfo?(dteller)
Jason: Well, I was planning to add this later, but yes, that's a good idea.
Flags: needinfo?(dteller)
(Assignee)

Comment 6

4 years ago
@David: Just an update: I have been fiddling around with the play/pause button functionality. Its coming along quite well. I think this patch is a good place to implement Jason's interval suggestion as well. What do you think?
Flags: needinfo?(dteller)
(Assignee)

Comment 7

4 years ago
Created attachment 8592603 [details] [diff] [review]
bug1150863_add_play_pause_button.diff

This is a WIP. Just showing you my progress. Please let me know what you think.
Attachment #8592603 - Flags: review?(dteller)
(Assignee)

Comment 8

4 years ago
Also, can I get assigned to this bug?
Assignee: nobody → wrahman0
Flags: needinfo?(dteller)
If implemnenting Jason's interval suggestion is simple enough, feel free to do it in this patch.
Comment on attachment 8592603 [details] [diff] [review]
bug1150863_add_play_pause_button.diff

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

::: toolkit/components/aboutperformance/content/aboutPerformance.js
@@ +161,5 @@
> +  if (timerId != null){
> +    clearInterval(timerId);
> +    timerId = null;
> +  }
> +}

Could you group `timerId`, `startUpdate` and `stopUpdate` in a single object? Say `AutoUpdate`, with a field `_timerId` and methods `pause` and `unpause`?

@@ +167,5 @@
>  function go() {
>    // Compute initial state immediately, then wait a little
>    // before we start computing diffs and refreshing.
>    State.update();
> +  setTimeout(update(), 1000);

Not `update()`, `update`, otherwise you'll run the code immediately.

::: toolkit/components/aboutperformance/content/aboutPerformance.xhtml
@@ +87,5 @@
>    <body onload="go()">
>      <table id="data">
>      </table>
> +    <input type="button" onclick="startUpdate()" value="Play"></input>
> +    <input type="button" onclick="stopUpdate()" value="Pause"></input>

Generally, we prefer giving an `id` to the elements and calling `addEventListener` from JS code.

I know, three lines above, we're violating that rules :)
Attachment #8592603 - Flags: review?(dteller) → feedback+
(Assignee)

Comment 11

4 years ago
Yeah, those are great suggestions. I havent had the time to look at this bug again (finishing up a sprint at work). Hopefully I can implement the changes tonight.
(Assignee)

Comment 12

4 years ago
Created attachment 8594284 [details] [diff] [review]
bug1150863_add_play_pause_button.diff

I implemented the changes you mentioned. I still need to add the drop down menu to select the intervals and some comments to the code. Please let me know what you think.
Attachment #8592603 - Attachment is obsolete: true
Flags: needinfo?(dteller)
Attachment #8594284 - Flags: feedback?(dteller)
Probably not for this bug, but something I think would be useful is an 'update on jank level n' mode - so that the page only refreshes when something seriously janks the browser (letting you see what caused the latest instancce). Divining the desired jank level might be difficult for users though (unless there was some indication of the highest jank level seen to date). Tracking the 'maximum jank' that anything has caused, and only updating when something surpasses that maximum might be more useful, but then you'd need a way to clear it.
Emanuel, don't hesitate to file a bug.
Flags: needinfo?(dteller)
Comment on attachment 8594284 [details] [diff] [review]
bug1150863_add_play_pause_button.diff

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

This looks mostly good.
Can you apply the changes below?

::: toolkit/components/aboutperformance/content/aboutPerformance.js
@@ +21,5 @@
>    {key: "totalCPOWTime", percentOfDeltaT: true, label: "Cross-Process (%)"},
>    {key: "ticks", percentOfDeltaT: false, label: "Activations"},
>  ];
>  
> +let AutoUpdate = {

This object and all its fields will need a little documentation.

@@ +25,5 @@
> +let AutoUpdate = {
> +
> +  _timerId: null,
> +
> +  startUpdate: function(){

Now that it's in an object `AutoUpdate`, you can just call these methods `start` and `stop`.

@@ +26,5 @@
> +
> +  _timerId: null,
> +
> +  startUpdate: function(){
> +    if (AutoUpdate._timerId != null) return;

We always put braces in an `if`, to avoid accidents.

@@ +31,5 @@
> +    AutoUpdate._timerId = window.setInterval(update, 2000);
> +  },
> +
> +  stopUpdate: function(){
> +      if (AutoUpdate._timerId != null){

Generally, we rather write
  if (AutoUpdate._timerId == null) {
    return;
  }
  clearInterval(...)
  this._timerId = null;

@@ +34,5 @@
> +  stopUpdate: function(){
> +      if (AutoUpdate._timerId != null){
> +        clearInterval(AutoUpdate._timerId);
> +        AutoUpdate._timerId = null;
> +      }

By the way, we indent with two whitespaces.

@@ +36,5 @@
> +        clearInterval(AutoUpdate._timerId);
> +        AutoUpdate._timerId = null;
> +      }
> +  }
> +  

Nit: Please make sure that you do not add whitespace at the end of lines.

@@ +168,5 @@
>  
>  function go() {
>    // Compute initial state immediately, then wait a little
>    // before we start computing diffs and refreshing.
> +  document.getElementById('playButton').addEventListener("click", AutoUpdate.startUpdate);

To avoid accidents, make it `() => AutoUpdate.startUpdate()` and `() => AutoUpdate.stopUpdate()`.

::: toolkit/components/aboutperformance/content/aboutPerformance.xhtml
@@ +87,5 @@
>    <body onload="go()">
>      <table id="data">
>      </table>
> +    <input type="button" id="playButton" value="Play"></input>
> +    <input type="button" id="pauseButton" value="Pause"></input>

Can you put these buttons on the top? Otherwise, we won't see them if the list is too large.
Attachment #8594284 - Flags: feedback?(dteller) → feedback+
(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #14)
> Emanuel, don't hesitate to file a bug.

OK, I filed bug 1156266.
(Assignee)

Comment 17

4 years ago
Great suggestions David. I just didnt understand one of the points you mentioned. 

You mentioned: To avoid accidents, make it `() => AutoUpdate.startUpdate()` and `() => AutoUpdate.stopUpdate()`. I thought that: () => AutoUpdate.startUpdate() was equivalent to AutoUpdate.startUpdate(). How does wrapping it in an arrow function make it safer?
Flags: needinfo?(dteller)
There is actually a difference:

let MyObject = {
  foo: 5,
  bar: function() {
    console.log(this.foo);
  }
};

MyObject.bar(); // Displays 5, as intended

let a = MyObject.bar;
a(); // Displays undefined, because we have lost `this`.

let b = () => MyObject.bar();
b(); // Displays 5, as intended

The use of `() => AutoUpdate.startUpdate()` protects `this` in case we decide to use it `startUpdate`.
Flags: needinfo?(dteller)
(Assignee)

Comment 19

4 years ago
Created attachment 8598249 [details] [diff] [review]
bug1150863_add_play_pause_button.diff

Hey David, I implemented the changes that you suggested. Since you are away until May 6th, I have asked Avi to review the changes.
Attachment #8594284 - Attachment is obsolete: true
Flags: needinfo?(dteller)
Attachment #8598249 - Flags: review?(avihpit)
(Assignee)

Comment 20

4 years ago
Hey Avi, I hope you are ok with reviewing my changes. You were one of the suggested reviewers. 

As part of this bug, there is one more addition that is yet to be implemented. So if you do review it, don't land the changes just yet.
Flags: needinfo?(avihpit)
Comment on attachment 8598249 [details] [diff] [review]
bug1150863_add_play_pause_button.diff

I'd Prefer David to review it.
Flags: needinfo?(avihpit)
Attachment #8598249 - Flags: review?(avihpit)
(Assignee)

Comment 22

4 years ago
Yeah, David will be back in two days. Might as well wait until then. In the meantime, I guess I can commit the extra changes that I made so that he can do a complete review of the patch. 

Thanks Avi.
Flags: needinfo?(dteller)
(Assignee)

Comment 23

4 years ago
Created attachment 8601248 [details] [diff] [review]
bug1150863_add_play_pause_button.diff

Hey David, this patch contains the dropdown that lets users choose the refresh rate.
Attachment #8598249 - Attachment is obsolete: true
Attachment #8601248 - Flags: review?(dteller)
Comment on attachment 8601248 [details] [diff] [review]
bug1150863_add_play_pause_button.diff

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

Looks good to me, with a few trivial changes below.
Also, could you add "Bug 1150863 - " at the start of your commit message?

::: toolkit/components/aboutperformance/content/aboutPerformance.js
@@ +23,5 @@
>    {key: "ticks", percentOfDeltaT: false, label: "Activations"},
>  ];
>  
> +/**
> + *

Nit: Please remove that empty line.

@@ +24,5 @@
>  ];
>  
> +/**
> + *
> + * Used to control the updates in the performance page.

to control the *live* updates

@@ +39,5 @@
> +   */
> +  _intervalDropdown: null,
> +
> +  /**
> +   * Starts updating the performance data every 2 seconds if the updates are paused.

Well, not necessarily every 2 seconds, right?

@@ +48,5 @@
> +    }
> +
> +    if (AutoUpdate._timerId == null) {
> +      AutoUpdate._timerId = window.setInterval(update,
> +        AutoUpdate._intervalDropdown.options[AutoUpdate._intervalDropdown.selectedIndex].value);

Can you cut this in several shorter lines?
Attachment #8601248 - Flags: review?(dteller) → review+
(Assignee)

Comment 25

4 years ago
Created attachment 8601692 [details] [diff] [review]
bug1150863_add_play_pause_button.diff

Welcome back David! I made the changes that you suggested.

Now that this bug is nearing the end, I was looking for another one that I can learn from. I know that the about:performance page still has a lot of development left, so if you have anything in mind, let me know.
Attachment #8601248 - Attachment is obsolete: true
Flags: needinfo?(dteller)
Attachment #8601692 - Flags: review?(dteller)
By the way, have you checked whether it passes all tests? In particular,

* the following command makes sure that the tests use the latest version of your code:
./mach build toolkit/components/aboutperformance

* the following command launches the unit tests on about:performance
./mach mochitest toolkit/components/aboutperformance/tests/browser/browser_aboutperformance.js
Flags: needinfo?(wrahman0)
(Assignee)

Comment 27

4 years ago
Yeah, they pass with the following result:

TEST-PASS | leakcheck | default process: no leaks detected!
runtests.py | Running tests: end.
TEST-INFO | checking window state
Browser Chrome Test Summary
	Passed: 1
	Failed: 0
	Todo: 0
*** End BrowserChrome Test Results ***
SUITE-END | took 116s
Flags: needinfo?(wrahman0)
Would you be interested in working on Bug 1162053?
Comment on attachment 8601692 [details] [diff] [review]
bug1150863_add_play_pause_button.diff

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

Looks good to me, but the patch doesn't apply cleanly. Can you pull the latest version and rebase your patch?
Attachment #8601692 - Flags: review?(dteller) → feedback+
(Assignee)

Comment 30

4 years ago
Yeah, I realize that my working branch is sorta behind. I will pull and resolve the conflict tomorrow. 

Also, Bug 1162053 looks very interesting. I would love to work on it.
Flags: needinfo?(dteller)
(Assignee)

Comment 31

4 years ago
I pulled the latest changes and interestingly, I was able to merge my changes (though 'hg qpush') without any conflicts. The .diff file was also unchanged. 

Here are the steps I followed to pull the changes:

hg qpop
hg pull
hg update
hg qpush

./mach clobber
./mach build
./mach build toolkits/components/aboutperformance/

After which I was able to run the build and see the changes applied and working correctly.

Finally:

hg qrefresh

I ran a diff on the two patches and saw no differences.

I am unfamiliar with the merge process on your end but I can't think of any steps that I may have missed. I will keep looking into it but I want to hear what you think.
Flags: needinfo?(dteller)
Mmmh... You're right, your patch seems to apply nicely, I'm not sure what happened.
Here's the Try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f26a631fa145
Flags: needinfo?(dteller)
Attachment #8601692 - Flags: feedback+ → review+
All clear. Let's land this.
(Assignee)

Comment 34

4 years ago
Just curious, how did you end up fixing the issue with the try server?
Flags: needinfo?(dteller)
The issue was on my computer, not on Try.
Flags: needinfo?(dteller)
Oops, forgot to mark this checkin-needed.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6c93e975c4ea
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.