-
Notifications
You must be signed in to change notification settings - Fork 109
Emit show-warning-dialog by app #196
Conversation
requires brave/muon#196 fix #8846 Auditors: @bridiver, @bbondy Test Plan: 1. Open Firefox 2. Open Brave and import from Firefox 3. There will be a warning window
@@ -26,7 +28,13 @@ | |||
|
|||
namespace importer { | |||
void ShowImportLockDialog(gfx::NativeWindow parent, | |||
const base::Callback<void(bool)>& callback) {} | |||
const base::Callback<void(bool)>& callback) { |
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.
don't we have to call this callback with is_continue
for OnImportLockDialogEnd
? Seems like we should send the callback in the emit
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.
because the current behavior is requiring users to do importing again (importer process ends)
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.
then shouldn't we always call it with false
to trigger NotifyImportEnded
?
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
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.
addressed in 783033a
@@ -339,6 +339,11 @@ source_set("importer") { | |||
"//electron/build:electron_config" | |||
] | |||
|
|||
include_dirs = [ | |||
# make sure chromium_src comes before the chrome src root | |||
"//electron/chromium_src", |
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 what you want here is a dep on //electron/chromium_src:importer
the only place we manually add include dirs in in chromium_src/BUILD.gn and even that shouldn't be necessary anymore now that we fixed the patches
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 merge, but please fix this 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.
It is used for the BrowserProcessImpl
../../electron/atom/browser/importer/profile_writer.cc:34:58: error: no member named 'app' in 'BrowserProcessImpl'
static_cast<BrowserProcessImpl*>(g_browser_process)->app();
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^
But if I use dep //electron/chromium_src:browser
, it will cause dep cycles
ERROR Dependency cycle:
//electron/chromium_src:browser ->
//electron/atom/browser:browser ->
//electron/atom/browser:importer ->
//electron/chromium_src:browser
requires brave/muon#196 fix #8846 Auditors: @bridiver, @bbondy Test Plan: 1. Open Firefox 2. Open Brave and import from Firefox 3. There will be a warning window
also fix lint
Auditors: @bridiver, @bbondy