Add Minimize Window command to Marionette

RESOLVED FIXED in Firefox 56

Status

P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Sl0v3C, Assigned: Sl0v3C)

Tracking

(Blocks: 1 bug, {pi-marionette-server})

unspecified
mozilla56
pi-marionette-server
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

(URL)

Attachments

(4 attachments, 2 obsolete attachments)

(Assignee)

Description

2 years ago
User Agent: Mozilla/5.0 (Windows NT 6.1) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/58.0.3029.110 Safari/537.36

Steps to reproduce:

Using selenium-python 3.4.3 firefox webdriver in Python script.
There is no minimize_window() function to minimize the firefox.
However w3c webdriver APIs contains the minimize command.


Actual results:

Cannot minimize the firefox


Expected results:

Selenium should add this command. And firefox should add the support for this command in marionette.
(Assignee)

Comment 1

2 years ago
I had patches for this BUG:
1. webdriver-rust part
2. geckodriver part
3. firefox(marionette) part
4. selenium-python part
item 1 & item 2 I had make PRs in their github project.
item 3 I don't know how to add reviewer and I cannot push it directly.
item 4 I think should be checked in at last, since this will influence the user when the support not ready in firefox or geckodriver part.

I want to someone can review the patch as attachments.
Please report the issue to the Selenium developer.
Status: UNCONFIRMED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → INVALID
Hmm, sorry, I pushed the button too quick.
Status: RESOLVED → REOPENED
Component: Untriaged → Marionette
Ever confirmed: true
Product: Firefox → Testing
Resolution: INVALID → ---
Version: 56 Branch → unspecified
Blocks: 721859
Status: REOPENED → NEW
Summary: When using selenium, there is no minimize command → Add support for `minimize` command
Attachment #8883291 - Attachment mime type: text/x-patch → text/plain
@yangi_peng, thank you for the patch! Would you mind also adding some unit tests for this feature, and as best supply the patch via mozreview? That would be wonderful. For details please see here:

https://wiki.mozilla.org/User:Mjzffr/New_Contributors
Flags: needinfo?(yangyi_peng)
(Assignee)

Comment 5

2 years ago
Add minimizeWindow command unit test
Flags: needinfo?(yangyi_peng)
(Assignee)

Comment 6

