Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Issue 29398669: Issue 5063 - [emscripten] Make FilterNotifier calls more efficient (Closed)

Created:
March 30, 2017, 10:08 a.m. by Wladimir Palant
Modified:
April 20, 2017, 8:20 a.m.
Reviewers:
sergei
CC:
hub
Base URL:
https://hg.adblockplus.org/adblockpluscore
Visibility:
Public.

Description

The impact on code size was less than I hoped for but this still makes the code cleaner and more performant.

Patch Set 1 #

Patch Set 2 : Added missing files #

Patch Set 3 : Rebased and made the code deal with 64-bit properties #

Patch Set 4 : Rebased #

Patch Set 5 : Removed newValue and oldValue parameters #

Total comments: 4

Patch Set 6 : Addressed comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+356 lines, -78 lines) Patch
A compiled/FilterNotifier.h View 1 2 3 4 1 chunk +65 lines, -0 lines 0 comments Download
A compiled/FilterNotifier.cpp View 1 2 3 4 5 1 chunk +36 lines, -0 lines 0 comments Download
M compiled/String.h View 1 2 1 chunk +10 lines, -0 lines 0 comments Download
M compiled/bindings.cpp View 2 chunks +3 lines, -0 lines 0 comments Download
M compiled/bindings.ipp View 1 2 3 2 chunks +21 lines, -12 lines 0 comments Download
M compiled/filter/ActiveFilter.h View 1 2 3 4 2 chunks +11 lines, -10 lines 0 comments Download
M compiled/shell.js View 1 chunk +0 lines, -2 lines 0 comments Download
M compiled/subscription/DownloadableSubscription.h View 1 2 1 chunk +24 lines, -12 lines 0 comments Download
M compiled/subscription/Subscription.h View 1 2 3 4 2 chunks +17 lines, -42 lines 0 comments Download
M test/filterClasses.js View 1 2 3 4 2 chunks +60 lines, -0 lines 0 comments Download
M test/subscriptionClasses.js View 1 2 3 4 5 2 chunks +109 lines, -0 lines 0 comments Download

Messages

Total messages: 8
Wladimir Palant
March 30, 2017, 10:08 a.m. (2017-03-30 10:08:25 UTC) #1
Wladimir Palant
Added Hubert to CC.
March 30, 2017, 5:41 p.m. (2017-03-30 17:41:09 UTC) #2
Wladimir Palant
I somehow forgot to add new files here. Patch set 2 fixes that.
April 6, 2017, 8:09 a.m. (2017-04-06 08:09:00 UTC) #3
Wladimir Palant
Patch set 3 has been rebased. Due to changes to subscription classes it now accepts ...
April 7, 2017, 7:18 a.m. (2017-04-07 07:18:00 UTC) #4
Wladimir Palant
In Patch set 5 I simplified this considerably by removing newValue and oldValue parameters altogether ...
April 15, 2017, 3:07 p.m. (2017-04-15 15:07:24 UTC) #5
sergei
https://codereview.adblockplus.org/29398669/diff/29413563/compiled/FilterNotifier.cpp File compiled/FilterNotifier.cpp (right): https://codereview.adblockplus.org/29398669/diff/29413563/compiled/FilterNotifier.cpp#newcode24 compiled/FilterNotifier.cpp:24: void FilterChange(Topic topic, Filter* filter) It's not important here, ...
April 19, 2017, 12:18 p.m. (2017-04-19 12:18:03 UTC) #6
Wladimir Palant
Addressed Sergei's comments in Patch set 6. https://codereview.adblockplus.org/29398669/diff/29413563/compiled/FilterNotifier.cpp File compiled/FilterNotifier.cpp (right): https://codereview.adblockplus.org/29398669/diff/29413563/compiled/FilterNotifier.cpp#newcode24 compiled/FilterNotifier.cpp:24: void FilterChange(Topic ...
April 20, 2017, 8:03 a.m. (2017-04-20 08:03:14 UTC) #7
sergei
April 20, 2017, 8:11 a.m. (2017-04-20 08:11:20 UTC) #8
LGTM

Powered by Google App Engine
This is Rietveld