search

chromium / suspicious-site-reporter

Extension for reporting suspicious sites to Safe Browsing.
Apache License 2.0
95 stars 22 forks source link

Improve performance by removing unneeded work #38

Closed ericlaw1979 closed 5 years ago

ericlaw1979 commented 5 years ago
ericlaw1979 commented 5 years ago

Hrm. While this generally improves performance dramatically, it looks like we need a bit more subtlety around the onUpdated handler. If you're on a page and you refresh it, it appears that somehow the browser is setting the icon back to the default_icon (despite making no calls to the extension's setBrowserActionAndIcon function). The code in the PR fails in this specific case because changeInfo.url is falsey and thus we never call setBrowserActionAndIcon to restore the proper color.

livvielin commented 5 years ago

Hrm. While this generally improves performance dramatically, it looks like we need a bit more subtlety around the onUpdated handler. If you're on a page and you refresh it, it appears that somehow the browser is setting the icon back to the default_icon (despite making no calls to the extension's setBrowserActionAndIcon function). The code in the PR fails in this specific case because changeInfo.url is falsey and thus we never call setBrowserActionAndIcon to restore the proper color.

Oh hmm, missed that edge case there. Need to give this some thought.

livvielin commented 5 years ago

Would it be possible to store just the flag color and number of alerts on the active tab, then set the flag and badge based on these values in the case of a refresh (i.e. !changeInfo.url)?

ericlaw1979 commented 5 years ago

Broadly, speaking, yes, caching the Alert State using the tab's FQDN would improve performance, but we'd have to maintain that cache somewhere, handle expiration, etc. We might need to do that in the future if the score calculations get more expensive to compute (history check seems like it's already probably costly, chrome://histograms/History.QueryHistory), but cache maintenance has non-trivial complexity.

I think we we can get the vast majority of the performance benefit by limiting scoring updates to active tabs whose URL or loading status changes. This should resolve the refresh case even if it means that we don't eliminate all of the redundant rescoring.

livvielin commented 5 years ago

SGTM, thanks! I'll merge this change.