Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(345)

Issue 29396582: Issue 5039 - add support of nullable non-object values in settings

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 years, 8 months ago by sergei
Modified:
1 year, 11 months ago
CC:
Felix Dahlke
Visibility:
Public.

Description

Do 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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+39 lines, -7 lines) Patch
M include/AdblockPlus/JsEngine.h View 1 2 3 4 1 chunk +6 lines, -0 lines 0 comments Download
M lib/prefs.js View 1 2 3 4 5 6 4 chunks +24 lines, -3 lines 0 comments Download
M src/FilterEngine.cpp View 1 2 3 4 5 2 chunks +3 lines, -4 lines 0 comments Download
M src/JsEngine.cpp View 1 2 3 4 1 chunk +6 lines, -0 lines 0 comments Download

Messages

Total messages: 19
sergei
2 years, 8 months ago (2017-03-28 09:26:42 UTC) #1
Oleksandr
I think it is more of a question to @Sebastian or @Dave, but I don't ...
2 years, 8 months ago (2017-03-28 12:42:08 UTC) #2
sergei
Adding Sebastian, Dave and Thomas. Could some of you please review it to ensure that ...
2 years, 8 months ago (2017-03-28 12:54:22 UTC) #3
sergei
BTW, it won't be too late to leave a comment here even if we commit ...
2 years, 8 months ago (2017-03-28 12:59:06 UTC) #4
Sebastian Noack
I don't really feel comfortable reviewing this code, as I never have seen any code ...
2 years, 8 months ago (2017-03-28 13:10:36 UTC) #5
sergei
On 2017/03/28 13:10:36, Sebastian Noack wrote: > I don't really feel comfortable reviewing this code, ...
2 years, 8 months ago (2017-03-28 14:31:41 UTC) #6
Oleksandr
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 ...
2 years, 8 months ago (2017-03-28 14:45:01 UTC) #7
sergei
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 ...
2 years, 8 months ago (2017-03-28 14:46:35 UTC) #8
Oleksandr
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 > ...
2 years, 8 months ago (2017-03-28 14:52:32 UTC) #9
Wladimir Palant
For what it's worth, I consider the use of preferences in https://issues.adblockplus.org/ticket/4931 the root issue ...
2 years, 8 months ago (2017-03-28 15:20:32 UTC) #10
sergei
On 2017/03/28 15:20:32, Wladimir Palant wrote: > For what it's worth, I consider the use ...
2 years, 8 months ago (2017-03-28 16:32:42 UTC) #11
Wladimir Palant
On 2017/03/28 16:32:42, sergei wrote: > If there are no strong objections against having that ...
2 years, 8 months ago (2017-03-28 18:03:09 UTC) #12
sergei
On 2017/03/28 18:03:09, Wladimir Palant wrote: > On 2017/03/28 16:32:42, sergei wrote: > > If ...
2 years, 8 months ago (2017-03-28 18:41:53 UTC) #13
anton
On 2017/03/28 18:41:53, sergei wrote: > On 2017/03/28 18:03:09, Wladimir Palant wrote: > > On ...
2 years, 8 months ago (2017-03-29 05:36:27 UTC) #14
sergei
Just in case I would like to clarify: This change is for libadblockplus repository, so ...
2 years, 8 months ago (2017-03-29 08:32:36 UTC) #15
anton
On 2017/03/29 08:32:36, sergei wrote: > Just in case I would like to clarify: > ...
2 years, 8 months ago (2017-03-29 08:37:09 UTC) #16
Eric
On 2017/03/29 08:37:09, anton wrote: > I was confused by Wladimir's comment related to adblcokpluscore. ...
2 years, 8 months ago (2017-03-29 14:48:53 UTC) #17
sergei
On 2017/03/29 14:48:53, Eric wrote: > On 2017/03/29 08:37:09, anton wrote: > > I was ...
2 years, 8 months ago (2017-03-29 16:25:07 UTC) #18
sergei
2 years, 8 months ago (2017-03-29 16:44:21 UTC) #19
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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5