Issue 29550598: Issue 5735 - Use JS Map instead of Object for Filter.knownFilters (Closed)
|
Created:
Sept. 20, 2017, 8:20 a.m. by sergei Modified:
Sept. 26, 2017, 1:58 p.m. CC:
Felix Dahlke Base URL:
https://github.com/adblockplus/adblockpluscore.git Visibility:
Public. |
Patch Set 1 #
Total comments: 4
Patch Set 2 : address comments #
Total comments: 7
Patch Set 3 : address comments #
Total comments: 1
MessagesTotal messages: 11
https://codereview.adblockplus.org/29550598/diff/29550599/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29550598/diff/29550599/lib/filterClasses.j... lib/filterClasses.js:112: return Filter.knownFilters.get(text); It makes little sense to have two calls here. You can call .get() directly and return if you get a result. https://codereview.adblockplus.org/29550598/diff/29550599/lib/filterStorage.js File lib/filterStorage.js (right): https://codereview.adblockplus.org/29550598/diff/29550599/lib/filterStorage.j... lib/filterStorage.js:340: { Nit: You can remove the brackets around this block.
https://codereview.adblockplus.org/29550598/diff/29550599/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29550598/diff/29550599/lib/filterClasses.j... lib/filterClasses.js:112: return Filter.knownFilters.get(text); On 2017/09/20 08:59:10, Wladimir Palant wrote: > It makes little sense to have two calls here. You can call .get() directly and > return if you get a result. I also thought about it because there are no `undefined` values stored in this object, but decided to leave it as it was before. Fixed. https://codereview.adblockplus.org/29550598/diff/29550599/lib/filterStorage.js File lib/filterStorage.js (right): https://codereview.adblockplus.org/29550598/diff/29550599/lib/filterStorage.j... lib/filterStorage.js:340: { On 2017/09/20 08:59:10, Wladimir Palant wrote: > Nit: You can remove the brackets around this block. Done.
https://codereview.adblockplus.org/29550598/diff/29550640/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29550598/diff/29550640/lib/filterClasses.j... lib/filterClasses.js:111: let ret = Filter.knownFilters.get(text); Nit: maybe a more meaningful name for this variable, such as existingFilter?
https://codereview.adblockplus.org/29550598/diff/29550640/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29550598/diff/29550640/lib/filterClasses.j... lib/filterClasses.js:111: let ret = Filter.knownFilters.get(text); On 2017/09/20 10:04:13, Wladimir Palant wrote: > Nit: maybe a more meaningful name for this variable, such as existingFilter? I think it's not for this change because `ret` is used here and in the method `Filter.fromObject` below, looks already like a pattern and it would be better to rename them at the same time, for example to `filter`. In addition later this variable is re-used for a new filter and one variable is actually sufficient here. Is it worth creation of the Noissue review with the renaming to `filter`?
LGTM https://codereview.adblockplus.org/29550598/diff/29550640/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29550598/diff/29550640/lib/filterClasses.j... lib/filterClasses.js:111: let ret = Filter.knownFilters.get(text); On 2017/09/20 10:27:33, sergei wrote: > On 2017/09/20 10:04:13, Wladimir Palant wrote: > > Nit: maybe a more meaningful name for this variable, such as existingFilter? > > I think it's not for this change because `ret` is used here and in the method > `Filter.fromObject` below, I see.
https://codereview.adblockplus.org/29550598/diff/29550640/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29550598/diff/29550640/lib/filterClasses.j... lib/filterClasses.js:82: * @type {Object} Actually, this needs to be adjusted - type should be Map.<string,Filter>
https://codereview.adblockplus.org/29550598/diff/29550640/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29550598/diff/29550640/lib/filterClasses.j... lib/filterClasses.js:82: * @type {Object} On 2017/09/20 11:00:27, Wladimir Palant wrote: > Actually, this needs to be adjusted - type should be Map.<string,Filter> Done.
LGTM
LGTM https://codereview.adblockplus.org/29550598/diff/29550640/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29550598/diff/29550640/lib/filterClasses.j... lib/filterClasses.js:111: let ret = Filter.knownFilters.get(text); On 2017/09/20 10:27:33, sergei wrote: > Is it worth creation of the Noissue review with the renaming to > `filter`? Yea I think so, if you don't want to just give it a better name now. https://codereview.adblockplus.org/29550598/diff/29550659/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29550598/diff/29550659/lib/filterClasses.j... lib/filterClasses.js:112: if (ret) Just an observation since I don't think it matters, but this behaviour is slightly different. If we cached a falsey value for a filter previously we would have returned it but now we won't.
https://codereview.adblockplus.org/29550598/diff/29550640/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29550598/diff/29550640/lib/filterClasses.j... lib/filterClasses.js:111: let ret = Filter.knownFilters.get(text); On 2017/09/20 12:30:24, kzar wrote: > On 2017/09/20 10:27:33, sergei wrote: > > Is it worth creation of the Noissue review with the renaming to > > `filter`? > > Yea I think so, if you don't want to just give it a better name now. Done in https://codereview.adblockplus.org/29550667/ 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
