Closed Bug 1436559 Opened 7 years ago Closed 7 years ago

setOverLink loves to do busy-work

Categories

(Firefox :: General, enhancement, P1)

Product:
Firefox
Component:
General
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: Gijs, Assigned: Gijs)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [fxperf])

Attachments

(1 file)

Bug 1436559 - stop doing busy-work in setOverLink and make textToSubURI available on Services.jsm,
59 bytes, text/x-review-board-request
florian
: review+
Details
Spotted when going through XPConnect stuff based on my favourite example, running all the urlbar/ tests, poking through callers to getService. There were 613 from browser.js:4337 (the top callsite). This code looks as follows, and gets called for onLocationChange (so every navigation): setOverLink(url, anchorElt) { const textToSubURI = Cc["@mozilla.org/intl/texttosuburi;1"]. getService(Ci.nsITextToSubURI); url = textToSubURI.unEscapeURIForUI("UTF-8", url); // Encode bidirectional formatting characters. // (RFC 3987 sections 3.2 and 4.1 paragraph 6) url = url.replace(/[\u200e\u200f\u202a\u202b\u202c\u202d\u202e]/g, encodeURIComponent); if (gURLBar && gURLBar._mayTrimURLs /* corresponds to browser.urlbar.trimURLs */) url = trimURL(url); this.overLink = url; LinkTargetDisplay.update(); }, But the onLocationChange call resets this and passes `""` for the url. There's no point doing all that work with no URL. When we do have a URI, we shouldn't continually re-fetch the reference to textToSubURI. Other consumers besides tabbrowser each have their own getter, which seems daft. Let's just add it to Services.jsm.
Comment on attachment 8949190 [details] Bug 1436559 - stop doing busy-work in setOverLink and make textToSubURI available on Services.jsm, https://reviewboard.mozilla.org/r/218578/#review224388 Looks good to me, please also cleanup https://searchfox.org/mozilla-central/rev/84cea84b12145d752e50ddca6be5462c38510e35/toolkit/content/widgets/autocomplete.xml#2003,2008-2013 ::: toolkit/components/places/UnifiedComplete.js:1752 (Diff revision 1) > // escaped URL in the action URI since that URL should be "canonical". But > // pass the pretty, unescaped URL as the match comment, since it's likely > // to be displayed to the user, and in any case the front-end should not > // rely on it being canonical. > let escapedURL = uri.displaySpec; > - let displayURL = textURIService.unEscapeURIForUI("UTF-8", uri.displaySpec); > + let displayURL = Services.textToSubURI.unEscapeURIForUI("UTF-8", uri.displaySpec); Looks like we can trivially avoid calling the displaySpec getter twice by using the escapedURL variable here.
Attachment #8949190 - Flags: review?(florian) → review+
Pushed by gijskruitbosch@gmail.com: https://hg.mozilla.org/integration/autoland/rev/46c758115832 stop doing busy-work in setOverLink and make textToSubURI available on Services.jsm, r=florian
Priority: -- → P1
Whiteboard: [fxperf]
Also please look at these toolkit - autocomplete failures: https://treeherder.mozilla.org/logviewer.html#?job_id=161135289&repo=autoland https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=161135289&filter-searchStr=b6fa2271c619b2309c35c19e063013354db37ba7 [task 2018-02-08T18:36:24.957Z] 18:36:24 INFO - TEST-UNEXPECTED-FAIL | toolkit/content/tests/chrome/test_autocomplete2.xul | uncaught exception - ReferenceError: Services is not defined at _unescapeUrl@chrome://global/content/bindings/autocomplete.xml:1781:11 [task 2018-02-08T18:36:24.958Z] 18:36:24 INFO - _adjustAcItem@chrome://global/content/bindings/autocomplete.xml:1954:26 [task 2018-02-08T18:36:24.959Z] 18:36:24 INFO - autocomplete-richlistitem_XBL_Constructor@chrome://global/content/bindings/autocomplete.xml:1390:11 [task 2018-02-08T18:36:24.960Z] 18:36:24 INFO - _appendCurrentResult@chrome://global/content/bindings/autocomplete.xml:1089:15 [task 2018-02-08T18:36:24.962Z] 18:36:24 INFO - _invalidate@chrome://global/content/bindings/autocomplete.xml:871:11 [task 2018-02-08T18:36:24.963Z] 18:36:24 INFO - _openAutocompletePopup@chrome://global/content/bindings/autocomplete.xml:824:13 [task 2018-02-08T18:36:24.969Z] 18:36:24 INFO - openAutocompletePopup@chrome://global/content/bindings/autocomplete.xml:802:11 [task 2018-02-08T18:36:24.969Z] 18:36:24 INFO - openPopup@chrome://global/content/bindings/autocomplete.xml:378:13 [task 2018-02-08T18:36:24.970Z] 18:36:24 INFO - set_popupOpen@chrome://global/content/bindings/autocomplete.xml:96:10 [task 2018-02-08T18:36:24.971Z] 18:36:24 INFO - startSearch@chrome://mochitests/content/chrome/toolkit/content/tests/chrome/test_autocomplete2.xul:70:5 [task 2018-02-08T18:36:24.971Z] 18:36:24 INFO - [task 2018-02-08T18:36:24.972Z] 18:36:24 INFO - simpletestOnerror@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:1645:11 [task 2018-02-08T18:36:24.974Z] 18:36:24 INFO - _appendCurrentResult@chrome://global/content/bindings/autocomplete.xml:1089:15 [task 2018-02-08T18:36:24.974Z] 18:36:24 INFO - _invalidate@chrome://global/content/bindings/autocomplete.xml:871:11 [task 2018-02-08T18:36:24.975Z] 18:36:24 INFO - _openAutocompletePopup@chrome://global/content/bindings/autocomplete.xml:824:13 [task 2018-02-08T18:36:24.976Z] 18:36:24 INFO - openAutocompletePopup@chrome://global/content/bindings/autocomplete.xml:802:11 [task 2018-02-08T18:36:24.977Z] 18:36:24 INFO - openPopup@chrome://global/content/bindings/autocomplete.xml:378:13 [task 2018-02-08T18:36:24.977Z] 18:36:24 INFO - set_popupOpen@chrome://global/content/bindings/autocomplete.xml:96:10 [task 2018-02-08T18:36:24.978Z] 18:36:24 INFO - startSearch@chrome://mochitests/content/chrome/toolkit/content/tests/chrome/test_autocomplete2.xul:70:5 [task 2018-02-08T18:36:24.979Z] 18:36:24 INFO - GECKO(4970) | JavaScript error: chrome://global/content/bindings/autocomplete.xml, line 1781: ReferenceError: Services is not defined [task 2018-02-08T18:36:24.980Z] 18:36:24 INFO - TEST-PASS | toolkit/content/tests/chrome/test_autocomplete2.xul | nomatch attribute shouldn't be present here [task 2018-02-08T18:36:24.981Z] 18:36:24 INFO - TEST-PASS | toolkit/content/tests/chrome/test_autocomplete2.xul | not nomatch and highlightNonMatches - should not be red [task 2018-02-08T18:36:24.982Z] 18:36:24 INFO - TEST-PASS | toolkit/content/tests/chrome/test_autocomplete2.xul | not nomatch and not highlightNonMatches - should not be red [task 2018-02-08T18:36:24.982Z] 18:36:24 INFO - TEST-PASS | toolkit/content/tests/chrome/test_autocomplete2.xul | The popup's autocompleteinput attribute is set to the ID of the textbox [task 2018-02-08T18:36:24.984Z] 18:36:24 INFO - GECKO(4970) | MEMORY STAT | vsize 20973776MB | residentFast 1006MB [task 2018-02-08T18:36:24.984Z] 18:36:24 INFO - TEST-OK | toolkit/content/tests/chrome/test_autocomplete2.xul | took 285ms [task 2018-02-08T18:36:24.985Z] 18:36:24 INFO - TEST-PASS | toolkit/content/tests/chrome/test_autocomplete2.xul | nomatch attribute not correctly set when expected [task 2018-02-08T18:36:24.985Z] 18:36:24 INFO - TEST-PASS | toolkit/content/tests/chrome/test_autocomplete2.xul | nomatch and highlightNonMatches - should be red [task 2018-02-08T18:36:24.986Z] 18:36:24 INFO - TEST-PASS | toolkit/content/tests/chrome/test_autocomplete2.xul | nomatch and not highlightNonMatches - should not be red [task 2018-02-08T18:36:24.987Z] 18:36:24 INFO - TEST-PASS | toolkit/content/tests/chrome/test_autocomplete2.xul | autocompleteinput on popup not set when closed [task 2018-02-08T18:36:24.987Z] 18:36:24 ERROR - chrome://mochitests/content/chrome/toolkit/content/tests/chrome/test_autocomplete2.xul logged result after SimpleTest.finish(): nomatch attribute not correctly set when expected [task 2018-02-08T18:36:24.988Z] 18:36:24 ERROR - chrome://mochitests/content/chrome/toolkit/content/tests/chrome/test_autocomplete2.xul logged result after SimpleTest.finish(): nomatch and highlightNonMatches - should be red [task 2018-02-08T18:36:24.989Z] 18:36:24 ERROR - chrome://mochitests/content/chrome/toolkit/content/tests/chrome/test_autocomplete2.xul logged result after SimpleTest.finish(): nomatch and not highlightNonMatches - should not be red [task 2018-02-08T18:36:24.990Z] 18:36:24 ERROR - chrome://mochitests/content/chrome/toolkit/content/tests/chrome/test_autocomplete2.xul logged result after SimpleTest.finish(): autocompleteinput on popup not set when closed [task 2018-02-08T18:36:24.990Z] 18:36:24 INFO - TEST-START | toolkit/content/tests/chrome/test_autocomplete3.xul [task 2018-02-08T18:36:24.991Z] 18:36:24 INFO - Not taking screenshot here: see the one that was previously logged [task 2018-02-08T18:36:24.993Z] 18:36:24 INFO - TEST-UNEXPECTED-FAIL | toolkit/content/tests/chrome/test_autocomplete3.xul | uncaught exception - NS_ERROR_FACTORY_EXISTS: Component returned failure code: 0xc1f30100 (NS_ERROR_FACTORY_EXISTS) [nsIComponentRegistrar.registerFactory] at @chrome://mochitests/content/chrome/toolkit/content/tests/chrome/test_autocomplete3.xul:78:1 [task 2018-02-08T18:36:24.993Z] 18:36:24 INFO -
Do we still need unEscapeURIForUI at all, by the way?
(In reply to Masatoshi Kimura [:emk] from comment #7) > Do we still need unEscapeURIForUI at all, by the way? How would we find out? Should we just be using uri.displaySpec, or something? (This should probably be a separate follow-up bug anyway, but even so...)
Flags: needinfo?(valentin.gosu)
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(VYV03354)
(In reply to :Gijs from comment #8) > (In reply to Masatoshi Kimura [:emk] from comment #7) > > Do we still need unEscapeURIForUI at all, by the way? > > How would we find out? Should we just be using uri.displaySpec, or something? Unfortunately, .displaySpec is insufficient at the moment. It does not unescape the path part. But I think we can just use decodeURIComponent. > (This should probably be a separate follow-up bug anyway, but even so...) I concur.
Flags: needinfo?(VYV03354)
Pushed by gijskruitbosch@gmail.com: https://hg.mozilla.org/integration/autoland/rev/0c39c734b419 stop doing busy-work in setOverLink and make textToSubURI available on Services.jsm, r=florian
Blocks: 1437082
Filed bug 1437082 about removing/replacing use of unEscapeURIForUI
Flags: needinfo?(valentin.gosu)
https://hg.mozilla.org/mozilla-central/rev/0c39c734b419
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox60: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Depends on: 1471811
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: