r105122 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r105121‎ | r105122 | r105123 >
Date:18:29, 4 December 2011
Author:johnduhart
Status:resolved (Comments)
Tags:brion 
Comment:
Adding new debugging toolbar

Needs a UI cleanup still, but for the most part is working.
Modified paths:
  • /trunk/phase3/RELEASE-NOTES-1.19 (modified) (history)
  • /trunk/phase3/includes/Article.php (modified) (history)
  • /trunk/phase3/includes/AutoLoader.php (modified) (history)
  • /trunk/phase3/includes/DefaultSettings.php (modified) (history)
  • /trunk/phase3/includes/GlobalFunctions.php (modified) (history)
  • /trunk/phase3/includes/Setup.php (modified) (history)
  • /trunk/phase3/includes/Skin.php (modified) (history)
  • /trunk/phase3/includes/db/Database.php (modified) (history)
  • /trunk/phase3/resources/Resources.php (modified) (history)
  • /trunk/phase3/resources/mediawiki/mediawiki.debug.css (added) (history)
  • /trunk/phase3/resources/mediawiki/mediawiki.debug.js (added) (history)

Diff [purge]

Index: trunk/phase3/RELEASE-NOTES-1.19
——@@ -97,6 +97,7 @@
9898 * (bug 32666) Special:ActiveUsers now allows a subpage to be used as the
9999 username (eg. Special:ActiveUsers/Username)
100100 * New JavaScript variable wgPageContentLanguage
 101+* Added new debugging toolbar, enabled with $wgDebugToolbar
101102
102103 === Bug fixes in 1.19 ===
103104 * $wgUploadNavigationUrl should be used for file redlinks if.
Index: trunk/phase3/includes/Article.php
——@@ -374,7 +374,7 @@
375375 */
376376 public function view() {
377377 global $wgUser, $wgOut, $wgRequest, $wgParser;
378 - global $wgUseFileCache, $wgUseETag;
 378+ global $wgUseFileCache, $wgUseETag, $wgDebugToolbar;
379379
380380 wfProfileIn( __METHOD__ );
381381
——@@ -429,7 +429,7 @@
430430 }
431431
432432 # Try client and file cache
433 - if ( $oldid === 0 && $this->mPage->checkTouched() ) {
 433+ if ( !$wgDebugToolbar && $oldid === 0 && $this->mPage->checkTouched() ) {
434434 if ( $wgUseETag ) {
435435 $wgOut->setETag( $parserCache->getETag( $this, $parserOptions ) );
436436 }
Index: trunk/phase3/includes/GlobalFunctions.php
——@@ -835,6 +835,8 @@
836836 wfErrorLog( $text, $wgDebugLogFile );
837837 }
838838 }
 839+
 840+ MWDebug::debugMsg( $text );
839841 }
840842
841843 /**
Index: trunk/phase3/includes/db/Database.php
——@@ -840,9 +840,13 @@
841841 throw new MWException( 'Tainted query found' );
842842 }
843843
 844+ $queryId = MWDebug::query( $sql, $fname, $isMaster );
 845+
844846 # Do the query and handle errors
845847 $ret = $this->doQuery( $commentedSql );
846848
 849+ MWDebug::queryTime( $queryId );
 850+
847851 # Try reconnecting if the connection was lost
848852 if ( false === $ret && $this->wasErrorReissuable() ) {
849853 # Transaction is gone, like it or not
Index: trunk/phase3/includes/Setup.php
——@@ -328,6 +328,8 @@
329329 $wgCookieSecure = ( substr( $wgServer, 0, 6 ) === 'https:' );
330330 }
331331
 332+MWDebug::init();
 333+
332334 if ( !defined( 'MW_COMPILED' ) ) {
333335 if ( !MWInit::classExists( 'AutoLoader' ) ) {
334336 require_once( "$IP/includes/AutoLoader.php" );
——@@ -392,6 +394,7 @@
393395 }
394396 }
395397 wfDebug( "$debug\n" );
 398+ MWDebug::processRequest( $wgRequest );
396399 }
397400
398401 wfProfileOut( $fname . '-misc1' );
Index: trunk/phase3/includes/AutoLoader.php
——@@ -436,6 +436,9 @@
437437 'ResultWrapper' => 'includes/db/DatabaseUtility.php',
438438 'SQLiteField' => 'includes/db/DatabaseSqlite.php',
439439
 440+ # includes/debug
 441+ 'MWDebug' => 'includes/debug/Debug.php',
 442+
440443 # includes/diff
441444 '_DiffEngine' => 'includes/diff/DairikiDiff.php',
442445 '_DiffOp' => 'includes/diff/DairikiDiff.php',
Index: trunk/phase3/includes/DefaultSettings.php
——@@ -4160,6 +4160,14 @@
41614161 */
41624162 $wgWikiID = false;
41634163
 4164+/**
 4165+ * Display the new debugging toolbar. This also enables profiling on database
 4166+ * queries and other useful output.
 4167+ *
 4168+ * @since 1.19
 4169+ */
 4170+$wgDebugToolbar = false;
 4171+
