Let the user select the sorting column by clicking the column header in about:performance
Categories
(Toolkit :: Performance Monitoring, enhancement)
Tracking
()
People
(Reporter: florian, Assigned: fluks.github)
References
Details
Attachments
(1 file)
No description provided.
|
Reporter | |
Comment 1•6 years ago
|
||
It's late to have this in 64, and Bryan agreed this isn't required for MVP, so I have no intent to uplift a patch for this to beta.
The option to sort Names is also a solution to prevent this https://bugzilla.mozilla.org/show_bug.cgi?id=1502080
Most user know this behavior from their OS "Task Manager" and we should also offer this option to improve UX and easily find resource hungry tasks.
LOL, this bug is open since 6 month.
This is a standard behavior of such columns and an important part of UX to see the completeness of the new Task Manager.
I don't understand why sorting wasn't added already. Without sorting, about:performance isn't really useable:
- The rows are jumping all over the place.
- I can't sort tabs and add-ons by performance impact (high > low), which seems like an essential functionality to me.
Moreover, users expect columns to be sortable by clicking the headers.
So please fix this in a coming release. Can't be too hard, is it?
|
|
Reporter | |
Comment 5•6 years ago
|
||
(In reply to Ruben from comment #4)
- I can't sort tabs and add-ons by performance impact (high > low), which seems like an essential functionality to me.
This is already the default behavior. It's not obvious because we sort over the energy impact of the last few seconds (5s iirc) to reduce the jumping around perception.
Who had this great idea to auto-sort by the influence of energy instead of memory?
This task manager is a great idea, but totally useless at the moment (without sorting).
This is a solution for Android or possibly for mobile devices. Most users do not use many tabs / add-ons when using an accumulator device. I really want to know how much people use about:performance when powered by a battery. Each time it opens, 1% of the battery is dead. xD
But many people use a huge number of tabs / addons with limited memory. Since v57, Firefox is the most memory-intensive application I've ever used. (except Adobe Suite) (3 to 15 GB of memory)
Until the release of the Discard API - It was IMPOSSIBLE to run Firefox for more than 24 hours without restarting the browser to clear approximately 10GB of wasted memory. Since Discard API, I can use processCount -1 instead of 4. (100+ Tabs)
The ability to sort by columns makes this great feature useful and should be used for months !!
The possibility to first sort Tab / Addons and then memory / power consumption would be the crown. But lets stay at the important fixes first.
Someone complained in the Firefox subreddit about the lack of ability to sort the columns and I implemented a quick fix to make it possible.
https://old.reddit.com/r/firefox/comments/blew8s/so_i_wanna_talk_about_how_completely_useless_the/emqwadl/
https://pastebin.com/2cCrUiHq
If this is something that can be included into Firefox, I could work on this bug.
|
||
Updated•5 years ago
|
|
|
Reporter | |
Comment 8•5 years ago
|
||
(In reply to fluks from comment #7)
Someone complained in the Firefox subreddit about the lack of ability to sort the columns and I implemented a quick fix to make it possible.
https://old.reddit.com/r/firefox/comments/blew8s/so_i_wanna_talk_about_how_completely_useless_the/emqwadl/
https://pastebin.com/2cCrUiHqIf this is something that can be included into Firefox, I could work on this bug.
Thanks for looking at this! I can mentor you if you would like to work on this bug and fix it.
Looking at the patch in the pastebin, here are a few things to improve:
- to reduce code duplication, we would want to have a single click listener on the header row, and look at the target of the event.
- the sorting logic should be inside the _sortCounters method.
(In reply to Florian Quèze [:florian] from comment #8)
I made the improvements you told.
Sorting the type column is problematic, because you want to sort it by the translations and I'm not sure about the code where I get the translations, it's a bit sketchy, but I didn't know any other way.
And should the ghost-windows be on the bottom anymore, like this comment says?
// Force 'Recently Closed Tabs' to be always at the bottom, because it'll
// never be actionable.
Here's the current patch.
diff --git a/toolkit/components/aboutperformance/content/aboutPerformance.js b/toolkit/components/aboutperformance/content/aboutPerformance.js
index 510a2a2904f3..228222d5040f 100644
--- a/toolkit/components/aboutperformance/content/aboutPerformance.js
+++ b/toolkit/components/aboutperformance/content/aboutPerformance.js
@@ -584,6 +584,40 @@ var Control = {
this._updateDisplay(true);
}
});
+
+ this._sortOrder = "";
+ document.querySelector("#dispatch-thead").addEventListener("click", async (event) => {
+ const columnId = event.target.getAttribute("data-l10n-id");
+ if (columnId === "column-type")
+ this._sortOrder = this._sortOrder === "type_asc" ? "type_desc" : "type_asc";
+ else if (columnId === "column-energy-impact")
+ this._sortOrder = this._sortOrder === "cpu_desc" ? "cpu_asc" : "cpu_desc";
+ else if (columnId === "column-memory")
+ this._sortOrder = this._sortOrder === "memory_desc" ? "memory_asc" : "memory_desc";
+ else if (columnId === "column-name")
+ this._sortOrder = this._sortOrder === "name_asc" ? "name_desc" : "name_asc";
+
+ await this._updateDisplay(true);
+ });
+
+ (async () => {
+ const types = [
+ "tab",
+ "subframe",
+ "tracker",
+ "addon",
+ "browser",
+ "worker",
+ "other",
+ ];
+ this._typeTranslations = (await document.l10n.formatValues(types.map(type => {
+ return { id: "type-" + type };
+ }))).reduce((acc, value, i) => {
+ acc[types[i]] = value;
+ return acc;
+ }, {});
+ this._typeTranslations["system-addon"] = this._typeTranslations.addon;
+ })();
},
_lastMouseEvent: 0,
_updateLastMouseEvent() {
@@ -729,6 +763,46 @@ var Control = {
},
_sortCounters(counters) {
return counters.sort((a, b) => {
+ if (this._sortOrder) {
+ let res;
+ switch (this._sortOrder) {
+ case "memory_asc":
+ res = a.memory - b.memory;
+ break;
+ case "memory_desc":
+ res = b.memory - a.memory;
+ break;
+ case "type_asc":
+ res = String.prototype.localeCompare.call(this._typeTranslations[a.type],
+ this._typeTranslations[b.type]);
+ break;
+ case "type_desc":
+ res = String.prototype.localeCompare.call(this._typeTranslations[b.type],
+ this._typeTranslations[a.type]);
+ break;
+ case "name_asc":
+ res = String.prototype.localeCompare.call(a.name, b.name);
+ break;
+ case "name_desc":
+ res = String.prototype.localeCompare.call(b.name, a.name);
+ break;
+ case "cpu_asc":
+ res = this._computeEnergyImpact(a.dispatchesSincePrevious,
+ a.durationSincePrevious) -
+ this._computeEnergyImpact(b.dispatchesSincePrevious,
+ b.durationSincePrevious);
+ break;
+ case "cpu_desc":
+ res = this._computeEnergyImpact(b.dispatchesSincePrevious,
+ b.durationSincePrevious) -
+ this._computeEnergyImpact(a.dispatchesSincePrevious,
+ a.durationSincePrevious);
+ break;
+ default:
+ res = String.prototype.localeCompare.call(a.name, b.name);
+ }
+ return res;
+ }
// Force 'Recently Closed Tabs' to be always at the bottom, because it'll
// never be actionable.
if (a.name.id && a.name.id == "ghost-windows")
|
|
Reporter | |
Comment 10•5 years ago
|
||
(In reply to fluks from comment #9)
(In reply to Florian Quèze [:florian] from comment #8)
I made the improvements you told.
Thanks! So that I can review your patch, it'll need to be submitted through phabricator. There's some documentation at
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch
If you need help with the patch submission process, people in the #introduction IRC channel can likely help you quickly.
Sorting the type column is problematic, because you want to sort it by the translations and I'm not sure about the code where I get the translations, it's a bit sketchy, but I didn't know any other way.
And should the ghost-windows be on the bottom anymore, like this comment says?
ghost windows should remain at the bottom yes.
The idea I had in mind for the behavior when sorting by type was to group all the tabs of the same windows together, and display them in the order they are in the tab bar. The front-most window would be at the top.
This is complicated enough that I think it's fine to keep for a follow-up bug if you aren't interested in implementing this behavior.
I don't think you need to worry about sorting by translated types. Putting tabs before add-ons should be enough. And the sort order doesn't need to apply to the subitems (it's unlikely that there would be enough subitems for sorting to be needed there).
- document.querySelector("#dispatch-thead").
This should be a simple getElementById call.
addEventListener("click", async (event) => {
Please add this event listener next to the other ones in the Control.init method.
const columnId = event.target.getAttribute("data-l10n-id");if (columnId === "column-type")this._sortOrder = this._sortOrder === "type_asc" ? "type_desc" : "type_asc";else if (columnId === "column-energy-impact")this._sortOrder = this._sortOrder === "cpu_desc" ? "cpu_asc" : "cpu_desc";else if (columnId === "column-memory")this._sortOrder = this._sortOrder === "memory_desc" ? "memory_asc" : "memory_desc";else if (columnId === "column-name")this._sortOrder = this._sortOrder === "name_asc" ? "name_desc" : "name_asc";
nit: front-end Firefox code uses == rather than ===, unless === is specifically needed.
case "name_asc":res = String.prototype.localeCompare.call(a.name, b.name);
a.name.localeCompare(b.name) should be enough. Is there a reason why it didn't work for you?
|
Assignee | |
Comment 11•5 years ago
|
||
(In reply to Florian Quèze [:florian] from comment #10)
(In reply to fluks from comment #9)
The idea I had in mind for the behavior when sorting by type was to group all the tabs of the same windows together, and display them in the order they are in the tab bar. The front-most window would be at the top.
This is complicated enough that I think it's fine to keep for a follow-up bug if you aren't interested in implementing this behavior.
Yes, let's do that. Perhaps I can work later on the follow-up bug too.
I don't think you need to worry about sorting by translated types. Putting tabs before add-ons should be enough. And the sort order doesn't need to apply to the subitems (it's unlikely that there would be enough subitems for sorting to be needed there).
And leave all the other types at the bottom?
addEventListener("click", async (event) => {
Please add this event listener next to the other ones in the Control.init method.
There are three event listeners just above and one in beginning of init. Or do you mean not using an inline function?
case "name_asc":res = String.prototype.localeCompare.call(a.name, b.name);a.name.localeCompare(b.name) should be enough. Is there a reason why it didn't work for you?
I saw this comment at the end of _sortCounters and used the same code without too much thinking.
// a.name is sometimes an object, so we can't use a.name.localeCompare.
return String.prototype.localeCompare.call(a.name, b.name);
I will try to send the patch to phabricator soon.
|
|
Reporter | |
Comment 12•5 years ago
|
||
(In reply to fluks from comment #11)
(In reply to Florian Quèze [:florian] from comment #10)
I don't think you need to worry about sorting by translated types. Putting tabs before add-ons should be enough. And the sort order doesn't need to apply to the subitems (it's unlikely that there would be enough subitems for sorting to be needed there).
And leave all the other types at the bottom?
Yes, it's fine to have them all at the bottom. The other types only exist for Nightly users anyway.
addEventListener("click", async (event) => {
Please add this event listener next to the other ones in the Control.init method.
There are three event listeners just above and one in beginning of init. Or do you mean not using an inline function?
Sorry, I was confused by the few lines of context I had when looking at the previous patch; ignore this comment.
|
|
Assignee | |
Comment 13•5 years ago
|
||
|
||
Comment 14•5 years ago
|
||
Pushed by fqueze@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/07ce2dd7db9d Let the user sort by column in about:performance. r=florian
|
||
Comment 15•5 years ago
|
||
| bugherder | ||
|
|
Reporter | |
Comment 16•5 years ago
|
||
Congratulations on your first Firefox patch landing! It should be part of the next Firefox nightly. If you are interested in further improving about:performance, I'm happy to help you find other bugs to look at.
|
||
Updated•5 years ago
|
|
|
Assignee | |
Comment 17•5 years ago
|
||
(In reply to Florian Quèze [:florian] from comment #16)
Congratulations on your first Firefox patch landing! It should be part of the next Firefox nightly. If you are interested in further improving about:performance, I'm happy to help you find other bugs to look at.
Thank you and again, thank you for helping me.
Yes, I can fix other about:performance bugs too.
|
|
Reporter | |
Comment 18•5 years ago
•
|
||
(In reply to fluks from comment #17)
Yes, I can fix other about:performance bugs too.
Bug 1498193 or bug 1498188 might be interesting to you. Or if you want to continue on something related to sorting of columns, making it discoverable when hovering the headers could make sense as a next patch.
|
|
Assignee | |
Comment 19•5 years ago
|
||
(In reply to Florian Quèze [:florian] from comment #18)
Bug 1498193 or bug 1498188 might be interesting to you. Or if you want to continue on something related to sorting of columns, making it discoverable when hovering the headers could make sense as a next patch.
I can make sorting discoverable and do something else after that.
|
|
Assignee | |
Comment 20•5 years ago
|
||
Every change needs an issue first, right? Should I create new or will you? I can do it if that's ok.
|
|
Reporter | |
Comment 21•5 years ago
|
||
(In reply to fluks from comment #20)
Every change needs an issue first, right? Should I create new or will you? I can do it if that's ok.
Yes, every change needs a bug where we can discuss and reference it. Feel free to open new bugs for the changes you want to make (or even the changes you think should be made, even if you don't expect to make these changes yourself).
|
||
Updated•5 years ago
|
Description
•