-
Notifications
You must be signed in to change notification settings - Fork 905
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 7645: Enable remote debugging by default and proxy devtools request #5500
Conversation
d2e4c70
to
726329c
Compare
726329c
to
8d942fc
Compare
|
||
#include "chrome/browser/devtools/url_constants.h" | ||
|
||
const char kRemoteFrontendDomain[] = "devtools.brave.com"; |
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.
can we not use the normal static redirect? My concern is that we're changing urls in different places and that makes it harder to find how/where/what we're changing
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.
@bridiver - I was trying that first, but it requires a much bigger re-write of the code.
Currently, the brave-proxy code lives in brave_proxying_url_loader_factory.cc
which lives in //brave/browser/net
. To use static redirect we need to invoke MaybeProxyRequest
from devtools_instrumentation::WillCreateURLLoaderFactory
which is present in //content/browser
and we cannot add a deps for //brave/browser/net
to //content/browser
.
So for static redirect to work, we'll need to move the proxying code to //content/
or //net
.
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.
@jumde do you remember if we have an issue for moving proxying code to //net
? At least brave_system_request_handler.cc
. I guess this is not the first layering problem with 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.
@iefremov - Filed one: brave/brave-browser#9770 - Thanks for the reminder.
Changes LGTM - but when running locally on Windows, it starts working and then gets to the generating report part. Unfortunately, that seems to close dev tools? Not sure if something is not being proxied properly? Will try a signed executable tomorrow (from PR builder) to see if it makes a difference |
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.
Release version works as expected! 😄 Nice work here
As mentioned in the issue, I believe what I ran into is captured w/ https://bugs.chromium.org/p/chromium/issues/detail?id=1036525 |
8d942fc
to
1b975be
Compare
1b975be
to
4f9734a
Compare
Resolves brave/brave-browser#7645
Submitter Checklist:
npm run lint
)git rebase master
(if needed).git rebase -i
to squash commits (if needed).Test Plan:
Reviewer Checklist:
After-merge Checklist:
changes has landed on.