Skip to content

Commit 094d0ec

Browse files
spylogssterspylogsster
committed
Fixup review comments
1 parent 5a89191 commit 094d0ec

File tree

6 files changed

+54
-26
lines changed

6 files changed

+54
-26
lines changed

‎browser/importer/brave_external_process_importer_host.h‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -26,13 +26,13 @@ class BraveExternalProcessImporterHost : public ExternalProcessImporterHost {
2626
using MockedInstallCallback =
2727
base::RepeatingCallback<void(const std::string&)>;
2828

29-
importer::ImporterProgressObserver* GetObserverForTesting();
30-
void NotifyImportEndedForTesting();
31-
3229
private:
3330
friend class ExternalProcessImporterHost;
3431
friend class BraveExternalProcessImporterHostUnitTest;
3532

33+
FRIEND_TEST_ALL_PREFIXES(BraveImporterObserverUnitTest, ImportEvents);
34+
FRIEND_TEST_ALL_PREFIXES(BraveImporterObserverUnitTest, DestroyObserverEarly);
35+
3636
~BraveExternalProcessImporterHost() override;
3737
void OnExtensionInstalled(const std::string& extension_id,
3838
bool success,
@@ -45,6 +45,8 @@ class BraveExternalProcessImporterHost : public ExternalProcessImporterHost {
4545
void DoNotLaunchImportForTesting();
4646
void SetInstallExtensionCallbackForTesting(MockedInstallCallback callback);
4747
void SetSettingsRemovedCallbackForTesting(base::RepeatingClosure callback);
48+
void NotifyImportEndedForTesting();
49+
importer::ImporterProgressObserver* GetObserverForTesting();
4850

4951
// ExternalProcessImporterHost overrides:
5052
void NotifyImportEnded() override;

‎browser/ui/webui/settings/brave_import_data_handler.cc‎

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,9 @@ BraveImportDataHandler::~BraveImportDataHandler() = default;
6363
void BraveImportDataHandler::StartImport(
6464
const importer::SourceProfile& source_profile,
6565
uint16_t imported_items) {
66+
if (!imported_items)
67+
return;
68+
6669
#if BUILDFLAG(IS_MAC)
6770
CheckDiskAccess(source_profile, imported_items);
6871
#else
@@ -80,22 +83,17 @@ void BraveImportDataHandler::StartImportImpl(
8083
ImportDataHandler::StartImport(source_profile, imported_items);
8184
return;
8285
}
83-
if (!imported_items)
84-
return;
8586
// If another import is already ongoing, let it finish silently.
8687
if (import_observers_.count(source_profile.source_path))
8788
import_observers_.erase(source_profile.source_path);
8889

8990
// Using weak pointers because it destroys itself when finshed.
9091
auto* importer_host = new ExternalProcessImporterHost();
9192
import_observers_[source_profile.source_path] =
92-
std::make_unique<settings::BraveImporterObserver>(
93+
std::make_unique<BraveImporterObserver>(
9394
importer_host, source_profile, imported_items,
9495
base::BindRepeating(&BraveImportDataHandler::NotifyImportProgress,
9596
weak_factory_.GetWeakPtr()));
96-
importer_host->set_observer(
97-
import_observers_[source_profile.source_path].get());
98-
9997
Profile* profile = Profile::FromWebUI(web_ui());
10098
importer_host->StartImportSettings(source_profile, profile, imported_items,
10199
new ProfileWriter(profile));

‎browser/ui/webui/settings/brave_import_data_handler.h‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@ class BraveImportDataHandler : public ImportDataHandler,
4242
// ImportDataHandler overrides:
4343
void StartImport(const importer::SourceProfile& source_profile,
4444
uint16_t imported_items) override;
45+
4546
void StartImportImpl(const importer::SourceProfile& source_profile,
4647
uint16_t imported_items);
4748
void NotifyImportProgress(const base::Value& info);
@@ -58,8 +59,7 @@ class BraveImportDataHandler : public ImportDataHandler,
5859
bool guide_dialog_is_requested_ = false;
5960
#endif
6061

61-
std::unordered_map<base::FilePath,
62-
std::unique_ptr<settings::BraveImporterObserver>>
62+
std::unordered_map<base::FilePath, std::unique_ptr<BraveImporterObserver>>
6363
import_observers_;
6464
base::WeakPtrFactory<BraveImportDataHandler> weak_factory_{this};
6565
};

‎browser/ui/webui/settings/brave_importer_observer.cc‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -10,8 +10,6 @@
1010
#include "base/logging.h"
1111
#include "chrome/browser/importer/external_process_importer_host.h"
1212

13-
namespace settings {
14-
1513
BraveImporterObserver::BraveImporterObserver(
1614
ExternalProcessImporterHost* importer_host,
1715
const importer::SourceProfile& source_profile,
@@ -22,6 +20,7 @@ BraveImporterObserver::BraveImporterObserver(
2220
callback_(std::move(callback)),
2321
importer_host_(importer_host) {
2422
DCHECK(importer_host);
23+
importer_host->set_observer(this);
2524
}
2625

2726
BraveImporterObserver::~BraveImporterObserver() {
@@ -69,9 +68,14 @@ void BraveImporterObserver::ImportEnded() {
6968
data.Set("items_to_import", imported_items_);
7069
data.Set("event", "ImportEnded");
7170
callback_.Run(base::Value(std::move(data)));
72-
71+
DCHECK(importer_host_);
7372
if (importer_host_)
7473
importer_host_->set_observer(nullptr);
74+
75+
importer_host_ = nullptr;
7576
}
7677

77-
} // namespace settings
78+
ExternalProcessImporterHost*
79+
BraveImporterObserver::GetImporterHostForTesting() {
80+
return importer_host_.get();
81+
}

‎browser/ui/webui/settings/brave_importer_observer.h‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -13,8 +13,6 @@
1313

1414
class ExternalProcessImporterHost;
1515

16-
namespace settings {
17-
1816
class BraveImporterObserver : public importer::ImporterProgressObserver {
1917
public:
2018
using ReportProgressCallback =
@@ -32,6 +30,9 @@ class BraveImporterObserver : public importer::ImporterProgressObserver {
3230
void ImportEnded() override;
3331

3432
private:
33+
FRIEND_TEST_ALL_PREFIXES(BraveImporterObserverUnitTest, ImportEvents);
34+
35+
ExternalProcessImporterHost* GetImporterHostForTesting();
3536
importer::SourceProfile source_profile_;
3637
uint16_t imported_items_ = 0;
3738
ReportProgressCallback callback_;
@@ -43,6 +44,4 @@ class BraveImporterObserver : public importer::ImporterProgressObserver {
4344
raw_ptr<ExternalProcessImporterHost> importer_host_; // weak
4445
};
4546

46-
} // namespace settings
47-
4847
#endif // BRAVE_BROWSER_UI_WEBUI_SETTINGS_BRAVE_IMPORTER_OBSERVER_H_

‎browser/ui/webui/settings/brave_importer_observer_unittest.cc‎

Lines changed: 32 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -15,8 +15,6 @@
1515
#include "content/public/test/browser_task_environment.h"
1616
#include "testing/gtest/include/gtest/gtest.h"
1717

18-
namespace settings {
19-
2018
class BraveImporterObserverUnitTest : public testing::Test {
2119
public:
2220
BraveImporterObserverUnitTest() {}
@@ -33,6 +31,7 @@ class BraveImporterObserverUnitTest : public testing::Test {
3331
content::BrowserTaskEnvironment task_environment_;
3432
base::Value expected_info_;
3533
int expected_calls_ = 0;
34+
raw_ptr<BraveExternalProcessImporterHost> _ = nullptr;
3635
};
3736

3837
TEST_F(BraveImporterObserverUnitTest, ImportEvents) {
@@ -42,8 +41,8 @@ TEST_F(BraveImporterObserverUnitTest, ImportEvents) {
4241
source_profile.importer_type = importer::TYPE_CHROME;
4342
source_profile.source_path = base::FilePath(FILE_PATH_LITERAL("test"));
4443
auto imported_items = importer::AUTOFILL_FORM_DATA | importer::PASSWORDS;
45-
std::unique_ptr<settings::BraveImporterObserver> observer =
46-
std::make_unique<settings::BraveImporterObserver>(
44+
std::unique_ptr<BraveImporterObserver> observer =
45+
std::make_unique<BraveImporterObserver>(
4746
importer_host, source_profile, imported_items,
4847
base::BindRepeating(
4948
&BraveImporterObserverUnitTest::NotifyImportProgress,
@@ -104,14 +103,40 @@ TEST_F(BraveImporterObserverUnitTest, ImportEvents) {
104103

105104
observer->ImportEnded();
106105
EXPECT_EQ(GetExpectedCalls(), 1);
106+
EXPECT_EQ(observer->GetImporterHostForTesting(), nullptr);
107107
// The observer should be removed on ImportEnded event.
108108
EXPECT_EQ(importer_host->GetObserverForTesting(), nullptr);
109-
// Checking the observer will be removed on destruction.
109+
110+
// ImportEnded event should not be called anymore.
111+
SetExpectedCalls(0);
112+
EXPECT_EQ(GetExpectedCalls(), 0);
113+
// Destroy host.
114+
importer_host->NotifyImportEndedForTesting();
115+
EXPECT_EQ(GetExpectedCalls(), 0);
116+
}
117+
118+
TEST_F(BraveImporterObserverUnitTest, DestroyObserverEarly) {
119+
auto* importer_host = new BraveExternalProcessImporterHost();
120+
importer::SourceProfile source_profile;
121+
source_profile.importer_name = u"importer_name";
122+
source_profile.importer_type = importer::TYPE_CHROME;
123+
source_profile.source_path = base::FilePath(FILE_PATH_LITERAL("test"));
124+
auto imported_items = importer::AUTOFILL_FORM_DATA | importer::PASSWORDS;
125+
std::unique_ptr<BraveImporterObserver> observer =
126+
std::make_unique<BraveImporterObserver>(
127+
importer_host, source_profile, imported_items,
128+
base::BindRepeating(
129+
&BraveImporterObserverUnitTest::NotifyImportProgress,
130+
base::Unretained(this)));
110131
importer_host->set_observer(observer.get());
132+
EXPECT_EQ(importer_host->GetObserverForTesting(), observer.get());
133+
EXPECT_EQ(GetExpectedCalls(), 0);
111134
observer.reset();
112135
EXPECT_EQ(importer_host->GetObserverForTesting(), nullptr);
136+
// ImportEnded event should not be called anymore.
137+
SetExpectedCalls(0);
138+
EXPECT_EQ(GetExpectedCalls(), 0);
113139
// Destroy host.
114140
importer_host->NotifyImportEndedForTesting();
141+
EXPECT_EQ(GetExpectedCalls(), 0);
115142
}
116-
117-
} // namespace settings

0 commit comments

Comments
 (0)