Issue 29519669: Issue 5539 - Implement "Acceptable Ads notification" (Closed)
|
Created:
Aug. 18, 2017, 11:51 p.m. by saroyanm Modified:
Sept. 22, 2017, 12:49 p.m. Reviewers:
Thomas Greiner Visibility:
Public. |
DescriptionPlease apply patch on top of "Patchset #3" of main General Tab review -> https://codereview.adblockplus.org/29478597/
Patch Set 1 #
Total comments: 23
Patch Set 2 : Rebased #
Total comments: 2
Patch Set 3 : #
Total comments: 8
Patch Set 4 : #
Total comments: 2
Patch Set 5 : Rebased #Patch Set 6 : #
Total comments: 6
Patch Set 7 : Rebased #Patch Set 8 : Addressed Thomas comment from patchset 6 #
MessagesTotal messages: 14
I prepared this patch as well, to just rebase after we done with General tab and speedup the process. https://codereview.adblockplus.org/29519669/diff/29519670/locale/en-US/new-op... File locale/en-US/new-options.json (right): https://codereview.adblockplus.org/29519669/diff/29519670/locale/en-US/new-op... locale/en-US/new-options.json:428: "message": "You currently allow Acceptable Ads to be shown. These ads are respectful and do not interfere with content that you view. Some of these ads require tracking to show you more relevant ads. Click <strong>YES</strong> to continue seeing relevant nonintrusive ads." In specs each sentence looks to be on it's own line, but we do not support row breaks in the strings. One solution is that I can put them inside <span>s with <br> in between: <span></span><br> <span></span><br> <span></span><br> <span></span> https://codereview.adblockplus.org/29519669/diff/29519670/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29519669/diff/29519670/new-options.js#newc... new-options.js:1228: openDialog("tracking"); In specs it says that "This is a one time message". Should we use Prefs for that, to store the value ?
https://codereview.adblockplus.org/29519669/diff/29519670/locale/en-US/new-op... File locale/en-US/new-options.json (right): https://codereview.adblockplus.org/29519669/diff/29519670/locale/en-US/new-op... locale/en-US/new-options.json:428: "message": "You currently allow Acceptable Ads to be shown. These ads are respectful and do not interfere with content that you view. Some of these ads require tracking to show you more relevant ads. Click <strong>YES</strong> to continue seeing relevant nonintrusive ads." On 2017/08/19 00:00:40, saroyanm wrote: > In specs each sentence looks to be on it's own line, but we do not support row > breaks in the strings. > One solution is that I can put them inside <span>s with <br> in between: > <span></span><br> > <span></span><br> > <span></span><br> > <span></span> This is not a viable solution because we don't support `<span>` tags either. Furthermore, it'd increase the probability of syntax errors that can be introduced during translation. If you don't want to modify the translation code you could simply split it up into one string per paragraph as we did with "options_page_header_*", for instance. Alternatively, you could implement support for `<br>`. I had to do that for Flattr already so if you want, I could provide you with the code we're using there. https://codereview.adblockplus.org/29519669/diff/29519670/locale/en-US/new-op... locale/en-US/new-options.json:430: "options_dialog_tracking_relevent_ads": { Typo: Replace "relevent" with "relevant" https://codereview.adblockplus.org/29519669/diff/29519670/locale/en-US/new-op... locale/en-US/new-options.json:430: "options_dialog_tracking_relevent_ads": { Detail: Those IDs are specific to how the text is phrased. Instead they should reflect what the message is. e.g. Replace "options_dialog_tracking_relevant_ads" with "options_dialog_tracking_allow" Replace "options_dialog_tracking_privacy_ads" with "options_dialog_tracking_disallow" https://codereview.adblockplus.org/29519669/diff/29519670/locale/en-US/new-op... locale/en-US/new-options.json:439: "description": "Text in 'Allow tracking on Acceptable Ads' dialog", Detail: I wasn't able to tell where this text is supposed to be without looking at the screenshot. It'd be great if the ID and the description were more descriptive. e.g. "options_dialog_tracking_note_title" "Text preceding list of notes in 'Allow tracking on Acceptable Ads' dialog" https://codereview.adblockplus.org/29519669/diff/29519670/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29519669/diff/29519670/new-options.js#newc... new-options.js:1032: function isPrivacyAndAdsEnabled() This function is a bit difficult to understand so a few recommendations from me would be: - Clarify intent of function in its name - Group similar tasks within the function - Clarify return statement e.g. function hasPrivacyConflict() { let acceptableAdsList = subscriptionsMap[acceptableAdsUrl]; let privacyList = null; for (let url in subscriptionsMap) { let subscription = subscriptionsMap[url]; if (subscription.recommended == "privacy") { privacyList = subscription; break; } } return acceptableAdsList && !acceptableAdsList.disabled && privacyList && !privacyList.disabled; } https://codereview.adblockplus.org/29519669/diff/29519670/new-options.js#newc... new-options.js:1227: if (isSubscriptionsLoaded && isPrivacyAndAdsEnabled()) Why is checking for `isSubscriptionsLoaded` necessary here? We're not doing it for any of the other checks either. https://codereview.adblockplus.org/29519669/diff/29519670/new-options.js#newc... new-options.js:1228: openDialog("tracking"); On 2017/08/19 00:00:40, saroyanm wrote: > In specs it says that "This is a one time message". > > Should we use Prefs for that, to store the value ? Yes. Probably best to prefix the preference name with "ui" to avoid naming collisions with existing or future preferences in adblockplus, adblockpluschrome or other users of adblockplusui. e.g. "ui_warn_tracking"
Thanks Thomas for reviewing the changes. https://codereview.adblockplus.org/29519669/diff/29519670/locale/en-US/new-op... File locale/en-US/new-options.json (right): https://codereview.adblockplus.org/29519669/diff/29519670/locale/en-US/new-op... locale/en-US/new-options.json:428: "message": "You currently allow Acceptable Ads to be shown. These ads are respectful and do not interfere with content that you view. Some of these ads require tracking to show you more relevant ads. Click <strong>YES</strong> to continue seeing relevant nonintrusive ads." On 2017/08/25 18:44:51, Thomas Greiner wrote: > On 2017/08/19 00:00:40, saroyanm wrote: > > In specs each sentence looks to be on it's own line, but we do not support row > > breaks in the strings. > > One solution is that I can put them inside <span>s with <br> in between: > > <span></span><br> > > <span></span><br> > > <span></span><br> > > <span></span> > > This is not a viable solution because we don't support `<span>` tags either. > Furthermore, it'd increase the probability of syntax errors that can be > introduced during translation. > > If you don't want to modify the translation code you could simply split it up > into one string per paragraph as we did with "options_page_header_*", for > instance. Actually that what I meant by the example of spans, so I was thinking to use <span>s instead of <p> tags. > > Alternatively, you could implement support for `<br>`. I had to do that for > Flattr already so if you want, I could provide you with the code we're using > there. Yes please, I think that will be best solution and save me time. https://codereview.adblockplus.org/29519669/diff/29519670/locale/en-US/new-op... locale/en-US/new-options.json:430: "options_dialog_tracking_relevent_ads": { On 2017/08/25 18:44:51, Thomas Greiner wrote: > Detail: Those IDs are specific to how the text is phrased. Instead they should > reflect what the message is. > > e.g. > Replace "options_dialog_tracking_relevant_ads" with > "options_dialog_tracking_allow" > Replace "options_dialog_tracking_privacy_ads" with > "options_dialog_tracking_disallow" agree. https://codereview.adblockplus.org/29519669/diff/29519670/locale/en-US/new-op... locale/en-US/new-options.json:439: "description": "Text in 'Allow tracking on Acceptable Ads' dialog", On 2017/08/25 18:44:51, Thomas Greiner wrote: > Detail: I wasn't able to tell where this text is supposed to be without looking > at the screenshot. It'd be great if the ID and the description were more > descriptive. > > e.g. > "options_dialog_tracking_note_title" > "Text preceding list of notes in 'Allow tracking on Acceptable Ads' dialog" Agree. https://codereview.adblockplus.org/29519669/diff/29519670/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29519669/diff/29519670/new-options.js#newc... new-options.js:1032: function isPrivacyAndAdsEnabled() On 2017/08/25 18:44:52, Thomas Greiner wrote: > This function is a bit difficult to understand so a few recommendations from me > would be: > - Clarify intent of function in its name > - Group similar tasks within the function > - Clarify return statement > > e.g. > > function hasPrivacyConflict() > { > let acceptableAdsList = subscriptionsMap[acceptableAdsUrl]; > let privacyList = null; > for (let url in subscriptionsMap) > { > let subscription = subscriptionsMap[url]; > if (subscription.recommended == "privacy") > { > privacyList = subscription; > break; > } > } > > return acceptableAdsList && !acceptableAdsList.disabled && > privacyList && !privacyList.disabled; > } Agree, thanks. https://codereview.adblockplus.org/29519669/diff/29519670/new-options.js#newc... new-options.js:1227: if (isSubscriptionsLoaded && isPrivacyAndAdsEnabled()) On 2017/08/25 18:44:51, Thomas Greiner wrote: > Why is checking for `isSubscriptionsLoaded` necessary here? We're not doing it > for any of the other checks either. I did it in order not to show on each load of the page as we don't have "pref" setting yet, if I would omit this the dialog will keep showing. https://codereview.adblockplus.org/29519669/diff/29519670/new-options.js#newc... new-options.js:1228: openDialog("tracking"); On 2017/08/25 18:44:51, Thomas Greiner wrote: > On 2017/08/19 00:00:40, saroyanm wrote: > > In specs it says that "This is a one time message". > > > > Should we use Prefs for that, to store the value ? > > Yes. Probably best to prefix the preference name with "ui" to avoid naming > collisions with existing or future preferences in adblockplus, adblockpluschrome > or other users of adblockplusui. > > e.g. "ui_warn_tracking" Thanks, I'll have a look into this.
https://codereview.adblockplus.org/29519669/diff/29519670/locale/en-US/new-op... File locale/en-US/new-options.json (right): https://codereview.adblockplus.org/29519669/diff/29519670/locale/en-US/new-op... locale/en-US/new-options.json:428: "message": "You currently allow Acceptable Ads to be shown. These ads are respectful and do not interfere with content that you view. Some of these ads require tracking to show you more relevant ads. Click <strong>YES</strong> to continue seeing relevant nonintrusive ads." On 2017/08/27 16:25:28, saroyanm wrote: > On 2017/08/25 18:44:51, Thomas Greiner wrote: > > On 2017/08/19 00:00:40, saroyanm wrote: > > > In specs each sentence looks to be on it's own line, but we do not support > row > > > breaks in the strings. > > > One solution is that I can put them inside <span>s with <br> in between: > > > <span></span><br> > > > <span></span><br> > > > <span></span><br> > > > <span></span> > > > > This is not a viable solution because we don't support `<span>` tags either. > > Furthermore, it'd increase the probability of syntax errors that can be > > introduced during translation. > > > > If you don't want to modify the translation code you could simply split it up > > into one string per paragraph as we did with "options_page_header_*", for > > instance. > Actually that what I meant by the example of spans, so I was thinking to use > <span>s instead of <p> tags. Ah ok. In that case `<div>` would be more appropriate than `<span>` though because it's already a block element so no need for `<br>`. > > Alternatively, you could implement support for `<br>`. I had to do that for > > Flattr already so if you want, I could provide you with the code we're using > > there. > Yes please, I think that will be best solution and save me time. I uploaded it to https://gist.github.com/ThomasGreiner/a2f32ed70cdb19e5e24654ae3b4f5fac (see `getNodes()`). But note that we're working with a virtual DOM there so you'd still need to adapt that to using the actual DOM API. Obviously, we need to be very careful when dealing with transforming arbitrary text into DOM elements so if you're uncomfortable with doing that feel free to use multiple strings instead as you suggested. https://codereview.adblockplus.org/29519669/diff/29519670/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29519669/diff/29519670/new-options.js#newc... new-options.js:1227: if (isSubscriptionsLoaded && isPrivacyAndAdsEnabled()) On 2017/08/27 16:25:28, saroyanm wrote: > On 2017/08/25 18:44:51, Thomas Greiner wrote: > > Why is checking for `isSubscriptionsLoaded` necessary here? We're not doing it > > for any of the other checks either. > > I did it in order not to show on each load of the page as we don't have "pref" > setting yet, if I would omit this the dialog will keep showing. So it's a stopgap solution until we have the preference? What about introducing the preference then if it's necessary for this feature to work?
Rebased, will adjust styles and address comments soon. https://codereview.adblockplus.org/29519669/diff/29544680/new-options.html File new-options.html (right): https://codereview.adblockplus.org/29519669/diff/29544680/new-options.html#ne... new-options.html:339: <span id="dialog-title-tracking" class="i18n_options_dialog_tracking_title"></span> This needs to be switched to h3
Sorry that it took so long: * Rebased * Comments addressed * New patch uploaded. Ready for the review. https://codereview.adblockplus.org/29519669/diff/29519670/locale/en-US/new-op... File locale/en-US/new-options.json (right): https://codereview.adblockplus.org/29519669/diff/29519670/locale/en-US/new-op... locale/en-US/new-options.json:428: "message": "You currently allow Acceptable Ads to be shown. These ads are respectful and do not interfere with content that you view. Some of these ads require tracking to show you more relevant ads. Click <strong>YES</strong> to continue seeing relevant nonintrusive ads." On 2017/08/29 10:20:41, Thomas Greiner wrote: > On 2017/08/27 16:25:28, saroyanm wrote: > > On 2017/08/25 18:44:51, Thomas Greiner wrote: > > > On 2017/08/19 00:00:40, saroyanm wrote: > > > > In specs each sentence looks to be on it's own line, but we do not support > > row > > > > breaks in the strings. > > > > One solution is that I can put them inside <span>s with <br> in between: > > > > <span></span><br> > > > > <span></span><br> > > > > <span></span><br> > > > > <span></span> > > > > > > This is not a viable solution because we don't support `<span>` tags either. > > > Furthermore, it'd increase the probability of syntax errors that can be > > > introduced during translation. > > > > > > If you don't want to modify the translation code you could simply split it > up > > > into one string per paragraph as we did with "options_page_header_*", for > > > instance. > > Actually that what I meant by the example of spans, so I was thinking to use > > <span>s instead of <p> tags. > > Ah ok. In that case `<div>` would be more appropriate than `<span>` though > because it's already a block element so no need for `<br>`. > > > > Alternatively, you could implement support for `<br>`. I had to do that for > > > Flattr already so if you want, I could provide you with the code we're using > > > there. > > Yes please, I think that will be best solution and save me time. > > I uploaded it to > https://gist.github.com/ThomasGreiner/a2f32ed70cdb19e5e24654ae3b4f5fac (see > `getNodes()`). But note that we're working with a virtual DOM there so you'd > still need to adapt that to using the actual DOM API. > > Obviously, we need to be very careful when dealing with transforming arbitrary > text into DOM elements so if you're uncomfortable with doing that feel free to > use multiple strings instead as you suggested. Having another look I think we don't need to use <br>, this seems to be different strings separated into paragraphs, or lists I think. https://codereview.adblockplus.org/29519669/diff/29519670/locale/en-US/new-op... locale/en-US/new-options.json:430: "options_dialog_tracking_relevent_ads": { On 2017/08/27 16:25:28, saroyanm wrote: > On 2017/08/25 18:44:51, Thomas Greiner wrote: > > Detail: Those IDs are specific to how the text is phrased. Instead they should > > reflect what the message is. > > > > e.g. > > Replace "options_dialog_tracking_relevant_ads" with > > "options_dialog_tracking_allow" > > Replace "options_dialog_tracking_privacy_ads" with > > "options_dialog_tracking_disallow" > > agree. Done. https://codereview.adblockplus.org/29519669/diff/29519670/locale/en-US/new-op... locale/en-US/new-options.json:439: "description": "Text in 'Allow tracking on Acceptable Ads' dialog", On 2017/08/27 16:25:28, saroyanm wrote: > On 2017/08/25 18:44:51, Thomas Greiner wrote: > > Detail: I wasn't able to tell where this text is supposed to be without > looking > > at the screenshot. It'd be great if the ID and the description were more > > descriptive. > > > > e.g. > > "options_dialog_tracking_note_title" > > "Text preceding list of notes in 'Allow tracking on Acceptable Ads' dialog" > > Agree. Done. https://codereview.adblockplus.org/29519669/diff/29519670/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29519669/diff/29519670/new-options.js#newc... new-options.js:1032: function isPrivacyAndAdsEnabled() On 2017/08/27 16:25:28, saroyanm wrote: > On 2017/08/25 18:44:52, Thomas Greiner wrote: > > This function is a bit difficult to understand so a few recommendations from > me > > would be: > > - Clarify intent of function in its name > > - Group similar tasks within the function > > - Clarify return statement > > > > e.g. > > > > function hasPrivacyConflict() > > { > > let acceptableAdsList = subscriptionsMap[acceptableAdsUrl]; > > let privacyList = null; > > for (let url in subscriptionsMap) > > { > > let subscription = subscriptionsMap[url]; > > if (subscription.recommended == "privacy") > > { > > privacyList = subscription; > > break; > > } > > } > > > > return acceptableAdsList && !acceptableAdsList.disabled && > > privacyList && !privacyList.disabled; > > } > > Agree, thanks. Done. https://codereview.adblockplus.org/29519669/diff/29519670/new-options.js#newc... new-options.js:1227: if (isSubscriptionsLoaded && isPrivacyAndAdsEnabled()) On 2017/08/29 10:20:41, Thomas Greiner wrote: > On 2017/08/27 16:25:28, saroyanm wrote: > > On 2017/08/25 18:44:51, Thomas Greiner wrote: > > > Why is checking for `isSubscriptionsLoaded` necessary here? We're not doing > it > > > for any of the other checks either. > > > > I did it in order not to show on each load of the page as we don't have "pref" > > setting yet, if I would omit this the dialog will keep showing. > > So it's a stopgap solution until we have the preference? What about introducing > the preference then if it's necessary for this feature to work? Done. https://codereview.adblockplus.org/29519669/diff/29519670/new-options.js#newc... new-options.js:1228: openDialog("tracking"); On 2017/08/27 16:25:28, saroyanm wrote: > On 2017/08/25 18:44:51, Thomas Greiner wrote: > > On 2017/08/19 00:00:40, saroyanm wrote: > > > In specs it says that "This is a one time message". > > > > > > Should we use Prefs for that, to store the value ? > > > > Yes. Probably best to prefix the preference name with "ui" to avoid naming > > collisions with existing or future preferences in adblockplus, > adblockpluschrome > > or other users of adblockplusui. > > > > e.g. "ui_warn_tracking" > > Thanks, I'll have a look into this. Done. https://codereview.adblockplus.org/29519669/diff/29544680/new-options.html File new-options.html (right): https://codereview.adblockplus.org/29519669/diff/29544680/new-options.html#ne... new-options.html:339: <span id="dialog-title-tracking" class="i18n_options_dialog_tracking_title"></span> On 2017/09/14 14:31:18, saroyanm wrote: > This needs to be switched to h3 Done.
https://codereview.adblockplus.org/29519669/diff/29544700/new-options.html File new-options.html (right): https://codereview.adblockplus.org/29519669/diff/29544700/new-options.html#ne... new-options.html:403: <button class="i18n_options_dialog_tracking_disallow secondary" data-pref="ui_warn_tracking" data-action="disallow-tracking,toggle-pref,close-dialog"></button> We should probably not change the preference until we know that "disallow-tracking" succeeded. Anyway, I noted this down in the spec PR about partial failures (see https://bitbucket.org/adblockplus/spec/pull-requests/55/issue-59-defined-beha...) so we can postpone it for now. https://codereview.adblockplus.org/29519669/diff/29544700/new-options.html#ne... new-options.html:403: <button class="i18n_options_dialog_tracking_disallow secondary" data-pref="ui_warn_tracking" data-action="disallow-tracking,toggle-pref,close-dialog"></button> All "disallow-tracking" does is run "switch-acceptable-ads" with the value "privacy" so what do we need "disallow-tracking" for if we could just write: <button class="…" data-action="switch-acceptable-ads,toggle-pref,close-dialog" data-value="privacy" data-pref="ui_warn_tracking"> https://codereview.adblockplus.org/29519669/diff/29544700/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29519669/diff/29544700/new-options.js#newc... new-options.js:32: let showTrackingWarning = true; This value needs to come from the preferences. Otherwise it'd always be `true` whenever the user opens the page. https://codereview.adblockplus.org/29519669/diff/29544700/new-options.js#newc... new-options.js:1029: privacyList && privacyList.disabled == false && showTrackingWarning; Based on its name, the purpose of this function is to check whether a privacy conflict exists, not whether the tracking warning should be shown. Therefore let's check `showTrackingWarning` elsewhere.
New patch uploaded https://codereview.adblockplus.org/29519669/diff/29544700/new-options.html File new-options.html (right): https://codereview.adblockplus.org/29519669/diff/29544700/new-options.html#ne... new-options.html:403: <button class="i18n_options_dialog_tracking_disallow secondary" data-pref="ui_warn_tracking" data-action="disallow-tracking,toggle-pref,close-dialog"></button> On 2017/09/15 16:15:23, Thomas Greiner wrote: > We should probably not change the preference until we know that > "disallow-tracking" succeeded. > > Anyway, I noted this down in the spec PR about partial failures (see > https://bitbucket.org/adblockplus/spec/pull-requests/55/issue-59-defined-beha...) > so we can postpone it for now. Acknowledged. https://codereview.adblockplus.org/29519669/diff/29544700/new-options.html#ne... new-options.html:403: <button class="i18n_options_dialog_tracking_disallow secondary" data-pref="ui_warn_tracking" data-action="disallow-tracking,toggle-pref,close-dialog"></button> On 2017/09/15 16:15:23, Thomas Greiner wrote: > All "disallow-tracking" does is run "switch-acceptable-ads" with the value > "privacy" so what do we need "disallow-tracking" for if we could just write: > > <button class="…" > data-action="switch-acceptable-ads,toggle-pref,close-dialog" > data-value="privacy" > data-pref="ui_warn_tracking"> Done. https://codereview.adblockplus.org/29519669/diff/29544700/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29519669/diff/29544700/new-options.js#newc... new-options.js:32: let showTrackingWarning = true; On 2017/09/15 16:15:24, Thomas Greiner wrote: > This value needs to come from the preferences. Otherwise it'd always be `true` > whenever the user opens the page. Well spotted, thanks. Done. https://codereview.adblockplus.org/29519669/diff/29544700/new-options.js#newc... new-options.js:1029: privacyList && privacyList.disabled == false && showTrackingWarning; On 2017/09/15 16:15:24, Thomas Greiner wrote: > Based on its name, the purpose of this function is to check whether a privacy > conflict exists, not whether the tracking warning should be shown. Therefore > let's check `showTrackingWarning` elsewhere. Agree, Done.
Rebased
Just one more thing to avoid a race condition. https://codereview.adblockplus.org/29519669/diff/29545694/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29519669/diff/29545694/new-options.js#newc... new-options.js:1206: if (hasPrivacyConflict() && showTrackingWarning) `getPref("ui_warn_tracking")` may not have returned the result at this point yet so let's wait for it. The way I usually like to do that is by using a promise. e.g. let promisedShowTrackingWarning = new Promise((resolve) => { getPref("ui_warn_tracking", resolve); });
Thanks Thomas, new patch uploaded and is ready for the review. https://codereview.adblockplus.org/29519669/diff/29545694/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29519669/diff/29545694/new-options.js#newc... new-options.js:1206: if (hasPrivacyConflict() && showTrackingWarning) On 2017/09/20 12:44:10, Thomas Greiner wrote: > `getPref("ui_warn_tracking")` may not have returned the result at this point yet > so let's wait for it. > > The way I usually like to do that is by using a promise. > > e.g. > let promisedShowTrackingWarning = new Promise((resolve) => > { > getPref("ui_warn_tracking", resolve); > }); I tried using Promises, but noticed that it didn't optimize the new patch, but I agree in general that we should use promises in current file. I think it will make sense to define in which cases we want to use promises, I've created a ticket for that -> https://issues.adblockplus.org/ticket/5747 I would like to discuss with you the content, so we can define what exactly we would like to change and in which case to use Promises. https://codereview.adblockplus.org/29519669/diff/29551685/new-options.html File new-options.html (right): https://codereview.adblockplus.org/29519669/diff/29551685/new-options.html#ne... new-options.html:400: <button class="i18n_options_dialog_tracking_allow primary default-focus" data-pref="ui_warn_tracking" data-action="toggle-pref,close-dialog"></button> If focus is not set, an error is being thrown. https://codereview.adblockplus.org/29519669/diff/29551685/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29519669/diff/29551685/new-options.js#newc... new-options.js:375: let {disabled} = subscription; Note: This is part of eslint error fix. I fixed couple of errors below as well. https://codereview.adblockplus.org/29519669/diff/29551685/new-options.js#newc... new-options.js:1202: getPref("ui_warn_tracking", (showTrackingWarning) => I'm not sure if using promise is better in this case, please let me know if you disagree, if I would use it here, it will be the only place that we are using it. As soon we will use this variable in multiple places I think in that case it would make sense to use promises to make code more readable, but while we just need it here, this code looks more compact in comparison to: new Promise((resolve) => { getPref("ui_warn_tracking", resolve); }).then((value) => { if (hasPrivacyConflict() && value) openDialog("tracking"); });
https://codereview.adblockplus.org/29519669/diff/29551685/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29519669/diff/29551685/new-options.js#newc... new-options.js:1202: getPref("ui_warn_tracking", (showTrackingWarning) => On 2017/09/21 15:50:40, saroyanm wrote: > I'm not sure if using promise is better in this case, please let me know if you > disagree, if I would use it here, it will be the only place that we are using > it. > > As soon we will use this variable in multiple places I think in that case it > would make sense to use promises to make code more readable, but while we just > need it here, this code looks more compact in comparison to: > > new Promise((resolve) => > { > getPref("ui_warn_tracking", resolve); > }).then((value) => > { > if (hasPrivacyConflict() && value) > openDialog("tracking"); > }); Using a promise you could've avoided making that request to the background page each time but this shouldn't happen too often anyway so this approach is also totally fine with me since it keeps things simple. It'd be great, however, if you could check for `hasPrivacyConflict()` before retrieving the preference because we don't need to check for `showTrackingWarning` if there's no privacy conflict. https://codereview.adblockplus.org/29519669/diff/29551685/new-options.js#newc... new-options.js:1358: "ui_warn_tracking"] Detail: We're no longer reacting to changes for the "ui_warn_tracking" preference so no need to listen to it anymore.
Rebased and new patch uploaded. https://codereview.adblockplus.org/29519669/diff/29551685/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29519669/diff/29551685/new-options.js#newc... new-options.js:1202: getPref("ui_warn_tracking", (showTrackingWarning) => On 2017/09/22 10:05:43, Thomas Greiner wrote: > On 2017/09/21 15:50:40, saroyanm wrote: > > I'm not sure if using promise is better in this case, please let me know if > you > > disagree, if I would use it here, it will be the only place that we are using > > it. > > > > As soon we will use this variable in multiple places I think in that case it > > would make sense to use promises to make code more readable, but while we just > > need it here, this code looks more compact in comparison to: > > > > new Promise((resolve) => > > { > > getPref("ui_warn_tracking", resolve); > > }).then((value) => > > { > > if (hasPrivacyConflict() && value) > > openDialog("tracking"); > > }); > > Using a promise you could've avoided making that request to the background page > each time but this shouldn't happen too often anyway so this approach is also > totally fine with me since it keeps things simple. > It'd be great, however, if you could check for `hasPrivacyConflict()` before > retrieving the preference because we don't need to check for > `showTrackingWarning` if there's no privacy conflict. I agree, done.
LGTM 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
