1206928 - Refactor Task Manager to improve maintainability and perceived performance.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::System::Task Manager, defect)

Comment 1

[GitHub Autolander Bot]

Attachment: [gaia] mcav:tm-perf > mozilla-b2g:master

Comment 2

[Marcus Cavanaugh [:mcav]]

Comment on attachment 8663927 [details] [review]
[gaia] mcav:tm-perf > mozilla-b2g:master

This patch refactors Task Manager to be smaller, smoother, and more
maintainable.

A brief video of this patch is here: https://youtu.be/Fkd7EQa0BxM

As I began working on Task Manager, I noticed that we've accumulated a lot of
technical debt, which would make the planned 2.5 (and beyond) changes difficult
and error-prone. This patch removes most of that debt and should reduce the risk
of problems going forward.

I've tried to refrain from doing too much in one patch, but in the process of
cleaning up the code, a couple enhancements naturally followed the work:

- The swipe-up-to-close gesture now feels smoother and natural.

  * Rather than using a fixed swipe-up threshold, we now use velocity to decide
    whether or not the user intended to kill the app. This better reflects
    the intent of the user.

  * The logic for swiping up has been extracted into a separate class, improving
    testability. This leaves the `Card` class largely inert.

  * Cards behave with proper easing, falling naturally back into place.

  * We have axis-lock, allowing scrolling side-to-side or swipe-up, but not
    both at the same time.

  * I did not attempt to use native vertical scrolling. While native scrolling
    may provide better FPS, I think a majority of the "this feels janky" feeling
    was due to logic/easing problems rather than actual scroll FPS. In my
    testing, swiping while following the finger seems very reasonable on Aries
    and Flame, perhaps because Task Manager is fully opaque.

- As discussed in a previous meeting, the legacy tarako
  `app.cards_view.screenshots.enabled` preference has been removed, as it
  introduced substantial complexity and does not appear to have been used or
  necessary on any of the devices we support today.

- CardsHelper has been renamed to TaskManagerUtils to better reflect its role;
  it now contains additional standalone functions for ease of testing.

- Our interaction with StackManager is now much more straightforward,
  reducing the need to maintain error-prone integer-based indexes.

- Filtering ("browser-only") is now only a few lines of code.

The rest of the changes are straightforward: cleaning up logic flows, reducing
complexity, and updating the code to modern standards. Similarly, the unit tests
have also been combed over to ensure they remain maintainable as well.

The diff may be hard to follow due to the nature of the refactoring; for ease of
review, I would suggest viewing `task_manager.js`, `card.js`, and
`task_manager_utils.js` independently, followed by a scan of the diff to see if
anything stands out. I've tried to structure the code in such a way that it can
be easily understood from top-to-bottom.

I have a patch in progress for adding the "new sheet" buttons, but I would like
to base that on top of this work.

Comment 3

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8663927 [details] [review]
[gaia] mcav:tm-perf > mozilla-b2g:master

I think it makes sense to remove the app.cards_view.screenshots.enabled toggle in a different bug. You'll also need to remove the developer menu settings entry at https://github.com/mozilla-b2g/gaia/blob/master/apps/settings/elements/developer.html#L105:L109

Leaving f? while I go through the rest.

Comment 4

[Marcus Cavanaugh [:mcav]]

Sure, I can pull that back in.

Comment 5

[Marcus Cavanaugh [:mcav]]

I've added back in the "app.cards_view.screenshots.enabled" setting in a second commit, with appropriate unit tests.

Comment 6

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8663927 [details] [review]
[gaia] mcav:tm-perf > mozilla-b2g:master

This is really nice work. But the scope of the changes give me pause - it seems to me whenever I've combined refactoring with adding or improving function, it comes back bite me. As one patch we've got the utils refactoring (which looks awesome btw), some stylistic changes to various methods, the SwipeToKill and axis-locking as well as significant unit test changes. I would rather see the unit test changes broken out so that they pass as-is (or with minimal changes) with the refactoring. Then one or more follow-ups to layer in those other changes. Some of the more code-cosmetic stuff can be done either piece-meal as you touch each section, or in a further follow-up.
 
So, I do like the end result, I just want to see this landed incrementally so we can trace cause and effect better in the commit log, and we're not stuck in an all-or-nothing position and at risk of backing the whole thing out by 2.5

Comment 7

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8663927 [details] [review]
[gaia] mcav:tm-perf > mozilla-b2g:master

Should be f+, just comments on delivery. If we can pair up patches with existing bugs with STR so much the better.

Comment 8

[Marcus Cavanaugh [:mcav]]

I understand your concern landing nontrivial patches, and I'd like to come to a
compromise to achieve our shared desire of landing patches that make sense.

A couple thoughts:

