Issue #430 - Filter hit counters aren't cleared

This issue is related to current ticket:
https://issues.adblockplus.org/ticket/430

[saroyanm, 2014-06-09 11:28:02]

Wladimir can you please have a look, should be quick one.
Thanks.

http://codereview.adblockplus.org/5373553692114944/diff/5629499534213120/lib/...
File lib/filterStorage.js (right):

http://codereview.adblockplus.org/5373553692114944/diff/5629499534213120/lib/...
lib/filterStorage.js:358: let {FilterListener} = require("filterListener");
Called FilterListener module inside of the function for exports.FilterListener
to be already set.

[Wladimir Palant, 2014-06-12 11:58:38]

http://codereview.adblockplus.org/5373553692114944/diff/5629499534213120/lib/...
File lib/filterStorage.js (right):

http://codereview.adblockplus.org/5373553692114944/diff/5629499534213120/lib/...
lib/filterStorage.js:358: let {FilterListener} = require("filterListener");
On 2014/06/09 11:28:02, saroyanm wrote:
> Called FilterListener module inside of the function for exports.FilterListener
> to be already set.

Actually, hardcoding this logic here doesn't seem too great. FilterStorage is a
generic component, it makes sense that it never uses FilterListener explicitly.
In fact, this logic should be implementable in FilterListener itself, in
onFilterChange:

  if (action == "hitCount" && newValue == 0)
  {
    // Filter hits are being reset, make sure these changes are saved.
    FilterListener.setDirty(0);
  }
  else if (action == "hitCount" || action == "lastHit")
    ...

This will allow some cleanups:

* FilterListener object should no longer be exported, it is an implementation
detail.
* ui.js no longer has to call FilterListener.setDirty(0) explicitly.

[saroyanm, 2014-06-12 16:56:02]

Hope we can give a go with last patch.

http://codereview.adblockplus.org/5373553692114944/diff/5629499534213120/lib/...
File lib/filterStorage.js (right):

http://codereview.adblockplus.org/5373553692114944/diff/5629499534213120/lib/...
lib/filterStorage.js:358: let {FilterListener} = require("filterListener");
On 2014/06/12 11:58:38, Wladimir Palant wrote:
> On 2014/06/09 11:28:02, saroyanm wrote:
> > Called FilterListener module inside of the function for
exports.FilterListener
> > to be already set.
> 
> Actually, hardcoding this logic here doesn't seem too great. FilterStorage is
a
> generic component, it makes sense that it never uses FilterListener
explicitly.
> In fact, this logic should be implementable in FilterListener itself, in
> onFilterChange:
> 
>   if (action == "hitCount" && newValue == 0)
>   {
>     // Filter hits are being reset, make sure these changes are saved.
>     FilterListener.setDirty(0);
>   }
>   else if (action == "hitCount" || action == "lastHit")
>     ...
> 
> This will allow some cleanups:
> 
> * FilterListener object should no longer be exported, it is an implementation
> detail.
> * ui.js no longer has to call FilterListener.setDirty(0) explicitly.

Thanks for idea. 
Done.

[Wladimir Palant, 2014-06-12 18:09:16]

LGTM

[saroyanm, 2014-06-13 09:13:44]

On 2014/06/12 18:09:16, Wladimir Palant wrote:
> LGTM

Deleted the patch by mistake, but have local changes. will commit, now..