1380936 - Add Minimize Window command to geckodriver

Status: RESOLVED FIXED

(Testing :: geckodriver, defect)

Description

[yangyi_peng (:Sl0v3C)]

Attachment: Add_support_for_minimize_Command_in_geckodriver.patch

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 1

[yangyi_peng (:Sl0v3C)]

Attachment: Bug 1380936 - Add support for 'minimize' command in geckodriver;

Need to merge after webdriver-rust update to include the commit "Implement the minimize window for webdriver-rust (#105)"

Review commit: https://reviewboard.mozilla.org/r/157326/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/157326/

Comment 2

[Andreas Tolfsen ❲:ato❳]

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.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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.

Comment 4

[yangyi_peng (:Sl0v3C)]

Attachment: Bug 1380936 - Add Minimize Window command to geckodriver;

Merge after webdriver-rust released contains commit:
https://github.com/mozilla/webdriver-rust/pull/105

Review commit: https://reviewboard.mozilla.org/r/158136/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/158136/

Comment 5

[Andreas Tolfsen ❲:ato❳]

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.

Comment 6

[yangyi_peng (:Sl0v3C)]

Comment on attachment 8887317 [details]
Bug 1380936 - Add Minimize Window command to geckodriver;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/158136/diff/1-2/

Comment 7

[yangyi_peng (:Sl0v3C)]

Comment on attachment 8887317 [details]
Bug 1380936 - Add Minimize Window command to geckodriver;

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

Comment 8

[Andreas Tolfsen ❲:ato❳]

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.

Comment 9

[yangyi_peng (:Sl0v3C)]

(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 10

[yangyi_peng (:Sl0v3C)]

Comment on attachment 8887317 [details]
Bug 1380936 - Add Minimize Window command to geckodriver;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/158136/diff/2-3/

Comment 11

[Andreas Tolfsen ❲:ato❳]

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.

Comment 12

[yangyi_peng (:Sl0v3C)]

(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 13

[yangyi_peng (:Sl0v3C)]

Comment on attachment 8887317 [details]
Bug 1380936 - Add Minimize Window command to geckodriver;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/158136/diff/3-4/

Comment 14

[Andreas Tolfsen ❲:ato❳]

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

Comment 15

[Andreas Tolfsen ❲:ato❳]

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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1255a559da03
Add Minimize Window command to geckodriver; r=ato

Comment 17

[yangyi_peng (:Sl0v3C)]

(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

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1255a559da03