Hardcode custom headers #9157
Hardcode custom headers #9157
Conversation
| CreateReferralHeader(kPartnerUpholdName, kPartnerUpholdDomains)); | ||
| headers.Append( | ||
| CreateReferralHeader(kPartnerGrammarlyName, kPartnerGrammarlyDomains)); | ||
| pref_service_->Set(kReferralHeaders, headers); |
There was a problem hiding this comment.
can't we just hardcode the whole value as a string and then parse the base::Value from json in one line?
There was a problem hiding this comment.
this is a style thing, and i prefer what i have. parsing invalid JSON will not be covered by tests. at least with this we get a little more safety with compile-time type checking
There was a problem hiding this comment.
so we can remove fake header data from the test and do real checks for real hardcoded domains. This will increase safety and reduce boilerplate.
|
After looking at the code I've found out that these headers are only used in |
Minimize network requests by hardcoding the referral headers as opposed to fetching them from the network. These headers are deprecated and won't change anymore. Resolves brave/brave-browser#16455
It is always empty
* Set the referrals in a singleton. Delete all the boilerplate code in
the network delegate helper
* Simplify the tests to reflect real referrer headers we use
We can delete a lot more dead code now.
| @@ -743,8 +694,8 @@ std::string BraveReferralsService::FormatExtraHeaders( | |||
| return std::string(); | |||
|
|
|||
| const base::DictionaryValue* request_headers_dict = nullptr; | |||
| if (!GetMatchingReferralHeaders(*referral_headers_list, &request_headers_dict, | |||
| url)) | |||
| if (!BraveReferralsHeaders::GetInstance()->GetMatchingReferralHeaders( | |||
There was a problem hiding this comment.
oh, this is interesting. Do we serve any headers from /promo/initialize/nonua"? @fmarier
There was a problem hiding this comment.
good catch. no we don't (i authored the current version of laptop-updates). going to remove this too
There was a problem hiding this comment.
confirmed with @aekeus this was good to remove
| std::string GetAPIKey(); | ||
| class BraveReferralsHeaders { | ||
| public: | ||
| static BraveReferralsHeaders* GetInstance() { |
There was a problem hiding this comment.
there is really no need to write the singleton boilerplate. Just a function that internally uses a static vector (or even better, flat_map) with hardcoded values would be enough.
| domains.Append(header); | ||
| headers_dict.SetKey("domains", std::move(domains)); | ||
|
|
||
| constexpr double expiration_ms = 31536000000.0; |
| constexpr char kPartnerUpholdName[] = "uphold"; | ||
| constexpr char kPartnerGrammarlyName[] = "grammarly"; | ||
|
|
||
| referral_headers_.push_back( |
There was a problem hiding this comment.
personally, I would make global
constexpr auto kHeaders = base::MakeFixedFlatMap<base::StringPiece, base::StringPiece> {
{"eaff.com", "eaff}, {"stg.eaff.com", "eaff"} , {"uphold.com", "uphold}, ...
}
then I would iterate over keys and match the domain and then get the header value in one line (if this is everything we need).
This is no longer available server-side.
| return std::string(); | ||
|
|
||
| const base::DictionaryValue* request_headers_dict = nullptr; | ||
| if (!GetMatchingReferralHeaders(*referral_headers_list, &request_headers_dict, |
There was a problem hiding this comment.
that means we don't need GetMatchingReferralHeaders overloads?
Resolves brave/brave-browser#16455
Submitter Checklist:
QA/YesorQA/No;release-notes/includeorrelease-notes/exclude;OS/...) to the associated issuenpm run test -- brave_browser_tests,npm run test -- brave_unit_tests,npm run lint,npm run gn_check,npm run tslintgit rebase master(if needed)Reviewer Checklist:
gnAfter-merge Checklist:
changes has landed on
Test Plan: