Dartest CL - Please review

client/testing/dartest/dartest.dart

+// Copyright (c) 2011, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+#library('dartest');
+
+#import('dart:dom');

[pdr, 2011/12/14 21:02:36]

I would strongly recommend using dart:html. It's generally way easier and more
concise.

[shauvik, 2011/12/16 07:19:27]

When I started, you couldn't do many things using dart:html. Maybe they fixed it
now. I'll migrate to dart:html after I fix all of the current issues. 

On 2011/12/14 21:02:36, pdr wrote:

+#import('../unittest/unittest_dartest.dart');
+
+#source('css.dart');
+
+/** DARTest provides a library to run tests in the app */
+class DARTest{

[pdr, 2011/12/14 21:02:36]

Under the names section of the style guide
(http://www.dartlang.org/articles/style-guide) it suggests capitalizing
abbreviations like words (i.e., DartTest).

[shauvik, 2011/12/16 07:19:27]

Kelly and I decided to have the same name across all places. The name of the
tool is DARTest - we reuse the T; so I think it isn't suitable for camel casing.

On 2011/12/14 21:02:36, pdr wrote:

+  Map<String, HTMLElement> inAppElements, fullAppElements, appElements;

[pdr, 2011/12/14 21:02:36]

I don't think these are necessary (if they are, can you explain?)

[shauvik, 2011/12/16 07:19:27]

I spoke to Kelly and Joel and its difficult and sometimes not possible (buggy)
to move DOM elements across windows.
https://developer.mozilla.org/en/DOM/document.importNode

inAppElements is a map of elements in the same window - which is never
destroyed; only hidden and shown.

fullAppElements is a map of elements always created for a new popup window.

appElements always points to one of the above.

On 2011/12/14 21:02:36, pdr wrote:

+  bool inAppMode;

[Kelly Norton, 2011/12/15 18:35:31]

I suspect you want these to be private _inAppMode?

[shauvik, 2011/12/16 07:19:27]

Done.

+  DOMWindow win, appWindow;

[Kelly Norton, 2011/12/15 18:35:31]

private?

[shauvik, 2011/12/16 07:19:27]

Done.

+
+  DARTest(){ 
+    win = window;

[Kelly Norton, 2011/12/15 18:35:31]

It seems unnecessary to store a reference to window, do you do this for when the
runner is not running inApp?

[shauvik, 2011/12/16 07:19:27]

Done.

+    appWindow = window;
+    dartestLoggingFunc = log;
+    inAppMode = true;
+    inAppElements = new HashMap<String, HTMLElement>();
+    appElements = inAppElements;
+    
+    DARTestCss.inject(win.document, true);

[Kelly Norton, 2011/12/15 18:35:31]

document should also be available as a top-level getter, so you don't have to
reference it via win.document.

[shauvik, 2011/12/16 07:19:27]

Done.

+  }
+  
+  void run() {
+    renderMain();
+    createResultsTable();
+  }
+
+  void log(String message){

[pdr, 2011/12/14 21:02:36]

Log calls should generally be behind a flag at worst, or removed entirely.

[shauvik, 2011/12/16 07:19:27]

Whole of dartest is behind a compiler flag. In discussion with Kelly, we
discussed that the logging calls are helpful for the developer and that we
wanted to have them.

I have tried to minimize the things that we log to not overwhelm the developer
with logging messages.

On 2011/12/14 21:02:36, pdr wrote:

+    appWindow.console.log(message);
+  }
+  
+  void addChildToElem(String key, HTMLElement child) {

[Kelly Norton, 2011/12/15 18:35:31]

This seems like a sizeable public API. Are all of these methods supposed to be
public?

[shauvik, 2011/12/16 07:19:27]

Made it private.

On 2011/12/15 18:35:31, Kelly Norton wrote:

+    if(appElements[key] != null) {
+      appElements[key].appendChild(child);
+    }
+  }
+  
+  /** Create the results table after loading tests */
+  void createResultsTable(){
+    log('Creating results table');
+    HTMLTableElement table = win.document.createElement('table');

[Kelly Norton, 2011/12/15 18:35:31]

I'm really confused by all the references to different windows. I could see you
needing to keep a window reference for when the runner is not inApp. However, if
that's the case here are some other things I would expect:

I would expect there to be a single reference to window, probably called
runnerWindow.

I would also expect that runnWindow != window would remove the need to have an
inApp bool field and instead inApp would be a getter.

[shauvik, 2011/12/16 07:19:27]

Done.

+    table.className = 'dt-results';
+    HTMLTableSectionElement head = win.document.createElement('thead');
+    head.innerHTML = '<tr><th>ID <th>Description <th>Result';
+    table.appendChild(head);
+    
+    HTMLTableSectionElement body = win.document.createElement('tbody');
+    body.id = 'dt-results-body';
+    body.innerHTML = '';
+    for (TestCase t in tests){
+      body.innerHTML += "<tr id='dt-test-${t.id}'>" + getTestDetails(t);
+      body.innerHTML += "<tr id='dt-detail-${t.id}' class='dt-hide'></span>";
+    }
+    table.appendChild(body);
+    addChildToElem('testBody', table);
+  }
+
+  /** Update the results table for test */
+  void updateResultsTable(TestCase t, DOMWindow domWin){
+    HTMLTableRowElement row = domWin.document.getElementById('dt-test-${t.id}');
+    row.className = 'dt-result-row';
+    row.innerHTML = getTestDetails(t);
+    
+    HTMLTableRowElement details = domWin.document.getElementById('dt-detail-${t.id}');

[pdr, 2011/12/14 21:02:36]

Line > 100 chars

[shauvik, 2011/12/16 07:19:27]

Done.

+    details.innerHTML = getTestStats(t);
+    
+    row.addEventListener('click', (Event e) {
+        if(details.className == 'dt-hide') {
+          details.className = '';
+        } else {
+          details.className = 'dt-hide';
+        }
+      }, true);
+  }
+  
+  /** Escape HTML Special Chars */
+  String escape(String str){
+    str = str.replaceAll('&','&');
+    str = str.replaceAll('<','&lt;');
+    str = str.replaceAll('>','&gt;');
+    str = str.replaceAll('"','&quot;');
+    str = str.replaceAll("'",'&#x27;');
+    str = str.replaceAll('/','&#x2F;');
+    return str;
+  }
+  
+  /** Get test results as table cells */

[pdr, 2011/12/14 21:02:36]

This can be private so you can rename it using the underscore notation:
_getTestDetails(TestCase t). (Here, and elsewhere)

[shauvik, 2011/12/16 07:19:27]

Done.

+  String getTestDetails(TestCase t) {

[pdr, 2011/12/14 21:02:36]

Instead of building up this table by strings, I would suggest building up from
DOM nodes which has the added benefit that you don't have to worry about
escaping since you don't use innerHTML.

[pdr, 2011/12/14 21:02:36]

Without escaping and innerHTML-based DOM manipulation, you can shorten this
function to something like:
String get _testDetails(TestCase t) => "${t.id} - ${t.result == null ? 'none' :
t.result}";

[shauvik, 2011/12/16 07:19:27]

Done.

[shauvik, 2011/12/16 07:19:27]

Actually I need that value in the class too. So, can't use this and have changed
it accordingly.

On 2011/12/14 21:02:36, pdr wrote:

+    String desc =  escape(t.description);
+    String result = "none";
+    if(t.result != null){
+        result = t.result;
+    }
+    return "<td>${t.id} <td>$desc <td class='dt-$result' " +
+      "title='${escape(t.message)}'> ${result.toUpperCase()}";
+  }
+  
+  String getTestStats(TestCase t) {

[pdr, 2011/12/14 21:02:36]

See comments in getTestDetails

[shauvik, 2011/12/16 07:19:27]

Done.

+    String message = '';
+    if(t.message != '') {
+      message = t.message + '<br>';

[Kelly Norton, 2011/12/15 18:35:31]

This looks problematic. where does t.message get html-escaped?

[shauvik, 2011/12/16 07:19:27]

Done.

+    }
+    if(t.stackTrace != null) {
+      message += '<pre>${t.stackTrace}</pre>';

[Kelly Norton, 2011/12/15 18:35:31]

Similar here, I don't see where t.stackTrace is escaped.

[shauvik, 2011/12/16 07:19:27]

Done.

+    }
+    return "<td colspan='3'> $message took ${printDuration(t.timeDuration)}</td>";

[pdr, 2011/12/14 21:02:36]

Line > 100 cols

[shauvik, 2011/12/16 07:19:27]

Done.

+  }
+  
+  /** Update the UI after running test */

[pdr, 2011/12/14 21:02:36]

Nit: make sure to use a period in your comments :)

