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
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants