Closed Bug 1396866 Opened 7 years ago Closed 7 years ago

Correct ElementResponseRect and WindowResponseRect types

Categories

(Testing :: geckodriver, enhancement)

Version 3
enhancement
Not set
normal

Tracking

(firefox57 fixed)

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: ato, Assigned: ato)

References

Details

Attachments

(3 files)

The field types for ElementResponseRect and WindowResponseRect needs
to be adjusted so that we use the correct data types according to
CSS.

A DOMRect (https://drafts.fxtf.org/geometry-1/#domrect) uses
‘unrestricted doubles’ for its x/y/width/height fields, which is
equivalent to an f64 with a finite check.  Fortunately the ‘Get
Element Rect’ command only returns data and does not have a
corresponding setter.

For WindowProxy
(https://drafts.csswg.org/cssom-view/#extensions-to-the-window-inter
face) the screenX/screenY/outerWidth/outerHeight fields are
platform-independent longs (meaning the bitness of the system
is not taken into account).  This is defined in WebIDL as “a
signed integer type that has values in the range [−2147483648,
2147483647]”.  This can be represented with an i32 internally.

Its corresponding setter, ‘Set Window Rect’ accepts all the same
fields but the WebDriver specification says that the input type
should be a JSON Number with the following restrictions:

> If width or height is neither null nor a Number from 0 to 264 − 1, return error with error code invalid argument.
> 
> If x or y is neither null nor a Number from −(263) to 263 − 1, return error with error code invalid argument.

(The specification uses 64-bit floats, but I’ve submitted
https://github.com/w3c/webdriver/pull/1084 to fix it to use 32-bit.)

This means that WebDriver imposes a further restriction on the
input of width/height, where we should use i32 as the internal data
type but also perform a non-negativity check on input because a
window’s size presumably cannot be negative.

Finally, all values need to be coerced from integer to float
(for ElementResponseRect) and from floats to integers (for
WindowResponseRect) because we want to be generous in what we
accept, but conservative in what we return.
Blocks: 1391605
Assignee: nobody → ato
Status: NEW → ASSIGNED
OS: Unspecified → All
Hardware: Unspecified → All
Blocks: 1397780
Comment on attachment 8906260 [details]
Bug 1396866 - Relax WindowRectResponse typing.

https://reviewboard.mozilla.org/r/178012/#review183196

::: testing/geckodriver/src/marionette.rs:807
(Diff revision 1)
>                               ErrorStatus::UnknownError,
> -                             "Failed to find y field").as_f64(),
> +                             "Failed to find y field").as_i64(),
>                      ErrorStatus::UnknownError,
> -                    "Failed to interpret y as float");
> +                    "Failed to interpret y as integer");
>  
> -                let rect = WindowRectResponse { x, y, width, height };
> +                let rect = WindowRectResponse {

Are we happy to let this overflow on an invalid response from marionette?
Attachment #8906260 - Flags: review?(james) → review+
Comment on attachment 8906259 [details]
Bug 1396866 - Restore window state on maximizing, minimizing, and fullscreening.

https://reviewboard.mozilla.org/r/178010/#review183782

::: testing/web-platform/tests/webdriver/tests/contexts/maximize_window.py:44
(Diff revision 1)
> +    """TODO"""
> +
> +
> +def test_handle_prompt_accept(new_session):
> +    """
> +    3. Handle any user prompts and return its value if it is an error.

I am not a big fan of copy pasting the spec here as it means any changes will require us to have to update all this text where ideally we can just look at the test.
Attachment #8906259 - Flags: review?(dburns) → review+
Comment on attachment 8906261 [details]
Bug 1396866 - Disable test_maximize_when_resized_to_max_size.

https://reviewboard.mozilla.org/r/178014/#review183784

::: testing/web-platform/tests/webdriver/tests/contexts/maximize_window.py:253
(Diff revision 1)
>      assert_success(second_response)
>      assert session.window.size == max_size
>  
>  
> +"""
> +TODO(ato): Implicit session start does not use configuration passed on

Can't we have this as a expected failure?
Attachment #8906261 - Flags: review?(dburns) → review+
Comment on attachment 8906259 [details]
Bug 1396866 - Restore window state on maximizing, minimizing, and fullscreening.

https://reviewboard.mozilla.org/r/178010/#review183782

> I am not a big fan of copy pasting the spec here as it means any changes will require us to have to update all this text where ideally we can just look at the test.

I thought jugglinmike had discussed this in some discussion, somewhere, that this was the preferred style.  If you don’t like it you should raise a discussion on the WG mailing list as it is now the predominant style in the wdspec tests.

If we decide not to do this, we should also do away with “step N” comments.
Comment on attachment 8906260 [details]
Bug 1396866 - Relax WindowRectResponse typing.

https://reviewboard.mozilla.org/r/178012/#review183196

> Are we happy to let this overflow on an invalid response from marionette?

Yes, these come directly from DOM so I’m happy to trust what that gives us.  If it turns out to be a problem we can revisit this.
Comment on attachment 8906261 [details]
Bug 1396866 - Disable test_maximize_when_resized_to_max_size.

https://reviewboard.mozilla.org/r/178014/#review183784

> Can't we have this as a expected failure?

No because it causes a failure if marked as unexpected-failure, and an error otherwise because of https://bugzil.la/1398459.
Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/dcadd5c09851
Restore window state on maximizing, minimizing, and fullscreening. r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/793de7b5f147
Relax WindowRectResponse typing. r=jgraham
https://hg.mozilla.org/integration/autoland/rev/1463cc055cf9
Disable test_maximize_when_resized_to_max_size. r=automatedtester
No longer blocks: 1397780
You need to log in before you can comment on or make changes to this bug.