Closed Bug 1470659 Opened 6 years ago Closed 6 years ago

Add setWindowRect capability

Categories

(Remote Protocol :: Marionette, enhancement, P1)

enhancement

Tracking

(firefox63 fixed)

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: ato, Assigned: ato)

References

(Blocks 1 open bug)

Details

Attachments

(7 files, 21 obsolete files)

3.08 KB, patch
whimboo
: review+
Details | Diff | Splinter Review
8.88 KB, patch
whimboo
: review+
Details | Diff | Splinter Review
10.63 KB, patch
automatedtester
: review+
Details | Diff | Splinter Review
6.58 KB, patch
whimboo
: review+
Details | Diff | Splinter Review
2.64 KB, patch
automatedtester
: review+
whimboo
: review+
Details | Diff | Splinter Review
4.37 KB, patch
whimboo
: review+
Details | Diff | Splinter Review
4.72 KB, patch
ato
: review+
Details | Diff | Splinter Review
Marionette is missing the setWindowRect capability mandated by WebDriver.
Assignee: nobody → ato
Blocks: webdriver
Status: NEW → ASSIGNED
Priority: -- → P1
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.
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.
It will never be possible to configure setWindowRect, and trying
to do so will cause geckodriver to return with an error.
Specific tests for the platformName capability do not belong in
the test for the response body structure.
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.
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.
Attached patch Regenerate WPT manifest. r=me (obsolete) — Splinter Review
Attachment #8993935 - Flags: review?(dburns)
Attachment #8993936 - Flags: review?(dburns)
Attachment #8993937 - Flags: review?(dburns)
Attachment #8993938 - Flags: review?(dburns)
Attachment #8993939 - Flags: review?(dburns)
Attachment #8993940 - Flags: review?(dburns)
Attachment #8993941 - Flags: review+
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.
Attachment #8994009 - Flags: review?(dburns)
Attachment #8993935 - Attachment is obsolete: true
Attachment #8993935 - Flags: review?(dburns)
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.
Attachment #8994010 - Flags: review?(dburns)
Attachment #8993936 - Attachment is obsolete: true
Attachment #8993936 - Flags: review?(dburns)
It will never be possible to configure setWindowRect, and trying
to do so will cause geckodriver to return with an error.
Attachment #8993937 - Attachment is obsolete: true
Attachment #8994011 - Flags: review?(dburns)
Attachment #8993937 - Flags: review?(dburns)
Specific tests for the platformName capability do not belong in
the test for the response body structure.
Attachment #8993938 - Attachment is obsolete: true
Attachment #8994012 - Flags: review?(dburns)
Attachment #8993938 - Flags: review?(dburns)
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.
Attachment #8994013 - Flags: review?(dburns)
Attachment #8993939 - Attachment is obsolete: true
Attachment #8993939 - Flags: review?(dburns)
Attached patch Parametrize capabilities tests. (obsolete) — Splinter Review
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.
Attachment #8994014 - Flags: review?(dburns)
Attachment #8993940 - Attachment is obsolete: true
Attachment #8993940 - Flags: review?(dburns)
Attached patch Regenerate WPT manifest. (obsolete) — Splinter Review
Attachment #8993941 - Attachment is obsolete: true
Attachment #8994015 - Flags: review+
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.
Attachment #8994009 - Flags: review-
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`?
Attachment #8994010 - Flags: review-
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 @@
>  
>  
> +@pytest.mark.skipif(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
> +
> +
> +@pytest.mark.skip_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.
Attachment #8994012 - Flags: review-
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.
Attachment #8994014 - Flags: review-
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 :)
Attachment #8994014 - Flags: review?(dburns)
Attachment #8994013 - Flags: review?(dburns) → review+
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
Attachment #8994012 - Flags: review?(dburns)
Attachment #8994011 - Flags: review?(dburns) → review+
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
Attachment #8994010 - Flags: review?(dburns)
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
Attachment #8994009 - Flags: review?(dburns)
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.
Attachment #8995949 - Flags: review?(hskupin)
Attachment #8994009 - Attachment is obsolete: true
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.
Attachment #8994010 - Attachment is obsolete: true
Attachment #8995950 - Flags: review?(hskupin)
It will never be possible to configure setWindowRect, and trying
to do so will cause geckodriver to return with an error.
Attachment #8995951 - Flags: review?(dburns)
Attachment #8994011 - Attachment is obsolete: true
Specific tests for the platformName capability do not belong in
the test for the response body structure.
Attachment #8994012 - Attachment is obsolete: true
Attachment #8995952 - Flags: review?(hskupin)
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.
Attachment #8995953 - Flags: review?(hskupin)
Attachment #8995953 - Flags: review?(dburns)
Attached patch Parametrize capabilities tests. (obsolete) — Splinter Review
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.
Attachment #8995954 - Flags: review?(hskupin)
Attachment #8994014 - Attachment is obsolete: true
Attached patch Regenerate WPT manifest. (obsolete) — Splinter Review
Attachment #8995955 - Flags: review+
Attachment #8994015 - Attachment is obsolete: true
Attachment #8995951 - Flags: review?(dburns) → review+
Attachment #8995953 - Flags: review?(hskupin)
Attachment #8995953 - Flags: review?(dburns)
Attachment #8995951 - Flags: review+
Attachment #8995949 - Flags: review?(hskupin) → review+
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.
Attachment #8995950 - Flags: review?(hskupin) → review+
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
> +
> +
> +@pytest.mark.skip_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
> +
> +
> +@pytest.mark.skip_if(platform_name is None, reason="Unsupported platform %s" % platform_name)
> +def test_platformName(new_session, add_browser_capabilities):

no mixed camelCase
Attachment #8995952 - Flags: review?(hskupin) → review+
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
Attachment #8995954 - Flags: review?(hskupin) → review+
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.
Attachment #8995953 - Flags: review-
(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
> > +
> > +
> > +@pytest.mark.skip_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
> > +
> > +
> > +@pytest.mark.skip_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.
(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.
(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+…
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.
Attachment #8996318 - Flags: review?(hskupin)
Attachment #8995949 - Attachment is obsolete: true
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.
Attachment #8996319 - Flags: review?(hskupin)
Attachment #8995950 - Attachment is obsolete: true
It will never be possible to configure setWindowRect, and trying
to do so will cause geckodriver to return with an error.
Attachment #8996320 - Flags: review?(dburns)
Attachment #8995951 - Attachment is obsolete: true
Specific tests for the platformName capability do not belong in
the test for the response body structure.
Attachment #8995952 - Attachment is obsolete: true
Attachment #8996321 - Flags: review?(hskupin)
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.
Attachment #8996322 - Flags: review?(hskupin)
Attachment #8996322 - Flags: review?(dburns)
Attachment #8994013 - Attachment is obsolete: true
Attachment #8995953 - Attachment is obsolete: true
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.
Attachment #8996323 - Flags: review?(hskupin)
Attachment #8995954 - Attachment is obsolete: true
Attachment #8995955 - Attachment is obsolete: true
Attachment #8996324 - Flags: review+
(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.

> > > +@pytest.mark.skip_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?
Attachment #8996318 - Flags: review?(hskupin) → review+
Attachment #8996319 - Flags: review?(hskupin) → review+
Attachment #8996321 - Flags: review?(hskupin) → review+
Attachment #8996322 - Flags: review?(hskupin) → review+
Attachment #8996323 - Flags: review?(hskupin) → review+
Attachment #8996320 - Flags: review?(dburns) → review+
Attachment #8996322 - Flags: review?(dburns) → review+
(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.
Pushed by atolfsen@mozilla.com:
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
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/12260 for changes under testing/web-platform/tests
(In reply to Andreas Tolfsen 「:ato」 from comment #48)
> OK, have updated them all.

Thanks!
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: