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

Add WebTorrent component extension when session restoring webtorrent tabs #6085

Merged
merged 1 commit into from
Jul 16, 2020
Merged

Add WebTorrent component extension when session restoring webtorrent tabs #6085

merged 1 commit into from
Jul 16, 2020

Conversation

yrliou
Copy link
Member

@yrliou yrliou commented Jul 15, 2020 •
edited
Loading

Resolves brave/brave-browser#7330

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.

@yrliou yrliou added this to the 1.13.x - Nightly milestone Jul 15, 2020
@yrliou yrliou self-assigned this Jul 15, 2020
@yrliou yrliou marked this pull request as ready for review July 15, 2020 19:59
@yrliou yrliou requested a review from bridiver as a code owner July 15, 2020 19:59
@yrliou yrliou force-pushed the webtorrent_session_restore branch from 1841490 to e9df185 Compare July 15, 2020 20:00
@yrliou yrliou requested review from mkarolin and bbondy July 15, 2020 20:00
bridiver
bridiver reviewed Jul 15, 2020
View reviewed changes
patches/chrome-browser-ui-BUILD.gn.patch Outdated
# Use of this source code is governed by a BSD-style license that can be
# found in the LICENSE file.

+import("//brave/browser/ui/ui.gni")
Copy link
Collaborator

Choose a reason for hiding this comment

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

you don't need to import anything for this, just put it in build/config/brave_build.gni

Copy link
Member Author

Choose a reason for hiding this comment

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

There was an import loop in this case.
As discussed on slack, add a target in chromium/chrome/browser/ui with required deps and let brave/browser/ui depends on that target in
fc0d608

bridiver
bridiver reviewed Jul 15, 2020
View reviewed changes
browser/ui/ui.gni Outdated
]

if (enable_brave_webtorrent) {
brave_browser_ui_deps += [
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe a comment here that it's for the chromium_src files?

bridiver
bridiver reviewed Jul 15, 2020
View reviewed changes
chromium_src/chrome/browser/ui/browser_tabrestore.cc
namespace {

#if BUILDFLAG(ENABLE_BRAVE_WEBTORRENT)
void MaybeLoadWebtorrent(Browser* browser,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we want to duplicate the code from BraveWebTorrentNavigationThrottle, maybe you can create a static method on BraveWebTorrentNavigationThrottle or something?

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 in fc0d608

bridiver
bridiver reviewed Jul 16, 2020
View reviewed changes
components/brave_webtorrent/browser/webtorrent_util.cc Outdated
@@ -57,6 +58,17 @@ void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry) {
registry->RegisterBooleanPref(kWebTorrentEnabled, true);
}

bool IsWebtorrentPage(const GURL& url) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit IsWebtorrentURL

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 in 2a601f7

@yrliou
Copy link
Member Author

yrliou commented Jul 16, 2020

CI has unrelated failure:
unit test on android:

BatAdsAdConversionsDatabaseTableTest.GetAdConversionsFromCatalogEndpoint
BatAdsCreativeAdNotificationsDatabaseTableTest.GetCreativeAdNotificationsFromCatalogEndpoint

dist on Windows:

Error information: "Error: SignerSign() failed." (-1073700864/0xc000a000)
01:24:06  SignTool Error: An unexpected internal error has occurred.

@yrliou yrliou force-pushed the webtorrent_session_restore branch from 2a601f7 to f69a0c2 Compare July 16, 2020 16:24
@yrliou
Copy link
Member Author

yrliou commented Jul 16, 2020

Latest force push is just for squashing into one commit, will cancel CI builds. Merging.

@yrliou yrliou merged commit 8c4b43d into master Jul 16, 2020
@yrliou yrliou deleted the webtorrent_session_restore branch July 16, 2020 16:26
@feross
Copy link
Contributor

feross commented Jul 17, 2020

Thank you for implementing this @yrliou. You're the best!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bridiver bridiver bridiver approved these changes

@mkarolin mkarolin Awaiting requested review from mkarolin

@bbondy bbondy Awaiting requested review from bbondy

Assignees

@yrliou yrliou

Labels
None yet
Projects
None yet
Milestone
1.13.x - Release #1
Development

Successfully merging this pull request may close these issues.

[Desktop] torrent and magnet link pages show blocked by extension message after browser restart
3 participants
@yrliou @feross @bridiver