r92855 MediaWiki - Code Review archive

Repository:	MediaWiki
Revision:	< r92854‎ \| r92855 \| r92856 >
Date:	10:45, 22 July 2011
Author:	ialex
Status:	resolved (Comments)
Tags:
Comment:	Reduce a bit the coupling between Skin and OutputPage: * Removed Skin::setupUserCss() and merged it in OutputPage::buildCssLinks() and OutputPage::buildCssLinksArray(), also removed its call from ApiParse.php * Moved Skin::userCanPreview() to OutputPage since two of the three calls to that function are made from there and the last one in SkinTemplate.php is in a somewhat deprecated function * Removed the Skin parameter from OutputPage::buildCssLinks(), OutputPage::getHeadScripts(), OutputPage::getBottomScripts() and OutputPage::makeResourceLoaderLink() since we now have a context Also made ApiParse.php call createContext() instead of create a new RequestContext manually and set its Title so so that it does not rely on $wgRequest and $wgTitle to get that member objects
Modified paths:	/trunk/phase3/includes/OutputPage.php (modified) (history) /trunk/phase3/includes/Skin.php (modified) (history) /trunk/phase3/includes/SkinTemplate.php (modified) (history) /trunk/phase3/includes/api/ApiParse.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/OutputPage.php
—	—	@@ -2269,7 +2269,6 @@
2270	2270	if ( $sk->commonPrintStylesheet() ) {
2271	2271	$this->addModuleStyles( 'mediawiki.legacy.wikiprintable' );
2272	2272	}
2273		~~- $sk->setupUserCss( $this );~~
2274	2273
2275	2274	$ret = Html::htmlHeader( array( 'lang' => $wgLang->getCode(), 'dir' => $userdir ) );
2276	2275
—	—	@@ -2286,9 +2285,9 @@
2287	2286	$ret .= Html::element( 'title', null, $this->getHTMLTitle() ) . "\n";
2288	2287
2289	2288	$ret .= implode( "\n", array(
2290		~~- $this->getHeadLinks( $sk, true ),~~
2291		~~- $this->buildCssLinks( $sk ),~~
2292		~~- $this->getHeadScripts( $sk ),~~
	2289	+ $this->getHeadLinks( null, true ),
	2290	+ $this->buildCssLinks(),
	2291	+ $this->getHeadScripts(),
2293	2292	$this->getHeadItems()
2294	2293	) );
2295	2294
—	—	@@ -2397,13 +2396,12 @@
2398	2397
2399	2398	/**
2400	2399	* TODO: Document
2401		~~- * @param $skin Skin~~
2402	2400	* @param $modules Array/string with the module name
2403	2401	* @param $only String ResourceLoaderModule TYPE_ class constant
2404	2402	* @param $useESI boolean
2405	2403	* @return string html <script> and <style> tags
2406	2404	*/
2407		~~- protected function makeResourceLoaderLink( Skin $skin, $modules, $only, $useESI = false ) {~~
	2405	+ protected function makeResourceLoaderLink( $modules, $only, $useESI = false ) {
2408	2406	global $wgLoadScript, $wgResourceLoaderUseESI,
2409	2407	$wgResourceLoaderInlinePrivateModules;
2410	2408	// Lazy-load ResourceLoader
—	—	@@ -2411,7 +2409,7 @@
2412	2410	$baseQuery = array(
2413	2411	'lang' => $this->getContext()->getLang()->getCode(),
2414	2412	'debug' => ResourceLoader::inDebugMode() ? 'true' : 'false',
2415		~~- 'skin' => $skin->getSkinName(),~~
	2413	+ 'skin' => $this->getSkin()->getSkinName(),
2416	2414	'only' => $only,
2417	2415	);
2418	2416	// Propagate printable and handheld parameters if present
—	—	@@ -2436,7 +2434,7 @@
2437	2435	// Recursively call us for every item
2438	2436	$links = '';
2439	2437	foreach ( $modules as $name ) {
2440		~~- $links .= $this->makeResourceLoaderLink( $skin, $name, $only, $useESI );~~
	2438	+ $links .= $this->makeResourceLoaderLink( $name, $only, $useESI );
2441	2439	}
2442	2440	return $links;
2443	2441	}
—	—	@@ -2501,6 +2499,7 @@
2502	2500	)
2503	2501	);
2504	2502	}
	2503	+ $links .= "\n";
