Closed Bug 1529380 Opened 6 months ago Closed 6 months ago

ContentSearch shouldn't do XHR for non-data URLs

Categories

(Firefox :: New Tab Page, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 67
Tracking Status
firefox67 --- fixed

People

(Reporter: Felipe, Assigned: Felipe)

References

Details

(Whiteboard: [fxperf:p1])

Attachments

(1 file)

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.

Forgot to put the reference link:

[1] https://perfht.ml/2IEzGeD

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?

Flags: needinfo?(rrosario)
See Also: → 1521758

(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.

Flags: needinfo?(rrosario)
Component: Search → Activity Streams: Newtab

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.

Pushed by fgomes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a09dd4b000f7
ContentSearch shouldn't do XHR for non-data URLs. r=r1cky
Blocks: 1522877
Status: ASSIGNED → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 67
Depends on: 1530499
Depends on: 1530767
No longer depends on: 1530499
Component: Activity Streams: Newtab → New Tab Page
You need to log in before you can comment on or make changes to this bug.