-
Notifications
You must be signed in to change notification settings - Fork 114
Conversation
Changes look good to me 😄 @bridiver is fixing linking issues w/ icu; after those are fixed I'll be able to try the fix out (and then will officially approve) |
@@ -1597,7 +1610,7 @@ index cb7913b0a05f54ea405f4da4b2ca050db72814d3..7bc5de612ef4c9eaaf1c2335f40e38fc | |||
+ // url.SchemeIsWSOrWSS())); |
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 you remove these changes? I think they're just whitespace, but I'd prefer to update them in a separate commit to avoid confusion
index 466e7ee84bf8cc45697def84f00af5c05f437f0f..ce083c782a4865ff0b955ab77b3cabe79b97383c 100644 | ||
--- a/build/win/BUILD.gn | ||
+++ b/build/win/BUILD.gn | ||
@@ -12,5 +12,8 @@ windows_manifest("default_exe_manifest") { |
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.
actually I don't think we want this change at all. We just need to replace the deps in electron_app
@@ -160,3 +160,10 @@ copy("dist_resources") { | |||
] | |||
} | |||
|
|||
windows_manifest("electron_manifest") { |
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'm a little confused because this isn't declared as a dep anywhere. I think it should replace "//build/win:default_exe_manifest"
in :electron_app
. I think it also needs default_compatibility_manifest
, common_controls_manifest
and as_invoker_manifest
to match the current atom.manifest file, but
default_compatibility_manifest, | ||
] | ||
+ deps = [ | ||
+ "//electorn/app/win:electron_manifest", |
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.
whoops- just noticed a misspelling here (s/electorn/electron)
I'll take care of the changes and update |
@@ -37,9 +37,6 @@ | |||
# Use C++11 library. | |||
'CLANG_CXX_LIBRARY': 'libc++', # -stdlib=libc++ | |||
}, | |||
'defines!': [ |
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 this snuck in there by accident
@@ -31,7 +31,6 @@ executable("electron_app") { | |||
] | |||
|
|||
sources = [ | |||
"//electron/atom/browser/resources/win/atom.rc", |
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'm actually not sure about this change, but we can delete the atom.manifest file
fix brave/browser-laptop#5926 Auditors: @bridiver, @bsclifton
++ |
fix brave/browser-laptop#5926
Auditors: @bridiver, @bsclifton