r107349 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r107348‎ | r107349 | r107350 >
Date:00:42, 27 December 2011
Author:krinkle
Status:resolved
Tags:
Comment:
[CentralNotice] code quality and clean top
* don't use deprecated globals, using mw.config.get instead.
-- My local wiki had $wgLegacyJavaScriptGlobals=false;, caused exceptions to be thrown in this script for wgSiteName etc.
* fix global variable leakage of 'targets' in function insertBanner(), now a local var
* removed quotes in keys where not needed and split object literals from few keys per line to one key per line
* moved setBannerHidingCookie() function statement to before it's first use
* strict comparisons
* moved double declared variables in inexistent "block scope" to function scope
* removed left-over call to Array.prototype.splice. The PHP output now (for a while, if not forever) returns a regular numeral array that json_encode() simply outputs as such, perfectly fine.
* moved locally declared variables in `for` loops to the top to emphasize that they are visible in the entire function. If you'd have two for-loops that both use "var i" in the first statement, it would be an infinite loop as inner for-loop would reset the outer loops' "i" variable on each iteration, since there only is one "i" variable.
Modified paths:
  • /trunk/extensions/CentralNotice/special/SpecialBannerController.php (modified) (history)

Diff [purge]

Index: trunk/extensions/CentralNotice/special/SpecialBannerController.php
——@@ -50,77 +50,88 @@
5151 ( function( $ ) {
5252 $.ajaxSetup({ cache: true });
5353 $.centralNotice = {
54 - 'data': {
55 - 'getVars': {},
56 - 'bannerType': 'default'
 54+ data: {
 55+ getVars: {},
 56+ bannerType: 'default'
5757 },
58 - 'fn': {
59 - 'loadBanner': function( bannerName, campaign, bannerType ) {
 58+ fn: {
 59+ loadBanner: function( bannerName, campaign, bannerType ) {
 60+ var bannerPageQuery, bannerPage, bannerScript;
 61+
6062 // Store the bannerType in case we need to set a banner hiding cookie later
6163 $.centralNotice.data.bannerType = bannerType;
6264 // Get the requested banner
63 - var bannerPageQuery = $.param( {
64 - 'banner': bannerName, 'campaign': campaign, 'userlang': wgUserLanguage,
65 - 'db': wgDBname, 'sitename': wgSiteName, 'country': Geo.country
 65+ bannerPageQuery = $.param( {
 66+ banner: bannerName,
 67+ campaign: campaign,
 68+ userlang: mw.config.get( 'wgUserLanguage' ),
 69+ db: mw.config.get( 'wgDBname' ),
 70+ sitename: mw.config.get( 'wgSiteName' ),
 71+ country: Geo.country
6672 } );
67 - var bannerPage = '?title=Special:BannerLoader&' + bannerPageQuery;
 73+ bannerPage = '?title=Special:BannerLoader&' + bannerPageQuery;
6874 JAVASCRIPT;
69 - $js .= "\n\t\t\t\tvar bannerScript = '<script type=\"text/javascript\" src=\"" .
 75+ $js .= "\n\t\t\t\tbannerScript = '<script type=\"text/javascript\" src=\"" .
7076 Xml::escapeJsString( $wgCentralPagePath ) .
7177 "' + bannerPage + '\"></script>';\n";
7278 $js .= <<<JAVASCRIPT
73 - if ( document.cookie.indexOf( 'centralnotice_'+bannerType+'=hide' ) == -1 ) {
 79+ if ( document.cookie.indexOf( 'centralnotice_' + bannerType + '=hide' ) === -1 ) {
7480 jQuery( '#siteNotice' ).prepend( '<div id="centralNotice" class="' +
7581 ( wgNoticeToggleState ? 'expanded' : 'collapsed' ) +
7682 ' cn-' + bannerType + '">'+bannerScript+'</div>' );
7783 }
7884 },
79 - 'loadBannerList': function( geoOverride ) {
 85+ loadBannerList: function( geoOverride ) {
 86+ var geoLocation, bannerListQuery, bannerListURL;
 87+
8088 if ( geoOverride ) {
81 - var geoLocation = geoOverride; // override the geo info
 89+ geoLocation = geoOverride; // override the geo info
8290 } else {
83 - var geoLocation = Geo.country; // pull the geo info
 91+ geoLocation = Geo.country; // pull the geo info
8492 }
85 - var bannerListQuery = $.param( { 'language': wgContentLanguage, 'project': wgNoticeProject, 'country': geoLocation } );
 93+ bannerListQuery = $.param( {
 94+ language: mw.config.get( 'wgContentLanguage' ),
 95+ project: mw.config.get( 'wgNoticeProject' ),
 96+ country: geoLocation
 97+ } );
8698 JAVASCRIPT;
87 - $js .= "\n\t\t\t\tvar bannerListURL = wgScript + '?title=' + encodeURIComponent('" .
 99+ $js .= "\n\t\t\t\tbannerListURL = mw.util.wikiScript() + '?title=' + encodeURIComponent('" .
88100 $wgContLang->specialPage( 'BannerListLoader' ) .
89101 "') + '&cache=/cn.js&' + bannerListQuery;\n";
90102 $js .= <<<JAVASCRIPT
91 - if ( wgNamespaceNumber !== -1 ) { // disable loading banners on Special pages
92 - var request = $.ajax( {
93 - url: bannerListURL,
94 - dataType: 'json',
95 - success: $.centralNotice.fn.chooseBanner
96 - } );
97 - }
 103+ $.ajax( {
 104+ url: bannerListURL,
 105+ dataType: 'json',
 106+ success: $.centralNotice.fn.chooseBanner
 107+ } );
98108 },
99 - 'chooseBanner': function( bannerList ) {
100 - // Convert the json object to a true array
101 - bannerList = Array.prototype.slice.call( bannerList );
 109+ chooseBanner: function( bannerList ) {
 110+ var groomedBannerList = [], i, j, pointer;
102111
103112 // Make sure there are some banners to choose from
104 - if ( bannerList.length == 0 ) return false;
 113+ if ( bannerList.length === 0 ) {
 114+ return false;
 115+ }
105116
106 - var groomedBannerList = [];
107 -
108 - for( var i = 0; i < bannerList.length; i++ ) {
 117+ for( i = 0; i < bannerList.length; i++ ) {
109118 // Only include this banner if it's intended for the current user
110 - if( ( wgUserName && bannerList[i].display_account ) ||
111 - ( !wgUserName && bannerList[i].display_anon == 1 ) )
 119+ if( ( !mw.user.anonymous() && bannerList[i].display_account === 1 ) ||
 120+ ( mw.user.anonymous() && bannerList[i].display_anon === 1 ) )
112121 {
113 - // add the banner to our list once per weight
114 - for( var j=0; j < bannerList[i].weight; j++ ) {
 122+ // Add the banner to our list once per weight
 123+ for( j = 0; j < bannerList[i].weight; j++ ) {
115124 groomedBannerList.push( bannerList[i] );
116125 }
117126 }
118127 }
119128
120129 // Return if there's nothing left after the grooming
121 - if( groomedBannerList.length == 0 ) return false;
 130+ if ( groomedBannerList.length === 0 ) {
 131+ return false;
 132+ }
122133
123134 // Choose a random key
124 - var pointer = Math.floor( Math.random() * groomedBannerList.length );
 135+ pointer = Math.floor( Math.random() * groomedBannerList.length );
125136
126137 // Load a random banner from our groomed list
127138 $.centralNotice.fn.loadBanner(
——@@ -129,27 +140,29 @@
130141 ( groomedBannerList[pointer].fundraising ? 'fundraising' : 'default' )
131142 );
132143 },
133 - 'getQueryStringVariables': function() {
134 - document.location.search.replace( /\??(?:([^=]+)=([^&]*)&?)/g, function () {
135 - function decode( s ) {
136 - return decodeURIComponent( s.split( "+" ).join( " " ) );
137 - }
138 - $.centralNotice.data.getVars[decode( arguments[1] )] = decode( arguments[2] );
 144+ getQueryStringVariables: function() {
 145+ function decode( s ) {
 146+ return decodeURIComponent( s.split( '+' ).join( ' ' ) );
 147+ }
 148+ document.location.search.replace( /\??(?:([^=]+)=([^&]*)&?)/g, function ( str, p1, p2 ) {
 149+ $.centralNotice.data.getVars[decode( p1 )] = decode( p2 );
139150 } );
140151 }
141152 }
142 - }
143 - jQuery( document ).ready( function ( $ ) {
 153+ };
 154+
 155+ $( document ).ready( function ( $ ) {
144156 // Initialize the query string vars
145157 $.centralNotice.fn.getQueryStringVariables();
146 - if( $.centralNotice.data.getVars['banner'] ) {
 158+ if( $.centralNotice.data.getVars.banner ) {
147159 // if we're forcing one banner
148 - $.centralNotice.fn.loadBanner( $.centralNotice.data.getVars['banner'] );
 160+ $.centralNotice.fn.loadBanner( $.centralNotice.data.getVars.banner );
149161 } else {
150162 // Look for banners ready to go NOW
151 - $.centralNotice.fn.loadBannerList( $.centralNotice.data.getVars['country'] );
 163+ $.centralNotice.fn.loadBannerList( $.centralNotice.data.getVars.country );
152164 }
153 - } ); //document ready
 165+ } );
 166+
154167 } )( jQuery );
155168 JAVASCRIPT;
156169 return $js;
——@@ -157,7 +170,7 @@
158171 }
159172
160173 function getToggleScripts() {
161 - $script = "var wgNoticeToggleState = (document.cookie.indexOf('hidesnmessage=1')==-1);\n\n";
 174+ $script = "var wgNoticeToggleState = document.cookie.indexOf( 'hidesnmessage=1' ) === -1;\n\n";
162175 return $script;
163176 }
164177
——@@ -165,38 +178,44 @@
166179 global $wgNoticeFundraisingUrl;
167180 $script = <<<JAVASCRIPT
168181 function insertBanner( bannerJson ) {
 182+ var url, targets;
 183+
169184 jQuery( 'div#centralNotice' ).prepend( bannerJson.bannerHtml );
170185 if ( bannerJson.autolink ) {
171186 JAVASCRIPT;
172 - $script .= "\n\t\tvar url = '" .
 187+ $script .= "\n\t\turl = '" .
173188 Xml::escapeJsString( $wgNoticeFundraisingUrl ) . "';\n";
174189 $script .= <<<JAVASCRIPT
175190 if ( ( bannerJson.landingPages !== null ) && bannerJson.landingPages.length ) {
176191 targets = String( bannerJson.landingPages ).split(',');
177192 url += "?" + jQuery.param( {
178 - 'landing_page': targets[Math.floor( Math.random() * targets.length )].replace( /^\s+|\s+$/, '' )
 193+ landing_page: targets[Math.floor( Math.random() * targets.length )].replace( /^\s+|\s+$/, '' )
179194 } );
180195 url += "&" + jQuery.param( {
181 - 'utm_medium': 'sitenotice', 'utm_campaign': bannerJson.campaign,
182 - 'utm_source': bannerJson.bannerName, 'language': wgUserLanguage,
183 - 'country': Geo.country
 196+ utm_medium: 'sitenotice',
 197+ utm_campaign: bannerJson.campaign,
 198+ utm_source: bannerJson.bannerName,
 199+ language: mw.config.get( 'wgUserLanguage' ),
 200+ country: Geo.country
184201 } );
185202 jQuery( '#cn-landingpage-link' ).attr( 'href', url );
186203 }
187204 }
188205 }
189 -function hideBanner() {
190 - jQuery( '#centralNotice' ).hide(); // Hide current banner
191 - var bannerType = $.centralNotice.data.bannerType;
192 - if ( bannerType === undefined ) bannerType = 'default';
193 - setBannerHidingCookie( bannerType ); // Hide future banners of the same type
194 -}
195206 function setBannerHidingCookie( bannerType ) {
196207 var e = new Date();
197208 e.setTime( e.getTime() + (14*24*60*60*1000) ); // two weeks
198 - var work='centralnotice_'+bannerType+'=hide; expires=' + e.toGMTString() + '; path=/';
 209+ var work = 'centralnotice_' + bannerType + '=hide; expires=' + e.toGMTString() + '; path=/';
199210 document.cookie = work;
200211 }
 212+function hideBanner() {
 213+ jQuery( '#centralNotice' ).hide(); // Hide current banner
 214+ var bannerType = jQuery.centralNotice.data.bannerType;
 215+ if ( bannerType === undefined ) {
 216+ bannerType = 'default';
 217+ }
 218+ setBannerHidingCookie( bannerType ); // Hide future banners of the same type
 219+}
201220 // This function is deprecated
202221 function toggleNotice() {
203222 hideBanner();

Follow-up revisions

RevisionCommit summaryAuthorDate
r107357[CentralNotice] Re-apply r107315, accidentally reverted in r107349...krinkle02:30, 27 December 2011

Status & tagging log