-
Notifications
You must be signed in to change notification settings - Fork 975
Conversation
5e11a47
to
709fafd
Compare
Could you add some sample torrent and magnet links that would demonstrate the added things nicely? Thanks! |
@bbondy sure, @feross collected some here: https://codepen.io/ferossity/full/qaezaB/ Right now you have to paste magnet links into the URL bar. Clicking a magnet link still tries to open an external app. I'll try to fix that later today. |
// * Move WebTorrent to its own renderer process, similar to the way it's done in | ||
// WebTorrent Desktop | ||
// * Remove this CSP exception: | ||
cspDirectives['connect-src'] = '*' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bbondy do these CSP rules apply only to the braveExtension, or to all extensions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dcposch only to the braveExtension, iirc. for security reasons, i would prefer that WebTorrent be its own "extension" similar to how we handle PDF reader (PDF.JS)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done!
@@ -0,0 +1,19 @@ | |||
(function () { | |||
var queryString = window.location.search |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like queryString is not used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will remove
module.exports.getSourceMagnetUrl = function (input) { | ||
const url = module.exports.getAppUrl('webtorrent.html') | ||
if (!input.startsWith(url)) return null | ||
return input.substring(input.indexOf('#') + 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't this check that the return value starts with 'magnet:?'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The target URL is internal-only and controlled by us, so I think that we can assume it will always be a magnet link or torrent file. This seems to be the assumption that is made by the analogous getSourceAboutUrl
function. Correct me if I'm wrong?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that sounds right, assuming you won't define page-anchor URLs like chrome-extension://.../webtorrent.html#about
or something.
how should this work for users who already have a |
It's also worth noting that as currently architected in this PR, WebTorrent can only connect to WebRTC peers, and not normal TCP/uTP ones. This is because we're running in a |
@diracdeltas As currently implemented, we ignore any protocol handlers registered by webpages through Does Brave expose an interface for configuring protocol handlers yet? Actually, here's an idea. What if we just add a new link to the Torrent Viewer page that says "Open in " for users who have a "magnet" protocol handler registered. Here's the current mock @dcposch is working on: So imagine another link added to that list. @diracdeltas Would that satisfy your use case? |
👍 |
New screenshotsThe file list now consists of regular hyperlinks. You can link to individual files within a torrent now, using the Right now, following one of those links is slow, because the new page has to connect to the swarm from scratch. That will be fixed once we move WebTorrent to its own process. |
a501321
to
f9d8d41
Compare
Almost ready for review! Work left to do
|
Also:
|
e896f82
to
349d13b
Compare
@@ -877,10 +887,27 @@ class Frame extends ImmutableComponent { | |||
method = (currentDetail, originalDetail) => | |||
windowActions.setAutofillCreditCardDetail(currentDetail, originalDetail) | |||
break | |||
case messages.TORRENT_MESSAGE: | |||
// Relay torrent IPC from the webview to a browser process |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this relaying is necessary; the webview can just call ipc.send
instead of ipc.sendToHost
to send a message to the browser process. See messages.CERT_ERROR_ACCEPTED
for instance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm. that doesn't seem to work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nvm, figured it out!
I think we're readyPlease try it out & let me know if there's anything that needs improvement! Code review also appreciated. Rebased on the latest master earlier today: 5e36e37, a few commits after Brave 0.12.10 @bbondy @diracdeltas @bradleyrichter @feross @BrendanEich In this PR
Not in this PRHere are some additional things you could do, probably after Brave 1.0
Latest screenshotsRegular webpage with magnet linksNavigate https://codepen.io/ferossity/full/qaezaB/. Click one of the links. Magnet pageBrave doesn't connect to any peers or to the DHT until you give it permission. A short legal warning lets users know they're about to join a P2P network. Click start. File listRight click on one of the named links, then click Open in New Tab. Magnet page for an individual fileThis is a magnet link with the same infohash as the one we were just on, but with an Since that torrent is already active, there's not disclaimer and no start button, the media starts streaming immediately. WebTorrent downloads pieces from peers in the order that we need them so we can start playing right away. Download linksBack in the first tab, click Download. This creates a normal download. Under the hood, it uses the same streaming torrent-to-HTTP local server as the You can also click Download Torrent File to save a BookmarksYou can bookmark a whole torrent or an individual file. PreferencesYou can disable the Torrent Viewer. In that case, magnet files will prompt the user to open an external app. LMK if this works for you! |
Rebased to get rid of the recent conflict and merged to master |
See #3256
This is still pretty raw. It demonstrates torrent support in Brave.
Visit a magnet URL
Bookmark a magnet link
Play files in a torrent
Here are some of the things we still need to do:
Feedback appreciated!
@BrendanEich @diracdeltas @ayumi @bbondy