Last modified: 2011-04-07 09:14:24 UTC

Wikimedia Bugzilla is closed!

Wikimedia migrated from Bugzilla to Phabricator. Bug reports are handled in Wikimedia Phabricator.
This static website is read-only and for historical purposes. It is not possible to log in and except for displaying bug reports and their history, links might be broken. See T29644, the corresponding Phabricator task for complete and up-to-date bug report information.
Bug 27644 - Finish ResourceLoader browser blacklist
Finish ResourceLoader browser blacklist
Status: RESOLVED WONTFIX
Product: MediaWiki
Classification: Unclassified
ResourceLoader (Other open bugs)
unspecified
All All
: Normal major (vote)
: ---
Assigned To: Trevor Parscal
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-02-22 21:26 UTC by Roan Kattouw
Modified: 2011-04-07 09:14 UTC (History)
5 users (show)

See Also:
Web browser: ---
Mobile Platform: ---
Assignee Huggle Beta Tester: ---


Attachments
Bug fixed? (970 bytes, patch)
2011-02-23 00:16 UTC, Sam Reed (reedy)
Details

Description Roan Kattouw 2011-02-22 21:26:55 UTC
Currently it's only IE < 6 and a bunch of TODO comments.

In resources/startup.js
Comment 1 Sam Reed (reedy) 2011-02-23 00:16:19 UTC
Created attachment 8197 [details]
Bug fixed?

Moment of boredom...

Bare in mind, I don't usually do Javascript ;)

Seems to work roughly in the console...
Comment 2 Krinkle 2011-02-23 07:57:11 UTC
(In reply to comment #1)
> Created attachment 8197 [details]
> Bug fixed?
> 
> Moment of boredom...
> 
> Bare in mind, I don't usually do Javascript ;)
> 
> Seems to work roughly in the console...

I'm fairly sure the same kind of check won't work for all of them. It works for IE6 but that's probably the exception:

IE6:
< navigator.appVersion
> "4.0 (compatible; MSIE 6.0; Windows NT 5.1; SV1)"

< navigator.appVersion.indexOf( 'MSIE' )
> 17

< navigator.appVersion.split( 'MSIE' )
> ["4.0 (compatible; ", " 6.0; Windows NT 5.1; SV1)"]

< navigator.appVersion.split( 'MSIE' )[1]
> " 6.0; Windows NT 5.1; SV1)"

< parseFloat(navigator.appVersion.split( 'MSIE' )[1])
> 6

But that won't work for Firefox which for example has it's version at the end of the useragent string seperated by a slash

ie.:
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.7) Gecko/20070918 Firefox/2
or:
< navigator.appVersion
> "5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.1.13) Gecko/20100914 Firefox/3.5.13"
< navigator.appVersion.split('Firefox')
> ["5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.1.13) Gecko/20100914 ", "/3.5.13"]
< navigator.appVersion.split('Firefox')[1]
> /3.5.13
< parseFloat(navigator.appVersion.split('Firefox')[1])
> NaN
Comment 3 Krinkle 2011-02-23 08:04:43 UTC
We'll need different manual checks for different browsers.

A few more examples of the complete navigator.userAgent (navigator.appVersion is a substring of that).

!!! DON'T RELY ON THESE! they're not from the older versions which we need to test for

* Opera 10.62 Windows (notice how the release version is at the end. Not 9.80)
	"Opera/9.80 (Windows NT 6.1; U; en) Presto/2.6.30 Version/10.62"

* (Mobile) Safari 3.2.2 (iPad)
	"Mozilla/5.0 (iPad; U; CPU OS 3_2_2 like Mac OS X; en-us) AppleWebKit/531.21.10 (KHTML, like Gecko) Version/4.0.4 Mobile/7B500 Safari/531.21.10"

* Safari 5.0.3 (Windows) (notice how the release version is not with the application name;  "5.0.3")
	"Mozilla/5.0 (Windows; U; Windows NT 6.0; en-us) AppleWebKit/533.19.4 (KHTML, like Gecko) Version/5.0.3 Safari/533.19.4"