2 years ago
(In reply to Henrik Skupin (:whimboo) from comment #4)
> @yangi_peng, thank you for the patch! Would you mind also adding some unit
> tests for this feature, and as best supply the patch via mozreview? That
> would be wonderful. For details please see here:
> 
> https://wiki.mozilla.org/User:Mjzffr/New_Contributors

@Henrik Skupin, thanks for your help. I already wrote the unit test and verified in my local side.
Uploaded it as the attachment.
I will file a bug to ask for the Commit Access later and learn how to push patch to the mozreview.
THX again!
Great. So basically there is no need to file a bug for push access. You won't need it those days when pushing patches to mozreview. Let me know if you have problems with setting up mozreview. You can reach us in #ateam on irc.mozilla.org.
Thanks for the patches!

geckodriver is now developed in mozilla-central, so a patch here for that part would be welcome. We will need to bump and release the webdriver library.

If possible I would appreciate a spec test under testing/web-platform/tests/webdriver for this behaviour, since that's more valuable than a marionette-only test.
Comment on attachment 8883473 [details] [diff] [review]
add_unit_test_4_minimize_command_in_marionette.patch

Review of attachment 8883473 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/marionette/client/marionette_driver/marionette.py
@@ +2045,5 @@
> +    def minimize_window(self):
> +        """ Resize the browser window currently receiving commands. The action
> +        should be equivalent to the user pressing the maximize button
> +        """
> +        return self._send_message("minimizeWindow")

I doubt this works in practice, since there is no minimizeWindow command in the server.
Attachment #8883473 - Flags: feedback-
(Assignee)

Comment 10

2 years ago
(In reply to Andreas Tolfsen ‹:ato› from comment #9)
> Comment on attachment 8883473 [details] [diff] [review]
> add_unit_test_4_minimize_command_in_marionette.patch
> 
> Review of attachment 8883473 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: testing/marionette/client/marionette_driver/marionette.py
> @@ +2045,5 @@
> > +    def minimize_window(self):
> > +        """ Resize the browser window currently receiving commands. The action
> > +        should be equivalent to the user pressing the maximize button
> > +        """
> > +        return self._send_message("minimizeWindow")
> 
> I doubt this works in practice, since there is no minimizeWindow command in
> the server.

@Andreas Tolfsen ‹:ato› Please help to check the first patch named "Add_minimizeWindow_command_in_marionette.patch". This unit test patch is based on merged the first patch.
What's more, I had verified this patch locally.
I will push the patches into the mozreview later.
Many thanks.
Comment hidden (mozreview-request)
(Assignee)

Comment 12

2 years ago
mozreview-review
Comment on attachment 8886493 [details]
Bug 1378121 - Add Window Minimize wdspec tests;

https://reviewboard.mozilla.org/r/157296/#review162786
Comment hidden (mozreview-request)
yangyi, can you please also add the Marionette changes as a separate commit to the mozreview patch? It should end-up before the wdspec test, or could even be in the same commit.
Assignee: nobody → yangyi_peng
Status: NEW → ASSIGNED
Flags: needinfo?(yangyi_peng)
(Assignee)

Comment 15

2 years ago
(In reply to Henrik Skupin (:whimboo) from comment #14)
> yangyi, can you please also add the Marionette changes as a separate commit
> to the mozreview patch? It should end-up before the wdspec test, or could
> even be in the same commit.

Sorry, I just add the bug ID, then the marionette commit changed to the wdspec test commit.
I am not familiar with the hg tool, I often use git.
Now I just want to push them as the same commit, what can I do?
THX!
Flags: needinfo?(yangyi_peng)
Please check with 'hg histedit' if both commits are present under your currently selected bookmark. If that is the case just fold in the former Marionette commit, and adjust the commit message and details.

Comment 17

2 years ago
mozreview-review
Comment on attachment 8886493 [details]
Bug 1378121 - Add Window Minimize wdspec tests;

https://reviewboard.mozilla.org/r/157296/#review162830

Drive-by review.

::: testing/web-platform/tests/webdriver/window_minimizing.py:23
(Diff revision 2)
> +    result = session.transport.send("POST", "session/%s/window/minimize" % session.session_id)
> +
> +    assert_error(result, "unexpected alert open")
> +
> +
> +def test_minimize_payload(session):

Missing test to restore (unminimise) window.

::: testing/web-platform/tests/webdriver/window_minimizing.py:24
(Diff revision 2)
> +
> +    assert_error(result, "unexpected alert open")
> +
> +
> +def test_minimize_payload(session):
> +    result = session.transport.send("POST", "session/%s/window/minimize" % session.session_id)

This returns a window rect which you need to test.

::: testing/web-platform/tests/webdriver/window_minimizing.py:27
(Diff revision 2)
> +
> +def test_minimize_payload(session):
> +    result = session.transport.send("POST", "session/%s/window/minimize" % session.session_id)
> +
> +    assert result.status == 200
> +    visible = str(session.execute_script("return window.document.visibilityState"))

Unnecessary string coercion.
Attachment #8886493 - Flags: review-
(Assignee)

Updated

2 years ago
Attachment #8886493 - Attachment is obsolete: true
Attachment #8886493 - Flags: review?(james)
Comment hidden (mozreview-request)
(Assignee)

Comment 19

2 years ago
mozreview-review
Comment on attachment 8887311 [details]
Bug 1378121 - Add Minimize Window command to marionette;

https://reviewboard.mozilla.org/r/158126/#review163286

Hi All,
The webspec test part I will push it after geckodriver & marionette parts both merged.
And I wonder why should check the rect after minimized the window. The rect seems no change after minimized the window.
I think only restore the window should check the rect is the same with the origin one or not.

Thanks.

Comment 20

2 years ago
mozreview-review
Comment on attachment 8887311 [details]
Bug 1378121 - Add Minimize Window command to marionette;

https://reviewboard.mozilla.org/r/158126/#review163586

This is very good!  Thanks for the patch.  I think this is close to being in a good state to land.  However, I want to request that you make a few minor changes.

::: commit-message-d4377:1
(Diff revision 1)
> +Bug 1378121 - Add Minimize Window command & unit test to marionette; r=whimboo, ato

Skip the part about adding unit tests.  Saying “Add Minimize Window command to Marionette” is sufficient.

::: testing/marionette/client/marionette_driver/marionette.py:2047
(Diff revision 1)
>          body = {"width": width, "height": height}
>          return self._send_message("setWindowSize", body)
>  
> +    def minimize_window(self):
> +        """ Resize the browser window currently receiving commands. The action
> +        should be equivalent to the user pressing the maximize button

s/should be/is/ and s/maximize/minimize/

::: testing/marionette/client/marionette_driver/marionette.py:2049
(Diff revision 1)
>  
> +    def minimize_window(self):
> +        """ Resize the browser window currently receiving commands. The action
> +        should be equivalent to the user pressing the maximize button
> +        """
> +        return self._send_message("minimizeWindow")

Use WebDriver:MinimizeWindow.

::: testing/marionette/driver.js:2906
(Diff revision 1)
> +  resp.body = {
> +    x: win.screenX,
> +    y: win.screenY,
> +    width: win.outerWidth,
> +    height: win.outerHeight,
> +    windowState: win.windowState,

I think it’s a good idea to return a property on the window’s current state.  The WebDriver specification allows us to include additional JSON fields.

However, I don’t think we should reuse the STATE_MINIMIZED and STATE_NORMAL because they are magic numbers.  Can you change this to return a field "state" (not "windowState") with a string "minimized"/"normal"?

::: testing/marionette/driver.js:3511
(Diff revision 1)
>    "goBack": GeckoDriver.prototype.goBack,
>    "goForward": GeckoDriver.prototype.goForward,
>    "isElementDisplayed": GeckoDriver.prototype.isElementDisplayed,
>    "isElementEnabled": GeckoDriver.prototype.isElementEnabled,
>    "isElementSelected": GeckoDriver.prototype.isElementSelected,
> +  "minimizeWindow": GeckoDriver.prototype.minimizeWindow,

We don’t need this.

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_minimize.py:16
(Diff revision 1)
> +
> +class TestWindowMinimize(MarionetteTestCase):
> +
> +    def setUp(self):
> +        MarionetteTestCase.setUp(self)
> +        self.visible = ""

Unused.
Attachment #8887311 - Flags: review?(ato) → review-
Yangyi, thanks for the patch!  This is great!

(In reply to yangyi_peng (:Sl0v3C) from comment #19)

> The webspec test part I will push it after geckodriver &
> marionette parts both merged.

Good plan.

> And I wonder why should check the rect after minimized the
> window. The rect seems no change after minimized the window.
> 
> I think only restore the window should check the rect is the same
> with the origin one or not.

That is strange, I agree.  I think you made the right call to
include a window state field in the returned JSON Object.  Even if
this isn’t specified in the WebDriver standard, we can optionally
include more data.

As I noted in the review, I want you to rename this field from
windowState to state and make it return a string instead of the
Mozilla-proprietary magic numbers from nsIDOMChromeWindow.
Summary: Add support for `minimize` command → Add Minimize Window command to Marionette
Keywords: ateam-marionette-server
OS: Unspecified → All
Priority: -- → P1
Hardware: Unspecified → All
Comment hidden (mozreview-request)
(Assignee)

Comment 23

2 years ago
(In reply to Andreas Tolfsen ‹:ato› from comment #21)
> Yangyi, thanks for the patch!  This is great!
> 
> (In reply to yangyi_peng (:Sl0v3C) from comment #19)
> 
> > The webspec test part I will push it after geckodriver &
> > marionette parts both merged.
> 
> Good plan.
> 
> > And I wonder why should check the rect after minimized the
> > window. The rect seems no change after minimized the window.
> > 
> > I think only restore the window should check the rect is the same
> > with the origin one or not.
> 
> That is strange, I agree.  I think you made the right call to
> include a window state field in the returned JSON Object.  Even if
> this isn’t specified in the WebDriver standard, we can optionally
> include more data.
> 
> As I noted in the review, I want you to rename this field from
> windowState to state and make it return a string instead of the
> Mozilla-proprietary magic numbers from nsIDOMChromeWindow.

Hi ato,
I pushed a new commit with modification as you said. Link: https://reviewboard.mozilla.org/r/158126/diff/2/
I have a question about the "Diff Revision" the 1st version I had modified the unit-tests.ini
But the 2nd version I don't modify it and I found the Diff Revision 2 didn't have this file.
Does these 2 version modifications will all be merged into code base or only the 2nd version.
What's more, when I modify the Marionette part based on the commit "https://reviewboard.mozilla.org/r/158136/"
in my local code base. Should I revert it and then modify the Marionette part then push the new commit.
I am confused with this.
Thanks!
(Assignee)

Comment 24

2 years ago
mozreview-review
Comment on attachment 8887311 [details]
Bug 1378121 - Add Minimize Window command to marionette;

https://reviewboard.mozilla.org/r/158126/#review164970

Comment 25

2 years ago
mozreview-review
Comment on attachment 8887311 [details]
Bug 1378121 - Add Minimize Window command to marionette;

https://reviewboard.mozilla.org/r/158126/#review165024

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

You appear to be removing MinimizeWindow here?

::: testing/geckodriver/src/marionette.rs:738
(Diff revision 2)
>                      ErrorStatus::UnknownError,
>                      "Failed to interpret width as float");
>  
>                  WebDriverResponse::ElementRect(RectResponse::new(x, y, width, height))
>              },
> -            FullscreenWindow | MinimizeWindow | MaximizeWindow | GetWindowRect |
> +            FullscreenWindow | MaximizeWindow | GetWindowRect |

And here?

::: testing/marionette/driver.js:2891
(Diff revision 2)
>  GeckoDriver.prototype.minimizeWindow = function* (cmd, resp) {
>    assert.firefox();
>    const win = assert.window(this.getCurrentWindow());
>    assert.noUserPrompt(this.dialog);
>  
> +  var current_state;

Nit: We use camel casing on all variables in the Gecko code base.

s/current_state/state/g to fix this.

::: testing/marionette/driver.js:2891
(Diff revision 2)
>  GeckoDriver.prototype.minimizeWindow = function* (cmd, resp) {
>    assert.firefox();
>    const win = assert.window(this.getCurrentWindow());
>    assert.noUserPrompt(this.dialog);
>  
> +  var current_state;

To scope this correctly, use let instead of var.
Attachment #8887311 - Flags: review?(ato) → review-
(In reply to yangyi_peng (:Sl0v3C) from comment #23)

> I pushed a new commit with modification as you said. Link:
> https://reviewboard.mozilla.org/r/158126/diff/2/ I have a question
> about the "Diff Revision" the 1st version I had modified the
> unit-tests.ini But the 2nd version I don't modify it and I found
> the Diff Revision 2 didn't have this file.

This is an interdiff between the changes from your first
modification of the original commit and the second. mozreview tracks
changes in commits between pushes so that it’s easier for the
reviewer to see what changed in a fixup.

In the second fixups, you do appear to have reverted the change to
unit-tests.ini, as you can see in the diff range between revision 1
and 2:

	https://reviewboard.mozilla.org/r/158126/diff/1-2/

> Does these 2 version modifications will all be merged
> into code base or only the 2nd version.  What's more,
> when I modify the Marionette part based on the commit
> "https://reviewboard.mozilla.org/r/158136/" in my local code
> base. Should I revert it and then modify the Marionette part then
> push the new commit.

It’s the full diff range that will be integrated; that is, the
range from orig..final.

To edit your commit, just check out the branch and amend the commit.
You don’t normally have to revert or re-do anything.  When you
have made the appropriate changes to your commit, you push them up
to mozreview again, and it will create interdiffs.

As a patch author, all you need to care about is the number of
commits and their order.
(Assignee)

Comment 27

2 years ago
(In reply to Andreas Tolfsen ‹:ato› from comment #26)
> (In reply to yangyi_peng (:Sl0v3C) from comment #23)
> 
> > I pushed a new commit with modification as you said. Link:
> > https://reviewboard.mozilla.org/r/158126/diff/2/ I have a question
> > about the "Diff Revision" the 1st version I had modified the
> > unit-tests.ini But the 2nd version I don't modify it and I found
> > the Diff Revision 2 didn't have this file.
> 
> This is an interdiff between the changes from your first
> modification of the original commit and the second. mozreview tracks
> changes in commits between pushes so that it’s easier for the
> reviewer to see what changed in a fixup.
> 
> In the second fixups, you do appear to have reverted the change to
> unit-tests.ini, as you can see in the diff range between revision 1
> and 2:
> 
> 	https://reviewboard.mozilla.org/r/158126/diff/1-2/
> 
> > Does these 2 version modifications will all be merged
> > into code base or only the 2nd version.  What's more,
> > when I modify the Marionette part based on the commit
> > "https://reviewboard.mozilla.org/r/158136/" in my local code
> > base. Should I revert it and then modify the Marionette part then
> > push the new commit.
> 
> It’s the full diff range that will be integrated; that is, the
> range from orig..final.
> 
> To edit your commit, just check out the branch and amend the commit.
> You don’t normally have to revert or re-do anything.  When you
> have made the appropriate changes to your commit, you push them up
> to mozreview again, and it will create interdiffs.
> 
> As a patch author, all you need to care about is the number of
> commits and their order.

Thanks.
I will learn how to use hg tool.
And commit --amend later.
Thanks again!
Comment hidden (mozreview-request)
(Assignee)

Comment 29

2 years ago
Hi ato,
Now it seems I cannot commit --amend.
I use the hg update -C -r to reset to the marionette commit.
Then modify some parts then hg commit --amend. It shows the error log as "中止: cannot amend changeset with children"
I think there is a long way to be familiar with the hg tool.
I will re-push a new commit with the clean code.
THX.
(Assignee)

Updated

2 years ago
Attachment #8887311 - Attachment is obsolete: true
Attachment #8887311 - Flags: review?(hskupin)
Attachment #8887311 - Flags: review?(ato)
Comment hidden (mozreview-request)
(Assignee)

Comment 31

2 years ago
mozreview-review
Comment on attachment 8888695 [details]
Bug 1378121 - Add Minimize Window command to marionette;

https://reviewboard.mozilla.org/r/159734/#review165080
(Assignee)

Updated

2 years ago
Attachment #8888695 - Flags: review?(james)
(In reply to yangyi_peng (:Sl0v3C) from comment #29)
> Now it seems I cannot commit --amend.
> I use the hg update -C -r to reset to the marionette commit.

I use git, so you’ll have to translate that into the appropriate hg vernacular.
(Assignee)

Comment 33

2 years ago
(In reply to Andreas Tolfsen ‹:ato› from comment #32)
> (In reply to yangyi_peng (:Sl0v3C) from comment #29)
> > Now it seems I cannot commit --amend.
> > I use the hg update -C -r to reset to the marionette commit.
> 
> I use git, so you’ll have to translate that into the appropriate hg
> vernacular.

OK. Now I re-pushed a new commit: 
https://reviewboard.mozilla.org/r/159734/#review165080
Please help to review this.
THX!

Comment 34

2 years ago
mozreview-review
Comment on attachment 8888695 [details]
Bug 1378121 - Add Minimize Window command to marionette;

https://reviewboard.mozilla.org/r/159734/#review166202

I am not a good reviewer for marionette patches; ato will be back from vacation soon so we should wait for him to review this.
Attachment #8888695 - Flags: review?(james)

Comment 35

2 years ago
mozreview-review
Comment on attachment 8888695 [details]
Bug 1378121 - Add Minimize Window command to marionette;

https://reviewboard.mozilla.org/r/159734/#review167068

Sorry for the lack of response here, but I’ve been on holiday.

As far as I can tell there is only one tiny linting mistake that needs to be fixed before this can be integrated.  I’ve passed on the link to the try job and a suggestion on how to fix it below.

As you can tell from the try job (https://treeherder.mozilla.org/#/jobs?repo=try&revision=12277fd3f3d6), there are some failures in MnH (headless Firefox), but since these are Tier-2 it is probably fine to land this change and fix those later.

::: testing/marionette/driver.js:2909
(Diff revision 1)
> +  resp.body = {
> +    x: win.screenX,
> +    y: win.screenY,
> +    width: win.outerWidth,
> +    height: win.outerHeight,
> +    state: state,

As you can see from the eslint job from CI, this is expected to be a shorthand: https://treeherder.mozilla.org/logviewer.html#?job_id=118202982&repo=try&lineNumber=4581

This would fix it:

> resp.body = {
>   x: win.screenX,
>   y: win.screenY,
>   width: win.outerWidth,
>   height: win.outerHeight,
>   state,
> };

You can lint locally using `./mach lint testing/marionette/driver.js`.
Attachment #8888695 - Flags: review?(ato) → review-
bdahl, as you can tell from
https://treeherder.mozilla.org/#/jobs?repo=try&revision=12277fd3f3d6,
introducing a WebDriver:MinimizeWindow command to the remote
protocol causes failures in MnH.  Since MnH is still Tier-2, I
assume it is fine to integrate this patch as-is, but I wanted to
raise a flag so that you are aware it will turn MnH orange.
(In reply to Andreas Tolfsen ‹:ato› from comment #36)
> bdahl, as you can tell from
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=12277fd3f3d6,
> introducing a WebDriver:MinimizeWindow command to the remote
> protocol causes failures in MnH.  Since MnH is still Tier-2, I
> assume it is fine to integrate this patch as-is, but I wanted to
> raise a flag so that you are aware it will turn MnH orange.

Can we just disable this test[1] for now in headless mode so we don't turn it orange? I've created bug 1384763 to fix this.

[1] = http://searchfox.org/mozilla-central/rev/ad093e98f42338effe2e2513e26c3a311dd96422/testing/marionette/harness/marionette_harness/tests/unit/unit-tests.ini#76
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 41

2 years ago
mozreview-review
Comment on attachment 8888695 [details]
Bug 1378121 - Add Minimize Window command to marionette;

https://reviewboard.mozilla.org/r/159734/#review167270
Attachment #8888695 - Flags: review?(ato) → review-
Comment hidden (mozreview-request)

Comment 43

2 years ago
mozreview-review
Comment on attachment 8890810 [details]
Bug 1378121 - Add WebDriver:MinimizeWindow command to Marionette;

https://reviewboard.mozilla.org/r/162018/#review167272
Attachment #8890810 - Flags: review?(ato) → review+
(In reply to Brendan Dahl [:bdahl] from comment #37)

> (In reply to Andreas Tolfsen ‹:ato› from comment #36)
> 
> > bdahl, as you can tell from
> > https://treeherder.mozilla.org/#/jobs?repo=try&revision=12277fd3f3d6,
> > introducing a WebDriver:MinimizeWindow command to the
> > remote protocol causes failures in MnH.  Since MnH is still
> > Tier-2, I assume it is fine to integrate this patch as-is, but
> > I wanted to raise a flag so that you are aware it will turn MnH
> > orange.
> 
> Can we just disable this test[1] for now in headless mode so we
> don't turn it orange? I've created bug 1384763 to fix this.

Sure thing!  That’s a much better idea.  I have amended the patch.
Sl0v3C: I have uploaded a slightly amended version of your patch
which fixes a few minor issues I discovered in the last pass.

As I amended your commit, I noticed that you do not have your full
name and email set.  The next time you upload a patch, it would be
good if you set that correctly.  I hope I got your name right.

This patch is now pending on a successful try run
(https://treeherder.mozilla.org/#/jobs?repo=try&revision=d68335105470).
The next step after that is integration.

Thanks for your patch, and sorry about the somewhat length review process!

Comment 46

2 years ago
Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/47f783c81d0c
Add WebDriver:MinimizeWindow command to Marionette; r=ato

Comment 47

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/47f783c81d0c
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago2 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
(Assignee)

Comment 48

2 years ago
(In reply to Andreas Tolfsen ‹:ato› from comment #45)
> Sl0v3C: I have uploaded a slightly amended version of your patch
> which fixes a few minor issues I discovered in the last pass.
> 
> As I amended your commit, I noticed that you do not have your full
> name and email set.  The next time you upload a patch, it would be
> good if you set that correctly.  I hope I got your name right.
> 
> This patch is now pending on a successful try run
> (https://treeherder.mozilla.org/#/jobs?repo=try&revision=d68335105470).
> The next step after that is integration.
> 
> Thanks for your patch, and sorry about the somewhat length review process!

ato: THX for your great help! I will add the full name and email later.
(Assignee)

Comment 49

2 years ago
(In reply to Andreas Tolfsen ‹:ato› from comment #45)
> Sl0v3C: I have uploaded a slightly amended version of your patch
> which fixes a few minor issues I discovered in the last pass.
> 
> As I amended your commit, I noticed that you do not have your full
> name and email set.  The next time you upload a patch, it would be
> good if you set that correctly.  I hope I got your name right.
> 
> This patch is now pending on a successful try run
> (https://treeherder.mozilla.org/#/jobs?repo=try&revision=d68335105470).
> The next step after that is integration.
> 
> Thanks for your patch, and sorry about the somewhat length review process!

Hi ato,
Here is a geckodriver part commit need to review.
I locally modify the Cargo.toml webdriver 0.27.0 -> 0.28.0, then cargo build --release to generate the geckodriver.
And I verified the command can work with the geckodriver patch.
Please help to review it:
https://reviewboard.mozilla.org/r/158136/diff/2#index_header
THX very much.
You need to log in before you can comment on or make changes to this bug.