-
Notifications
You must be signed in to change notification settings - Fork 872
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
- Loading branch information
Showing
11 changed files
with
448 additions
and
23 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
31 changes: 15 additions & 16 deletions
31
components/omnibox/browser/brave_autocomplete_controller.cc
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,27 +1,26 @@ | ||
/* This Source Code Form is subject to the terms of the Mozilla Public | ||
/* Copyright (c) 2020 The Brave Authors. All rights reserved. | ||
* 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/omnibox/browser/brave_autocomplete_controller.h" | ||
#include "brave/components/omnibox/browser/topsites_provider.h" | ||
|
||
#include <memory> | ||
#include <utility> | ||
|
||
#include "brave/components/omnibox/browser/suggested_sites_provider.h" | ||
#include "brave/components/omnibox/browser/topsites_provider.h" | ||
|
||
BraveAutocompleteController::BraveAutocompleteController( | ||
std::unique_ptr<AutocompleteProviderClient> provider_client, | ||
AutocompleteControllerDelegate* delegate, | ||
int provider_types) : | ||
AutocompleteController( | ||
std::move(provider_client), | ||
delegate, | ||
provider_types) | ||
{ | ||
if (provider_types & AutocompleteProvider::TYPE_SEARCH) { | ||
providers_.push_back(new TopSitesProvider(provider_client_.get())); | ||
} | ||
} | ||
|
||
BraveAutocompleteController::~BraveAutocompleteController() { | ||
int provider_types) : AutocompleteController(std::move(provider_client), | ||
delegate, provider_types) { | ||
if (provider_types & AutocompleteProvider::TYPE_SEARCH) { | ||
providers_.push_back(new TopSitesProvider(provider_client_.get())); | ||
providers_.push_back(new SuggestedSitesProvider(provider_client_.get())); | ||
} | ||
} | ||
|
||
} | ||
BraveAutocompleteController::~BraveAutocompleteController() { | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
/* Copyright (c) 2020 The Brave Authors. All rights reserved. | ||
* 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/omnibox/browser/suggested_sites_match.h" | ||
|
||
// This is the provider for Brave Suggested Sites | ||
SuggestedSitesMatch::SuggestedSitesMatch(const GURL& destination_url, | ||
const GURL& stripped_destination_url, const base::string16& display, | ||
const bool allow_default) : | ||
destination_url_(destination_url), | ||
stripped_destination_url_(stripped_destination_url), | ||
display_(display), allow_default_(allow_default) { | ||
} | ||
|
||
SuggestedSitesMatch::SuggestedSitesMatch(const SuggestedSitesMatch& other) { | ||
destination_url_ = other.destination_url_; | ||
stripped_destination_url_ = other.stripped_destination_url_; | ||
display_ = other.display_; | ||
allow_default_ = other.allow_default_; | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
/* Copyright (c) 2020 The Brave Authors. All rights reserved. | ||
* 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_OMNIBOX_BROWSER_SUGGESTED_SITES_MATCH_H_ | ||
#define BRAVE_COMPONENTS_OMNIBOX_BROWSER_SUGGESTED_SITES_MATCH_H_ | ||
|
||
#include "base/strings/string16.h" | ||
#include "url/gurl.h" | ||
|
||
// This is the provider for Brave Suggested Sites | ||
class SuggestedSitesMatch { | ||
public: | ||
SuggestedSitesMatch(const SuggestedSitesMatch& other); | ||
SuggestedSitesMatch(const GURL& destination_url, | ||
const GURL& stripped_destination_url, | ||
const base::string16& display, | ||
const bool allow_default); | ||
GURL destination_url_; | ||
GURL stripped_destination_url_; | ||
base::string16 display_; | ||
bool allow_default_; | ||
}; | ||
|
||
#endif // BRAVE_COMPONENTS_OMNIBOX_BROWSER_SUGGESTED_SITES_MATCH_H_ |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,110 @@ | ||
/* Copyright (c) 2020 The Brave Authors. All rights reserved. | ||
* 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/omnibox/browser/suggested_sites_provider.h" | ||
|
||
#include <algorithm> | ||
#include <utility> | ||
|
||
#include "base/strings/utf_string_conversions.h" | ||
#include "components/omnibox/browser/autocomplete_input.h" | ||
|
||
// As from autocomplete_provider.h: | ||
// Search Secondary Provider (suggestion) | 100++ | ||
const int SuggestedSitesProvider::kRelevance = 100; | ||
|
||
|
||
SuggestedSitesProvider::SuggestedSitesProvider( | ||
AutocompleteProviderClient* client) | ||
: AutocompleteProvider(AutocompleteProvider::TYPE_SEARCH) { | ||
} | ||
|
||
void SuggestedSitesProvider::Start(const AutocompleteInput& input, | ||
bool minimal_changes) { | ||
matches_.clear(); | ||
if (input.from_omnibox_focus() || | ||
(input.type() == metrics::OmniboxInputType::EMPTY) || | ||
(input.type() == metrics::OmniboxInputType::QUERY)) { | ||
return; | ||
} | ||
|
||
const std::string input_text = | ||
base::ToLowerASCII(base::UTF16ToUTF8(input.text())); | ||
auto check_add_match = | ||
[&](const std::pair<std::string, SuggestedSitesMatch>& pair) { | ||
const std::string& current_site = pair.first; | ||
const SuggestedSitesMatch& match = pair.second; | ||
// Don't bother matching until 4 chars, or less if it's an exact match | ||
if (input_text.length() < 4 && | ||
current_site.length() != input_text.length()) { | ||
return; | ||
} | ||
size_t foundPos = current_site.find(input_text); | ||
// We'd normally check for npos here but we want only people that | ||
// really want these suggestions. Example don't suggest bitcoin and | ||
// litecoin for just a coin search. | ||
if (foundPos == 0) { | ||
ACMatchClassifications styles = | ||
StylesForSingleMatch(input_text, | ||
base::UTF16ToASCII(match.display_)); | ||
AddMatch(base::ASCIIToUTF16(current_site), match, styles); | ||
if (match.allow_default_ && | ||
current_site.length() == input_text.length()) { | ||
// It's guaranteed that matches_ has at least 1 item | ||
// here because of the previous AddMatch call. | ||
size_t last_index = matches_.size() - 1; | ||
matches_[last_index].SetAllowedToBeDefault(input); | ||
// As from autocomplete_provider.h: | ||
// Search Primary Provider (what you typed) | 1300 | ||
matches_[last_index].relevance = 1301; | ||
} | ||
} | ||
}; | ||
|
||
std::for_each(suggested_sites_.begin(), suggested_sites_.end(), | ||
check_add_match); | ||
} | ||
|
||
SuggestedSitesProvider::~SuggestedSitesProvider() {} | ||
|
||
// static | ||
ACMatchClassifications SuggestedSitesProvider::StylesForSingleMatch( | ||
const std::string &input_text, | ||
const std::string &site) { | ||
ACMatchClassifications styles; | ||
size_t foundPos = site.find(input_text); | ||
if (std::string::npos == foundPos) { | ||
styles.push_back(ACMatchClassification(0, ACMatchClassification::NONE)); | ||
} else if (foundPos == 0) { | ||
styles.push_back(ACMatchClassification( | ||
0, ACMatchClassification::URL | ACMatchClassification::MATCH)); | ||
if (site.length() > input_text.length()) { | ||
styles.push_back(ACMatchClassification(input_text.length(), | ||
ACMatchClassification::URL)); | ||
} | ||
} else { | ||
styles.push_back(ACMatchClassification(0, ACMatchClassification::URL)); | ||
styles.push_back(ACMatchClassification( | ||
foundPos, ACMatchClassification::URL | ACMatchClassification::MATCH)); | ||
if (site.length() > foundPos + input_text.length()) { | ||
styles.push_back( | ||
ACMatchClassification(foundPos + input_text.length(), 0)); | ||
} | ||
} | ||
return styles; | ||
} | ||
|
||
void SuggestedSitesProvider::AddMatch(const base::string16& match_string, | ||
const SuggestedSitesMatch& data, | ||
const ACMatchClassifications& styles) { | ||
AutocompleteMatch match(this, kRelevance + matches_.size(), false, | ||
AutocompleteMatchType::NAVSUGGEST); | ||
match.fill_into_edit = data.display_; | ||
match.destination_url = data.destination_url_; | ||
match.contents = data.display_; | ||
match.contents_class = styles; | ||
match.stripped_destination_url = data.stripped_destination_url_; | ||
matches_.push_back(match); | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,46 @@ | ||
/* Copyright (c) 2020 The Brave Authors. All rights reserved. | ||
* 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_OMNIBOX_BROWSER_SUGGESTED_SITES_PROVIDER_H_ | ||
#define BRAVE_COMPONENTS_OMNIBOX_BROWSER_SUGGESTED_SITES_PROVIDER_H_ | ||
|
||
#include <map> | ||
#include <string> | ||
|
||
#include "base/compiler_specific.h" | ||
#include "base/macros.h" | ||
#include "base/strings/string16.h" | ||
#include "brave/components/omnibox/browser/suggested_sites_match.h" | ||
#include "components/omnibox/browser/autocomplete_match.h" | ||
#include "components/omnibox/browser/autocomplete_provider.h" | ||
|
||
class AutocompleteProviderClient; | ||
|
||
// This is the provider for Brave Suggested Sites | ||
class SuggestedSitesProvider : public AutocompleteProvider { | ||
public: | ||
explicit SuggestedSitesProvider(AutocompleteProviderClient* client); | ||
|
||
// AutocompleteProvider: | ||
void Start(const AutocompleteInput& input, bool minimal_changes) override; | ||
|
||
private: | ||
~SuggestedSitesProvider() override; | ||
static std::map<std::string, SuggestedSitesMatch> suggested_sites_; | ||
|
||
static const int kRelevance; | ||
|
||
void AddMatch(const base::string16& match_string, | ||
const SuggestedSitesMatch& match, | ||
const ACMatchClassifications& styles); | ||
|
||
static ACMatchClassifications StylesForSingleMatch( | ||
const std::string &input_text, | ||
const std::string &site); | ||
|
||
DISALLOW_COPY_AND_ASSIGN(SuggestedSitesProvider); | ||
}; | ||
|
||
#endif // BRAVE_COMPONENTS_OMNIBOX_BROWSER_SUGGESTED_SITES_PROVIDER_H_ |
Oops, something went wrong.
e8fdde7
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might not want to use your reference links...
e8fdde7
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Revert please
e8fdde7
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WTH is this @brave-dev? That's dodgy as hell. Revert these changes if you want to keep your users.
e8fdde7
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change is now disabled in source via:
#5761
Basically, this functionality is behind a flag which you can disable:
With the fix above, the
Show Brave suggested sites in autocomplete suggestions
setting will be defaulted to false. Existing users that haven't modified the setting will have it turned off with our next release (planned for Thurs June 11). We're also considering a hotfix that would be released before thatI've started a Nightly build and we'll also have this fix uplifted to Beta soon (with a build following the merge there)
e8fdde7
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They've lose my trust as a user from this. If they were willing to consider/review and merge this change in the first place then they're clearly not as ethical as I would have hoped and now I know they're willing to exploit their user base. People are obviously free to make up their own minds but that's my 2c.
e8fdde7
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just remove it. Don't want it buried in a flag. remove it.
e8fdde7
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's removed now, will be deployed to Release channel soon.
But a couple points from my perspective...
This is a URL bar suggestion entry. It is not much different from every browser which puts a ref code in search results. But in the case of searches, there's nothing in the UI that tells you the search query you will enter will have a ref code. In this case there is UI that shows you.
I agree we should not autocomplete the result though.
e8fdde7
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So you think there are no financial incentives involved when browsers set their client ids as a search query ? That's funny.
e8fdde7
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the feedback guys.
I don't claim we're free from misjudgments from time to time but we 1) work in the open and 2) listen to and value user feedback.
Could I dig deeper to get more feedback? I'd like to know if the problem is the suggestions themselves (I don't think so) or more precisely that we made a misjudgement with a default match to one of those suggestions.
This is the block of code that I think rubs people the wrong way
https://github.com/brave/brave-core/blob/master/components/omnibox/browser/suggested_sites_provider.cc#L64
Or more precisely we could set
allow_default_
tofalse
for all entries in the last param here https://github.com/brave/brave-core/blob/master/components/omnibox/browser/suggested_sites_provider_data.cce8fdde7
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes absolutely the suggestions are a problem. Not just because they are there, but, Crypto-Currency trading? That really is the seamy, deep-dark underbelly of the Internet.
There is no special case. CryptoCurrency trading != reputable.
e8fdde7
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the reply,
The suggestions and specifically the added reference id are both problems.
Here are some of my initial thoughts and questions of such a change.
"Brave is on a mission to fix the web by giving users a safer, faster and better browsing experience while growing support for content creators through a new attention-based ecosystem of rewards." - https://brave.com/about
I'd be interested to hear your thoughts on the above.
e8fdde7
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://twitter.com/BrendanEich/status/1269844174385233920
e8fdde7
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned in that thread, there's a difference between adding a browser referrer when using the browser's built in search bar and modifying a url that a user types in to append your own personal referral code.
EDIT: To clarify "personal" - Not implying this is a single person's id being used.
EDIT2: Read further in the topic that this id is just to say it's a Brave browser client and not an account referral. This is definitely better than my initial thought. I do think there is still problem that a customer typed in url is modified though.
e8fdde7
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is very disappointing.
The whole idea was to make no profit from tying a user behavior to the browsing habits.
This fundamental design idea was broken.
Apart from this you make profit and don't give the users a fair share.
e8fdde7
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the feedback.
So I put this change in which will make it so that if the suggestions are on it will never match user input for one of those suggestions:
#5771
That being said it's still currently off by default from a different pull request.
I'm also checking to see if we can have a better vanity referral code because I can understand that
ref=39346846
makes it look like tracking or like a personal code more thanref=brave
even know there is no tracking entropy exposed with either.Even know the matches did show
ref=39346846
, we're also discussing internally about making those suggestions stand out differently in the UI even more. Separately we're also considering having an option for this in on-boarding as well.e8fdde7
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bbondy Why these sites in particular? Why was the decision made to include referral sites for cryptocurrency websites and what benefit would it be to brave?