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

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, enhancement, P3)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

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.

Comment 1

[Alvaro Reina (:AlvaroRe)]

Hi, i'd like to solve this.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

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.

Comment 3

[Alvaro Reina (:AlvaroRe)]

Here's is the patch: https://reviewboard.mozilla.org/r/222952 
Hope I did this right.

Comment 4

[Alvaro Reina (:AlvaroRe)]

Attachment: Bug 1440577 - Marionette has to use "eForceQuit" for in_app shutdown and restart requests

Review commit: https://reviewboard.mozilla.org/r/222952/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/222952/

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

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.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

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.

Comment 7

[Alvaro Reina (:AlvaroRe)]

Attachment: Bug 1440577 - Changed 'eAttemptQuit' occurrences in docstring to 'eForceQuit'

Review commit: https://reviewboard.mozilla.org/r/235356/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/235356/

Comment 8

[Alvaro Reina (:AlvaroRe)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/222952/diff/1-2/

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

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.

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

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

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Alvaro, would you mind finishing this patch? It would be great to have it in tree.

Comment 12

[Alvaro Reina (:AlvaroRe)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/222952/diff/2-3/

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

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!

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d2e9671be0df
Marionette has to use "eForceQuit" for in_app shutdown and restart requests r=whimboo

Comment 15

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/d2e9671be0df

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

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.