-
-
Notifications
You must be signed in to change notification settings - Fork 370
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 AQS and some other parameters from search URLs #731
base: master
Are you sure you want to change the base?
Conversation
if (!optional) | ||
url->insert(start, kDefaultCount); | ||
} else if (parameter == "google:assistedQueryStats") { | ||
- replacements->push_back(Replacement(GOOGLE_ASSISTED_QUERY_STATS, start)); |
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.
Will this make sure that {google:assistedQueryStats}
disappears from the URL?
Edit: I checked, answer is yes.
} else if (parameter == "google:currentPageUrl") { | ||
- replacements->push_back(Replacement(GOOGLE_CURRENT_PAGE_URL, start)); | ||
} else if (parameter == "google:cursorPosition") { | ||
replacements->push_back(Replacement(GOOGLE_CURSOR_POSITION, start)); |
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 should be removed as well.
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 related to omnibox suggestions.
just uncheck the settings
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.
Which settings would these be?
#if defined(OS_ANDROID) || defined(OS_IOS) | ||
- url->insert(start, "sourceid=chrome-mobile&"); | ||
#else | ||
- url->insert(start, "sourceid=chrome&"); |
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 would keep this one e.g. same as Desktop search.
please don't merge. I have got a crash writing "chrome:/"
|
What page is that? |
chrome://omnibox/ |
I found the cause is "Disable omission of URL elements in Omnibox" patch. EDIT: |
If you look for each of those constants (
In which way? Is it because of a specific flag? |
Sorry, I didn't have time to look at it. however i tried that by putting the flags back and applying this
everything works perfectly. |
See my previous comment. |
what do you think if we close this pr? what you ask me is impossible... the flag is used everywhere in the code, understanding what changes is a big problem. |
What I am asking is that merging the PR does not make URL elisions re-appear somewhere in the browser. If it is possible then we can change the current patch, otherwise not because we would loose that feature. |
I'm sorry to come back to this, but I think you should rethink about your position.
note, this is just an example, but code that relies on those flags for things that have nothing to do with display there is all over the place.
Please allow me to edit the |
This patch has been used for long in Bromite and I do not recall of bugs related to it.
This is an approach of the type "let the users suffer the bugs and report them", and I do not agree with it. The simplified domain display has been abandoned: https://bugs.chromium.org/p/chromium/issues/detail?id=1090393#c75 So the patch could be updated accordingly, but I am afraid it is covering omissions that were introduced also before that experiment. |
bugs maybe not (although in my experience, many users never complain) but
and neither do I in principle.
before writing to you I checked in the history of cr. if you look in https://chromium.googlesource.com/chromium/src/+blame/refs/heads/main/chrome/browser/flag_descriptions.cc for it's a correct principle, I make my team follow it too, if it's possible (and make sense) not to duplicate code. |
fixes #726
I left the necessary parameters for
autocompletesuggestions and image search