2505	2504	continue;
2506	2505	}
2507	2506	// Special handling for the user group; because users might change their stuff
—	—	@@ -2553,12 +2552,11 @@
2554	2553	* JS stuff to put in the <head>. This is the startup module, config
2555	2554	* vars and modules marked with position 'top'
2556	2555	*
2557		~~- * @param $sk Skin object to use~~
2558	2556	* @return String: HTML fragment
2559	2557	*/
2560		~~- function getHeadScripts( Skin $sk ) {~~
	2558	+ function getHeadScripts() {
2561	2559	// Startup - this will immediately load jquery and mediawiki modules
2562		~~- $scripts = $this->makeResourceLoaderLink( $sk, 'startup', ResourceLoaderModule::TYPE_SCRIPTS, true );~~
	2560	+ $scripts = $this->makeResourceLoaderLink( 'startup', ResourceLoaderModule::TYPE_SCRIPTS, true );
2563	2561
2564	2562	// Load config before anything else
2565	2563	$scripts .= Html::inlineScript(
—	—	@@ -2569,8 +2567,8 @@
2570	2568
2571	2569	// Script and Messages "only" requests marked for top inclusion
2572	2570	// Messages should go first
2573		~~- $scripts .= $this->makeResourceLoaderLink( $sk, $this->getModuleMessages( true, 'top' ), ResourceLoaderModule::TYPE_MESSAGES );~~
2574		~~- $scripts .= $this->makeResourceLoaderLink( $sk, $this->getModuleScripts( true, 'top' ), ResourceLoaderModule::TYPE_SCRIPTS );~~
	2571	+ $scripts .= $this->makeResourceLoaderLink( $this->getModuleMessages( true, 'top' ), ResourceLoaderModule::TYPE_MESSAGES );
	2572	+ $scripts .= $this->makeResourceLoaderLink( $this->getModuleScripts( true, 'top' ), ResourceLoaderModule::TYPE_SCRIPTS );
2575	2573
2576	2574	// Modules requests - let the client calculate dependencies and batch requests as it likes
2577	2575	// Only load modules that have marked themselves for loading at the top
—	—	@@ -2590,17 +2588,15 @@
2591	2589	* JS stuff to put at the bottom of the <body>: modules marked with position 'bottom',
2592	2590	* legacy scripts ($this->mScripts), user preferences, site JS and user JS
2593	2591	*
2594		~~- * @param $sk Skin~~
2595		- *
2596	2592	* @return string
2597	2593	*/
2598		~~- function getBottomScripts( Skin $sk ) {~~
	2594	+ function getBottomScripts() {
2599	2595	global $wgUseSiteJs, $wgAllowUserJs;
2600	2596
2601	2597	// Script and Messages "only" requests marked for bottom inclusion
2602	2598	// Messages should go first
2603		~~- $scripts = $this->makeResourceLoaderLink( $sk, $this->getModuleMessages( true, 'bottom' ), ResourceLoaderModule::TYPE_MESSAGES );~~
2604		~~- $scripts .= $this->makeResourceLoaderLink( $sk, $this->getModuleScripts( true, 'bottom' ), ResourceLoaderModule::TYPE_SCRIPTS );~~
	2599	+ $scripts = $this->makeResourceLoaderLink( $this->getModuleMessages( true, 'bottom' ), ResourceLoaderModule::TYPE_MESSAGES );
	2600	+ $scripts .= $this->makeResourceLoaderLink( $this->getModuleScripts( true, 'bottom' ), ResourceLoaderModule::TYPE_SCRIPTS );
2605	2601
2606	2602	// Modules requests - let the client calculate dependencies and batch requests as it likes
2607	2603	// Only load modules that have marked themselves for loading at the bottom
—	—	@@ -2620,7 +2616,7 @@
2621	2617
2622	2618	// Add site JS if enabled
2623	2619	if ( $wgUseSiteJs ) {
2624		~~- $scripts .= $this->makeResourceLoaderLink( $sk, 'site', ResourceLoaderModule::TYPE_SCRIPTS );~~
	2620	+ $scripts .= $this->makeResourceLoaderLink( 'site', ResourceLoaderModule::TYPE_SCRIPTS );
2625	2621	if( $this->getUser()->isLoggedIn() ){
2626	2622	$userScripts[] = 'user.groups';
2627	2623	}
—	—	@@ -2628,7 +2624,7 @@
2629	2625
2630	2626	// Add user JS if enabled
2631	2627	if ( $wgAllowUserJs && $this->getUser()->isLoggedIn() ) {
2632		~~- if( $this->getTitle() && $this->getTitle()->isJsSubpage() && $sk->userCanPreview() ) {~~
	2628	+ if( $this->getTitle() && $this->getTitle()->isJsSubpage() && $this->userCanPreview() ) {
2633	2629	# XXX: additional security check/prompt?
2634	2630	$scripts .= Html::inlineScript( "\n" . $this->getRequest()->getText( 'wpTextbox1' ) . "\n" ) . "\n";
2635	2631	} else {
—	—	@@ -2637,7 +2633,7 @@
2638	2634	$userScripts[] = 'user';
2639	2635	}
2640	2636	}
2641		~~- $scripts .= $this->makeResourceLoaderLink( $sk, $userScripts, ResourceLoaderModule::TYPE_SCRIPTS );~~
	2637	+ $scripts .= $this->makeResourceLoaderLink( $userScripts, ResourceLoaderModule::TYPE_SCRIPTS );
2642	2638
2643	2639	return $scripts;
2644	2640	}
—	—	@@ -2697,12 +2693,36 @@
2698	2694	}
2699	2695
2700	2696	/**
2701		~~- * @param $sk Skin~~
	2697	+ * To make it harder for someone to slip a user a fake
	2698	+ * user-JavaScript or user-CSS preview, a random token
	2699	+ * is associated with the login session. If it's not
	2700	+ * passed back with the preview request, we won't render
	2701	+ * the code.
	2702	+ *
	2703	+ * @return bool
	2704	+ */
	2705	+ public function userCanPreview() {
	2706	+ if ( $this->getRequest()->getVal( 'action' ) != 'submit'
	2707	+ \|\| !$this->getRequest()->wasPosted()
	2708	+ \|\| !$this->getUser()->matchEditToken(
	2709	+ $this->getRequest()->getVal( 'wpEditToken' ) )
	2710	+ ) {
	2711	+ return false;
	2712	+ }
	2713	+ if ( !$this->getTitle()->isJsSubpage() && !$this->getTitle()->isCssSubpage() ) {
	2714	+ return false;
	2715	+ }
	2716	+
	2717	+ return !count( $this->getTitle()->getUserPermissionsErrors( 'edit', $this->getUser() ) );
	2718	+ }
	2719	+
	2720	+ /**
	2721	+ * @param $unused Unused
2702	2722	* @param $addContentType bool
2703	2723	*
2704	2724	* @return string HTML tag links to be put in the header.
2705	2725	*/
2706		~~- public function getHeadLinks( Skin $sk, $addContentType = false ) {~~
	2726	+ public function getHeadLinks( $unused = null, $addContentType = false ) {
2707	2727	global $wgUniversalEditButton, $wgFavicon, $wgAppleTouchIcon, $wgEnableAPI,
2708	2728	$wgSitename, $wgVersion, $wgHtml5, $wgMimeType,
2709	2729	$wgFeed, $wgOverrideSiteFeed, $wgAdvertisedFeedTypes,
—	—	@@ -2974,17 +2994,46 @@
2975	2995	/**
2976	2996	* Build a set of <link>s for the stylesheets specified in the $this->styles array.
2977	2997	* These will be applied to various media & IE conditionals.
2978		~~- * @param $sk Skin object~~
2979	2998	*
2980	2999	* @return string
2981	3000	*/
2982		~~- public function buildCssLinks( $sk ) {~~
2983		~~- $ret = '';~~
	3001	+ public function buildCssLinks() {
	3002	+ global $wgUseSiteCss, $wgAllowUserCss, $wgAllowUserCssPrefs;
	3003	+
	3004	+ $this->getSkin()->setupSkinUserCss( $this );
	3005	+
2984	3006	// Add ResourceLoader styles
2985	3007	// Split the styles into four groups
2986	3008	$styles = array( 'other' => array(), 'user' => array(), 'site' => array(), 'private' => array(), 'noscript' => array() );
2987	3009	$resourceLoader = $this->getResourceLoader();
2988		~~- foreach ( $this->getModuleStyles() as $name ) {~~
	3010	+
	3011	+ $moduleStyles = $this->getModuleStyles();
	3012	+
	3013	+ // Per-site custom styles
	3014	+ if ( $wgUseSiteCss ) {
	3015	+ $moduleStyles[] = 'site';
	3016	+ $moduleStyles[] = 'noscript';
	3017	+ if( $this->getUser()->isLoggedIn() ){
	3018	+ $moduleStyles[] = 'user.groups';
	3019	+ }
	3020	+ }
	3021	+
	3022	+ // Per-user custom styles
	3023	+ if ( $wgAllowUserCss ) {
	3024	+ if ( $this->getTitle()->isCssSubpage() && $this->userCanPreview() ) {
	3025	+ // @todo FIXME: Properly escape the cdata!
	3026	+ $out->addInlineStyle( $this->getRequest()->getText( 'wpTextbox1' ) );
	3027	+ } else {
	3028	+ $moduleStyles[] = 'user';
	3029	+ }
	3030	+ }
	3031	+
	3032	+ // Per-user preference styles
	3033	+ if ( $wgAllowUserCssPrefs ) {
	3034	+ $moduleStyles[] = 'user.options';
	3035	+ }
	3036	+
	3037	+ foreach ( $moduleStyles as $name ) {
2989	3038	$group = $resourceLoader->getModule( $name )->getGroup();
2990	3039	// Modules in groups named "other" or anything different than "user", "site" or "private"
2991	3040	// will be placed in the "other" group
—	—	@@ -2995,7 +3044,7 @@
2996	3045	// dynamically added styles to override statically added styles from other modules. So the order
2997	3046	// has to be other, dynamic, site, private, user
2998	3047	// Add statically added styles for other modules
2999		~~- $ret .= $this->makeResourceLoaderLink( $sk, $styles['other'], ResourceLoaderModule::TYPE_STYLES );~~
	3048	+ $ret = $this->makeResourceLoaderLink( $styles['other'], ResourceLoaderModule::TYPE_STYLES );
3000	3049	// Add normal styles added through addStyle()/addInlineStyle() here
3001	3050	$ret .= implode( "\n", $this->buildCssLinksArray() ) . $this->mInlineStyles;
3002	3051	// Add marker tag to mark the place where the client-side loader should inject dynamic styles
—	—	@@ -3006,7 +3055,7 @@
3007	3056	// 'private' at present only contains user.options, so put that before 'user'
3008	3057	// Any future private modules will likely have a similar user-specific character
3009	3058	foreach ( array( 'site', 'noscript', 'private', 'user' ) as $group ) {
3010		~~- $ret .= $this->makeResourceLoaderLink( $sk, $styles[$group],~~
	3059	+ $ret .= $this->makeResourceLoaderLink( $styles[$group],
3011	3060	ResourceLoaderModule::TYPE_STYLES
3012	3061	);
3013	3062	}
—	—	@@ -3015,6 +3064,13 @@
3016	3065
3017	3066	public function buildCssLinksArray() {
3018	3067	$links = array();
	3068	+
	3069	+ // Add any extension CSS
	3070	+ foreach ( $this->mExtStyles as $url ) {
	3071	+ $this->addStyle( $url );
	3072	+ }
	3073	+ $this->mExtStyles = array();
	3074	+
3019	3075	foreach( $this->styles as $file => $options ) {
3020	3076	$link = $this->styleLink( $file, $options );
3021	3077	if( $link ) {
Index: trunk/phase3/includes/api/ApiParse.php
—	—	@@ -253,16 +253,16 @@
254	254	}
255	255
256	256	if ( isset( $prop['headitems'] ) \|\| isset( $prop['headhtml'] ) ) {
257		~~- $context = new RequestContext;~~
	257	+ $context = $this->createContext();
	258	+ $context->setTitle( $titleObj );
258	259	$context->getOutput()->addParserOutputNoText( $p_result );
259	260
260	261	if ( isset( $prop['headitems'] ) ) {
261	262	$headItems = $this->formatHeadItems( $p_result->getHeadItems() );
262	263
263		~~- $context->getSkin()->setupUserCss( $context->getOutput() );~~
264	264	$css = $this->formatCss( $context->getOutput()->buildCssLinksArray() );
265	265
266		~~- $scripts = array( $context->getOutput()->getHeadScripts( $context->getSkin() ) );~~
	266	+ $scripts = array( $context->getOutput()->getHeadScripts() );
267	267
268	268	$result_array['headitems'] = array_merge( $headItems, $css, $scripts );
269	269	}
Index: trunk/phase3/includes/SkinTemplate.php
—	—	@@ -208,8 +208,8 @@
209	209	$tpl->setRef( 'xhtmldefaultnamespace', $wgXhtmlDefaultNamespace );
210	210	$tpl->set( 'xhtmlnamespaces', $wgXhtmlNamespaces );
211	211	$tpl->set( 'html5version', $wgHtml5Version );
212		~~- $tpl->set( 'headlinks', $out->getHeadLinks( $this ) );~~
213		~~- $tpl->set( 'csslinks', $out->buildCssLinks( $this ) );~~
	212	+ $tpl->set( 'headlinks', $out->getHeadLinks() );
	213	+ $tpl->set( 'csslinks', $out->buildCssLinks() );
214	214
215	215	if( $wgUseTrackbacks && $out->isArticleRelated() ) {
216	216	$tpl->set( 'trackbackhtml', $out->getTitle()->trackbackRDF() );
—	—	@@ -1278,7 +1278,7 @@
1279	1279	wfProfileIn( __METHOD__ );
1280	1280
1281	1281	if( $allowUserJs && $this->loggedin ) {
1282		~~- if( $this->getTitle()->isJsSubpage() and $this->userCanPreview() ) {~~
	1282	+ if( $this->getTitle()->isJsSubpage() and $this->getOutput()->userCanPreview() ) {
1283	1283	# XXX: additional security check/prompt?
1284	1284	$this->userjsprev = '/<![CDATA[/ ' . $wgRequest->getText( 'wpTextbox1' ) . ' /]]>/';
1285	1285	} else {
Index: trunk/phase3/includes/Skin.php
—	—	@@ -308,30 +308,6 @@
309	309	}
310	310
311	311	/**
312		~~- * To make it harder for someone to slip a user a fake~~
313		~~- * user-JavaScript or user-CSS preview, a random token~~
314		~~- * is associated with the login session. If it's not~~
315		~~- * passed back with the preview request, we won't render~~
316		~~- * the code.~~
317		- *
318		~~- * @return bool~~
319		~~- */~~
320		~~- public function userCanPreview() {~~
321		~~- if ( $this->getRequest()->getVal( 'action' ) != 'submit'~~
322		~~- \|\| !$this->getRequest()->wasPosted()~~
323		~~- \|\| !$this->getUser()->matchEditToken(~~
324		~~- $this->getRequest()->getVal( 'wpEditToken' ) )~~
325		~~- ) {~~
326		~~- return false;~~
327		~~- }~~
328		~~- if ( !$this->getTitle()->isJsSubpage() && !$this->getTitle()->isCssSubpage() ) {~~
329		~~- return false;~~
330		~~- }~~
331		-
332		~~- return !count( $this->getTitle()->getUserPermissionsErrors( 'edit', $this->getUser() ) );~~
333		~~- }~~
334		-
335		- /**
336	312	* Generated JavaScript action=raw&gen=js
337	313	* This used to load MediaWiki:Common.js and the skin-specific style
338	314	* before the ResourceLoader.
—	—	@@ -357,48 +333,6 @@
358	334	}
359	335
360	336	/**
361		~~- * @private~~
362		~~- * @todo document~~
363		~~- * @param $out OutputPage~~
364		~~- */~~
365		~~- function setupUserCss( OutputPage $out ) {~~
366		~~- global $wgUseSiteCss, $wgAllowUserCss, $wgAllowUserCssPrefs;~~
367		-
368		~~- wfProfileIn( __METHOD__ );~~
369		-
370		~~- $this->setupSkinUserCss( $out );~~
371		~~- // Add any extension CSS~~
372		~~- foreach ( $out->getExtStyle() as $url ) {~~
373		~~- $out->addStyle( $url );~~
374		~~- }~~
375		-
376		~~- // Per-site custom styles~~
377		~~- if ( $wgUseSiteCss ) {~~
378		~~- $out->addModuleStyles( array( 'site', 'noscript' ) );~~
379		~~- if( $this->getUser()->isLoggedIn() ){~~
380		~~- $out->addModuleStyles( 'user.groups' );~~
381		~~- }~~
382		~~- }~~
383		-
384		~~- // Per-user custom styles~~
385		~~- if ( $wgAllowUserCss ) {~~
386		~~- if ( $this->getTitle()->isCssSubpage() && $this->userCanPreview() ) {~~
387		~~- // @todo FIXME: Properly escape the cdata!~~
388		~~- $out->addInlineStyle( $this->getRequest()->getText( 'wpTextbox1' ) );~~
389		~~- } else {~~
390		~~- $out->addModuleStyles( 'user' );~~
391		~~- }~~
392		~~- }~~
393		-
394		~~- // Per-user preference styles~~
395		~~- if ( $wgAllowUserCssPrefs ) {~~
396		~~- $out->addModuleStyles( 'user.options' );~~
397		~~- }~~
398		-
399		~~- wfProfileOut( __METHOD__ );~~
400		~~- }~~
401		-
402		- /**
403	337	* Get the query to generate a dynamic stylesheet
404	338	*
405	339	* @return array
—	—	@@ -695,7 +629,7 @@
696	630	// TODO and the suckage continues. This function is really just a wrapper around
697	631	// OutputPage::getBottomScripts() which takes a Skin param. This should be cleaned
698	632	// up at some point
699		~~- $bottomScriptText = $out->getBottomScripts( $this );~~
	633	+ $bottomScriptText = $out->getBottomScripts();
700	634	wfRunHooks( 'SkinAfterBottomScripts', array( $this, &$bottomScriptText ) );
701	635
702	636	return $bottomScriptText;

Follow-up revisions

Revision	Commit summary	Author	Date
r92856	And I forgot to rename that variable in r92855	ialex	10:48, 22 July 2011

Comments

#Comment by Aaron Schulz (talk | contribs) 18:46, 2 August 2011

Seems OK.

Status & tagging log

18:46, 2 August 2011 Aaron Schulz (talk | contribs) changed the status of r92855 [removed: new added: resolved]