Opened 5 months ago

Closed 5 months ago

Last modified 5 months ago

#6582 closed change (fixed)

Add pollyfill for tabs.remove

Reported by: saroyanm Assignee:
Priority: P4 Milestone: Adblock-Plus-3.1-for-Chrome-Opera-Firefox
Module: Platform Keywords:
Cc: kzar, greiner, sebastian, mjethani Blocked By:
Blocking: Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Ross Verified working: yes
Review URL(s):

https://codereview.adblockplus.org/29753571/

Description (last modified by sebastian)

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.

Change History (19)

comment:1 Changed 5 months ago by saroyanm

  • Cc sebastian added

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

comment:2 follow-up: Changed 5 months ago by sebastian

  • Cc mjethani added

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.

comment:3 in reply to: ↑ 2 Changed 5 months ago by saroyanm

  • Description modified (diff)

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,

comment:4 Changed 5 months ago by saroyanm

  • Description modified (diff)

comment:5 Changed 5 months ago by mjethani

  • Review URL(s) modified (diff)

I've submitted a patch.

comment:6 follow-up: Changed 5 months ago by 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.

comment:7 in reply to: ↑ 6 Changed 5 months ago by 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.

comment:8 follow-up: Changed 5 months ago by 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()?

comment:9 in reply to: ↑ 8 Changed 5 months ago by 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

comment:10 Changed 5 months ago by 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.

comment:11 follow-up: Changed 5 months ago by 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.

comment:12 in reply to: ↑ 11 Changed 5 months ago by 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.

comment:13 follow-up: Changed 5 months ago by 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().

Last edited 5 months ago by sebastian (previous) (diff)

comment:14 Changed 5 months ago by sebastian

  • Blocking 6386 removed
  • Description modified (diff)
  • Priority changed from Unknown to P4
  • Ready set

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.

comment:15 Changed 5 months ago by abpbot

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

comment:16 Changed 5 months ago by sebastian

  • Milestone set to Adblock-Plus-for-Chrome-Opera-Firefox-next
  • Resolution set to fixed
  • Status changed from new to closed

comment:17 in reply to: ↑ 13 Changed 5 months ago by 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

comment:18 Changed 5 months ago by 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

comment:19 Changed 5 months ago by Ross

  • Tester changed from Unknown to Ross
  • Verified working set

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

Note: See TracTickets for help on using tickets.