Opened 14 months ago

Closed 8 weeks ago

Last modified 2 weeks ago

#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):

https://codereview.adblockplus.org/29897555/

Description (last modified by mjethani)

Background

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

[TBD]

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.

Change History (12)

comment:1 Changed 14 months ago by mjethani

  • Description modified (diff)

comment:2 Changed 13 months ago by jsonesen

  • Owner set to jsonesen

comment:3 Changed 13 months ago 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 13 months ago by jsonesen (previous) (diff)

comment:4 Changed 12 months ago by jsonesen

  • Review URL(s) modified (diff)

comment:5 Changed 12 months ago by abpbot

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

comment:6 Changed 12 months ago by mjethani

  • Description modified (diff)

comment:7 Changed 12 months ago by mjethani

  • Description modified (diff)

comment:8 Changed 12 months ago by abpbot

Last edited 12 months ago by jsonesen (previous) (diff)

comment:9 Changed 9 months ago by abpbot

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

comment:10 Changed 8 months ago by ukacar

  • Verified working set

comment:11 Changed 3 months ago by jha2125

spam

Last edited 2 weeks ago by kzar (previous) (diff)

comment:12 Changed 8 weeks ago 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.

Note: See TracTickets for help on using tickets.