r87722 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r87721‎ | r87722 | r87723 >
Date:15:25, 9 May 2011
Author:neilk
Status:ok (Comments)
Tags:todo 
Comment:
do not create console logging div unless configured to do so
Modified paths:
  • /trunk/extensions/UploadWizard/resources/mw.Log.js (modified) (history)

Diff [purge]

Index: trunk/extensions/UploadWizard/resources/mw.Log.js
@@ -61,37 +61,39 @@
6262 if ( typeof window.console !== 'undefined' && typeof window.console.log === 'function' ) {
6363 window.console.log( s );
6464 } else {
65 - // Set timestamp
66 - var d = new Date();
67 - var time = ( pad( d.getHours(), 2 ) + ':' + pad( d.getMinutes(), 2 ) + pad( d.getSeconds(), 2 ) + pad( d.getMilliseconds(), 3 ) );
68 - // Show a log box for console-less browsers
69 - var $log = $( '#mw-log-console' );
70 - if ( !$log.length ) {
71 - $log = $( '<div id="mw-log-console"></div>' )
72 - .css( {
73 - 'position': 'fixed',
74 - 'overflow': 'auto',
75 - 'z-index': 500,
76 - 'bottom': '0px',
77 - 'left': '0px',
78 - 'right': '0px',
79 - 'height': '150px',
80 - 'background-color': 'white',
81 - 'border-top': 'solid 2px #ADADAD'
82 - } )
83 - .appendTo( 'body' );
 65+ if ( typeof mw.log.makeConsole !== 'undefined' && mw.log.makeConsole ) {
 66+ // Set timestamp
 67+ var d = new Date();
 68+ var time = ( pad( d.getHours(), 2 ) + ':' + pad( d.getMinutes(), 2 ) + pad( d.getSeconds(), 2 ) + pad( d.getMilliseconds(), 3 ) );
 69+ // Show a log box for console-less browsers
 70+ var $log = $( '#mw-log-console' );
 71+ if ( !$log.length ) {
 72+ $log = $( '<div id="mw-log-console"></div>' )
 73+ .css( {
 74+ 'position': 'fixed',
 75+ 'overflow': 'auto',
 76+ 'z-index': 500,
 77+ 'bottom': '0px',
 78+ 'left': '0px',
 79+ 'right': '0px',
 80+ 'height': '150px',
 81+ 'background-color': 'white',
 82+ 'border-top': 'solid 2px #ADADAD'
 83+ } )
 84+ .appendTo( 'body' );
 85+ }
 86+ $log.append(
 87+ $( '<div></div>' )
 88+ .css( {
 89+ 'border-bottom': 'solid 1px #DDDDDD',
 90+ 'font-size': 'small',
 91+ 'font-family': 'monospace',
 92+ 'padding': '0.125em 0.25em'
 93+ } )
 94+ .text( s )
 95+ .append( '<span style="float:right">[' + time + ']</span>' )
 96+ );
8497 }
85 - $log.append(
86 - $( '<div></div>' )
87 - .css( {
88 - 'border-bottom': 'solid 1px #DDDDDD',
89 - 'font-size': 'small',
90 - 'font-family': 'monospace',
91 - 'padding': '0.125em 0.25em'
92 - } )
93 - .text( s )
94 - .append( '<span style="float:right">[' + time + ']</span>' )
95 - );
9698 }
9799 };
98100

Comments

#Comment by Brion VIBBER (talk | contribs)   19:48, 9 May 2011

mw.log.makeConsole never seems to be set anywhere; is this something that's meant to be activated via debug console or user JS? If so it probably needs to be documented.

#Comment by NeilK (talk | contribs)   19:57, 9 May 2011

correct, I am just making sure it doesn't get triggered. it causes super ugly divs to get created in IE otherwise

the obvious thing could be to obtain some kind of log option from config, but, where do we get that?

it's a big mess right now between mw.log and mw.Log :(

#Comment by Krinkle (talk | contribs)   19:49, 9 May 2011

Should probably use the core mw.log rather than this copy named mw.Log.

Was created because at the time mw.log wasn't in core IIRC.

#Comment by Krinkle (talk | contribs)   19:53, 9 May 2011

I'd go bonkers and fix all doubling between UpWiz modules and core modules, however I'm not sure which compatibility rule is set and when.

It's been deployed to 1.17wmf1 last week. Is it acceptable to require 1.18 now and perform any fixes in the wmf-branch directly ? Would probably make testing easier as well (wouldn't be the first time we referring to a method in the mw.-object that exists in trunk but not on the cluster).

#Comment by NeilK (talk | contribs)   19:54, 9 May 2011

usually there are real reasons not to reconcile the libraries, or IMO the UW version is superior

#Comment by NeilK (talk | contribs)   19:55, 9 May 2011

of course I want to fix all doubling. Also I've committed myself to talking about some of these libraries at Wikimania, precisely to force that  :)

#Comment by Krinkle (talk | contribs)   20:36, 9 May 2011

@TODO, removing Fixme

#Comment by Brion VIBBER (talk | contribs)   20:37, 9 May 2011

Marking as OK with a todo for now, as noted above the basic functionality is fine and this gets the extra consoles out of the way when not debugging in console-less IE. Proper fix is to merge with the newer mw.log, but it doesn't block deployment.

#Comment by NeilK (talk | contribs)   20:37, 9 May 2011

Marking this as "ok" but only for now -- it's a required hack for prod to stop undesirable behaviour in IE

#Comment by 😂 (talk | contribs)   20:41, 9 May 2011

Brion marked it ok, you didn't. You can't mark your own revs as ok :)

Status & tagging log