Implement window resize and move for Firefox and Ruby + Java.

This adds two resources to the wire protocol:
    
      /session/:sessionId/window/:windowHandle/size
      /session/:sessionId/window/:windowHandle/location
    
    Both resources accept GET and POST requests to get and set the values.

[jleyba, 2011-11-05 01:52:33]

Mostly style nits

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

http://codereview.appspot.com/5342045/diff/1/java/client/src/org/openqa/selen...
java/client/src/org/openqa/selenium/WebDriver.java:235: Window window();
Javadoc on all public APIs

http://codereview.appspot.com/5342045/diff/1/java/client/src/org/openqa/selen...
java/client/src/org/openqa/selenium/WebDriver.java:439: interface Window {
Javadoc

http://codereview.appspot.com/5342045/diff/1/java/client/src/org/openqa/selen...
java/client/src/org/openqa/selenium/WebDriver.java:440: void setSize(int width,
int height);
I'd only specify setSize(Dimension) on the core interface.

http://codereview.appspot.com/5342045/diff/1/java/client/src/org/openqa/selen...
java/client/src/org/openqa/selenium/WebDriver.java:443: void setLocation(int x,
int y);
Drop

http://codereview.appspot.com/5342045/diff/1/java/client/src/org/openqa/selen...
File java/client/src/org/openqa/selenium/remote/HttpCommandExecutor.java
(right):

http://codereview.appspot.com/5342045/diff/1/java/client/src/org/openqa/selen...
java/client/src/org/openqa/selenium/remote/HttpCommandExecutor.java:282:
.put(GET_WINDOW_SIZE, get("/session/:sessionId/window/:windowHandle/size"))
So we're going to allow changing the size/location of windows that are not
currently selected?

http://codereview.appspot.com/5342045/diff/1/java/client/src/org/openqa/selen...
File java/client/src/org/openqa/selenium/remote/RemoteWebDriver.java (right):

http://codereview.appspot.com/5342045/diff/1/java/client/src/org/openqa/selen...
java/client/src/org/openqa/selenium/remote/RemoteWebDriver.java:559:
ImmutableMap.of("windowHandle", "current", "width", width, "height", height));
Style-nit: In java we typically indent 4 spaces on wrap instead of chopping down
(there's a couple of edits above that could be rolled back with this)

http://codereview.appspot.com/5342045/diff/1/java/client/src/org/openqa/selen...
java/client/src/org/openqa/selenium/remote/RemoteWebDriver.java:572:
setLocation(targetLocation.getX(), targetLocation.getY());
Why does Point use accessors for x and y, but Dimension exposes width and height
as public properties?  Could we do the same for Point since it's a basic type

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

http://codereview.appspot.com/5342045/diff/1/java/client/test/org/openqa/sele...
java/client/test/org/openqa/selenium/SingleTestSuite.java:82:
.onlyRun("WindowTest")
Svn log has so much churn for this file :)

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

http://codereview.appspot.com/5342045/diff/1/java/client/test/org/openqa/sele...
java/client/test/org/openqa/selenium/WindowTest.java:47: int targetWidth =
size.width - 20;
Might want to comment that the target size is relative to starting since we
don't control the window's initial size.

http://codereview.appspot.com/5342045/diff/1/javascript/atoms/window.js
File javascript/atoms/window.js (right):