[shauvik, 2011/12/16 07:19:27]

Done.

+  void updateDARTestUI(TestCase test){

[Kelly Norton, 2011/12/15 18:35:31]

space before {

[shauvik, 2011/12/16 07:19:27]

Done.

+    // Update table

[pdr, 2011/12/14 21:02:36]

unnecessary comment

[shauvik, 2011/12/16 07:19:27]

Done.

+    updateResultsTable(test, win);
+    if(!inAppMode) {
+      updateResultsTable(test, appWindow);
+    }
+    
+    if(test.result != null) {
+      log('  Result: ${test.result.toUpperCase()} ${test.message}');
+    }
+    if(test.timeDuration != null){
+      log('  took ${printDuration(test.timeDuration)}');
+    }
+    updateStatusProgress(appElements);
+    if(!inAppMode) {
+      updateStatusProgress(inAppElements);
+    }
+  }
+  
+  void updateStatusProgress(Map<String, HTMLElement> elements){

[Kelly Norton, 2011/12/15 18:35:31]

space before {

[shauvik, 2011/12/16 07:19:27]

Done.

+    // Update progressbar
+    var pPass = ((numTestsRun - numTestsFailed - numTestsErrors) / tests.length) * 100;

[pdr, 2011/12/14 21:02:36]

Line > 100 cols (here and elsewhere).

[shauvik, 2011/12/16 07:19:27]

Done.

+    elements['green'].setAttribute('style', 'width:$pPass%');

[Kelly Norton, 2011/12/15 18:35:31]

Why do you use a map to store references to things like the 'green' element?
Would it make more sense to just have these be fields in this class?

[shauvik, 2011/12/16 07:19:27]

Actually everything is stored in maps. We need to update elements in the
corresponding map - inAppElements or fullAppElements. 

On 2011/12/15 18:35:31, Kelly Norton wrote:

[Kelly Norton, 2011/12/16 19:48:13]

I'm just saying that a map is definitely not the best thing to hold these. Now
you are essentially using a map for a standard structure. The classic static way
to do this would be:

interface Elements {
  Element get green();
  Element get red();
  Element get orange();
}

class InAppElements {
  Element red;
  Element green;
  Element orange;
  Element other;
}

class OutAppElements extends Elements {
  Element red;
  Element green;
  Element orange;
  Element otherother;
}

void updateTheView(Elements elements) {
  elements.green.textContent = 'boo';
}

You could also so it with class inheritance too. A simpler alternative is to
just pass in the references as parameters.

void updateTheView(Element green, Element red, Element orange) {
}

I think there are a lot of solutions here that would beat using a map as a
struct. 

+    var pFailed = pPass + (numTestsFailed / tests.length) * 100;
+    elements['red'].setAttribute('style', 'width:$pFailed%');
+    var pErrors = pFailed + (numTestsErrors / tests.length) * 100;
+    elements['orange'].setAttribute('style', 'width:$pErrors%');
+
+    // Update status
+    elements['testsRunElem'].innerText = '$numTestsRun';

[pdr, 2011/12/14 21:02:36]

There's no need to do this for your strings. Just do:
elements['testsRunElem'].innerText = numTestsRun;

[Kelly Norton, 2011/12/15 18:35:31]

You want to use textContent instead of innerText as innerText is not standard
and unsupported on mozilla browsers. Plus, textContent is faster and more
predictable.

[shauvik, 2011/12/16 07:19:27]

Yes, I did that to get rid of int-to-string warnings. Now changed to:

elements['testsRunElem'].innerText = numTestsRun.toString();

On 2011/12/14 21:02:36, pdr wrote:

[shauvik, 2011/12/16 07:19:27]

Cool, Thanks for the tip. I didn't know innerText wasn't supported in Firefox.

On 2011/12/15 18:35:31, Kelly Norton wrote:

+    elements['testsFailedElem'].innerText = '$numTestsFailed';
+    elements['testsErrorsElem'].innerText = '$numTestsErrors';
+  }
+  
+  String printDuration(Duration timeDuration) {
+    StringBuffer out = new StringBuffer();
+    if(timeDuration.inDays > 0){
+      out.add('${timeDuration.inDays} days ');
+    }
+    if(timeDuration.inHours > 0){
+      out.add('${timeDuration.inHours} hrs ');
+    }
+    if(timeDuration.inMinutes > 0){
+      out.add('${timeDuration.inMinutes} mins ');
+    }
+    if(timeDuration.inSeconds > 0){
+      out.add('${timeDuration.inSeconds} s ');
+    }
+    if(timeDuration.inMilliseconds > 0 || out.length == 0){
+      out.add('${timeDuration.inMilliseconds} ms');
+    }
+    return out.toString();
+  }
+
+  /** Populates the floating div with controls and toolbar */
+  HTMLDivElement renderMain(){
+    HTMLDivElement containerDiv = win.document.createElement('div');
+    containerDiv.className = 'dt-container';
+    appElements['containerDiv'] = containerDiv;
+    
+    // Add the test controls
+    HTMLDivElement mainElem = win.document.createElement('div');
+    mainElem.className = 'dt-main';
+    appElements['mainElem'] = mainElem;
+    
+    showTestControls();
+    
+    // Create header to hold window controls
+    if(inAppMode) {
+      HTMLDivElement headDiv = win.document.createElement('div');
+      headDiv.className = 'dt-header';
+      headDiv.innerHTML = '<b>DARTest: In-App View</b>';
+      HTMLImageElement close = win.document.createElement('img');
+      close.className = 'dt-header-close';
+      close.addEventListener('click', (Event){
+        containerDiv.className = 'dt-hide';
+      }, true);
+      HTMLImageElement pop = win.document.createElement('img');
+      pop.className = 'dt-header-pop';
+      pop.addEventListener('click', (Event) => dartestMaximize(), true);
+      HTMLImageElement minMax = win.document.createElement('img');
+      minMax.className = 'dt-header-min';
+      minMax.addEventListener('click', (Event) {
+        if (mainElem.classList.contains('dt-hide')){
+          mainElem.classList.remove('dt-hide');
+          mainElem.classList.add('dt-show');
+          minMax.className = 'dt-header-min';
+        } else {
+          if (mainElem.classList.contains('dt-show')){
+            mainElem.classList.remove('dt-show');
+          }
+          mainElem.classList.add('dt-hide');
+          minMax.className = 'dt-header-max';
+        }
+      }, true);
+      headDiv.appendChild(close);
+      headDiv.appendChild(pop);
+      headDiv.appendChild(minMax);
+      
+      containerDiv.appendChild(headDiv);
+    }
+
+    HTMLDivElement tabDiv = win.document.createElement('div');
+    tabDiv.className = 'dt-tab';
+    HTMLUListElement tabList = win.document.createElement('ul');
+    HTMLLIElement testingTab = win.document.createElement('li');
+    HTMLLIElement coverageTab = win.document.createElement('li');
+    testingTab.className = 'dt-tab-selected';
+    testingTab.innerText = 'Testing';
+    testingTab.addEventListener('click', (Event) {
+      showTestControls();
+      changeTabs(testingTab, coverageTab);
+    }, true);
+    tabList.appendChild(testingTab);
+    coverageTab.innerText = 'Coverage';
+    coverageTab.addEventListener('click', (Event) {
+      showCoverageControls();
+      changeTabs(coverageTab, testingTab);
+    }, true);
+    tabList.appendChild(coverageTab); 
+    tabDiv.appendChild(tabList);
+    containerDiv.appendChild(tabDiv);
+    
+    if(!inAppMode) {
+      HTMLDivElement popIn = win.document.createElement('div');
+      popIn.className = 'dt-minimize';
+      popIn.innerHTML = 'Pop In &#8690;';
+      popIn.addEventListener('click', (Event) => dartestMinimize(), true);
+      containerDiv.appendChild(popIn);
+    }  
+    
+    containerDiv.appendChild(mainElem);
+    win.document.body.appendChild(containerDiv);
+  }
+  
+  void changeTabs(HTMLLIElement clickedTab, HTMLLIElement oldTab){
+    oldTab.className = '';
+    clickedTab.className = 'dt-tab-selected';
+  }
+  
+  void showTestControls(){
+    HTMLDivElement testBody = appElements['testBody'];
+    if(testBody == null){
+      testBody = win.document.createElement('div');
+      appElements['testBody'] = testBody;
+    
+      // Create a toolbar to hold action buttons
+      HTMLDivElement toolDiv = win.document.createElement('div');
+      toolDiv.className = 'dt-toolbar';
+      HTMLButtonElement runBtn = win.document.createElement('button');
+      runBtn.innerHTML = '&#9658;';
+      runBtn.title = 'Run Tests';
+      runBtn.className = 'dt-button dt-run';
+      runBtn.addEventListener('click', (Event) {
+        log('Running tests');
+        uiUpdateFunc = updateDARTestUI;
+        runDARTests();
+      }, true);
+      toolDiv.appendChild(runBtn);
+      HTMLButtonElement exportBtn = win.document.createElement('button');
+      exportBtn.innerHTML = '&#8631;';
+      exportBtn.title = 'Export Results';
+      exportBtn.className = 'dt-button dt-run';
+      exportBtn.addEventListener('click', (Event e) {
+        log('Exporting results');
+        exportTestResults();
+      }, true);
+      toolDiv.appendChild(exportBtn);
+      testBody.appendChild(toolDiv);
+      
+      // Create a datalist element for showing test status"<td class='dt-pass'> PASS";
+      HTMLDListElement statList = win.document.createElement('dl');
+      statList.className = 'dt-status';
+      HTMLElement runsDt = win.document.createElement('dt');
+      runsDt.innerText = 'Runs:';
+      statList.appendChild(runsDt);
+      HTMLElement testsRunElem = win.document.createElement('dd');
+      appElements['testsRunElem'] = testsRunElem;
+      testsRunElem.innerText = '$numTestsRun';
+      statList.appendChild(testsRunElem);
+  
+      HTMLElement failDt = win.document.createElement('dt');
+      failDt.innerText = 'Failed:';
+      statList.appendChild(failDt);
+      HTMLElement testsFailedElem = win.document.createElement('dd');
+      appElements['testsFailedElem'] = testsFailedElem;
+      testsFailedElem.innerText = '$numTestsFailed';
+      statList.appendChild(testsFailedElem);
+  
+      HTMLElement errDt = win.document.createElement('dt');
+      errDt.innerText = 'Errors:';
+      statList.appendChild(errDt);
+      HTMLElement testsErrorsElem = win.document.createElement('dd');
+      appElements['testsErrorsElem'] = testsErrorsElem;
+      testsErrorsElem.innerText = '$numTestsErrors';
+      statList.appendChild(testsErrorsElem);  
+      testBody.appendChild(statList);
+      
+      // Create progressbar and add red, green, orange bars
+      HTMLDivElement progressDiv = win.document.createElement('div');
+      progressDiv.className = 'dt-progressbar';
+      progressDiv.innerHTML = "<span style='width:100%'></span>";
+      
+      HTMLSpanElement orange = win.document.createElement('span');
+      appElements['orange'] = orange;
+      orange.className = 'orange';
+      progressDiv.appendChild(orange);
+  
+      HTMLSpanElement red = win.document.createElement('span');
+      appElements['red'] = red;
+      red.className = 'red';
+      progressDiv.appendChild(red);
+      
+      HTMLSpanElement green = win.document.createElement('span');
+      appElements['green'] = green;
+      green.className = 'green';
+  
+      progressDiv.appendChild(green);
+      testBody.appendChild(progressDiv);
+      
+      HTMLDivElement hiddenElem = win.document.createElement('div');
+      hiddenElem.className = 'dt-hide';
+      hiddenElem.innerHTML = "<a id='dt-export' download='test_results.csv' href='#' />";
+      testBody.appendChild(hiddenElem);
+      
+      addChildToElem('mainElem', testBody);
+    }
+    
+    // Show hide divs
+    showHide('testBody', 'coverageBody');
+  } 
+  
+  void showCoverageControls(){
+    HTMLDivElement coverageBody = appElements['coverageBody'];
+    if(coverageBody == null){
+      coverageBody = win.document.createElement('div');
+      appElements['coverageBody'] = coverageBody;
+      
+      HTMLPreElement covPreElem = win.document.createElement('pre');
+      appElements['covPreElem'] = covPreElem;
+      coverageBody.appendChild(covPreElem);
+      
+      HTMLTableElement covTable = win.document.createElement('table');
+      covTable.className = 'dt-results';
+      HTMLTableSectionElement head = win.document.createElement('thead');
+      head.innerHTML = '<tr><th>Unit <th>Function <th>Statement <th>Branch';
+      covTable.appendChild(head);
+      HTMLTableSectionElement covTableBody = win.document.createElement('tbody');
+      appElements['covTableBody'] = covTableBody;
+          covTableBody.id = 'dt-results-body';
+          covTable.appendChild(covTableBody);
+      coverageBody.appendChild(covTable);
+      
+      addChildToElem('mainElem', coverageBody);
+    }
+    showHide('coverageBody', 'testBody');
+
+    appElements['covPreElem'].innerText = getCoverageSummary();
+        appElements['covTableBody'].innerHTML = getCoverageDetails();
+  }
+  
+  void showHide(String toShow, String toHide) {

[pdr, 2011/12/14 21:02:36]

I'd split this into show and hide.

[shauvik, 2011/12/16 07:19:27]

Done.

+    HTMLElement show = appElements[toShow];
+    HTMLElement hide = appElements[toHide];
+    if(show != null){
+      if(show.classList.contains('dt-hide')) {
+        show.classList.remove('dt-hide');
+      }
+      show.classList.add('dt-show');
+    }
+    if(hide != null){
+      if(hide.classList.contains('dt-show')){
+        hide.classList.remove('dt-show');
+      }
+      hide.classList.add('dt-hide');
+    }
+  }
+
+  void dartestMaximize(){
+    showHide('', 'containerDiv');
+    win = win.open('', 'dartest-window', 'width=600,height=750');
+    log('Window:'+win.toString());

[pdr, 2011/12/14 21:02:36]

I would remove these logging calls, or put them behind a flag.

[shauvik, 2011/12/16 07:19:27]

Done.

+    win.document.title = 'Dartest';
+    inAppMode = false;
+    fullAppElements = new HashMap<String, HTMLElement>();
+    appElements = fullAppElements;
+    DARTestCss.inject(win.document, false);
+    run();
+    if(numTestsRun > 0) {
+      populateResults();
+    }
+  }
+  
+  void populateResults() {

[pdr, 2011/12/14 21:02:36]

I think this can be:
tests.forEach(TestCase t) => updateDartTestUi(t);

[shauvik, 2011/12/16 07:19:27]

Thanks. Its slightly different that what you proposed.

tests.forEach((TestCase t) => _updateDARTestUI(t));

On 2011/12/14 21:02:36, pdr wrote:

+    for(TestCase t in tests) {
+      updateDARTestUI(t);
+    }
+  }
+  
+  void dartestMinimize(){
+    win.close();
+    win = appWindow;
+    inAppMode = true;
+    appElements = inAppElements;
+    showHide('containerDiv', '');
+  }
+
+  void exportTestResults(){
+    String csvData = getTestResultsCSV();
+    log(csvData);
+    HTMLAnchorElement exportLink = win.document.getElementById('dt-export');
+    
+    /** Bug: Can't instantiate WebKitBlobBuilder
+     *  If this bug is fixed, we can remove the urlencode and lpad function.
+    WebKitBlobBuilder bb = new WebKitBlobBuilder();
+    bb.append(csvData);
+    Blob blob = bb.getBlob('text/plain;charset=${document.characterSet}');
+    exportLink.href = window.webkitURL.createObjectURL(blob);
+    **/
+    
+    exportLink.href = 'data:text/csv,'+urlencode(csvData);
+    
+    MouseEvent ev = document.createEvent("MouseEvents");
+    ev.initMouseEvent("click", true, false, window, 0, 0, 0, 0, 0
+        , false, false, false, false, 0, null);
+    exportLink.dispatchEvent(ev);
+    
+  }
+
+  String urlencode(String s) {

[Kelly Norton, 2011/12/15 18:35:31]

How is the different from the native encodeURI?

[shauvik, 2011/12/16 07:19:27]

Actually I think encodeURI isn't supported in dart:dom or dart:html -- or
atleast I couldn't find it. So, I wrote this one by myself.

On 2011/12/15 18:35:31, Kelly Norton wrote:

+    StringBuffer out = new StringBuffer();
+    for(int i=0;i<s.length;i++) {
+      int cc = s.charCodeAt(i);
+      if((cc>=48 && cc<=57) || (cc>=65 && cc<=90) || (cc>=97 && cc<=122)) {
+        out.add(s[i]);
+      } else {
+        out.add('%${lpad(cc.toRadixString(16),2).toUpperCase()}');
+      }
+    }
+    return out.toString();
+  }
+
+  String lpad(String s, int n) {
+    if(s.length<n) {

[Kelly Norton, 2011/12/15 18:35:31]

s.length < n

Lots of other space problems in this file.

[shauvik, 2011/12/16 07:19:27]

Done.

+      for(int i=0;i<n-s.length;i++) {
+        s = '0'+s;
+      }
+    }
+    return s;
+  }
+}