Skip to content

Commit

Permalink
Display chrome:// urls as brave:// in the LocationBar
Browse files Browse the repository at this point in the history
This only affects the DisplayURL in that, when the url is being edited or copied, the actual chrome://*** url will be present.
This seems acceptable given:
- These urls 99% of the time are for display purposes only, as a result of another UI action, and will not be typed manually
- We intend to support brave://*** actual urls, redirecting to chrome://*** actual URLs automatically, which will get displayed as brave://*** URLs (brave/brave-browser#810)

Fix brave/brave-browser#1458
  • Loading branch information
petemill committed Oct 13, 2018
1 parent cf2c77a commit 4136ab9
Show file tree
Hide file tree
Showing 9 changed files with 165 additions and 4 deletions.
3 changes: 3 additions & 0 deletions chromium_src/chrome/browser/ui/browser.cc
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,12 @@
#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/toolbar/brave_toolbar_model_impl.h"

#define ToolbarModelImpl BraveToolbarModelImpl
#define BrowserContentSettingBubbleModelDelegate BraveBrowserContentSettingBubbleModelDelegate
#define BrowserCommandController BraveBrowserCommandController
#include "../../../../../chrome/browser/ui/browser.cc"
#undef BrowserContentSettingBubbleModelDelegate
#undef BrowserCommandController
#undef ToolbarModelImpl
30 changes: 30 additions & 0 deletions components/toolbar/BUILD.gn
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
source_set("toolbar") {

sources = [
"brave_toolbar_model_impl.cc",
"brave_toolbar_model_impl.h",
"constants.cc",
"constants.h",
]

deps = [
"//base",
"//components/toolbar"
]

}

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

deps = [
":toolbar",
"//base",
"//components/toolbar",
"//testing/gtest",
"//url",
]
}
36 changes: 36 additions & 0 deletions components/toolbar/brave_toolbar_model_impl.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
// This Source Code Form is subject to the terms of the Mozilla Public
// License, v. 2.0. If a copy of the MPL was not distributed with this file,
// you can obtain one at http://mozilla.org/MPL/2.0/.

#include "brave/components/toolbar/brave_toolbar_model_impl.h"

#include "base/strings/string_util.h"
#include "base/strings/utf_string_conversions.h"
#include "brave/components/toolbar/constants.h"
#include "components/toolbar/toolbar_model_impl.h"

using namespace brave_toolbar;

namespace {

const base::string16 original_scheme_part =
base::ASCIIToUTF16(kOriginalInternalUIScheme);
const base::string16 replacement_scheme_part =
base::ASCIIToUTF16(kInternalUIScheme);

}

base::string16 BraveToolbarModelImpl::GetURLForDisplay() const {
base::string16 formatted_text = ToolbarModelImpl::GetURLForDisplay();

const GURL url(GetURL());
// Only replace chrome:// with brave:// if scheme is "chrome" and
// it has not been stripped from the display text
if (url.SchemeIs(kOriginalInternalUIScheme) &&
base::StartsWith(formatted_text, original_scheme_part,
base::CompareCase::INSENSITIVE_ASCII)) {
formatted_text.replace(0, original_scheme_part.length(),
replacement_scheme_part);
}
return formatted_text;
}
18 changes: 18 additions & 0 deletions components/toolbar/brave_toolbar_model_impl.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
// This Source Code Form is subject to the terms of the Mozilla Public
// License, v. 2.0. If a copy of the MPL was not distributed with this file,
// you can obtain one at http://mozilla.org/MPL/2.0/.

#ifndef BRAVE_COMPONENTS_TOOLBAR_BRAVE_TOOLBAR_MODEL_IMPL_H_
#define BRAVE_COMPONENTS_TOOLBAR_BRAVE_TOOLBAR_MODEL_IMPL_H_

#include "components/toolbar/toolbar_model_impl.h"

class BraveToolbarModelImpl : public ToolbarModelImpl {
public:
using ToolbarModelImpl::ToolbarModelImpl;
base::string16 GetURLForDisplay() const override;
private:
DISALLOW_COPY_AND_ASSIGN(BraveToolbarModelImpl);
};

#endif
10 changes: 10 additions & 0 deletions components/toolbar/constants.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
// This Source Code Form is subject to the terms of the Mozilla Public
// License, v. 2.0. If a copy of the MPL was not distributed with this file,
// you can obtain one at http://mozilla.org/MPL/2.0/.

#include "brave/components/toolbar/constants.h"