Comment 4 Sam Reed (reedy) 2011-02-23 08:15:23 UTC
(In reply to comment #2)
> I'm fairly sure the same kind of check won't work for all of them. It works for
> IE6 but that's probably the exception:
> 

Why am I not suprised?
Comment 5 Sam Reed (reedy) 2011-02-23 08:28:06 UTC
navigator.appVersion
"5.0 (X11; U; Linux x86_64; en-US) AppleWebKit/534.20 (KHTML, like Gecko) Chrome/11.0.672.2 Safari/534.20"
navigator.appVersion.split('Chrome')
["5.0 (X11; U; Linux x86_64; en-US) AppleWebKit/534.20 (KHTML, like Gecko) ", "/11.0.672.2 Safari/534.20"]
navigator.appVersion.split('Chrome')[1]
"/11.0.672.2 Safari/534.20"

I guess that possibly means Safari gets a false positive on Chrome, though the "version number" is much higher


What also wouldn't suprise me, if there were some browsers that have changed their useragent formatting over the years...

navigator.appVersion.split('Chrome/')[1]
"11.0.672.2 Safari/534.20"
parseFloat(navigator.appVersion.split('Chrome/')[1])
11


That works...

So, that's 3 we can do with the same Format - FF, Chrome, IE

N-2 solutions is better than N solutions (where N is the number of browsers)

Possibly Opera and Safari as one (bar iPad)
Comment 6 Sam Reed (reedy) 2011-02-23 08:30:00 UTC
+	return !( browserVersionCheck( 'MSIE', 6 )
+		|| browserVersionCheck( 'Chrome/', 1 )
+		|| browserVersionCheck( 'Firefox/', 2 )
+		|| browserVersionCheck( 'Safari', 3 )
+		|| browserVersionCheck( 'Opera', 9 )
+	);

Should work then....
Comment 7 Krinkle 2011-02-23 11:20:45 UTC
(In reply to comment #6)
> +    return !( browserVersionCheck( 'MSIE', 6 )
> +        || browserVersionCheck( 'Chrome/', 1 )
> +        || browserVersionCheck( 'Firefox/', 2 )
> +        || browserVersionCheck( 'Safari', 3 )
> +        || browserVersionCheck( 'Opera', 9 )
> +    );
> 
> Should work then....

No because Opera also uses a slash, and, moreover, the release version in opera browsers is after "Version/" not after "Opera/" (atleast in the current Opera).

-

Same thing with Safari " (... ) Version/5.0.3 Safari/533.19.4 (..) "

The Safari release version is 5.0.3, "533.19.4" is an internal code (presumably a build number from Apple and/or of the WebKit engine specifically)

-

"Firefox/" would work I guess (including the slash in the split)

-

Chrome is tricky indeed. Apple released Safari but they're also behind WebKit (although WebKit is open source, it's still backed by Apple in some ways) - so the "Safari"-string gets in the face from time to time when using non-Safari WebKit browsers.

See also the source of http://svn.wikimedia.org/viewvc/mediawiki/trunk/phase3/resources/jquery/jquery.client.js?view=markup

and look at:
http://ajax.googleapis.com/ajax/libs/jquery/1.5.0/jquery.js
It has some interesting stuff. Seperated over three sections (ctrl-f find them):
// Useragent RegExp
and
uaMatch: function( ua ) { 
and
browserMatch.browser
Comment 8 Sam Reed (reedy) 2011-02-23 11:22:58 UTC
I was meaning for the first 3. I submitted it, and then realised i hadn't commented to say that.

Checking for Chrome before Safari in the list helps reduce Safari false positives
Comment 9 Sam Reed (reedy) 2011-02-23 11:29:19 UTC
One would almost wonder, why jQuery itself doesn't have a simple javascript function to do exactly that

doesBrowserSupportjQuery()
Comment 10 Mark A. Hershberger 2011-02-23 21:49:13 UTC
Something like http://api.jquery.com/jQuery.support/ perhaps?
Comment 11 Krinkle 2011-02-23 21:52:57 UTC
(In reply to comment #10)
> Something like http://api.jquery.com/jQuery.support/ perhaps?

No, not really. For one, we can't use jQuery since we're testing to detect browsers that don't support jQuery, jQuery itself isn't loaded yet. The startUp module is the very first thing that's executed.

Secondly, although we could use parts of jQuery's source code, jQuery.support is an object to help with feature detection, not browser detection.

(In reply to comment #9)
> One would almost wonder, why jQuery itself doesn't have a simple javascript
> function to do exactly that
> 
> doesBrowserSupportjQuery()

Good point, I think I read an article from Paul Irish (jQuery developer) on this a few weeks ago. I'll ask him about it.
Comment 12 Sam Reed (reedy) 2011-02-23 21:56:07 UTC
Seems nicer than reinventing the wheel seemingly unnecessarily
Comment 13 Krinkle 2011-04-06 10:41:08 UTC
Okay, after some chat on IRC I'm going to mark this WONTFIX.

IE6 isn't an issue since we actually support that (and so does jQuery, for now).

The numbers in the comparison may be confusing but it's about "less than", so what were planning to blacklist is:
* IE 5.5 and below
* Firefox 1.5 and below
* Safari 2 and below
* Opera 8 and below

According to http://stats.wikimedia.org/wikimedia/squids/SquidReportClients.htm those are way to small to actually spend time on in core.

jQuery was loaded on all pages in 1.16wmf fine without any known bugs or complaints, and unless needed we're not starting.
Comment 14 Trevor Parscal 2011-04-06 15:53:16 UTC
This might be a good place to think about mobile redirects too, if we are indeed moving it to the top especially.
Comment 15 Roan Kattouw 2011-04-06 15:56:11 UTC
(In reply to comment #14)
> This might be a good place to think about mobile redirects too, if we are
> indeed moving it to the top especially.
Good point. Once startup lives at the top, the mobile redirect should totally be in there.

However, the mobile redirect script currently uses a few MW-exported JS vars (wgPagename etc.), we'd have to export those early. Not too hard if we write a special module for it.
Comment 16 MZMcBride 2011-04-06 23:32:23 UTC
(In reply to comment #15)
> Good point. Once startup lives at the top, the mobile redirect should totally
> be in there.
> 
> However, the mobile redirect script currently uses a few MW-exported JS vars
> (wgPagename etc.), we'd have to export those early. Not too hard if we write a
> special module for it.

Gah. Can't we just kill the JS redirect altogether? I thought it was a temporary stop-gap measure before server-side support was implemented. I think any efforts should be focused on killing it altogether, not trying to find ways to make it work with ResourceLoader.
Comment 17 Roan Kattouw 2011-04-07 09:14:24 UTC
(In reply to comment #16)
> Gah. Can't we just kill the JS redirect altogether? I thought it was a
> temporary stop-gap measure before server-side support was implemented. I think
> any efforts should be focused on killing it altogether, not trying to find ways
> to make it work with ResourceLoader.
That's all very true, but the fact of the matter is that the group of people with the skills to implement the mobile redirect server side (which, AFAIK, is blocking on migrating from Squid to Varnish) is completely separate from the group of people with the skills to make the JS redirect suck less. The JS redirect is a stopgap measure, but that doesn't mean it's not warranted for a JS dev to spend an hour making it better while the ops folks figure out how to do this server-side.

Note You need to log in before you can comment on or make changes to this bug.


Navigation
Links