Add Minimize Window command to geckodriver

RESOLVED FIXED in Firefox 57

Status

defect
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Sl0v3C, Assigned: Sl0v3C)

Tracking

(Blocks 1 bug)

Trunk
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

2 years ago
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:56.0) Gecko/20100101 Firefox/56.0
Build ID: 20170705104852

Steps to reproduce:

No support for 'minimize' windows command


Actual results:

window cannot be minimized


Expected results:

window should be minimized
Comment hidden (mozreview-request)

Comment 2

2 years ago
mozreview-review
Comment on attachment 8886508 [details]
Bug 1380936 - Add support for 'minimize' command in geckodriver;

https://reviewboard.mozilla.org/r/157326/#review162834

::: commit-message-8ec32:1
(Diff revision 1)
> +Bug 1380936 - Add support for 'minimize' command in geckodriver; r?ato,whimboo
> +Need to merge after webdriver-rust update to include the commit "Implement the minimize window for webdriver-rust (#105)"

There should be a line after the first line of the commit message, and the second block shouldn’t exceed ~72 characters.

It would also be better to include a canonical URI to the PR instead of its title.

::: commit-message-8ec32:1
(Diff revision 1)
> +Bug 1380936 - Add support for 'minimize' command in geckodriver; r?ato,whimboo

I would rephrase this as “Add Minimize Window command to geckodriver”.

::: testing/geckodriver/src/marionette.rs:1015
(Diff revision 1)
>              CloseWindow => (Some("close"), None),
>              GetTimeouts => (Some("getTimeouts"), None),
>              SetTimeouts(ref x) => (Some("setTimeouts"), Some(x.to_marionette())),
>              SetWindowRect(ref x) => (Some("setWindowRect"), Some(x.to_marionette())),
>              GetWindowRect => (Some("getWindowRect"), None),
> +            MinimizeWindow => (Some("minimizeWindow"), None),

The Marionette command should be added as "WebDriver:MinimizeWindow" because it is a new command.  geckodriver uses some legacy Marionette command names due to backwards compatibility.

See https://searchfox.org/mozilla-central/rev/01d27fdd3946f7210da91b18fcccca01d7324fe2/testing/marionette/driver.js#3332 for further information.
Attachment #8886508 - Flags: review?(ato) → review-

Comment 3

2 years ago
mozreview-review
Comment on attachment 8886508 [details]
Bug 1380936 - Add support for 'minimize' command in geckodriver;

https://reviewboard.mozilla.org/r/157326/#review162838

Code-wise this seems to look fine, except that I do not see a version bump for the webdriver crate yet. As such I will remove the review flag for now.

::: commit-message-8ec32:2
(Diff revision 1)
> +Bug 1380936 - Add support for 'minimize' command in geckodriver; r?ato,whimboo
> +Need to merge after webdriver-rust update to include the commit "Implement the minimize window for webdriver-rust (#105)"

You should definitely update the commit message here, so that it explains what is getting added in more detail. Maybe check other patches for geckodriver to see which details others added to the message.
Attachment #8886508 - Flags: review?(hskupin)
(Assignee)

Updated

2 years ago
Attachment #8886508 - Attachment is obsolete: true
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8887317 - Flags: review?(hskupin)
Assignee: nobody → yangyi_peng
Blocks: webdriver
Status: UNCONFIRMED → ASSIGNED
Depends on: 1378121
Ever confirmed: true
OS: Unspecified → All
Hardware: Unspecified → All
Version: 55 Branch → Trunk
Summary: No support for 'minimize' windows command → Add Minimize Window command to geckodriver

Comment 5

2 years ago
mozreview-review
Comment on attachment 8887317 [details]
Bug 1380936 - Add Minimize Window command to geckodriver;

https://reviewboard.mozilla.org/r/158136/#review163598

One tiny little change is necessary.

