r83552 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r83551‎ | r83552 | r83553 >
Date:23:44, 8 March 2011
Author:reedy
Status:reverted (Comments)
Tags:
Comment:
* (bug 27756) When viewing diffs, syntax highlight is not being applyed anymore on preview

Do a couple of bits of code tidyup to go along with it (It really needs more love...)
Modified paths:
  • /trunk/extensions/FlaggedRevs/client/flaggedrevs.js (modified) (history)

Diff [purge]

Index: trunk/extensions/FlaggedRevs/client/flaggedrevs.js
——@@ -185,6 +185,7 @@
186186 var contentsDiv = document.getElementById("mw-fr-revisioncontents");
187187 var prevLink = document.getElementById("differences-prevlink");
188188 var nextLink = document.getElementById("differences-nextlink");
 189+
189190 var timeoutId = null;
190191 if( contentsDiv ) {
191192 var diffUIParams = document.getElementById("mw-fr-diff-dataform");
——@@ -192,7 +193,7 @@
193194 var origContents = contentsDiv.innerHTML;
194195 contentsDiv.innerHTML = "<span class='loading mw-small-spinner spinner'>" +
195196 "</span><span class='loading' >" + wgRevContents.waiting + "</span>";
196 - var requestArgs = 'action=parse&prop=text|categorieshtml|languageshtml&format=xml';
 197+ var requestArgs = 'action=parse&prop=text|categorieshtml|languageshtml|headitems&format=xml';
197198 if ( window.wgCurRevisionId == oldRevId && window.wgPageName ) {
198199 requestArgs += '&page=' + encodeURIComponent( window.wgPageName );
199200 } else {
——@@ -207,21 +208,24 @@
208209 success : function( result ) {
209210 contentsDiv.innerHTML = "";
210211 contents = jQuery(result).find("text");
211 - if ( contents && contents.text() ) {
 212+ if ( contents.text() ) {
212213 contentsDiv.innerHTML += contents.text();
213214 } else {
214215 contentsDiv.innerHTML = wgRevContents.error + " " + origContents;
215216 }
216217 categoryhtml = jQuery(result).find("categorieshtml");
217 - if ( categoryhtml && categoryhtml.text() ) {
 218+ if ( categoryhtml.text() ) {
218219 contentsDiv.innerHTML += categoryhtml.text();
219220 }
220221 languageshtml = jQuery(result).find("languageshtml");
221 - if ( languageshtml && languageshtml.text() ) {
 222+ if ( languageshtml.text() ) {
222223 contentsDiv.innerHTML += "<div class='langlinks' >" +
223224 languageshtml.text() + "</div>";
224225 }
225 -
 226+ $headitems = jQuery(result).find("hi");
 227+ if ( $headitems.text() ) {
 228+ $('head').append( $headitems.text() );
 229+ }
226230 },
227231 error : function(xmlHttpRequest, textStatus, errThrown) {
228232 contentsDiv.innerHTML = wgRevContents.error + " " + origContents;

Follow-up revisions

RevisionCommit summaryAuthorDate
r84582Refactor FlaggedRevs modules:...krinkle23:12, 22 March 2011
r88639(bug 27756, bug 26328) Removed AJAX preview (on diff) loading: causes more tr...aaron14:09, 23 May 2011

Comments

#Comment by Aaron Schulz (talk | contribs)   15:43, 17 March 2011

This just blanks out the page on every diff (FF & Opera, maybe more).

#Comment by Reedy (talk | contribs)   15:46, 17 March 2011

Can you tell me which of the changes break it? I'm not installing and configuring FR when I've no real need to...

I'm guessing if the whole page is blank, it's something to do with 

-				if ( contents && contents.text() ) {
+				if ( contents.text() ) {
#Comment by Aaron Schulz (talk | contribs)   16:06, 17 March 2011

Looks like: +$('head').append( $headitems.text() );

#Comment by Reedy (talk | contribs)   16:08, 17 March 2011

I'll catch Krinkle when he's about. He said it was correct JS-wise

#Comment by Krinkle (talk | contribs)   00:01, 23 March 2011

It is indeed correct in general.

However during the rewrite in r84582 I've reverted this revision because it blanks the page and causes a bunch of other problems.

The reason is actually quite simple. The API return headitems also includes resources unrelated to FlaggedRevs (MediaWiki library, jQuery, other extensions etc.). Adding that to the <head></nowiki</code> (ie. loading the scripts) causes those other scripts to be redefined and reinitialized (ie. all registered plugins are broken, any dom-ready calls may get duplicated causing events to elements to be bound twice). And due to some (valid!) document.write usage it blanks the page, because the output of ResourceLoader in <code><nowiki><head> assumes the page to be still loading. However since this is loaded in way later via the API the document is already ready and thus the page is rewritten/blanked.

#Comment by Krinkle (talk | contribs)   00:02, 23 March 2011

It is indeed correct in general.

However during the rewrite in r84582 I've reverted this revision because it blanks the page and causes a bunch of other problems.

The reason is actually quite simple. The API return headitems also includes resources unrelated to FlaggedRevs (MediaWiki library, jQuery, other extensions etc.). Adding that to the <head> (ie. loading the scripts) causes those other scripts to be redefined and reinitialized (ie. all registered plugins are broken, any dom-ready calls may get duplicated causing events to elements to be bound twice). And due to some (valid!) document.write usage it blanks the page, because the output of ResourceLoader in <head> assumes the page to be still loading. However since this is loaded in way later via the API the document is already ready and thus the page is rewritten/blanked.

Status & tagging log