Closed Bug 1380936 Opened 3 years ago Closed 3 years ago

Add Minimize Window command to geckodriver

Categories

(Testing :: geckodriver, defect)

defect
Not set
normal

Tracking

(firefox57 fixed)

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: Sl0v3C, Assigned: Sl0v3C)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

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.
Attachment #8886508 - Flags: review?(ato) → 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)
Attachment #8886508 - Attachment is obsolete: true
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 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-
Attachment #8887317 - Flags: review?(ato) → review?(hskupin)
Attachment #8887317 - Flags: review?(hskupin)
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.
Attachment #8887317 - Flags: review?(ato) → review-
(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
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.
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.
https://hg.mozilla.org/mozilla-central/rev/1255a559da03
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.