Opened 9 months ago

Closed 2 months ago

#6891 closed change (fixed)

Properly distinguish objects from classes

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

https://codereview.adblockplus.org/29868577/

Description (last modified by mjethani)

Background

There are a number of objects in core that share some of their state and provide some methods for querying and modifying the state.

The FilterStorage object in lib/filterStorage.js, for example:

exports.FilterStorage = {
  initialized: false,

  loadFromDisk()
  {
    // Load subscriptions from disk.
    this.initialized = true;
  }
};

These objects are incorrectly referred to as "classes" in the documentation, even though they are just singleton objects. Furthermore, their names start with upper case letters (e.g. FilterStorage as opposed to filterStorage), which makes it even more confusing to reason about these objects. They also use the this keyword internally.

There are two solutions:

  1. Make them proper singletons so that they are truly the sole instances of their respective classes. For example, the filterStorage object could be an instance of the FilterStorage class.
  1. Let them remain standalone objects (or "modules", if you will), not related to any class, and stop using the this keyword internally to avoid confusion.

Note that in the latter case JSDoc does not appear to correctly associate the object's properties with the object; there is no documentation generated for the properties.

What to change

After #6854, rename FilterNotifier in lib/filterNotifier.js to filterNotifier.

Rename FilterStorage in lib/filterStorage.js to filterStorage. The current implementation of FilterStorage should be wrapped into an ES2015 class (#6741), and the filterStorage object should be an instance of this class.

Integration notes

The FilterNotifier object exported by lib/filterNotifier.js is now renamed to filterNotifier. It is now directly an instance of EventEmitter, with no intermediate object in its prototype chain (i.e. filterNotifier.construtor is EventEmitter and filterNotifier.__proto__ is EventEmitter.prototype, though these internal details shouldn't matter).

After changeset 58b0ccc6c289, the FilterStorage object exported out of lib/filterStorage.js is now called filterStorage instead.

Hints for testers

For the dependency update in #6933, there is no change in behavior here, therefore there is nothing specific to test (the current test cases should cover it if there has been any omission in the dependency update). Watch out for errors in the background page console.

Change History (16)

comment:1 Changed 9 months ago by abpbot

A commit referencing this issue has landed:
Issue 6891 - Rename FilterNotifier to filterNotifier

comment:2 Changed 9 months ago by mjethani

  • Cc sebastian greiner added
  • Ready set
  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

comment:3 Changed 9 months ago by mjethani

  • Description modified (diff)

comment:4 Changed 9 months ago by greiner

  • Cc wspee added

I've created #6907 for the corresponding adblockplusui changes.

Are we expecting any more changes of this kind to be added to the next major release? If so, it'd be great if we could reduce the overhead associated with each small change by grouping the necessary UI changes together.

comment:5 Changed 9 months ago by mjethani

Yes, I think there will be more changes for #6891 and #6741 (both works in progress), as well as possibly #6559, for the next major release; for #6891 there will definitely be some renaming of things to do.

Last edited 9 months ago by mjethani (previous) (diff)

comment:6 Changed 8 months ago by mjethani

  • Description modified (diff)

comment:7 Changed 7 months ago by abpbot

A commit referencing this issue has landed:
Issue 6891, 6741 - Rename FilterStorage to filterStorage

comment:8 Changed 7 months ago by mjethani

  • Description modified (diff)

comment:9 Changed 7 months ago by mjethani

  • Owner set to mjethani

comment:10 Changed 7 months ago by greiner

  • Description modified (diff)

Fixed typo in description.

comment:11 follow-up: Changed 7 months ago by Ross

Should we verify this ticket yet? Or is it still open? I haven't seen any errors in the background console so far.

comment:12 in reply to: ↑ 11 ; follow-up: Changed 7 months ago by mjethani

Replying to Ross:

Should we verify this ticket yet? Or is it still open?

This is going to remain open until all the changes are done in core. You could verify the dependency update in the browser extension instead (e.g. #6933).

comment:13 in reply to: ↑ 12 Changed 7 months ago by mjethani

Replying to mjethani:

Replying to Ross:

Should we verify this ticket yet? Or is it still open?

This is going to remain open until all the changes are done in core. You could verify the dependency update in the browser extension instead (e.g. #6933).

So just to be clear, I don't think there's anything specific to test related to this ticket for #6933.

comment:14 Changed 3 months ago by abpbot

A commit referencing this issue has landed:
Issue 6891, 6741 - Rename FilterStorage to filterStorage

comment:15 Changed 3 months ago by ukacar

  • Verified working set

comment:16 Changed 2 months ago by mjethani

  • Description modified (diff)
  • Resolution set to fixed
  • Status changed from reviewing to closed

I am closing this now since it has already been marked verified. I'll open a new one for further changes.

Note: See TracTickets for help on using tickets.