Feature #14945

Review dave_2.js

Added by sajolida 2017-11-09 18:50:43 . Updated 2017-12-16 07:37:08 .

Status:
Resolved
Priority:
Normal
Assignee:
Category:
Installation
Target version:
Start date:
2017-11-09
Due date:
% Done:

0%

Feature Branch:
master
Type of work:
Code
Blueprint:

Starter:
Affected tool:
Deliverable for:

Description

Cody will review the display logic and the interaction with download_2.inline.html as it’s quite complete but Ulrike said she would review the JavaScript for syntax, style, best practices, code structure, security, etc.

I’m pushing changes to master as I make it because I need the page to be on the production website for testing. So the review should cover /install/inc/js/dave_2.js on master. Yeah, rock’n’roll!


Files

dave_2.js (10708 B) Anonymous, 2017-11-12 19:52:20

Subtasks


History

#1 Updated by sajolida 2017-11-09 18:55:48

  • Description updated

#2 Updated by Anonymous 2017-11-12 19:52:34

  • File dave_2.js added
  • Assignee set to sajolida
  • QA Check set to Dev Needed

Hey!

I did not dare pushing to master and I did not test any of my modifications. I am attaching the file here for you so you can do a diff and test yourself.
I only found some minor issues, like the need to wrap regexps and the syntax of the EventListener function. Please test what I did. I also added some URLs for you to verify yourself.

#3 Updated by sajolida 2017-11-14 13:17:32

  • Target version changed from Tails_3.3 to Tails_3.5

#4 Updated by sajolida 2017-11-14 15:47:44

  • Assignee deleted (sajolida)
  • QA Check changed from Dev Needed to Ready for QA

Cool!

I applied your changes in a bunch of commits, please have a second look:

7a6deb9390 Fix grammar
d984f67ab2 Extract function
abff6a6ac0 Explain how the page is reset at first
5399fcfee2 Wrap regexp
e257655cff Use 'let' instead of 'var' in 'for' loops
873ddf7a35 Use a class instead of embedding CSS in the JS code
7bb9426dde Factorize
36aa0a13cc Explain why we're doing this with JS instead of CSS
a63118b976 Use double quotes instead of single quotes
  • Regarding “function showFloatingToggleableLinks” I added a comment to explain why I have to do this in JS. See 36aa0a13cc and tell me if that’s not clear enough.
  • Regarding escaping the file name. I tried with '*?\~_-" as file name and it’s displayed correctly, so I think we don’t need that for display purpose. If you were thinking about security issues, please elaborate and maybe tell me a bit more about how I should escape (sanitize?) this.
  • I didn’t merged the third parameter that you added to EventTarget.addEventListener() (wantsUntrusted=false) because Mozilla says it’s Firefox specific: https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener.
  • I didn’t understand what you meant by “incorrect function call” so if what I copied from your version is not enough, please elaborate.

#5 Updated by intrigeri 2017-11-18 10:52:01

  • Affected tool changed from Download and Verification Extension to Verification Extension

#6 Updated by Anonymous 2017-11-20 12:54:45

  • Assignee set to sajolida
  • QA Check changed from Ready for QA to Dev Needed
  • Affected tool changed from Verification Extension to Download and Verification Extension

> I applied your changes in a bunch of commits, please have a second look:

Great!

> * Regarding “function showFloatingToggleableLinks” I added a comment to explain why I have to do this in JS. See 36aa0a13cc and tell me if that’s not clear enough.

Ack.

> * Regarding escaping the file name. I tried with '*?\~_-" as file name and it’s displayed correctly, so I think we don’t need that for display purpose. If you were thinking about security issues, please elaborate and maybe tell me a bit more about how I should escape (sanitize?) this.

Yes that was my idea, see https://stackoverflow.com/questions/16860287/security-comparison-of-eval-and-innerhtml-for-clientside-javascript as an example.

> * I didn’t merged the third parameter that you added to EventTarget.addEventListener() (wantsUntrusted=false) because Mozilla says it’s Firefox specific: https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener.

Ok, but you could add it if it does not break Chrome, right?

> * I didn’t understand what you meant by “incorrect function call” so if what I copied from your version is not enough, please elaborate.

I think you changed it already.

Thanks for adding the link “I already downloaded the ISO.”
I just tried everything again and I have some comments on UX. Please add them yourself to relevant tickets if you agree:

  • When I see “Verification failed” I immediately had the feeling that I want to know why. Could we add something like “Checksums did not match”? Or something similar?
  • Actually I know now that the verification failed because I tried to verify Tails 3.2 and the checksum was matched against 3.3 - maybe we can make it clear somewhere what we compare or when people want to compare an older version tell them that this is not possible?
  • When I installed the extension, the “Verify” button becomes visible. But IMO one can easily miss this modification. Would it be possible to make it more visible? Either by using another colour or by making this button bigger or animate the background colour or something similar?

#7 Updated by sajolida 2017-11-22 21:04:12

  • Status changed from Confirmed to Resolved
  • Assignee deleted (sajolida)
  • QA Check deleted (Dev Needed)

Cool! I moved your comments to Bug #14998, Bug #14997, and Feature #14921.

I can close this ticket now.

#8 Updated by intrigeri 2017-12-16 07:37:08

  • Affected tool deleted (Download and Verification Extension)