Closed
Bug 1378121
Opened 7 years ago
Closed 7 years ago
Add Minimize Window command to Marionette
Categories
(Remote Protocol :: Marionette, defect, P1)
Remote Protocol
Marionette
Tracking
(firefox56 fixed)
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: Sl0v3C, Assigned: Sl0v3C)
References
(Blocks 1 open bug, )
Details
(Keywords: pi-marionette-server)
Attachments
(4 files, 2 obsolete files)
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•7 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.
Comment 2•7 years ago
|
||
Please report the issue to the Selenium developer.
Status: UNCONFIRMED → RESOLVED
Closed: 7 years ago
Resolution: --- → INVALID
Comment 3•7 years ago
|
||
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
Updated•7 years ago
|
Blocks: webdriver
Status: REOPENED → NEW
Summary: When using selenium, there is no minimize command → Add support for `minimize` command
Updated•7 years ago
|
Attachment #8883291 -
Attachment mime type: text/x-patch → text/plain
Comment 4•7 years ago
|
||
@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 6•7 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!
Comment 7•7 years ago
|
||
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.
Comment 8•7 years ago
|
||
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 9•7 years ago
|
||
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•7 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•7 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) |
Comment 14•7 years ago
|
||
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•7 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)
Comment 16•7 years ago
|
||
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•7 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•7 years ago
|
Attachment #8886493 -
Attachment is obsolete: true
Attachment #8886493 -
Flags: review?(james)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 19•7 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•7 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-
Comment 21•7 years ago
|
||
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.
Updated•7 years ago
|
Summary: Add support for `minimize` command → Add Minimize Window command to Marionette
Updated•7 years ago
|
Comment hidden (mozreview-request) |
Assignee | ||
Comment 23•7 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•7 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•7 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-
Comment 26•7 years ago
|
||
(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•7 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•7 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•7 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•7 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•7 years ago
|
Attachment #8888695 -
Flags: review?(james)
Comment 32•7 years ago
|
||
(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•7 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•7 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•7 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-
Comment 36•7 years ago
|
||
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.
Comment 37•7 years ago
|
||
(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•7 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•7 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+
Comment 44•7 years ago
|
||
(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.
Comment 45•7 years ago
|
||
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•7 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•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/47f783c81d0c
Status: ASSIGNED → RESOLVED
Closed: 7 years ago → 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Assignee | ||
Comment 48•7 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•7 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.
Updated•1 year ago
|
Product: Testing → Remote Protocol
You need to log in
before you can comment on or make changes to this bug.
Description
•