Skip to content
This repository has been archived by the owner on Jan 4, 2019. It is now read-only.

Emit show-warning-dialog by app #196

Merged
merged 1 commit into from
May 16, 2017
Merged

Emit show-warning-dialog by app #196

merged 1 commit into from
May 16, 2017

Conversation

darkdh
Copy link
Member

@darkdh darkdh commented May 13, 2017

also fix lint

Auditors: @bridiver, @bbondy

@darkdh darkdh self-assigned this May 13, 2017
@darkdh darkdh requested review from bridiver and bbondy May 13, 2017 03:57
darkdh added a commit to brave/browser-laptop that referenced this pull request May 13, 2017
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) {
Copy link
Collaborator

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

Copy link
Member Author

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)

Copy link
Collaborator

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will do

Copy link
Member Author

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",
Copy link
Collaborator

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

Copy link
Collaborator

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

Copy link
Member Author

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

@bridiver bridiver merged commit 1b4abcb into master May 16, 2017
bsclifton pushed a commit to brave/browser-laptop that referenced this pull request May 16, 2017
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
@darkdh darkdh deleted the 8846 branch May 23, 2017 18:26
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants