Issue 5410618009976832: issue #690 - don't consider special subscriptions when complaining about high subscription count (Closed)
|
Created:
July 26, 2014, 7:07 p.m. by saroyanm Modified:
July 30, 2014, 11:01 a.m. Reviewers:
Wladimir Palant Visibility:
Public. |
DescriptionThis issue is related to current ticket:
https://issues.adblockplus.org/ticket/690
Patch Set 1 : #
Total comments: 5
Patch Set 2 : #
Total comments: 4
Patch Set 3 : #MessagesTotal messages: 7
Wladimir can you please have a look when you have time. Looks to be a quick one. http://codereview.adblockplus.org/5410618009976832/diff/5639274879778816/chro... File chrome/content/ui/sendReport.js (right): http://codereview.adblockplus.org/5410618009976832/diff/5639274879778816/chro... chrome/content/ui/sendReport.js:953: for each (let {url} in FilterStorage.subscriptions.filter(this.subscriptionFilter)) Not sure if we need separate generalSubscriptionFilter, for not to iterate, but I guess we can be okey with this solution.
http://codereview.adblockplus.org/5410618009976832/diff/5639274879778816/chro... File chrome/content/ui/sendReport.js (right): http://codereview.adblockplus.org/5410618009976832/diff/5639274879778816/chro... chrome/content/ui/sendReport.js:882: numGeneralSubscriptions: 0, I see no point renaming this property, your new name doesn't explain any more than the old one did. http://codereview.adblockplus.org/5410618009976832/diff/5639274879778816/chro... chrome/content/ui/sendReport.js:953: for each (let {url} in FilterStorage.subscriptions.filter(this.subscriptionFilter)) On 2014/07/26 19:52:07, saroyanm wrote: > Not sure if we need separate generalSubscriptionFilter, for not to iterate, but > I guess we can be okey with this solution. Please simply extend subscriptionFilter - this function is only used in issuesDataSource and nowhere else. Note that it is already different from subscriptionsDataSource.subscriptionFilter because the requirements are different here.
On 2014/07/28 09:17:18, Wladimir Palant wrote: > http://codereview.adblockplus.org/5410618009976832/diff/5639274879778816/chro... > File chrome/content/ui/sendReport.js (right): > > http://codereview.adblockplus.org/5410618009976832/diff/5639274879778816/chro... > chrome/content/ui/sendReport.js:882: numGeneralSubscriptions: 0, > I see no point renaming this property, your new name doesn't explain any more > than the old one did. > > http://codereview.adblockplus.org/5410618009976832/diff/5639274879778816/chro... > chrome/content/ui/sendReport.js:953: for each (let {url} in > FilterStorage.subscriptions.filter(this.subscriptionFilter)) > On 2014/07/26 19:52:07, saroyanm wrote: > > Not sure if we need separate generalSubscriptionFilter, for not to iterate, > but > > I guess we can be okey with this solution. > > Please simply extend subscriptionFilter - this function is only used in > issuesDataSource and nowhere else. Note that it is already different from > subscriptionsDataSource.subscriptionFilter because the requirements are > different here. Updated.
updated http://codereview.adblockplus.org/5410618009976832/diff/5639274879778816/chro... File chrome/content/ui/sendReport.js (right): http://codereview.adblockplus.org/5410618009976832/diff/5639274879778816/chro... chrome/content/ui/sendReport.js:882: numGeneralSubscriptions: 0, On 2014/07/28 09:17:18, Wladimir Palant wrote: > I see no point renaming this property, your new name doesn't explain any more > than the old one did. Done. http://codereview.adblockplus.org/5410618009976832/diff/5639274879778816/chro... chrome/content/ui/sendReport.js:953: for each (let {url} in FilterStorage.subscriptions.filter(this.subscriptionFilter)) On 2014/07/28 09:17:18, Wladimir Palant wrote: > On 2014/07/26 19:52:07, saroyanm wrote: > > Not sure if we need separate generalSubscriptionFilter, for not to iterate, > but > > I guess we can be okey with this solution. > > Please simply extend subscriptionFilter - this function is only used in > issuesDataSource and nowhere else. Seams like the issuesDataSource.subscriptionFilter also used by subscriptionUpdateDataSource, so I've created subscriptionUpdateDataSource.subscriptionFilter, to also check whether all subscripptions updated or not, including "Anti adblock notifications" and "whitelist".
A few style issues below but seems fine otherwise. http://codereview.adblockplus.org/5410618009976832/diff/5657382461898752/chro... File chrome/content/ui/sendReport.js (right): http://codereview.adblockplus.org/5410618009976832/diff/5657382461898752/chro... chrome/content/ui/sendReport.js:755: let subscriptions = FilterStorage.subscriptions.filter(subscriptionUpdateDataSource.subscriptionFilter); this.subscriptionFilter please. http://codereview.adblockplus.org/5410618009976832/diff/5657382461898752/chro... chrome/content/ui/sendReport.js:896: && s.url != Prefs.subscriptions_antiadblockurl) Please try to keep the lines shorter than 80 characters. In this particular case it means having each part of the condition on a separate line. Also, we typically leave the operator (&& here) before the line break. Finally, please add brackets around the body of the if statement here, otherwise with multiline conditions it's not immediately obvious where the body starts.
Updated. http://codereview.adblockplus.org/5410618009976832/diff/5657382461898752/chro... File chrome/content/ui/sendReport.js (right): http://codereview.adblockplus.org/5410618009976832/diff/5657382461898752/chro... chrome/content/ui/sendReport.js:755: let subscriptions = FilterStorage.subscriptions.filter(subscriptionUpdateDataSource.subscriptionFilter); On 2014/07/30 05:44:35, Wladimir Palant wrote: > this.subscriptionFilter please. Oops. http://codereview.adblockplus.org/5410618009976832/diff/5657382461898752/chro... chrome/content/ui/sendReport.js:896: && s.url != Prefs.subscriptions_antiadblockurl) On 2014/07/30 05:44:35, Wladimir Palant wrote: > Please try to keep the lines shorter than 80 characters. In this particular case > it means having each part of the condition on a separate line. Also, we > typically leave the operator (&& here) before the line break. Finally, please > add brackets around the body of the if statement here, otherwise with multiline > conditions it's not immediately obvious where the body starts. Thanks, done.
LGTM 
|
