WebDriverCommand::FullscreenWindow is mapped to wrong Marionette command

RESOLVED FIXED in Firefox 57

Status

Testing
geckodriver
RESOLVED FIXED
9 months ago
8 months ago

People

(Reporter: ato, Assigned: ato)

Tracking

Version 3
mozilla57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments, 2 obsolete attachments)

(Assignee)

Description

9 months ago
The WebDriverCommand::FullscreenWindow in geckodriver maps to the non-existent Marionette command "fullscreenWindow".  It should be "fullscreen".
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

9 months ago
Assignee: nobody → ato
Status: NEW → ASSIGNED

Comment 5

9 months ago
mozreview-review
Comment on attachment 8894491 [details]
Bug 1388036 - Map WebDriverCommand::FullscreenWindow correctly.

https://reviewboard.mozilla.org/r/165654/#review170738

::: testing/geckodriver/src/marionette.rs:1044
(Diff revision 1)
>              SetTimeouts(ref x) => (Some("setTimeouts"), Some(x.to_marionette())),
>              SetWindowRect(ref x) => (Some("setWindowRect"), Some(x.to_marionette())),
>              GetWindowRect => (Some("getWindowRect"), None),
>              MinimizeWindow => (Some("WebDriver:MinimizeWindow"), None),
>              MaximizeWindow => (Some("maximizeWindow"), None),
> -            FullscreenWindow => (Some("fullscreenWindow"), None),
> +            FullscreenWindow => (Some("fullscreen"), None),

When it wasn't working correctly so far, why don't we use the `WebDriver:FullscreenWindow` command directly?

Comment 6

9 months ago
mozreview-review
Comment on attachment 8894494 [details]
Bug 1388036 - Add WPT tests for Fullscreen Window.

https://reviewboard.mozilla.org/r/165660/#review170756

::: testing/web-platform/tests/webdriver/tests/fullscreen_window.py:26
(Diff revision 1)
> +def test_handle_user_prompt(session):
> +    # step 2
> +    session.url = alert_doc
> +    response = fullscreen(session)
> +    assert_error(response, "unexpected alert open")
> +

We need to add more tests like in `get_title.py` for prompts

::: testing/web-platform/tests/webdriver/tests/fullscreen_window.py:47
(Diff revision 1)
> +    rect = response.body["value"]
> +    assert "width" in rect
> +    assert "height" in rect
> +    assert "x" in rect
> +    assert "y" in rect
> +    assert isinstance(rect["width"], float)

This should test against `(int, float)`
Attachment #8894494 - Flags: review?(dburns) → review-

Comment 7

9 months ago
mozreview-review
Comment on attachment 8894492 [details]
Bug 1388036 - Add client.Session#fullscreen API to WebDriver client.

https://reviewboard.mozilla.org/r/165656/#review170758
Attachment #8894492 - Flags: review?(dburns) → review+

Comment 8

9 months ago
mozreview-review
Comment on attachment 8894493 [details]
Bug 1388036 - Clean up fullscreen state after wdspec test.

https://reviewboard.mozilla.org/r/165658/#review170760
Attachment #8894493 - Flags: review?(dburns) → review+

Comment 9

9 months ago
mozreview-review
Comment on attachment 8894491 [details]
Bug 1388036 - Map WebDriverCommand::FullscreenWindow correctly.

https://reviewboard.mozilla.org/r/165654/#review170762
Attachment #8894491 - Flags: review?(dburns) → review+
(Assignee)

Comment 10

9 months ago
mozreview-review-reply
Comment on attachment 8894491 [details]
Bug 1388036 - Map WebDriverCommand::FullscreenWindow correctly.

https://reviewboard.mozilla.org/r/165654/#review170738

> When it wasn't working correctly so far, why don't we use the `WebDriver:FullscreenWindow` command directly?

WebDriver::FullscreenWindow is only available in Firefox 56 and greater.  By using "fullscreen" it will work with earlier Firefoxen.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 13

9 months ago
mozreview-review-reply
Comment on attachment 8894491 [details]
Bug 1388036 - Map WebDriverCommand::FullscreenWindow correctly.

https://reviewboard.mozilla.org/r/165654/#review170738

> WebDriver::FullscreenWindow is only available in Firefox 56 and greater.  By using "fullscreen" it will work with earlier Firefoxen.

Makes sense. Thanks.
(Assignee)

Comment 14

9 months ago
The new fullscreen tests appear to cause too many intermittents with Linux debug builds.  I suspect I need to revisit the actually Marionette implementation and make it more reliable.

Comment 15

9 months ago
mozreview-review
Comment on attachment 8894494 [details]
Bug 1388036 - Add WPT tests for Fullscreen Window.

https://reviewboard.mozilla.org/r/165660/#review171362
Attachment #8894494 - Flags: review?(dburns) → review+
(Assignee)

Updated

9 months ago
Summary: WebDriverCommand::FullscreenWindow is mapped to wrong Marionette command → WebDriverCommand:FullscreenWindow is mapped to wrong Marionette command
(Assignee)

Updated

9 months ago
Summary: WebDriverCommand:FullscreenWindow is mapped to wrong Marionette command → WebDriverCommand::FullscreenWindow is mapped to wrong Marionette command
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

8 months ago
Attachment #8894492 - Attachment is obsolete: true
(Assignee)

Updated

8 months ago
Attachment #8894493 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

8 months ago
Blocks: 1391691

Comment 25

8 months ago
mozreview-review
Comment on attachment 8898821 [details]
Bug 1388036 - Restore window when setting window rect.

https://reviewboard.mozilla.org/r/170194/#review175896
Attachment #8898821 - Flags: review?(dburns) → review+

Comment 26

8 months ago
Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d8f04b1a073f
Restore window when setting window rect. r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/8efd7bd68e91
Map WebDriverCommand::FullscreenWindow correctly. r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/c0f5b257ba6b
Add WPT tests for Fullscreen Window. r=automatedtester

Comment 27

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d8f04b1a073f
https://hg.mozilla.org/mozilla-central/rev/8efd7bd68e91
https://hg.mozilla.org/mozilla-central/rev/c0f5b257ba6b
Status: ASSIGNED → RESOLVED
Last Resolved: 8 months ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.