Remove AQS and some other parameters from search URLs #731

uazo · 2020-09-06T12:11:42Z

fixes #726

I left the necessary parameters for ~~autocomplete~~ suggestions and image search

csagan5 · 2020-09-06T14:30:20Z

build/patches/Remove-some-params-from-search-url.patch

+     if (!optional)
+       url->insert(start, kDefaultCount);
+   } else if (parameter == "google:assistedQueryStats") {
+-    replacements->push_back(Replacement(GOOGLE_ASSISTED_QUERY_STATS, start));


Will this make sure that {google:assistedQueryStats} disappears from the URL?

Edit: I checked, answer is yes.

csagan5 · 2020-09-06T14:30:40Z

build/patches/Remove-some-params-from-search-url.patch

+   } 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));


This should be removed as well.

it's related to omnibox suggestions.
just uncheck the settings

Which settings would these be?

csagan5 · 2020-09-06T14:31:23Z

build/patches/Remove-some-params-from-search-url.patch

+ #if defined(OS_ANDROID) || defined(OS_IOS)
+-    url->insert(start, "sourceid=chrome-mobile&");
+ #else
+-    url->insert(start, "sourceid=chrome&");


I would keep this one e.g. same as Desktop search.

uazo · 2020-09-06T17:39:14Z

please don't merge. I have got a crash writing "chrome:/"

Abort message: '[FATAL:autocomplete_match.cc(1333)] Check failed: i->offset > last_offset (7 vs. 16) Classification for "chrome://version/" with offset of 7 is unsorted in relation to last offset of 16. Provider: HistoryURL.'

uazo · 2020-09-08T05:37:20Z

Some additional info.
I found the same behavior in bromite:

that value is strange.
activating the flag "Prevent line autocomplete":

it seems more correct to me.
I'm trying to figure out where the bug is

csagan5 · 2020-09-10T19:44:05Z

Some additional info.
I found the same behavior in bromite:

What page is that?

uazo · 2020-09-11T15:35:04Z

What page is that?

chrome://omnibox/

uazo · 2020-09-14T07:27:47Z

I found the cause is "Disable omission of URL elements in Omnibox" patch.
https://github.com/bromite/bromite/blob/master/build/patches/Disable-omission-of-URL-elements-in-Omnibox.patch
it would probably be better not to change those flags, but change the caller.
do you remember where it is used?

EDIT:
https://source.chromium.org/chromium/chromium/src/+/master:components/url_formatter/android/java/src/org/chromium/components/url_formatter/UrlFormatter.java
it would not be better to edit directly that?

csagan5 · 2020-09-14T20:30:17Z

it would not be better to edit directly that?

If you look for each of those constants (kFormatUrlOmitHTTP etc), they are used in many places; editing that file will not solve this.

I found the cause is "Disable omission of URL elements in Omnibox" patch.

In which way? Is it because of a specific flag?

uazo · 2020-09-19T12:04:25Z

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

