Issue 29339314: Issue 3870 - Rewrite legacy options page to use async messages (Closed)
|
Created:
April 3, 2016, 10:51 a.m. by kzar Modified:
April 7, 2016, 2:04 p.m. Reviewers:
Sebastian Noack Visibility:
Public. |
DescriptionIssue 3870 - Rewrite legacy options page to use async messages
Patch Set 1 #
Total comments: 19
Patch Set 2 : Addressed initial feedback #Patch Set 3 : Fix Downloading... indicators #
Total comments: 13
Patch Set 4 : Removed redundant spaces and imports, fixed some typos #Patch Set 5 : Handle subscription events properly ("Downloading...") #
Total comments: 2
Patch Set 6 : Make use of the subscription.isDownloading property #
Total comments: 4
Patch Set 7 : Update for adblockplusui changes and work around around a race condition with Downloading... label #
Total comments: 2
Patch Set 8 : Fetch acceptable ads URL once initially and re-use #
Total comments: 2
Patch Set 9 : Avoid another race condition #
MessagesTotal messages: 22
Patch Set 1 https://codereview.adblockplus.org/29339314/diff/29339315/options.js File options.js (right): https://codereview.adblockplus.org/29339314/diff/29339315/options.js#newcode57 options.js:57: ext.backgroundPage.sendMessage(message, callback); (I considered having this function return a promise instead of taking a callback, but it didn't seem to improve anything.)
https://codereview.adblockplus.org/29339314/diff/29339315/options.js File options.js (left): https://codereview.adblockplus.org/29339314/diff/29339315/options.js#oldcode15 options.js:15: * along with Adblock Plus. If not, see <http://www.gnu.org/licenses/>. This rewrite is a good opportunity to turn on strict mode for this script. https://codereview.adblockplus.org/29339314/diff/29339315/options.js#oldcode180 options.js:180: var userFilters = backgroundPage.getUserFilters(); How about removing this function from background.js? https://codereview.adblockplus.org/29339314/diff/29339315/options.js File options.js (right): https://codereview.adblockplus.org/29339314/diff/29339315/options.js#newcode61 options.js:61: var getDocLink = wrapper({type: "app.get", what: "doclink"}, "link"); This is redundant with getDocLink() from common.js https://codereview.adblockplus.org/29339314/diff/29339315/options.js#newcode148 options.js:148: getInfo("platform", function (platform) Please use app.get[features] like the new options page does. https://codereview.adblockplus.org/29339314/diff/29339315/options.js#newcode496 options.js:496: if (key == "notifications_showui") Nit: There will be soon a third case for safari_content_blockers. So how about using a switch/case like here like I did for the new options page? https://codereview.adblockplus.org/29339314/diff/29339315/options.js#newcode749 options.js:749: case "focus-section": This should be handled the same way app.respond[addSubscription]. I created a sperate issue and wrote a patch to account for that in the message responder and new options page: https://issues.adblockplus.org/ticket/3887
https://codereview.adblockplus.org/29339314/diff/29339315/options.js File options.js (right): https://codereview.adblockplus.org/29339314/diff/29339315/options.js#newcode669 options.js:669: isSubscriptionDownloading(subscriptions[i].url, function (isDownloading) Nit: Redundant space after function statement.
https://codereview.adblockplus.org/29339314/diff/29339315/options.js File options.js (right): https://codereview.adblockplus.org/29339314/diff/29339315/options.js#newcode672 options.js:672: label.textContent = inProgressMessage; This will always be the label of the last subscription.
Patch Set 2 : Addressed initial feedback https://codereview.adblockplus.org/29339314/diff/29339315/options.js File options.js (left): https://codereview.adblockplus.org/29339314/diff/29339315/options.js#oldcode15 options.js:15: * along with Adblock Plus. If not, see <http://www.gnu.org/licenses/>. On 2016/04/03 15:05:14, Sebastian Noack wrote: > This rewrite is a good opportunity to turn on strict mode for this script. Done. https://codereview.adblockplus.org/29339314/diff/29339315/options.js#oldcode180 options.js:180: var userFilters = backgroundPage.getUserFilters(); On 2016/04/03 15:05:14, Sebastian Noack wrote: > How about removing this function from background.js? Done. https://codereview.adblockplus.org/29339314/diff/29339315/options.js File options.js (right): https://codereview.adblockplus.org/29339314/diff/29339315/options.js#newcode61 options.js:61: var getDocLink = wrapper({type: "app.get", what: "doclink"}, "link"); On 2016/04/03 15:05:14, Sebastian Noack wrote: > This is redundant with getDocLink() from common.js I don't think adblockplusui/common.js is included by the legacy options page. https://codereview.adblockplus.org/29339314/diff/29339315/options.js#newcode148 options.js:148: getInfo("platform", function (platform) On 2016/04/03 15:05:14, Sebastian Noack wrote: > Please use app.get[features] like the new options page does. Done. https://codereview.adblockplus.org/29339314/diff/29339315/options.js#newcode496 options.js:496: if (key == "notifications_showui") On 2016/04/03 15:05:14, Sebastian Noack wrote: > Nit: There will be soon a third case for safari_content_blockers. So how about > using a switch/case like here like I did for the new options page? Done. https://codereview.adblockplus.org/29339314/diff/29339315/options.js#newcode669 options.js:669: isSubscriptionDownloading(subscriptions[i].url, function (isDownloading) On 2016/04/03 15:11:41, Sebastian Noack wrote: > Nit: Redundant space after function statement. Done. https://codereview.adblockplus.org/29339314/diff/29339315/options.js#newcode749 options.js:749: case "focus-section": On 2016/04/03 15:05:14, Sebastian Noack wrote: > This should be handled the same way app.respond[addSubscription]. I created a > sperate issue and wrote a patch to account for that in the message responder and > new options page: https://issues.adblockplus.org/ticket/3887 Done. (Not tested yet as leaving for airport shortly.)
Patch Set 3 : Fix Downloading... indicators https://codereview.adblockplus.org/29339314/diff/29339315/options.js File options.js (right): https://codereview.adblockplus.org/29339314/diff/29339315/options.js#newcode672 options.js:672: label.textContent = inProgressMessage; On 2016/04/03 15:27:40, Sebastian Noack wrote: > This will always be the label of the last subscription. Oops, you're right. Done.
https://codereview.adblockplus.org/29339314/diff/29339315/options.js File options.js (right): https://codereview.adblockplus.org/29339314/diff/29339315/options.js#newcode61 options.js:61: var getDocLink = wrapper({type: "app.get", what: "doclink"}, "link"); On 2016/04/03 15:50:19, kzar wrote: > On 2016/04/03 15:05:14, Sebastian Noack wrote: > > This is redundant with getDocLink() from common.js > > I don't think adblockplusui/common.js is included by the legacy options page. Given that common.js is already used by the first run page, you might just want make the options page use it as well. However, if it would only be for that function I wouldn't insist. https://codereview.adblockplus.org/29339314/diff/29339334/options.js File options.js (left): https://codereview.adblockplus.org/29339314/diff/29339334/options.js#oldcode664 options.js:664: function updateFilterLists() So the download status is only updated when a manual subscription update is triggered? That seems to be a regression. https://codereview.adblockplus.org/29339314/diff/29339334/options.js File options.js (right): https://codereview.adblockplus.org/29339314/diff/29339334/options.js#newcode97 options.js:97: getDocLink("acceptable_ads", function (url) Not: Redundant space after function statement, not only in this line. https://codereview.adblockplus.org/29339314/diff/29339334/options.js#newcode643 options.js:643: importRawFilters(text, function (errors) You have to specify removeExisting when using the filter.importRaw message here. https://codereview.adblockplus.org/29339314/diff/29339334/options.js#newcode758 options.js:758: "[data-section='" + message.section + "']" It will be message.args[0], not message.section.
https://codereview.adblockplus.org/29339314/diff/29339334/background.js File background.js (left): https://codereview.adblockplus.org/29339314/diff/29339334/background.js#oldco... background.js:39: for (var i = 0; i < FilterStorage.subscriptions.length; i++) Please remove the imports for names no longer used in this file.
Patch Set 4 : Removed redundant spaces and imports, fixed some typos https://codereview.adblockplus.org/29339314/diff/29339315/options.js File options.js (right): https://codereview.adblockplus.org/29339314/diff/29339315/options.js#newcode61 options.js:61: var getDocLink = wrapper({type: "app.get", what: "doclink"}, "link"); On 2016/04/03 17:18:03, Sebastian Noack wrote: > On 2016/04/03 15:50:19, kzar wrote: > > On 2016/04/03 15:05:14, Sebastian Noack wrote: > > > This is redundant with getDocLink() from common.js > > > > I don't think adblockplusui/common.js is included by the legacy options page. > > Given that common.js is already used by the first run page, you might just want > make the options page use it as well. However, if it would only be for that > function I wouldn't insist. I think I prefer to leave this as it is. https://codereview.adblockplus.org/29339314/diff/29339334/background.js File background.js (left): https://codereview.adblockplus.org/29339314/diff/29339334/background.js#oldco... background.js:39: for (var i = 0; i < FilterStorage.subscriptions.length; i++) On 2016/04/03 18:19:59, Sebastian Noack wrote: > Please remove the imports for names no longer used in this file. Done. https://codereview.adblockplus.org/29339314/diff/29339334/options.js File options.js (left): https://codereview.adblockplus.org/29339314/diff/29339334/options.js#oldcode664 options.js:664: function updateFilterLists() On 2016/04/03 17:18:04, Sebastian Noack wrote: > So the download status is only updated when a manual subscription update is > triggered? That seems to be a regression. Yep, sorry I forgot to mention that in the review. (As we discussed in IRC it also looks like a limitation with the new options page, I'm investigating that now.) https://codereview.adblockplus.org/29339314/diff/29339334/options.js File options.js (right): https://codereview.adblockplus.org/29339314/diff/29339334/options.js#newcode97 options.js:97: getDocLink("acceptable_ads", function (url) On 2016/04/03 17:18:04, Sebastian Noack wrote: > Not: Redundant space after function statement, not only in this line. Done. https://codereview.adblockplus.org/29339314/diff/29339334/options.js#newcode643 options.js:643: importRawFilters(text, function (errors) On 2016/04/03 17:18:04, Sebastian Noack wrote: > You have to specify removeExisting when using the filter.importRaw message here. Done. https://codereview.adblockplus.org/29339314/diff/29339334/options.js#newcode758 options.js:758: "[data-section='" + message.section + "']" On 2016/04/03 17:18:04, Sebastian Noack wrote: > It will be message.args[0], not message.section. Done.
https://codereview.adblockplus.org/29339314/diff/29339334/options.js File options.js (left): https://codereview.adblockplus.org/29339314/diff/29339334/options.js#oldcode664 options.js:664: function updateFilterLists() On 2016/04/05 11:10:15, kzar wrote: > On 2016/04/03 17:18:04, Sebastian Noack wrote: > > So the download status is only updated when a manual subscription update is > > triggered? That seems to be a regression. > > Yep, sorry I forgot to mention that in the review. (As we discussed in IRC it > also looks like a limitation with the new options page, I'm investigating that > now.) The problem with the new options page seems to be unrelated. For some reason Manvel made things more complicated as necessary using Object.observe() rather than just responding to subscription changes. And the way he did it the code updating the UI when the download status changed doesn't get triggered. I already started to work on fixing that. If you simply update the download status on the corresponding subscription change, rather than doing it when a manual update is triggered, everything should just work as before with the old options page.
https://codereview.adblockplus.org/29339314/diff/29339334/options.js File options.js (left): https://codereview.adblockplus.org/29339314/diff/29339334/options.js#oldcode664 options.js:664: function updateFilterLists() On 2016/04/05 12:26:15, Sebastian Noack wrote: > On 2016/04/05 11:10:15, kzar wrote: > > On 2016/04/03 17:18:04, Sebastian Noack wrote: > > > So the download status is only updated when a manual subscription update is > > > triggered? That seems to be a regression. > > > > Yep, sorry I forgot to mention that in the review. (As we discussed in IRC it > > also looks like a limitation with the new options page, I'm investigating that > > now.) > > The problem with the new options page seems to be unrelated. For some reason > Manvel made things more complicated as necessary using Object.observe() rather > than just responding to subscription changes. And the way he did it the code > updating the UI when the download status changed doesn't get triggered. I > already started to work on fixing that. > > If you simply update the download status on the corresponding subscription > change, rather than doing it when a manual update is triggered, everything > should just work as before with the old options page. Yea, I figured that out already, well apart from I didn't check `git blame` :p There is one problem however, the downloadStatus event messages kind of suck at the moment. A "synchronize_ok" event is dispatched both when the download starts and when it finishes! (Assuming it was successful anyway.) I can hack around this by checking the lastDownloaded timestamp but I'd rather fix the messages instead. I think that the downloadStatus should be "in_progress" when the download has started, I'm going to see what Wladimir thinks as it's a core change.
https://codereview.adblockplus.org/29339314/diff/29339334/options.js File options.js (left): https://codereview.adblockplus.org/29339314/diff/29339334/options.js#oldcode664 options.js:664: function updateFilterLists() On 2016/04/05 12:36:53, kzar wrote: > On 2016/04/05 12:26:15, Sebastian Noack wrote: > > On 2016/04/05 11:10:15, kzar wrote: > > > On 2016/04/03 17:18:04, Sebastian Noack wrote: > > > > So the download status is only updated when a manual subscription update > is > > > > triggered? That seems to be a regression. > > > > > > Yep, sorry I forgot to mention that in the review. (As we discussed in IRC > it > > > also looks like a limitation with the new options page, I'm investigating > that > > > now.) > > > > The problem with the new options page seems to be unrelated. For some reason > > Manvel made things more complicated as necessary using Object.observe() rather > > than just responding to subscription changes. And the way he did it the code > > updating the UI when the download status changed doesn't get triggered. I > > already started to work on fixing that. > > > > If you simply update the download status on the corresponding subscription > > change, rather than doing it when a manual update is triggered, everything > > should just work as before with the old options page. > > Yea, I figured that out already, well apart from I didn't check `git blame` :p > > There is one problem however, the downloadStatus event messages kind of suck at > the moment. A "synchronize_ok" event is dispatched both when the download starts > and when it finishes! (Assuming it was successful anyway.) I can hack around > this by checking the lastDownloaded timestamp but I'd rather fix the messages > instead. I think that the downloadStatus should be "in_progress" when the > download has started, I'm going to see what Wladimir thinks as it's a core > change. I think the solution is way simpler. Simply update the download status here when either subscription.downloadStatus or subscription.downloadCount is triggered. If I understand correctly, that is how it worked for the old options page. Not sure whether it's intended whether synchronize_ok is dispatched in both cases though.
Handle subscription events properly ("Downloading...")
https://codereview.adblockplus.org/29339314/diff/29339400/options.js File options.js (right): https://codereview.adblockplus.org/29339314/diff/29339400/options.js#newcode467 options.js:467: downloading = true; What if the subscription is already/still downloading when the options page is loaded, or when another subscription change occurs while it's still downloading? You have to always check whether the subscription is downloading regardless of the occurred event when updating it.
Patch Set 6 : Make use of the subscription.isDownloading property https://codereview.adblockplus.org/29339314/diff/29339400/options.js File options.js (right): https://codereview.adblockplus.org/29339314/diff/29339400/options.js#newcode467 options.js:467: downloading = true; On 2016/04/06 08:50:17, Sebastian Noack wrote: > What if the subscription is already/still downloading when the options page is > loaded, or when another subscription change occurs while it's still downloading? > You have to always check whether the subscription is downloading regardless of > the occurred event when updating it. Done.
https://codereview.adblockplus.org/29339314/diff/29339411/dependencies File dependencies (right): https://codereview.adblockplus.org/29339314/diff/29339411/dependencies#newcode6 dependencies:6: adblockplusui = adblockplusui hg:667a6e6f5388 git:a0fb5d6 I guess this needs to be updated again. https://codereview.adblockplus.org/29339314/diff/29339411/options.js File options.js (right): https://codereview.adblockplus.org/29339314/diff/29339411/options.js#newcode728 options.js:728: case "switchToOptionsSection": As per Thomas' suggestion I changed that message to "focusSection" for the new options page. So we should adapt the code here as well.
Patch Set 7 : Update for adblockplusui changes and work around around a race condition with Downloading... label https://codereview.adblockplus.org/29339314/diff/29339411/dependencies File dependencies (right): https://codereview.adblockplus.org/29339314/diff/29339411/dependencies#newcode6 dependencies:6: adblockplusui = adblockplusui hg:667a6e6f5388 git:a0fb5d6 On 2016/04/06 23:00:18, Sebastian Noack wrote: > I guess this needs to be updated again. Done. https://codereview.adblockplus.org/29339314/diff/29339411/options.js File options.js (right): https://codereview.adblockplus.org/29339314/diff/29339411/options.js#newcode728 options.js:728: case "switchToOptionsSection": On 2016/04/06 23:00:18, Sebastian Noack wrote: > As per Thomas' suggestion I changed that message to "focusSection" for the new > options page. So we should adapt the code here as well. Done.
https://codereview.adblockplus.org/29339314/diff/29339555/options.js File options.js (right): https://codereview.adblockplus.org/29339314/diff/29339555/options.js#newcode480 options.js:480: subscription.isDownloading = true; That is because you asynchronously retrieve the Acceptable Ads URL. However, while you wait for a response, the "downloading" notification already arrives. Note that theoretically, other subscription changes might occur meanwhile as well, causing similar issues. How about simply retrieving the Acceptable Ads URL once as very first thing you do when the options page loads, and caching it in a global variable? That would not only make this hack redundant, but also reduces the total amount of messages sent and simplifies the code a bit.
Patch Set 8 : Fetch acceptable ads URL once initially and re-use https://codereview.adblockplus.org/29339314/diff/29339555/options.js File options.js (right): https://codereview.adblockplus.org/29339314/diff/29339555/options.js#newcode480 options.js:480: subscription.isDownloading = true; On 2016/04/07 12:49:14, Sebastian Noack wrote: > That is because you asynchronously retrieve the Acceptable Ads URL. However, > while you wait for a response, the "downloading" notification already arrives. > Note that theoretically, other subscription changes might occur meanwhile as > well, causing similar issues. > > How about simply retrieving the Acceptable Ads URL once as very first thing you > do when the options page loads, and caching it in a global variable? That would > not only make this hack redundant, but also reduces the total amount of messages > sent and simplifies the code a bit. Much better solution and works fine. Done.
https://codereview.adblockplus.org/29339314/diff/29339562/options.js File options.js (right): https://codereview.adblockplus.org/29339314/diff/29339562/options.js#newcode98 options.js:98: $("#acceptableAdsLink").attr("href", acceptableAdsUrl); We have a race here. Dependent on whether we first get a response with the Acceptable Ads URL, or whether the DOM is completed first the URL might be set correctly or not. If you simply move getPref("subscriptions_exceptionsurl", ..) here, and this line into it's callback, you would eliminate that race condition.
Patch Set 9 : Avoid another race condition https://codereview.adblockplus.org/29339314/diff/29339562/options.js File options.js (right): https://codereview.adblockplus.org/29339314/diff/29339562/options.js#newcode98 options.js:98: $("#acceptableAdsLink").attr("href", acceptableAdsUrl); On 2016/04/07 13:15:51, Sebastian Noack wrote: > We have a race here. Dependent on whether we first get a response with the > Acceptable Ads URL, or whether the DOM is completed first the URL might be set > correctly or not. > > If you simply move getPref("subscriptions_exceptionsurl", ..) here, and this > line into it's callback, you would eliminate that race condition. Done.
LGTM 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