------------------------
On splitting this patch:

Most of the refactoring seen here is only possible by decoupling TaskManager's
relationship with Card's mechanics (position, lifecycle, attributes), logic
changes which together make up 90% of the changes you see here.

This leaves only the SwipeToKillMotion extraction (which itself provides
axis-lock). I don't think it's feasible to split anything else into its own
patch; the time investment dramatically outweighs the benefit beyond that.

The unit test changes are a necessary part of refactoring. Liberal refactoring
requires liberal unit test changes, and I think that liberal refactoring is
needed here to provide a stable foundation going forward. Worth noting:
the integration tests remain almost exactly the same as before.

------------------------
On risk:

I'm operating under the following strongly-held opinion: 

We should not attempt to land new TaskManager 2.5-desired features on top of the
existing code base. (The current code is hard to follow and full of debt, risk,
and complications.)

Therefore, if the refactoring must be backed out, so must the new features.

The new Task Manager features are not committed for the 2.5 release. We are
weeks away from 2.5 FL, and over a month before release availability. This patch
is not complicated, even though the diff appears intimidating. Even if this
patch introduced regressions, we would have at least a month to discover and
patch them before reverting to the old task manager.

------------------------

As mentioned above, the only portion I think could be separated is the
swipe-to-kill motion. I can do so tomorrow. Without that, I see no reason to not
consider the rest to be a reasonable changeset, i.e.:

- This patch: Refactoring
- bug 1066761: swipe-to-kill

Does that sound like a fair compromise?

Comment 9

[Sam Foster [:sfoster] (he/him)]

> As mentioned above, the only portion I think could be separated is the
> swipe-to-kill motion. I can do so tomorrow. Without that, I see no reason to
> not
> consider the rest to be a reasonable changeset, i.e.:
> 
> - This patch: Refactoring
> - bug 1066761: swipe-to-kill
> 
> Does that sound like a fair compromise?

So where I'm getting most stuck is the changes to task_manager_test.js; some of the changes in there I understand as being tests and code that has been refactored out and no-longer relevant. But others seem gratuitous and possibly broken. Its hard to tell. When you say the time investment to re-do this outweighs the benefit, I suspect you are only thinking of your own time, not that of your reviewers, QA and people attempting to track down problems in the future. If you make a bunch of changes *and* also change most of the unit tests, its not refactoring, its just rewriting stuff. The unit tests' prime reason-to-be is to insulate us from inadvertent breakage and the extent of the changes in there removes any such continuity and guarantee. 

Case in point: bug 1176895. The 'center apps' suite that covers the fix to this bug is gone in this patch and I'm not sure why. The fact that the diff makes this hard to deduce is a bad smell to me and suggests the patch needs splitting. 

These are my concerns. I do want to see this land, but lets get a 2nd opinion from Etienne on the how. I really don't mind being molified if the consensus is that I'm being overly cautious here.

Comment 10

[Marcus Cavanaugh [:mcav]]

(In reply to Sam Foster [:sfoster] from comment #9)

> When you say the time investment to re-do
> this outweighs the benefit, I suspect you are only thinking of your own
> time, not that of your reviewers, QA and people attempting to track down
> problems in the future.

No, I'm thinking about everyone's time. Each patch requires new overhead,
new context switches, and time investment for all involved.

> Case in point: bug 1176895. The 'center apps' suite that covers the fix to
> this bug is gone in this patch and I'm not sure why. The fact that the diff
> makes this hard to deduce is a bad smell to me and suggests the patch needs
> splitting.

I made some judgment calls when going over the tests; I removed some cases
that could be consolidated, edited others, and added new ones. With the 
'center apps' suite specifically, I felt that the coverage in [1] sufficiently
addressed this concern, because TaskManager's currentCard property is derived
from the current scroll position. In general, I avoid removing tests unless I 
feel that they are either redundant or no longer useful. Unit tests, specifically,
are filled with implementation details that are bound to change.

If there are specific areas in which you find the updated tests deficient, I'm
happy to address those cases; perhaps 'center apps' is a candidate for
more expansion. That's part of the point of the review process -- to find things
that I've missed. But I think most of the test changes are reasonable and retain
coverage that is just as useful as those before.

I'd rather see suggestions such as "let's re-add coverage for this explicitly"
in the cases where you think something's missing, rather than shying away from
improvements just because it "smells bad".

I'm looking for "hey, here's five places where I think we need to add coverage
similar to what we had before", that's fine -- that's actionable and that's reasonable.
That doesn't take a lot of time from the reviewer. (Nor would having the reviewer
review several patches save any time.)

In general, I think you're being overly cautious here. I think the proposed patch
here is usable and beneficial, and that the coverage is very close to what we had
before. But let's get Etienne's opinion and go from there.

