Fixes to the wire protocol of get local and session storage item

[Eran, 2011-07-29 13:52:08]

I'm not really sure why this change is needed - and from the comments in the
code, it seems like Nayeem isn't, either. GET/DELETE requests should be
differentiated by the server. It's working for anything else, and this hack
wouldn't be android-specific - it will taint all of the drivers which support
HTML5 local storage.

To sum it up, the issue with get/delete mixing should be investigated - I don't
see any reason for this patch to go in.

[berrada, 2011-07-29 16:48:45]

The get and delete mapper operate separately and iPhone supports WebStorage
using the same wire commands. Let's take a look at this together today. 

On 2011/07/29 13:52:08, Eran wrote:
> I'm not really sure why this change is needed - and from the comments in the
> code, it seems like Nayeem isn't, either. GET/DELETE requests should be
> differentiated by the server. It's working for anything else, and this hack
> wouldn't be android-specific - it will taint all of the drivers which support
> HTML5 local storage.
> 
> To sum it up, the issue with get/delete mixing should be investigated - I
don't
> see any reason for this patch to go in.

[Nayeem, 2011-07-29 21:44:10]

On 2011/07/29 21:33:03, Nayeem wrote:

The problem was not actually with get/remove issue. There was a setting conflict
at the UrlMapper class while creating the ResultConfig objects for both
ClearLocalStorage and GetLocalStorageItem (same for ClearSessionStorage and
GetSessionStorageItem). 

For example, the UrlMapper.bind("/session/:sessionId/local_storage/size",
ClearStorage.class ) method removes the ResultConfig instance for
GetLocalStorageItem as the url matched with
"/session/:sessionId/local_storage/:key"

[berrada, 2011-07-29 21:44:34]

LGTM

So the problem was that session_storage/:key was overriding session_storage/size
on the client side. Thanks for the fix Nayeem!

http://codereview.appspot.com/4833042/diff/4001/java/server/src/org/openqa/se...
File java/server/src/org/openqa/selenium/remote/server/DriverServlet.java
(right):

http://codereview.appspot.com/4833042/diff/4001/java/server/src/org/openqa/se...
java/server/src/org/openqa/selenium/remote/server/DriverServlet.java:270: //
conflicting any more.
Remove this comment.

[Eran, 2011-08-01 11:53:54]

Please make the changes in wire.py, run it to re-generate the wiki page and
update both, in addition to the Java code.

http://codereview.appspot.com/4833042/diff/4001/java/client/src/org/openqa/se...
File java/client/src/org/openqa/selenium/remote/HttpCommandExecutor.java
(right):

