Tails

#1 Updated by intrigeri 2016-05-10 03:26:53

• Category set to Infrastructure
• Status changed from New to Confirmed
• Priority changed from Normal to Elevated
• Target version set to Tails_2.4

(This is something we were supposed to deliver in April => bumping priority a bit.)

#2 Updated by Anonymous 2016-05-13 03:08:22

• Status changed from Confirmed to In Progress

#3 Updated by Anonymous 2016-05-13 03:25:20

• Assignee set to intrigeri
• QA Check set to Info Needed

Tested in IE and remembered that IE does not support promises. That means, that in no IE version the script may work. Should we replace the part of the code using the promise for IE browsers?

—> Update. looking a bit more into this. We still have the first version of the code, using XHR. We could either create a file for IE browsers only, using this code. That would be a double thingy to maintain, but still, it would allow for clean codebase and we can delete this file once IE proposes Promises. Another possibility is to include an external library, but I think that’s something we wanted to avoid and we should still avoid.

In Safari, Chrome and Opera and FF Javascript Promises are supported.

I’ve tested this also in current Sid Chromium and it works.

Reference: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise#Browser_compatibility

#4 Updated by Anonymous 2016-05-13 03:25:30

• % Done changed from 0 to 10

#5 Updated by intrigeri 2016-05-13 13:29:27

• Assignee deleted (intrigeri)
• QA Check deleted (Info Needed)

> Tested in IE and remembered that IE does not support promises. That means, that in no IE version the script may work. Should we replace the part of the code using the promise for IE browsers?

IMO our mirror dispatcher should support major browsers, and that (I believe) still includes IE. Now, I’ll let it up to you to judge if it’s better to replace the promise-based implementation for everyone, or to maintain a second implementation just for IE :)

Cheers!

#6 Updated by Anonymous 2016-05-15 06:30:56

• Assignee set to intrigeri
• % Done changed from 10 to 20
• Feature Branch set to 451f:mirror-pool-dispatcher/master

I agree completely.

So I’ve pushed a fix for this. For now, I use two scripts, one for IE, one for the other browsers.
The reason is mainly that Object.assign is not supported in IE (only latest version) and this throws an error and prevents the script from functioning. Also see https://msdn.microsoft.com/en-us/library/dn858229%28v=vs.94%29.aspx.

Otherwise, mirror-dispatcher-ie.js works exactly the same as mirror-dispatcher.js, except that it does not use Promises. I’ve tested that in Iceweasel and Chromium, too and it works as expected.

I am completely aware that it’s a bit a pain to maintain two scripts.

This also makes us modify how we include the mirror-dispatcher.js script on the website. Please have a look at mirror-dispatcher-web.html for an example. It makes no big difference IMO, but it’s a bit weird. We actually need to dynamically write which script to use, based on browser detection in JS. It would have been nicer to be able to use a conditional comment for IE. However, the use of those has been deprecated in IE10, so this does not fullfil our needs.

I am still searching for a way to make IE ignore Object.assign’s syntax instead of throwing an error.
If I manage to do so, we could use this script for both, but then again you’ll also need to reverify in nodejs for the Upgrader. So maybe the better solution is to use these two scripts and not bother more than that?

What do you think?

#9 Updated by Anonymous 2016-05-16 07:08:37

• QA Check set to Info Needed

Please tell me if it works in node and then I can rename the new file and move the old file somewhere else for a shiny future with Promises in all browsers.

#10 Updated by intrigeri 2016-05-16 12:11:02

• Assignee deleted (intrigeri)
• QA Check changed from Info Needed to Dev Needed

Good news! I’m glad we don’t need to have two scripts (well, if we had, we would have factorized the 99% common bits into a single library that both scripts would have loaded, but still :)

In passing, I think you pushed to the wrong place (tails@git.tails.boum.org:mirror-pool-dispatcher.git): tails@git.tails.boum.org:mirror-pool-dispatcher.git is a mirror of the official repo, and whatever you pushed there will be overwritten next time someone pushes to the official repo; so, please push to tails@git.tails.boum.org:451f/mirror-pool-dispatcher.git instead. I’ve done it for you this time, so that your work doesn’t suddenly disappears from the Internet.

> Please tell me if it works in node

I’m happy to report that it works fine both in sid’s NodeJS, and in Jessie’s :)

> and then I can rename the new file and move the old file somewhere else for a shiny future with Promises in all browsers.

Please go ahead! (The old file is in Git’s history, so no need to keep in in the working tree.)

Two more comments while I’m at it:

• I personally find that moving JSON.parse to get is somewhat confusing, and makes the role of the get function not obviously match its name. I could find no explanation for this change in the Git history. Can we revert back to the previous split of responsibilities between functions, or is there a good reason for the new one (and then, get should be renamed IMO)?
• It would be nice to notify co6 that the code she’s reviewing is going to change again soon, so she can hold on and avoid spending time on it now.

#11 Updated by Anonymous 2016-05-17 10:43:29

• Assignee set to intrigeri
• QA Check changed from Dev Needed to Ready for QA

#14 Updated by intrigeri 2016-05-17 10:50:45

• Assignee deleted (intrigeri)
• % Done changed from 20 to 50
• QA Check changed from Ready for QA to Dev Needed

Here are a couple small code style comments.

• I see tow new if statement without curly braces. I’d rather not :)
• Missing words after “nor throw an error. Disabling”?

Aside of those, all right for me at commit 66ee38230ef3385b43dfeae4439a9aca5d030adb!

> I’ll let you retest a last time from your side

Tested on sid and Jessie, works fine!

> and then close this ticket?

Wait, was this tested in Safari? I see a note about it supporting promises, but no explicit statement regarding whether the code was actually tested there.

Also, it would be nice to test the latest code in Chromium again, just in case, perhaps?

#18 Updated by Anonymous 2016-05-20 11:01:21

• Status changed from In Progress to Resolved
• % Done changed from 50 to 100
• QA Check deleted (Dev Needed)

I’ve retested it in Safari 9 on Mac OSX and it works as expected.

Closing.

#20 Updated by intrigeri 2016-05-23 14:09:50

• Assignee deleted ()

#21 Updated by BitingBird 2016-06-26 11:15:42

• Priority changed from Elevated to Normal