Issue 29829611: Issue 6786 - Display the Safari App Extension migration page (Closed)
|
Created:
July 13, 2018, 5:36 p.m. by kzar Modified:
July 13, 2018, 8:16 p.m. Reviewers:
Sebastian Noack Visibility:
Public. |
DescriptionIssue 6786 - Display the Safari App Extension migration page
Patch Set 1 #
Total comments: 7
Patch Set 2 : Addressed Sebastian's feedback #Patch Set 3 : Only retry on failure #MessagesTotal messages: 11
Patch Set 1
https://codereview.adblockplus.org/29829611/diff/29829612/background.js File background.js (right): https://codereview.adblockplus.org/29829611/diff/29829612/background.js#newco... background.js:72: fetch("https://eyeo.to/adblockplus/safari-app-extension-migration") I wonder, should we perhaps just send a HEAD request instead of downloading the whole document? https://codereview.adblockplus.org/29829611/diff/29829612/background.js#newco... background.js:84: window.setInterval(showMigrationPageWhenReady, 1000 * 60 * 60 * 24); As far as I understand the issue description, we only want to try again in 24h if the page wasn't online yet. Also then we don't have to bother about the scenario in which the tab is already open, just using ext.pages.open() here, and don't need to touch (and re-test) the logic showing the options page. Furthermore, we might want to call setTimeout() (recursively) instead of using setInterval(). That is because if an interval is missed while your computer was in standby/sleep it is skipped, however if a timeout was missed, it is performed as soon as you resume from standby.
https://codereview.adblockplus.org/29829611/diff/29829612/safari/ext/backgrou... File safari/ext/background.js (right): https://codereview.adblockplus.org/29829611/diff/29829612/safari/ext/backgrou... safari/ext/background.js:598: if (page.url.href == url && tab.browserWindow == safari.application.activeBrowserWindow) I just figured, this wouldn't work anyway as expected if a redirect is involved.
https://codereview.adblockplus.org/29829611/diff/29829612/safari/ext/backgrou... File safari/ext/background.js (right): https://codereview.adblockplus.org/29829611/diff/29829612/safari/ext/backgrou... safari/ext/background.js:598: if (page.url.href == url && tab.browserWindow == safari.application.activeBrowserWindow) On 2018/07/13 18:04:18, Sebastian Noack wrote: > I just figured, this wouldn't work anyway as expected if a redirect is involved. Ignore that comment. It actually would work, since you pass in the destination URL determined through the fetch() API.
Patch Set 2 : Addressed Sebastian's feedback https://codereview.adblockplus.org/29829611/diff/29829612/background.js File background.js (right): https://codereview.adblockplus.org/29829611/diff/29829612/background.js#newco... background.js:72: fetch("https://eyeo.to/adblockplus/safari-app-extension-migration") On 2018/07/13 18:01:35, Sebastian Noack wrote: > I wonder, should we perhaps just send a HEAD request instead of downloading the > whole document? Done. https://codereview.adblockplus.org/29829611/diff/29829612/background.js#newco... background.js:84: window.setInterval(showMigrationPageWhenReady, 1000 * 60 * 60 * 24); On 2018/07/13 18:01:35, Sebastian Noack wrote: > As far as I understand the issue description, we only want to try again in 24h > if the page wasn't online yet. Also then we don't have to bother about the > scenario in which the tab is already open, just using ext.pages.open() here, and > don't need to touch (and re-test) the logic showing the options page. I've adjusted the issue description to clarify. > Furthermore, we might want to call setTimeout() (recursively) instead of using > setInterval(). That is because if an interval is missed while your computer was > in standby/sleep it is skipped, however if a timeout was missed, it is performed > as soon as you resume from standby. Good point, done. https://codereview.adblockplus.org/29829611/diff/29829612/safari/ext/backgrou... File safari/ext/background.js (right): https://codereview.adblockplus.org/29829611/diff/29829612/safari/ext/backgrou... safari/ext/background.js:598: if (page.url.href == url && tab.browserWindow == safari.application.activeBrowserWindow) On 2018/07/13 18:06:37, Sebastian Noack wrote: > On 2018/07/13 18:04:18, Sebastian Noack wrote: > > I just figured, this wouldn't work anyway as expected if a redirect is > involved. > > Ignore that comment. It actually would work, since you pass in the destination > URL determined through the fetch() API. Yes, I originally made that mistake but noticed it during testing.
The changes look good to me, if we really want to keep showing that page every 24h. I'm not sure though if this might be a little to intrusive, in particular since the extension will technically keep working (even on Safari 12 with your other change). Also perhaps one more scenario to consider, what's about new users installing the extension? With the current approach, they would get both the first run page and the migration page (if online). I wonder if it would be worth/desirable to only show the latter to them?
On 2018/07/13 18:50:00, Sebastian Noack wrote: > The changes look good to me, if we really want to keep showing that page every > 24h. I'm not sure though if this might be a little to intrusive, in particular > since the extension will technically keep working (even on Safari 12 with your > other change). Fair enough, do you suggest once a week or a different interval? I'd like to agree on something and release it soon, since we're running out of time. > Also perhaps one more scenario to consider, what's about new users installing > the extension? With the current approach, they would get both the first run page > and the migration page (if online). I wonder if it would be worth/desirable to > only show the latter to them? Don't we have to show the first run page for legal reasons, e.g. to explain about AA? I think it's more important to get something released than to worry about this edge case anyway.
On 2018/07/13 18:50:00, Sebastian Noack wrote: > The changes look good to me, if we really want to keep showing that page every > 24h. I'm not sure though if this might be a little to intrusive, in particular > since the extension will technically keep working (even on Safari 12 with your > other change). Fair enough, do you suggest once a week or a different interval? I'd like to agree on something and release it soon, since we're running out of time. > Also perhaps one more scenario to consider, what's about new users installing > the extension? With the current approach, they would get both the first run page > and the migration page (if online). I wonder if it would be worth/desirable to > only show the latter to them? Don't we have to show the first run page for legal reasons, e.g. to explain about AA? I think it's more important to get something released than to worry about this edge case anyway.
On 2018/07/13 19:02:13, kzar wrote: > Fair enough, do you suggest once a week or a different interval? I'd like to > agree on something and release it soon, since we're running out of time. Well, my suggestion is to stick with 24h, but stop as soon as the page has been showed once (in this browsing session), like originally specified in the issue. > Don't we have to show the first run page for legal reasons, e.g. to explain > about AA? I think it's more important to get something released than to worry > about this edge case anyway. I don't think that this matters much if we tell the user instead to NOT use this but another product, not to mention that nobody will look at the first run page anyway if the migration page pops up a second later pushing the first run page into the background. But yeah, it's probably not too important to bother about that scenario.
Patch Set 3 : Only retry on failure
LGTM 
|
