Project

General

Profile

Bug #11405

Bug #7161: Support more than 24 HTTP mirrors

Feature #8640: Have the mirror pool dispatcher library audited

Test mirror pool dispatcher script in different browsers

Added by u about 3 years ago. Updated almost 3 years ago.

Status:
Resolved
Priority:
Normal
Assignee:
-
Category:
Infrastructure
Target version:
Start date:
05/10/2016
Due date:
% Done:

100%

Feature Branch:
451f:mirror-pool-dispatcher/master
Type of work:
Test
Blueprint:
Starter:
Affected tool:

Description

Including IE & Safari on Windows & possibly MacOS.

History

#1 Updated by intrigeri about 3 years ago

  • 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 u about 3 years ago

  • Status changed from Confirmed to In Progress

#3 Updated by u about 3 years ago

  • Assignee changed from u 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 u about 3 years ago

  • % Done changed from 0 to 10

#5 Updated by intrigeri about 3 years ago

  • Assignee changed from intrigeri to u
  • 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 u about 3 years ago

  • Assignee changed from u 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?

#7 Updated by u about 3 years ago

So, IE expects something like that:

Object.assign(exports, transformURL);

instead of

Object.assign(exports, { transformURL });

Actually, only DAVE expects the latter. Let's see how we could fix this in DAVE eventually.

#8 Updated by u about 3 years ago

Ok, I've renamed mirror-dispatcher-ie.js to mirror-dispatcher-nopromise.js.
I've tested this with DAVE and it works correctly too.

That means that we could now switch to using this file only. But we need to verify this works from your side too.

#9 Updated by u about 3 years ago

  • 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 about 3 years ago

  • Assignee changed from intrigeri to u
  • 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 (:mirror-pool-dispatcher.git): :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 :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 u about 3 years ago

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

#12 Updated by u about 3 years ago

intrigeri wrote:

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 :)

ack :)

In passing, I think you pushed to the wrong place (:mirror-pool-dispatcher.git): :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 :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.

Ok, sorry about that. I did it correctly this time.

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 :)

yay!

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)?

There was no reason other than the PoC code which worked like this, so I moved it back where it was before. Thanks for noticing.

  • 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.

Done.

#13 Updated by u about 3 years ago

I'll let you retest a last time from your side and then close this ticket?

#14 Updated by intrigeri about 3 years ago

  • Assignee changed from intrigeri to u
  • % 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?

#15 Updated by u about 3 years ago

intrigeri wrote:

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"?

taking care of that now.

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!

ack.

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.

it was tested on a Windows safari, but i'll recheck with the current version and will test in Mac OS too.

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

always testing in chromium.

#16 Updated by u about 3 years ago

u wrote:

intrigeri wrote:

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"?

taking care of that now.

pushed.

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.

it was tested on a Windows safari, but i'll recheck with the current version and will test in Mac OS too.

Will report back.

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

always testing in chromium.

tested again in TBB and Chromium. Works correctly.

#17 Updated by intrigeri about 3 years ago

taking care of that now.

pushed.

Good, merged!

#18 Updated by u about 3 years ago

  • 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.

#19 Updated by intrigeri about 3 years ago

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

Woohoo! \o/

#20 Updated by intrigeri about 3 years ago

  • Assignee deleted (u)

#21 Updated by BitingBird almost 3 years ago

  • Priority changed from Elevated to Normal

Also available in: Atom PDF