41644172 /** @} */ # end of profiling, testing and debugging }
41654173
41664174 /************************************************************************//**
Index: trunk/phase3/includes/Skin.php
——@@ -531,13 +531,15 @@
532532 protected function generateDebugHTML() {
533533 global $wgShowDebug;
534534
 535+ $html = MWDebug::getDebugHTML();
 536+
535537 if ( $wgShowDebug ) {
536538 $listInternals = $this->formatDebugHTML( $this->getOutput()->mDebugtext );
537 - return "\n<hr />\n<strong>Debug data:</strong><ul id=\"mw-debug-html\">" .
 539+ $html .= "\n<hr />\n<strong>Debug data:</strong><ul id=\"mw-debug-html\">" .
538540 $listInternals . "</ul>\n";
539541 }
540542
541 - return '';
 543+ return $html;
542544 }
543545
544546 /**
Index: trunk/phase3/resources/Resources.php
——@@ -489,6 +489,10 @@
490490 'debugScripts' => 'resources/mediawiki/mediawiki.log.js',
491491 'debugRaw' => false,
492492 ),
 493+ 'mediawiki.debug' => array(
 494+ 'scripts' => 'resources/mediawiki/mediawiki.debug.js',
 495+ 'styles' => 'resources/mediawiki/mediawiki.debug.css',
 496+ ),
493497 'mediawiki.htmlform' => array(
494498 'scripts' => 'resources/mediawiki/mediawiki.htmlform.js',
495499 ),
Index: trunk/phase3/resources/mediawiki/mediawiki.debug.css
——@@ -0,0 +1,111 @@
 2+.mw-debug {
 3+ width: 100%;
 4+ text-align: left;
 5+ position: fixed;
 6+ bottom: 0;
 7+ background-color: #eee;
 8+ border-top: 1px solid #ccc;
 9+}
 10+
 11+.mw-debug pre {
 12+ font-family: Monaco, "Consolas", "Lucida Console", "Courier New", monospace;
 13+ font-size: 11px;
 14+ padding: 0;
 15+ margin: 0;
 16+ background: none;
 17+ border: none;
 18+}
 19+
 20+.mw-debug ul {
 21+ margin: 0;
 22+ list-style: none;
 23+ box-sizing: border-box;
 24+}
 25+
 26+.mw-debug li {
 27+ padding: 2px 5px;
 28+ width: 100%;
 29+ display: table;
 30+}
 31+
 32+.mw-debug-bit {
 33+ min-height: 25px;
 34+ display: inline-block;
 35+ padding: 5px;
 36+ border-right: 1px solid #ccc;
 37+ font-size: 13px;
 38+}
 39+
 40+.mw-debug-pane {
 41+ max-height: 300px;
 42+ overflow: scroll;
 43+ border-top: 2px solid #ccc;
 44+ display: none;
 45+ font-size: 11px;
 46+ background-color: #e1eff2;
 47+ box-sizing: border-box;
 48+}
 49+
 50+.mw-debug-right {
 51+ float: right;
 52+}
 53+
 54+#mw-debug-querylist tr {
 55+
 56+}
 57+
 58+#mw-debug-querylist td {
 59+ padding: 2px 5px;
 60+ border-bottom: 1px solid #ccc;
 61+}
 62+
 63+.mw-debug-query-time {
 64+ color: #808080;
 65+}
 66+
 67+#mw-debug-pane-request {
 68+ padding: 20px;
 69+}
 70+
 71+#mw-debug-pane-request table {
 72+ width: 100%;
 73+ margin: 10px 0 30px;
 74+}
 75+
 76+#mw-debug-pane-request tr,
 77+#mw-debug-pane-request th,
 78+#mw-debug-pane-request td,
 79+#mw-debug-pane-request table {
 80+ border: 1px solid #D0DBB3;
 81+ border-collapse: collapse;
 82+ margin: 0;
 83+}
 84+
 85+#mw-debug-pane-request th,
 86+#mw-debug-pane-request td {
 87+ font-size: 12px;
 88+ padding: 8px 10px;
 89+}
 90+
 91+#mw-debug-pane-request th {
 92+ background-color: #F1F7E2;
 93+ font-weight: bold;
 94+}
 95+
 96+#mw-debug-pane-request td {
 97+ background-color: white;
 98+}
 99+
 100+#mw-debug-pane-querylist table {
 101+ border-spacing: 0;
 102+}
 103+
 104+#mw-debug-pane-includes li,
 105+#mw-debug-pane-querylist tr td {
 106+ background-color: #ccc;
 107+}
 108+
 109+#mw-debug-pane-includes li:nth-child(odd),
 110+#mw-debug-pane-querylist tr:nth-child(odd) td {
 111+ background-color: #ddd;
 112+}
Index: trunk/phase3/resources/mediawiki/mediawiki.debug.js
——@@ -0,0 +1,208 @@
 2+/**
 3+ * Javascript for the new debug toolbar, enabled with $wgDebugToolbar
 4+ *
 5+ * @author John Du Hart
 6+ * @since 1.19
 7+ */
 8+
 9+( function( $ ) {
 10+
 11+ var debug = mw.Debug = {
 12+ /**
 13+ * Toolbar container element
 14+ *
 15+ * @var {jQuery}
 16+ */
 17+ $container: null,
 18+
 19+ /**
 20+ * Array containing data for the debug toolbar
 21+ *
 22+ * @var {Array}
 23+ */
 24+ data: {},
 25+
 26+ /**
 27+ * Initializes the debugging pane
 28+ *
 29+ * @param {Array} data
 30+ */
 31+ init: function( data ) {
 32+ this.data = data;
 33+ this.buildHtml();
 34+
 35+ // Insert the container into the DOM
 36+ $( 'body' ).append( this.$container );
 37+
 38+ $( '.mw-debug-panelink' ).click( this.switchPane );
 39+ },
 40+
 41+ /**
 42+ * Switches between panes
 43+ *
 44+ * @todo Store cookie for last pane open
 45+ * @context {Element}
 46+ * @param {jQuery.Event} e
 47+ */
 48+ switchPane: function( e ) {
 49+ var currentPaneId = debug.$container.data( 'currentPane' ),
 50+ requestedPaneId = $(this).parent().attr( 'id' ).substr( 9 ),
 51+ $currentPane = $( '#mw-debug-pane-' + currentPaneId ),
 52+ $requestedPane = $( '#mw-debug-pane-' + requestedPaneId );
 53+ e.preventDefault();
 54+
 55+ // Hide the current pane
 56+ if ( requestedPaneId === currentPaneId ) {
 57+ $currentPane.slideUp();
 58+ debug.$container.data( 'currentPane', null );
 59+ return;
 60+ }
 61+
 62+ debug.$container.data( 'currentPane', requestedPaneId );
 63+
 64+ if ( currentPaneId === undefined || currentPaneId === null ) {
 65+ $requestedPane.slideDown();
 66+ } else {
 67+ $currentPane.hide();
 68+ $requestedPane.show();
 69+ }
 70+ },
 71+
 72+ /**
 73+ * Constructs the HTML for the debugging toolbar
 74+ */
 75+ buildHtml: function() {
 76+ this.$container = $( '<div></div>' )
 77+ .attr({
 78+ id: 'mw-debug-container',
 79+ class: 'mw-debug'
 80+ });
 81+
 82+ var html = '';
 83+
 84+ html += '<div class="mw-debug-bit" id="mw-debug-mwversion">'
 85+ + '<a href="https://www.mediawiki.org//www.mediawiki.org/">MediaWiki</a>: '
 86+ + this.data.mwVersion + '</div>';
 87+
 88+ html += '<div class="mw-debug-bit" id="mw-debug-phpversion">'
 89+ + '<a href="https://www.mediawiki.org//www.php.net/">PHP</a>: '
 90+ + this.data.phpVersion + '</div>';
 91+
 92+ html += '<div class="mw-debug-bit" id="mw-debug-time">'
 93+ + 'Time: ' + this.data.time.toFixed( 5 ) + 's</div>';
 94+ html += '<div class="mw-debug-bit" id="mw-debug-memory">'
 95+ + 'Memory: ' + this.data.memory + ' (<span title="Peak usage">'
 96+ + this.data.memoryPeak + '</span>)</div>';
 97+
 98+ var queryLink = '<a href="#" class="mw-debug-panelink" id="mw-debug-query-link">Queries: '
 99+ + this.data.queries.length + '</a>';
 100+ html += '<div class="mw-debug-bit" id="mw-debug-querylist">'
 101+ + queryLink + '</div>';
 102+
 103+ var debugLink = '<a href="#" class="mw-debug-panelink" id="mw-debug-debuglog-link">Debug Log ('
 104+ + this.data.debugLog.length + ' lines)</a>';
 105+ html += '<div class="mw-debug-bit" id="mw-debug-debuglog">'
 106+ + debugLink + '</div>';
 107+
 108+ var requestLink = '<a href="#" class="mw-debug-panelink" id="mw-debug-request-link">Request</a>';
 109+ html += '<div class="mw-debug-bit" id="mw-debug-request">'
 110+ + requestLink + '</div>';
 111+
 112+ var filesLink = '<a href="#" class="mw-debug-panelink" id="mw-debug-files-includes">'
 113+ + this.data.includes.length + ' Files Included</a>';
 114+ html += '<div class="mw-debug-bit" id="mw-debug-includes">'
 115+ + filesLink + '</div>';
 116+
 117+ html += '<div class="mw-debug-pane" id="mw-debug-pane-querylist">'
 118+ + this.buildQueryTable() + '</div>';
 119+ html += '<div class="mw-debug-pane" id="mw-debug-pane-debuglog">'
 120+ + this.buildDebugLogTable() + '</div>';
 121+ html += '<div class="mw-debug-pane" id="mw-debug-pane-request">'
 122+ + this.buildRequestPane() + '</div>';
 123+ html += '<div class="mw-debug-pane" id="mw-debug-pane-includes">'
 124+ + this.buildIncludesPane() + '</div>';
 125+
 126+ this.$container.html( html );
 127+ },
 128+
 129+ /**
 130+ * Query list pane
 131+ */
 132+ buildQueryTable: function() {
 133+ var html = '<table id="mw-debug-querylist">';
 134+
 135+ for ( var i = 0, length = this.data.queries.length; i < length; i++ ) {
 136+ var query = this.data.queries[i];
 137+
 138+ html += '<tr><td>' + ( i + 1 ) + '</td>';
 139+
 140+ html += '<td>' + query.sql + '</td>';
 141+ html += '<td><span class="mw-debug-query-time">(' + query.time.toFixed( 4 ) + 'ms)</span> ' + query.function + '</td>';
 142+
 143+ html += '</tr>';
 144+ }
 145+
 146+ html += '</table>';
 147+
 148+ return html;
 149+ },
 150+
 151+ /**
 152+ * Legacy debug log pane
 153+ */
 154+ buildDebugLogTable: function() {
 155+ var html = '<ul>';
 156+
 157+ for ( var i = 0, length = this.data.debugLog.length; i < length; i++ ) {
 158+ var line = this.data.debugLog[i];
 159+ html += '<li>' + line.replace( /\n/g, "<br />\n" ) + '</li>';
 160+ }
 161+
 162+ return html + '</ul>';
 163+ },
 164+
 165+ /**
 166+ * Request information pane
 167+ */
 168+ buildRequestPane: function() {
 169+ var buildTable = function( title, data ) {
 170+ var t = '<h2>' + title + '</h2>'
 171+ + '<table> <tr> <th>Key</th> <th>Value</th> </tr>';
 172+
 173+ for ( var key in data ) {
 174+ if ( !data.hasOwnProperty( key ) ) {
 175+ continue;
 176+ }
 177+ var value = data[key];
 178+
 179+ t += '<tr><th>' + key + '</th><td>' + value + '</td></tr>';
 180+ }
 181+
 182+ t += '</table>';
 183+ return t;
 184+ };
 185+
 186+ var html = this.data.request.method + ' ' + this.data.request.url;
 187+ html += buildTable( 'Headers', this.data.request.headers );
 188+ html += buildTable( 'Parameters', this.data.request.params );
 189+
 190+ return html;
 191+ },
 192+
 193+ /**
 194+ * Included files pane
 195+ */
 196+ buildIncludesPane: function() {
 197+ var html = '<ul>';
 198+
 199+ for ( var i = 0, l = this.data.includes.length; i < l; i++ ) {
 200+ var file = this.data.includes[i];
 201+ html += '<li><span class="mw-debug-right">' + file.size + '</span> ' + file.name + '</li>';
 202+ }
 203+
 204+ html += '</ul>';
 205+ return html;
 206+ }
 207+ };
 208+
 209+} )( jQuery );
