Skip to content
This repository has been archived by the owner on Jan 17, 2023. It is now read-only.

Shutdown the embedded webExtension when bootstrap is asked to shutdown #2712

Merged
merged 1 commit into from
Apr 19, 2017

Conversation

ianb
Copy link
Contributor

@ianb ianb commented Apr 19, 2017

@rhelmer
Copy link
Contributor

rhelmer commented Apr 19, 2017

lgtm, this is almost exactly what I tested locally.

@jaredhirsch
Copy link
Member

@ianb This looks good. Sadly, I think we need to change the "if reason == APP_STARTUP" bit from the startup method--otherwise I think the pref observer will stop working when the addon is upgraded. Would you mind setting a flag when the observer is registered, then checking that flag, instead of the reason constant, inside startup? We always want exactly one listener.

@ianb
Copy link
Contributor Author

ianb commented Apr 19, 2017

@6a68 I'm not sure I understand. if (reason === APP_STARTUP) only protects appStartupObserver.register() (which seems to unregister itself). But it doesn't stop the rest of startup() from being called, including the pref observer. Unless I miss what you are thinking about here.

@jaredhirsch
Copy link
Member

@ianb Ah yeah, you're right! Sorry about that, merging

@jaredhirsch jaredhirsch merged commit 9a23339 into master Apr 19, 2017
@jaredhirsch jaredhirsch deleted the shutdown-in-bootstrap branch April 19, 2017 21:30
ianb added a commit that referenced this pull request Apr 20, 2017
For CircleCI, start server before starting Selenium tests
Make sure the pref for system-disabled is True before installing (to workaround #2712), and False before running tests
Make channel configurable via $FIREFOX_CHANNEL
Allow the tester to keep the window open with $NO_CLOSE
Create a test that clicks the Screenshots button, skips onboarding, clicks Save Visible, and confirms a tab opens with a shot URL
Make driver instantiation, which includes installing the add-on, async and blocking on add-on installation
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants