Reviewer, also quick note on the piwik.js file being modified. It was assumed that the minified version of the file would be generated as part of build process, and hence explicitly not modified.

Please let me know if anything else needs to be done along with this JS file.

The change looks ok to me, if we can set a cookie properly, that should be enough. To be safe though, we could surround the test in a try-catch. What do you think @tsteur ?

Sounds good @diosmosis
does it make sense to simply return '0' on any error as we probably can't rely on navigatorAlias.cookieEnabled in this case?

Yes, makes sense, if something bad happens when we try and set a cookie, we can't set a cookie.

Thanks @diosmosis and @tsteur for your feedback. Is there any action item from my side or will you be able to help merge this change?

There is nothing that needs to be done. We will include this in the 3.11 release. There's not enough testing time for 3.10 release unfotunately.

Actually, I couldn't reproduce the warning @siva538

FYI: I always got "Access denied" when using showModalDialog until I realised you need to turn off the popup blocker in IE10/11 for it to test.

In general I was worried removing cookieEnabled check that hasCookies() might always return true cause in the end we're simply writing a property on document.cookie=... and reading the same property within the same request. I only tested latest Chrome and Firefox and IE11 but seems like browsers actually won't store any value on document.cookie when cookies are disabled. So it looks like merging the PR will be fine and the browser doesn't start treating this property like any regular property.

Couldn't find a blame why this check was added initially to see if it causes any problem maybe c654e73#diff-1279d666063b65e6d6777f902d11574f

@mattab not sure if there could be any regression or problem with it but suppose it should be fine.

At the same time popups are blocked by default in IE and not sure where this method showModalDialog actually works.

Assuming all browsers hopefully behave the same it should be fine to merge.

Not sure if you're still having problems reproducing or not, but the file below should help. You can open it in IE11 straight from your desktop, just hit "Allow Blocked Content" at the bottom if you get prompted.

testmodalpopup.zip

To re-iterate, this is an important fix that is causing lot of problems with the manual code modifications to piwik.js file and yet times it is getting overwritten and resulting in production critical issues.

Thanks for your help and understanding. Please let us know if you can merge this for 3.12.

@tsteur Reckon we can just merge it. In the worst case it would regress the accuracy of tracking how many users support Cookies. I'll set a reminder to check the reports in 2 months.

Sweet, this PR is good to merge. We just need to update the minified tracker version afterwards.

Just merged, thanks @siva538 for the great first contribution!