Dom Navigation for htmlunit and firefox driver

[jiayao, 2008-05-07 22:20:48]

Just trying out this code review system by Guido :)

[Simon Stewart, 2008-05-08 11:02:15]

My essential point is that there shouldn't be a need for the "findChildElement"
methods because the existing "findElement" method could take an "ElementFinder"
(though that's a terrible name)

http://codereview.appspot.com/873/diff/1/10
File common/src/java/com/googlecode/webdriver/By.java (right):

http://codereview.appspot.com/873/diff/1/10#newcode43
Line 43: public WebElement findChildElement(WebElement element) {
I'd prefer to have "findElement" take a "ElementFinder" or something similar.
This would mean that we don't need to modify around with By, but would mean an
additional interface hanging off both WebDriver and WebElement

http://codereview.appspot.com/873/diff/1/9
File common/src/java/com/googlecode/webdriver/WebElement.java (right):

http://codereview.appspot.com/873/diff/1/9#newcode140
Line 140: List<WebElement> findElements(By by);
Extract "findElements" and "findElement" on to an interface and make WebDriver
and WebElement extend it.

http://codereview.appspot.com/873/diff/1/4
File firefox/src/extension/components/utils.js (right):

http://codereview.appspot.com/873/diff/1/4#newcode367
Line 367: var response = "";
This variable is unused.

http://codereview.appspot.com/873/diff/1/2
File firefox/src/java/com/googlecode/webdriver/firefox/FirefoxWebElement.java
(right):

http://codereview.appspot.com/873/diff/1/2#newcode187
Line 187: return findElementByXPath("*[@id = '" + id + "']");
This approach is going to be extremely slow on IE. It would be better to modify
the existing methods within JS code to use the document element as the root
element, pull the creation of the child elements into a sub-method and then have
these be straight calls through to the new firefox methods. I think that makes
sense, but let me know if it means nothing to you :)

[jiayao, 2008-05-08 20:21:57]

Thanks Simon, I have replied you comments and will be working the proposed
changes tomorrow.

http://codereview.appspot.com/873/diff/1/10
File common/src/java/com/googlecode/webdriver/By.java (right):

http://codereview.appspot.com/873/diff/1/10#newcode43
Line 43: public WebElement findChildElement(WebElement element) {
I agree. Will do that.

http://codereview.appspot.com/873/diff/1/9
File common/src/java/com/googlecode/webdriver/WebElement.java (right):

http://codereview.appspot.com/873/diff/1/9#newcode140
Line 140: List<WebElement> findElements(By by);
Make sense, I will do that.

http://codereview.appspot.com/873/diff/1/4
File firefox/src/extension/components/utils.js (right):

http://codereview.appspot.com/873/diff/1/4#newcode367
Line 367: var response = "";
On 2008/05/08 11:02:16, simon.m.stewart wrote:
> This variable is unused.

Done.

http://codereview.appspot.com/873/diff/1/2
File firefox/src/java/com/googlecode/webdriver/firefox/FirefoxWebElement.java
(right):

http://codereview.appspot.com/873/diff/1/2#newcode187
Line 187: return findElementByXPath("*[@id = '" + id + "']");
I just had a second thought about this. We shouldn't support finding multiple
elements with same Ids anyway. How about I just reuse selectElementById in
firefoxDriver.js ?
And for findElementsById, just throw UnsupportedOperationException.
For findElementsByName, the FirefoxDriver simply doesnt support it, should I do
the same or emulate it with findByXpath?

[Simon Stewart, 2008-05-08 21:07:37]

http://codereview.appspot.com/873/diff/1/2
File firefox/src/java/com/googlecode/webdriver/firefox/FirefoxWebElement.java
(right):

http://codereview.appspot.com/873/diff/1/2#newcode187
Line 187: return findElementByXPath("*[@id = '" + id + "']");
On 2008/05/08 20:21:58, Jiayao.Yu wrote:
> I just had a second thought about this. We shouldn't support finding multiple
> elements with same Ids anyway. How about I just reuse selectElementById in
> firefoxDriver.js ?
> And for findElementsById, just throw UnsupportedOperationException.
> For findElementsByName, the FirefoxDriver simply doesnt support it, should I
do
> the same or emulate it with findByXpath?

Although technically it's an error to have elements with the same id, people
still do it, so we should support it anyway. Reusing "selectElementById" is a
very good idea (though we should check that any found nodes are descendants of
the root node), and we can certainly emulate finding by name if that's what we
need to do.

[jiayao, 2008-05-09 13:36:40]

Hey Simon, I've made the changes proposed. Could you have another look?

[Simon Stewart, 2008-05-09 14:17:02]

The ElementFinder doesn't appear to be included in the svn diff. Hmmm....
Thinking about it, a better name for ElementFinder may be "BaseOfElementSearch"
--- I think that better describes the role that the interface plays in the
system, but I'm happy to be talked out it :) Robert Chatley tends to be good at
naming things, so it may be worth talking with him too.

[jiayao, 2008-05-09 15:38:12]

Hi Simon,

As we discussed, I have moved SearchContext down to the concrete driver level.
However, to keep WebDriver.findElements to continue working, I have to leave
findElements in WebDriver and WebElement. Seems a bit naughty, because it's
unclear where are the findElement functions in the drivers are implementing
from. What do you think?
And I have deprecated By.findElements(WebDriver) while adding
By.findElements(SearchContext) .  Although the new API requires a cast, I don't
think it is a problem because most user code dont use this API directly anyway.