1470659 - Add setWindowRect capability

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, enhancement, P1)

Description

[Andreas Tolfsen ❲:ato❳]

Marionette is missing the setWindowRect capability mandated by WebDriver.

Comment 1

[Andreas Tolfsen ❲:ato❳]

Attachment: Simplify Marionette capability parsing. r=automatedtester

Change the switch-statement to only do assertions, and replace the
v variable with any parsed values before writing it to the matched
set of capabilities in one location.

Comment 2

[Andreas Tolfsen ❲:ato❳]

Attachment: Add setWindowRect capability to Marionette. r=automatedtester

The setWindowRect capability is mandated by the WebDriver standard
and is an indication whether the driver supports manipulating the
window dimensions and position.

This will always be true for Firefox and always false for Fennec.

Comment 3

[Andreas Tolfsen ❲:ato❳]

Attachment: Add setWindowRect capability to geckodriver. r=automatedtester

It will never be possible to configure setWindowRect, and trying
to do so will cause geckodriver to return with an error.

Comment 4

[Andreas Tolfsen ❲:ato❳]

Attachment: Move platformName test to separate file. r=automatedtester

Specific tests for the platformName capability do not belong in
the test for the response body structure.

Comment 5

[Andreas Tolfsen ❲:ato❳]

Attachment: Move timeouts test to separate file. r=automatedtester

For similar reasons as for platformName, tests for configuring the
timeouts object do not belong in the same parent test as those for
response body structure.

Comment 6

[Andreas Tolfsen ❲:ato❳]

Attachment: Parametrize capabilities tests. r=automatedtester

To be able to ignore tests for individual capabilities we need to
parametrize these tests.  geckodriver now supports setWindowRect,
but fails the proxy capability test because it is for some reason
not propagated back.

Comment 7

[Andreas Tolfsen ❲:ato❳]

Attachment: Regenerate WPT manifest. r=me

Comment 8

[Andreas Tolfsen ❲:ato❳]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=074a58643fa8e710e0de3b6b6a111110f88fc038

Comment 9

[Andreas Tolfsen ❲:ato❳]

Attachment: Simplify Marionette capability parsing.

Change the switch-statement to only do assertions, and replace the
v variable with any parsed values before writing it to the matched
set of capabilities in one location.

Comment 10

[Andreas Tolfsen ❲:ato❳]

Attachment: Add setWindowRect capability to Marionette.

The setWindowRect capability is mandated by the WebDriver standard
and is an indication whether the driver supports manipulating the
window dimensions and position.

This will always be true for Firefox and always false for Fennec.

Comment 11

[Andreas Tolfsen ❲:ato❳]

Attachment: Add setWindowRect capability to geckodriver.

It will never be possible to configure setWindowRect, and trying
to do so will cause geckodriver to return with an error.

Comment 12

[Andreas Tolfsen ❲:ato❳]

Attachment: Move platformName test to separate file.

Specific tests for the platformName capability do not belong in
the test for the response body structure.

Comment 13

[Andreas Tolfsen ❲:ato❳]

Attachment: Move timeouts test to separate file.

For similar reasons as for platformName, tests for configuring the
timeouts object do not belong in the same parent test as those for
response body structure.

Comment 14

[Andreas Tolfsen ❲:ato❳]

Attachment: Parametrize capabilities tests.

To be able to ignore tests for individual capabilities we need to
parametrize these tests.  geckodriver now supports setWindowRect,
but fails the proxy capability test because it is for some reason
not propagated back.

Comment 15

[Andreas Tolfsen ❲:ato❳]

Attachment: Regenerate WPT manifest.

Comment 16

[Andreas Tolfsen ❲:ato❳]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ba022f2f59c1a505cf61fabc7f4a711ebc3d183c

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8994009 [details] [diff] [review]
Simplify Marionette capability parsing.

