Opened on 03/29/2019 at 10:43:45 AM

Closed on 04/05/2019 at 05:11:08 PM

Last modified on 07/10/2019 at 12:46:16 PM

#7423 closed change (fixed)

Stop using the filterValidation module

Reported by: kzar Assignee: kzar
Priority: P2 Milestone:
Module: User-Interface Keywords: manifestv3
Cc: greiner, sebastian Blocked By:
Blocking: #7308, #7338 Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Unknown Verified working: yes
Review URL(s):

Description (last modified by kzar)


With #7338 we're going to remove the filterValidation module, since background scripts (e.g. messageResponder.js) no longer will have access to the DOM APIs which we rely on to validate CSS selectors.

What to change

  • Move adblockpluschrome/lib/filterValidation.js code to adblockplusui
  • Remove CSS validation logic from filter validation code


See also ui#383

Hints for testers

  • Test that custom filters can be added using the "Block element" tool and the options page. Also test that websites can be whitelisted properly. Test that when invalid filters are entered by the user the proper messages are displayed - see this table for some example filters. Note: We're no longer validating CSS selectors, so the "... is not a valid CSS selector" message is no longer expected to be displayed.

Hints for translators

Removed "invalid_css_selector" string.

Integration notes

  • Delete adblockpluschrome/lib/filterValidation.js and adblockpluschrome/qunit/tests/filterValidation.js, see #7338 for more context.

Attachments (0)

Change History (21)

comment:1 Changed on 03/29/2019 at 12:24:36 PM by greiner

That module does more than just validate CSS selectors so it sounds like we should move the entire module to adblockplusui and then split off the parts that are dependent on the DOM so that those are run in the context of the various UI pages.

comment:2 Changed on 03/29/2019 at 01:31:11 PM by kzar

Sounds good to me, since it seems like messageResponder.js is the only consumer of the filterValidation module.

comment:3 Changed on 03/29/2019 at 01:33:18 PM by greiner

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

Thanks, I updated the ticket description to reflect that.

comment:4 Changed on 03/29/2019 at 01:34:08 PM by greiner

  • Description modified (diff)

Added link to GitLab issue.

comment:5 Changed on 03/29/2019 at 01:49:49 PM by kzar

Would you mind upping the priority of this issue? I ask since it blocks #7338 which is required for cluster 0.

comment:6 Changed on 03/29/2019 at 01:56:11 PM by greiner

  • Priority changed from P3 to P2

Done. It's mostly a formality for us anyway. The way we in reality prioritize things is by assigning them to a UI release. For now I've assigned it to release-2018-5 whose dependency update is scheduled for the time when we decide to make the next major extension release.

comment:7 Changed on 03/29/2019 at 05:30:05 PM by kzar

Gotya OK, thanks.

comment:8 in reply to: ↑ description ; follow-up: Changed on 03/29/2019 at 10:41:47 PM by sebastian

  • Move adblockpluschrome/lib/filterValidation.js to adblockplus/ui/lib/filterValidation.js

With the CSS validation code removed, all is left of the filterValidation module is a trivial wrapper around Fitler.fromText() and instance of InvalidFilter. Furthermore, the functions parseFilter() and parseFilters() are only called from one code path in messageResponder.js. So why not just inline the code there?

comment:9 in reply to: ↑ 8 Changed on 04/01/2019 at 02:36:36 PM by greiner

  • Description modified (diff)

Replying to sebastian:

So why not just inline the code there?

Makes sense. I updated the ticket description to leave it up to whoever implements it to assess how to best integrate it.

comment:10 Changed on 04/02/2019 at 09:34:55 AM by kzar

  • Owner set to kzar

comment:11 Changed on 04/02/2019 at 02:16:30 PM by greiner

  • Blocking 7431 added

comment:12 Changed on 04/02/2019 at 02:37:14 PM by kzar

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

comment:13 Changed on 04/02/2019 at 02:54:39 PM by greiner

  • Blocking 7308 added; 7431 removed

As requested, I've changed the corresponding dependency update to #7308 (i.e. release-2018-5.-4).

comment:14 Changed on 04/02/2019 at 02:58:37 PM by kzar

  • Description modified (diff)

comment:15 Changed on 04/02/2019 at 03:00:37 PM by greiner

  • Description modified (diff)

Added hints for translators.

comment:16 Changed on 04/02/2019 at 03:05:42 PM by kzar

  • Description modified (diff)

comment:17 Changed on 04/02/2019 at 03:06:12 PM by kzar

  • Description modified (diff)

comment:18 Changed on 04/02/2019 at 03:35:07 PM by kzar

  • Description modified (diff)

comment:19 Changed on 04/05/2019 at 05:11:08 PM by greiner

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

comment:20 Changed on 04/26/2019 at 12:48:44 PM by abpbot

A commit referencing this issue has landed:
Issue 7423 - Stop relying on the filterValidation module

comment:21 Changed on 07/10/2019 at 12:46:16 PM by ukacar

  • Verified working set

Add Comment

Modify Ticket

Change Properties
as closed .
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from kzar.
Note: See TracTickets for help on using tickets.