ContentSearch shouldn't do XHR for non-data URLs
Categories
(Firefox :: New Tab Page, defect)
Tracking
()
Tracking | Status | |
---|---|---|
firefox67 | --- | fixed |
People
(Reporter: Felipe, Assigned: Felipe)
References
Details
(Whiteboard: [fxperf:p1])
Attachments
(1 file)
(deleted),
text/x-phabricator-request
|
Details |
ContentSearch.jsm iterates through the search engines to gather all the information needed to send to the content process (list of engines, icons, etc.)
Some engines have their icons specified as data URLs. In order not to send the full data URL over the message manager, it XHRs these URLs to get an array buffer and send a blob:aaaa-bbbb-cccc
However, some icons are not data URLs, they are resource:// URLs that maps to omni.ja. But ContentSearch does the same, and that ends up XHR'ing the omni.ja in the main thread, which turns out to be really terrible [1], just to send a blob: url when it could originally send the original resource: URL.
There are other things that can be improved in ContentSearch: it gets the icons of all engines every time (including these long data urls), but the page currently only uses the icon for the active engine, now that the one-off UI has been removed from there. We should improve this too in a follow-up bug.
Assignee | ||
Comment 1•6 years ago
|
||
Forgot to put the reference link:
Comment 2•6 years ago
|
||
I believe r1cky ended up reusing contentSearch to get the icon even for handoff as reimplementing getting the icon of the current engine didn't really seem right. He also filed a bug to share implementation between activity stream and about:privatebrowsing in bug 1521758 -- not sure if there's activity there?
Updated•6 years ago
|
Comment 3•6 years ago
|
||
(In reply to Ed Lee :Mardak from comment #2)
He also filed a bug to share implementation between activity stream and about:privatebrowsing in bug 1521758 -- not sure if there's activity there?
It's on my list. I was holding off for easier uplift of the bugs that were being filed but that seems to have stabilized. Also, it's just an experiment in release for now, so maybe we should wait for the results before optimizing.
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 4•6 years ago
|
||
Assignee | ||
Comment 5•6 years ago
|
||
So I did a couple of things here:
- removed the unused uriFlag parameter from currentStateObj
- adjusted the browser_ContentSearch.js test which basically needs a copy of the original functions (/sadface) to make sure both paths are tested.
Since nothing is as simple is it looks, there was one problem here.. The .ico URIs apparently don't work with <img> tags, only as background-image in CSS. And the implementation for the one-off search buttons uses <img> (the larger current engine icon in the input box uses background-image)
I spent some time trying to convert it to be a <div> with a background-image, but it would take more CSS adjustments than what would be worth doing in this bug, considering that the in-content one-off UI will likely go away.
So I limited the performance optimization that this bug brings to the awesomebar-handoff case. If for some reason that gets backed out, we can come back to this problem and also cover the one-off UI.
Also, as Ricky mentioned in comment 3, there's a lot of opportunity for performance improvements and cleanup here once the awesomebar-handoff sticks. I'll file some other bugs to share more of what I saw that could be optimized.
Comment 7•6 years ago
|
||
bugherder |
Comment 8•6 years ago
|
||
bugherder |
Updated•5 years ago
|
Description
•