Andreas Tolfsen ❲:ato❳ Assignee
	Description • 7 years ago

The WPT WebDriver client currently only unmarshals responses for
some commands (notable execute_script and execute_async_script).
For the client API we will want to unmarshal all response bodies.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 10 • 7 years ago
mozreview-review

Comment on attachment 8921877 [details]
Bug 1411281 - Unmarshal all responses in WPT WebDriver client

https://reviewboard.mozilla.org/r/192918/#review198284

::: testing/web-platform/tests/tools/webdriver/webdriver/client.py:436
(Diff revision 2)
> +        if response.status != 200:
> +            cls = error.get(value.get("error"))
> +            raise cls(value.get("message"))

This should not be here, I think this snuck in during rebase.  It is covered by error.from_response above.

	James Graham [:jgraham]
	Comment 11 • 7 years ago
mozreview-review

Comment on attachment 8921874 [details]
Bug 1411281 - Associate web element identifier with webdriver.Element

https://reviewboard.mozilla.org/r/192912/#review198314

	James Graham [:jgraham]
	Comment 12 • 7 years ago
mozreview-review

Comment on attachment 8921875 [details]
Bug 1411281 - Add equality test for webdriver.Element

https://reviewboard.mozilla.org/r/192914/#review198316

::: testing/web-platform/tests/tools/webdriver/webdriver/client.py:620
(Diff revision 2)
>          self.id = id
>          assert id not in self.session._element_cache
>          self.session._element_cache[self.id] = self
>  
> +    def __eq__(self, other):
> +        return isinstance(other, Element) and self.id == other.id

Theoretically you also need to check the sessions are the same.

	James Graham [:jgraham]
	Comment 13 • 7 years ago
mozreview-review

Comment on attachment 8921876 [details]
Bug 1411281 - Swap webdriver.Element ctor arguments

https://reviewboard.mozilla.org/r/192916/#review198526

