Skip to content
This repository has been archived by the owner on Apr 3, 2020. It is now read-only.

Commit

Permalink
Merge of b212392 to branch-heads/2564 (M48).
Browse files Browse the repository at this point in the history 
Fix for multi-profile triggered reset API usage.

Previously, the second and future opened profiles would not re-show the reset flow if there was not an unclean shutdown due to AddSpecialURLs being called twice if the "if (!HasPendingUncleanExit(profile_)) " branch was taken here: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/startup/startup_browser_creator_impl.cc&l=534

This CL takes a saner approach to preserving the "has reset trigger" state until after Launch() has completed.

It also improves the browser test by making the profile state more explicitly controlled at each stage of the tests.

BUG=508970
[email protected]

Review URL: https://codereview.chromium.org/1465143002

Cr-Commit-Position: refs/heads/master@{#361739}
(cherry picked from commit b212392)

Review URL: https://codereview.chromium.org/1483983002 .

Cr-Commit-Position: refs/branch-heads/2564@{#158}
Cr-Branched-From: 1283eca-refs/heads/master@{#359700}
  • Loading branch information
@robertshield
robertshield committed Nov 30, 2015
1 parent b6b8b7a commit 9a0f44f
Show file tree
Hide file tree
Showing 4 changed files with 96 additions and 8 deletions.
6 changes: 3 additions & 3 deletions chrome/browser/ui/startup/startup_browser_creator_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -905,9 +905,10 @@ void StartupBrowserCreatorImpl::AddSpecialURLs(

// If this Profile is marked for a reset prompt, ensure the reset
// settings dialog appears.
if (CheckAndClearProfileResetTrigger())
if (ProfileHasResetTrigger()) {
url_list->insert(url_list->begin(),
internals::GetTriggeredResetSettingsURL());
}
}

// For first-run, the type will be FIRST_RUN_LAST for all systems except for
Expand Down Expand Up @@ -989,15 +990,14 @@ void StartupBrowserCreatorImpl::RecordRapporOnStartupURLs(
}
}

bool StartupBrowserCreatorImpl::CheckAndClearProfileResetTrigger() const {
bool StartupBrowserCreatorImpl::ProfileHasResetTrigger() const {
bool has_reset_trigger = false;
#if defined(OS_WIN)
TriggeredProfileResetter* triggered_profile_resetter =
TriggeredProfileResetterFactory::GetForBrowserContext(profile_);
// TriggeredProfileResetter instance will be nullptr for incognito profiles.
if (triggered_profile_resetter) {
has_reset_trigger = triggered_profile_resetter->HasResetTrigger();
triggered_profile_resetter->ClearResetTrigger();
}
#endif // defined(OS_WIN)
return has_reset_trigger;
Expand Down
5 changes: 2 additions & 3 deletions chrome/browser/ui/startup/startup_browser_creator_impl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -158,9 +158,8 @@ class StartupBrowserCreatorImpl {
// Record Rappor metrics on startup URLs.
void RecordRapporOnStartupURLs(const std::vector<GURL>& urls_to_open);

// Checks whether |profile_| has a reset trigger set and then clears the
// reset trigger.
bool CheckAndClearProfileResetTrigger() const;
// Checks whether |profile_| has a reset trigger set.
bool ProfileHasResetTrigger() const;

const base::FilePath cur_dir_;
const base::CommandLine& command_line_;
Expand Down
87 changes: 86 additions & 1 deletion chrome/browser/ui/startup/startup_browser_creator_triggered_reset_browsertest_win.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
#include "chrome/browser/browser_process.h"
#include "chrome/browser/profile_resetter/triggered_profile_resetter.h"
#include "chrome/browser/profile_resetter/triggered_profile_resetter_factory.h"
#include "chrome/browser/profiles/profile_manager.h"
#include "chrome/browser/signin/signin_promo.h"
#include "chrome/browser/ui/browser.h"
#include "chrome/browser/ui/browser_finder.h"
Expand All @@ -19,6 +20,7 @@
#include "chrome/browser/ui/tabs/tab_strip_model.h"
#include "chrome/common/url_constants.h"
#include "chrome/test/base/in_process_browser_test.h"
#include "chrome/test/base/ui_test_utils.h"
#include "components/keyed_service/content/browser_context_dependency_manager.h"
#include "testing/gtest/include/gtest/gtest.h"

Expand All @@ -44,12 +46,18 @@ class MockTriggeredProfileResetter : public TriggeredProfileResetter {
MockTriggeredProfileResetter() : TriggeredProfileResetter(nullptr) {}

void Activate() override {}
bool HasResetTrigger() override { return true; }
bool HasResetTrigger() override { return has_reset_trigger_; }
static void SetHasResetTrigger(bool has_reset_trigger) {
has_reset_trigger_ = has_reset_trigger;
}

private:
static bool has_reset_trigger_;
DISALLOW_COPY_AND_ASSIGN(MockTriggeredProfileResetter);
};

bool MockTriggeredProfileResetter::has_reset_trigger_ = false;

scoped_ptr<KeyedService> BuildMockTriggeredProfileResetter(
content::BrowserContext* context) {
return make_scoped_ptr(new MockTriggeredProfileResetter);
Expand Down Expand Up @@ -106,6 +114,9 @@ IN_PROC_BROWSER_TEST_F(StartupBrowserCreatorTriggeredResetTest,
// Close the browser.
CloseBrowserAsynchronously(browser());

// Prep the next launch to offer a reset prompt.
MockTriggeredProfileResetter::SetHasResetTrigger(true);

// Do a simple non-process-startup browser launch.
base::CommandLine dummy(base::CommandLine::NO_PROGRAM);
StartupBrowserCreatorImpl launch(base::FilePath(), dummy,
Expand Down Expand Up @@ -141,6 +152,9 @@ IN_PROC_BROWSER_TEST_F(StartupBrowserCreatorTriggeredResetTest,
browser_creator.AddFirstRunTab(GURL("http://new_tab_page"));
browser_creator.AddFirstRunTab(test_server()->GetURL("files/title1.html"));

// Prep the next launch to be offered a reset prompt.
MockTriggeredProfileResetter::SetHasResetTrigger(true);

// Do a process-startup browser launch.
base::CommandLine dummy(base::CommandLine::NO_PROGRAM);
StartupBrowserCreatorImpl launch(base::FilePath(), dummy, &browser_creator,
Expand All @@ -165,3 +179,74 @@ IN_PROC_BROWSER_TEST_F(StartupBrowserCreatorTriggeredResetTest,
EXPECT_EQ("title1.html",
tab_strip->GetWebContentsAt(1)->GetURL().ExtractFileName());
}

IN_PROC_BROWSER_TEST_F(StartupBrowserCreatorTriggeredResetTest,
TestMultiProfile) {
SessionStartupPref pref(SessionStartupPref::DEFAULT);
SessionStartupPref::SetStartupPref(browser()->profile(), pref);

// Keep the browser process running while browsers are closed.
g_browser_process->AddRefModule();

// Close the browser.
CloseBrowserAsynchronously(browser());

// Prep the next launch to offer a reset prompt.
MockTriggeredProfileResetter::SetHasResetTrigger(true);

// Do a simple non-process-startup browser launch.
base::CommandLine dummy(base::CommandLine::NO_PROGRAM);
{
StartupBrowserCreatorImpl launch(base::FilePath(), dummy,
chrome::startup::IS_NOT_FIRST_RUN);
ASSERT_TRUE(launch.Launch(browser()->profile(), std::vector<GURL>(), false,
browser()->host_desktop_type()));
}

// This should have created a new browser window. |browser()| is still
// around at this point, even though we've closed its window.
Browser* new_browser = FindOneOtherBrowser(browser());
ASSERT_TRUE(new_browser);

// Now create a second browser instance pointing to a different profile.
ProfileManager* profile_manager = g_browser_process->profile_manager();
base::FilePath path =
profile_manager->user_data_dir().AppendASCII("test_profile");
Profile* other_profile =
Profile::CreateProfile(path, nullptr, Profile::CREATE_MODE_SYNCHRONOUS);
profile_manager->RegisterTestingProfile(other_profile, true, false);

// Use a couple same-site HTTP URLs.
ASSERT_TRUE(test_server()->Start());
std::vector<GURL> urls;
urls.push_back(test_server()->GetURL("files/title1.html"));
urls.push_back(test_server()->GetURL("files/title2.html"));

// Set the startup preference to open these URLs.
SessionStartupPref other_prefs(SessionStartupPref::URLS);
other_prefs.urls = urls;
SessionStartupPref::SetStartupPref(other_profile, other_prefs);

// Again prep the next launch to get a reset prompt.
MockTriggeredProfileResetter::SetHasResetTrigger(true);

// Same kind of simple non-process-startup browser launch.
{
StartupBrowserCreatorImpl launch(base::FilePath(), dummy,
chrome::startup::IS_NOT_FIRST_RUN);
ASSERT_TRUE(launch.Launch(other_profile, std::vector<GURL>(), false,
new_browser->host_desktop_type()));
}

Browser* other_profile_browser =
chrome::FindBrowserWithProfile(other_profile,
new_browser->host_desktop_type());
ASSERT_NE(nullptr, other_profile_browser);

// Check for the expected reset dialog in the second browser too.
TabStripModel* other_tab_strip = other_profile_browser->tab_strip_model();
ASSERT_LT(0, other_tab_strip->count());
EXPECT_EQ(internals::GetTriggeredResetSettingsURL(),
other_tab_strip->GetActiveWebContents()->GetURL());
g_browser_process->ReleaseModule();
}
6 changes: 5 additions & 1 deletion chrome/browser/ui/webui/options/reset_profile_settings_handler.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -92,8 +92,12 @@ void ResetProfileSettingsHandler::GetLocalizedValues(
TriggeredProfileResetter* triggered_profile_resetter =
TriggeredProfileResetterFactory::GetForBrowserContext(profile);
// TriggeredProfileResetter instance will be nullptr for incognito profiles.
if (triggered_profile_resetter)
if (triggered_profile_resetter) {
reset_tool_name = triggered_profile_resetter->GetResetToolName();

// Now that a reset UI has been shown, don't trigger again for this profile.
triggered_profile_resetter->ClearResetTrigger();
}
#endif

if (reset_tool_name.empty()) {
Expand Down

0 comments on commit 9a0f44f

Please sign in to comment.