Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(365)

Issue 29338734: Issue 3839 - Adapt for Prefs event changes (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 years, 8 months ago by Sebastian Noack
Modified:
3 years, 8 months ago
Reviewers:
Thomas Greiner
Visibility:
Public.

Description

Issue 3839 - Adapt for Prefs event changes

Patch Set 1 #

Total comments: 2

Patch Set 2 : #

Total comments: 9

Patch Set 3 : app.listen doesn't need a FilterNotifier listener #

Patch Set 4 : Reuse event emitter logic #

Patch Set 5 : Simplified FilterNotifier logic #

Patch Set 6 : Cleaned up onFilterChange() #

Total comments: 8

Patch Set 7 : Addresed comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+93 lines, -58 lines) Patch
M background.js View 1 2 3 4 5 6 3 chunks +42 lines, -20 lines 0 comments Download
M messageResponder.js View 1 2 3 4 5 6 8 chunks +51 lines, -38 lines 0 comments Download

Messages

Total messages: 8
Sebastian Noack
https://codereview.adblockplus.org/29338734/diff/29338735/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29338734/diff/29338735/messageResponder.js#newcode65 messageResponder.js:65: var pages = changeListeners.keys(); This is unrelated, but since ...
3 years, 8 months ago (2016-03-19 19:17:30 UTC) #1
Sebastian Noack
I forgot to adapt the mock implementation in background.js (I only tested with the extension). ...
3 years, 8 months ago (2016-03-21 16:28:38 UTC) #2
Thomas Greiner
https://codereview.adblockplus.org/29338734/diff/29338841/background.js File background.js (right): https://codereview.adblockplus.org/29338734/diff/29338841/background.js#newcode98 background.js:98: on: function(preference, callback) What about extending `Notifier` instead so ...
3 years, 8 months ago (2016-03-21 17:55:56 UTC) #3
Sebastian Noack
https://codereview.adblockplus.org/29338734/diff/29338841/background.js File background.js (right): https://codereview.adblockplus.org/29338734/diff/29338841/background.js#newcode98 background.js:98: on: function(preference, callback) On 2016/03/21 17:55:55, Thomas Greiner wrote: ...
3 years, 8 months ago (2016-03-21 19:40:07 UTC) #4
Sebastian Noack
https://codereview.adblockplus.org/29338734/diff/29338897/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29338734/diff/29338897/messageResponder.js#newcode101 messageResponder.js:101: for (var i = 1; i < arguments.length; i++) ...
3 years, 8 months ago (2016-03-22 07:29:57 UTC) #5
Thomas Greiner
Thanks for addressing the comments. Just two smaller ones before we're good. https://codereview.adblockplus.org/29338734/diff/29338897/background.js File background.js ...
3 years, 8 months ago (2016-03-22 13:29:40 UTC) #6
Sebastian Noack
https://codereview.adblockplus.org/29338734/diff/29338897/background.js File background.js (right): https://codereview.adblockplus.org/29338734/diff/29338897/background.js#newcode60 background.js:60: var listeners = this._eventEmitter[""]; On 2016/03/22 13:29:40, Thomas Greiner ...
3 years, 8 months ago (2016-03-22 14:24:11 UTC) #7
Thomas Greiner
3 years, 8 months ago (2016-03-22 14:46:18 UTC) #8
LGTM
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5