\ No newline at end of file

Sign-offs

UserFlagDate
Krinkleinspected00:31, 2 January 2012
Krinkletested00:31, 2 January 2012

Follow-up revisions

RevisionCommit summaryAuthorDate
r105123svn:eol-style native on all phase3 stuff, since I messed that up in r105122...johnduhart18:35, 4 December 2011
r106306Followup r105122 & r105123, fixes and improvements per CR...johnduhart02:26, 15 December 2011
r107604[mediawiki.debug] apply code conventions...krinkle23:49, 29 December 2011
r109354dbg toolbar: prevents screen jumping...hashar10:36, 18 January 2012
r111035DebugToolbar: drop specific monospace font...hashar14:34, 9 February 2012

Comments

#Comment by Brion VIBBER (talk | contribs)   19:05, 6 December 2011

A lot of the HTML building in the JS side is inserting server-provided strings directly into raw HTML. These strings are actually plaintext, and should be escaped.

Best practice is to set text and attributes using jQuery methods. Instead of:

html += '

  • ' + file.size + ' ' + file.name + '

    • '; consider something like: $('

  • ') .append( $('').text(file.size) ) .append( ' ' + file.name ) .appendTo($list); and return elements or a jQuery wrapper directly instead of HTML. If you need to create HTML source directly, then make liberal use of mw.html.escape() or mw.html.element() helper functions (the former is like htmlspecialchars(), the latter is like Html::element()).
    • #Comment by Brion VIBBER (talk | contribs)   19:06, 6 December 2011

    Damn i shoulda previewed that more closely. ;)

    Best practice is to set text and attributes using jQuery methods. Instead of: 

     html += '<li><span class="mw-debug-right">' + file.size + '</span> ' + file.name + '</li>';

    consider something like: 

    $('<li>')
   .append( $('<span class="mw-debug-right"></span>').text(file.size) )
   .append( ' ' + file.name )
   .appendTo($list);

    and return elements or a jQuery wrapper directly instead of HTML.

    If you need to create HTML source directly, then make liberal use of mw.html.escape() or mw.html.element() helper functions (the former is like htmlspecialchars(), the latter is like Html::element()).

    #Comment by Brion VIBBER (talk | contribs)   19:10, 6 December 2011

    Another minor suggestion: use white-space: pre-line in the CSS for potentially long lines such as the SQL. On supporting browsers (Firefox works I know) this helps it to break long lines at commas, slashes etc that otherwise would only break at whitespace.

    #Comment by Brion VIBBER (talk | contribs)   19:11, 6 December 2011

    By the way -- this looks TOTALLY AWESOME and should be pretty handy. :DDD Another great feature would be a filter search within the queries...

    #Comment by Nikerabbit (talk | contribs)   19:16, 6 December 2011

    How does this relate to the other existing debugging facilities?

    #Comment by Brion VIBBER (talk | contribs)   19:19, 6 December 2011

    This is basically a supercharged version of $wgDebugComments, and should replace it.

    #Comment by Nikerabbit (talk | contribs)   08:25, 7 December 2011

    The good thing and the bad thing about $wgDebugComments is that it is invisible unless you look for it. But I kind of like this too. Even more if it was re-sizable :)

    #Comment by Brion VIBBER (talk | contribs)   19:19, 6 December 2011

    Keeping the fixme, removed it by mistake.

    #Comment by Krinkle (talk | contribs)   23:49, 29 December 2011

    Very nice :)

    #Comment by Krinkle (talk | contribs)   00:31, 2 January 2012

    Reviewed JS/CSS part. Not marking OK per the PHP changes.

    #Comment by Tim Starling (talk | contribs)   05:26, 2 January 2012

    Adding "brion" tag, the author is ready for another review.

    #Comment by Hashar (talk | contribs)   20:42, 20 January 2012

    Being bold and marking this revision as resolved. This has been reviewed / tested by lot of people now.

    #Comment by Nikerabbit (talk | contribs)   10:47, 9 February 2012

    Should the monospaced variant of Dejavu be here too? Or just not specify any fonts? 

    +	font-family: Monaco, "Consolas", "Lucida Console", "Courier New", monospace;
    #Comment by Hashar (talk | contribs)   14:29, 9 February 2012

    We should probably just drop the font-family. skins/common/commonElements.css define pre using 

    font-family: monospace, Courier;

    Note that "Courier" in this case is always ignored, it is just there to fix monospace font size.

    #Comment by Hashar (talk | contribs)   14:34, 9 February 2012

    Removed with r111035.

    Status & tagging log