Issue 29332492: Issue 3269 - Display uninstallation page when uninstalled (Closed)
|
Created:
Dec. 9, 2015, 3:11 p.m. by kzar Modified:
Dec. 15, 2015, 5:14 p.m. Reviewers:
Sebastian Noack Visibility:
Public. |
DescriptionIssue 3269 - Display uninstallation page when uninstalled
Patch Set 1 #
Total comments: 3
Patch Set 2 : Create ext.setUninstallURL abstraction #
Total comments: 14
Patch Set 3 : Addressed feedback #
Total comments: 8
Patch Set 4 : Addressed further comments #
Total comments: 6
Patch Set 5 : Addressed more feedback #
Total comments: 2
Patch Set 6 : Check the API exists before using it #
MessagesTotal messages: 11
Patch Set 1 Patch Set 2 : Create ext.setUninstallURL abstraction https://codereview.adblockplus.org/29332492/diff/29332493/metadata.safari File metadata.safari (right): https://codereview.adblockplus.org/29332492/diff/29332493/metadata.safari#new... metadata.safari:9: backgroundScripts -= ext/uninstall.js I'm not sure if this is the best approach, open to suggestions.
https://codereview.adblockplus.org/29332492/diff/29332493/metadata.safari File metadata.safari (right): https://codereview.adblockplus.org/29332492/diff/29332493/metadata.safari#new... metadata.safari:9: backgroundScripts -= ext/uninstall.js On 2015/12/09 15:40:44, kzar wrote: > I'm not sure if this is the best approach, open to suggestions. uninstall.js isn't part of the abstraction layer. And therefore rather belongs into "lib", not "ext". Moreover, I would make it a module, bundled with adblockplus.js, see the "convert_js" section. https://codereview.adblockplus.org/29332492/diff/29332497/chrome/ext/uninstal... File chrome/ext/uninstall.js (right): https://codereview.adblockplus.org/29332492/diff/29332497/chrome/ext/uninstal... chrome/ext/uninstall.js:18: (function() Once you made this a proper module the IIFE will be redundant. Also note that the ES 6 features below (i.e. block scoping and for-of loops) aren't transpiled by jshydra currently. However, they will be once you made this a module. https://codereview.adblockplus.org/29332492/diff/29332497/chrome/ext/uninstal... chrome/ext/uninstall.js:22: let prefs = require("prefs"); Nit: let {Prefs} = require("prefs"); https://codereview.adblockplus.org/29332492/diff/29332497/chrome/ext/uninstal... chrome/ext/uninstall.js:26: let uninstall_url = "https://adblockplus.org/en/uninstall-abp"; Please use camel-case consistently. https://codereview.adblockplus.org/29332492/diff/29332497/chrome/ext/uninstal... chrome/ext/uninstall.js:26: let uninstall_url = "https://adblockplus.org/en/uninstall-abp"; Please don't hard code the language part of the URL! We have to setup a redirect (if we haven't one yet) similar as for other links used in the extension, and then use Utils.getDocLink() here. https://codereview.adblockplus.org/29332492/diff/29332497/chrome/ext/uninstal... chrome/ext/uninstall.js:36: if (key in module_key[0]) I don't see why that check would be necessary. https://codereview.adblockplus.org/29332492/diff/29332497/chrome/ext/uninstal... chrome/ext/uninstall.js:40: search.push("notificationDownloadCount=" + encodeURIComponent(downlCount)); You could simplify the loop above if you just handle Utils.appLocale the same way as the notificationDownloadCount. https://codereview.adblockplus.org/29332492/diff/29332497/chrome/ext/uninstal... chrome/ext/uninstall.js:51: prefs.Prefs.onLoaded.addListener(setUninstallURL); You should remove the listener once it has been called. Otherwise this will introduce a small memory leak.
Patch Set 3 : Addressed feedback https://codereview.adblockplus.org/29332492/diff/29332493/metadata.safari File metadata.safari (right): https://codereview.adblockplus.org/29332492/diff/29332493/metadata.safari#new... metadata.safari:9: backgroundScripts -= ext/uninstall.js On 2015/12/09 17:37:31, Sebastian Noack wrote: > On 2015/12/09 15:40:44, kzar wrote: > > I'm not sure if this is the best approach, open to suggestions. > > uninstall.js isn't part of the abstraction layer. And therefore rather belongs > into "lib", not "ext". > > Moreover, I would make it a module, bundled with adblockplus.js, see the > "convert_js" section. Done. https://codereview.adblockplus.org/29332492/diff/29332497/chrome/ext/uninstal... File chrome/ext/uninstall.js (right): https://codereview.adblockplus.org/29332492/diff/29332497/chrome/ext/uninstal... chrome/ext/uninstall.js:18: (function() On 2015/12/09 17:37:31, Sebastian Noack wrote: > Once you made this a proper module the IIFE will be redundant. Also note that > the ES 6 features below (i.e. block scoping and for-of loops) aren't transpiled > by jshydra currently. However, they will be once you made this a module. Done. https://codereview.adblockplus.org/29332492/diff/29332497/chrome/ext/uninstal... chrome/ext/uninstall.js:22: let prefs = require("prefs"); On 2015/12/09 17:37:31, Sebastian Noack wrote: > Nit: let {Prefs} = require("prefs"); Done. https://codereview.adblockplus.org/29332492/diff/29332497/chrome/ext/uninstal... chrome/ext/uninstall.js:26: let uninstall_url = "https://adblockplus.org/en/uninstall-abp"; On 2015/12/09 17:37:31, Sebastian Noack wrote: > Please use camel-case consistently. Done. https://codereview.adblockplus.org/29332492/diff/29332497/chrome/ext/uninstal... chrome/ext/uninstall.js:26: let uninstall_url = "https://adblockplus.org/en/uninstall-abp"; On 2015/12/09 17:37:31, Sebastian Noack wrote: > Please don't hard code the language part of the URL! > > We have to setup a redirect (if we haven't one yet) similar as for other links > used in the extension, and then use Utils.getDocLink() here. Done. (Opened issue 3403 for the redirection) https://codereview.adblockplus.org/29332492/diff/29332497/chrome/ext/uninstal... chrome/ext/uninstall.js:36: if (key in module_key[0]) On 2015/12/09 17:37:31, Sebastian Noack wrote: > I don't see why that check would be necessary. Done. https://codereview.adblockplus.org/29332492/diff/29332497/chrome/ext/uninstal... chrome/ext/uninstall.js:40: search.push("notificationDownloadCount=" + encodeURIComponent(downlCount)); On 2015/12/09 17:37:31, Sebastian Noack wrote: > You could simplify the loop above if you just handle Utils.appLocale the same > way as the notificationDownloadCount. Done. https://codereview.adblockplus.org/29332492/diff/29332497/chrome/ext/uninstal... chrome/ext/uninstall.js:51: prefs.Prefs.onLoaded.addListener(setUninstallURL); On 2015/12/09 17:37:31, Sebastian Noack wrote: > You should remove the listener once it has been called. Otherwise this will > introduce a small memory leak. Done.
https://codereview.adblockplus.org/29332492/diff/29332508/lib/uninstall.js File lib/uninstall.js (right): https://codereview.adblockplus.org/29332492/diff/29332508/lib/uninstall.js#ne... lib/uninstall.js:23: const uninstallURL = Utils.getDocLink("uninstall-abp"); IMO, there is no point in caching this into a global variable. We only use that variable once and not in any hotspot. https://codereview.adblockplus.org/29332492/diff/29332508/lib/uninstall.js#ne... lib/uninstall.js:33: search.push("appLocale=" + encodeURIComponent(Utils.appLocale)); Note that the URL returned by Utils.getDocLink() already specifies the appLocale with the "lang" parameter. https://codereview.adblockplus.org/29332492/diff/29332508/lib/uninstall.js#ne... lib/uninstall.js:49: // URL. (But we're not sure if they have already been loaded so we have to do Actually we know that prefs haven't been loaded yet. As the background scripts are executed synchronously, the top level code here is a excuted before any async events. (In the future we should probably use promises for onLoaded events and similar to avoid confusion and boilerplate though) https://codereview.adblockplus.org/29332492/diff/29332508/safari/ext/common.js File safari/ext/common.js (right): https://codereview.adblockplus.org/29332492/diff/29332508/safari/ext/common.j... safari/ext/common.js:223: ext.setUninstallURL = function (url) { }; This is dead code. Dead code is bad, regardless how trivial it is, as its never executed/tested. Given the current scheme, I also wonder whether we should just use chrome.runtime.setUninstallURL directly in unistall.js for simplicity.
Patch Set 4 : Addressed further comments https://codereview.adblockplus.org/29332492/diff/29332508/lib/uninstall.js File lib/uninstall.js (right): https://codereview.adblockplus.org/29332492/diff/29332508/lib/uninstall.js#ne... lib/uninstall.js:23: const uninstallURL = Utils.getDocLink("uninstall-abp"); On 2015/12/12 00:31:08, Sebastian Noack wrote: > IMO, there is no point in caching this into a global variable. We only use that > variable once and not in any hotspot. Done. https://codereview.adblockplus.org/29332492/diff/29332508/lib/uninstall.js#ne... lib/uninstall.js:33: search.push("appLocale=" + encodeURIComponent(Utils.appLocale)); On 2015/12/12 00:31:08, Sebastian Noack wrote: > Note that the URL returned by Utils.getDocLink() already specifies the appLocale > with the "lang" parameter. OK, updated the issues accordingly and Done. https://codereview.adblockplus.org/29332492/diff/29332508/lib/uninstall.js#ne... lib/uninstall.js:49: // URL. (But we're not sure if they have already been loaded so we have to do On 2015/12/12 00:31:08, Sebastian Noack wrote: > Actually we know that prefs haven't been loaded yet. As the background scripts > are executed synchronously, the top level code here is a excuted before any > async events. > > (In the future we should probably use promises for onLoaded events and similar > to avoid confusion and boilerplate though) Done. https://codereview.adblockplus.org/29332492/diff/29332508/safari/ext/common.js File safari/ext/common.js (right): https://codereview.adblockplus.org/29332492/diff/29332508/safari/ext/common.j... safari/ext/common.js:223: ext.setUninstallURL = function (url) { }; On 2015/12/12 00:31:09, Sebastian Noack wrote: > This is dead code. Dead code is bad, regardless how trivial it is, as its never > executed/tested. > > Given the current scheme, I also wonder whether we should just use > chrome.runtime.setUninstallURL directly in unistall.js for simplicity. Done.
https://codereview.adblockplus.org/29332492/diff/29332575/lib/uninstall.js File lib/uninstall.js (right): https://codereview.adblockplus.org/29332492/diff/29332575/lib/uninstall.js#ne... lib/uninstall.js:26: for (let key of ["addonName", "addonVersion", "application", I just rembered that jshydra generates quite crappy code if you use array literals in for-of loops: for (var _loopIndex55 = 0; _loopIndex55 < ["addonName", "addonVersion", "application", "applicationVersion", "platform", "platformVersion"].length; ++_loopIndex55) { var key = ["addonName", "addonVersion", "application", "applicationVersion", "platform", "platformVersion"][_loopIndex55]; search.push(key + "=" + encodeURIComponent(info[key])); } It basically duplicates the code in the of clause. So I suggest to store that array in a variable. Or alternatively, just omit the loop like we do in adblockplus/lib/downloader.js. https://codereview.adblockplus.org/29332492/diff/29332575/lib/uninstall.js#ne... lib/uninstall.js:34: require("utils").Utils.getDocLink("uninstall-abp") + "&" + search.join("&") As mentioned in the issue, I think we should just call it "uninstalled". https://codereview.adblockplus.org/29332492/diff/29332575/lib/uninstall.js#ne... lib/uninstall.js:34: require("utils").Utils.getDocLink("uninstall-abp") + "&" + search.join("&") I'd rather keep imports at the top, even if only used once, if it's only for consistency. But that way it'd be also more obvious which module depends on which other module.
Patch Set 5 : Addressed more feedback https://codereview.adblockplus.org/29332492/diff/29332575/lib/uninstall.js File lib/uninstall.js (right): https://codereview.adblockplus.org/29332492/diff/29332575/lib/uninstall.js#ne... lib/uninstall.js:26: for (let key of ["addonName", "addonVersion", "application", On 2015/12/12 13:43:27, Sebastian Noack wrote: > I just rembered that jshydra generates quite crappy code if you use array > literals in for-of loops: > > for (var _loopIndex55 = 0; _loopIndex55 < ["addonName", "addonVersion", > "application", "applicationVersion", "platform", "platformVersion"].length; > ++_loopIndex55) > { > var key = ["addonName", "addonVersion", "application", "applicationVersion", > "platform", "platformVersion"][_loopIndex55]; > search.push(key + "=" + encodeURIComponent(info[key])); > } > > It basically duplicates the code in the of clause. So I suggest to store that > array in a variable. Or alternatively, just omit the loop like we do in > adblockplus/lib/downloader.js. Done. https://codereview.adblockplus.org/29332492/diff/29332575/lib/uninstall.js#ne... lib/uninstall.js:34: require("utils").Utils.getDocLink("uninstall-abp") + "&" + search.join("&") On 2015/12/12 13:43:27, Sebastian Noack wrote: > I'd rather keep imports at the top, even if only used once, if it's only for > consistency. But that way it'd be also more obvious which module depends on > which other module. Done. https://codereview.adblockplus.org/29332492/diff/29332575/lib/uninstall.js#ne... lib/uninstall.js:34: require("utils").Utils.getDocLink("uninstall-abp") + "&" + search.join("&") On 2015/12/12 13:43:27, Sebastian Noack wrote: > As mentioned in the issue, I think we should just call it "uninstalled". Done. (Provisionally, if they disagree on the issue we might need to change it back.)
LGTM. But before merging this we should have the redirect in place of course.
https://codereview.adblockplus.org/29332492/diff/29332584/lib/uninstall.js File lib/uninstall.js (right): https://codereview.adblockplus.org/29332492/diff/29332584/lib/uninstall.js#ne... lib/uninstall.js:35: chrome.runtime.setUninstallURL(Utils.getDocLink("uninstalled") + "&" + How I could I miss that? Of course, we have to check for the existence of that API before calling it for compatibility with older Chrome versions.
Patch Set 6 : Check the API exists before using it (Other changes due to rebase onto CSS property filter changes. Sorry I forgot to upload those separately.) https://codereview.adblockplus.org/29332492/diff/29332584/lib/uninstall.js File lib/uninstall.js (right): https://codereview.adblockplus.org/29332492/diff/29332584/lib/uninstall.js#ne... lib/uninstall.js:35: chrome.runtime.setUninstallURL(Utils.getDocLink("uninstalled") + "&" + On 2015/12/15 06:41:18, Sebastian Noack wrote: > How I could I miss that? Of course, we have to check for the existence of that > API before calling it for compatibility with older Chrome versions. Done.
LGTM 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
