static/js/index.js - Issue 29341378: Issue 4027 - make the download button available for windows insider users

Delta Between Two Patch Sets: static/js/index.js

Issue 29341378: Issue 4027 - make the download button available for windows insider users (Closed)

Left Patch Set: Created May 13, 2016, 10:40 a.m.

Right Patch Set: Created May 18, 2016, 12:13 p.m.

Left:
Right:

Use n/p to move between diff chunks; N/P to move between comments.

Jump to:

Left: Side by side diff | Download
Right: Side by side diff | Download

LEFT	RIGHT
1 (function()	1 (function()

2 {	2 {

3 var visibleTab;	3 var visibleTab;

4 var container;	4 var container;

5	5

6 window.toggleMore = function()	6 window.toggleMore = function()

7 {	7 {

8 if (container.className == "hidden")	8 if (container.className == "hidden")

9 container.className = visibleTab \|\| getDefaultTab();	9 container.className = visibleTab \|\| getDefaultTab();

10 else	10 else

(...skipping 11 matching lines...) Expand all Loading...
22 {	22 {

23 var content = document.getElementById("content");	23 var content = document.getElementById("content");

24 var ua = content.className.match(/ua\-([^\s]+)/);	24 var ua = content.className.match(/ua\-([^\s]+)/);

25 visibleTab = ua && ua[1] \|\| "firefox";	25 visibleTab = ua && ua[1] \|\| "firefox";

26 return visibleTab;	26 return visibleTab;

27 }	27 }

28	28

29 function init()	29 function init()

30 {	30 {

31 container = document.getElementById("more-container");	31 container = document.getElementById("more-container");

32 if(document.getElementById("content").className.indexOf("edge") > -1)	32 checkEdgeSupport();
Sebastian Noack 2016/05/17 08:50:13 Perhaps moving this check inside checkEdgeSupport( Perhaps moving this check inside checkEdgeSupport(). Then you could simply store the value of document.getElementById("content") in a variable instead duplication the lookup. saroyanm 2016/05/17 12:38:54 Done. Show quoted text On 2016/05/17 08:50:13, Sebastian Noack wrote: > Perhaps moving this check inside checkEdgeSupport(). Then you could simply store > the value of document.getElementById("content") in a variable instead > duplication the lookup. Done.
33 checkEdgeSupport();

34 }	33 }

35	34

36 function checkEdgeSupport()	35 function checkEdgeSupport()

37 {	36 {

38 if (!window.navigator.userAgent)	37 if (!window.navigator.userAgent)
saroyanm 2016/05/13 10:50:20 userAgent detection through navigator is deprecate userAgent detection through navigator is deprecated ( https://developer.mozilla.org/en-US/docs/Web/API/NavigatorID/userAgent ), I'm not sure if there is another way to detect the users, but this way is not reliable currently and can start not working at anypoint in the future. Maybe there is a feature that has been added in the new builds or maybe we can check if the extension support is available on Edge versions, please let me know guys if you know better way to detect this. Sebastian Noack 2016/05/17 08:50:13 I don't think there is a different way to detect t Show quoted text On 2016/05/13 10:50:20, saroyanm wrote: > userAgent detection through navigator is deprecated ( > https://developer.mozilla.org/en-US/docs/Web/API/NavigatorID/userAgent ), I'm > not sure if there is another way to detect the users, but this way is not > reliable currently and can start not working at anypoint in the future. Maybe > there is a feature that has been added in the new builds or maybe we can check > if the extension support is available on Edge versions, please let me know guys > if you know better way to detect this. I don't think there is a different way to detect the browser versions on the client side. However, I don't expect navigator.userAgent going away any time soon, and in particular not while we need that check here, since a lot of websites rely on it. So I think it's fine to rely on navigator.userAgent here. saroyanm 2016/05/17 12:38:53 Acknowledged. Show quoted text On 2016/05/17 08:50:13, Sebastian Noack wrote: > On 2016/05/13 10:50:20, saroyanm wrote: > > userAgent detection through navigator is deprecated ( > > https://developer.mozilla.org/en-US/docs/Web/API/NavigatorID/userAgent ), I'm > > not sure if there is another way to detect the users, but this way is not > > reliable currently and can start not working at anypoint in the future. Maybe > > there is a feature that has been added in the new builds or maybe we can check > > if the extension support is available on Edge versions, please let me know > guys > > if you know better way to detect this. > > I don't think there is a different way to detect the browser versions on the > client side. However, I don't expect navigator.userAgent going away any time > soon, and in particular not while we need that check here, since a lot of > websites rely on it. So I think it's fine to rely on navigator.userAgent here. Acknowledged.
39 return;	38 return;

40	39

	40 var content = document.getElementById("content");

41 var match = window.navigator.userAgent.match(/Edge\/(\d+.\d+)/);	41 var match = window.navigator.userAgent.match(/Edge\/(\d+.\d+)/);

42 if (match && match.length > 0 && parseFloat(match[1]) >= 14.14342)	42 if (match && parseFloat(match[1]) >= 14.14342 &&
Sebastian Noack 2016/05/17 08:50:13 The check for \|match.length > 0\| is redundant. If The check for \|match.length > 0\| is redundant. If there is a match it will always be an array with one element per group (even if the group would be optional). saroyanm 2016/05/17 12:38:54 Done. Show quoted text On 2016/05/17 08:50:13, Sebastian Noack wrote: > The check for \|match.length > 0\| is redundant. If there is a match it will > always be an array with one element per group (even if the group would be > optional). Done.	Oleksandr 2016/05/18 12:53:15 14 is a version of an EdgeHTML. 14342 is a Windows 14 is a version of an EdgeHTML. 14342 is a Windows build number. Logically I think we should check only the Windows build number. EdgeHTML version might not be connected to the extensions functionality at all. See: https://blogs.windows.com/msedgedev/2015/09/21/understanding-versions-in-an-e... Sebastian Noack 2016/05/18 12:57:25 Yeah, I agree. However, it seems, the change alrea Show quoted text On 2016/05/18 12:53:15, Oleksandr wrote: > 14 is a version of an EdgeHTML. 14342 is a Windows build number. Logically I > think we should check only the Windows build number. EdgeHTML version might not > be connected to the extensions functionality at all. > > See: > https://blogs.windows.com/msedgedev/2015/09/21/understanding-versions-in-an-e... Yeah, I agree. However, it seems, the change already landed.
43 document.getElementById("content").className += " edge-supported";	43 content.classList.contains("edge"))
Sebastian Noack 2016/05/17 08:50:13 How about using classList? We don't need to care a How about using classList? We don't need to care about compatibility with old IE versions here. saroyanm 2016/05/17 12:38:53 Good point Show quoted text On 2016/05/17 08:50:13, Sebastian Noack wrote: > How about using classList? We don't need to care about compatibility with old IE > versions here. Good point saroyanm 2016/05/17 12:38:54 Done. Show quoted text On 2016/05/17 08:50:13, Sebastian Noack wrote: > How about using classList? We don't need to care about compatibility with old IE > versions here. Done.
	44 content.classList.add("edge-supported");

44 }	45 }

45	46

46 init();	47 init();

47 })();	48 })();

LEFT	RIGHT