http://codereview.appspot.com/5342045/diff/1/javascript/atoms/window.js#newco...
javascript/atoms/window.js:147: /**
style nit: 2 blank lines between top-level definitions in closure-style code

http://codereview.appspot.com/5342045/diff/1/javascript/atoms/window.js#newco...
javascript/atoms/window.js:163: /** Set the outer size of the window.
Move down a line.

http://codereview.appspot.com/5342045/diff/1/javascript/atoms/window.js#newco...
javascript/atoms/window.js:165: * @param {!goog.math.Size=} size The new window
size.
Drop the "=" suffix, that is used to indicate the parameter is optional.  That's
actually all the Closure compiler requires to identify an optional parameter,
but Google JS style requires the param name to have the "opt_" prefix, so you'll
see that throughout Closure-based code.

http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml

http://codereview.appspot.com/5342045/diff/1/javascript/atoms/window.js#newco...
javascript/atoms/window.js:175: /** Get the location of the window.
Move down a line.

http://codereview.appspot.com/5342045/diff/1/javascript/atoms/window.js#newco...
javascript/atoms/window.js:196: /** Set the location of the window.
Move down a line.

http://codereview.appspot.com/5342045/diff/1/javascript/firefox-driver/extens...
File javascript/firefox-driver/extension/components/firefoxDriver.js (right):

http://codereview.appspot.com/5342045/diff/1/javascript/firefox-driver/extens...
javascript/firefox-driver/extension/components/firefoxDriver.js:1297:
if(parameters.windowHandle != "current") {
You should be able to extract this check to a common function and apply it to
all 4 operations as a precondition.

FirefoxDriver.prototype.getWindowSize.preconditions = [
    fxdriver.preconditions.targetsCurrentWindow ];

http://codereview.appspot.com/5342045/diff/1/wire.py
File wire.py (right):

http://codereview.appspot.com/5342045/diff/1/wire.py#newcode661
wire.py:661: AddJsonParameter('x', '{number}', 'The X coordinate to position the
window at.').
We should specify that coordinates are relative to the upper left corner of the
screen.

[jarib, 2011-11-05 03:04:50]

Thanks Jason! What's the thing to do after addressing your comments - should I
create a new review with the updated patch? Is there some way to amend this one?

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

http://codereview.appspot.com/5342045/diff/1/java/client/src/org/openqa/selen...
java/client/src/org/openqa/selenium/WebDriver.java:235: Window window();
On 2011/11/05 01:52:33, jleyba wrote:
> Javadoc on all public APIs

Done.

http://codereview.appspot.com/5342045/diff/1/java/client/src/org/openqa/selen...
java/client/src/org/openqa/selenium/WebDriver.java:439: interface Window {
On 2011/11/05 01:52:33, jleyba wrote:
> Javadoc

Done.

http://codereview.appspot.com/5342045/diff/1/java/client/src/org/openqa/selen...
java/client/src/org/openqa/selenium/WebDriver.java:440: void setSize(int width,
int height);
On 2011/11/05 01:52:33, jleyba wrote:
> I'd only specify setSize(Dimension) on the core interface.

Done.

http://codereview.appspot.com/5342045/diff/1/java/client/src/org/openqa/selen...
java/client/src/org/openqa/selenium/WebDriver.java:443: void setLocation(int x,
int y);
On 2011/11/05 01:52:33, jleyba wrote:
> Drop

Done.

http://codereview.appspot.com/5342045/diff/1/java/client/src/org/openqa/selen...
File java/client/src/org/openqa/selenium/remote/HttpCommandExecutor.java
(right):

http://codereview.appspot.com/5342045/diff/1/java/client/src/org/openqa/selen...
java/client/src/org/openqa/selenium/remote/HttpCommandExecutor.java:282:
.put(GET_WINDOW_SIZE, get("/session/:sessionId/window/:windowHandle/size"))
Simon wanted to keep that option open at the wire level on the mailing list:

"The URL should be " /session/:sessionId/window/:windowHandle/..." to
allow us room in the future to deal with windows that are not
currently focussed."

http://codereview.appspot.com/5342045/diff/1/java/client/src/org/openqa/selen...
File java/client/src/org/openqa/selenium/remote/RemoteWebDriver.java (right):

http://codereview.appspot.com/5342045/diff/1/java/client/src/org/openqa/selen...
java/client/src/org/openqa/selenium/remote/RemoteWebDriver.java:559:
ImmutableMap.of("windowHandle", "current", "width", width, "height", height));
I *think* this was done by Google's IntelliJ code style :) I'll roll these back.

http://codereview.appspot.com/5342045/diff/1/java/client/src/org/openqa/selen...
java/client/src/org/openqa/selenium/remote/RemoteWebDriver.java:559:
ImmutableMap.of("windowHandle", "current", "width", width, "height", height));
On 2011/11/05 01:52:33, jleyba wrote:
> Style-nit: In java we typically indent 4 spaces on wrap instead of chopping
down
> (there's a couple of edits above that could be rolled back with this)

Done.

http://codereview.appspot.com/5342045/diff/1/java/client/src/org/openqa/selen...
java/client/src/org/openqa/selenium/remote/RemoteWebDriver.java:572:
setLocation(targetLocation.getX(), targetLocation.getY());
You're right, Point already has these as public fields. Will fix.

http://codereview.appspot.com/5342045/diff/1/java/client/src/org/openqa/selen...
java/client/src/org/openqa/selenium/remote/RemoteWebDriver.java:572:
setLocation(targetLocation.getX(), targetLocation.getY());
On 2011/11/05 01:52:33, jleyba wrote:
> Why does Point use accessors for x and y, but Dimension exposes width and
height
> as public properties?  Could we do the same for Point since it's a basic type

Done.

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

http://codereview.appspot.com/5342045/diff/1/java/client/test/org/openqa/sele...
java/client/test/org/openqa/selenium/WindowTest.java:47: int targetWidth =
size.width - 20;
On 2011/11/05 01:52:33, jleyba wrote:
> Might want to comment that the target size is relative to starting since we
> don't control the window's initial size.

Done.

http://codereview.appspot.com/5342045/diff/1/javascript/atoms/window.js
File javascript/atoms/window.js (right):

http://codereview.appspot.com/5342045/diff/1/javascript/atoms/window.js#newco...
javascript/atoms/window.js:147: /**
On 2011/11/05 01:52:33, jleyba wrote:
> style nit: 2 blank lines between top-level definitions in closure-style code

Done.

http://codereview.appspot.com/5342045/diff/1/javascript/atoms/window.js#newco...
javascript/atoms/window.js:163: /** Set the outer size of the window.
On 2011/11/05 01:52:33, jleyba wrote:
> Move down a line.

Done.

http://codereview.appspot.com/5342045/diff/1/javascript/atoms/window.js#newco...
javascript/atoms/window.js:165: * @param {!goog.math.Size=} size The new window
size.
On 2011/11/05 01:52:33, jleyba wrote:
> Drop the "=" suffix, that is used to indicate the parameter is optional. 
That's
> actually all the Closure compiler requires to identify an optional parameter,
> but Google JS style requires the param name to have the "opt_" prefix, so
you'll
> see that throughout Closure-based code.
> 
> http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml

Done.

http://codereview.appspot.com/5342045/diff/1/javascript/atoms/window.js#newco...
javascript/atoms/window.js:175: /** Get the location of the window.
On 2011/11/05 01:52:33, jleyba wrote:
> Move down a line.

Done.

http://codereview.appspot.com/5342045/diff/1/javascript/atoms/window.js#newco...
javascript/atoms/window.js:196: /** Set the location of the window.
On 2011/11/05 01:52:33, jleyba wrote:
> Move down a line.

Done.

http://codereview.appspot.com/5342045/diff/1/javascript/firefox-driver/extens...
File javascript/firefox-driver/extension/components/firefoxDriver.js (right):

http://codereview.appspot.com/5342045/diff/1/javascript/firefox-driver/extens...
javascript/firefox-driver/extension/components/firefoxDriver.js:1297:
if(parameters.windowHandle != "current") {
That complains about fxdriver not being defined, probably for the same reason 
the alert preconditions are set up in the FirefoxDriver constructor, and I can't
do the same here (I think) since I need the command parameters. 

I'll just extract the check to
FirefoxDriver.prototype.assertTargetsCurrentWindow_ for now.

http://codereview.appspot.com/5342045/diff/1/wire.py
File wire.py (right):

http://codereview.appspot.com/5342045/diff/1/wire.py#newcode661
wire.py:661: AddJsonParameter('x', '{number}', 'The X coordinate to position the
window at.').
On 2011/11/05 01:52:33, jleyba wrote:
> We should specify that coordinates are relative to the upper left corner of
the
> screen.

Done.

[dawagner, 2011-11-05 13:22:48]

http://codereview.appspot.com/5342045/diff/1/java/client/src/org/openqa/selen...
File java/client/src/org/openqa/selenium/remote/RemoteWebDriver.java (right):

http://codereview.appspot.com/5342045/diff/1/java/client/src/org/openqa/selen...
java/client/src/org/openqa/selenium/remote/RemoteWebDriver.java:577: Response
response = execute(DriverCommand.GET_WINDOW_SIZE,
ImmutableMap.of("windowHandle", "current"));
Line length > 100 ( (

http://codereview.appspot.com/5342045/diff/1/java/client/src/org/openqa/selen...
java/client/src/org/openqa/selenium/remote/RemoteWebDriver.java:588: Response
response = execute(DriverCommand.GET_WINDOW_LOCATION,
ImmutableMap.of("windowHandle", "current"));
Line length

http://codereview.appspot.com/5342045/diff/1/javascript/firefox-driver/extens...
File javascript/firefox-driver/extension/components/firefoxDriver.js (right):

http://codereview.appspot.com/5342045/diff/1/javascript/firefox-driver/extens...
javascript/firefox-driver/extension/components/firefoxDriver.js:1321:
if(parameters.windowHandle != "current") {
Space after if

[jarib, 2011-11-05 13:25:58]

http://codereview.appspot.com/5342045/diff/1/java/client/src/org/openqa/selen...
File java/client/src/org/openqa/selenium/remote/RemoteWebDriver.java (right):

http://codereview.appspot.com/5342045/diff/1/java/client/src/org/openqa/selen...
java/client/src/org/openqa/selenium/remote/RemoteWebDriver.java:577: Response
response = execute(DriverCommand.GET_WINDOW_SIZE,
ImmutableMap.of("windowHandle", "current"));
On 2011/11/05 13:22:48, dawagner wrote:
> Line length > 100 ( (

Done.

http://codereview.appspot.com/5342045/diff/1/java/client/src/org/openqa/selen...
java/client/src/org/openqa/selenium/remote/RemoteWebDriver.java:588: Response
response = execute(DriverCommand.GET_WINDOW_LOCATION,
ImmutableMap.of("windowHandle", "current"));
On 2011/11/05 13:22:48, dawagner wrote:
> Line length

Done.

http://codereview.appspot.com/5342045/diff/1/javascript/firefox-driver/extens...
File javascript/firefox-driver/extension/components/firefoxDriver.js (right):

http://codereview.appspot.com/5342045/diff/1/javascript/firefox-driver/extens...
javascript/firefox-driver/extension/components/firefoxDriver.js:1321:
if(parameters.windowHandle != "current") {
On 2011/11/05 13:22:48, dawagner wrote:
> Space after if

Done.

[luke.semerau, 2011-11-05 16:20:23]

http://codereview.appspot.com/5342045/diff/11001/javascript/atoms/window.js
File javascript/atoms/window.js (right):

http://codereview.appspot.com/5342045/diff/11001/javascript/atoms/window.js#n...
javascript/atoms/window.js:174: win.resizeTo(size.width, size.height);
This won't work for FF7? ~ if it's the main window. Am I missing something? Or
are we just planning on not supporting resizing of the main window (because the
browsers don't allow it)?

[jarib, 2011-11-05 16:25:04]

http://codereview.appspot.com/5342045/diff/11001/javascript/atoms/window.js
File javascript/atoms/window.js (right):

http://codereview.appspot.com/5342045/diff/11001/javascript/atoms/window.js#n...
javascript/atoms/window.js:174: win.resizeTo(size.width, size.height);
On 2011/11/05 16:20:23, luke.semerau wrote:
> This won't work for FF7? ~ if it's the main window. Am I missing something? Or
> are we just planning on not supporting resizing of the main window (because
the
> browsers don't allow it)?

This works fine in FF7. The extension has more privileges when it comes to
moving/resizing than the normal executeScript calls - this is the reason I'm
adding the new API.

[luke.semerau, 2011-11-05 16:36:24]

Ah, good :)

I have a patch for python ready to add this too. 

-Luke

On Nov 5, 2011, at 9:25 AM, jari.bakken@gmail.com wrote:

> 
> http://codereview.appspot.com/5342045/diff/11001/javascript/atoms/window.js
> File javascript/atoms/window.js (right):
> 
>
http://codereview.appspot.com/5342045/diff/11001/javascript/atoms/window.js#n...
> javascript/atoms/window.js:174: win.resizeTo(size.width, size.height);
> On 2011/11/05 16:20:23, luke.semerau wrote:
>> This won't work for FF7? ~ if it's the main window. Am I missing
> something? Or
>> are we just planning on not supporting resizing of the main window
> (because the
>> browsers don't allow it)?
> 
> This works fine in FF7. The extension has more privileges when it comes
> to moving/resizing than the normal executeScript calls - this is the
> reason I'm adding the new API.
> 
> http://codereview.appspot.com/5342045/

[dawagner, 2011-11-05 20:51:18]

http://codereview.appspot.com/5342045/diff/11001/javascript/firefox-driver/ex...
File javascript/firefox-driver/extension/components/firefoxDriver.js (right):

http://codereview.appspot.com/5342045/diff/11001/javascript/firefox-driver/ex...
javascript/firefox-driver/extension/components/firefoxDriver.js:1334: //
TODO(jari): could this be made into a precondition?
IIRC either of the following will work for preconditions:
1) Actually compile the firefoxdriver using closure, which Simon is working on
at the moment
2) In the FirefoxDriver constructor, do the precondition setting, as is done for
some of the alerts methods at the moment.  I don't like this, as it's not very
discoverable, and a runtime rather than compile-time thing, but it would work :)

[jarib, 2011-11-05 22:02:07]

http://codereview.appspot.com/5342045/diff/11001/javascript/firefox-driver/ex...
File javascript/firefox-driver/extension/components/firefoxDriver.js (right):

http://codereview.appspot.com/5342045/diff/11001/javascript/firefox-driver/ex...
javascript/firefox-driver/extension/components/firefoxDriver.js:1334: //
TODO(jari): could this be made into a precondition?
On 2011/11/05 20:51:18, dawagner wrote:
> IIRC either of the following will work for preconditions:
> 1) Actually compile the firefoxdriver using closure, which Simon is working on
> at the moment
> 2) In the FirefoxDriver constructor, do the precondition setting, as is done
for
> some of the alerts methods at the moment.  I don't like this, as it's not very
> discoverable, and a runtime rather than compile-time thing, but it would work
:)

I'm not sure how I'd get hold of the command parameters from the constructor
though. Seems like waiting for Simon's work would make sense :)

[simonstewart, 2011-11-07 15:39:51]

http://codereview.appspot.com/5342045/diff/20001/java/client/src/org/openqa/s...
File java/client/src/org/openqa/selenium/WebDriver.java (right):

http://codereview.appspot.com/5342045/diff/20001/java/client/src/org/openqa/s...
java/client/src/org/openqa/selenium/WebDriver.java:238: Window window();
Annotate with @Beta please.

http://codereview.appspot.com/5342045/diff/20001/java/client/src/org/openqa/s...
java/client/src/org/openqa/selenium/WebDriver.java:445: * not just the view
port.
Do you want to include that this is synonymous with window.resize() in JS?

http://codereview.appspot.com/5342045/diff/20001/java/client/src/org/openqa/s...
java/client/src/org/openqa/selenium/WebDriver.java:452: * Set the location of
the current window, relative to the upper left corner of the screen.
Do you want to include that this is the same as window.resizeTo() in JS?

http://codereview.appspot.com/5342045/diff/20001/javascript/atoms/window.js
File javascript/atoms/window.js (right):

http://codereview.appspot.com/5342045/diff/20001/javascript/atoms/window.js#n...
javascript/atoms/window.js:184: bot.window.getLocation = function(opt_win) {
What's the risk of this being mistaken for "window.location"?
bot.window.getPosition?

[jarib, 2011-11-07 16:13:15]

Added @Beta, s/location/position/

http://codereview.appspot.com/5342045/diff/20001/java/client/src/org/openqa/s...
File java/client/src/org/openqa/selenium/WebDriver.java (right):

http://codereview.appspot.com/5342045/diff/20001/java/client/src/org/openqa/s...
java/client/src/org/openqa/selenium/WebDriver.java:238: Window window();
On 2011/11/07 15:39:52, simonstewart wrote:
> Annotate with @Beta please.

Done.

http://codereview.appspot.com/5342045/diff/20001/java/client/src/org/openqa/s...
java/client/src/org/openqa/selenium/WebDriver.java:445: * not just the view
port.
On 2011/11/07 15:39:52, simonstewart wrote:
> Do you want to include that this is synonymous with window.resize() in JS?

Done.

http://codereview.appspot.com/5342045/diff/20001/java/client/src/org/openqa/s...
java/client/src/org/openqa/selenium/WebDriver.java:452: * Set the location of
the current window, relative to the upper left corner of the screen.
On 2011/11/07 15:39:52, simonstewart wrote:
> Do you want to include that this is the same as window.resizeTo() in JS?

Done.

http://codereview.appspot.com/5342045/diff/20001/javascript/atoms/window.js
File javascript/atoms/window.js (right):

http://codereview.appspot.com/5342045/diff/20001/javascript/atoms/window.js#n...
javascript/atoms/window.js:184: bot.window.getLocation = function(opt_win) {
On 2011/11/07 15:39:52, simonstewart wrote:
> What's the risk of this being mistaken for "window.location"?
> bot.window.getPosition?

Done.

[simonstewart, 2011-11-07 16:18:28]

LGTM with one small nit

http://codereview.appspot.com/5342045/diff/8022/java/client/src/org/openqa/se...
File java/client/src/org/openqa/selenium/remote/RemoteWebDriver.java (right):

http://codereview.appspot.com/5342045/diff/8022/java/client/src/org/openqa/se...
java/client/src/org/openqa/selenium/remote/RemoteWebDriver.java:566: public void
setPosition(int x, int y) {
Why not just merge this into the method from the API?

[jarib, 2011-11-07 16:34:22]

Committed as r14578.

http://codereview.appspot.com/5342045/diff/8022/java/client/src/org/openqa/se...
File java/client/src/org/openqa/selenium/remote/RemoteWebDriver.java (right):

http://codereview.appspot.com/5342045/diff/8022/java/client/src/org/openqa/se...
java/client/src/org/openqa/selenium/remote/RemoteWebDriver.java:566: public void
setPosition(int x, int y) {
On 2011/11/07 16:18:28, simonstewart wrote:
> Why not just merge this into the method from the API?

Done.