http://codereview.appspot.com/4833042/diff/4001/java/client/src/org/openqa/se...
java/client/src/org/openqa/selenium/remote/HttpCommandExecutor.java:197: 
I understand the issue. In that case, I'd suggest making all key-specific URLs
consistent:
.put(GET_LOCAL_STORAGE_ITEM, get(".../local_storage/key/:key")
.put(REMOVE_LOCAL_STORAGE_ITEM, delete(".../local_storage/key/:key")
.put(SET_LOCAL_STORAGE_ITEM, post(".../local_storage/key")

So the only operations for .../local_storage are ones that modify the entire
local storage.

[Nayeem, 2011-08-02 17:46:06]

Done.

wire.py  does not currently have the documentation on the protocols of HTML5. I
will update them in a different CL.

On 2011/08/01 11:53:54, Eran wrote:
> Please make the changes in wire.py, run it to re-generate the wiki page and
> update both, in addition to the Java code.
> 
>
http://codereview.appspot.com/4833042/diff/4001/java/client/src/org/openqa/se...
> File java/client/src/org/openqa/selenium/remote/HttpCommandExecutor.java
> (right):
> 
>
http://codereview.appspot.com/4833042/diff/4001/java/client/src/org/openqa/se...
> java/client/src/org/openqa/selenium/remote/HttpCommandExecutor.java:197: 
> I understand the issue. In that case, I'd suggest making all key-specific URLs
> consistent:
> .put(GET_LOCAL_STORAGE_ITEM, get(".../local_storage/key/:key")
> .put(REMOVE_LOCAL_STORAGE_ITEM, delete(".../local_storage/key/:key")
> .put(SET_LOCAL_STORAGE_ITEM, post(".../local_storage/key")
> 
> So the only operations for .../local_storage are ones that modify the entire
> local storage.

[Eran, 2011-08-04 13:52:02]

Hi,

I've had a chat with Simon about this, he proposed an even simpler scheme:
Make the get call on /session/:sessionId/local_storage return the size as one of
the fields of the JSON object it returns. You could then use the original scheme
of /session/:sessionId/local_storage/:key (get, remove), we'll have one less
command in the wire protocol, won't need the 'key' prefix for every key and even
be more REST-like.

[Nayeem, 2011-08-04 15:49:20]

Hi,

/session/:sessionId/local_**storage is currently used to get all keys of the
localStorage, so it will not work to return the size.

-Nayeem

On Thu, Aug 4, 2011 at 6:52 AM, <eranm@google.com> wrote:

> Hi,
>
> I've had a chat with Simon about this, he proposed an even simpler
> scheme:
> Make the get call on /session/:sessionId/local_**storage return the size
> as one of the fields of the JSON object it returns. You could then use
> the original scheme of /session/:sessionId/local_**storage/:key (get,
> remove), we'll have one less command in the wire protocol, won't need
> the 'key' prefix for every key and even be more REST-like.
>
>
>
>
>
http://codereview.appspot.com/**4833042/<http://codereview.appspot.com/4833042/>
>

[shs_google.com, 2011-08-04 15:55:58]

You know that we get to define what gets returned, right? :)

In a RESTish architecture a "GET" can return a representation of a
resource. Currently, the representation we've decided to return is a
list of the keys contained within this resource. We can return more
data though, and part of that could be the key. That is, we're looking
at a data structure like:

{
  'size': 12,
  'keys': [ array of keys ]
}

Simon

On Thu, Aug 4, 2011 at 4:49 PM, Abdullah Al-Nayeem <alnayeem@google.com> wrote:
> Hi,
> /session/:sessionId/local_storage is currently used to get all keys of the
> localStorage, so it will not work to return the size.
>
> -Nayeem
> On Thu, Aug 4, 2011 at 6:52 AM, <eranm@google.com> wrote:
>>
>> Hi,
>>
>> I've had a chat with Simon about this, he proposed an even simpler
>> scheme:
>> Make the get call on /session/:sessionId/local_storage return the size
>> as one of the fields of the JSON object it returns. You could then use
>> the original scheme of /session/:sessionId/local_storage/:key (get,
>> remove), we'll have one less command in the wire protocol, won't need
>> the 'key' prefix for every key and even be more REST-like.
>>
>>
>>
>> http://codereview.appspot.com/4833042/
>
>

[Nayeem, 2011-08-07 21:26:06]

Hi,

I understand now. We can follow this approach to define this data structure.
But won't it happen at a higher cost? We will be doing two operations on the
server side by executing two atoms on keys and size. We may not gain much at
the client side too. One other thing that appears to me that it would
require some more refactoring at my current chromedriver implementation of
webstorage. Changing a path requires me to change several files! :(

-Nayeem

On Thu, Aug 4, 2011 at 8:55 AM, Simon Stewart <shs@google.com> wrote:

> You know that we get to define what gets returned, right? :)
>
> In a RESTish architecture a "GET" can return a representation of a
> resource. Currently, the representation we've decided to return is a
> list of the keys contained within this resource. We can return more
> data though, and part of that could be the key. That is, we're looking
> at a data structure like:
>
> {
>  'size': 12,
>  'keys': [ array of keys ]
> }
>
> Simon
>
> On Thu, Aug 4, 2011 at 4:49 PM, Abdullah Al-Nayeem <alnayeem@google.com>
> wrote:
> > Hi,
> > /session/:sessionId/local_storage is currently used to get all keys of
> the
> > localStorage, so it will not work to return the size.
> >
> > -Nayeem
> > On Thu, Aug 4, 2011 at 6:52 AM, <eranm@google.com> wrote:
> >>
> >> Hi,
> >>
> >> I've had a chat with Simon about this, he proposed an even simpler
> >> scheme:
> >> Make the get call on /session/:sessionId/local_storage return the size
> >> as one of the fields of the JSON object it returns. You could then use
> >> the original scheme of /session/:sessionId/local_storage/:key (get,
> >> remove), we'll have one less command in the wire protocol, won't need
> >> the 'key' prefix for every key and even be more REST-like.
> >>
> >>
> >>
> >> http://codereview.appspot.com/4833042/
> >
> >
>

[Eran, 2011-08-08 17:05:53]

Touching the wire protocol is rarely simple :)
I understand the pain, however, we're getting towards making WebDriver a
standard and the wire protocol is an essential part of this standard, so it's
important we get it right.
I'll be happy to quickly review these changes to the wire protocol separately,
as to not block other changes.

[berrada, 2011-08-08 21:36:09]

I'm not convinced this is the right approach either. The solution proposed is
instead of adding another wire command, combine the two commands to get the size
and get the keys into one wire. But this will result into slower performance for
drivers because the cost associated to executing JS is higher than the cost to
adding a wire command.

Let's chat after tomorrow's team meeting.

On 2011/08/08 17:05:53, Eran wrote:
> Touching the wire protocol is rarely simple :)
> I understand the pain, however, we're getting towards making WebDriver a
> standard and the wire protocol is an essential part of this standard, so it's
> important we get it right.
> I'll be happy to quickly review these changes to the wire protocol separately,
> as to not block other changes.

[berrada, 2011-08-10 18:59:11]

*Ping*

Simon, as I explained to Eran earlier, I'm not convinced this is the right
approach because every request to getKeys() and getSize() will do more work than
actually required. Injecting JS in a browser is more expensive than adding a
wire command. 

FYI, Nayeem is leaving on Friday so it'd great if he could get his code in
before.


On 2011/08/08 21:36:09, berrada wrote:
> I'm not convinced this is the right approach either. The solution proposed is
> instead of adding another wire command, combine the two commands to get the
size
> and get the keys into one wire. But this will result into slower performance
for
> drivers because the cost associated to executing JS is higher than the cost to
> adding a wire command.
> 
> Let's chat after tomorrow's team meeting.
> 
> On 2011/08/08 17:05:53, Eran wrote:
> > Touching the wire protocol is rarely simple :)
> > I understand the pain, however, we're getting towards making WebDriver a
> > standard and the wire protocol is an essential part of this standard, so
it's
> > important we get it right.
> > I'll be happy to quickly review these changes to the wire protocol
separately,
> > as to not block other changes.

[berrada, 2011-08-10 18:59:18]

*Ping*

Simon, as I explained to Eran earlier, I'm not convinced this is the right
approach because every request to getKeys() and getSize() will do more work than
actually required. Injecting JS in a browser is more expensive than adding a
wire command. 

FYI, Nayeem is leaving on Friday so it'd great if he could get his code in
before.


On 2011/08/08 21:36:09, berrada wrote:
> I'm not convinced this is the right approach either. The solution proposed is
> instead of adding another wire command, combine the two commands to get the
size
> and get the keys into one wire. But this will result into slower performance
for
> drivers because the cost associated to executing JS is higher than the cost to
> adding a wire command.
> 
> Let's chat after tomorrow's team meeting.
> 
> On 2011/08/08 17:05:53, Eran wrote:
> > Touching the wire protocol is rarely simple :)
> > I understand the pain, however, we're getting towards making WebDriver a
> > standard and the wire protocol is an essential part of this standard, so
it's
> > important we get it right.
> > I'll be happy to quickly review these changes to the wire protocol
separately,
> > as to not block other changes.