Deac Alin Reporter
	Description • 7 years ago

Steps to reproduce:
1. On an existing profile, go to about:telemetry
2. At the bottom-left, click "Raw JSON"
-> A new tab opens with the data URI for the raw JSON.
3. Open new tab and start typing in the search field 
4. Close the data URI tab

Actual Results
3. While the Data Uri tab is active/present(from step 2), input text in the search field is slower than normal (it can be observed a delay)
The issue is also present when using the backspace key to remove the text in the Search field.

Expected Results
3. While the Data Uri tab is active/present(from step 2), input text in the search field should not be affected (performance wise)

4. Close the data URI tab
Text input performance reverts to normal

Note: Check the attached screen record of the issue. 
The issue is most visible on Mac 10.x and Windows 10, less on Ubuntu.

	Deac Alin Reporter
	Comment 1 • 7 years ago

Screen-record > https://www.youtube.com/watch?v=rf5zQUV7VGI&feature=youtu.be

	Florian Quèze [:florian]
	Comment 2 • 7 years ago

The problem seems to be with the 'Switch to Tab' item that displays the full data url before adding an ellipsis in there.
A profile (https://perfht.ml/2hCy2dv) shows we are spending most of the time in _handleOverflow.

	Marco Bonardo [:mak]
	Comment 3 • 7 years ago

Apart from bug 1356532, in general it doesn't make sense to render a giant string, when we know we'll crop it regardless. We could limit the text length in any case to a meaningful amount, so that rendering and overflow calculations may be cheaper.

	:Gijs (he/him) Assignee
	Comment 4 • 6 years ago

It seems unfortunately the fix from bug 907001 was not fully effective, because the text run limit is defined on the inputbox but needs to be on the popup.

I'm looking at whether there's any further improvements we can make here.

	:Gijs (he/him) Assignee
	Comment 8 • 6 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=236e09ce9be62841d41e4839e1405cafe93241d7

	Jared Wein [:jaws] (please needinfo? me)
	Comment 9 • 6 years ago
mozreview-review

Comment on attachment 8975450 [details]
Bug 1414246 - actually use textRunsMaxLen to limit autocomplete text run length in address bar autocomplete,

https://reviewboard.mozilla.org/r/243744/#review249576

::: toolkit/content/widgets/autocomplete.xml:2018
(Diff revision 1)
>            }
>  
>            if (Array.isArray(title)) {
>              // For performance reasons we may want to limit the title size.
>              if (popup.textRunsMaxLen) {
> -              title = title.map(t => t.substr(0, popup.textRunsMaxLen));
> +              title.forEach(t => t[0] = t[0].substr(0, popup.textRunsMaxLen));

Based on https://searchfox.org/mozilla-central/rev/a85db9e29eb3f022dbaf8b9a6390ecbacf51e7dd/toolkit/content/widgets/autocomplete.xml#1690-1691 I think we should be limiting the length of each even-numbered index of the array, not just the 0-index.

	Marco Bonardo [:mak]
	Comment 10 • 6 years ago
mozreview-review

Comment on attachment 8975450 [details]
Bug 1414246 - actually use textRunsMaxLen to limit autocomplete text run length in address bar autocomplete,

https://reviewboard.mozilla.org/r/243744/#review249644

::: toolkit/content/widgets/autocomplete.xml:1949
(Diff revision 1)
>                displayUrl = input.trimValue(displayUrl);
>              displayUrl = this._unescapeUrl(displayUrl);
>            }
>            // For performance reasons we may want to limit the displayUrl size.
>            if (popup.textRunsMaxLen) {
> -            displayUrl = displayUrl.substr(0, popup.textRunsMaxLen);
> +            displayUrl = (displayUrl || "").substr(0, popup.textRunsMaxLen);

instead of substr on an empty string, you could just:
if (popup.textRunsMaxLen && displayUrl) {

::: toolkit/content/widgets/autocomplete.xml:2024
(Diff revision 1)
>              }
>              this._setUpEmphasisedSections(this._titleText, title);
>            } else {
>              // For performance reasons we may want to limit the title size.
>              if (popup.textRunsMaxLen) {
> -              title = title.substr(0, popup.textRunsMaxLen);
> +              title = (title || "").substr(0, popup.textRunsMaxLen);

ditto

	Marco Bonardo [:mak]
	Comment 11 • 6 years ago
mozreview-review

Comment on attachment 8975451 [details]
Bug 1414246 - only create page-icon URLs for some schemes (avoiding long page-icon:data URIs),

https://reviewboard.mozilla.org/r/243746/#review249648

::: toolkit/components/places/UnifiedComplete.js:308
(Diff revision 1)
>  // TODO (bug 1045924): use foreign_count once available.
>  const SQL_BOOKMARKED_URL_QUERY = urlQuery("AND bookmarked");
>  
>  const SQL_BOOKMARKED_TYPED_URL_QUERY = urlQuery("AND bookmarked AND h.typed = 1");
>  
> +const DEFAULT_FAVICON_URL = "chrome://mozapps/skin/places/defaultFavicon.svg";

Please use PlacesUtils.favicons.defaultFavicon where necessary, instead of this const. It's not an hot path so we should not even need to memoize it.

::: toolkit/components/places/UnifiedComplete.js:344
(Diff revision 1)
> +const kProtocolsWithIcons = kSchemesWithIcons.map(s => s + ":");
> +function iconHelper(url) {
> +  if (typeof url == "string") {
> +    for (let i = 0; i < kProtocolsWithIcons.length; i++) {
> +      if (url.startsWith(kProtocolsWithIcons[i])) {
> +        return "page-icon:" + url;

if (typeof url == "string") {
  return kProtocolsWithIcons.some(s => url.startsWith(s)) ?
    "page-icon:" + url : PlacesUtils.favicons.defaultFavicon;
}

some() should be more optimizeable than a bare loop

::: toolkit/components/places/UnifiedComplete.js:349
(Diff revision 1)
> +        return "page-icon:" + url;
> +      }
> +    }
> +    return DEFAULT_FAVICON_URL;
> +  }
> +  if (url && "href" in url && kProtocolsWithIcons.includes(url.protocol)) {

Is this check faster than instanceof URL?

::: toolkit/components/places/UnifiedComplete.js:352
(Diff revision 1)
> +    return DEFAULT_FAVICON_URL;
> +  }
> +  if (url && "href" in url && kProtocolsWithIcons.includes(url.protocol)) {
> +    return "page-icon:" + url.href;
> +  }
> +  if (url && "spec" in url && kSchemesWithIcons.includes(url.scheme)) {

is this check faster than instanceof Ci.nsIURI?

Fwiw, nothing seems to be passing an nsIURI, and I don't think we'll add further use of it, so maybe you could simplify the code by only providing the protocols (with ":")

	:Gijs (he/him) Assignee
	Comment 12 • 6 years ago
mozreview-review

Comment on attachment 8975451 [details]
Bug 1414246 - only create page-icon URLs for some schemes (avoiding long page-icon:data URIs),

https://reviewboard.mozilla.org/r/243746/#review249904

::: toolkit/components/places/UnifiedComplete.js:349
(Diff revision 1)
> +        return "page-icon:" + url;
> +      }
> +    }
> +    return DEFAULT_FAVICON_URL;
> +  }
> +  if (url && "href" in url && kProtocolsWithIcons.includes(url.protocol)) {

I don't know. I've made it do an instanceof check instead. I'm always a bit wary of those because of the issue of different globals, but it looks like that isn't a practical issue with these builtins, maybe because of webidl.

::: toolkit/components/places/UnifiedComplete.js:352
(Diff revision 1)
> +    return DEFAULT_FAVICON_URL;
> +  }
> +  if (url && "href" in url && kProtocolsWithIcons.includes(url.protocol)) {
> +    return "page-icon:" + url.href;
> +  }
> +  if (url && "spec" in url && kSchemesWithIcons.includes(url.scheme)) {

Removed this.

	Mozilla Autoland
	Comment 15 • 6 years ago

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 5f8a44aaf15b5c257f3510f22f7bdeaa9343b5ae -d 339ffc4c6651: rebasing 463538:5f8a44aaf15b "Bug 1414246 - actually use textRunsMaxLen to limit autocomplete text run length in address bar autocomplete, r=jaws"
merging browser/base/content/urlbarBindings.xml
merging toolkit/content/widgets/autocomplete.xml
rebasing 463539:972d63c04cee "Bug 1414246 - only create page-icon URLs for some schemes (avoiding long page-icon:data URIs), r=mak" (tip)
merging toolkit/components/places/UnifiedComplete.js
warning: conflicts while merging toolkit/components/places/UnifiedComplete.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

	Pulsebot
	Comment 18 • 6 years ago

Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/3e25e096a786
actually use textRunsMaxLen to limit autocomplete text run length in address bar autocomplete, r=jaws
https://hg.mozilla.org/integration/autoland/rev/1a5e82c2b8fb
only create page-icon URLs for some schemes (avoiding long page-icon:data URIs), r=mak

	Dorel Luca [:dluca]
	Comment 19 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/3e25e096a786
https://hg.mozilla.org/mozilla-central/rev/1a5e82c2b8fb