search

clarkbw / searchspot

Firefox add-on for better searching. Multiple search suggestions from a number of different providers. Local search based on GeoLocation. Better search engine preferences management.
13 stars 5 forks source link

fetchImageDataSync needs some cleanup #22

Closed clarkbw closed 12 years ago

clarkbw commented 12 years ago

The introduction of fetchImageDataSync is problematic and not really unnecessary. It looks like you are doing the sync retrieval inside an async XHR request handler which in turn sends an asynchronous notification, so it doesn't appear like you are really saving any control flow complexity. Since this is triggered by the search engine collector which will operate during normal user browsing, this represents an unnecessary risk in terms of nested event loop spinning and any potential jank. I'm not sure if there are nuanced reasons for the choice, but if there are, just using an XHR to get the data to a local in-memory buffer could be good.

NB: I'm talking about the code in this commit: 8bde0871d41879951a3b2da73283e4c0611b57eb

clarkbw commented 12 years ago

Everything has been switched over to async retrieval of the image data such that now the control flow looks like this:

fetch XML
  parse XML
  fetch IMAGE
    emit(engine)

I continued to use the NetUtils package which I could stop using and move to an XHR call however I would still have to do all the same image data mechanics after the XHR returns. If using NetUtils seems like a bad choice then it's certainly an option; I should be able to get the ContentType from an XHR call so there isn't much difference there.