Opened 11 months ago

Closed 2 months ago

#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/
https://codereview.adblockplus.org/29555744/

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.

Change History (14)

comment:1 Changed 11 months ago 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: Changed 11 months ago by kzar

  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

Any idea if we can remove the line commented as a Firefox workaround in skin/mobile-options.css?

comment:3 in reply to: ↑ 2 Changed 11 months ago 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 11 months ago 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 11 months ago 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 11 months ago by abpbot

comment:7 Changed 11 months ago by kzar

  • Status changed from reviewing to reopened

comment:8 Changed 11 months ago by sebastian

  • Review URL(s) modified (diff)
  • Status changed from reopened to reviewing

comment:9 Changed 11 months ago by abpbot

comment:10 Changed 11 months ago by kzar

  • Status changed from reviewing to reopened

comment:11 Changed 10 months ago by kzar

  • Blocking 5080 removed

comment:12 Changed 3 months ago by greiner

@kzar Is there anything left to do here or can we close this ticket?

comment:13 Changed 2 months ago by kzar

I don't *think* there are, but then we keep finding things like #6669 and #6562. I think close this for now, if we find anything more we can reopen this / open a new issue.

comment:14 Changed 2 months ago by greiner

  • Resolution set to fixed
  • Status changed from reopened to closed

Sounds reasonable. Thanks for checking.

Note: See TracTickets for help on using tickets.