You also need to bump the webdriver crate, which was just released, to 0.28.0 (https://crates.io/crates/webdriver/0.28.0).  When you do, I think you should be able to run `./mach vendor rust` from the top-level source directory instead of `cargo update` as you normally would, because we vendor the dependencies in-tree.

::: testing/geckodriver/src/marionette.rs:1015
(Diff revision 1)
>              CloseWindow => (Some("close"), None),
>              GetTimeouts => (Some("getTimeouts"), None),
>              SetTimeouts(ref x) => (Some("setTimeouts"), Some(x.to_marionette())),
>              SetWindowRect(ref x) => (Some("setWindowRect"), Some(x.to_marionette())),
>              GetWindowRect => (Some("getWindowRect"), None),
> +            MinimizeWindow => (Some("minimizeWindow"), None),

Call WebDriver:MinimizeWindow here.
Attachment #8887317 - Flags: review?(ato) → review-
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8887317 - Flags: review?(ato) → review?(hskupin)
Attachment #8887317 - Flags: review?(hskupin)
(Assignee)

Comment 7

2 years ago
mozreview-review
Comment on attachment 8887317 [details]
Bug 1380936 - Add Minimize Window command to geckodriver;

https://reviewboard.mozilla.org/r/158136/#review164974
This likely needs to be rebased now.  You need to change
testing/geckodriver/Cargo.toml’s webdriver entry to "0.28.0", then
run `./mach vendor rust` to update the dependencies.  Note that
you cannot use `cargo update` directly as we vendor Rust library
dependencies in third_party/rust.
(Assignee)

Comment 9

2 years ago
(In reply to Andreas Tolfsen ‹:ato› from comment #8)
> This likely needs to be rebased now.  You need to change
> testing/geckodriver/Cargo.toml’s webdriver entry to "0.28.0", then
> run `./mach vendor rust` to update the dependencies.  Note that
> you cannot use `cargo update` directly as we vendor Rust library
> dependencies in third_party/rust.

So when I finish the `./mach vendor rust`, should I add all the updated dependencies into the commit?
Thanks.
Comment hidden (mozreview-request)

Comment 11

2 years ago
mozreview-review
Comment on attachment 8887317 [details]
Bug 1380936 - Add Minimize Window command to geckodriver;

https://reviewboard.mozilla.org/r/158136/#review168822

::: testing/geckodriver/src/marionette.rs:31
(Diff revision 3)
> -    GetWindowHandles, CloseWindow, SetWindowRect,
> -    GetWindowRect, MaximizeWindow, FullscreenWindow, SwitchToWindow, SwitchToFrame,
> +    GetWindowHandles, CloseWindow, SetWindowRect, GetWindowRect,
> +    MinimizeWindow, MaximizeWindow, FullscreenWindow, SwitchToWindow, SwitchToFrame

This causes a build error (see https://treeherder.mozilla.org/#/jobs?repo=try&revision=05f7547949fb&selectedJob=119997208) because there is no comma after SwitchToFrame.
Attachment #8887317 - Flags: review?(ato) → review-
(Assignee)

Comment 12

2 years ago
(In reply to Andreas Tolfsen ‹:ato› from comment #11)
> Comment on attachment 8887317 [details]
> Bug 1380936 - Add Minimize Window command to geckodriver;
> 
> https://reviewboard.mozilla.org/r/158136/#review168822
> 
> ::: testing/geckodriver/src/marionette.rs:31
> (Diff revision 3)
> > -    GetWindowHandles, CloseWindow, SetWindowRect,
> > -    GetWindowRect, MaximizeWindow, FullscreenWindow, SwitchToWindow, SwitchToFrame,
> > +    GetWindowHandles, CloseWindow, SetWindowRect, GetWindowRect,
> > +    MinimizeWindow, MaximizeWindow, FullscreenWindow, SwitchToWindow, SwitchToFrame
> 
> This causes a build error (see
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=05f7547949fb&selectedJob=119997208) because there is
> no comma after SwitchToFrame.

@@ Sorry for this error, it should be made by re-pushing the commit.
Thanks.
Comment hidden (mozreview-request)

Comment 14

2 years ago
mozreview-review
Comment on attachment 8887317 [details]
Bug 1380936 - Add Minimize Window command to geckodriver;

https://reviewboard.mozilla.org/r/158136/#review169154

The try run looks good now: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c607c713ed6&group_state=expanded
Attachment #8887317 - Flags: review?(ato) → review+
And thanks again for the excellent patch!  If you want to continue
submitting patches to Gecko, I will apply for commit access level
1 for you, which will grant you permissions to push to the try/CI
server to test your changes.

Let me know on IRC (I’m ato in #ateam on irc.mozilla.org) or by
email if you want this.

Comment 16

2 years ago
Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1255a559da03
Add Minimize Window command to geckodriver; r=ato
(Assignee)

Comment 17

2 years ago
(In reply to Andreas Tolfsen ‹:ato› from comment #15)
> And thanks again for the excellent patch!  If you want to continue
> submitting patches to Gecko, I will apply for commit access level
> 1 for you, which will grant you permissions to push to the try/CI
> server to test your changes.
> 
> Let me know on IRC (I’m ato in #ateam on irc.mozilla.org) or by
> email if you want this.

Hi ato,
Thanks for your great help!
I will push the wdspec test part for these related patches later.
You can help me to apply for commit access level 1 for me.
Thanks again.

Comment 18

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1255a559da03
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.