namespace brave_toolbar {
const char kOriginalInternalUIScheme[] = "chrome";
const char kInternalUIScheme[] = "brave";
}
13 changes: 13 additions & 0 deletions components/toolbar/constants.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
// This Source Code Form is subject to the terms of the Mozilla Public
// License, v. 2.0. If a copy of the MPL was not distributed with this file,
// you can obtain one at http://mozilla.org/MPL/2.0/.

#ifndef BRAVE_COMPONENTS_TOOLBAR_CONSTANTS_H_
#define BRAVE_COMPONENTS_TOOLBAR_CONSTANTS_H_

namespace brave_toolbar {
extern const char kOriginalInternalUIScheme[];
extern const char kInternalUIScheme[];
}

#endif
47 changes: 47 additions & 0 deletions components/toolbar/toolbar_model_impl_unittest.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
// This Source Code Form is subject to the terms of the Mozilla Public
// License, v. 2.0. If a copy of the MPL was not distributed with this file,
// you can obtain one at http://mozilla.org/MPL/2.0/.

#include "brave/components/toolbar/brave_toolbar_model_impl.h"

#include "base/strings/utf_string_conversions.h"
#include "components/toolbar/toolbar_model_delegate.h"
#include "testing/gtest/include/gtest/gtest.h"
#include "url/gurl.h"

namespace {

class FakeToolbarModelDelegate : public ToolbarModelDelegate {
public:
void SetURL(const GURL& url) { url_ = url; }

// ToolbarModelDelegate:
base::string16 FormattedStringWithEquivalentMeaning(
const GURL& url,
const base::string16& formatted_url) const override {
return formatted_url;
}

bool GetURL(GURL* url) const override {
*url = url_;
return true;
}

private:
GURL url_;
};

TEST(BraveToolbarModelImplTest,
DisplayUrlRewritesScheme) {
FakeToolbarModelDelegate delegate;
auto model = std::make_unique<BraveToolbarModelImpl>(&delegate, 1024);

delegate.SetURL(GURL("chrome://page"));

// Verify that both the full formatted URL and the display URL add the test
// suffix.
EXPECT_EQ(base::ASCIIToUTF16("brave://page"),
model->GetURLForDisplay());
}

} // namespace
11 changes: 7 additions & 4 deletions patches/chrome-browser-ui-BUILD.gn.patch
Original file line number Diff line number Diff line change
@@ -1,16 +1,19 @@
diff --git a/chrome/browser/ui/BUILD.gn b/chrome/browser/ui/BUILD.gn
index ea8b1a687eb567635a04e613b89b9a143b7c6af2..bc9ee6c3e5ac900df61a989661066039d4588da1 100644
index ea8b1a687eb567635a04e613b89b9a143b7c6af2..5e43eddc4274bfbe8d618ba5eaa0bb4648d1b9d4 100644
--- a/chrome/browser/ui/BUILD.gn
+++ b/chrome/browser/ui/BUILD.gn
@@ -1010,6 +1010,7 @@ jumbo_split_static_library("ui") {
@@ -1010,6 +1010,10 @@ jumbo_split_static_library("ui") {
"//ui/webui",
"//v8:v8_version",
]
+ deps += [ "//brave/browser/ui" ]
+ deps += [

This comment has been minimized.

Copy link
@bridiver

bridiver Oct 23, 2018

Collaborator

we should never be adding two deps like this. One dep should encapsulate everything we need for the chromium code

This comment has been minimized.

Copy link
@petemill

petemill Oct 23, 2018

Author Member

@bridiver sounds like a good aim. So it's enough to have brave/browser/ui depend on brave/components/toolbar?

+ "//brave/browser/ui",
+ "//brave/components/toolbar",
+ ]
allow_circular_includes_from +=
[ "//chrome/browser/ui/webui/bluetooth_internals" ]

@@ -2649,10 +2650,13 @@ jumbo_split_static_library("ui") {
@@ -2649,10 +2653,13 @@ jumbo_split_static_library("ui") {
]
deps += [ "//google_update" ]
} else {
Expand Down
1 change: 1 addition & 0 deletions test/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,7 @@ test("brave_unit_tests") {
"//chrome/test:test_support",
"//components/prefs",
"//components/prefs:test_support",
"//brave/components/toolbar:unit_tests",
"//components/version_info",
"//content/test:test_support",
"//components/signin/core/browser",
Expand Down

0 comments on commit 4136ab9

Please sign in to comment.