1414018 - Redirect current tab to new New Tab after disabling New Tab extension in doorhanger

Status: VERIFIED FIXED

(WebExtensions :: Frontend, defect, P3)

Description

[Mark Striemer [:mstriemer]]

When the "Restore Settings" button is clicked in the New Tab doorhanger the current tab is (normally) closed. Instead we should replace the current tab with what the New Tab would be after the change.

Disabling an extension is supposed to close all of the tabs associated with that extension, which is why the current tab closes. To handle this we may need to:

1. Redirect the tab to about:blank so it doesn't close.
2. Disable the extension (the tab should stay open since it's at about:blank).
3. Once the New Tab Service has been updated, redirect to new tab.
4. Ensure the doorhanger appears for the next extension, if needed.

If we know what URL we need to redirect to beforehand we can likely just do steps 3 and 4 above.

Comment 1

[Mark Striemer [:mstriemer]]

Attachment: Bug 1414018 - Go to new New Tab page after disabling add-on in New Tab doorhanger

Review commit: https://reviewboard.mozilla.org/r/205218/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/205218/

Comment 2

[Mark Striemer [:mstriemer]]

Attachment: restore-settings-multiple.mov.gif

Here's how this would look if you install two New Tab extensions then open a new tab and restore both.

Comment 3

[Andrew Swan [:aswan]]

Comment on attachment 8934314 [details]
Bug 1414018 - Go to new New Tab page after disabling add-on in New Tab doorhanger

https://reviewboard.mozilla.org/r/205218/#review211594

::: browser/components/extensions/test/browser/browser_ext_url_overrides_newtab.js:42
(Diff revision 1)
>      }
>      Services.obs.addObserver(observer, eventName);
>    });
>  }
>  
> +function waitForURL(url) {

can you just use `BrowserTestUtils.browserLoaded()` or `BrowserTestUtils.waitForLocationChange()` here?

::: browser/components/extensions/test/browser/browser_ext_url_overrides_newtab.js:449
(Diff revision 1)
> +  // FIXME: We need to enable the add-on so it gets cleared from the
> +  // ExtensionSettingsStore for now. See bug 1408226.

didn't this get fixed with bug 1404584?

Comment 4

[Mark Striemer [:mstriemer]]

Comment on attachment 8934314 [details]
Bug 1414018 - Go to new New Tab page after disabling add-on in New Tab doorhanger

https://reviewboard.mozilla.org/r/205218/#review211594

> didn't this get fixed with bug 1404584?

This is still an issue because we're using `callOnClose()` to update `ExtensionSettingsStore`. That just maps to a shutdown listener which will only get called once. In that case if the extension is disabled then removed the store is still in the disabled state.

Comment 5

[Mark Striemer [:mstriemer]]

Comment on attachment 8934314 [details]
Bug 1414018 - Go to new New Tab page after disabling add-on in New Tab doorhanger

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/205218/diff/1-2/

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8d680183148c
Go to new New Tab page after disabling add-on in New Tab doorhanger r=aswan

Comment 7

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/8d680183148c

Comment 8

[marius.santa]

Attachment: tabs restore.gif

I can reproduce this issue on Firefox 58.0a1 (20171102222620) under Wind 10 64-bit. 

This issue is verified as fixed on Firefox 59.0a1 (20171227100314) under Wind 10 64-bit and Mac OS X 10.13.