Opened on 09/22/2017 at 10:23:06 AM
Closed on 06/04/2018 at 06:12:04 PM
#5750 closed change (fixed)
Remove workarounds required for legacy Firefox support
| Reported by: | kzar | Assignee: | |
|---|---|---|---|
| Priority: | P3 | Milestone: | |
| Module: | User-Interface | Keywords: | |
| Cc: | greiner, sebastian, tlucas | Blocked By: | |
| Blocking: | Platform: | Unknown / Cross platform | |
| Ready: | yes | Confidential: | no |
| Tester: | Unknown | Verified working: | no |
| Review URL(s): |
https://codereview.adblockplus.org/29552642/ |
||
Description
Background
adblockplusui/messageResponder.js conditionally requires some modules. This is so that modules for the Chrome extension can be required without breaking Firefox compatability.
These conditional requires lead to webpack throwing "Critical dependency" warnings and I believe leave to potentially unnecessary code being bundled. Legacy Firefox compatibility is no longer required with the move to web extensions so we should probably remove them.
What to change
- Make all the module requires in messageResponder.js unconditional, avoiding the "Critical depdendency" warnings. (Something like this.)
- Remove any other workarounds which are only required for the legacy Firefox extension.
Attachments (0)
Change History (14)
comment:1 Changed on 09/22/2017 at 10:41:53 AM by greiner
- Priority changed from Unknown to P3
- Ready set
- Summary changed from Remove option requires required for Firefox support from messageResponder.js to Remove optional requires required for Firefox support from messageResponder.js
comment:2 follow-up: ↓ 3 Changed on 09/22/2017 at 12:47:16 PM by kzar
- Review URL(s) modified (diff)
- Status changed from new to reviewing
comment:3 in reply to: ↑ 2 Changed on 09/22/2017 at 01:27:59 PM by greiner
Replying to kzar:
Any idea if we can remove the line commented as a Firefox workaround in skin/mobile-options.css?
AFAIK there's nothing related to the legacy Firefox extension in the code for either the mobile or the new options page because those were created with only web extensions in mind.
comment:4 Changed on 09/22/2017 at 01:51:53 PM by kzar
Cool OK, so I think the only other question is the Firefox workarounds in adblockplusui/common.js for the openSharePopup function.
comment:5 Changed on 09/22/2017 at 02:03:01 PM by kzar
- Summary changed from Remove optional requires required for Firefox support from messageResponder.js to Remove workarounds required for legacy Firefox support
comment:6 Changed on 09/22/2017 at 03:28:36 PM by abpbot
A commit referencing this issue has landed:
Issue 5750 - Always require subscriptionInit, avoid "critical dependency"
comment:7 Changed on 09/22/2017 at 03:29:22 PM by kzar
- Status changed from reviewing to reopened
comment:8 Changed on 09/25/2017 at 02:32:01 PM by sebastian
- Review URL(s) modified (diff)
- Status changed from reopened to reviewing
comment:9 Changed on 09/25/2017 at 03:30:23 PM by abpbot
A commit referencing this issue has landed:
Issue 5750 - Remove ext_background requires needed for legacy Gecko
comment:10 Changed on 09/25/2017 at 03:31:18 PM by kzar
- Status changed from reviewing to reopened
comment:11 Changed on 10/10/2017 at 10:17:26 AM by kzar
- Blocking 5080 removed
comment:12 Changed on 06/01/2018 at 04:31:21 PM by greiner
@kzar Is there anything left to do here or can we close this ticket?
comment:13 Changed on 06/04/2018 at 05:56:09 PM by kzar
comment:14 Changed on 06/04/2018 at 06:12:04 PM by greiner
- Resolution set to fixed
- Status changed from reopened to closed
Sounds reasonable. Thanks for checking.
Any idea if we can remove the line commented as a Firefox workaround in skin/mobile-options.css?