Issue 29363607: Issue 4612 - enable AA on first run and make automatic adding of any subscription optional (Closed)
Patch Set 1 #
Total comments: 17
Patch Set 2 : address comments #Patch Set 3 : address comments #
Total comments: 26
Patch Set 4 : address comments #Patch Set 5 : reduce number of attempts to remove test files #
MessagesTotal messages: 14
Didn't check the test code yet, but looks good all in all. Some comments. https://codereview.adblockplus.org/29363607/diff/29363608/lib/init.js File lib/init.js (right): https://codereview.adblockplus.org/29363607/diff/29363608/lib/init.js#newcode56 lib/init.js:56: // Choose default subscription and add it I guess we don't need that comment anymore if the pref name is self explanatory. https://codereview.adblockplus.org/29363607/diff/29363608/lib/init.js#newcode59 lib/init.js:59: console.log(node.url); Do we want this to stay in here? https://codereview.adblockplus.org/29363607/diff/29363608/lib/init.js#newcode78 lib/init.js:78: Nit: Why add an empty line here? https://codereview.adblockplus.org/29363607/diff/29363608/lib/prefs.js File lib/prefs.js (right): https://codereview.adblockplus.org/29363607/diff/29363608/lib/prefs.js#newcode35 lib/prefs.js:35: first_run_enable_acceptable_ads: true, Why not move this down, so it's right next to the other one? https://codereview.adblockplus.org/29363607/diff/29363608/lib/prefs.js#newcode48 lib/prefs.js:48: first_run_enable_current_locale_subscription: true, Hm, I think the name could be improved. How about `first_run_enable_ad_blocking`? I guess _how_ we enable ad blocking doesn't really matter here, because it's obvious that we need to add some list for that. https://codereview.adblockplus.org/29363607/diff/29363608/lib/prefs.js#newcode53 lib/prefs.js:53: "first_run_enable_acceptable_ads", "first_run_enable_current_locale_subscription"]; Back when we discussed what should be preconfigurable in our products, those two things weren' on that list, so let's not make it preconfigurable.
https://codereview.adblockplus.org/29363607/diff/29363608/lib/init.js File lib/init.js (right): https://codereview.adblockplus.org/29363607/diff/29363608/lib/init.js#newcode56 lib/init.js:56: // Choose default subscription and add it On 2016/11/21 11:41:11, Felix Dahlke wrote: > I guess we don't need that comment anymore if the pref name is self explanatory. Agree. https://codereview.adblockplus.org/29363607/diff/29363608/lib/init.js#newcode59 lib/init.js:59: console.log(node.url); On 2016/11/21 11:41:11, Felix Dahlke wrote: > Do we want this to stay in here? Sorry, forgot to remove after debugging. https://codereview.adblockplus.org/29363607/diff/29363608/lib/init.js#newcode78 lib/init.js:78: On 2016/11/21 11:41:11, Felix Dahlke wrote: > Nit: Why add an empty line here? Done. https://codereview.adblockplus.org/29363607/diff/29363608/lib/prefs.js File lib/prefs.js (right): https://codereview.adblockplus.org/29363607/diff/29363608/lib/prefs.js#newcode35 lib/prefs.js:35: first_run_enable_acceptable_ads: true, On 2016/11/21 11:41:11, Felix Dahlke wrote: > Why not move this down, so it's right next to the other one? I wanted to keep it close to subscriptions_exceptionsurl because they both belong to AA functionality. Should it be moved down? https://codereview.adblockplus.org/29363607/diff/29363608/lib/prefs.js#newcode48 lib/prefs.js:48: first_run_enable_current_locale_subscription: true, On 2016/11/21 11:41:11, Felix Dahlke wrote: > Hm, I think the name could be improved. How about > `first_run_enable_ad_blocking`? I guess _how_ we enable ad blocking doesn't > really matter here, because it's obvious that we need to add some list for that. I think that "enable_ad_blocking" is confusing because it implies that there is a way to disable ad blocking at all which is not correct because here it is about choosing of the initial subscription. What about first_run_enable_auto_choosing_subscription? https://codereview.adblockplus.org/29363607/diff/29363608/lib/prefs.js#newcode53 lib/prefs.js:53: "first_run_enable_acceptable_ads", "first_run_enable_current_locale_subscription"]; On 2016/11/21 11:41:11, Felix Dahlke wrote: > Back when we discussed what should be preconfigurable in our products, those two > things weren' on that list, so let's not make it preconfigurable. Could you please remind where it had been discussed, just wonder about the reasons that it cannot be here? It seems a bit strange to have some restrictions here because the user of libadblockplus can always do whatever he wants either by direct modifying of the code or by injecting a custom implementation of FileSystem which serves a desired prefs.json when it does not exist. I would say that absence of documentation of preconfigurable values in readme is already an indication that they are not supposed to be publicly used, isn't it enough? Secondly, it actually was one of the ideas of the issue, first_run_enable_acceptable_ads and first_run_enable_current_locale_subscription are introduced only to have such functionality optional, so one can easily disable it in particular tests. If we remove them from preconfigurable then it will only complicate the code of tests but anyone can still do it.
https://codereview.adblockplus.org/29363607/diff/29363608/lib/prefs.js File lib/prefs.js (right): https://codereview.adblockplus.org/29363607/diff/29363608/lib/prefs.js#newcode48 lib/prefs.js:48: first_run_enable_current_locale_subscription: true, On 2016/11/21 14:03:20, sergei wrote: > On 2016/11/21 11:41:11, Felix Dahlke wrote: > > Hm, I think the name could be improved. How about > > `first_run_enable_ad_blocking`? I guess _how_ we enable ad blocking doesn't > > really matter here, because it's obvious that we need to add some list for > that. > > I think that "enable_ad_blocking" is confusing because it implies that there is > a way to disable ad blocking at all which is not correct because here it is > about choosing of the initial subscription. What about > first_run_enable_auto_choosing_subscription? Or maybe first_run_enable_subscription_auto_choosing?
https://codereview.adblockplus.org/29363607/diff/29363608/lib/prefs.js File lib/prefs.js (right): https://codereview.adblockplus.org/29363607/diff/29363608/lib/prefs.js#newcode35 lib/prefs.js:35: first_run_enable_acceptable_ads: true, On 2016/11/21 14:03:20, sergei wrote: > On 2016/11/21 11:41:11, Felix Dahlke wrote: > > Why not move this down, so it's right next to the other one? > > I wanted to keep it close to subscriptions_exceptionsurl because they both > belong to AA functionality. Should it be moved down? IMO yes, but you could also move the other thing up I guess. https://codereview.adblockplus.org/29363607/diff/29363608/lib/prefs.js#newcode48 lib/prefs.js:48: first_run_enable_current_locale_subscription: true, On 2016/11/21 14:03:20, sergei wrote: > On 2016/11/21 11:41:11, Felix Dahlke wrote: > > Hm, I think the name could be improved. How about > > `first_run_enable_ad_blocking`? I guess _how_ we enable ad blocking doesn't > > really matter here, because it's obvious that we need to add some list for > that. > > I think that "enable_ad_blocking" is confusing because it implies that there is > a way to disable ad blocking at all which is not correct because here it is > about choosing of the initial subscription. What about > first_run_enable_auto_choosing_subscription? There is a way to disable ad blocking - this pref :) If we don't auto select an ad blocking filter list, there is no ad blocking, at least from our perspective. So that name makes most sense to me. But then again, I'm wondering if we really want to split these prefs up. There's no point in automatically adding the AA filter list, since that one works only in tandem with the ad blocking subscriptions we auto select. So we could combine it into one pref along the lines of `first_run_subscription_auto_select`, or maybe in the same vein as the other options rather `enable_subscription_auto_selection`. https://codereview.adblockplus.org/29363607/diff/29363608/lib/prefs.js#newcode53 lib/prefs.js:53: "first_run_enable_acceptable_ads", "first_run_enable_current_locale_subscription"]; On 2016/11/21 14:03:20, sergei wrote: > On 2016/11/21 11:41:11, Felix Dahlke wrote: > > Back when we discussed what should be preconfigurable in our products, those > two > > things weren' on that list, so let's not make it preconfigurable. > > Could you please remind where it had been discussed, just wonder about the > reasons that it cannot be here? It seems a bit strange to have some restrictions > here because the user of libadblockplus can always do whatever he wants either > by direct modifying of the code or by injecting a custom implementation of > FileSystem which serves a desired prefs.json when it does not exist. > > I would say that absence of documentation of preconfigurable values in readme is > already an indication that they are not supposed to be publicly used, isn't it > enough? > > Secondly, it actually was one of the ideas of the issue, > first_run_enable_acceptable_ads and first_run_enable_current_locale_subscription > are introduced only to have such functionality optional, so one can easily > disable it in particular tests. If we remove them from preconfigurable then it > will only complicate the code of tests but anyone can still do it. My bad, I forgot how that stuff works :P Yeah of course, if those aren't preconfigurable, that defeats the purpose.
https://codereview.adblockplus.org/29363607/diff/29363608/lib/prefs.js File lib/prefs.js (right): https://codereview.adblockplus.org/29363607/diff/29363608/lib/prefs.js#newcode48 lib/prefs.js:48: first_run_enable_current_locale_subscription: true, On 2016/11/22 11:45:08, Felix Dahlke wrote: > On 2016/11/21 14:03:20, sergei wrote: > > On 2016/11/21 11:41:11, Felix Dahlke wrote: > > > Hm, I think the name could be improved. How about > > > `first_run_enable_ad_blocking`? I guess _how_ we enable ad blocking doesn't > > > really matter here, because it's obvious that we need to add some list for > > that. > > > > I think that "enable_ad_blocking" is confusing because it implies that there > is > > a way to disable ad blocking at all which is not correct because here it is > > about choosing of the initial subscription. What about > > first_run_enable_auto_choosing_subscription? > > There is a way to disable ad blocking - this pref :) If we don't auto select an > ad blocking filter list, there is no ad blocking, at least from our perspective. > So that name makes most sense to me. No, if these pref values are false there is still ad blocking which is based on manually added filters, that's how I thought to have tests for FilterEngine::Matches. > But then again, I'm wondering if we really want to split these prefs up. There's > no point in automatically adding the AA filter list, since that one works only > in tandem with the ad blocking subscriptions we auto select. So we could combine > it into one pref along the lines of `first_run_subscription_auto_select`, or > maybe in the same vein as the other options rather > `enable_subscription_auto_selection`. Right, I guess, currently there is no real demand to have them separate, so it seems to be better to have only one controlling parameter for both of them. I'm going to use 'first_run_subscription_auto_select'.
updated
Looks good, just a bunch of nits from looking at the test code. https://codereview.adblockplus.org/29363607/diff/29363751/lib/init.js File lib/init.js (right): https://codereview.adblockplus.org/29363607/diff/29363751/lib/init.js#newcode61 lib/init.js:61: FilterStorage.addSubscription(subscription); Nit: Seems we do this after setting the subscription properties in ABP for Chrome. I guess we don't have to do it the same way, but it makes a bit more sense to me that way: First create the subscription object, then set its properties, then add it to FilterStorage, then synchronize it. https://codereview.adblockplus.org/29363607/diff/29363751/lib/init.js#newcode68 lib/init.js:68: let aaSubscription = Subscription.fromURL(Prefs.subscriptions_exceptionsurl); Nit: Mind adding an empty line above this to separate ad blocking and AA here? Would make it a bit easier to read IMO. https://codereview.adblockplus.org/29363607/diff/29363751/test/FilterEngine.cpp File test/FilterEngine.cpp (right): https://codereview.adblockplus.org/29363607/diff/29363751/test/FilterEngine.c... test/FilterEngine.cpp:21: using namespace AdblockPlus; Nit: Empty line after this to be in line with the other files? https://codereview.adblockplus.org/29363607/diff/29363751/test/FilterEngine.c... test/FilterEngine.cpp:119: FileSystemPtr m_fileSystem; From https://adblockplus.org/coding-style: "No hungarian notation, no special variable name prefixes or suffixes denoting type or scope." https://codereview.adblockplus.org/29363607/diff/29363751/test/FilterEngine.c... test/FilterEngine.cpp:125: // Since there are neither in memory FS nor functionality to work with Wording nit: "Since there 'is' neither 'in-memory' FS nor functionality to work with directories',' [...]" https://codereview.adblockplus.org/29363607/diff/29363751/test/FilterEngine.c... test/FilterEngine.cpp:126: // directories use the hack: manually clean the directory. Do you mean "functionality to work with _a different directory_", maybe? https://codereview.adblockplus.org/29363607/diff/29363751/test/FilterEngine.c... test/FilterEngine.cpp:132: auto jsEngine = JsEngine::New(appInfo); I guess my C++ might be getting rusty, but why assign this to a temporary local variable here, rather than directly to the member variable? https://codereview.adblockplus.org/29363607/diff/29363751/test/FilterEngine.c... test/FilterEngine.cpp:149: // writing into files. Otherwise we get Permission denied. Wording nit: Maybe put "Permission denied" in quotes, or use a lower-case "p"? https://codereview.adblockplus.org/29363607/diff/29363751/test/FilterEngine.c... test/FilterEngine.cpp:151: while (i-- > 0 && m_jsEngine.lock()) { Nit: Opening brace on its own line please (might as well skip the braces here of course since there's just one statement in the body). https://codereview.adblockplus.org/29363607/diff/29363751/test/FilterEngine.c... test/FilterEngine.cpp:605: const auto aaUrl = filterEngine->GetPref("subscriptions_exceptionsurl")->AsString(); Hm, maybe we should hard code the URL as well here, same as with the Chinese subscription? No strong opinion, up to you. https://codereview.adblockplus.org/29363607/diff/29363751/test/FilterEngine.c... test/FilterEngine.cpp:612: } else if (subscriptions[1]->GetProperty("url")->AsString() == aaUrl) Nit: Closing brace on its own line please.
https://codereview.adblockplus.org/29363607/diff/29363751/lib/init.js File lib/init.js (right): https://codereview.adblockplus.org/29363607/diff/29363751/lib/init.js#newcode61 lib/init.js:61: FilterStorage.addSubscription(subscription); On 2016/11/22 17:37:16, Felix Dahlke wrote: > Nit: Seems we do this after setting the subscription properties in ABP for > Chrome. I guess we don't have to do it the same way, but it makes a bit more > sense to me that way: First create the subscription object, then set its > properties, then add it to FilterStorage, then synchronize it. Completely agree, fixed. https://codereview.adblockplus.org/29363607/diff/29363751/lib/init.js#newcode68 lib/init.js:68: let aaSubscription = Subscription.fromURL(Prefs.subscriptions_exceptionsurl); On 2016/11/22 17:37:16, Felix Dahlke wrote: > Nit: Mind adding an empty line above this to separate ad blocking and AA here? > Would make it a bit easier to read IMO. Done. https://codereview.adblockplus.org/29363607/diff/29363751/test/FilterEngine.cpp File test/FilterEngine.cpp (right): https://codereview.adblockplus.org/29363607/diff/29363751/test/FilterEngine.c... test/FilterEngine.cpp:21: using namespace AdblockPlus; On 2016/11/22 17:37:16, Felix Dahlke wrote: > Nit: Empty line after this to be in line with the other files? Done. https://codereview.adblockplus.org/29363607/diff/29363751/test/FilterEngine.c... test/FilterEngine.cpp:119: FileSystemPtr m_fileSystem; On 2016/11/22 17:37:17, Felix Dahlke wrote: > From https://adblockplus.org/coding-style: "No hungarian notation, no special > variable name prefixes or suffixes denoting type or scope." I thought we finally started to use #pragma once and prefixes for members of classes in C++. https://codereview.adblockplus.org/29333474/diff/29345688/compiled/String.h This two things give a lot of help. https://codereview.adblockplus.org/29363607/diff/29363751/test/FilterEngine.c... test/FilterEngine.cpp:125: // Since there are neither in memory FS nor functionality to work with On 2016/11/22 17:37:16, Felix Dahlke wrote: > Wording nit: "Since there 'is' neither 'in-memory' FS nor functionality to work > with directories',' [...]" Done. https://codereview.adblockplus.org/29363607/diff/29363751/test/FilterEngine.c... test/FilterEngine.cpp:126: // directories use the hack: manually clean the directory. On 2016/11/22 17:37:16, Felix Dahlke wrote: > Do you mean "functionality to work with _a different directory_", maybe? No, it's possible to configure working directory of DefaultFileSystem, I mean a cross platform API to work with directories, for instance it would be useful to have a scoped temporary directory or at least some functions to get a path to temporary writable directory, create and remove directories and in addition generate a temporary folder name. I have not added it here because I'm sure it should be in another commit. https://codereview.adblockplus.org/29363607/diff/29363751/test/FilterEngine.c... test/FilterEngine.cpp:132: auto jsEngine = JsEngine::New(appInfo); On 2016/11/22 17:37:16, Felix Dahlke wrote: > I guess my C++ might be getting rusty, but why assign this to a temporary local > variable here, rather than directly to the member variable? JsEngine::New returns std::shared_ptr but the type of member is std::weak_ptr, it's a part of the hack with sleep_for used in removeFileIfExists. It is to deal with the following race condition. Currently we have issues with circular references https://issues.adblockplus.org/ticket/3594 and with detached threads https://issues.adblockplus.org/ticket/3595. Current version at master allows different tests to influence another tests, for example, when the next test is running, writing thread from a previous test can corrupt, e.g. patterns.ini. Eliminating of circular references relieves a bit but not completely. Asynchronous operation will be ignored if there is already no JsEngine, however there is still a race condition, thread of asynchronous operation still can keep JsEngine alive even when we have already no strong references to it. sleep_for gives an asynchronous operation some time to finish its work, so afterwards we can remove files created by current test. I have complicated the code even more :) because it should take into account not only jsEngine of current test but also async operations from previous tests (actually it happens too often on my machine that patterns.ini is blocked by some previous test). All these hacks will go away as the life span of asynchronous operations is no longer than the life span of both JsEngine and FilterEngine. https://codereview.adblockplus.org/29363607/diff/29363751/test/FilterEngine.c... test/FilterEngine.cpp:149: // writing into files. Otherwise we get Permission denied. On 2016/11/22 17:37:16, Felix Dahlke wrote: > Wording nit: Maybe put "Permission denied" in quotes, or use a lower-case "p"? Done. https://codereview.adblockplus.org/29363607/diff/29363751/test/FilterEngine.c... test/FilterEngine.cpp:151: while (i-- > 0 && m_jsEngine.lock()) { On 2016/11/22 17:37:16, Felix Dahlke wrote: > Nit: Opening brace on its own line please (might as well skip the braces here of > course since there's just one statement in the body). Done. https://codereview.adblockplus.org/29363607/diff/29363751/test/FilterEngine.c... test/FilterEngine.cpp:605: const auto aaUrl = filterEngine->GetPref("subscriptions_exceptionsurl")->AsString(); On 2016/11/22 17:37:16, Felix Dahlke wrote: > Hm, maybe we should hard code the URL as well here, same as with the Chinese > subscription? No strong opinion, up to you. I will remove aaUrl and use Subscription::IsAA instead in an upcoming commit. https://codereview.adblockplus.org/29363607/diff/29363751/test/FilterEngine.c... test/FilterEngine.cpp:612: } else if (subscriptions[1]->GetProperty("url")->AsString() == aaUrl) On 2016/11/22 17:37:16, Felix Dahlke wrote: > Nit: Closing brace on its own line please. Done.
Could you please review this issue? Since it blocks #4604, what do you think about for present committing only js part and committing tests after landing of https://codereview.adblockplus.org/29361562/?
JS part LGTM. The tests look somewhat hacky indeed, and since we are still not sure about https://codereview.adblockplus.org/29361562/ I'd postpone the tests for now.
Sorry, forgot to comment here, thanks for the reminder. LGTM! My gut feeling would be to land the tests as-is and then improve them once the other issue has landed. I prefer hacky tests to no tests :P But no strong opinion. https://codereview.adblockplus.org/29363607/diff/29363751/test/FilterEngine.cpp File test/FilterEngine.cpp (right): https://codereview.adblockplus.org/29363607/diff/29363751/test/FilterEngine.c... test/FilterEngine.cpp:119: FileSystemPtr m_fileSystem; On 2016/11/23 14:20:50, sergei wrote: > On 2016/11/22 17:37:17, Felix Dahlke wrote: > > From https://adblockplus.org/coding-style: "No hungarian notation, no special > > variable name prefixes or suffixes denoting type or scope." > > I thought we finally started to use #pragma once and prefixes for members of > classes in C++. > https://codereview.adblockplus.org/29333474/diff/29345688/compiled/String.h > > This two things give a lot of help. #pragma is not restricted by the code style and we've started to use it at least in adblockplusie (plus, I don't understand how that is related). Hungarian notation is not something we started using to my knowledge (or should use unless there's a really good reason IMO). But since you addressed it, we can discuss that elsewhere (and maybe it is time to disconnect our C++ coding style from Mozilla's, to make it easier to understand and evolve). https://codereview.adblockplus.org/29363607/diff/29363751/test/FilterEngine.c... test/FilterEngine.cpp:126: // directories use the hack: manually clean the directory. On 2016/11/23 14:20:50, sergei wrote: > On 2016/11/22 17:37:16, Felix Dahlke wrote: > > Do you mean "functionality to work with _a different directory_", maybe? > > No, it's possible to configure working directory of DefaultFileSystem, I mean a > cross platform API to work with directories, for instance it would be useful to > have a scoped temporary directory or at least some functions to get a path to > temporary writable directory, create and remove directories and in addition > generate a temporary folder name. I have not added it here because I'm sure it > should be in another commit. Acknowledged. https://codereview.adblockplus.org/29363607/diff/29363751/test/FilterEngine.c... test/FilterEngine.cpp:132: auto jsEngine = JsEngine::New(appInfo); On 2016/11/23 14:20:50, sergei wrote: > On 2016/11/22 17:37:16, Felix Dahlke wrote: > > I guess my C++ might be getting rusty, but why assign this to a temporary > local > > variable here, rather than directly to the member variable? > > JsEngine::New returns std::shared_ptr but the type of member is std::weak_ptr, > it's a part of the hack with sleep_for used in removeFileIfExists. It is to deal > with the following race condition. > > Currently we have issues with circular references > https://issues.adblockplus.org/ticket/3594 and with detached threads > https://issues.adblockplus.org/ticket/3595. Current version at master allows > different tests to influence another tests, for example, when the next test is > running, writing thread from a previous test can corrupt, e.g. patterns.ini. > Eliminating of circular references relieves a bit but not completely. > Asynchronous operation will be ignored if there is already no JsEngine, however > there is still a race condition, thread of asynchronous operation still can keep > JsEngine alive even when we have already no strong references to it. sleep_for > gives an asynchronous operation some time to finish its work, so afterwards we > can remove files created by current test. I have complicated the code even more > :) because it should take into account not only jsEngine of current test but > also async operations from previous tests (actually it happens too often on my > machine that patterns.ini is blocked by some previous test). > > All these hacks will go away as the life span of asynchronous operations is no > longer than the life span of both JsEngine and FilterEngine. Acknowledged. https://codereview.adblockplus.org/29363607/diff/29363751/test/FilterEngine.c... test/FilterEngine.cpp:605: const auto aaUrl = filterEngine->GetPref("subscriptions_exceptionsurl")->AsString(); On 2016/11/23 14:20:50, sergei wrote: > On 2016/11/22 17:37:16, Felix Dahlke wrote: > > Hm, maybe we should hard code the URL as well here, same as with the Chinese > > subscription? No strong opinion, up to you. > > I will remove aaUrl and use Subscription::IsAA instead in an upcoming commit. Acknowledged.
I have rebased on master and changed the number of iterations in tests, because otherwise it takes too long because currently JsEngine is never deleted.
LGTM 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
