Add pollyfill for tabs.remove

[saroyanm]

Background

Currently, our ​Web Extension API polyfill doesn't wrap browser.tabs.remove(). Hence it doesn't return a promise. While we don't have any need to respond to the asynchrnous result of tabs.remove() at the moment, and the polyfill isn't meant to be feature complete, we use tabs.remove() in some places, and in order to avoid confusion we decided to add it to the polyfill.

What to change

Add a pollyfill for the tabs.remove method in order to return promise in Chrome.

[saroyanm]

Note: I can prepare a patch when this ticket gets ready.

[sebastian]

At the moment, we don't have any code that needs to repsond to the result of tabs.remove(). Once we have a use case we can add it to the polyfill. But from #6386 it isn't clear to me yet what do you need that for.

[saroyanm]

Replying to sebastian:

At the moment, we don't have any code that needs to repsond to the result of tabs.remove(). Once we have a use case we can add it to the polyfill. But from #6386 it isn't clear to me yet what do you need that for.

Sorry, I should have mentioned that initially. I thought we were providing complete pollyfill for the webextenisons API.
We need it in order to use platform independant ​callback in the Issue Reporter. I added link to the description as well,

[mjethani]

I've submitted a patch.

[sebastian]

I still don't get what are you doing there? Why is it important to wait until the browser reports back after requesting the tab to close? We don't care anywhere else where we close any tabs.

Regardless whether there is a legit reason for that or not, I don't think that we have to extend the polyfill prematurely. This should rather be an integration note on the issue integrating the respective UI change.

[saroyanm]

Replying to sebastian:

I still don't get what are you doing there? Why is it important to wait until the browser reports back after requesting the tab to close?

The idea is to collect requests and filterhit data until user click on Show report data on the comments page, according to the ​specification this should close the dataGathering tab, so the idea is to open the Show report data dialog only after the data gathering tab is closed in order to provide right information to the user about what data has been collected so far.

[sebastian]

Again, why do you have to wait for the browser to report back that the tab has been closed? This seems to only add unnecessary complexity and adds some (potentially visible) delay. Why don't you just show the dialog right after you call tabs.remove()?

[saroyanm]

Replying to sebastian:

Again, why do you have to wait for the browser to report back that the tab has been closed? This seems to only add unnecessary complexity and adds some (potentially visible) delay. Why don't you just show the dialog right after you call tabs.remove()?

From the issue comment:
browser.tabs.remove() is asynchronous which means that, theoretically, a request could be added even after we unhide the data overlay

[greiner]

I don't know why this would be worth debating in the context of Platform.

We want to be absolutely sure that we're not recording anything anymore before we show the dialog. While this could be achieved by stopping our listener, that seems redundant given that we need to close the tab anyway.

[sebastian]

Yes, unregistering your listener is the right thing to do here. That not only reliably makes sure that you won't record any further events, but also avoids the listener from leaking.

[saroyanm]

Replying to sebastian:

Yes, unregistering your listener is the right thing to do here. That not only reliably makes sure that you won't record any further events, but also avoids the listener from leaking.

That's an option as well.
FWIW: I'm unregistering the listener anyway when the tab is closed, considering the logic that the data gathering should be registered during the tab lifecycle. Closing tab sounds simpler approach to me.

[sebastian]

Note that currently, as of the current ​work in progress, of the hitLogger API, there is no code cleaning up listeners when the tab is invalidated. FWIW the definition of a tab's life-cycle is ambiguous (is a tab valid untill its closed? or until navigation occurs? what is about pre-rendered tabs?). So it might be best to leave this to the caller. Furthermore, this API is still matter to change, so we might as well end up passing in the tabId to the listener rather than registering listeners for a specific tab. Either way, for now, you should assume that you have to unregister the listener, and if you do anyway, there is not much of a point to wait for completion of tabs.remove().

[sebastian]

As per the discussion in the ​code review we decided to make tabs.remove() return a promise, for consistency. Still as per the above discussion, I think it should not be relied on for the issue reporter.

[abpbot]

A commit referencing this issue has landed:
​Issue 6582 - Wrap tabs.remove

[saroyanm]

Replying to sebastian:

Either way, for now, you should assume that you have to unregister the listener, and if you do anyway, there is not much of a point to wait for completion of tabs.remove().

Yes, I'm unregistering on ​tabs.onRemove in messageResponder.

Still as per the above discussion, I think it should not be relied on for the issue reporter

I think this can be discussed further in the issue itself, as I do unregister the listener when the tab is removed, so this logic I think still can rely on removing the tab, but if you and Thomas think my current implementation is wrong I'll change that.

As per the discussion in the ​code review we decided to make tabs.remove() return a promise

Sounds good

[saroyanm]

Anyway I made a clarification comment, but can't tag you @sebastian in the gitlab, see -> ​https://gitlab.com/eyeo/adblockplus/adblockplusui/merge_requests/6#note_67517838

[Ross]

Done. Does not look to have caused any regressions/issues. Issue reporter is working as expected.

ABP 3.0.4.2034
Firefox 59 / 55 / 51 / Windows 10
Chrome 66 / 58 / 49 / Windows 7
Opera 52 / 45 / 36 / Windows 10