Skip to content
This repository was archived by the owner on Mar 15, 2018. It is now read-only.

defer loading of persona include.js (bug 983556)#398

Closed
cvan wants to merge 1 commit intomozilla:masterfrom
cvan:potatopersona
Closed

defer loading of persona include.js (bug 983556)#398
cvan wants to merge 1 commit intomozilla:masterfrom
cvan:potatopersona

Conversation

@cvan
Copy link
Copy Markdown
Contributor

@cvan cvan commented Mar 15, 2014

  1. When the page first loads, and if the user was not already signed in, we set cursor: wait for the "Sign in" link so it's clear that the user is in some holding pattern. (I haven't been able to see this, even on my crappy Comcast connection, unless I'm proxying Persona through a delayer.)

  2. We check if Persona has been loaded every 25 milliseconds (simple 'id' in navigator check).

    a. If Persona loads within the 30 seconds, we resolve the deferred and let the user sign in (i.e., we call navigator.id.watch).
    b. If Persona times out after 30 seconds, we reject the deferred and pop up a notification telling the user that Persona is responsive.

Feel free to test this locally with http://deelay.me/5000/http://login.persona.org/include.js to simulate a slow Persona load.

Comment thread hearth/index.html
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll need to update this in zamboni's mkt.commonplace template of course

@cvan
Copy link
Copy Markdown
Contributor Author

cvan commented Mar 17, 2014

Comment thread hearth/media/js/login.js Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe not a huge deal, but nothing is telling you that the setInterval actually waited for persona_step ms and not more. If you want to be safe, I think you need to do Date comparisons here.

Also, I am not sure you need the interval to be as low as 25ms.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well, it's going to be at most 24ms off.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can be much much worse. Browsers don't respect setInterval/setTimeout when you are not the foreground tab, not to mention possible delays because other processes want the CPU for themselves.

If you want to test, open a tab with http://virgule.net/tmp/test-interval.html, quickly open another tab and start doing something in that tab for a few seconds, come back and look at your js console ; you should see that some iterations took up to 1 second instead of 25 ms.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good call, will change

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to keep it as low as possible

@cvan
Copy link
Copy Markdown
Contributor Author

cvan commented Mar 20, 2014

Looks very well thought out and the biggest gain might be from the <script async... itself.

Notice we already doing defer: view-source:https://marketplace.firefox.com

But, yeah... async is what we want.

@cvan
Copy link
Copy Markdown
Contributor Author

cvan commented Mar 20, 2014

💚 the build passed! Travis is a doggone liar!

https://travis-ci.org/mozilla/fireplace/builds/21192431

@cvan cvan closed this Mar 20, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants