New patch for web SQL database in Android Webdriver

[Eran, 2011-07-20 12:11:14]

Hi,
That's not a complete review - after the issues raised here are addressed, I'll
do a more through one.

http://codereview.appspot.com/4802041/diff/1/android/src/java/org/openqa/sele...
File android/src/java/org/openqa/selenium/android/AndroidDriver.java (right):

http://codereview.appspot.com/4802041/diff/1/android/src/java/org/openqa/sele...
android/src/java/org/openqa/selenium/android/AndroidDriver.java:68: FindsById,
FindsByLinkText, FindsByName, FindsByXPath, TakesScreenshot,
Why is the support for BrowserConnection going away?

http://codereview.appspot.com/4802041/diff/1/android/src/java/org/openqa/sele...
android/src/java/org/openqa/selenium/android/AndroidDriver.java:633: }
catch(JSONException e) {
Under which conditions can this exception be thrown? Shouldn't we throw a new
WebDriverException if execution of the SQL statement failed due to an unknown
reason?

http://codereview.appspot.com/4802041/diff/1/java/client/src/org/openqa/selen...
File java/client/src/org/openqa/selenium/android/AndroidDriver.java (right):

http://codereview.appspot.com/4802041/diff/1/java/client/src/org/openqa/selen...
java/client/src/org/openqa/selenium/android/AndroidDriver.java:90: 
These methods are protected in the superclass - why extend the visibility?
Nothing outside of the RemoteWebDriver (or its subclasses) should be executing
commands directly.

http://codereview.appspot.com/4802041/diff/1/java/client/test/org/openqa/sele...
File java/client/test/org/openqa/selenium/html5/HTML5Test.java (right):

http://codereview.appspot.com/4802041/diff/1/java/client/test/org/openqa/sele...
java/client/test/org/openqa/selenium/html5/HTML5Test.java:48: 
If you're disabling a test, please:
1. Do it in a separate change.
2. Open an issue to document the failure of this feature to work. Bonus points
if you assign it to yourself and fix it.

According to codesearch, there are a couple of tests using this feature. Please
verify that we're not breaking existing functionality here.

http://codereview.appspot.com/4802041/diff/1/java/client/test/org/openqa/sele...
java/client/test/org/openqa/selenium/html5/HTML5Test.java:211: AppCacheStatus
status = ((ApplicationCache) driver).getStatus();
Why has the expected status changed?

http://codereview.appspot.com/4802041/diff/1/java/client/test/org/openqa/sele...
java/client/test/org/openqa/selenium/html5/HTML5Test.java:214: // Check if
resources are retrieved from browser's cache.
Does this cast succeed, given that you've removed this interface from the
AndroidDriver class?

[berrada, 2011-07-20 17:48:53]

Hi Nayeem,

I see 2 CLs for DB support. I 've published my changes on the previous one.
Please merge the two CL into one.

[Nayeem, 2011-08-05 20:21:05]

http://codereview.appspot.com/4802041/diff/1/android/src/java/org/openqa/sele...
File android/src/java/org/openqa/selenium/android/AndroidDriver.java (right):

http://codereview.appspot.com/4802041/diff/1/android/src/java/org/openqa/sele...
android/src/java/org/openqa/selenium/android/AndroidDriver.java:68: FindsById,
FindsByLinkText, FindsByName, FindsByXPath, TakesScreenshot,
On 2011/07/20 12:11:14, Eran wrote:
> Why is the support for BrowserConnection going away?

There was a bug in the original browserconnection implementation. I fixed that
bug and submitted a different CL. In this CL, I am restoring these removals.

http://codereview.appspot.com/4802041/diff/1/android/src/java/org/openqa/sele...
android/src/java/org/openqa/selenium/android/AndroidDriver.java:633: }
catch(JSONException e) {
On 2011/07/20 12:11:14, Eran wrote:
> Under which conditions can this exception be thrown? Shouldn't we throw a new
> WebDriverException if execution of the SQL statement failed due to an unknown
> reason?

Done.

http://codereview.appspot.com/4802041/diff/1/java/client/src/org/openqa/selen...
File java/client/src/org/openqa/selenium/android/AndroidDriver.java (right):

http://codereview.appspot.com/4802041/diff/1/java/client/src/org/openqa/selen...
java/client/src/org/openqa/selenium/android/AndroidDriver.java:90: 
On 2011/07/20 12:11:14, Eran wrote:
> These methods are protected in the superclass - why extend the visibility?
> Nothing outside of the RemoteWebDriver (or its subclasses) should be executing
> commands directly.

For this CL, it is not required to expose this visibility. I will update with a
following patchset. 

But, I will need to access this function externally for implementations of
LocalStorage and SessionStorage. These storage classes are variables will be
used as variables of the Webdriver instances. To provide access to these
"execute" method, we might need to modify the RemoteWebdriver or the individual
browser-specific Webdriver instances.

http://codereview.appspot.com/4802041/diff/1/java/client/test/org/openqa/sele...
File java/client/test/org/openqa/selenium/html5/HTML5Test.java (right):

http://codereview.appspot.com/4802041/diff/1/java/client/test/org/openqa/sele...
java/client/test/org/openqa/selenium/html5/HTML5Test.java:48: 
On 2011/07/20 12:11:14, Eran wrote:
> If you're disabling a test, please:
> 1. Do it in a separate change.
> 2. Open an issue to document the failure of this feature to work. Bonus points
> if you assign it to yourself and fix it.
> 
> According to codesearch, there are a couple of tests using this feature.
Please
> verify that we're not breaking existing functionality here.

In the modified patchset, I am uncommenting them. They should be uncommented.

http://codereview.appspot.com/4802041/diff/1/java/client/test/org/openqa/sele...
java/client/test/org/openqa/selenium/html5/HTML5Test.java:211: AppCacheStatus
status = ((ApplicationCache) driver).getStatus();
On 2011/07/20 12:11:14, Eran wrote:
> Why has the expected status changed? 

I will send this change as a separate CL. To answer your question:
For this test, there is an appcache associated with the host and it is valid. So
the status cannot be UNCACHED. Whether or not the status is CHECKING,
UPDATEREADY, etc, it might depend on the time. But given the test with Android
and Chrome driver with 4s delay, the status is IDLE.  The appcache load finishes
during this period.

http://codereview.appspot.com/4802041/diff/1/java/client/test/org/openqa/sele...
java/client/test/org/openqa/selenium/html5/HTML5Test.java:214: // Check if
resources are retrieved from browser's cache.
On 2011/07/20 12:11:14, Eran wrote:
> Does this cast succeed, given that you've removed this interface from the
> AndroidDriver class?

It will work. Since the AndroidWebdriver of this CL does not implement the
ApplicationCache