Opened on 03/31/2017 at 07:34:30 AM
Closed on 08/21/2017 at 09:08:51 AM
Last modified on 09/25/2017 at 02:08:29 PM
#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/ |
||
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.
Attachments (0)
Change History (20)
comment:1 Changed on 03/31/2017 at 07:39:24 AM by kzar
- Review URL(s) modified (diff)
- Status changed from new to reviewing
comment:2 Changed on 03/31/2017 at 08:57:03 AM by kzar
- Blocking 5080 added
comment:3 Changed on 03/31/2017 at 09:01:01 AM by kzar
- Blocking 4864 removed
comment:4 Changed on 04/03/2017 at 07:52:13 AM by kzar
- Blocking 5089 added
comment:5 Changed on 04/03/2017 at 07:52:35 AM by kzar
- Blocking 5080 removed
comment:6 Changed on 04/03/2017 at 08:00:51 AM by trev
- Description modified (diff)
- Priority changed from Unknown to P3
comment:7 Changed on 04/03/2017 at 08:02:58 AM by kzar
- Owner kzar deleted
comment:9 Changed on 05/29/2017 at 03:30:52 PM by kzar
- Cc hfiguiere added
comment:10 Changed on 05/29/2017 at 03:57:49 PM by hfiguiere
I think it make sense to do this without waiting for issue #3143.
comment:11 Changed on 06/03/2017 at 11:51:55 AM by kzar
- Cc jsonesen added
- Status changed from reviewing to reopened
comment:12 Changed on 06/05/2017 at 09:47:50 PM by hfiguiere
- Owner set to hfiguiere
comment:13 Changed on 06/09/2017 at 05:12:21 PM by hfiguiere
- Review URL(s) modified (diff)
comment:14 Changed on 08/14/2017 at 01:03:06 PM by kzar
- Blocked By 5516 added
comment:15 Changed on 08/16/2017 at 03:57:14 PM by kzar
- Owner changed from hfiguiere to kzar
- Review URL(s) modified (diff)
comment:16 Changed on 08/16/2017 at 03:57:25 PM by kzar
- Status changed from reopened to reviewing
comment:17 Changed on 08/16/2017 at 03:59:42 PM by kzar
- Description modified (diff)
comment:18 Changed on 08/21/2017 at 09:05:51 AM by abpbot
A commit referencing this issue has landed:
Issue 5079, 5516 - Use webpack for browser tests, modules for content scripts
comment:19 Changed on 08/21/2017 at 09:08:51 AM by kzar
- Resolution set to fixed
- Status changed from reviewing to closed
comment:20 Changed on 09/25/2017 at 02:08:29 PM 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
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.