Issue 6615790883176448: Issue 2034 - Respect chrome.webRequest.MAX_HANDLER_BEHAVIOR_CHANGED_CALLS_PER_10_MINUTES (Closed)
|
Created:
Feb. 23, 2015, 5:18 p.m. by Sebastian Noack Modified:
April 13, 2015, 10:30 a.m. Visibility:
Public. |
DescriptionIssue 2034 - Respect chrome.webRequest.MAX_HANDLER_BEHAVIOR_CHANGED_CALLS_PER_10_MINUTES
Patch Set 1 #
Total comments: 4
Patch Set 2 : Rebased and deferred handlerBehaviorChanged() until navigation occurs #
Total comments: 7
Patch Set 3 : Renamed function to propagateHandlerBehaviorChange #
Total comments: 2
Patch Set 4 : Renamed overlooked variable name #
MessagesTotal messages: 10
http://codereview.adblockplus.org/6615790883176448/diff/5629499534213120/chro... File chrome/ext/background.js (right): http://codereview.adblockplus.org/6615790883176448/diff/5629499534213120/chro... chrome/ext/background.js:354: } Is this really a good approach? I think that we should leave quota measuring up to Chrome. The goal isn't calling this function exactly the number of times that won't trigger the warning - the point is rather that it shouldn't be called too often. How about the following approach: * handlerBehaviorChanged can be called every 600 / chrome.webRequest.MAX_HANDLER_BEHAVIOR_CHANGED_CALLS_PER_10_MINUTES seconds (quotaInterval). * If handlerBehaviorChanged is called, check for an existent timeout. If there is one - ignore that call. * If there is no timeout, calculate currentInterval as the difference between current time and time of previous handlerBehaviorChanged call. * If currentInterval is larger than quotaInterval - pass the call to Chrome and remember the current time as time of previous call. * If currentInterval is smaller than quotaInterval: create a timeout to call handlerBehaviorChanged after |quotaInterval - currentInterval| seconds (clear timeout before doing that, to make sure something actually happens). This will make sure that the quota is never exceeded, and should be much simpler than the code here. More importantly, it will make sure we don't overuse CPU resources even for smaller time intervals - and refreshing will never be delayed by more than 30 seconds.
http://codereview.adblockplus.org/6615790883176448/diff/5629499534213120/chro... File chrome/ext/background.js (right): http://codereview.adblockplus.org/6615790883176448/diff/5629499534213120/chro... chrome/ext/background.js:354: } We call handlerBehaviorChanged() only if it did. That is once a day for most users. However, sometimes users perform subsequent filter changes. And if you add/remove filters you care more about seeing the effect of those change, but not how fast you can reload or navigate to a similar a page (this is all handlerBehaviorChanged is about, it is merely flushing the in-memory-per-tab cache). So this quota is stupid to have in the first place. It doesn't matter whether the in-memory cache isn't flushed for 2 or for 9 minutes. It results into buggy/unexpected behavior either way. So my strategy here is to maintain user experience as long as possible by immediately flushing the in-memory cache, until the silly quota is exceeded. So most users won't even note that there is a quota.
http://codereview.adblockplus.org/6615790883176448/diff/5629499534213120/chro... File chrome/ext/background.js (right): http://codereview.adblockplus.org/6615790883176448/diff/5629499534213120/chro... chrome/ext/background.js:354: } If users are changing filters frequently (more often than every 30 seconds), then it doesn't sound like they want to see immediate results - they are rather working off a batch of changes, and we shouldn't call handlerBehaviorChanged() after each individual change.
http://codereview.adblockplus.org/6615790883176448/diff/5629499534213120/chro... File chrome/ext/background.js (right): http://codereview.adblockplus.org/6615790883176448/diff/5629499534213120/chro... chrome/ext/background.js:354: } On 2015/02/23 19:22:42, Wladimir Palant wrote: > If users are changing filters frequently (more often than every 30 seconds), > then it doesn't sound like they want to see immediate results - they are rather > working off a batch of changes, and we shouldn't call handlerBehaviorChanged() > after each individual change. How about this: The new patch defers handlerBehaviorChanged() until onBeforeNavigate, that means until the user reloads a page or loads a new one. This way we won't call handlerBehaviorChanged() while the user might work off a batch of changes regardless how long it takes. Plus the implementation is simpler than what you and I initially suggested.
http://codereview.adblockplus.org/6615790883176448/diff/5634387206995968/chro... File chrome/ext/background.js (right): http://codereview.adblockplus.org/6615790883176448/diff/5634387206995968/chro... chrome/ext/background.js:331: function handlerBehaviorChanged() That's a confusing name. We have three things all called handlerBehaviorChanged: Chrome's API function, our API function and this private function. While the former two have similar meaning, the latter doesn't. Maybe rename it into something like propagateChange()? http://codereview.adblockplus.org/6615790883176448/diff/5634387206995968/chro... chrome/ext/background.js:342: setTimeout(function() { handlerBehaviorChangedQuota++; }, 600000); This will "recover" the quota way too slowly. I guess the timeout should be 600000/chrome.webRequest.MAX_HANDLER_BEHAVIOR_CHANGED_CALLS_PER_10_MINUTES? http://codereview.adblockplus.org/6615790883176448/diff/5634387206995968/chro... chrome/ext/background.js:356: onBeforeNavigate.addListener(handlerBehaviorChanged); My initial reaction was: "cannot be that the same listener will be added twice." Well, it can...
http://codereview.adblockplus.org/6615790883176448/diff/5634387206995968/chro... File chrome/ext/background.js (right): http://codereview.adblockplus.org/6615790883176448/diff/5634387206995968/chro... chrome/ext/background.js:331: function handlerBehaviorChanged() On 2015/04/09 18:45:02, Wladimir Palant wrote: > That's a confusing name. We have three things all called handlerBehaviorChanged: > Chrome's API function, our API function and this private function. While the > former two have similar meaning, the latter doesn't. > > Maybe rename it into something like propagateChange()? Good point. I went for propagateHandlerBehaviorChange(). http://codereview.adblockplus.org/6615790883176448/diff/5634387206995968/chro... chrome/ext/background.js:342: setTimeout(function() { handlerBehaviorChangedQuota++; }, 600000); On 2015/04/09 18:45:02, Wladimir Palant wrote: > This will "recover" the quota way too slowly. I think it is correct. This makes sure to not call handlerBehaviorChanged() more ofthen than MAX_HANDLER_BEHAVIOR_CHANGED_CALLS_PER_10_MINUTES within 10 minutes. > I guess the timeout should be 600000/chrome.webRequest.MAX_HANDLER_BEHAVIOR_CHANGED_CALLS_PER_10_MINUTES? This would recover the quota way to quickly (every 30 seconds), potentially calling handlerBehaviorChanged() way more frequently than MAX_HANDLER_BEHAVIOR_CHANGED_CALLS_PER_10_MINUTES within 10 minutes. http://codereview.adblockplus.org/6615790883176448/diff/5634387206995968/chro... chrome/ext/background.js:356: onBeforeNavigate.addListener(handlerBehaviorChanged); On 2015/04/09 18:45:02, Wladimir Palant wrote: > My initial reaction was: "cannot be that the same listener will be added twice." > Well, it can... Yep, it can. ;)
http://codereview.adblockplus.org/6615790883176448/diff/5634387206995968/chro... File chrome/ext/background.js (right): http://codereview.adblockplus.org/6615790883176448/diff/5634387206995968/chro... chrome/ext/background.js:342: setTimeout(function() { handlerBehaviorChangedQuota++; }, 600000); You are right, the current approach is correct. http://codereview.adblockplus.org/6615790883176448/diff/5718998062727168/chro... File chrome/ext/background.js (right): http://codereview.adblockplus.org/6615790883176448/diff/5718998062727168/chro... chrome/ext/background.js:338: chrome.webNavigation.onBeforeNavigate.removeListener(handlerBehaviorChanged); Feel free to change the name here as well. Please don't neglect testing your changes.
http://codereview.adblockplus.org/6615790883176448/diff/5718998062727168/chro... File chrome/ext/background.js (right): http://codereview.adblockplus.org/6615790883176448/diff/5718998062727168/chro... chrome/ext/background.js:338: chrome.webNavigation.onBeforeNavigate.removeListener(handlerBehaviorChanged); On 2015/04/10 06:58:14, Wladimir Palant wrote: > Feel free to change the name here as well. Please don't neglect testing your > changes. Done.
LGTM 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
