Implemented webdriver SetWindowSize.

[asajeffrey]

Thank you for contributing to Servo! Please replace each [ ] by [X] when the step is complete, and replace __ with appropriate data:

• ./mach build -d does not report any errors
• ./mach test-tidy does not report any errors
• These changes fix #10467 (github issue number if applicable).

Either:

• There are tests for these changes OR
• These changes do not require tests because the new tests are in web-platform-tests/wpt#3024

Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process.

---

This change is

[asajeffrey]

r? @jgraham

[asajeffrey]

Must remember to update #8623 if/when this merges.

[jgraham]

Reviewed 7 of 7 files at r1.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

components/script/webdriver_handlers.rs, line 302 [r1] (raw file):

    let untyped = size.to_untyped();
    // TODO: when window puts in security checks for resize, we will need to rewrite this
    window.ResizeTo(untyped.width as i32, untyped.height as i32);

This is async; I think the webdriver spec currently expects the resize to be synchronous.

---

components/webdriver_server/lib.rs, line 375 [r1] (raw file):

    fn handle_set_window_size(&self, params: &WindowSizeParameters) -> WebDriverResult<WebDriverResponse> {
        let (sender, receiver) = ipc::channel().unwrap();
        let size = Size2D::from_untyped(&Size2D::new(params.width as f32, params.height as f32));

What happens if the size is greater than the size of the screen?

---

Comments from Reviewable

[asajeffrey]

Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

components/script/webdriver_handlers.rs, line 302 [r1] (raw file):

Previously, jgraham wrote…

This is async; I think the webdriver spec currently expects the resize to be synchronous.

Hmm, true. This is going to be a bit painful to implement, as resizing in the compositor is async. How wedded is the spec to synchronous resize?

---

Comments from Reviewable

[jgraham]

Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

components/script/webdriver_handlers.rs, line 302 [r1] (raw file):

Previously, asajeffrey (Alan Jeffrey) wrote…

Hmm, true. This is going to be a bit painful to implement, as resizing in the compositor is async. How wedded is the spec to synchronous resize?

Well, it wold be nice, but I guess I can argue that on some platforms it might not be possible to implement in a sync way.

---

Comments from Reviewable

[asajeffrey]

Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

components/script/webdriver_handlers.rs, line 302 [r1] (raw file):

Previously, jgraham wrote…

Well, it wold be nice, but I guess I can argue that on some platforms it might not be possible to implement in a sync way.

So how to proceed? I can reimplement this as sync, but it's going to be quite a bit of rewiring, which I'd rather not do if we think we're going to rewrite the spec.

---

Comments from Reviewable

[highfive]

New code was committed to pull request.

[asajeffrey]

I made SetWindowSize wait for a resize event before responding.

[highfive]

New code was committed to pull request.

[asajeffrey]

Added a timeout to SetWindowSize.

[bors-servo]

☔ The latest upstream changes (presumably #11243) made this pull request unmergeable. Please resolve the merge conflicts.

[asajeffrey]

Rebased.

[bors-servo]

☔ The latest upstream changes (presumably #11274) made this pull request unmergeable. Please resolve the merge conflicts.

[jgraham]

Reviewed 3 of 8 files at r4, 2 of 6 files at r5, 3 of 4 files at r6.
Review status: 8 of 9 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

---

components/webdriver_server/lib.rs, line 226 [r6] (raw file):

            script_timeout: 30_000,
            load_timeout: 300_000,
            resize_timeout: 30_000,

I think this should be much smaller like a few hundred ms?

---

components/webdriver_server/lib.rs, line 368 [r6] (raw file):

        let window_size = receiver.recv().unwrap();
        let vp = window_size.visible_viewport;

I think this is wrong per spec, as it is supposed to include the window border &c. I am not particularly worried about that at this time (I am also not sure that the spec is really right here).

---

components/webdriver_server/lib.rs, line 393 [r6] (raw file):

                Ok(WebDriverResponse::WindowSize(window_size_response))
            },
            None => Err(WebDriverError::new(ErrorStatus::Timeout, "Resize timed out")),

I think in this case we should return the current window size.

---

Comments from Reviewable

[asajeffrey]

Rebased and fixed.

Previously, bors-servo wrote…

☔ The latest upstream changes (presumably #11274) made this pull request unmergeable. Please resolve the merge conflicts.

---

Review status: 8 of 9 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

components/webdriver_server/lib.rs, line 226 [r6] (raw file):

Previously, jgraham wrote…

I think this should be much smaller like a few hundred ms?

Changed it to 500.

---

components/webdriver_server/lib.rs, line 393 [r6] (raw file):

Previously, jgraham wrote…

I think in this case we should return the current window size.

Done.

---

Comments from Reviewable

[jgraham]

Reviewed 1 of 6 files at r8, 1 of 4 files at r9, 5 of 5 files at r10.
Review status: 8 of 9 files reviewed at latest revision, 1 unresolved discussion.

---

Comments from Reviewable

[jgraham]

Reviewed 1 of 6 files at r8.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[jgraham]

@bors-servo r+

[bors-servo]

📌 Commit 8797f6f has been approved by jgraham

[bors-servo]

⌛ Testing commit 8797f6f with merge 2063bde...

[bors-servo]

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows

[asajeffrey]

Oops, this should have been squashed before merging.

[jgraham]

Ah, sorry :(

[asajeffrey]

No biggie.