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 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 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 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 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.

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

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

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.

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

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