diff --git a/components/url_formatter/android/java/src/org/chromium/components/url_formatter/UrlFormatter.java b/components/url_formatter/android/java/src/org/chromium/components/url_formatter/UrlFormatter.java
index 3ad37094fc..37ba76c43a 100644
--- a/components/url_formatter/android/java/src/org/chromium/components/url_formatter/UrlFormatter.java
+++ b/components/url_formatter/android/java/src/org/chromium/components/url_formatter/UrlFormatter.java
@@ -50,7 +50,7 @@ public final class UrlFormatter {
      * @return Formatted URL.
      */
     public static String formatUrlForDisplayOmitScheme(String uri) {
-        return UrlFormatterJni.get().formatUrlForDisplayOmitScheme(uri);
+        return UrlFormatterJni.get().formatUrlForCopy(uri);
     }

     /**
@@ -70,7 +70,7 @@ public final class UrlFormatter {
      * @return Formatted URL.
      */
     public static String formatUrlForDisplayOmitHTTPScheme(String uri) {
-        return UrlFormatterJni.get().formatUrlForDisplayOmitHTTPScheme(uri);
+        return UrlFormatterJni.get().formatUrlForCopy(uri);
     }

     /**
@@ -90,7 +90,7 @@ public final class UrlFormatter {
      * @return Formatted URL.
      */
     public static String formatUrlForDisplayOmitSchemeOmitTrivialSubdomains(String uri) {
-        return UrlFormatterJni.get().formatUrlForDisplayOmitSchemeOmitTrivialSubdomains(uri);
+        return UrlFormatterJni.get().formatUrlForCopy(uri);
     }
     /**
      * Builds a String representation of <code>uri</code> suitable for display to the user,
@@ -108,7 +108,7 @@ public final class UrlFormatter {
      * @return Formatted URL.
      */
     public static String formatUrlForDisplayOmitUsernamePassword(String uri) {
-        return UrlFormatterJni.get().formatUrlForDisplayOmitUsernamePassword(uri);
+        return UrlFormatterJni.get().formatUrlForCopy(uri);
     }

     /**
@@ -131,7 +131,7 @@ public final class UrlFormatter {
      *         it fails to parse it.
      */
     public static String formatUrlForSecurityDisplay(String uri) {
-        return UrlFormatterJni.get().formatStringUrlForSecurityDisplay(uri, SchemeDisplay.SHOW);
+        return UrlFormatterJni.get().formatUrlForCopy(uri);
     }

     /**
@@ -143,7 +143,7 @@ public final class UrlFormatter {
      */
     public static String formatUrlForSecurityDisplay(GURL url, @SchemeDisplay int schemeDisplay) {
         if (url == null) return "";
-        return UrlFormatterJni.get().formatUrlForSecurityDisplay(url, schemeDisplay);
+        return UrlFormatterJni.get().formatUrlForCopy(url.getSpec());
     }

     /**
@@ -153,7 +153,7 @@ public final class UrlFormatter {
      */
     @Deprecated
     public static String formatUrlForSecurityDisplay(String uri, @SchemeDisplay int schemeDisplay) {
-        return UrlFormatterJni.get().formatStringUrlForSecurityDisplay(uri, schemeDisplay);
+        return UrlFormatterJni.get().formatUrlForCopy(uri);
     }

     @VisibleForTesting

everything works perfectly.

csagan5 · 2020-09-19T14:58:11Z

If you look for each of those constants (kFormatUrlOmitHTTP etc), they are used in many places; editing that file will not solve this.

See my previous comment.

uazo · 2021-01-30T16:34:45Z

If you look for each of those constants (kFormatUrlOmitHTTP etc), they are used in many places; editing that file will not solve this.

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.
maybe we may think of giving the possibility to edit by the user?

csagan5 · 2021-04-14T21:18:44Z

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.
maybe we may think of giving the possibility to edit by the user?

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.

uazo · 2021-06-24T15:30:44Z

If you look for each of those constants (kFormatUrlOmitHTTP etc), they are used in many places

I'm sorry to come back to this, but I think you should rethink about your position.
for example, this code expects to have only that value in domain, while in bromite, being kFormatUrlOmitTrivialSubdomains zero, the whole url would return, bringing bugs not easily fixed or worse security problems.

std::vector<std::string> GetMatchingDomains(
    const std::vector<password_manager::MatchingReusedCredential>&
        matching_reused_credentials) {
  std::vector<std::string> matching_domains;
  matching_domains.reserve(matching_reused_credentials.size());
  for (const auto& credential : matching_reused_credentials) {
    // This only works for Web credentials. For Android credentials, there needs
    // to be special handing and should use affiliation information instead of
    // the signon_realm.
    std::string domain = base::UTF16ToUTF8(url_formatter::FormatUrl(
        GURL(credential.signon_realm),
        url_formatter::kFormatUrlOmitDefaults |
            url_formatter::kFormatUrlOmitHTTPS |
            url_formatter::kFormatUrlOmitTrivialSubdomains |
            url_formatter::kFormatUrlTrimAfterHost,
        net::UnescapeRule::SPACES, nullptr, nullptr, nullptr));
    matching_domains.push_back(std::move(domain));
  }
  return base::flat_set<std::string>(std::move(matching_domains)).extract();
}

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.

What I am asking is that merging the PR does not make URL elisions re-appear somewhere in the browser

Please allow me to edit the Disable-omission-of-URL-elements-in-Omnibox.patch, if, subsequently, elided urls emerge we will fix them.

csagan5 · 2021-06-24T20:45:08Z

bringing bugs not easily fixed or worse security problems.

This patch has been used for long in Bromite and I do not recall of bugs related to it.

if, subsequently, elided urls emerge we will fix them.

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.

uazo · 2021-06-25T07:36:53Z

This patch has been used for long in Bromite and I do not recall of bugs related to it.

bugs maybe not (although in my experience, many users never complain) but dchecks failed a bit.

This is an approach of the type "let the users suffer the bugs and report them", and I do not agree with it.

and neither do I in principle.
however I think that modification is too invasive and incorrect. I'd rather have a small UI bug than a big hidden one

So the patch could be updated accordingly, but I am afraid it is covering omissions that were introduced also before that experiment.

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 kOmniboxUIHideSteadyStateUrlTrivialSubdomainsName or kOmniboxUIHideSteadyStateUrlSchemeName you will see that, at that time, it was easy to go back to the code that disables the elide, but now it is no longer so, and in my opinion simply because the developers used, for something else, those flags that previously had been done only for displaying.

it's a correct principle, I make my team follow it too, if it's possible (and make sense) not to duplicate code.

remove aqs and some other parameters from search urls

bdf169a

csagan5 changed the title ~~remove aqs and some other parameters from search urls~~ Remove AQS and some other parameters from search URLs Sep 6, 2020

csagan5 reviewed Sep 6, 2020

View reviewed changes

apply suggestions

9662da1

uazo mentioned this pull request Dec 3, 2021

Always ask for permissions #1606

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove AQS and some other parameters from search URLs #731

Remove AQS and some other parameters from search URLs #731

uazo commented Sep 6, 2020 •

edited

csagan5 Sep 6, 2020 •

edited

csagan5 Sep 6, 2020

uazo Sep 6, 2020

csagan5 Nov 5, 2020

csagan5 Sep 6, 2020

uazo commented Sep 6, 2020 •

edited

uazo commented Sep 8, 2020

csagan5 commented Sep 10, 2020

uazo commented Sep 11, 2020

uazo commented Sep 14, 2020 •

edited

csagan5 commented Sep 14, 2020

uazo commented Sep 19, 2020

csagan5 commented Sep 19, 2020

uazo commented Jan 30, 2021

csagan5 commented Apr 14, 2021

uazo commented Jun 24, 2021

csagan5 commented Jun 24, 2021

uazo commented Jun 25, 2021

Remove AQS and some other parameters from search URLs #731

Are you sure you want to change the base?

Remove AQS and some other parameters from search URLs #731

Conversation

uazo commented Sep 6, 2020 • edited

csagan5 Sep 6, 2020 • edited

Choose a reason for hiding this comment

csagan5 Sep 6, 2020

Choose a reason for hiding this comment

uazo Sep 6, 2020

Choose a reason for hiding this comment

csagan5 Nov 5, 2020

Choose a reason for hiding this comment

csagan5 Sep 6, 2020

Choose a reason for hiding this comment

uazo commented Sep 6, 2020 • edited

uazo commented Sep 8, 2020

csagan5 commented Sep 10, 2020

uazo commented Sep 11, 2020

uazo commented Sep 14, 2020 • edited

csagan5 commented Sep 14, 2020

uazo commented Sep 19, 2020

csagan5 commented Sep 19, 2020

uazo commented Jan 30, 2021

csagan5 commented Apr 14, 2021

uazo commented Jun 24, 2021

csagan5 commented Jun 24, 2021

uazo commented Jun 25, 2021

uazo commented Sep 6, 2020 •

edited

csagan5 Sep 6, 2020 •

edited

uazo commented Sep 6, 2020 •

edited

uazo commented Sep 14, 2020 •

edited