Review of attachment 8994009 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/marionette/capabilities.js
@@ +475,3 @@
>  
>              if (Object.values(PageLoadStrategy).includes(v)) {
>                matched.set("pageLoadStrategy", v);

I think we want to remove this call to `set` too, by negating the check and replace it with the else case, right?

@@ +475,5 @@
>  
>              if (Object.values(PageLoadStrategy).includes(v)) {
>                matched.set("pageLoadStrategy", v);
>              } else {
> +              throw new InvalidArgumentError("Unknown page load strategy: " + v);

Use a template string here to not have to use `+`.

@@ +488,1 @@
>            break;

Remove the extra `break`.

@@ +494,5 @@
>          case "unhandledPromptBehavior":
>            assert.string(v,
>                pprint`Expected ${k} to be a string, got ${v}`);
>  
>            if (Object.values(UnhandledPromptBehavior).includes(v)) {

Same as for page load strategy.

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8994010 [details] [diff] [review]
Add setWindowRect capability to Marionette.

Review of attachment 8994010 [details] [diff] [review]:
-----------------------------------------------------------------

Please also add Python unit tests for this addition in test_capabilities.py.

::: testing/marionette/capabilities.js
@@ +488,5 @@
>            break;
> +
> +        case "setWindowRect":
> +          assert.boolean(v, pprint`Expected ${k} to be boolean, got ${v}`);
> +          if (v && Services.appinfo.name != "Firefox") {

Why don't you use the global `appinfo` object here in regards of `firefox` vs. `Firefox`?

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8994012 [details] [diff] [review]
Move platformName test to separate file.

Review of attachment 8994012 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/web-platform/tests/webdriver/tests/new_session/merge.py
@@ +7,3 @@
>  
>  
> [e-mail](platform_name is None, reason="Unsupported platform")

Mind to add the platform name to the reason string?

::: testing/web-platform/tests/webdriver/tests/new_session/page_load_strategy.py
@@ +3,5 @@
> +def test_pageLoadStrategy(new_session, add_browser_capabilities):
> +    response, _ = new_session({"capabilities": {
> +        "alwaysMatch": add_browser_capabilities({"pageLoadStrategy": "eager"})}})
> +    value = assert_success(response)
> +

nit: you can remove this empty line.

::: testing/web-platform/tests/webdriver/tests/new_session/platform_name.py
@@ +1,2 @@
> +import pytest
> +import sys

Unused import of sys.

@@ +1,5 @@
> +import pytest
> +import sys
> +
> +from tests.support.asserts import assert_success
> +from tests.support import platform_name

nit: those lines should be flipped.

@@ +4,5 @@
> +from tests.support.asserts import assert_success
> +from tests.support import platform_name
> +
> +
> [e-mail]_if(platform_name is None)

Please add a reason string similar to the merge.py file.

::: testing/web-platform/tests/webdriver/tests/support/capabilities.py
@@ +1,1 @@
> +platform_name = {

I don't see this file being used anywhere in favor of __init__.py. So please remove.

Comment 20

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8994014 [details] [diff] [review]
Parametrize capabilities tests.

Review of attachment 8994014 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/web-platform/tests/webdriver/tests/new_session/response.py
@@ +1,3 @@
>  # META: timeout=long
>  
> +import pytest

pytest is not a standard lib module and needs to be added after `uuid`.

@@ +20,5 @@
> +    ("acceptInsecureCerts", bool),
> +    ("setWindowRect", bool),
> +    ("timeouts", dict),
> +    ("proxy", dict),
> +    ("pageLoadStrategy", basestring),

Mind sorting the caps as listed in the spec? It would make it easier to compare if something is missing.

@@ +22,5 @@
> +    ("timeouts", dict),
> +    ("proxy", dict),
> +    ("pageLoadStrategy", basestring),
> +])
> +def test_capability_type(new_session, add_browser_capabilities, capability, type):

Please note that this parametrization will unnecessarily restart the browser for each parameter. Given that we use no browser capabilities you want to use the `session` fixture instead.

@@ +35,5 @@
> +    ("acceptInsecureCerts", False),
> +    ("setWindowRect", True),
> +    ("timeouts", {"implicit": 0, "pageLoad": 300000, "script": 30000}),
> +    ("proxy", {}),
> +    ("pageLoadStrategy", "normal"),

Looks like I missed to add the test for the `unhandledPromptBehavior` capability. Mind adding it here too?

@@ +42,1 @@
>      response, _ = new_session({"capabilities": {"alwaysMatch": add_browser_capabilities({})}})

Same problem for restarts as noted above.

Comment 21

[David Burns :automatedtester]

Comment on attachment 8994014 [details] [diff] [review]
Parametrize capabilities tests.

Review of attachment 8994014 [details] [diff] [review]:
-----------------------------------------------------------------

Agree with whimboo's comments, will let him finish this review :)

Comment 22

[David Burns :automatedtester]

Comment on attachment 8994012 [details] [diff] [review]
Move platformName test to separate file.

Review of attachment 8994012 [details] [diff] [review]:
-----------------------------------------------------------------

Removing myself as reviewer as I agree with Whimboo's comments. I will let him follow up

Comment 23

[David Burns :automatedtester]

Comment on attachment 8994010 [details] [diff] [review]
Add setWindowRect capability to Marionette.

Review of attachment 8994010 [details] [diff] [review]:
-----------------------------------------------------------------

I will let whimboo follow up here

Comment 24

[David Burns :automatedtester]

Comment on attachment 8994009 [details] [diff] [review]
Simplify Marionette capability parsing.

Review of attachment 8994009 [details] [diff] [review]:
-----------------------------------------------------------------

I will let whimboo follow up here

Comment 25

[Andreas Tolfsen ❲:ato❳]

Attachment: Simplify Marionette capability parsing.

Change the switch-statement to only do assertions, and replace the
v variable with any parsed values before writing it to the matched
set of capabilities in one location.

Comment 26

[Andreas Tolfsen ❲:ato❳]

Attachment: Add setWindowRect capability to Marionette.

The setWindowRect capability is mandated by the WebDriver standard
and is an indication whether the driver supports manipulating the
window dimensions and position.

This will always be true for Firefox and always false for Fennec.

Comment 27

[Andreas Tolfsen ❲:ato❳]

Attachment: Add setWindowRect capability to geckodriver.

It will never be possible to configure setWindowRect, and trying
to do so will cause geckodriver to return with an error.

Comment 28

[Andreas Tolfsen ❲:ato❳]

Attachment: Move platformName test to separate file.

Specific tests for the platformName capability do not belong in
the test for the response body structure.

Comment 29

[Andreas Tolfsen ❲:ato❳]

Attachment: Move timeouts test to separate file.

For similar reasons as for platformName, tests for configuring the
timeouts object do not belong in the same parent test as those for
response body structure.

Comment 30

[Andreas Tolfsen ❲:ato❳]

Attachment: Parametrize capabilities tests.

To be able to ignore tests for individual capabilities we need to
parametrize these tests.  geckodriver now supports setWindowRect,
but fails the proxy capability test because it is for some reason
not propagated back.

Comment 31

[Andreas Tolfsen ❲:ato❳]

Attachment: Regenerate WPT manifest.

Comment 32

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8995950 [details] [diff] [review]
Add setWindowRect capability to Marionette.

Review of attachment 8995950 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/marionette/harness/marionette_harness/tests/unit/test_capabilities.py
@@ +154,5 @@
>              with self.assertRaisesRegexp(SessionNotCreatedException, "InvalidArgumentError"):
>                  self.marionette.start_session({"pageLoadStrategy": value})
>  
> +    def test_set_window_rect(self):
> +        if self.browser_name == "Fennec":

Fennec won't exist that long anymore, so better to put it into the else case instead of Firefox.

Comment 33

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8995952 [details] [diff] [review]
Move platformName test to separate file.

Review of attachment 8995952 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/web-platform/tests/webdriver/tests/new_session/platform_name.py
@@ +1,4 @@
> +import pytest
> +
> +from tests.support.asserts import assert_success
> +from tests.support import platform_name

why not flipped?

@@ +3,5 @@
> +from tests.support.asserts import assert_success
> +from tests.support import platform_name
> +
> +
> [e-mail]_if(platform_name is None, reason="Unsupported platform %s" % platform_name)

Lets prefer to use `format` all over the place.

@@ +4,5 @@
> +from tests.support import platform_name
> +
> +
> [e-mail]_if(platform_name is None, reason="Unsupported platform %s" % platform_name)
> +def test_platformName(new_session, add_browser_capabilities):

no mixed camelCase

Comment 34

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8995954 [details] [diff] [review]
Parametrize capabilities tests.

Review of attachment 8995954 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/web-platform/tests/webdriver/tests/new_session/response.py
@@ +19,5 @@
> +    ("acceptInsecureCerts", bool),
> +    ("pageLoadStrategy", basestring),
> +    ("proxy", dict),
> +    ("setWindowRect", bool),
> +    ("timeouts", dict),

It would still be good if you could add `unhandledPromptBehavior` here too as pointed out in my last review.

@@ +21,5 @@
> +    ("proxy", dict),
> +    ("setWindowRect", bool),
> +    ("timeouts", dict),
> +])
> +def test_capability_type(session, add_browser_capabilities, capability, type):

`add_browser_capabilities` is unused

@@ +34,5 @@
> +    ("proxy", {}),
> +    ("setWindowRect", True),
> +    ("timeouts", {"implicit": 0, "pageLoad": 300000, "script": 30000}),
> +])
> +def test_capability_default_value(session, add_browser_capabilities, capability, default_value):

