1209589 - Collect Google Analytics data for users on IE 10 & 11

Status: RESOLVED FIXED

(Hello (Loop) :: Client, defect, P2)

Comment 1

[Romain Testard [:RT]]

Gareth, is this something you can help us with or is that a Hello dev task?

Comment 2

[Adam Roach [:abr]]

(In reply to Romain Testard [:RT] from comment #1)
> Gareth, is this something you can help us with or is that a Hello dev task?

This is completely hello dev; it's just a one-line (or so) change here:

https://dxr.mozilla.org/mozilla-central/source/browser/components/loop/standalone/content/index.html#51

Comment 3

[Adam Roach [:abr]]

Chris -- because I needed to talk about it elsewhere, I threw together a first-pass attempt at what this would look like:

  (function() {
     function insertScript(url) {
        var node = document.createElement("script");
        var sibling = document.getElementsByTagName("script")[0];
        node.async = 1;
        node.src = url;
        sibling.parentNode.insertBefore(node, sibling);
      }
 
      // Detect Internet Explorer version
      var ua = navigator.userAgent;
      var re  = new RegExp("MSIE ([0-9]{1,}[\.0-9]{0,})");
      var ie = -1;
      if (re.exec(ua)) {
        ie = parseFloat( RegExp.$1 );
      }

      // window.navigator.doNotTrack "yes" is for old versions of FF
      // window.navigator.doNotTrack "1" is for current versions of FF + Chrome + Opera
      // window.doNotTrack is Safari + IE11
      // window.navigator.msDoNotTrack for IE9 and IE10
           
      if (Math.floor(ie) == 10 || Math.floor(ie) == 11 ||
           (window.navigator.doNotTrack !== "yes" &&
            window.navigator.doNotTrack !== "1" &&
            window.doNotTrack !== "1" &&
            window.navigator.msDoNotTrack !== "1")
         ) {
 
 
        // This is an unfolded, readable version of the official GA inclusion
        // script.
        window.GoogleAnalyticsObject = "ga";
        window.ga = window.ga || function() {
          (window.ga.q = window.ga.q || []).push(arguments);
        };
        window.ga.l = 1 * new Date();
 
        insertScript("//www.google-analytics.com/analytics.js");
 
        window.ga("create", "UA-36116321-15", "auto");
        window.ga("set", "anonymizeIp", true);
        window.ga("send", "pageview");
      }
    })(); 

I don't have the means to test this, however.

Comment 4

[Adam Roach [:abr]]

Chris -- as follow-up, that first-pass snippet doesn't include the optimizely-including code, since it wasn't the topic of the conversation we were having. Please make sure that the optimizely code remains on the page. :)

Thanks.

Comment 5

[Chris Rafuse :crafuse]

Attachment: Collect Google Analytics data for users on IE 10 & 11

Comment 6

[Chris Rafuse :crafuse]

Attachment: Collect Google Analytics data for users on IE 10 & 11

Comment 7

[Chris Rafuse :crafuse]

(In reply to Adam Roach [:abr] from comment #4)
> Chris -- as follow-up, that first-pass snippet doesn't include the
> optimizely-including code, since it wasn't the topic of the conversation we
> were having. Please make sure that the optimizely code remains on the page.
> :)
> 
> Thanks.
Thanks for the working code.

See this code describe the more accurate IE version detection issue.
http://codepen.io/gapcode/pen/vEJNZN

Due to IE11 changes in userAgent removing MSIE VERSION.X from IE11 userAgent string results in IE11 detection more difficult.
This one detects Trident string for IE11 or could use bit space in the userAgent string IE12 and above.
To detect IE10 and IE11 we need to sub query if it is correct version.

In this patch code my approach uses a capabilities detection and subqueries for type or version. Performance aside this makes it clear what property is being used by which browser.

It shows as Safari in the capability detection of where window.doNotTrack property is detected and if we detect Safari browser.

IE12 detection included in the IE11 capability detection group helps narrow on which version to honour.

Pulling Mark into this, he may be able to test these scripts provided as he may have a Windows test environment.

Comment 8

[Chris Rafuse :crafuse]

Comment on attachment 8670037 [details] [diff] [review]
Collect Google Analytics data for users on IE 10 & 11

Has consoles for detection and will remove after review.  Need to see if this works on IE browsers.

Comment 9

