User "Raindrift" posted a comment on MediaWiki.r104400. Full URL: http://www.mediawiki.org/wiki/Special:Code/MediaWiki/104400#c26664 Commit summary:
updates per code review comments: http://www.mediawiki.org/wiki/TimedMediaHandler/ReviewNotes#mw.TextSource.js == mw.TimedText.js == * fixed spelling: userLanugage to userLanguage * added default bottom padding config for textOffset * added code comment to clarify Animate param in text resize during player resize * added comment to explain default relative text size * corrected spelling of loadCurrentSubSrouce to loadCurrentSubSource and added function documentation * Removed all the add transcript support. Should be part of a gadget ( i.e once this ships could restore the miro universal subs gadget to a working state ) * added code documentation for track "kind" attribute and associated menu build out * moved TimedText.BelowVideoBlackBoxHeight to config == mw.TextSource.js == ( Moved TextSource to its own file ) * move loaded = true to after actual loading. * updated comments to point to bug 29126 * Added some comments for large regex used in srt parsing. * refactored the match handling of srt parsing to local convenience functions == mw.TimedTextMediaWikiSources.js == * fixed spelling ofmw.MediaWikTrackProvider * clean up getTimedTextNS conditional logic * updated default value to 710 per http://www.mediawiki.org/wiki/Extension_namespace_registration namespace register Comment: + //for( var i = 0; i < this.enabledSources.length ; i++ ) { + //var source = this.enabledSources[ i ]; + var source = this.enabledSources[ 0 ]; + if( source ) { + this.updateSourceDisplay( source, currentTime ); + } + //} Did you mean to leave these commented, or is this leftover debugging cruft that was committed by accident? + // Update text ( use "html" instead of "text" so that subtitle format can + // include html formating + // TOOD we should scrub this for non-formating html + $textTarget.append( + $('<span />') + .css( this.getCaptionCss() ) + .html( caption.content ) + ); If the TODO here is left unfixed, does it introduce an XSS vulnerability? Is the caption sanitized elsewhere? If not, what about using wikitext for the caption instead of HTML? + var emFontMap = { '6': .375, '7': .438, '8' : .5, '9': .563, '10': .625, '11':.688, + '12':.75, '13': .813, '14': .875, '15':.938, '16':1, '17':1.063, '18': 1.125, '19': 1.888, + '20':1.25, '21':1.313, '22':1.375, '23':1.438, '24':1.5}; Would it make more sense to just do pointSize * .0625, then round to 3 decimal places? Also, is it necessary to account for font sizes that are not the 16px default, like when the screen is zoomed? I know it's possible to measure the height of 1em using a trick like the one here: http://stackoverflow.com/questions/739940/detect-browser-font-size but I'm not sure if that's necessary or appropriate in this context. It's hard to tell how this code is actually used from the diff. Perhaps it's not really important that it be correct in all cases. _______________________________________________ MediaWiki-CodeReview mailing list [email protected] https://lists.wikimedia.org/mailman/listinfo/mediawiki-codereview
