Opened on 08/28/2018 at 11:47:04 AM
Closed on 08/29/2018 at 12:33:30 PM
Last modified on 10/26/2018 at 12:23:28 PM
#6893 closed change (fixed)
Split out the INIParser class into its own file
Reported by: | mjethani | Assignee: | mjethani |
---|---|---|---|
Priority: | P3 | Milestone: | |
Module: | Core | Keywords: | |
Cc: | Blocked By: | ||
Blocking: | Platform: | Unknown / Cross platform | |
Ready: | yes | Confidential: | no |
Tester: | Ross | Verified working: | yes |
Review URL(s): |
Description (last modified by mjethani)
Background
The INIParser class is currently internal to lib/filterStorage.js. It is not well documented. It is tested only as part of lib/filterStorage.js.
This is an important class for multiple reasons, most importantly performance. If we are to make changes to the INI file format (see discussion in #6861), this class should firstly be well written and well documented. We are also migrating to ES2015 classes on a file-by-file basis (see #6741), and it would be easier to manage the changes if large files like lib/filterStorage.js are gradually split up into smaller files.
What to change
- Move the definition of INIParser into lib/iniParser.js
- Convert to ES2015 class
- Make properties not needed by other modules "private" (use an underscore prefix)
- Add JSDoc comments
Additional notes
The unit testing for this class is currently covered by test/filterStorage_readwrite.js. This is OK, there's no need to write separate tests, but it may make sense to do so in the future, especially if there are changes to the file format.
Hints for testers
This is a development/refactoring task and there are no specific things to test, as there is no expected change in behavior. Just make sure that there are no regressions, i.e. filters are saved and loaded correctly to and from the patterns.ini file.
Try this:
- Add a filter foo in the "My filters" box and click Save
- Add another filter ##.foo and click Save
- Add a comment ! foo and click Save
- Restart the extension (disable and enabled on chrome://extensions)
- Open the options page and see that the two filters and comment are still there
Attachments (0)
Change History (9)
comment:2 Changed on 08/29/2018 at 12:28:05 PM by abpbot
comment:3 Changed on 08/29/2018 at 12:33:30 PM by mjethani
- Description modified (diff)
- Resolution set to fixed
- Status changed from new to closed
comment:5 Changed on 10/24/2018 at 01:32:35 PM by Ross
I have a dumb question: Where should we be looking for patterns.ini now?
comment:6 Changed on 10/24/2018 at 02:45:10 PM by mjethani
Not a dumb question at all. In the current extension for Chrome, you could get the contents of patterns.ini by pasting the following line into the background page console:
browser.storage.local.get("file:patterns.ini").then(console.log);
This should give you an object with two properties, content and lastModified, and the content property is the serialized data.
comment:8 Changed on 10/25/2018 at 11:59:32 AM by mjethani
Having said the above, I don't think anybody should have to manually examine the patterns.ini. I've added a simple test idea in the description now.
comment:9 Changed on 10/26/2018 at 12:23:28 PM by Ross
- Tester changed from Unknown to Ross
- Verified working set
Thanks, that makes sense! This looks to be working as expected then.
ABP 3.3.2.2176
Firefox 62 / 51 / Windows 10
Chrome 69 / 49 / Windows 10
Opera 56 / 36 / Windows 10
A commit referencing this issue has landed:
Issue 6893, 6741 - Split out INIParser into its own file