Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use brave:// for the virtual url and chrome:// for the internal u… #1385

Merged
merged 2 commits into from
Jan 31, 2019
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 38 additions & 13 deletions browser/brave_content_browser_client.cc
Original file line number Diff line number Diff line change
Expand Up @@ -61,29 +61,52 @@ using extensions::ChromeContentBrowserClientExtensionsPart;

namespace {

bool HandleURLRewrite(GURL* url,
content::BrowserContext* browser_context) {
if (url->SchemeIs(content::kChromeUIScheme) &&
(url->host() == chrome::kChromeUIWelcomeHost ||
url->host() == chrome::kChromeUIWelcomeWin10Host)) {
*url = GURL(kBraveUIWelcomeURL);
bool HandleURLOverrideRewrite(GURL* url,
content::BrowserContext* browser_context) {
// redirect sync-internals
if (url->host() == chrome::kChromeUISyncInternalsHost ||
url->host() == chrome::kChromeUISyncHost) {
Copy link
Member

@simonhong simonhong Jan 23, 2019

Choose a reason for hiding this comment

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

nit: I think this second condition in if clause can be removed.

GURL::Replacements replacements;
replacements.SetHostStr(chrome::kChromeUISyncHost);
*url = url->ReplaceComponents(replacements);
return true;
}
if (url->SchemeIs(content::kChromeUIScheme) &&
(url->host() == kBraveUISyncHost)) {
*url = GURL(kBraveUISyncURL);

// no special win10 welcome page
if (url->host() == chrome::kChromeUIWelcomeWin10Host ||
url->host() == chrome::kChromeUIWelcomeHost) {
*url = GURL(chrome::kChromeUIWelcomeURL);
Copy link
Member

Choose a reason for hiding this comment

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

Q) Why do you make rest handler skip for these two hosts (sync and welcome) by returning true?
Do you think upstream url handler doesn't need to handle them?

Copy link
Member

Choose a reason for hiding this comment

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

Ah! I saw upstream code tries to rewritten it. Ignore my above comment.

return true;
}

return false;
}

bool HandleURLReverseRewrite(GURL* url,

bool HandleURLReverseOverrideRewrite(GURL* url,
content::BrowserContext* browser_context) {
if (url->spec() == kBraveUIWelcomeURL ||
url->spec() == kBraveUISyncURL) {
if (url->host() == chrome::kChromeUIWelcomeHost ||
url->host() == chrome::kChromeUISyncHost) {
GURL::Replacements replacements;
replacements.SetSchemeStr(kBraveUIScheme);
*url = url->ReplaceComponents(replacements);
return true;
}

return false;
}

bool HandleURLRewrite(GURL* url,
content::BrowserContext* browser_context) {
if (url->SchemeIs(kBraveUIScheme)) {
GURL::Replacements replacements;
replacements.SetSchemeStr(content::kChromeUIScheme);
*url = url->ReplaceComponents(replacements);
}

if (HandleURLOverrideRewrite(url, browser_context))
return true;

return false;
}

Expand All @@ -110,8 +133,10 @@ void BraveContentBrowserClient::BrowserURLHandlerCreated(
handler->AddHandlerPair(&webtorrent::HandleTorrentURLRewrite,
&webtorrent::HandleTorrentURLReverseRewrite);
handler->AddHandlerPair(&HandleURLRewrite,
&HandleURLReverseRewrite);
&HandleURLReverseOverrideRewrite);
ChromeContentBrowserClient::BrowserURLHandlerCreated(handler);
handler->AddHandlerPair(&HandleURLOverrideRewrite,
&HandleURLReverseOverrideRewrite);
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to add new pair after adding super class' handlers?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, the order is critical because of rewriting that the superclass does

}

bool BraveContentBrowserClient::AllowAccessCookie(const GURL& url, const GURL& first_party,
Expand Down
134 changes: 113 additions & 21 deletions browser/brave_content_browser_client_browsertest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
#include "chrome/browser/ui/browser_window.h"
#include "chrome/common/buildflags.h"
#include "chrome/common/chrome_content_client.h"
#include "chrome/common/webui_url_constants.h"
#include "chrome/test/base/in_process_browser_test.h"
#include "chrome/test/base/ui_test_utils.h"
#include "components/content_settings/core/browser/host_content_settings_map.h"
Expand Down Expand Up @@ -114,36 +115,127 @@ class BraveContentBrowserClientTest : public InProcessBrowserTest {
};

IN_PROC_BROWSER_TEST_F(BraveContentBrowserClientTest, CanLoadChromeURL) {
GURL chrome_settings_url("chrome://about/");
std::vector<GURL> urls{chrome_settings_url, GURL("about:about/"),
GURL("brave://about/")};
std::for_each(urls.begin(), urls.end(),
[this, &chrome_settings_url](const GURL& url) {
content::WebContents* contents =
browser()->tab_strip_model()->GetActiveWebContents();
ui_test_utils::NavigateToURL(browser(), url);
ASSERT_TRUE(WaitForLoadStop(contents));
EXPECT_STREQ(contents->GetLastCommittedURL().spec().c_str(),
chrome_settings_url.spec().c_str());
});
std::vector<std::string> pages {
chrome::kChromeUIWelcomeHost,
};

std::vector<std::string> schemes {
"about:",
"brave://",
"chrome://",
};

for (const std::string& page : pages) {
for (const std::string& scheme : schemes) {
content::WebContents* contents =
browser()->tab_strip_model()->GetActiveWebContents();
ui_test_utils::NavigateToURL(browser(), GURL(scheme + page + "/"));
ASSERT_TRUE(WaitForLoadStop(contents));

EXPECT_STREQ(contents->GetController().GetLastCommittedEntry()
->GetVirtualURL().spec().c_str(),
("brave://" + page + "/").c_str());
EXPECT_STREQ(contents->GetController().GetLastCommittedEntry()
->GetURL().spec().c_str(),
("chrome://" + page + "/").c_str());
}
}
}

IN_PROC_BROWSER_TEST_F(BraveContentBrowserClientTest, CanLoadCustomBravePages) {
std::vector<GURL> urls {
GURL("chrome://adblock/"),
std::vector<std::string> pages {
"adblock",
#if BUILDFLAG(BRAVE_REWARDS_ENABLED)
GURL("chrome://rewards/"),
"rewards",
#endif
GURL("chrome://welcome/"), GURL("chrome://sync/")
chrome::kChromeUISyncHost,
};

std::vector<std::string> schemes {
"brave://",
"chrome://",
};

for (const std::string& page : pages) {
for (const std::string& scheme : schemes) {
content::WebContents* contents =
browser()->tab_strip_model()->GetActiveWebContents();
ui_test_utils::NavigateToURL(browser(), GURL(scheme + page + "/"));
ASSERT_TRUE(WaitForLoadStop(contents));

EXPECT_STREQ(contents->GetController().GetLastCommittedEntry()
->GetVirtualURL().spec().c_str(),
("brave://" + page + "/").c_str());
EXPECT_STREQ(contents->GetController().GetLastCommittedEntry()
->GetURL().spec().c_str(),
("chrome://" + page + "/").c_str());
}
}
}

IN_PROC_BROWSER_TEST_F(BraveContentBrowserClientTest, CanLoadAboutHost) {
std::vector<std::string> schemes {
"chrome://",
"brave://",
};
std::for_each(urls.begin(), urls.end(), [this](const GURL& url) {

for (const std::string& scheme : schemes) {
content::WebContents* contents =
browser()->tab_strip_model()->GetActiveWebContents();
ui_test_utils::NavigateToURL(browser(), url);
ui_test_utils::NavigateToURL(browser(), GURL(scheme + "about/"));
ASSERT_TRUE(WaitForLoadStop(contents));

EXPECT_STREQ(contents->GetController().GetLastCommittedEntry()
->GetVirtualURL().spec().c_str(),
"brave://about/");
EXPECT_STREQ(contents->GetController().GetLastCommittedEntry()
->GetURL().spec().c_str(),
"chrome://chrome-urls/");
}
}

IN_PROC_BROWSER_TEST_F(BraveContentBrowserClientTest,
RewriteChromeSyncInternals) {
std::vector<std::string> schemes {
"brave://",
"chrome://",
};

for (const std::string& scheme : schemes) {
content::WebContents* contents =
browser()->tab_strip_model()->GetActiveWebContents();
ui_test_utils::NavigateToURL(browser(), GURL(scheme + "sync-internals/"));
ASSERT_TRUE(WaitForLoadStop(contents));

EXPECT_STREQ(contents->GetController().GetLastCommittedEntry()
->GetVirtualURL().spec().c_str(),
"brave://sync/");
EXPECT_STREQ(contents->GetController().GetLastCommittedEntry()
->GetURL().spec().c_str(),
"chrome://sync/");
}
}

IN_PROC_BROWSER_TEST_F(BraveContentBrowserClientTest,
RewriteChromeWelcomeWin10) {
std::vector<std::string> schemes {
"brave://",
"chrome://",
};

for (const std::string& scheme : schemes) {
content::WebContents* contents =
browser()->tab_strip_model()->GetActiveWebContents();
ui_test_utils::NavigateToURL(browser(), GURL(scheme + "welcome-win10/"));
ASSERT_TRUE(WaitForLoadStop(contents));
EXPECT_STREQ(contents->GetLastCommittedURL().spec().c_str(),
url.spec().c_str());
});

EXPECT_STREQ(contents->GetController().GetLastCommittedEntry()
->GetVirtualURL().spec().c_str(),
"brave://welcome/");
EXPECT_STREQ(contents->GetController().GetLastCommittedEntry()
->GetURL().spec().c_str(),
"chrome://welcome/");
}
}

IN_PROC_BROWSER_TEST_F(BraveContentBrowserClientTest, RewriteMagnetURLURLBar) {
Expand Down
2 changes: 1 addition & 1 deletion browser/ui/webui/brave_web_ui_controller_factory.cc
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ WebUIFactoryFunction GetWebUIFactoryFunction(WebUI* web_ui,
url.host_piece() == kDonateHost ||
#endif
url.host_piece() == kWelcomeHost ||
url.host_piece() == kBraveUIWelcomeURL ||
url.host_piece() == chrome::kChromeUIWelcomeURL ||
(url.host_piece() == kBraveUISyncHost &&
brave_sync::BraveSyncService::is_enabled()) ||
url.host_piece() == chrome::kChromeUINewTabHost ||
Expand Down
7 changes: 5 additions & 2 deletions browser/ui/webui/brave_welcome_ui_browsertest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
#include "chrome/browser/ui/tabs/tab_strip_model.h"
#include "chrome/common/webui_url_constants.h"
#include "chrome/test/base/in_process_browser_test.h"
#include "content/public/browser/navigation_entry.h"
#include "content/public/test/test_navigation_observer.h"
#include "content/public/test/test_utils.h"

Expand All @@ -38,8 +39,10 @@ IN_PROC_BROWSER_TEST_F(BraveWelcomeUIBrowserTest, PRE_StartupURLTest) {
content::WebContents* web_contents = tab_strip->GetWebContentsAt(0);
content::TestNavigationObserver observer(web_contents, 1);
observer.Wait();
EXPECT_EQ(kBraveUIWelcomeURL,
tab_strip->GetWebContentsAt(0)->GetURL().possibly_invalid_spec());
EXPECT_STREQ("brave://welcome/",
tab_strip->GetWebContentsAt(0)
->GetController().GetLastCommittedEntry()
->GetVirtualURL().possibly_invalid_spec().c_str());
}

// Check wheter startup url is not welcome ui at second run.
Expand Down
37 changes: 19 additions & 18 deletions chromium_src/chrome/browser/browser_about_handler.cc
Original file line number Diff line number Diff line change
Expand Up @@ -3,32 +3,33 @@
* You can obtain one at http://mozilla.org/MPL/2.0/. */

#define FixupBrowserAboutURL FixupBrowserAboutURL_ChromiumImpl
#define WillHandleBrowserAboutURL WillHandleBrowserAboutURL_ChromiumImpl
#include "../../../../chrome/browser/browser_about_handler.cc"
#undef FixupBrowserAboutURL
#undef WillHandleBrowserAboutURL

#include "brave/common/url_constants.h"
#include "brave/common/webui_url_constants.h"

bool FixupBrowserAboutURLBraveImpl(GURL* url,
content::BrowserContext* browser_context) {
if (url->SchemeIs(kBraveUIScheme)) {
bool FixupBrowserAboutURL(GURL* url,
content::BrowserContext* browser_context) {
bool result = FixupBrowserAboutURL_ChromiumImpl(url, browser_context);

// no special win10 welcome page
if (url->host() == chrome::kChromeUIWelcomeWin10Host)
*url = GURL(chrome::kChromeUIWelcomeURL);

// redirect sync-internals
if (url->host() == chrome::kChromeUISyncInternalsHost) {
GURL::Replacements replacements;
replacements.SetSchemeStr(content::kChromeUIScheme);
replacements.SetHostStr(chrome::kChromeUISyncHost);
*url = url->ReplaceComponents(replacements);
}
return true;
}


bool FixupBrowserAboutURL(GURL* url,
content::BrowserContext* browser_context) {
FixupBrowserAboutURLBraveImpl(url, browser_context);
return FixupBrowserAboutURL_ChromiumImpl(url, browser_context);
}
if (url->SchemeIs(content::kChromeUIScheme) &&
url->host() != chrome::kChromeUINewTabHost) {
Copy link
Member

Choose a reason for hiding this comment

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

Just curiosity, any particular reason about excluding newtab host?

Copy link
Collaborator Author

@bridiver bridiver Jan 31, 2019

Choose a reason for hiding this comment

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

yes, it breaks the extension newtab override functionality

GURL::Replacements replacements;
replacements.SetSchemeStr(kBraveUIScheme);
*url = url->ReplaceComponents(replacements);
}

bool WillHandleBrowserAboutURL(GURL* url,
content::BrowserContext* browser_context) {
FixupBrowserAboutURLBraveImpl(url, browser_context);
return WillHandleBrowserAboutURL_ChromiumImpl(url, browser_context);
return result;
}
3 changes: 0 additions & 3 deletions chromium_src/chrome/browser/ui/browser.cc
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,9 @@
#include "chrome/browser/ui/browser_content_setting_bubble_model_delegate.h"
#include "brave/browser/ui/brave_browser_content_setting_bubble_model_delegate.h"
#include "brave/browser/ui/brave_browser_command_controller.h"
#include "brave/components/omnibox/browser/brave_location_bar_model_impl.h"

#define LocationBarModelImpl BraveLocationBarModelImpl
#define BrowserContentSettingBubbleModelDelegate BraveBrowserContentSettingBubbleModelDelegate
#define BrowserCommandController BraveBrowserCommandController
#include "../../../../../chrome/browser/ui/browser.cc"
#undef BrowserContentSettingBubbleModelDelegate
#undef BrowserCommandController
#undef LocationBarModelImpl
1 change: 0 additions & 1 deletion common/webui_url_constants.cc
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ const char kDonateJS[] = "brave_donate.js";
const char kBraveNewTabJS[] = "brave_new_tab.js";
const char kBraveUISyncHost[] = "sync";
const char kBraveSyncJS[] = "brave_sync.js";
const char kBraveUIWelcomeURL[] = "chrome://welcome/";
const char kBraveUIRewardsURL[] = "chrome://rewards/";
const char kBraveUIAdblockURL[] = "chrome://adblock/";
const char kBraveUIDonateURL[] = "chrome://donate/";
Expand Down
1 change: 0 additions & 1 deletion common/webui_url_constants.h
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ extern const char kDonateJS[];
extern const char kBraveNewTabJS[];
extern const char kBraveUISyncHost[];
extern const char kBraveSyncJS[];
extern const char kBraveUIWelcomeURL[];
extern const char kBraveUIRewardsURL[];
extern const char kBraveUIAdblockURL[];
extern const char kBraveUIDonateHost[];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ class BraveRewardsBrowserTest : public InProcessBrowserTest {
}

GURL rewards_url() {
GURL rewards_url("chrome://rewards");
GURL rewards_url("brave://rewards");
return rewards_url;
}

Expand Down
19 changes: 1 addition & 18 deletions components/omnibox/browser/BUILD.gn
Original file line number Diff line number Diff line change
@@ -1,13 +1,11 @@
source_set("browser") {
# Only //components/omnibox/browser target can depend on this target
# because this target expands //components/omnibox/browser implementation.
visibility = [ "//components/omnibox/browser", ":unit_tests" ]
visibility = [ "//components/omnibox/browser" ]

sources = [
"brave_autocomplete_controller.cc",
"brave_autocomplete_controller.h",
"brave_location_bar_model_impl.cc",
"brave_location_bar_model_impl.h",
"constants.cc",
"constants.h",
"topsites_provider_data.cc",
Expand All @@ -22,18 +20,3 @@ source_set("browser") {
"//third_party/metrics_proto",
]
}

source_set("unit_tests") {
testonly = true
sources = [
"location_bar_model_impl_unittest.cc",
]

deps = [
":browser",
"//base",
"//components/omnibox/browser",
"//testing/gtest",
"//url",
]
}
Loading