Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #6372 Makes WebTorrent extension non-persistent #3638

Merged
merged 2 commits into from
Oct 22, 2019

Conversation

AndriusA
Copy link

@AndriusA AndriusA commented Oct 9, 2019

Fix brave/brave-browser#6372

Turning extension background to persistent: false allows browser to unload it when not in use. To avoid launching the extension on every page activity/tab switch/window switch needed to switch to using webNavigation events that can be filtered by URL patterns.

Completely removed window event handling as no key functionality appears to depend on it.

Removed most of tab events, only leaving chrome.tabs.onRemoved to keep torrent cleanup for now.

Submitter Checklist:

Test Plan:

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions
  • Verify test plan is specified in PR before merging to source

After-merge Checklist:

  • The associated issue milestone is set to the smallest version that the
    changes has landed on.
  • All relevant documentation has been updated.

@AndriusA AndriusA requested review from feross and yrliou October 9, 2019 11:01
@AndriusA AndriusA force-pushed the webtorrent-non-persistent branch 2 times, most recently from 7bc3d1e to 491dc3c Compare October 9, 2019 11:24
Fix #6372

Turning extension background to `persistent: false` allows browser to unload it when not in use. To avoid launching the extension on every page activity/tab switch/window switch needed to switch to using `webNavigation` events that can be filtered by URL patterns.

Completely removed window event handling as no key functionality appears to depend on it.

Removed most of tab events, only leaving `chrome.tabs.onRemoved` to keep torrent cleanup for now.
@AndriusA AndriusA force-pushed the webtorrent-non-persistent branch from 491dc3c to a609a71 Compare October 9, 2019 13:58
@cezaraugusto cezaraugusto self-requested a review October 9, 2019 18:48
@NejcZdovc NejcZdovc added this to the 0.72.x - Nightly milestone Oct 11, 2019
@NejcZdovc NejcZdovc added the feature/webtorrent Label for webtorrent related issues label Oct 11, 2019
Copy link
Contributor

@cezaraugusto cezaraugusto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code LGTM. No errors logged in the extension while doing a manual test.

Manual tests included downloading a .torrent file and clicking a magnet link. Both worked as expected in both possible scenarios (webtorrent on/off).

@petemill
Copy link
Member

I would suggest this needs an issue with QA/yes where the test plan looks at the task manager for the webtorrent extension process to only show up when in use, and then quit itself again afterwards.

Copy link
Member

@yrliou yrliou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, also tested on local and it worked as expected.

@yrliou yrliou added the CI/skip Do not run CI builds (except noplatform) label Oct 22, 2019
@yrliou
Copy link
Member

yrliou commented Oct 22, 2019

CI is green already, labeled CI/skip for the incoming console log removal.

@yrliou yrliou merged commit 6670537 into master Oct 22, 2019
@yrliou yrliou deleted the webtorrent-non-persistent branch October 22, 2019 18:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI/skip Do not run CI builds (except noplatform) feature/webtorrent Label for webtorrent related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

WebTorrent extension should be non-persistent
5 participants