Closed Bug 1209589 Opened 6 years ago Closed 6 years ago

Collect Google Analytics data for users on IE 10 & 11

Categories

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

defect
Points:
2

Tracking

(firefox44 fixed)

RESOLVED FIXED
mozilla44
Iteration:
44.2 - Oct 19
Tracking Status
firefox44 --- fixed

People

(Reporter: RT, Assigned: crafuse)

References

Details

(Whiteboard: [metrics])

User Story

Acceptance criteria:
- Honor DNT signal for users who have turned on DNT themselves (Chrome, Firefox, IE7, IE8, IE9 and IE12 users) on Google Analytics and do not send event data (current implementation)
- For IE 10 & 11 users where DNT is turned on by default (no explicit user action to enable DNT), carry on collecting event data

Attachments

(1 file, 10 obsolete files)

3.94 KB, patch
standard8
: review+
Details | Diff | Splinter Review
No description provided.
User Story: (updated)
Rank: -61
Priority: -- → P1
Rank: -61 → 22
Gareth, is this something you can help us with or is that a Hello dev task?
Flags: needinfo?(garethcull.bugs)
Rank: 22 → 19
Priority: P1 → P2
Whiteboard: [metrics]
(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
Flags: needinfo?(garethcull.bugs)
Assignee: nobody → chris.rafuse
Points: --- → 2
Status: NEW → ASSIGNED
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.
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.
Attachment #8669957 - Attachment is obsolete: true
(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 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.
Attachment #8670037 - Flags: review?(standard8)
Attachment #8670037 - Flags: review?(dmose)
Attachment #8670037 - Flags: feedback?(adam)
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"
Attachment #8670037 - Flags: feedback?(adam) → feedback+
Attachment #8670037 - Attachment is obsolete: true
Attachment #8670037 - Flags: review?(standard8)
Attachment #8670037 - Flags: review?(dmose)
Moving optimizely script out of conditional test for DNT.
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.
Attachment #8670403 - Flags: review?(standard8)
Attachment #8670403 - Flags: review?(dmose)
Attachment #8670403 - Flags: feedback?(adam)
Comment on attachment 8670403 [details] [diff] [review]
Collect Google Analytics data for users on IE 10 & 11

Need to add review comment changes.
Attachment #8670403 - Attachment is obsolete: true
Attachment #8670403 - Flags: review?(standard8)
Attachment #8670403 - Flags: review?(dmose)
Attachment #8670403 - Flags: feedback?(adam)
(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 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.
Attachment #8670435 - Flags: review?(standard8)
Attachment #8670435 - Flags: review?(dmose)
Attachment #8670435 - Flags: feedback?(adam)
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!
Attachment #8670435 - Flags: feedback?(adam) → feedback+
Attachment #8670435 - Attachment is obsolete: true
Attachment #8670435 - Flags: review?(standard8)
Attachment #8670435 - Flags: review?(dmose)
Comment on attachment 8670538 [details] [diff] [review]
Collect Google Analytics data for users on IE 10 & 11

Squashed version includes first commit changes
Attachment #8670538 - Flags: review?(dmose)
Attachment #8670538 - Flags: review?(standard8)
Attachment #8670538 - Flags: review?(adam)
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) {
Attachment #8670538 - Flags: review?(standard8) → review-
Attachment #8670538 - Attachment is obsolete: true
Attachment #8670538 - Flags: review?(dmose)
Attachment #8670538 - Flags: review?(adam)
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.
Attachment #8670908 - Flags: review?(standard8)
Attachment #8670908 - Flags: review?(dmose)
Attachment #8670908 - Flags: review?(adam)
Blocks: 1212428
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") {
Attachment #8670908 - Flags: review?(standard8) → review+
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 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.
Attachment #8670908 - Flags: review?(adam) → review?(standard8)
Attachment #8670908 - Attachment is obsolete: true
Attachment #8670908 - Flags: review?(standard8)
Attachment #8670908 - Flags: review?(dmose)
Attachment #8671439 - Flags: review?(standard8)
Comment on attachment 8671439 [details] [diff] [review]
Collect Google Analytics data for users on IE 10 & 11

This version HAS the console logs
Comment on attachment 8671444 [details] [diff] [review]
Collect Google Analytics data for users on IE 10 & 11

This version does NOT have console logs.
Attachment #8671444 - Flags: review?(standard8)
Attachment #8671444 - Flags: review?(dmose)
Attachment #8671444 - Flags: review?(adam)
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
Attachment #8671444 - Flags: review?(dmose)
Attachment #8671444 - Flags: review?(adam)
Attachment #8671439 - Attachment is obsolete: true
Attachment #8671439 - Flags: review?(standard8)
Attachment #8671444 - Attachment is obsolete: true
Attachment #8671444 - Flags: review?(standard8)
Comment on attachment 8672046 [details] [diff] [review]
Collect Google Analytics data for users on IE 10 & 11

WITH console log output.
Attachment #8672046 - Flags: review?(standard8)
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.
Attachment #8672048 - Flags: review?(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.
Attachment #8672048 - Flags: review?(standard8) → review+
Attachment #8672046 - Flags: review?(standard8)
Comment on attachment 8673859 [details] [diff] [review]
Collect Google Analytics data for users on IE 10 & 11

Fixed indentation nit.
Attachment #8673859 - Flags: review?(standard8)
Attachment #8672048 - Attachment is obsolete: true
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.
Attachment #8673859 - Flags: review?(standard8) → review+
Attachment #8672046 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/2f685ede3d97
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Iteration: --- → 44.2 - Oct 19
You need to log in before you can comment on or make changes to this bug.