-
Notifications
You must be signed in to change notification settings - Fork 868
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
Add Brave WebUI about:payments and brave-extension loading #15
Conversation
7756c23
to
b6dce28
Compare
std::string ComponentLoader::Add(int manifest_resource_id, | ||
const base::FilePath& root_directory) { | ||
if (!ignore_whitelist_for_testing_ && | ||
- !IsComponentExtensionWhitelisted(manifest_resource_id)) |
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.
wouldn't it be easier to patch IsComponentExtensionWhitelisted
instead of changing all the calls to it? If new calls to IsComponentExtensionWhitelisted
show up in the code they might also get missed
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.
OK that sounds good.
common/resource_bundle_helper.cc
Outdated
return base::mac::PathForFrameworkBundleResource( | ||
CFSTR("brave_resources.pak")); | ||
#else | ||
base::FilePath extensions_shell_and_test_pak_path; |
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.
extensions_shell_and_test_pak_path?
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.
Fixed, the function was copied / modified from somewhere else.
@@ -50,3 +51,10 @@ BraveBrowserProcessImpl::https_everywhere_service() { | |||
brave_shields::HTTPSEverywhereServiceFactory(); | |||
return https_everywhere_service_.get(); | |||
} | |||
|
|||
void BraveBrowserProcessImpl::PreMainMessageLoopRun() { |
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.
we can override CreateBrowserMainParts
in BraveContentBrowserClient
and add our own Parts*
classes with AddParts
</outputs> | ||
<release seq="1"> | ||
<includes> | ||
<include name="IDR_BRAVE_EXTENSON_INJECT_HTML" file="build/inject.html" type="BINDATA" /> |
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.
these files seem like they're missing?
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.
they're from src/brave/vendor/brave-extension
You'll need to run yarn install && yarn build
to get them from within that dir.
There's a PR on brave/brave which has build config for that.
common/url_constants.cc
Outdated
@@ -0,0 +1,4 @@ | |||
#include "brave/common/url_constants.h" |
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.
should this be webui_url_constants like in chrome?
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.
It's not webui_url_constants
in chrome, it's chrome/common/url_constants.h
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 see, I was looking at 64, but might as well do the same now?
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.
do this in a follow up as well?
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 did this when I landed it.
59a722b
to
c175206
Compare
content::WebUIDataSource::Create(kPaymentsHost); | ||
|
||
if (name == kPaymentsHost) { | ||
source->AddResourcePath(kPaymentsJS, IDR_BRAVE_PAYMENTS_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.
I was thinking we could pass these in as params in brave_web_ui_controller_factory.h
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 do this in the new tab page PR once I work on that.
extension_misc::kZIPUnpackerExtensionId, | ||
extension_misc::kZipArchiverExtensionId, | ||
#endif | ||
+ "mnojpmjdmbbfmejpflffifhffcmidifd" |
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 was thinking we would patch in a method call to add as may as we wanted, but we can do that in a follow up
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 do this once we need to add a second one.
switch (manifest_resource_id) { | ||
// Please keep the list in alphabetical order. | ||
case IDR_BOOKMARKS_MANIFEST: | ||
+ case IDR_BRAVE_EXTENSON: |
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.
same as above
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.
ditto my above comment.
Which includes: - Using chrome white list directly instead of replacing the file. - Created a browser extra parts subclass and register it, it is used for resource loading init for the browser process. - Fix linux build
"//brave/vendor/brave-extension", | ||
] | ||
|
||
output_dir = "$root_gen_dir/brave" |
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 think you want to use target_gen_dir here instead of root_gen_dir/brave
ae48d15
to
f937a93
Compare
script moved to brave-extensions and renamed to webui_url_constants.*, so landing. |
3rd party fingerprinting protection
Update adblock dep to latest
Fix brave/brave-browser-snap#5
Fix https://github.com/brave/brave/issues/45
Requires https://github.com/brave/brave/pull/46