[1] https://github.com/mozilla-b2g/gaia/pull/31897/files#diff-8ead3c1b475ca6bf0301670783024248R338

Comment 11

[Marcus Cavanaugh [:mcav]]

The latest commit [1] reintroduces several tests:

- The entire "center apps" suite.
- The "placement" test -- noting that the "placement" test is broken on master, because it attempts to compare `idx` to a DOM node rather than domNode.length [2]
- The "observes settings" suite.
- "no touch input handled while opening selected app".

Note that the "orientation" suite has been superseded by the "waitForScreenToBeReady" tests in [3].


[1] https://github.com/mcav/gaia/commit/87d45966f1cf095e4f816c62957690cd507cac2c
[2] https://github.com/mozilla-b2g/gaia/blob/master/apps/system/test/unit/task_manager_test.js#L557
[3] https://github.com/mozilla-b2g/gaia/pull/31897/files#diff-711c0175e8ed7454599f80680029d047R90

Comment 12

[Marcus Cavanaugh [:mcav]]

To put the open question more concisely (for the benefit of Etienne), I see a few options for how to move forward. If the primary concern here is overzealous test changes, I could:

A.  Nuke the current unit test changes, and then re-port task_manager_test.js with the goal of minimizing changes and making the diff easy to follow. Leave the non-test changes as-is.

B.  Manually compare the new test cases to the old, explicitly noting where each test went and why.

If, instead, this changeset is too large, I could:

C.  Remove the SwipeToKill and Card changes from this patch, leaving only TaskManager/Card refactoring.

D.  Abandon the refactor for now.  Pursue incremental refactoring in smaller bugs if/when time allows; focus instead on implementing new features on the existing codebase.

Any of those options are reasonable decisions, and I defer to you both to decide which option we should take.

Thanks for your time spent discussing this here; it's helpful to have this conversation so that I can provide patches better suited to your workflows in the future.

Comment 13

[Etienne Segonzac (:etienne)]

Comment on attachment 8663927 [details] [review]
[gaia] mcav:tm-perf > mozilla-b2g:master

Alright, I spent a lot of quality time with this patch _before_ reading the comments thread, but I guess it works out ok since the goal was to give a balanced third opinion :)

First, the code change: I want it.
The promise based show/hide, the utils extraction, everything.
I'd feel much more confident entering the stabilisation phase with this landed.

I must confess some bias: I work from a moz space with a lot of foxfooders and I get complaints weekly about the current task manager performance. So better performance is 2.5 must have as far as I'm concerned :)
This patch both gives us a nice perceived performance boost and will make smaller tweaks easier in the future (have I mentioned I love the show/hide promise chain? :))

That said, I did make a _lot_ of comments on the PR. And they're all test related. So I was clearly sharing Sam's concerns before even reading about them :)

* * *

My vote is for option B, and I think I already did a fair share of the "test coverage audit".

With all the comments addressed (a bit time consuming but there's nothing complex) I believe we'll be in really good shape coverage-wise, maybe even slightly higher than before (some of the comments go beyond what we have on master) :)

Gr-eat work! And as always, I'm happy to help out with bugfix/polish going forward.

Comment 14

[Marcus Cavanaugh [:mcav]]

Comment on attachment 8663927 [details] [review]
[gaia] mcav:tm-perf > mozilla-b2g:master

I've updated the patch with the test additions and changes per your comments. The only other notable change is a minor tweak in Card.js to work around a Gecko flame-specific rendering bug 1209194, as noted in comments.

I've tested on both Flame and Aries, and have not found any regressions.

This patch also fixes screen-reader support (bug 1208490) for swiping back and forth between cards, which had regressed on master some time ago. Unit tests now more thoroughly verify this functionality.

Comment 15

[Etienne Segonzac (:etienne)]

Comment on attachment 8663927 [details] [review]
[gaia] mcav:tm-perf > mozilla-b2g:master

Made a few comments on github, adding a |transition:none| for the initial placement of the cards is the only mandatory one.

Big week :)

Comment 16

[Marcus Cavanaugh [:mcav]]

- I addressed your "transition: none" comment by moving the default transition under "#cards-view.active" to ensure it only animates when the TM is active (and this happens after we've set the initial layout).

- I've added a followup for eventSafety here: https://bugzilla.mozilla.org/show_bug.cgi?id=1209819

- I left in the try/catch blocks in task_manager.js, but as noted in my GH comment, I see that they're not necessary since mocha is smart enough to catch the errors appropriately anyway. It's nice to fail faster but I didn't have that thought in mind at the time.

master: https://github.com/mozilla-b2g/gaia/commit/87350343dc71e2ff460f9e3b6046c98a75bfb97c