968035 - browser-syncui.js should simply sync init code by using .whenLoaded() promise

Status: RESOLVED INVALID

(Firefox :: Sync, defect, P3)

Description

[Mark Hammond [:markh] [:mhammond]]

Bug 967313 introduced a .whenLoaded() promise-based function to simplify code that needs to initialize sync.  browser-syncui.js should use this as it would simplify things quite a bit

Comment 1

[Chris Karlof [:ckarlof]]

Markh, good first bug?

Comment 2

[Harshal Lele]

I would like to work on this bug, but it's my first bug.Can you help me out?

Comment 3

[wilson.sun]

Hi, I am a student from UofT, and I choose this bug for my good first bug assignment. I need someone to help me get started, much appreciated!

Comment 4

[Mark Hammond [:markh] [:mhammond]]

The work here is to remove the "weave:service:ready" observer and the code handling the notification, and replace it with something like:

this.weaveService.whenLoaded().then(() => this.initUI())

the removal of the observer notification would also be removed.

(In reply to Harshal Lele from comment #2)
> I would like to work on this bug, but it's my first bug.Can you help me out?

(In reply to wilson.sun from comment #3)
> Hi, I am a student from UofT, and I choose this bug for my good first bug
> assignment. I need someone to help me get started, much appreciated!

This is unfortunate - I'm not sure how to resolve these 2 requests :/ I encourage either of you to attach a patch as soon as you can.

Comment 5

[wilson.sun]

Thanks for the response, I understand how to fix it now. I am also wondering how could I test this after I made the changes.

Comment 6

[wilson.sun]

(In reply to Mark Hammond [:markh] from comment #4)
> The work here is to remove the "weave:service:ready" observer and the code
> handling the notification, and replace it with something like:
> 
> this.weaveService.whenLoaded().then(() => this.initUI())
> 
> the removal of the observer notification would also be removed.
> 
> (In reply to Harshal Lele from comment #2)
> > I would like to work on this bug, but it's my first bug.Can you help me out?
> 
> (In reply to wilson.sun from comment #3)
> > Hi, I am a student from UofT, and I choose this bug for my good first bug
> > assignment. I need someone to help me get started, much appreciated!
> 
> This is unfortunate - I'm not sure how to resolve these 2 requests :/ I
> encourage either of you to attach a patch as soon as you can.

Thanks for the response, I understand how to fix it now. I am also wondering how could I test this after I made the changes since the behaviour will be the same as before.

Comment 7

[Mark Hammond [:markh] [:mhammond]]

There's no good story for automatically testing this - I think the key will be thorough manual testing.

Comment 8

[George Veneel Dogga [::TheLayman]]

Hey is this still a bug?
Can I work on this?

Comment 9

[Mark Hammond [:markh] [:mhammond]]

I don't think we can do this - we'd need some way to cancel the pending promise if the window unloads before it fires, otherwise we'd leak lots of things (ie, see the unload handler in browser-syncui.js - I don't see how we'd do that)