Issue 29570584: Issue 5847 - Add prefs.set message handler (Closed)
DescriptionIssue 5847 - Add prefs.set message handler
Patch Set 1 #Patch Set 2 : Honour forceValue for other preferences while at it #
Total comments: 2
Patch Set 3 : Instead add the new prefs.set message handler #
Total comments: 2
Patch Set 4 : Fixed typo #
Total comments: 3
Patch Set 5 : Don't toggle notifications when prefs.set value is omitted #MessagesTotal messages: 11
Patch Set 1 Patch Set 2 : Honour forceValue for other preferences while at it
https://codereview.adblockplus.org/29570584/diff/29570587/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29570584/diff/29570587/messageResponder.js... messageResponder.js:343: if (typeof message.forceValue == "undefined") I really think there should be a separate "prefs.set" for this, and even "notifications_ignoredcategories" could be covered by it, since passing forceValue there is the equivalent of setting the preference.
https://codereview.adblockplus.org/29570584/diff/29570587/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29570584/diff/29570587/messageResponder.js... messageResponder.js:343: if (typeof message.forceValue == "undefined") On 2017/10/09 14:41:45, Manish Jethani wrote: > I really think there should be a separate "prefs.set" for this, and even > "notifications_ignoredcategories" could be covered by it, since passing > forceValue there is the equivalent of setting the preference. Yea you're probably right. I'll update the issue.
On 2017/10/09 14:45:09, kzar wrote: > https://codereview.adblockplus.org/29570584/diff/29570587/messageResponder.js > File messageResponder.js (right): > > https://codereview.adblockplus.org/29570584/diff/29570587/messageResponder.js... > messageResponder.js:343: if (typeof message.forceValue == "undefined") > On 2017/10/09 14:41:45, Manish Jethani wrote: > > I really think there should be a separate "prefs.set" for this, and even > > "notifications_ignoredcategories" could be covered by it, since passing > > forceValue there is the equivalent of setting the preference. > > Yea you're probably right. I'll update the issue. Note that if you add a "prefs.set" then, similar to "prefs.toggle", it should also return the new value.
Patch Set 3 : Instead add the new prefs.set message handler
https://codereview.adblockplus.org/29570584/diff/29570606/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29570584/diff/29570606/messageResponder.js... messageResponder.js:343: return Prefs[message.key] = !Prefs[message.value]; Here you just mean to assign message.value.
Patch Set 4 : Fixed typo https://codereview.adblockplus.org/29570584/diff/29570606/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29570584/diff/29570606/messageResponder.js... messageResponder.js:343: return Prefs[message.key] = !Prefs[message.value]; On 2017/10/09 15:02:55, Manish Jethani wrote: > Here you just mean to assign message.value. Whoops good catch, Done.
https://codereview.adblockplus.org/29570584/diff/29570612/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29570584/diff/29570612/messageResponder.js... messageResponder.js:340: if (message.key == "notifications_ignoredcategories") Um ... what if message.value is undefined? Then it'll get toggled, but that's not the intention. I think it should be a no-op in that case. So maybe we should add another check 'typeof message.value == "boolean")'
Patch Set 5 : Don't toggle notifications when prefs.set value is omitted https://codereview.adblockplus.org/29570584/diff/29570612/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29570584/diff/29570612/messageResponder.js... messageResponder.js:340: if (message.key == "notifications_ignoredcategories") On 2017/10/09 15:16:52, Manish Jethani wrote: > Um ... what if message.value is undefined? Then it'll get toggled, but that's > not the intention. I think it should be a no-op in that case. > > So maybe we should add another check 'typeof message.value == "boolean")' Well I agree that toggling isn't the intention in that situation, but not about the no-op part. So kinda-Done.
LGTM https://codereview.adblockplus.org/29570584/diff/29570612/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29570584/diff/29570612/messageResponder.js... messageResponder.js:340: if (message.key == "notifications_ignoredcategories") On 2017/10/09 15:24:08, kzar wrote: > On 2017/10/09 15:16:52, Manish Jethani wrote: > > Um ... what if message.value is undefined? Then it'll get toggled, but that's > > not the intention. I think it should be a no-op in that case. > > > > So maybe we should add another check 'typeof message.value == "boolean")' > > Well I agree that toggling isn't the intention in that situation, but not about > the no-op part. So kinda-Done. Hmm ... OK, yeah, that's probably better.
LGTM 
|