[Adam Roach [:abr]]

Comment on attachment 8670037 [details] [diff] [review]
Collect Google Analytics data for users on IE 10 & 11

Review of attachment 8670037 [details] [diff] [review]:
-----------------------------------------------------------------

The console.log() bits are okay for debugging; I assume they'll be removed prior to landing.

A lot of these are formatting and style nits, but there's a pretty large functional flaw with the IE11/12 detection.

::: browser/components/loop/standalone/content/index.html
@@ +47,5 @@
>          var devTimeUrl = devTimeUrl.replace(/^exports.*!/, '');
>          insertScript(devTimeUrl, true);
>        }
>  
>        (function() {

I'd like to see a comment near the top here explaining the logic of this function. Something like:

// Various browsers use different mechanisms to indicate whether
// the user has opted-in to the "Do Not Track" mechanism. We check for
// each of them and disable the use of Google Analytics if any are
// present. Note that Internet Explorer versions 10 and 11 did not
// have an opt-in "Do not Track" mechanism, so we specifically
// ignore their indications.

@@ +55,1 @@
>            insertScript("//cdn.optimizely.com/js/2768540301.js");

Let's move this out to the base level. The current DNT policy (under development, but I don't expect it to change on this point) reads: "Optimizely may be used regardless of the user's DNT preference. It does not combine data about visitors to Mozilla Web properties with data from non-Mozilla properties. It is operating in a service provider capacity on behalf of Mozilla, with IP anonymization and random IDs that are specific to Mozilla’s account."

@@ +84,5 @@
> +        if (window.navigator.doNotTrack == "yes" || window.navigator.doNotTrack == "1"){
> +          console.log("Disabling Google Analytics on FF + Chrome + Oprea", window.navigator.doNotTrack);
> +          trackEnable = false;
> +        }
> +        // window.doNotTrack is Safari + IE11 + IE12

// window.doNotTrack is "1" for Safari, IE11, and IE12

@@ +87,5 @@
> +        }
> +        // window.doNotTrack is Safari + IE11 + IE12
> +        // ignore IE11 DNT setting
> +        if (window.doNotTrack == "1" ){
> +          console.log("Disabling Google Analytics on Safari + IE11");

We should remove the log messages, but I think you mean IE12 here.

@@ +88,5 @@
> +        // window.doNotTrack is Safari + IE11 + IE12
> +        // ignore IE11 DNT setting
> +        if (window.doNotTrack == "1" ){
> +          console.log("Disabling Google Analytics on Safari + IE11");
> +          //honour if Safari

Space after // -- capitalize "Honour"

@@ +89,5 @@
> +        // ignore IE11 DNT setting
> +        if (window.doNotTrack == "1" ){
> +          console.log("Disabling Google Analytics on Safari + IE11");
> +          //honour if Safari
> +          var is_chrome = navigator.userAgent.indexOf('Chrome') > -1;

* use isChrome rather than is_chrome
* Use double quotes

@@ +90,5 @@
> +        if (window.doNotTrack == "1" ){
> +          console.log("Disabling Google Analytics on Safari + IE11");
> +          //honour if Safari
> +          var is_chrome = navigator.userAgent.indexOf('Chrome') > -1;
> +          var is_safari = navigator.userAgent.indexOf("Safari") > -1;

Use isSafari rather than is_safari

@@ +91,5 @@
> +          console.log("Disabling Google Analytics on Safari + IE11");
> +          //honour if Safari
> +          var is_chrome = navigator.userAgent.indexOf('Chrome') > -1;
> +          var is_safari = navigator.userAgent.indexOf("Safari") > -1;
> +          if ((is_chrome)&&(is_safari)) {is_safari=false;}

Fix formatting and spaces:

if (isChrome && isSafari) {
  isSafari = false;
}

Also, this logic needs explanation: presumably, there's something about some user agent's string that requires this behavior. Without knowing what that is, the logic of "if it is Chrome and it is Safari, then it's not Safari" is very confusing.

@@ +95,5 @@
> +          if ((is_chrome)&&(is_safari)) {is_safari=false;}
> +          if (is_safari){
> +            trackEnable = false;
> +          } else {
> +            //is MS IE11 or IE12

Space after // -- captialize "Is"

@@ +97,5 @@
> +            trackEnable = false;
> +          } else {
> +            //is MS IE11 or IE12
> +            console.log("/x64|x32/ig.test(window.navigator.userAgent)", /x64|x32/ig.test(window.navigator.userAgent));
> +            //detect IE12 bit space and honor IE12

Space after // -- Capitalize "Detect"

"bit space"? I don't think I follow what this is intended to mean. Maybe something like:

// IE 12 includes a CPU architecture string, while IE 11 does not.
// We use this architecture string to detect IE 12 and honor its
// DNT setting.

However, on a quick check, I don't think this is true. According to <https://msdn.microsoft.com/en-us/library/hh869301%28v=vs.85%29.aspx>, the UA string for IE11 on 64-bit Windows 8.1 is "Mozilla/5.0 (Windows NT 6.3; Win64, x64; Trident/7.0; Touch; rv:11.0) like Gecko". It seems to me that the following code will treat this as IE12.

In fact, glancing over the extremely wide variety of UA strings on that page, the only thing that appears to be mostly reliable is the presence of /rv[ :]11/ in the UA string for IE11. It's missing from some configurations, but those appear to be somewhat rare.

In other words, I would suggest changing this to:

// Don't enable tracking for IE11
if (!/rv[ :]11/g.test(window.navigator.userAgent)) {

@@ +103,5 @@
> +              trackEnable = false;
> +            }
> +          }
> +        }
> +        //window.navigator.msDoNotTrack for IE9 and IE10

Space after //

// window.navigator.msDoNotTrack is "1" for IE9 and IE10

@@ +104,5 @@
> +            }
> +          }
> +        }
> +        //window.navigator.msDoNotTrack for IE9 and IE10
> +        //ignore IE10 DNT settings

