-
Notifications
You must be signed in to change notification settings - Fork 895
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
Remove YouTube and Instagram share ID parameters from URLs #21255
Conversation
35b6a30
to
2ff0b1b
Compare
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.
Would it be safer to limit too www. ? would it need to apply to m.youtube.com, music.youtube.com, tv.youtube.com ?
2ff0b1b
to
8e83798
Compare
It automatically includes subdomains. I've added a comment to that effect in the PR. |
8e83798
to
0513712
Compare
0513712
to
bb8f07e
Compare
bb8f07e
to
05a9a51
Compare
88cc8de
to
cc1aa76
Compare
// https://example.com/index.php and from http://www.example.com/ for | ||
// example. | ||
static const auto kScopedQueryStringTrackers = | ||
std::map<std::string_view, std::vector<std::string_view>>({ |
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.
probably with std::array
we can keep the whole thing constexpr
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 tried to do this, but I wasn't able to make it work as a constexpr
with an std::array
because the size of the array is variable.
cc1aa76
to
5e2bab6
Compare
The ABP list format also separates domains using commas.
Use a vector instead of a comma-separated string since this is a short list and it doesn't have to be optimized prematurely.
5e2bab6
to
3016a59
Compare
Fixes brave/brave-browser#34719 and brave/brave-browser#35094
Submitter Checklist:
QA/Yes
orQA/No
;release-notes/include
orrelease-notes/exclude
;OS/...
) to the associated issuenpm run test -- brave_browser_tests
,npm run test -- brave_unit_tests
wikinpm run lint
,npm run presubmit
wiki,npm run gn_check
,npm run tslint
git rebase master
(if needed)Reviewer Checklist:
gn
After-merge Checklist:
changes has landed on
Test Plan:
200 OK
request (ignore the200 Internal Redirect
request) does not include thesi
parameter.igsh
parameter is only present inInternal Redirect
requests.Here's what it looks like before the change in this PR:
and here's the new expected result as of this PR:
The
youtu.be
URL will now show a303 Internal Redirect
instead of a303 See Other
:Note: this test will only work the first time. You need to delete the profile or clear all data before testing this otherwise service workers will interfere with the requests.