::: testing/web-platform/tests/tools/webdriver/webdriver/client.py:613
(Diff revision 2)
>      A web element is an abstraction used to identify an element when
>      it is transported via the protocol, between remote- and local ends.
>      """
>      identifier = "element-6066-11e4-a52e-4f735466cecf"
>  
> -    def __init__(self, session, id):
> +    def __init__(self, id, session):

I guess I don't care, but this doesn't seem obviously like an improvement.

	James Graham [:jgraham]
	Comment 14 • 7 years ago
mozreview-review

Comment on attachment 8921877 [details]
Bug 1411281 - Unmarshal all responses in WPT WebDriver client

https://reviewboard.mozilla.org/r/192918/#review198532

::: testing/web-platform/tests/tools/webdriver/webdriver/protocol.py:10
(Diff revision 2)
> +
> +"""WebDriver wire protocol codecs."""
> +
> +
> +class Encoder(json.JSONEncoder):
> +

No blank line here.

::: testing/web-platform/tests/tools/webdriver/webdriver/protocol.py:24
(Diff revision 2)
> +            return {webdriver.Element.identifier: obj.id}
> +        return super(ProtocolEncoder, self).default(obj)
> +
> +
> +class Decoder(json.JSONDecoder):
> +

No blank line here.

::: testing/web-platform/tests/tools/webdriver/webdriver/protocol.py:33
(Diff revision 2)
> +            object_hook=self.object_hook, *args, **kwargs)
> +
> +    def object_hook(self, payload):
> +        if isinstance(payload, (list, tuple)):
> +            return [self.object_hook(x) for x in payload]
> +        elif isinstance(payload, dict) and webdriver.Element.identifier in payload:

This seems to end up putting a lot of type-specific information directly into this class, which doesn't seem ideal.

::: testing/web-platform/tests/tools/webdriver/webdriver/transport.py:89
(Diff revision 2)
>  
>      def url(self, suffix):
>          return urlparse.urljoin(self.url_prefix, suffix)
>  
> -    def send(self, method, uri, body=None, headers=None):
> +    def send(self, method, uri, body=None, headers=None,
> +        encoder=json.JSONEncoder, decoder=json.JSONDecoder,

This indentation is wrong.

	James Graham [:jgraham]
	Comment 15 • 7 years ago
mozreview-review

Comment on attachment 8921955 [details]
Bug 1411281 - Make assert_same_element accept webdriver.Element

https://reviewboard.mozilla.org/r/192964/#review198534

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 16 • 7 years ago
mozreview-review-reply

Comment on attachment 8921875 [details]
Bug 1411281 - Add equality test for webdriver.Element

https://reviewboard.mozilla.org/r/192914/#review198316

> Theoretically you also need to check the sessions are the same.

Good point.  I’ll squeeze in an additional patch that implements
__eq__ for webdriver.Session and add that here.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 17 • 7 years ago
mozreview-review-reply

Comment on attachment 8921876 [details]
Bug 1411281 - Swap webdriver.Element ctor arguments

https://reviewboard.mozilla.org/r/192916/#review198526

> I guess I don't care, but this doesn't seem obviously like an improvement.

I initially wanted to change webdriver.Element so that it could be
constructed without an associated session to take advantage of its
equality test in assert_same_element.  I later decided against this
due to the complexity involved for apparently little gain.

However, I still think it makes more sense to construct Element
with the web element UUID first since this is its defining
characteristic.  It only needs a session because that happens to
store the connection.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 18 • 7 years ago
mozreview-review-reply

Comment on attachment 8921877 [details]
Bug 1411281 - Unmarshal all responses in WPT WebDriver client

https://reviewboard.mozilla.org/r/192918/#review198532

> This seems to end up putting a lot of type-specific information directly into this class, which doesn't seem ideal.

It necessarily needs to have some degree of knowledge about what
it’s looking for.  We know that in WebDriver HTTP bodies the web
element identifier, the web window identifier, and the web frame
identifier always represents some kind of complex object.

The alternative to this approach is to encode this into each command
that we reasonably expects to send or receive these types, which is
an approach this patch is trying to move us away from.

I guess the lines that follow (lines 34-37) could be moved to a
class method on webdriver.Element or something.  Would that address
your concern?

	James Graham [:jgraham]
	Comment 25 • 7 years ago
mozreview-review

Comment on attachment 8922392 [details]
Bug 1411281 - Add equality test for webdriver.Session

https://reviewboard.mozilla.org/r/193432/#review199022

::: testing/web-platform/tests/tools/webdriver/webdriver/client.py:377
(Diff revision 1)
>          self.find = Find(self)
>          self.alert = UserPrompt(self)
>          self.actions = Actions(self)
>  
> +    def __eq__(self, other):
> +        return self.session_id is not None \

Don't use lint continuation characters. Use parens around the condition

```
return (self.session_id is not None and
        […])
```

	James Graham [:jgraham]
	Comment 26 • 7 years ago
mozreview-review

Comment on attachment 8921877 [details]
Bug 1411281 - Unmarshal all responses in WPT WebDriver client

https://reviewboard.mozilla.org/r/192918/#review199028

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 27 • 7 years ago

I still need a decision on the remaining open issue from
https://bugzil.la/1411281#c18.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 34 • 7 years ago
mozreview-review-reply

Comment on attachment 8922392 [details]
Bug 1411281 - Add equality test for webdriver.Session

https://reviewboard.mozilla.org/r/193432/#review199022

> Don't use lint continuation characters. Use parens around the condition
> 
> ```
> return (self.session_id is not None and
>         […])
> ```

I love it when you can only use a subset of the language.  It would
be worth standing up a linter job to catch these things so we
don’t have to waste time on it in reviews.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 35 • 7 years ago

New try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dea38cc8f7ed

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 42 • 7 years ago
mozreview-review-reply

Comment on attachment 8921877 [details]
Bug 1411281 - Unmarshal all responses in WPT WebDriver client

https://reviewboard.mozilla.org/r/192918/#review198532

> It necessarily needs to have some degree of knowledge about what
> it’s looking for.  We know that in WebDriver HTTP bodies the web
> element identifier, the web window identifier, and the web frame
> identifier always represents some kind of complex object.
> 
> The alternative to this approach is to encode this into each command
> that we reasonably expects to send or receive these types, which is
> an approach this patch is trying to move us away from.
> 
> I guess the lines that follow (lines 34-37) could be moved to a
> class method on webdriver.Element or something.  Would that address
> your concern?

I have now made this change, and I think it indeed looks better.
Please pitch in soon, or I will go ahead and land this change.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 43 • 7 years ago

try run with Element.from_json change: https://treeherder.mozilla.org/#/jobs?repo=try&revision=304407bc3034

	James Graham [:jgraham]
	Comment 44 • 7 years ago

Please go ahead and land.

	Pulsebot
	Comment 45 • 7 years ago

Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f58e9711dfc5
Associate web element identifier with webdriver.Element r=jgraham
https://hg.mozilla.org/integration/autoland/rev/4fc2942a3c69
Add equality test for webdriver.Session r=jgraham
https://hg.mozilla.org/integration/autoland/rev/2e0cca1c1e87
Add equality test for webdriver.Element r=jgraham
https://hg.mozilla.org/integration/autoland/rev/151ca34c9c62
Swap webdriver.Element ctor arguments r=jgraham
https://hg.mozilla.org/integration/autoland/rev/6da19712fa96
Unmarshal all responses in WPT WebDriver client r=jgraham
https://hg.mozilla.org/integration/autoland/rev/475350247c52
Make assert_same_element accept webdriver.Element r=jgraham

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 46 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/f58e9711dfc5
https://hg.mozilla.org/mozilla-central/rev/4fc2942a3c69
https://hg.mozilla.org/mozilla-central/rev/2e0cca1c1e87
https://hg.mozilla.org/mozilla-central/rev/151ca34c9c62
https://hg.mozilla.org/mozilla-central/rev/6da19712fa96
https://hg.mozilla.org/mozilla-central/rev/475350247c52