Marionette has to use "eForceQuit" for in_app shutdown and restart requests

RESOLVED FIXED in Firefox 61

Status

P3
normal
RESOLVED FIXED
a year ago
11 months ago

People

(Reporter: whimboo, Assigned: AlvaroRe, Mentored)

Tracking

(Blocks: 2 bugs, {good-first-bug})

Version 3
mozilla61
good-first-bug
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 fixed)

Details

(Whiteboard: [lang=py])

User Story

To get started please follow the steps as listed here:

https://firefox-source-docs.mozilla.org/testing/marionette/marionette/NewContributors.html

In case of questions don't hesitate to ask, here or in irc.mozilla.org/#ateam.

Attachments

(1 attachment, 1 obsolete attachment)

Currently Marionette uses `eAttemptQuit` for an in_app shutdown request:

https://dxr.mozilla.org/mozilla-central/rev/3f474e48db7f7b0f1c2c090f812290a8d9a21650/testing/marionette/client/marionette_driver/marionette.py#1040

This should be changed to `eForceQuit`, which then will force all the windows to close.

Beside the code change also the Python docstring of the method needs to be updated.

After changing it make sure that all Marionette tests are still passing.
Whiteboard: [lang=py]
(Assignee)

Comment 1

a year ago
Hi, i'd like to solve this.
Alvaro, you are welcome to work on this bug. I will assign it to you once a patch got uploaded. In case of questions don't hesitate to ask.
(Assignee)

Comment 3

a year ago
Here's is the patch: https://reviewboard.mozilla.org/r/222952 
Hope I did this right.
Alvaro, thank you for the patch! I have seen you uploaded it already to mozreview. Can I assume that it is ready for review? If yes, please open the reviewboard link, and set `whimboo` as reviewer for the patch.
Flags: needinfo?(arb952)
Assignee: nobody → arb952
Status: NEW → ASSIGNED
(Assignee)

Updated

a year ago
Attachment #8953712 - Flags: review?(hskupin)
Priority: -- → P3
(Reporter)

Comment 6

a year ago
mozreview-review
Comment on attachment 8953712 [details]
Bug 1440577 - Marionette has to use "eForceQuit" for in_app shutdown and restart requests

https://reviewboard.mozilla.org/r/222952/#review229182

Thank you for the patch. Code-wise it looks fine, but please update the docstring as mentioned inline. In the meantime I will trigger a try build so we can see if there are any regressions as caused by this change.

::: testing/marionette/client/marionette_driver/marionette.py:1038
(Diff revision 1)
>          # removed from here in Firefox 55 at the earliest.
>  
>          # remove duplicates
>          flags = set(shutdown_flags)
>  
> -        # add eAttemptQuit if there are no *Quits
> +        # add eForceQuit if there are no *Quits

The docstring I was referring to is above and related to the method itself. Check for the `shutdown_flags` parameter.
Attachment #8953712 - Flags: review?(hskupin) → review-
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8966643 - Flags: review?(hskupin)
Attachment #8953712 - Flags: review?(hskupin)
(Reporter)

Comment 9

11 months ago
mozreview-review
Comment on attachment 8966643 [details]
Bug 1440577 - Changed 'eAttemptQuit' occurrences in docstring to 'eForceQuit'

https://reviewboard.mozilla.org/r/235356/#review241276

::: testing/marionette/client/marionette_driver/marionette.py:1014
(Diff revision 1)
>      def _request_in_app_shutdown(self, *shutdown_flags):
>          """Attempt to quit the currently running instance from inside the
>          application.
>  
>          Duplicate entries in `shutdown_flags` are removed, and
> -        `"eAttemptQuit"` is added if no other `*Quit` flags are given.
> +        `"eForceQuit"` is added if no other `*Quit` flags are given.

Please squash those changes into the other commit. You can use `hg histedit` in combination with `r` for the second commit. Also make sure that the commit message has a proper message. It's not necessary to call out the docstring changes.
Attachment #8966643 - Flags: review?(hskupin)
(Reporter)

Comment 10

11 months ago
mozreview-review
Comment on attachment 8953712 [details]
Bug 1440577 - Marionette has to use "eForceQuit" for in_app shutdown and restart requests

https://reviewboard.mozilla.org/r/222952/#review241278
Attachment #8953712 - Flags: review+
Alvaro, would you mind finishing this patch? It would be great to have it in tree.
Comment hidden (mozreview-request)
(Assignee)

Updated

11 months ago
Attachment #8966643 - Attachment is obsolete: true
Sorry for being late here. I somehow missed your last update on that bug and to check the results from try. It all looks fine now and I'm going to push your patch. Thanks a lot!
Flags: needinfo?(arb952)

Comment 14

11 months ago
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d2e9671be0df
Marionette has to use "eForceQuit" for in_app shutdown and restart requests r=whimboo

Comment 15

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d2e9671be0df
Status: ASSIGNED → RESOLVED
Last Resolved: 11 months ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Alvaro, thank you for working on this bug! In case you are interested to contribute more please let me know. As best via email, and we can figure out a good next bug for you.
You need to log in before you can comment on or make changes to this bug.