Opened on 09/10/2018 at 02:44:55 PM

Closed on 08/29/2019 at 05:43:52 PM

Last modified on 10/08/2019 at 06:05:38 PM

#6940 closed change (rejected)

Use underscore prefixes for internal properties

Reported by: mjethani Assignee:
Priority: P3 Milestone:
Module: Core Keywords: closed-in-favor-of-gitlab
Cc: hfiguiere, jsonesen, kzar, sergz Blocked By:
Blocking: Platform: Unknown / Cross platform
Ready: no Confidential: no
Tester: Unknown Verified working: yes
Review URL(s):

Description (last modified by mjethani)


In lib/matcher.js, the Matcher class has an internal property called keywordByFilter. Any other code using instances of this class has no business accessing this property; in fact, CombinedMatcher, which is in the same source file, doesn't even access this property itself but rather uses the getKeywordForFilter method. So clearly this property is supposed to be "private" or internal to the class.

Since we're moving to ES2015 classes (#6741), we should also update the code to clearly mark internal properties as internal in some way. There was an idea about using private symbols for this (#6424), which was rejected in the browser extension and would not be feasible for core anyway due to performance. Instead we should just stick with the common convention of prefixing the name of the property with an underscore (_).

There are good reasons for keeping an object's internal state private to the object itself (e.g. #6916).

An internal property of an object should:

  1. Have a name beginning with an underscore (_)
  2. If a JSDoc comment is present, it must include the @private tag
  3. The property should not be accessed from outside the object's own constructor and methods (we might have to update our ESLint config for this)

Some properties must be accessed directly from within the same source file for performance reasons, yet they are not meant to be used from other code; in such cases, let's just mark the property @protected instead and use no underscore.

What to change


Integration notes

With changeset 7126b8cc2fad, the blacklist and whitelist properties of CombinedMatcher are now "private". They can still be accessed as before using their new names starting with an underscore, but if there's any need to do this, please file an issue and let us know the use case and we'll try to provide a proper API for whatever you're trying to do. After changeset e8254db792d0 it should no longer be necessary to access the whitelist property directly since the implementation of CombinedMatcher's matchesAny method is now optimized for $document, $elemhide, $generichide, and $genericblock.

In adblockpluschrome, please update lib/whitelisting.js and lib/filterComposer.js to use the matchesAny method of defaultMatcher directly.

Hints for testers

With changeset 7126b8cc2fad we have renamed some internal properties, but all of this is covered by unit tests. There is no change in the implementation (so there's nothing to test). The dependency update for adblockpluschrome will require some changes to the code there, please see the relevant dependency update ticket for testing hints.

Attachments (0)

Change History (12)

comment:1 Changed on 09/10/2018 at 02:47:24 PM by mjethani

  • Description modified (diff)

comment:2 Changed on 10/01/2018 at 04:23:04 AM by jsonesen

  • Owner set to jsonesen

comment:3 Changed on 10/01/2018 at 05:42:08 AM by jsonesen

  • Owner jsonesen deleted

I get the feeling this is more of a meta ticket and may include multiple commits, so I unassigned myself.

Last edited on 10/01/2018 at 05:43:09 AM by jsonesen

comment:4 Changed on 10/23/2018 at 02:05:28 AM by jsonesen

  • Review URL(s) modified (diff)

comment:5 Changed on 10/24/2018 at 07:09:01 PM by abpbot

A commit referencing this issue has landed:
Issue 6940 - Use underscore prefixes in lib/matcher.js

comment:6 Changed on 10/24/2018 at 07:18:37 PM by mjethani

  • Description modified (diff)

comment:7 Changed on 10/24/2018 at 07:23:09 PM by mjethani

  • Description modified (diff)

comment:8 Changed on 11/08/2018 at 08:57:55 PM by abpbot

Last edited on 11/08/2018 at 08:59:15 PM by jsonesen

comment:9 Changed on 02/07/2019 at 03:23:36 AM by abpbot

A commit referencing this issue has landed:
Issue 6940 - Use underscore prefixes in lib/matcher.js

comment:10 Changed on 02/21/2019 at 08:55:37 AM by ukacar

  • Verified working set

comment:11 Changed on 07/27/2019 at 10:10:19 AM by jha2125


Last edited on 10/08/2019 at 06:05:38 PM by kzar

comment:12 Changed on 08/29/2019 at 05:43:52 PM by sebastian

  • Keywords closed-in-favor-of-gitlab added
  • Resolution set to rejected
  • Status changed from new to closed

Sorry, but we switched to GitLab. If this issue is still relevant, please file it again in the new issue tracker.

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 (none).
Note: See TracTickets for help on using tickets.