Closed Bug 932388 Opened 9 years ago Closed 9 years ago

[1.3] Music App launch time improvements

Categories

(Firefox OS Graveyard :: Gaia::Music, defect, P1)

ARM
Gonk (Firefox OS)
defect

Tracking

(Not tracked)

RESOLVED FIXED
1.3 Sprint 4 - 11/8

People

(Reporter: huseby, Assigned: huseby)

Details

(Keywords: perf, Whiteboard: [c=progress p=4 s= u=1.3] )

Attachments

(1 file)

Music app launch time improvements tracking bug.
Can you please give me more detail on your test setup?  I need to know what music data files you had stored on the device and if it was on the internal flash or an SD card.  If I can replicate you test setup, then I'll be able to replicate your results better.

Thank!
Status: NEW → ASSIGNED
Flags: needinfo?(mvikram)
This is the same patch, just rebased onto the latest master.  It moves the bluetooth comms init until after the musicdb scan is finished.  That allows the UI to be shown about 100ms faster.
Attachment #826263 - Flags: review?(dkuo)
Flags: needinfo?(mvikram)
Attachment #826263 - Flags: review?(dflanagan)
Comment on attachment 826263 [details] [review]
This shaves 100ms off of the Music app startup time.

I'd prefer not to defer the bluetooth init until after the scan because when there is lots of new music, the scan can take a minute or more. And we want the user to be able to play existing music normally while the scan is in progress.

But we should certainly delay the bluetooth init until the first screen is displayed.

Please try moving the MusicComms.init() call up a few lines so it is inside the callback function passed to showCurrentView().  In theory that is the right spot for it, so if it works there, that would be better.

In fact, you might move the call to scan() into that callback as well, so that we don't start scanning until the first screen is displayed.
Attachment #826263 - Flags: review?(dflanagan) → review-
Dave, sorry about not getting the review for you, I didn't noticed you moved the bug to here.

I agree with David because of the scanend takes some time if users got lots of files, and David's suggestion also make sense to me so probably you can test it first then I can do the review after you have some results, thanks.
Attachment #826263 - Flags: review?(dkuo)
Comment on attachment 826263 [details] [review]
This shaves 100ms off of the Music app startup time.

I made the changes suggested in the review.
Attachment #826263 - Flags: review- → review?(dflanagan)
Comment on attachment 826263 [details] [review]
This shaves 100ms off of the Music app startup time.

Transferring the review to Dominic
Attachment #826263 - Flags: review?(dflanagan) → review?(dkuo)
sent email to dkuo to get a time he can review this.
Comment on attachment 826263 [details] [review]
This shaves 100ms off of the Music app startup time.

Dave,

The patch looks good and I also tested it to make sure it doesn't break to remote controls, and since travis failed on your PR so I will cherry-pick
your commit and send another PR to get travis green, then merge for you.
Attachment #826263 - Flags: review?(dkuo) → review+
Landed on master: 29892f86e3531081fe6e507f2a25e79e301dddd5

Thanks Dave!
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.