Space after // -- capitalize "Ignore"

@@ +107,5 @@
> +        //window.navigator.msDoNotTrack for IE9 and IE10
> +        //ignore IE10 DNT settings
> +        if (window.navigator.msDoNotTrack == "1") {
> +          console.log("Disabling Google Analytics on IE9 and IE11: window.navigator.msDoNotTrack", window.navigator.msDoNotTrack);
> +          //honour IE9

Space after // -- captialize "Honour":

// Honour DNT setting for IE9

@@ +108,5 @@
> +        //ignore IE10 DNT settings
> +        if (window.navigator.msDoNotTrack == "1") {
> +          console.log("Disabling Google Analytics on IE9 and IE11: window.navigator.msDoNotTrack", window.navigator.msDoNotTrack);
> +          //honour IE9
> +          var isIE9 = window.navigator.userAgent.search('IE 9');

Double quotes

@@ +110,5 @@
> +          console.log("Disabling Google Analytics on IE9 and IE11: window.navigator.msDoNotTrack", window.navigator.msDoNotTrack);
> +          //honour IE9
> +          var isIE9 = window.navigator.userAgent.search('IE 9');
> +
> +          if(isIE9) {

Space after "if"

This decomposition seems a bit overwrought. I suggest something more like:

// Honor MS-specific "Do Not Track" for IE 9 only
if (window.navigator.msDoNotTrack == "1" &&
    window.navigator.userAgent.search("IE 9")) {
  trackEnable = false;
}

@@ +116,5 @@
> +          }
> +          console.log("isIE9", isIE9);
> +        }
> +
> +        if(trackEnable) {

Space after "if"

Comment 10

[Chris Rafuse :crafuse]

Moving optimizely script out of conditional test for DNT.

Comment 11

[Chris Rafuse :crafuse]

Attachment: Collect Google Analytics data for users on IE 10 & 11

Comment 12

[Chris Rafuse :crafuse]

Comment on attachment 8670403 [details] [diff] [review]
Collect Google Analytics data for users on IE 10 & 11

the optimizely script is always inserted onto the page.

Comment 13

[Chris Rafuse :crafuse]

Comment on attachment 8670403 [details] [diff] [review]
Collect Google Analytics data for users on IE 10 & 11

Need to add review comment changes.

Comment 14

[Adam Roach [:abr]]

(In reply to Adam Roach [:abr] from comment #9)

> In other words, I would suggest changing this to:
> 
> // Don't enable tracking for IE11

Of course, I meant "disable" rather than "enable" here.

Comment 15

[Chris Rafuse :crafuse]

Attachment: Collect Google Analytics data for users on IE 10 & 11

Comment 16

[Chris Rafuse :crafuse]

Comment on attachment 8670435 [details] [diff] [review]
Collect Google Analytics data for users on IE 10 & 11

Review comment changes and optimizely script always loads first.

Comment 17

[Adam Roach [:abr]]

Comment on attachment 8670435 [details] [diff] [review]
Collect Google Analytics data for users on IE 10 & 11

Review of attachment 8670435 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. Thanks!

Comment 18

[Chris Rafuse :crafuse]

Attachment: Collect Google Analytics data for users on IE 10 & 11

Comment 19

[Chris Rafuse :crafuse]

Comment on attachment 8670538 [details] [diff] [review]
Collect Google Analytics data for users on IE 10 & 11

Squashed version includes first commit changes

Comment 20

[Mark Banner (:standard8)]

Comment on attachment 8670538 [details] [diff] [review]
Collect Google Analytics data for users on IE 10 & 11

Review of attachment 8670538 [details] [diff] [review]:
-----------------------------------------------------------------

Ok, I don't think this is far off. I'd like to check the IE 11 case, but I need to figure out how to do that in my VM. Lets get the patch updated for the IE 10 case first, and then I'll investigate updating IE.

::: browser/components/loop/standalone/content/index.html
@@ +87,5 @@
> +        var trackEnable = true;
> +        // window.navigator.doNotTrack "yes" is for old versions of FF
> +        // window.navigator.doNotTrack "1" is for current versions of FF + Chrome + Opera
> +        console.log("window.navigator.userAgent", window.navigator.userAgent);
> +        if (window.navigator.doNotTrack == "yes" || window.navigator.doNotTrack == "1"){

nit: missing space after )

We should also be using === and !== in this file. 

It'd also be nice to split the if statement onto two lines since it splits easily.

@@ +109,5 @@
> +          }
> +        }
> +        // window.navigator.msDoNotTrack is "1" for IE9 and IE10
> +        // Honor MS-specific "Do Not Track" for IE 9 only
> +        if (window.navigator.msDoNotTrack == "1" && window.navigator.userAgent.search("IE 9")) {

Please split this onto two lines, however you also need to add a check for != 1 for the search:

if (window.navigator.msDoNotTrack == "1" &&
    window.navigator.userAgent.search("IE 9") != -1) {

Comment 21

[Chris Rafuse :crafuse]

Attachment: Collect Google Analytics data for users on IE 10 & 11

Comment 22

[Chris Rafuse :crafuse]

Comment on attachment 8670908 [details] [diff] [review]
Collect Google Analytics data for users on IE 10 & 11

Cleaned up styling and added not minus 1 for IE9 search.  Left consoles logs in for testing on Mark's possible Windows test machine.

Comment 23

[Adam Roach [:abr]]

Comment on attachment 8670908 [details] [diff] [review]
Collect Google Analytics data for users on IE 10 & 11

Review of attachment 8670908 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, with one nit.

Please do not land this until you remove the console.log() statements.

::: browser/components/loop/standalone/content/index.html
@@ +93,5 @@
> +          trackEnable = false;
> +        }
> +        // window.doNotTrack is "1" for Safari, IE11, and IE12
> +        // Ignore IE11 DNT setting
> +        if (window.doNotTrack == "1" ){

Swap paren and space (sorry, didn't notice this before):

if (window.doNotTrack == "1") {

Comment 24

[Adam Roach [:abr]]

Comment on attachment 8670908 [details] [diff] [review]
Collect Google Analytics data for users on IE 10 & 11

Review of attachment 8670908 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, with one nit.

Please do not land this until you remove the console.log() statements.

::: browser/components/loop/standalone/content/index.html
@@ +93,5 @@
> +          trackEnable = false;
> +        }
> +        // window.doNotTrack is "1" for Safari, IE11, and IE12
> +        // Ignore IE11 DNT setting
> +        if (window.doNotTrack == "1" ){

Swap paren and space (sorry, didn't notice this before):

if (window.doNotTrack == "1") {

Comment 25

[Adam Roach [:abr]]

Comment on attachment 8670908 [details] [diff] [review]
Collect Google Analytics data for users on IE 10 & 11

Review of attachment 8670908 [details] [diff] [review]:
-----------------------------------------------------------------

Gah. Changed the wrong review flag. Adding Mark back to the list.

Comment 26

[Chris Rafuse :crafuse]

Attachment: Collect Google Analytics data for users on IE 10 & 11

Comment 27

[Chris Rafuse :crafuse]

Comment on attachment 8671439 [details] [diff] [review]
Collect Google Analytics data for users on IE 10 & 11

This version HAS the console logs

Comment 28

[Chris Rafuse :crafuse]

Attachment: Collect Google Analytics data for users on IE 10 & 11

Comment 29

[Chris Rafuse :crafuse]

Comment on attachment 8671444 [details] [diff] [review]
Collect Google Analytics data for users on IE 10 & 11

This version does NOT have console logs.

Comment 30

[Mark Banner (:standard8)]

Comment on attachment 8671444 [details] [diff] [review]
Collect Google Analytics data for users on IE 10 & 11

Review of attachment 8671444 [details] [diff] [review]:
-----------------------------------------------------------------

Adam's already given r+ so no need to repeat. I can review this as well - so Dan doesn't need to.

I'm still finding a way to see if we can get this tested on IE 11, in the meantime a few comments.

::: browser/components/loop/standalone/content/index.html
@@ +84,5 @@
> +        // https://developer.mozilla.org/en-US/docs/Web/API/Navigator/doNotTrack#Browser_compatibility
> +        var trackEnable = true;
> +        // window.navigator.doNotTrack "yes" is for old versions of FF
> +        // window.navigator.doNotTrack "1" is for current versions of FF + Chrome + Opera
> +        if (window.navigator.doNotTrack === "yes" || window.navigator.doNotTrack == "1") {

You need to change all occurances of == to === and != to !== please.

@@ +89,5 @@
> +          trackEnable = false;
> +        }
> +        // window.doNotTrack is "1" for Safari, IE11, and IE12
> +        // Ignore IE11 DNT setting
> +        if (window.doNotTrack == "1"){

nit: missing space after )

@@ +91,5 @@
> +        // window.doNotTrack is "1" for Safari, IE11, and IE12
> +        // Ignore IE11 DNT setting
> +        if (window.doNotTrack == "1"){
> +          // Honour if Safari or IE12
> +          if (navigator.userAgent.indexOf("Safari") > -1) {

nit: I think I'd prefer !== -1

Comment 31

[Chris Rafuse :crafuse]

Attachment: Collect Google Analytics data for users on IE 10 & 11

Comment 32

[Chris Rafuse :crafuse]

Attachment: Collect Google Analytics data for users on IE 10 & 11

Comment 33

[Chris Rafuse :crafuse]

Comment on attachment 8672046 [details] [diff] [review]
Collect Google Analytics data for users on IE 10 & 11

WITH console log output.

Comment 34

[Chris Rafuse :crafuse]

Comment on attachment 8672048 [details] [diff] [review]
Collect Google Analytics data for users on IE 10 & 11

WITH OUT console logs.
Made all conditions use === tests.
Clean up of code style.

Comment 35

[Mark Banner (:standard8)]

Comment on attachment 8672048 [details] [diff] [review]
Collect Google Analytics data for users on IE 10 & 11

Review of attachment 8672048 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good, I've been able to test it in IE 10 & 11 and it seems to do the right thing. r=Standard8 with the nit fixed.

I think we should really get unit tests on this, but currently it'd be very difficult given that its in-line - once bug 1212428 is completed, then we might be able to add tests.

::: browser/components/loop/standalone/content/index.html
@@ +62,5 @@
>            // script.
>            window.GoogleAnalyticsObject = "ga";
>            window.ga = window.ga || function() {
> +                    (window.ga.q = window.ga.q || []).push(arguments);
> +                  };

nit: this indentation should stay the same as what it was.

Comment 36

[Chris Rafuse :crafuse]

Attachment: Collect Google Analytics data for users on IE 10 & 11

Comment 37

[Chris Rafuse :crafuse]

Comment on attachment 8673859 [details] [diff] [review]
Collect Google Analytics data for users on IE 10 & 11

Fixed indentation nit.

Comment 38

[Mark Banner (:standard8)]

Comment on attachment 8673859 [details] [diff] [review]
Collect Google Analytics data for users on IE 10 & 11

No need to request r+ again when I've already given it, unless there's bigger changes than were maybe expected.

Comment 39

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/2f685ede3d97

Comment 40

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2f685ede3d97