Opened 9 months ago

Closed 4 months ago

Last modified 4 months ago

#7016 closed change (fixed)

Convert serialization functions into generators

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

Description (last modified by mjethani)

Background

The serialization functions in the Filter and Subscripton classes look somewhat like this (pseudocode):

/**
 * @param {string[]} buffer
 */
function serialize(buffer)
{
  buffer.push("[Filter]");
  buffer.push("text=" + this.text);
}

The exportData function in lib/filterStorage.js has code like this:

let buffer = [];
for (let filter of filters)
{
  filter.serialize(buffer);
  for (let line of buffer)
    yield line;
  buffer.splice(0);
}

exportData is a generator function. It is wasteful to create a temporary array, pass it to the serialization function for each filter, and empty it out for the next filter in the list. Instead, the serialization function could also be a generator that yields each "line" to be serialized.

With this change, the code in exportData would look like this instead:

for (let filter of filters)
  yield* filter.serialize();

What to change

Convert the serialization functions in the Filter and Subscription classes to generators, and make the corresponding changes in the exportData function.

Tips for testers

Ensure custom filters added from the options page, and the devtools panel persists between browser restarts (see hints in #6893).

Also try the following:

  1. Subscribe to a filter list
  2. Try a filter from the filter list, it should work
  3. Reload the extension and try the filter again, it should work
  4. Disable the subscription, reload the extension, and try the filter again, it should no longer work
  5. Enable the subscription, reload the extension, and try the filter again, it should work again
  6. Remove the subscription (click the trash icon), reload the extension, and try the filter again, it should once again no longer work

Change History (12)

comment:1 Changed 9 months ago by jsonesen

  • Owner set to jsonesen

comment:2 Changed 9 months ago by sebastian

  • Cc sebastian added

Please add some integration notes to adapt this code in
adblockpluschrome which currently relies on subscription's serialize() and serializeFilters() methods to take an array as an argument.

comment:3 Changed 8 months ago by abpbot

A commit referencing this issue has landed:
Issue 7016 - Convert serialization functions into generators

comment:4 Changed 7 months ago by erikvold

  • Cc erikvold added

comment:5 Changed 5 months ago by jsonesen

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

comment:6 Changed 5 months ago by mjethani

  • Resolution fixed deleted
  • Status changed from closed to reopened

comment:7 follow-up: Changed 5 months ago by mjethani

Hey Jon,

I have reopened this because it doesn't have a "Hints for testers" section. We need this.

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

comment:8 in reply to: ↑ 7 Changed 5 months ago by jsonesen

Replying to mjethani:

Hey Jon,

I have reopened this because it doesn't have a "Hints for testers" section. We need this.

Good catch, I woefully neglected this ticket when I landed the change I will fix this.

comment:9 Changed 5 months ago by abpbot

A commit referencing this issue has landed:
Issue 7016 - Convert serialization functions into generators

comment:10 Changed 4 months ago by jsonesen

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

comment:11 Changed 4 months ago by mjethani

  • Description modified (diff)

comment:12 Changed 4 months ago by ukacar

  • Verified working set
Note: See TracTickets for help on using tickets.