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.

Sign up for GitHub

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

Jump to bottom

[WIP] WebTorrent Support #353

Closed
wants to merge 9 commits into from
Closed

[WIP] WebTorrent Support #353

wants to merge 9 commits into from

Conversation

yrliou
Copy link
Member

@yrliou yrliou commented Aug 22, 2018 •
edited
Loading

TODO:

  • Automation test
  • Add webtorrent and bittorrent-tracker forks into vendor and use them instead
  • Investigate DCHECK error caused by sockets.tcp.connect

What's included in this PR:

  • WebTorrent support of magnet links

What's not included in this PR:

  • WebTorrent support of .torrent links
  • Support of trackless torrents using bittorrent-dht/client
  • l10n

Submitter Checklist:

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Ran git rebase -i to squash commits (if needed).
  • Tagged reviewers and labelled the pull request as needed.
  • Request a security/privacy review as needed.
  • Add appropriate QA labels (QA/Needed or QA/No-QA-Needed) to include the closed issue in milestone

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

@yrliou yrliou self-assigned this Aug 22, 2018
yrliou
yrliou commented Aug 22, 2018
View reviewed changes
patches/chrome-browser-extensions-component_extensions_whitelist-whitelist.cc.patch Outdated
bool IsComponentExtensionWhitelisted(int manifest_resource_id) {
switch (manifest_resource_id) {
// Please keep the list in alphabetical order.
+ case IDR_BRAVE_EXTENSON:
+ case IDR_BRAVE_WEBTORRENT:
Copy link
Member Author

Choose a reason for hiding this comment

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

I'll adapt @bridiver's work in sync WIP on moving it to chromium_src

@yrliou yrliou requested a review from bbondy August 22, 2018 19:02
bridiver
bridiver reviewed Aug 22, 2018
View reviewed changes
patches/net-url_request-url_request_context_builder.cc.patch
}
#endif // !BUILDFLAG(DISABLE_FTP_SUPPORT)

+ job_factory->SetProtocolHandler("magnet", std::make_unique<MagnetProtocolHandler>());
Copy link
Collaborator

Choose a reason for hiding this comment

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

this should not be added here. See ProfileIOData::AddProtocolHandlersToBuilder and ProfileIOData::InstallProtocolHandlers

bridiver
bridiver reviewed Aug 22, 2018
View reviewed changes
script/build-brave-webtorrent.py Outdated
@@ -0,0 +1,51 @@
import argparse
Copy link
Collaborator

Choose a reason for hiding this comment

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

we already have a transpile_web_ui action and it should be adapted if any changes are needed to handle this

Copy link
Member Author

Choose a reason for hiding this comment

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

addressed by 32c0fe2

bridiver
bridiver reviewed Aug 22, 2018
View reviewed changes
.gitignore Outdated
@@ -2,6 +2,7 @@
.tags*
/.idea/
/browser/resources/brave_extension/
/browser/resources/brave_webtorrent/*.bundle.js
Copy link
Collaborator

Choose a reason for hiding this comment

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

we shouldn't be writing back into the source directory. Please see the wip sync branch

Copy link
Member Author

Choose a reason for hiding this comment

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

It seems we couldn't access these *.bundle.js files from the extension if it's in gen, so I'm still generating webpack output files into src, they're now under components/brave_webtorrent/entension/out.

bridiver
bridiver reviewed Aug 22, 2018
View reviewed changes
browser/resources/brave_webtorrent/BUILD.gn Outdated
@@ -0,0 +1,22 @@
import("//tools/grit/grit_rule.gni")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this should all move into components/webtorrent and resources should be kept inside the component like sync

Copy link
Member Author

Choose a reason for hiding this comment

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

addressed by 32c0fe2

bridiver
bridiver reviewed Aug 22, 2018
View reviewed changes
browser/resources/brave_extension.grd Outdated
@@ -20,6 +20,11 @@
<include name="IDR_BRAVE_EXTENSON_EN_US_MESSAGES_JSON" file="brave_extension/_locales/en_US/messages.json" type="BINDATA" />
<include name="IDR_BRAVE_EXTENSON_BRAVE_SHIELDS_HTML" file="brave_extension/braveShieldsPanel.html" type="BINDATA" />
<include name="IDR_BRAVE_EXTENSON_BRAVELIZER_JS" file="brave_extension/bravelizer.css" type="BINDATA" />

<!-- Brave Webtorrent -->
Copy link
Collaborator

@bridiver bridiver Aug 22, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

see 88818ab - it handles both local and generated resources

Copy link
Member Author

Choose a reason for hiding this comment

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

addressed by 32c0fe2

@yrliou yrliou force-pushed the webtorrent_ext branch 2 times, most recently from 71c0c62 to 1940802 Compare August 23, 2018 06:14
@yrliou yrliou closed this Aug 23, 2018
@yrliou yrliou deleted the webtorrent_ext branch August 23, 2018 20:33
@yrliou
Copy link
Member Author

yrliou commented Aug 23, 2018

Oops... sorry, I accidentally delete this remote branch, I'll open a new PR.

@yrliou yrliou mentioned this pull request Aug 23, 2018
Merged
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bridiver bridiver bridiver left review comments

@bbondy bbondy Awaiting requested review from bbondy

Assignees

@yrliou yrliou

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@yrliou @bridiver