`add_browser_capabilities` is unused

Comment 35

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8995953 [details] [diff] [review]
Move timeouts test to separate file.

Review of attachment 8995953 [details] [diff] [review]:
-----------------------------------------------------------------

I assume this patch is ready to be reviewed and you only missed to set the reviewer.

::: testing/web-platform/tests/webdriver/tests/new_session/timeouts.py
@@ +3,5 @@
> +from tests.support.asserts import assert_success
> +
> +
> +def test_default_values(new_session, add_browser_capabilities):
> +    response, _ = new_session({"capabilities": {"alwaysMatch": add_browser_capabilities({})}})

Same as for platform. Use the `session` capability to not force a restart.

@@ +17,5 @@
> +    {"implicit": 444, "pageLoad": 300000,"script": 30000},
> +    {"implicit": 0, "pageLoad": 444,"script": 30000},
> +    {"implicit": 0, "pageLoad": 300000,"script": 444},
> +])
> +def test_timeouts(new_session, add_browser_capabilities, timeouts):

Same here.

Comment 36

[Andreas Tolfsen ❲:ato❳]

(In reply to Henrik Skupin (:whimboo) from comment #33)
> Comment on attachment 8995952 [details] [diff] [review]
> Move platformName test to separate file.
> 
> Review of attachment 8995952 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: testing/web-platform/tests/webdriver/tests/new_session/platform_name.py
> @@ +1,4 @@
> > +import pytest
> > +
> > +from tests.support.asserts import assert_success
> > +from tests.support import platform_name
> 
> why not flipped?

They _are_ sorted according to lexicographical order, e.g. sort(1).

In any case, this is not crucial to the function of the patch.

> @@ +3,5 @@
> > +from tests.support.asserts import assert_success
> > +from tests.support import platform_name
> > +
> > +
> > [e-mail]_if(platform_name is None, reason="Unsupported platform %s" % platform_name)
> 
> Lets prefer to use `format` all over the place.

Why?  AIUI they are functionally equivalent.

> @@ +4,5 @@
> > +from tests.support import platform_name
> > +
> > +
> > [e-mail]_if(platform_name is None, reason="Unsupported platform %s" % platform_name)
> > +def test_platformName(new_session, add_browser_capabilities):
> 
> no mixed camelCase

Here the test name is referring to a specific symbol so I don’t
think we should rewrite it, but maybe we can change the test title
to something more meaningful considering the file’s name is already
platform_name.py.  I’ve put test_corresponds_to_local_system there
now which I think will read better in the results table as it talks
about what the test is specifically testing.

Comment 37

[Andreas Tolfsen ❲:ato❳]

(In reply to Henrik Skupin (:whimboo) from comment #34)

> ::: testing/web-platform/tests/webdriver/tests/new_session/response.py
> @@ +19,5 @@
> > +    ("acceptInsecureCerts", bool),
> > +    ("pageLoadStrategy", basestring),
> > +    ("proxy", dict),
> > +    ("setWindowRect", bool),
> > +    ("timeouts", dict),
> 
> It would still be good if you could add `unhandledPromptBehavior` here too
> as pointed out in my last review.

This was not already in the original test and really should be
addressed separately, but I’ve added it now.

Comment 38

[Andreas Tolfsen ❲:ato❳]

(In reply to Henrik Skupin (:whimboo) from comment #35)
> Comment on attachment 8995953 [details] [diff] [review]
> Move timeouts test to separate file.
> 
> Review of attachment 8995953 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I assume this patch is ready to be reviewed and you only missed to set the
> reviewer.

Both you and AutomatedTester previously gave this patch r+…

Comment 39

[Andreas Tolfsen ❲:ato❳]

Attachment: Simplify Marionette capability parsing.

Change the switch-statement to only do assertions, and replace the
v variable with any parsed values before writing it to the matched
set of capabilities in one location.

Comment 40

[Andreas Tolfsen ❲:ato❳]

Attachment: Add setWindowRect capability to Marionette.

The setWindowRect capability is mandated by the WebDriver standard
and is an indication whether the driver supports manipulating the
window dimensions and position.

This will always be true for Firefox and always false for Fennec.

Comment 41

[Andreas Tolfsen ❲:ato❳]

Attachment: Add setWindowRect capability to geckodriver.

It will never be possible to configure setWindowRect, and trying
to do so will cause geckodriver to return with an error.

Comment 42

[Andreas Tolfsen ❲:ato❳]

Attachment: Move platformName test to separate file.

Specific tests for the platformName capability do not belong in
the test for the response body structure.

Comment 43

[Andreas Tolfsen ❲:ato❳]

Attachment: Move timeouts test to separate file.

For similar reasons as for platformName, tests for configuring the
timeouts object do not belong in the same parent test as those for
response body structure.

Comment 44

[Andreas Tolfsen ❲:ato❳]

Attachment: Parametrize capabilities tests.

To be able to ignore tests for individual capabilities we need to
parametrize these tests.  geckodriver now supports setWindowRect,
but fails the proxy capability test because it is for some reason
not propagated back.

Comment 45

[Andreas Tolfsen ❲:ato❳]

Attachment: Regenerate WPT manifest.

Comment 46

[Andreas Tolfsen ❲:ato❳]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2836194ccae76068547cdcd0ae16a6a2d1de94d4

Comment 47

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Andreas Tolfsen 「:ato」 from comment #36)
> > > +from tests.support.asserts import assert_success
> > > +from tests.support import platform_name
> > 
> > why not flipped?
> 
> They _are_ sorted according to lexicographical order, e.g. sort(1).

sort clearly doesn't sort it this way for me on MacOS. 

> > Lets prefer to use `format` all over the place.
> 
> Why?  AIUI they are functionally equivalent.

While the old style formatting is not deprecated yet, it is recommended for Python 3 to use the new style.

> > > [e-mail]_if(platform_name is None, reason="Unsupported platform %s" % platform_name)
> > > +def test_platformName(new_session, add_browser_capabilities):
> > 
> > no mixed camelCase
> 
> Here the test name is referring to a specific symbol so I don’t
> think we should rewrite it, but maybe we can change the test title
> to something more meaningful considering the file’s name is already
> platform_name.py.  I’ve put test_corresponds_to_local_system there
> now which I think will read better in the results table as it talks
> about what the test is specifically testing.

Didn't this cause a linter failure before?

Comment 48

[Andreas Tolfsen ❲:ato❳]

(In reply to Henrik Skupin (:whimboo) from comment #47)

> (In reply to Andreas Tolfsen 「:ato」 from comment #36)  
> 
> > > > +from tests.support.asserts import assert_success import
> > > > +from tests.supportplatform_name
> > > 
> > > why not flipped?
> > 
> > They _are_ sorted according to lexicographical order, e.g.
> > sort(1).
> 
> sort clearly doesn't sort it this way for me on MacOS.

I noticed that too.  Using "LC_ALL=en_US.utf8 sort" appears to fix
the issue on my Linux workstation.

> > > Lets prefer to use `format` all over the place.
> > 
> > Why?  AIUI they are functionally equivalent.
> 
> While the old style formatting is not deprecated yet, it is
> recommended for Python 3 to use the new style.

OK, have updated them all.

Comment 49

[Andreas Tolfsen ❲:ato❳]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=553048b2502d8ca3a53bd267c644d01dcf873e56

Comment 50

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/39954f25c48e
Simplify Marionette capability parsing. r=whimboo
https://hg.mozilla.org/integration/mozilla-inbound/rev/203d1449aa59
Add setWindowRect capability to Marionette. r=whimboo
https://hg.mozilla.org/integration/mozilla-inbound/rev/e99e43a1e400
Add setWindowRect capability to geckodriver. r=automatedtester
https://hg.mozilla.org/integration/mozilla-inbound/rev/a3d141d56e53
Move platformName test to separate file. r=whimboo
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ad9872c27e0
Move timeouts test to separate file. r=automatedtester,whimboo
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5d9ec944fe0
Parametrize capabilities tests. r=whimboo
https://hg.mozilla.org/integration/mozilla-inbound/rev/b04c9a64cd61
Regenerate WPT manifest. r=me

Comment 51

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/12260 for changes under testing/web-platform/tests

Comment 52

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Can't merge web-platform-tests PR due to failing upstream checks:
Github PR https://github.com/web-platform-tests/wpt/pull/12260
* continuous-integration/travis-ci/pr (https://travis-ci.org/web-platform-tests/wpt/builds/410923358?utm_source=github_status&utm_medium=notification)

Comment 53

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Andreas Tolfsen 「:ato」 from comment #48)
> OK, have updated them all.

Thanks!

Comment 54

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/39954f25c48e
https://hg.mozilla.org/mozilla-central/rev/203d1449aa59
https://hg.mozilla.org/mozilla-central/rev/e99e43a1e400
https://hg.mozilla.org/mozilla-central/rev/a3d141d56e53
https://hg.mozilla.org/mozilla-central/rev/0ad9872c27e0
https://hg.mozilla.org/mozilla-central/rev/b5d9ec944fe0
https://hg.mozilla.org/mozilla-central/rev/b04c9a64cd61

Comment 55

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged