Opened 3 years ago

Closed 3 years ago

Last modified 2 months ago

#5063 closed change (fixed)

[emscripten] Make FilterNotifier calls more efficient

Reported by: trev Assignee: trev
Priority: P2 Milestone:
Module: Core Keywords:
Cc: Blocked By:
Blocking: #4122, #5144 Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

https://codereview.adblockplus.org/29398669/

Description (last modified by trev)

Background

We currently call the JS class FilterNotifier whenever a filter or subscription property changes. However, we unnecessarily bloat the code size by creating a new EM_ASM block for each property. Also, we have the topic names in Emscripten even though these are constants - this wastes memory and requires a string conversion to be performed for each call. Finally, we currently have wrong topic names, e.g. filter.mDisabled rather than filter.disabled.

In addition, property change notifications transmit both the old and the new property value. This functionality is as good as unused - only FilterListener using the new value as far as I can see, and here it isn't really necessary.

What to change

Create a FilterNotifier C++ namespace providing the functionality to notify JavaScript code about changes - use the same EM_ASM block for all notifications. Define the topics as an enum and map the numerical topics to strings in JavaScript via custom bindings generator (#5062). Remove newValue and oldValue parameters.

Change History (6)

comment:1 Changed 3 years ago by trev

  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

comment:2 Changed 3 years ago by trev

  • Blocking 5144 added
  • Summary changed from Make FilterNotifier calls more efficient to [emscripten] Make FilterNotifier calls more efficient

comment:3 Changed 3 years ago by trev

  • Description modified (diff)

comment:4 Changed 3 years ago by abpbot

A commit referencing this issue has landed:
Issue 5063 - [emscripten Make FilterNotifier calls more efficient]

comment:5 Changed 3 years ago by trev

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

comment:6 Changed 3 months ago by tenmez190

spam

Last edited 2 months ago by kzar (previous) (diff)
Note: See TracTickets for help on using tickets.