-
Notifications
You must be signed in to change notification settings - Fork 879
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
Backend Work for Crypto Wallets reset feature #4364
Conversation
0a75e5c
to
53c6327
Compare
781c5fc
to
6f397b0
Compare
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.
1 minor nit, and 1 question to make sure it's right behavior. Nice work.
GURL url = web_contents->GetURL(); | ||
if (url.SchemeIs(content::kChromeUIScheme) && | ||
url.host() == ethereum_remote_client_host) { | ||
web_contents->Close(); |
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.
Does this actually get rid of the tab_strip's webcontents too? if so this might cause some wonkiness with the iterator on the count.
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'll do some tests and double check, I'll report back 👍
6f397b0
to
da3a537
Compare
Adding service and controller for brave_wallet
da3a537
to
6e7411e
Compare
#include "components/keyed_service/core/keyed_service.h" | ||
|
||
namespace base { | ||
class SequencedTaskRunner; |
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.
why is this here? It isn't referenced anywhere in the header
BraveWalletService::BraveWalletService(content::BrowserContext* context) | ||
: context_(context), | ||
controller_(new BraveWalletController(context)), | ||
weak_factory_(this) { |
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.
why does this have a weak ptr? It is never used for anything
} | ||
|
||
// static | ||
BraveWalletService* BraveWalletServiceFactory::GetForProfile(Profile* profile) { |
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.
this factory needs to go in brave/browser/brave_wallet
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 moved that already in a diff PR a few weeks ago
Has the fix been released into Brave stable yet? Currently |
This PR closes brave/brave-browser#5741 however there is no hint how to actually remove/initialize Brave Wallet
|
Partially fixes: brave/brave-browser#5741
Fixes: brave/brave-browser#7716
After the deletion of the DB entry, two things have to happen:
brave://wallet
need to closeSubmitter Checklist:
npm run lint
)git rebase master
(if needed).git rebase -i
to squash commits (if needed).Test Plan:
Note: The Extension at this time lacks the
"management"
permissions allowing programmatic reload, so the above snippet will not work. However, these steps can be used to verify functionality:brave://wallet
brave://wallet
, confirm state is freshReviewer Checklist:
After-merge Checklist:
changes has landed on.