r70900 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r70899‎ | r70900 | r70901 >
Date:17:11, 11 August 2010
Author:platonides
Status:resolved (Comments)
Tags:
Comment:
getSkin() should get a Title as parameter.
Modified paths:
  • /trunk/phase3/includes/parser/LinkHolderArray.php (modified) (history)
  • /trunk/phase3/includes/parser/Parser.php (modified) (history)
  • /trunk/phase3/includes/parser/ParserOptions.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/parser/Parser.php
@@ -1145,7 +1145,7 @@
11461146 substr( $m[0], 0, 20 ) . '"' );
11471147 }
11481148 $url = wfMsg( $urlmsg, $id);
1149 - $sk = $this->mOptions->getSkin();
 1149+ $sk = $this->mOptions->getSkin( $this->mTitle );
11501150 $la = $sk->getExternalLinkAttributes( "external $CssClass" );
11511151 return "<a href=\"{$url}\"{$la}>{$keyword} {$id}</a>";
11521152 } elseif ( isset( $m[5] ) && $m[5] !== '' ) {
@@ -1174,7 +1174,7 @@
11751175 global $wgContLang;
11761176 wfProfileIn( __METHOD__ );
11771177
1178 - $sk = $this->mOptions->getSkin();
 1178+ $sk = $this->mOptions->getSkin( $this->mTitle );
11791179 $trail = '';
11801180
11811181 # The characters '<' and '>' (which were escaped by
@@ -1423,7 +1423,7 @@
14241424 global $wgContLang;
14251425 wfProfileIn( __METHOD__ );
14261426
1427 - $sk = $this->mOptions->getSkin();
 1427+ $sk = $this->mOptions->getSkin( $this->mTitle );
14281428
14291429 $bits = preg_split( $this->mExtLinkBracketedRegex, $text, -1, PREG_SPLIT_DELIM_CAPTURE );
14301430 $s = array_shift( $bits );
@@ -1572,7 +1572,7 @@
15731573 * @private
15741574 */
15751575 function maybeMakeExternalImage( $url ) {
1576 - $sk = $this->mOptions->getSkin();
 1576+ $sk = $this->mOptions->getSkin( $this->mTitle );
15771577 $imagesfrom = $this->mOptions->getAllowExternalImagesFrom();
15781578 $imagesexception = !empty( $imagesfrom );
15791579 $text = false;
@@ -1648,7 +1648,7 @@
16491649 $e1_img = "/^([{$tc}]+)\\|(.*)\$/sD";
16501650 }
16511651
1652 - $sk = $this->mOptions->getSkin();
 1652+ $sk = $this->mOptions->getSkin( $this->mTitle );
16531653 $holders = new LinkHolderArray( $this );
16541654
16551655 # split the entire text string on occurences of [[
@@ -1991,7 +1991,7 @@
19921992 */
19931993 function makeKnownLinkHolder( $nt, $text = '', $query = '', $trail = '', $prefix = '' ) {
19941994 list( $inside, $trail ) = Linker::splitTrail( $trail );
1995 - $sk = $this->mOptions->getSkin();
 1995+ $sk = $this->mOptions->getSkin( $this->mTitle );
19961996 # FIXME: use link() instead of deprecated makeKnownLinkObj()
19971997 $link = $sk->makeKnownLinkObj( $nt, $text, $query, $inside, $prefix );
19981998 return $this->armorLinks( $link ) . $trail;
@@ -3743,7 +3743,7 @@
37443744 }
37453745
37463746 # We need this to perform operations on the HTML
3747 - $sk = $this->mOptions->getSkin();
 3747+ $sk = $this->mOptions->getSkin( $this->mTitle );
37483748
37493749 # headline counter
37503750 $headlineCount = 0;
@@ -4480,7 +4480,7 @@
44814481 $ig->setParser( $this );
44824482 $ig->setHideBadImages();
44834483 $ig->setAttributes( Sanitizer::validateTagAttributes( $params, 'table' ) );
4484 - $ig->useSkin( $this->mOptions->getSkin() );
 4484+ $ig->useSkin( $this->mOptions->getSkin( $this->mTitle ) );
44854485 $ig->mRevisionId = $this->mRevisionId;
44864486
44874487 if ( isset( $params['showfilename'] ) ) {
@@ -4618,7 +4618,7 @@
46194619 # * text-bottom
46204620
46214621 $parts = StringUtils::explode( "|", $options );
4622 - $sk = $this->mOptions->getSkin();
 4622+ $sk = $this->mOptions->getSkin( $this->mTitle );
46234623
46244624 # Give extensions a chance to select the file revision for us
46254625 $skip = $time = $descQuery = false;
Index: trunk/phase3/includes/parser/ParserOptions.php
@@ -74,9 +74,9 @@
7575 function getIsPrintable() { $this->accessedOptions['printable'] = true;
7676 return $this->mIsPrintable; }
7777
78 - function getSkin() {
 78+ function getSkin( $title = null ) {
7979 if ( !isset( $this->mSkin ) ) {
80 - $this->mSkin = $this->mUser->getSkin();
 80+ $this->mSkin = $this->mUser->getSkin( $title );
8181 }
8282 return $this->mSkin;
8383 }
Index: trunk/phase3/includes/parser/LinkHolderArray.php
@@ -132,7 +132,7 @@
133133 global $wgContLang;
134134
135135 $colours = array();
136 - $sk = $this->parent->getOptions()->getSkin();
 136+ $sk = $this->parent->getOptions()->getSkin( $this->parent->mTitle );
137137 $linkCache = LinkCache::singleton();
138138 $output = $this->parent->getOutput();
139139
@@ -269,7 +269,7 @@
270270
271271 wfProfileIn( __METHOD__ );
272272 # Make interwiki link HTML
273 - $sk = $this->parent->getOptions()->getSkin();
 273+ $sk = $this->parent->getOptions()->getSkin( $this->parent->mTitle );
274274 $output = $this->parent->getOutput();
275275 $replacePairs = array();
276276 foreach( $this->interwikis as $key => $link ) {
@@ -294,7 +294,7 @@
295295 $variantMap = array(); // maps $pdbkey_Variant => $keys (of link holders)
296296 $output = $this->parent->getOutput();
297297 $linkCache = LinkCache::singleton();
298 - $sk = $this->parent->getOptions()->getSkin();
 298+ $sk = $this->parent->getOptions()->getSkin( $this->parent->mTitle );
299299 $threshold = $this->getStubThreshold();
300300 $titlesToBeConverted = '';
301301 $titlesAttrs = array();

Follow-up revisions

RevisionCommit summaryAuthorDate
r81675Cleanup to r70900, r72481: don't construct new skin objects just because the ...demon01:08, 8 February 2011
r81684MFT r81675: avoid creating unneeded skin objects due to changes in r70900, r7...brion02:48, 8 February 2011
r81685MFT from r81675 via REL1_17 r81684: cleanup for r70900, r72481brion02:50, 8 February 2011

Comments

#Comment by MarkAHershberger (talk | contribs)   23:03, 5 January 2011

Looks good, but what does r72481 (where getSkin now doesn't set mSkin when a title is passed in) mean for this change?

#Comment by Trevor Parscal (WMF) (talk | contribs)   00:47, 8 February 2011

It means we are creating lots of title objects. What motivated this change?

#Comment by Trevor Parscal (WMF) (talk | contribs)   00:49, 8 February 2011

Eh hem.. I meant skin objects

#Comment by Trevor Parscal (WMF) (talk | contribs)   00:52, 8 February 2011

Marking OK for now - someone who might be able to offer more insights into how much of a performance issue this might be might want to weigh in however.

#Comment by 😂 (talk | contribs)   01:10, 8 February 2011

I cleaned up this and r72481 in r81675 so we don't keep creating skin objects over and over.

#Comment by Catrope (talk | contribs)   22:30, 20 July 2011

Setting to resolved. According to Chad this is not an issue any more because the excessive creation of Skin objects was curbed in r81675, and because most calls to getSkin() are going away now that Linker is static.

#Comment by Brion VIBBER (talk | contribs)   02:38, 8 February 2011

Doesn't seem useful, and required weird cleanup. Rumblings in #wikimedia-dev to revert it, and I'm inclined to agree.

#Comment by Platonides (talk | contribs)   23:52, 8 February 2011

I didn't remember this revision. I think it was because User::getSkin() wants a Title to work properly. Otherwise it uses the generic $wgOut->getTitle(); which could be wrong.

Status & tagging log