796239 - [Task Manager] Should use transform instead of scrollLeft

Status: RESOLVED INVALID

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

Description

[GH to BZ]

[GitHub issue by timdream on 2012-09-05T21:22:06Z, https://github.com/mozilla-b2g/gaia/issues/4370]
Very bad performance.

Comment 1

[GH to BZ]

[GitHub comment by timdream on 2012-09-16T18:17:07Z]
Although scrolling generates bands on repaint area, the performance is not that bad. Closing as invalid.

Comment 2

[GH to BZ]

[GitHub comment by cgjones on 2012-09-17T18:46:54Z]
This still a valid issue that needs to be fixed.

Comment 3

[GH to BZ]

[GitHub comment by autonome on 2012-09-17T22:30:40Z]
/cc @michalbe

Comment 4

[GH to BZ]

[GitHub comment by timdream on 2012-09-18T05:24:05Z]
@cgjones Fair enough.

Comment 5

[Alex Keybl [:akeybl]]

We're trying to understand the criticality of this blocking-basecamp+ issue. What's the user impact here?

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

This is needed to reliably hit the perf target for this bit of UI.

Comment 7

[Michał Budzyński (:michalbe)]

Acording to what :vingtetun said, scrolling works way better than transforms eg. in Settings or Gallery. Do we really want to rebuild cardsview?

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Those are different use cases than here.

I reset to blocking? so that we can reevaluate priority based on field testing.

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

Blocking because of performance. We can re-prioritize or unblock when triaging later.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Panning in the Cards View looks OK here at the moment (on the Otoro). Can you give exact STR or some way to measure the performance against a concrete target?

Comment 11

[JP Rosevear [:jpr]]

Malina, what is the status of eideticker/b2g, could we measure the framerate on this?

Comment 12

[William Lachance (:wlach)]

(In reply to JP Rosevear [:jpr] from comment #11)
> Malina, what is the status of eideticker/b2g, could we measure the framerate
> on this?

[ I'll jump in here, since I'm the main contact for B2G/Eideticker now; mdas may have more information ]

We have eideticker "working", minus some flakiness and bugs in the support for handling gestures (which may be fixed by bug 801578).

The key for being able to measure something like this through eideticker would be being able to reproduce the test case in question. To that end, I guess my questions would be:

1. What is this card view?
2. Is there a way to get to the screen with this card view programatically? (e.g. through marionette)
3. What gesture/action do we need to produce in order to reproduce the performance issue? (if there is one)

Comment 13

[Andrew Overholt [:overholt]]

(In reply to William Lachance (:wlach) from comment #12)
> (In reply to JP Rosevear [:jpr] from comment #11)
> > Malina, what is the status of eideticker/b2g, could we measure the framerate
> > on this?
> 
> [ I'll jump in here, since I'm the main contact for B2G/Eideticker now; mdas
> may have more information ]
> 
> We have eideticker "working", minus some flakiness and bugs in the support
> for handling gestures (which may be fixed by bug 801578).
> 
> The key for being able to measure something like this through eideticker
> would be being able to reproduce the test case in question. To that end, I
> guess my questions would be:
> 
> 1. What is this card view?

A set of small screenshots of all running apps.  The screenshots can be horizontally scrolled and when clicked on, that app opens.

> 2. Is there a way to get to the screen with this card view programatically?
> (e.g. through marionette)

Long-press on home does it.  Not sure how that works in marionette.

> 3. What gesture/action do we need to produce in order to reproduce the
> performance issue? (if there is one)

If you turn on paint flashing [1], you can see that horizontal scrolling causes many flashes on the screenshots.

Jeff Muizelaar told me that the title of this bug is correct:  we should use a transform instead of scrollLeft.

[1]
Settings > Device Information > More Information > Developer > Flash repainted area

Comment 14

[Jonathan Griffin (:jgriffin)]

> > 2. Is there a way to get to the screen with this card view programatically?
> > (e.g. through marionette)
> 
> Long-press on home does it.  Not sure how that works in marionette.

It looks like the way to do this programatically is to dispatch a 'holdhome' event to the window containing the system app, see:

https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/cards_view.js#L571

I can help with the Marionette commands needed to do this.

Comment 15

[Jonathan Griffin (:jgriffin)]

(In reply to Jonathan Griffin (:jgriffin) from comment #14)
> > > 2. Is there a way to get to the screen with this card view programatically?
> > > (e.g. through marionette)
> > 
> > Long-press on home does it.  Not sure how that works in marionette.
> 
> It looks like the way to do this programatically is to dispatch a 'holdhome'
> event to the window containing the system app, see:
> 
> https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/cards_view.
> js#L571
> 
> I can help with the Marionette commands needed to do this.

This turns out to be quite easy.  Once you have started a marionette session, you can invoke the cards view using:

marionette.execute_script("window.dispatchEvent(new Event('holdhome'));")

If you've switched into an app frame (most of which run OOP), you'll first need to switch back to the main frame using:

marionette.switch_to_frame()

Comment 16

[JP Rosevear [:jpr]]

Thanks Jonathan - Will can we measure the frame rate here now?

Comment 17

[William Lachance (:wlach)]

(In reply to JP Rosevear [:jpr] from comment #16)
> Thanks Jonathan - Will can we measure the frame rate here now?

I'm having trouble reproducing the swipe gesture required to test this with the dummy input driver on the panda. I was trying to track down the underlying cause last week, and I think I may have finally figured it out:

https://bugzilla.mozilla.org/show_bug.cgi?id=783023#c19

Hopefully we can get this sorted out this week.

Comment 18

[Malini Das [:mdas] - Away, not checking bugmail]

Erg, my comment wasn't submitted X days ago. Removing f? since wlach is handling eideticker work.

Comment 19

[William Lachance (:wlach)]

(In reply to William Lachance (:wlach) from comment #17)
> (In reply to JP Rosevear [:jpr] from comment #16)
> > Thanks Jonathan - Will can we measure the frame rate here now?
> 
> I'm having trouble reproducing the swipe gesture required to test this with
> the dummy input driver on the panda. I was trying to track down the
> underlying cause last week, and I think I may have finally figured it out:
> 
> https://bugzilla.mozilla.org/show_bug.cgi?id=783023#c19
> 
> Hopefully we can get this sorted out this week.

Gestures are working now, but I need to add support for a new type of eideticker test to support what we're trying to do here. Working on that now.

Comment 20

[William Lachance (:wlach)]

(In reply to William Lachance (:wlach) from comment #19)
> (In reply to William Lachance (:wlach) from comment #17)
> > (In reply to JP Rosevear [:jpr] from comment #16)
> > > Thanks Jonathan - Will can we measure the frame rate here now?
> > 
> > I'm having trouble reproducing the swipe gesture required to test this with
> > the dummy input driver on the panda. I was trying to track down the
> > underlying cause last week, and I think I may have finally figured it out:
> > 
> > https://bugzilla.mozilla.org/show_bug.cgi?id=783023#c19
> > 
> > Hopefully we can get this sorted out this week.
> 
> Gestures are working now, but I need to add support for a new type of
> eideticker test to support what we're trying to do here. Working on that now.

Added bug 809730 to track this. No final patch yet, but I've gotten quite far with the idea. Should have something to show for it next week.

Comment 21

[Alex Keybl [:akeybl]]

Milestoning for C2 (deadline of 12/10), as this meets the criteria of "known P2 bugs found before or during C1".

Comment 22

[William Lachance (:wlach)]

(In reply to William Lachance (:wlach) from comment #20)
> > Gestures are working now, but I need to add support for a new type of
> > eideticker test to support what we're trying to do here. Working on that now.
> 
> Added bug 809730 to track this. No final patch yet, but I've gotten quite
> far with the idea. Should have something to show for it next week.

Actually, I guess the important thing here is really the test, not the thing that we need to write the test. Created a bug to track that, and removing 809730 from the list (it's done anyway).

Comment 23

[William Lachance (:wlach)]

No idea why my last comment reset all that stuff, I thought bugzilla was designed to prevent you from doing that. Anyway, unclobbering.

Comment 24

[Michał Budzyński (:michalbe)]

Attachment: patch

Comment 25

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Michal Budzynski (:michalbe) from comment #24)
> Created attachment 683989 [details]
> patch

This patch looks good to me. I read through it without testing it. I can give you a r+ in a moment.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #10)
> Panning in the Cards View looks OK here at the moment (on the Otoro). Can
> you give exact STR or some way to measure the performance against a concrete
> target?

The only problem is that this patch is a little bit too big. I afraid that the work might trigger some unforeseen regression.

Actually, I seriously doubt why we are blocking release on this bug -- cards view looks OK on device, and Josh said we really shouldn't drop common features and spend time on polishing advanced features like what we are doing right now.

I prefer this gets an blocking- and have someone a+ it for landing.

Comment 26

[David Scravaglieri [:scravag]]

Should we block on this as we don't know if we will keep cardview

Comment 27

[Josh Carpenter [:jcarpenter]]

Do not block. As per Tim, resources are better spent elsewhere right now.

Comment 28

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 683989 [details]
patch

Vivien, do you think this is safe to land?

Comment 29

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 683989 [details]
patch

If this is not a blocker or is not too noticeable I would said it does not make sense to take risk for it.

Comment 30

[Ben Francis [:benfrancis]]

As this is no longer blocking-basecamp should it be moved out of the C2 milestone?

Comment 31

[Julien Wajsberg [:julienw] (PTO -> Aug 14)]

should we land this now ?

(after a mandatory rebase of course :) )

Comment 32

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

That's what the master branch is for :).

Comment 33

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

This would also allow us to animate the transitions with CSS, which would get higher frame rate and avoid problems like bug 836664 comment 15.

Comment 34

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Task switcher UX has come a long way in the last week, but we still need this for buttery smooth operation.

Comment 35

[Julien Wajsberg [:julienw] (PTO -> Aug 14)]

Michal, or Cristian, is this obsolete with the new task switcher ? (I think so)

Comment 36

[Cristian Rodriguez (:crdlc)]

yes it is obsolete IMHO