Initial check-in of the code.

Domita autotest

[Simon Stewart, 2009-06-29 13:18:48]

This looks pretty good to me. A couple of minor comments in the review, but
there's nothing major :)

http://codereview.appspot.com/87067/diff/1/4
File tests/com/google/caja/plugin/DomitaTest.java (right):

http://codereview.appspot.com/87067/diff/1/4#newcode80
Line 80: driver.findElements(By.xpath("//*[contains(@class,'clickme')]/*"));
If you find this slow in IE, then you can use:

WebElement parent = driver.findElement(By.className("clickme"));
List<WebElement> clickingList = parent.findElements(By.xpath("*"));

The first lookup will be fast, and you're then only scanning a subset of the DOM
for the child nodes.

http://codereview.appspot.com/87067/diff/1/4#newcode99
Line 99: //driver.close();
driver.quit() will properly release all the resources. "close" only closes a
single window (which won't quit the browser if more than one window is open)

[ihab.awad, 2009-06-29 23:24:48]

http://codereview.appspot.com/87067/diff/1/28
File build.xml (right):

http://codereview.appspot.com/87067/diff/1/28#newcode81
Line 81: <pathelement
path="${third_party}/java/webdriver/commons-collections-3.2.1.jar"/>
Can you reformat to 80 cols if possible? We've been using that so far for most
of our files.

http://codereview.appspot.com/87067/diff/1/4
File tests/com/google/caja/plugin/DomitaTest.java (right):

http://codereview.appspot.com/87067/diff/1/4#newcode1
Line 1: // Copyright 2009 Google Inc. All Rights Reserved.
Please copy and paste in a full copyright header from one of the other Java
files in our repo.

http://codereview.appspot.com/87067/diff/1/4#newcode34
Line 34: {
Code style: Move opening brace to previous line. (Also in a few other places.)

http://codereview.appspot.com/87067/diff/1/4#newcode45
Line 45: assertTrue("Starting the local web server failed!", false);
Instead of assertTrue(msg, false), can use fail(msg).

http://codereview.appspot.com/87067/diff/1/4#newcode73
Line 73: driver.get("http://localhost:8000/" +
Code style: we usually put the operator at the start of the following line. So
like:

  driver.get("http://localhost:8000/"
             + "ant-lib/com/google/caja/plugin/domita_test.html");

or:

  driver.get(
    "http://localhost:8000/"
    + "ant-lib/com/google/caja/plugin/domita_test.html");

http://codereview.appspot.com/87067/diff/1/2
File tests/com/google/caja/plugin/domita_test.html (right):

http://codereview.appspot.com/87067/diff/1/2#newcode284
Line 284: node.className.substring(0, 8) == 'clickme-')
Code style: In JS, we always use "===" unless we really *mean* "==", since the
JS "==" operator has nontrivial semantics.

http://codereview.appspot.com/87067/diff/1/2#newcode285
Line 285: node.className = node.className.substring(8);
Code style: We always put the body of a conditional inside { ... } braces.

[maoziqing, 2009-06-30 16:53:46]

Thank you for the review.

It seems findElements(By.className("clickme")) does not work correctly. It
returns an empty list.

Best,
Ziqing

On 2009/06/29 13:18:48, Simon Stewart wrote:
> This looks pretty good to me. A couple of minor comments in the review, but
> there's nothing major :)
> 
> http://codereview.appspot.com/87067/diff/1/4
> File tests/com/google/caja/plugin/DomitaTest.java (right):
> 
> http://codereview.appspot.com/87067/diff/1/4#newcode80
> Line 80: driver.findElements(By.xpath("//*[contains(@class,'clickme')]/*"));
> If you find this slow in IE, then you can use:
> 
> WebElement parent = driver.findElement(By.className("clickme"));
> List<WebElement> clickingList = parent.findElements(By.xpath("*"));
> 
> The first lookup will be fast, and you're then only scanning a subset of the
DOM
> for the child nodes.
> 
> http://codereview.appspot.com/87067/diff/1/4#newcode99
> Line 99: //driver.close();
> driver.quit() will properly release all the resources. "close" only closes a
> single window (which won't quit the browser if more than one window is open)

[ihab.awad, 2009-06-30 20:15:34]

LGTM with one final code style comment. Thanks!

http://codereview.appspot.com/87067/diff/2004/3004
File tests/com/google/caja/plugin/domita_test.html (right):

http://codereview.appspot.com/87067/diff/2004/3004#newcode291
Line 291: node.className = node.className.substring(8); }
Code style: Please move closing brace to next line.

[maoziqing, 2009-06-30 20:33:36]

test

http://codereview.appspot.com/87067/diff/1/4
File tests/com/google/caja/plugin/DomitaTest.java (right):

http://codereview.appspot.com/87067/diff/1/4#newcode1
Line 1: // Copyright 2009 Google Inc. All Rights Reserved.
On 2009/06/29 23:24:48, ihab.awad wrote:
> Please copy and paste in a full copyright header from one of the other Java
> files in our repo.

Done.

http://codereview.appspot.com/87067/diff/1/4#newcode34
Line 34: {
On 2009/06/29 23:24:48, ihab.awad wrote:
> Code style: Move opening brace to previous line. (Also in a few other places.)

test

[maoziqing, 2009-06-30 20:43:05]

Committed revision 3549