Issue 29396582: Issue 5039 - add support of nullable non-object values in settings
DescriptionDo it for allowed_connection_type.
Patch Set 1 #Patch Set 2 : fix comment and rename local var #
Total comments: 3
Patch Set 3 : address comment and rename local var #Patch Set 4 : fix comparison with undefined #Patch Set 5 : add proper support of nullable setting values #
Total comments: 2
Patch Set 6 : rebase #Patch Set 7 : add comment #Patch Set 8 : rebase #
MessagesTotal messages: 19
I think it is more of a question to @Sebastian or @Dave, but I don't think we should be introducing optional values. Why not just add allowed_connection_type to defaults? https://codereview.adblockplus.org/29396582/diff/29396600/lib/prefs.js File lib/prefs.js (right): https://codereview.adblockplus.org/29396582/diff/29396600/lib/prefs.js#newcode51 lib/prefs.js:51: let optionalValues_ExpectedType = { Nit: how about optionalValues_ExpectedTypes for better distinction below. https://codereview.adblockplus.org/29396582/diff/29396600/lib/prefs.js#newcode91 lib/prefs.js:91: if (!isValueTypeCorrect(key, value)) It looks like this is done only for one property allowed_connection_type. Maybe there's no need to go too generic and just check the specific key and typeof value here?
Adding Sebastian, Dave and Thomas. Could some of you please review it to ensure that it does not conflict with adblockpluscore? JIC - we need to land it ASAP, ideally today - it's for libadblockplus repository
BTW, it won't be too late to leave a comment here even if we commit it. We can commit now simply because we very much need that fix, but the form it's done can be always adjusted later.
I don't really feel comfortable reviewing this code, as I never have seen any code in libadblockplus before, that said I didn't even know that it has it's own lib/prefs.js implementation. Wladimir is probably the best candidate to review JavaScript code in libadblockplus. But FWIW, I agree with Ollie. We don't have a special mechanism for optional values in any other prefs implementation, but just define defaults instead.
On 2017/03/28 13:10:36, Sebastian Noack wrote: > I don't really feel comfortable reviewing this code, as I never have seen any > code in libadblockplus before, that said I didn't even know that it has it's own > lib/prefs.js implementation. Wladimir is probably the best candidate to review > JavaScript code in libadblockplus. > > But FWIW, I agree with Ollie. We don't have a special mechanism for optional > values in any other prefs implementation, but just define defaults instead. Added a support for nullable values. https://codereview.adblockplus.org/29396582/diff/29396600/lib/prefs.js File lib/prefs.js (right): https://codereview.adblockplus.org/29396582/diff/29396600/lib/prefs.js#newcode51 lib/prefs.js:51: let optionalValues_ExpectedType = { On 2017/03/28 12:42:08, Oleksandr wrote: > Nit: how about optionalValues_ExpectedTypes for better distinction below. it's a bit difficult naming case, done.
https://codereview.adblockplus.org/29396582/diff/29396633/lib/prefs.js File lib/prefs.js (right): https://codereview.adblockplus.org/29396582/diff/29396633/lib/prefs.js#newcode49 lib/prefs.js:49: allowed_connection_type: null I think it would be better to have an empty string here instead. allowed_connection_type: "" This would remove the need for all the other changes file too.
https://codereview.adblockplus.org/29396582/diff/29396633/lib/prefs.js File lib/prefs.js (right): https://codereview.adblockplus.org/29396582/diff/29396633/lib/prefs.js#newcode49 lib/prefs.js:49: allowed_connection_type: null On 2017/03/28 14:45:01, Oleksandr wrote: > I think it would be better to have an empty string here instead. > > allowed_connection_type: "" > > This would remove the need for all the other changes file too. That where we started from, an empty string is a magic value.
On 2017/03/28 14:46:35, sergei wrote: > https://codereview.adblockplus.org/29396582/diff/29396633/lib/prefs.js > File lib/prefs.js (right): > > https://codereview.adblockplus.org/29396582/diff/29396633/lib/prefs.js#newcode49 > lib/prefs.js:49: allowed_connection_type: null > On 2017/03/28 14:45:01, Oleksandr wrote: > > I think it would be better to have an empty string here instead. > > > > allowed_connection_type: "" > > > > This would remove the need for all the other changes file too. > > That where we started from, an empty string is a magic value. I would not treat an empty string as a magic value. In fact I think it is magic numbers that we should be worried about, not a more generic magic values. If anything an empty string is a perfectly valid pre-initialized string in my book.
For what it's worth, I consider the use of preferences in https://issues.adblockplus.org/ticket/4931 the root issue here. The application provides a callback that determines whether a connection is allowed - yet it needs libadblockplus to tell it which connection types are currently configured as allowed? This makes no sense whatsoever, why should libadblockplus know anything about connection types and why the general assumption that the decision is being make based on connection type? That's application logic, it doesn't belong in libadblockplus in the first place.
On 2017/03/28 15:20:32, Wladimir Palant wrote: > For what it's worth, I consider the use of preferences in > https://issues.adblockplus.org/ticket/4931 the root issue here. The application > provides a callback that determines whether a connection is allowed - yet it > needs libadblockplus to tell it which connection types are currently configured > as allowed? This makes no sense whatsoever, why should libadblockplus know > anything about connection types and why the general assumption that the decision > is being make based on connection type? That's application logic, it doesn't > belong in libadblockplus in the first place. Originally #4931 is for the functionality preventing the downloading of only subscriptions when current connection is inappropriate, e.g. because it's a metered connection. Ideally this check should be in adblockpluscore when a subscription "thinks" that it's time for update and before initiating a network request for several reasons. So, basically a function "returning" Boolean is enough and there is no need for any setting, that's correct. Now let's think about a user of libadblockplus and in particular about libadblockplus-android. There is an UI to "configure libadblockplus", like disabling AA, manipulating subscriptions, I think something else and choosing of allowed connection type to update subscriptions, which is using libadblockplus as a permanent storage. Of course developers of libadblockplus-android could complicate the things by introducing a new separate storage or by finding likely a hacky way to continue to use libadblockplus as a storage. But instead of that sophistication libadblockplus could provide with the API to store that preference and a way of proper use of it. BTW, it also speeds up getting of this functionality which is very important and is the reason we decided to have at least some hack in libadblockplus for present. Because of that it's in libadblockplus. If there are some objections then for present I would propose to consider it rather as some sort of extension, like updater.js, which, I guess, is much less common than the discussed preference, and refactor it later. If there are no strong objections against having that preference in libadblockplus, could you please focus on the scope of this issue because we need it very much :).
On 2017/03/28 16:32:42, sergei wrote: > If there are no strong objections against having that preference in > libadblockplus, could you please focus on the scope of this issue because we > need it very much :). Point is: the scope of this issue is NOT LGTM. adblockpluscore has exactly four types of preferences and none of them allows null values. Changing the prefs implementation here without adjusting all the other implementations makes no sense, you might end up code that expects certain behavior. In other words, this change needs to start in adblockpluscore and propagate to the dependencies then. Given that you can achieve the same with an empty string I don't really see a point. Note that FilterStorage APIs don't need to actually expose that empty string, they could return null nevertheless.
On 2017/03/28 18:03:09, Wladimir Palant wrote: > On 2017/03/28 16:32:42, sergei wrote: > > If there are no strong objections against having that preference in > > libadblockplus, could you please focus on the scope of this issue because we > > need it very much :). > > Point is: the scope of this issue is NOT LGTM. adblockpluscore has exactly four > types of preferences and none of them allows null values. Changing the prefs > implementation here without adjusting all the other implementations makes no > sense, you might end up code that expects certain behavior. In other words, this > change needs to start in adblockpluscore and propagate to the dependencies then. > Given that you can achieve the same with an empty string I don't really see a > point. Note that FilterStorage APIs don't need to actually expose that empty > string, they could return null nevertheless. I guess one of four types is object, so why does null not one of them? Officially typeof null === "object", so no conflicts here, that's because I have changed it from undefined to null. It looks very bad when one passes an empty string to setter and gets a nullptr with getter. I would like to have a sane public API from the beginning or at least as soon as possible and adjust internals later. BTW, the proposed implementation does not allow any arbitrary preference to have null as a default value, it's only for specially defined preferences, in that particular case only for allowed_connection_type which is never used by core. We could simply put a comment to that list of preferences which can have null value as a default value to not put there a preference used by core.
On 2017/03/28 18:41:53, sergei wrote: > On 2017/03/28 18:03:09, Wladimir Palant wrote: > > On 2017/03/28 16:32:42, sergei wrote: > > > If there are no strong objections against having that preference in > > > libadblockplus, could you please focus on the scope of this issue because we > > > need it very much :). > > > > Point is: the scope of this issue is NOT LGTM. adblockpluscore has exactly > four > > types of preferences and none of them allows null values. Changing the prefs > > implementation here without adjusting all the other implementations makes no > > sense, you might end up code that expects certain behavior. In other words, > this > > change needs to start in adblockpluscore and propagate to the dependencies > then. > > Given that you can achieve the same with an empty string I don't really see a > > point. Note that FilterStorage APIs don't need to actually expose that empty > > string, they could return null nevertheless. > > I guess one of four types is object, so why does null not one of them? > Officially typeof null === "object", so no conflicts here, that's because I have > changed it from undefined to null. > > It looks very bad when one passes an empty string to setter and gets a nullptr > with getter. I would like to have a sane public API from the beginning or at > least as soon as possible and adjust internals later. BTW, the proposed > implementation does not allow any arbitrary preference to have null as a default > value, it's only for specially defined preferences, in that particular case only > for allowed_connection_type which is never used by core. We could simply put a > comment to that list of preferences which can have null value as a default value > to not put there a preference used by core. Let me add my 5 cents. It's true that it originates from agreement over API between libadblockplus and libadblockplus-android: https://hg.adblockplus.org/libadblockplus/file/tip/include/AdblockPlus/Filter... On the first invocation (and until it's expilitely set) there is no any value set. So we try to follow common - to pass `nullptr` in c++ or `null` in java meaning "no value". It's a considered to be a bad design to pass any hardcoded value like "" or "*" or "any" that actually means "no value" (see "magic number antipattern"). It becomes even bad if it arrives from one module (libadblockplus) to another (libadblockplus-android), because in the case of any change it's difficult to find the bug and fix it. I do understand it originates from the initial assumption in the core that any variable has value and has default value. So my original intention is to take one of the decisions: 1) the following agreement "null ptr means no value, no any magic numbers". If it does not suit into the following assumptions in the core you can use suggestion 2: 2) hide it within libadblockplus only (let's say you save some magic number indicating "no value") and still save the agreement between libadblockplus and libadblockplus-android. BTW i can't see any reasonable reason to still have the assumption that any variable has value and has a default value, because it's not limited neither storage (you can consider no key in `prefs.json` as no value) neither language. in ideal case our connection type is enum with 'any', 'wifi', 'wifi...' values, but the sergei's design is that libadblcokplus is now aware of this (and it is in libadblockplus-android, see https://codereview.adblockplus.org/29379647/diff/29396555/libadblockplus-andr...) 3) if #2 is still not suitable due to changes efforts, bug probability or any other reason we could use already used by libadblockplus-android behind the scene enum value: https://codereview.adblockplus.org/29379647/diff/29396555/libadblockplus-andr... line#52. In this case it's not magic number as the user has 3 choices and it's the value of one of them. And in this case we should: 1) document usage of this magic number in both libadblockplus and libadblockplus-android. 2) avoid passing ptr to std::string and use std::string instead in both setter (https://hg.adblockplus.org/libadblockplus/file/tip/include/AdblockPlus/Filter...) and callback (https://hg.adblockplus.org/libadblockplus/file/tip/include/AdblockPlus/Filter...).
Just in case I would like to clarify: This change is for libadblockplus repository, so any suggestion to have a workaround in libadblockplus sounds strange. I find the proposed changes as the least hacky workaround in libadblockplus. This change does not affect adblockpluscore, in adblockpluscore there is even no lib/prefs.js (https://github.com/adblockplus/adblockpluscore/tree/master/lib). That setting will not be and should not be in adblockpluscore, it belongs to libadblockplus and it is only for convenience.
On 2017/03/29 08:32:36, sergei wrote: > Just in case I would like to clarify: > > This change is for libadblockplus repository, so any suggestion to have a > workaround in libadblockplus sounds strange. I find the proposed changes as the > least hacky workaround in libadblockplus. > > This change does not affect adblockpluscore, in adblockpluscore there is even no > lib/prefs.js (https://github.com/adblockplus/adblockpluscore/tree/master/lib). > > That setting will not be and should not be in adblockpluscore, it belongs to > libadblockplus and it is only for convenience. I was confused by Wladimir's comment related to adblcokpluscore. So i don't see any problems of accepting/returning `nullptr` instead of any magic strings.
On 2017/03/29 08:37:09, anton wrote: > I was confused by Wladimir's comment related to adblcokpluscore. Wladimir's comment, insofar as I read it, has little to do with API details and a lot to do with preference values independent of the API that manipulates them. This change is adding a bunch of code to deal with null values as preference values and not merely as API interface values. Wladimir's point is that if you do that, you need to do it consistently across all preference values, which means it has to be in adblockpluscore. For what it's worth, when Wladmir made that comment, the title of this review was "Issue 5039, 5040 - add support of optional values in settings, do to for allowed_connection_type". That has since changed, dropping issue 5040, for which a separate change landed a couple hours ago. Wladimir will have to weigh in on whether his comment applied to one or both issues. That said, Wladimir's comment applies to this whole line of development, which suffers systematically from contortions to avoid changing adblockpluscore. The top-level idea is to avoid using network resources. Now the best way not to use a resource is never to ask to use it in the first place. To do it right, this would mean altering the code to download filter lists and to check for updates, and that code is in adblockpluscore. The way this code is structured, it goes down to the very bottom of the C++ call stack and generates an error whenever the network resource is used. That error then propagates through adblockpluscore, which has not been altered to deal with it in any constructive way. If you're lucky, it gets passed up to the application "properly" (across years of unrelated changes to adblockpluscore) and the application can deal with it by looking at the preference and choosing to ignore the error. This architecture is a recipe for disaster, apparently all to avoid dealing with another development team.
On 2017/03/29 14:48:53, Eric wrote:
> On 2017/03/29 08:37:09, anton wrote:
> > I was confused by Wladimir's comment related to adblcokpluscore.
>
> Wladimir's comment, insofar as I read it, has little to do with API details
and
> a lot to do with preference values independent of the API that manipulates
them.
> This change is adding a bunch of code to deal with null values as preference
> values and not merely as API interface values. Wladimir's point is that if you
> do that, you need to do it consistently across all preference values, which
> means it has to be in adblockpluscore.
Could please someone explain what actually should be done in ragard to that in
adblockpluscore and why? Just in case, the change does not affect the way we
work with preferences in adblcokpluscore and allowed_connection_type is not used
by adblockpluscore. I also proposed to put a comment asking to not put any
preference used by core into nullableValues_ExpectedTypes (done).
Another way to explain it:
lib/prefs.js here is a backend which is used to store our preferences, I would
say that API used by adblockpluscore is not affected by this change. In Firefox
and in Chrome we use another backends and each of those backends has its own
additional functionality and limitations. This change is about similar
additional functionality of the project specific storage backend in order to
reuse it for another feature. If you insist I can even hide preferences
mentioned in nullableValues_ExpectedTypes in the getter of
require("prefs").Prefs. Should I do it?
>
> For what it's worth, when Wladmir made that comment, the title of this review
> was "Issue 5039, 5040 - add support of optional values in settings, do to for
> allowed_connection_type". That has since changed, dropping issue 5040, for
which
> a separate change landed a couple hours ago. Wladimir will have to weigh in on
> whether his comment applied to one or both issues.
Yes, it's been landed because we need that functionality working in
libadblockplus-android right now. Oleksandr was against landing of these changes
because "You are touching a lot of critical code, when we have minimal time for
testing. Having it in GetAllowedConnection would localize possible bugs, I
think.", "it is natural to not see the bugs before they are exposed :) You are
touching the initialization and persistence routine of *all* prefs" and "and
like anton suggested, we can get back to this after we push this rushing
change." and in general I agree with these arguments. Now it's landed and it's
not hurting and we have some time to discuss it.
> That said, Wladimir's comment applies to this whole line of development, which
> suffers systematically from contortions to avoid changing adblockpluscore. The
> top-level idea is to avoid using network resources. Now the best way not to
use
> a resource is never to ask to use it in the first place. To do it right, this
> would mean altering the code to download filter lists and to check for
updates,
> and that code is in adblockpluscore. The way this code is structured, it goes
> down to the very bottom of the C++ call stack and generates an error whenever
> the network resource is used. That error then propagates through
> adblockpluscore, which has not been altered to deal with it in any
constructive
> way. If you're lucky, it gets passed up to the application "properly" (across
> years of unrelated changes to adblockpluscore) and the application can deal
with
> it by looking at the preference and choosing to ignore the error. This
> architecture is a recipe for disaster, apparently all to avoid dealing with
> another development team.
I disagree here. It has been done that way technically not to avoid dealing with
another development team but because of an extrime time pressure for that
feature. Felix who is CTO was one of that decision makers and if there is no yet
issue for adblcokpluscore, it's just a question of time and of course it was
clear from the beginning that that functionality should be in core, And just in
case, in addition, of course we are aware how badly the current approach
influences the UX visually.
On 2017/03/29 16:25:07, sergei wrote:
...
> If you insist I can even hide preferences
> mentioned in nullableValues_ExpectedTypes in the getter of
> require("prefs").Prefs. Should I do it?
Actually, I thought a bit how to hide preferences mentioned in
nullableValues_ExpectedTypes in accessors of require("prefs").Prefs and I think
it does not worth the efforts. Even more, we do have already additional
preferences in libadblockplus which are not used by adblockpluscore and no one
cares about hiding of them.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
