Changed JavaScript Tracking Code generation to use "//" #344
Conversation
…erifying whether the site is using "https", or not, and generating a corresponding URL. "//" has been part of the RFC for well over 18 years. It has been confirmed to work in all major browsers (Firefox, Chrome, IE, Opera). References: - RFC 1808 Section 4: http://www.ietf.org/rfc/rfc1808.txt - RFC 3986 Section 5.2: http://www.ietf.org/rfc/rfc3986.txt - Stack Overflow QA 1: http://stackoverflow.com/q/4659345/238722 - Stack Overflow QA 2: http://stackoverflow.com/q/6785442/238722
|
Thanks for the pull request! I see from Stackoverflow answers that It works on major browsers, but what about non major browsers? For example, it "seems" important that Piwik JS tracker also tracks IE6 browsers... is it the case that this will work on IE6? or do you think we should not care whether Piwik tracker works on such older browsers? Maybe you can use eg. http://browsershots.org/ to check which browser supports this, and then we can decide together publicly here whether it's acceptable to drop tracking for these older browsers? |
|
Hi Matt, It actually works on IE6 - I obtained an IE6 VM from http://modern.ie/, and performed the test with the following file: http://www.orangecoding.com/double_slash.htm The IE6 results can be found here: Now, as for the rest of it - I need some help figuring out how to fix the issues that Travis is reporting. Would you be able to provide some pointers? Thanks, |
|
Take for example this failing job: https://travis-ci.org/piwik/piwik/jobs/29180839 what's happening is this: there is an API that writes out the javascript tracker code. During the integration tests, all APIs are called, and the output as XML is stored and compared to expected output. When changing the JS tracking code such as this PR, the output of this API method changes. So it fails the test. To fix this problem you can modify the file: The other test failure is a unit test. You can modify the expected value in the test file here: https://github.com/piwik/piwik/blob/master/tests/PHPUnit/Integration/Core/PiwikTest.php#L55-55 You can use travis to run the tests, or if you have some time to play, you can also setup PHPUnit on your local machine so you can run any Piwik test locally. See information in: https://github.com/piwik/piwik/blob/master/tests/README.md#how-to-run-piwik-tests |
|
Matt ( @mattab ), Thanks for your help - I've got all of the tests passing now. I also went ahead and did a browsershots with all of the lowest (oldest) versions available of all browsers on each OS available. The results are available here: http://browsershots.org/http://www.orangecoding.com/double_slash.htm Successful:
Failures:
I would vouch for the ones that failed to be accepted as unsupported. I have not confirmed whether the issue is browsershots integration, or the double-slash method. Failed browsers development activity:
Hope this helps make a decision whether to accept the double-slash change or not. Regards, |
|
I added this to 2.6.0 milestone, so I'm planning to merge the PR. If anyone has any feedback now is a good time to send it :) |
|
@mattab The only thing I would like noted related to this PR is that this PR modifies the JavaScript code specifically. Stefan Giehl's comment mentions that this PR includes the fallback image, but it actually doesn't. I didn't think of it at the time =. I will go back and make the same protocol-agnostic changes for the fallback image. Would you rather wait for those in this commit, or leave them to be a separate PR? I would personally suggest a separate PR, so that this one can get into production sooner, rather than later. |
|
Thanks for heads up.
I won't merge until Sept 5th at earliest, so if you do the image fallback change before, it's best to put it in the same PR, otherwise in a different one is fine. cheers |
|
As far I can remember, Lynx and Links2 are shell console browser that doesn't parse javascript. |
|
@tassoman You were absolutely correct! The failed browsers above are due to JavaScript not being available. I have submitted the page to browsershots.org once again, this time including the failed browsers only, and all passed with the correct message!
This means that all browsers are working properly with the change. |
Ok that's a really good news, so this change is 100% awesome. |
|
@mattab I believe I have successfully changed all places where the fallback image is generated. All tests are passing. I think we're good to go on this enhancement. |
Changed JavaScript Tracking Code generation to use "//"
|
Thanks @Irrational86 for the great Pull request. I've merged it as it's nice to move piece by piece in the future of the web. Cheers! |
…tion #344 adding optout iframe screenshot
…h should be used in the generated tracking code refs #344
Changed JavaScript Tracking Code generation to use "//", instead of verifying whether the site is using "https", or not, and generating a corresponding URL.
"//" has been part of the RFC for well over 18 years. It has been confirmed to work in all major browsers (Firefox, Chrome, IE, Opera).
References: