New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 616897 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
not working at Google anymore
Closed: Jul 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Feature



Sign in to add a comment

Task Manager: support a sort order that shows process hierarchy

Project Member Reported by nick@chromium.org, Jun 2 2016

Issue description

In the old task manager, the model ordering was meaningful: tasks were generated in tab iteration order, and the browser process was always first.

In the new task manager, the browser process will still always be first, but the rest of the processes appear in an essentially random order in the TaskManagerTableModel.

Although the model ordering is the default display order of the task manager, once a different sort order is selected (by sorting one of the columns), there's no way to get back to the default ordering, even if you restart the browser; this happens because the new task manager persists the column sort as a preference.

My proposal is threefold:
 - First, we should make sorting by the "Task" row mean "sort by the model order" instead of alphabetically by title. This will fix problems like 54909 and 606985. It is important to note that ordering already not strictly alphabetical, since it preserves groups.
 - Second, we should greatly improve the default model ordering, by making it reflect process type and the process hierarchy -- specifically, we should list subframe/plugin rows under their parent tasks where possible.
 - Third, to replace the loss of usability of alphabetical sorting, we should the feature request of  bug 614551 , and select the last active tab when the task manager view is initially shown.
 

Comment 1 by creis@chromium.org, Jun 7 2016

Components: Internals>Sandbox>SiteIsolation
Project Member

Comment 2 by bugdroid1@chromium.org, Jun 22 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0088a36baa443168daf851f3ccf95a2ad7090802

commit 0088a36baa443168daf851f3ccf95a2ad7090802
Author: nick <nick@chromium.org>
Date: Wed Jun 22 17:25:56 2016

Make Task Manager sort more meaningful:
 - Model order is now more meaningful:
    - Browser process is always first.
    - Gpu process is always second.
    - More generally, sorting of non-subtasks is done by process
      type, followed by tab id, and finally task ID.
       - Using tab ID puts "Tab:" rows rows in tab creation order, and
         provides preserves the row's position in the list even when
         a tab navigates cross-process (though not across prerendering)
       - Sorting by process type places the browser process first,
         the gpu process second, etc, in a way that's predictable.
         (sorting by task ID or pid resulted in an unpredictable
         ordering).
    - Subtasks (i.e. subframes) appear right after their parents.
    - Using task_id as a tiebreaker within a group of related tasks
      gives a predictable ordering, since this is effectively
      task-creation order.
    - Groups are kept on consecutive rows, as before. Within a group,
      rows are ordered by the same sort comparator that is used
      to sort between groups.
 - To effect the above sorting changes, the following tweaks to
   the TaskManager private interfaces are needed:
    - Expose a GetParentTask() method on Task.
    - TaskGroup switches to using an unsorted vector of Tasks.
    - Expose a Task ctor that supports overriding the pid, for
      use in unittests.

This does not fix the UI problem where there's no way for
the user to get back to the model order after sorting by any
of the columns. A follow-on CL will need to address that.

BUG= 616897 , 54909 , 239611 , 606985 ,527455
TESTS=unit_tests, browser_tests

Review-Url: https://codereview.chromium.org/2028753002
Cr-Commit-Position: refs/heads/master@{#401328}

[modify] https://crrev.com/0088a36baa443168daf851f3ccf95a2ad7090802/chrome/browser/task_management/providers/task.cc
[modify] https://crrev.com/0088a36baa443168daf851f3ccf95a2ad7090802/chrome/browser/task_management/providers/task.h
[modify] https://crrev.com/0088a36baa443168daf851f3ccf95a2ad7090802/chrome/browser/task_management/providers/web_contents/subframe_task.cc
[modify] https://crrev.com/0088a36baa443168daf851f3ccf95a2ad7090802/chrome/browser/task_management/providers/web_contents/subframe_task.h
[modify] https://crrev.com/0088a36baa443168daf851f3ccf95a2ad7090802/chrome/browser/task_management/sampling/task_group.cc
[modify] https://crrev.com/0088a36baa443168daf851f3ccf95a2ad7090802/chrome/browser/task_management/sampling/task_group.h
[modify] https://crrev.com/0088a36baa443168daf851f3ccf95a2ad7090802/chrome/browser/task_management/sampling/task_manager_impl.cc
[add] https://crrev.com/0088a36baa443168daf851f3ccf95a2ad7090802/chrome/browser/task_management/sampling/task_manager_impl_unittest.cc
[modify] https://crrev.com/0088a36baa443168daf851f3ccf95a2ad7090802/chrome/browser/task_management/task_manager_browsertest.cc
[modify] https://crrev.com/0088a36baa443168daf851f3ccf95a2ad7090802/chrome/browser/task_management/task_manager_interface.h
[modify] https://crrev.com/0088a36baa443168daf851f3ccf95a2ad7090802/chrome/browser/task_management/task_manager_observer.h
[modify] https://crrev.com/0088a36baa443168daf851f3ccf95a2ad7090802/chrome/chrome_tests_unit.gypi

Comment 3 by nick@chromium.org, Jun 27 2016

Current plan here is to make the column click behavior be three-phase rather than toggle: "ascending/descending/no sort" instead of the current "ascending/descending".
Project Member

Comment 4 by bugdroid1@chromium.org, Jul 25 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/cb592d4b10cd8677aeac0694c5befd489b262a7a

commit cb592d4b10cd8677aeac0694c5befd489b262a7a
Author: nick <nick@chromium.org>
Date: Mon Jul 25 21:59:15 2016

TableView: Make ToggleSortOrder a three-phase operation that cycles through
(ascending -> descending -> no sort) if the column is initially ascending,
or (descending -> ascending -> no sort) if the column is initially
descending.

Expose a set_sort_descriptors() method so that TaskManagerTableModel can
apply its saved user preference directly. Previously that worked
by calling ToggleSortOrder the right number of times, which was weird,
since ToggleSortOrder is basically a click event handler.

Expand unittests to verify the implementation.

BUG= 616897 , 54909 

Review-Url: https://codereview.chromium.org/2146033003
Cr-Commit-Position: refs/heads/master@{#407607}

[modify] https://crrev.com/cb592d4b10cd8677aeac0694c5befd489b262a7a/chrome/browser/ui/task_manager/task_manager_table_model.cc
[modify] https://crrev.com/cb592d4b10cd8677aeac0694c5befd489b262a7a/chrome/browser/ui/task_manager/task_manager_table_model.h
[modify] https://crrev.com/cb592d4b10cd8677aeac0694c5befd489b262a7a/chrome/browser/ui/views/new_task_manager_view.cc
[modify] https://crrev.com/cb592d4b10cd8677aeac0694c5befd489b262a7a/chrome/browser/ui/views/new_task_manager_view.h
[modify] https://crrev.com/cb592d4b10cd8677aeac0694c5befd489b262a7a/ui/views/controls/table/table_view.cc
[modify] https://crrev.com/cb592d4b10cd8677aeac0694c5befd489b262a7a/ui/views/controls/table/table_view.h
[modify] https://crrev.com/cb592d4b10cd8677aeac0694c5befd489b262a7a/ui/views/controls/table/table_view_unittest.cc

Comment 5 by nick@chromium.org, Jul 25 2016

Status: Fixed (was: Started)

Sign in to add a comment