There are various things we can do to simplify this code: 1. use nsMainThreadPtrHandle for the callback, currently we are doing a fancy .swap to avoid addrefing on the wrong thread 2. then we don't need anymore to derive from a base class, it can be completely removed, that ends up simplifying some of the classes who don't need a callback 3. merge classes that only depend on another class 4. properly set classes as final 5. use fatal asserts all around 6. merge ::Start methods directly into nsFaviconService.cpp. The reason this was not done originally is that we had 2 APIs implemented in parallel, but the old API is gone, so it makes sense to move back some code. 7. Likely rename to just FaviconHelpers Since I have already done some of this work as part of bug 1265420, and since we plan to work on hi-dpi favicons, it's worth doing these cleanups now.

I'll merge the patches before landing, so please ignore commit messages. I'm also not using mozreview cause these are built on top of bug 1265420 and I'm not sure how to manage 2 different concatenated reviews in draft mode... maybe it works, not sure.

Comment on attachment 8748202 [details] [diff] [review] Part4: Use fatal assertions Review of attachment 8748202 [details] [diff] [review]: ----------------------------------------------------------------- Might be worth #defining ASSERT_MAIN_THREAD, ASSERT_NOT_MAIN_THREAD? Seems like there should be MOZ versions of those available globally but I don't think there are.

(In reply to Drew Willcoxon :adw from comment #9) > Might be worth #defining ASSERT_MAIN_THREAD, ASSERT_NOT_MAIN_THREAD? Seems > like there should be MOZ versions of those available globally but I don't > think there are. It may be nice, but the benefit is very tiny, MOZ_ASSERT(NS_IsMainThead()) is just 10 chars more than ASSERT_MAIN_THREAD.