1279243 - Add quit_in_app() to marionette driver.

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect, P3)

Description

[André Reinald]

Those methods are needed by session-test in order to examine and/or change the profile between Firefox restarts.

Comment 1

[André Reinald]

Attachment: Bug 1279243 - Add quit_in_app() to marionette driver;

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

Comment 2

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8761612 [details]
Bug 1279243 - Add quit_in_app() to marionette driver;

https://reviewboard.mozilla.org/r/58728/#review55666

::: testing/marionette/client/marionette_driver/marionette.py:1008
(Diff revision 1)
>              self.instance.restart(prefs)
>              self.raise_for_port(self.wait_for_port())
>              self.start_session()
>              self._reset_timeouts()
>  
> +    def quit_instance(self):

I think it would be more accurate to call this method `disconnect_instance`, since the gecko instance is not being stopped.

::: testing/marionette/client/marionette_driver/marionette.py:1013
(Diff revision 1)
> +    def quit_instance(self):
> +        """
> +        This will terminate the currently running instance.
> +        """
> +        if not self.instance:
> +            raise errors.MarionetteException("restart can only be called "

Update doc string to replace `restart`

::: testing/marionette/client/marionette_driver/marionette.py:1028
(Diff revision 1)
> +        # The instance is restarting itself; we will no longer be able to
> +        # track it by pid, so mark it as 'detached'.
> +        self.instance.detached = True
> +        self.raise_for_port(self.wait_for_port())
> +
> +    def restart_instance(self):

I'm not sure why you need this in a separate method. Why not just call `start_session` and `_reset_timeouts` directly whereever you need to?

In any case, based on my understanding, this should be called something like `reset_session`, with the docstring updated accordingly. It's not doing anything to an instance.

::: testing/marionette/client/marionette_driver/marionette.py:1058
(Diff revision 1)
> -            # track it by pid, so mark it as 'detached'.
> -            self.instance.detached = True
>          else:
>              self.delete_session()
>              self.instance.restart(clean=clean)
> -        self.raise_for_port(self.wait_for_port())
> +            self.raise_for_port(self.wait_for_port())

Why is this indented into the else block now? Did you mean to just move it into quit_instance()?

Please also edit your commit message to indicate that this is a refactor and explain why the changes are needed.

Comment 3

[André Reinald]

Comment on attachment 8761612 [details]
Bug 1279243 - Add quit_in_app() to marionette driver;

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

Comment 4

[André Reinald]

https://reviewboard.mozilla.org/r/58728/#review55666

> I think it would be more accurate to call this method `disconnect_instance`, since the gecko instance is not being stopped.

The current restart() method:
1. comments "This will terminate the currently running instance"
2. calls self._send_message("quitApplication", {"flags": restart_flags})

This why I assume the instance is stopped. Am I wrong?

> Update doc string to replace `restart`

Done.

> I'm not sure why you need this in a separate method. Why not just call `start_session` and `_reset_timeouts` directly whereever you need to?
> 
> In any case, based on my understanding, this should be called something like `reset_session`, with the docstring updated accordingly. It's not doing anything to an instance.

Because:
1. I assume _reset_timeouts() is meant to be private.
2. even those 2 lines form a logical sequence which can be factored as they will be called from both restart() and my own code to come.

Again, the comment in restart() states "... and spawn a new instance with the same profile" which is what I want. Is the comment misleading?

> Why is this indented into the else block now? Did you mean to just move it into quit_instance()?

Because self.raise_for_port(self.wait_for_port()) is called from my new quit_instance() method called in the "then block", so now it should only be called in the "else block".

Done.

Comment 5

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

https://reviewboard.mozilla.org/r/58728/#review55666

> The current restart() method:
> 1. comments "This will terminate the currently running instance"
> 2. calls self._send_message("quitApplication", {"flags": restart_flags})
> 
> This why I assume the instance is stopped. Am I wrong?

My mistake, it is being stopped, but the name and docstring still don't reflect the behaviour. The instance is restarted (see `restart_flags`) from within the browser, so a name like `restart_in_app` is more accurate.

> Because:
> 1. I assume _reset_timeouts() is meant to be private.
> 2. even those 2 lines form a logical sequence which can be factored as they will be called from both restart() and my own code to come.
> 
> Again, the comment in restart() states "... and spawn a new instance with the same profile" which is what I want. Is the comment misleading?

I think it's better to just use `_reset_timeouts()` directly -- go ahead and rename it to `reset_timeouts()`; it's ok if it's non-private.

(Either way, yes, the name `restart_instance` and its docstring are misleading because the instance is not restarted by this new method. The comment in `restart()` about spawing a new instance refers to `self.instance.restart(...)` and `self._send_message("quit...")`)

Comment 6

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8761612 [details]
Bug 1279243 - Add quit_in_app() to marionette driver;

https://reviewboard.mozilla.org/r/58728/#review57846

In the future, I suggest submitting changes like this as part of a patch series for the relevant session-test work so that we can understand/discuss the context of the refactoring.

Comment 7

[André Reinald]

https://reviewboard.mozilla.org/r/58728/#review55666

> My mistake, it is being stopped, but the name and docstring still don't reflect the behaviour. The instance is restarted (see `restart_flags`) from within the browser, so a name like `restart_in_app` is more accurate.

What does "restarted from within the browser" mean? Isn't it like the user manually quitting the browser from the menu? Just wanted to make sure as this is what I need.

> I think it's better to just use `_reset_timeouts()` directly -- go ahead and rename it to `reset_timeouts()`; it's ok if it's non-private.
> 
> (Either way, yes, the name `restart_instance` and its docstring are misleading because the instance is not restarted by this new method. The comment in `restart()` about spawing a new instance refers to `self.instance.restart(...)` and `self._send_message("quit...")`)

There is another reason to add the "restart_instance" method: I need to restart the session with self.session_id. Should I also use this field outside of the object?

Comment 8

[André Reinald]

https://reviewboard.mozilla.org/r/58728/#review57846

I actually have a bunch of interdependent bugs for this session-test work, each having one or more patches. How does the "patch series" work in this configuration?

Comment 9

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

https://reviewboard.mozilla.org/r/58728/#review57846

Here's an example of a patch series where some small changes in mozrunner are grouped under a bug for Marionette: https://reviewboard.mozilla.org/r/61358/ Whether patches go in the same bug or not is fluid: I try to think of it in terms of what would make sense to land independently and what can be understood by a reviewer without extra context.
In that example, when adding something substantial to mozrunner was needed as a dependency, that work went under a separate bug: https://reviewboard.mozilla.org/r/58256/

Comment 10

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

https://reviewboard.mozilla.org/r/58728/#review55666

> What does "restarted from within the browser" mean? Isn't it like the user manually quitting the browser from the menu? Just wanted to make sure as this is what I need.

Yes, when user quits manually, `nsiAppStartup.quit()` is called (but without restart flags). In general, "from within the browser" means it's restarted via a chrome-level JavaScript call (See https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIAppStartup#quit() and https://dxr.mozilla.org/mozilla-central/rev/63cc31d6cc1c8089590461016ce0b4a2fb77ecbc/testing/marionette/driver.js#2576-2591) instead of being restarted externally by mozrunner's ProcessHandler.

> There is another reason to add the "restart_instance" method: I need to restart the session with self.session_id. Should I also use this field outside of the object?

I'm ok with using self.session_id outside of the object.

Comment 11

[André Reinald]

Comment on attachment 8761612 [details]
Bug 1279243 - Add quit_in_app() to marionette driver;

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

Comment 12

[André Reinald]

Comment on attachment 8761612 [details]
Bug 1279243 - Add quit_in_app() to marionette driver;

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

Comment 13

[André Reinald]

https://reviewboard.mozilla.org/r/58728/#review55666

> Yes, when user quits manually, `nsiAppStartup.quit()` is called (but without restart flags). In general, "from within the browser" means it's restarted via a chrome-level JavaScript call (See https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIAppStartup#quit() and https://dxr.mozilla.org/mozilla-central/rev/63cc31d6cc1c8089590461016ce0b4a2fb77ecbc/testing/marionette/driver.js#2576-2591) instead of being restarted externally by mozrunner's ProcessHandler.

Removed restart_instance() method, and will make the 2 calls from outside the class.
Renamed quit_instance() to quit_in_app().

> I'm ok with using self.session_id outside of the object.

Ok, then the method restart_instance() is supressed and the bug title was changed.

Comment 14

[André Reinald]

https://reviewboard.mozilla.org/r/58728/#review57846

Right, that will make sense for future patches which belong to meta-bug 1247589. I'll ask you how to achieve that when time comes.

Comment 15

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8761612 [details]
Bug 1279243 - Add quit_in_app() to marionette driver;

https://reviewboard.mozilla.org/r/58728/#review60414

Comment 16

[Carsten Book [:Tomcat]]

has problems to apply:

Hunk #3 FAILED at 1039
1 out of 3 hunks FAILED -- saving rejects to file testing/marionette/client/marionette_driver/marionette.py.rej
patch failed to apply
abort: fix up the working directory and run hg transplant --continue

Comment 17

[André Reinald]

Comment on attachment 8761612 [details]
Bug 1279243 - Add quit_in_app() to marionette driver;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58728/diff/4-5/

Comment 18

[André Reinald]

Rebased latest patch to central.
Testing before asking for checkin again.

Comment 19

[André Reinald]

Comment on attachment 8761612 [details]
Bug 1279243 - Add quit_in_app() to marionette driver;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58728/diff/5-6/

Comment 20

[André Reinald]

Comment on attachment 8761612 [details]
Bug 1279243 - Add quit_in_app() to marionette driver;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58728/diff/6-7/

Comment 21

[André Reinald]

Rebased on feaaf1af1065.
Works as expected.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2bb2126f4034
Add quit_in_app() to marionette driver. r=maja_zf

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2bb2126f4034

Comment 24

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Hm, I wasn't aware of this bug and had a look at the code changes now. Some parts feel wrong to me. I also miss a unit test for the new feature. So I'm going ahead and file a new bug for getting this more streamlined.