Opened 3 years ago

Closed 2 years ago

Last modified 2 years ago

#5079 closed change (fixed)

Export ElemHideEmulation and splitSelector

Reported by: kzar Assignee: kzar
Priority: P3 Milestone:
Module: Core Keywords:
Cc: sebastian, trev, hfiguiere, jsonesen Blocked By: #5516
Blocking: #5089 Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Ross Verified working: yes
Review URL(s):

https://codereview.adblockplus.org/29399602/
https://codereview.adblockplus.org/29460576/
https://codereview.adblockplus.org/29517687/

Description (last modified by kzar)

Background

We want to begin using modules for the adblockpluschrome content scripts too. This is so that we don't need to have so many /* globals ... */ comments now that we're using ESLint.

What to change

Rename adblockpluscore/chrome/content/elemHideEmulation.js into adblockpluscore/lib/content/elemHideEmulation.js and turn it into a proper CommonJS module exporting ElemHideEmulation. Move splitSelector into adblockpluscore/lib/common.js and export that too. Have elemHideEmulation.js call require("common.js") rather than expect it to be loaded in global scope (common.js can export its symbols unconditionally then). Unit tests need to be adjusted as well.

Integration notes

Make sure to adjust elemHideEmulation import to consider its new location. Also, it now needs to be used as a CommonJS module.

Change History (20)

comment:1 Changed 3 years ago by kzar

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

comment:2 Changed 3 years ago by kzar

  • Blocking 5080 added

comment:3 Changed 3 years ago by kzar

  • Blocking 4864 removed

comment:4 Changed 3 years ago by kzar

  • Blocking 5089 added

comment:5 Changed 3 years ago by kzar

  • Blocking 5080 removed

comment:6 Changed 3 years ago by trev

  • Description modified (diff)
  • Priority changed from Unknown to P3

comment:7 Changed 3 years ago by kzar

  • Owner kzar deleted

comment:8 Changed 3 years ago by trev

  • Description modified (diff)
  • Ready set

comment:9 Changed 2 years ago by kzar

  • Cc hfiguiere added

Adding you as Cc Hubert since you've been working with this code already, if you have a chance to help with this one sometime we'd appreciate it.

comment:10 Changed 2 years ago by hfiguiere

I think it make sense to do this without waiting for issue #3143.

comment:11 Changed 2 years ago by kzar

  • Cc jsonesen added
  • Status changed from reviewing to reopened

comment:12 Changed 2 years ago by hfiguiere

  • Owner set to hfiguiere

comment:13 Changed 2 years ago by hfiguiere

  • Review URL(s) modified (diff)

comment:14 Changed 2 years ago by kzar

  • Blocked By 5516 added

comment:15 Changed 2 years ago by kzar

  • Owner changed from hfiguiere to kzar
  • Review URL(s) modified (diff)

comment:16 Changed 2 years ago by kzar

  • Status changed from reopened to reviewing

comment:17 Changed 2 years ago by kzar

  • Description modified (diff)

comment:19 Changed 2 years ago by kzar

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

comment:20 Changed 2 years ago by Ross

  • Tester changed from Unknown to Ross
  • Verified working set

Done. Element hiding functionality still works (except for non-regression #5773).

ABP 1.13.3.1838
Chrome 52 / 61 / Windows 7
Opera 40 / 47 / Windows 7

Note: See TracTickets for help on using tickets.