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

Issue 29370996: Issue 4783 - Use Port API for the messageResponder (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 years, 10 months ago by kzar
Modified:
2 years, 10 months ago
Visibility:
Public.

Description

Issue 4783 - Use Port API for the messageResponder

Patch Set 1 #

Total comments: 6

Patch Set 2 : Add polyfilly for messaging API to background.js #

Total comments: 6

Patch Set 3 : Use EventEmitter #

Total comments: 2

Patch Set 4 : Improve messaging implementation in background.js #

Unified diffs Side-by-side diffs Delta from patch set Stats (+319 lines, -239 lines) Patch
M background.js View 1 2 3 1 chunk +47 lines, -0 lines 0 comments Download
M messageResponder.js View 2 chunks +272 lines, -239 lines 0 comments Download

Messages

Total messages: 14
kzar
Patch Set 1 This change is required for my continuing work to target the browserext ...
2 years, 10 months ago (2017-01-11 08:48:25 UTC) #1
Sebastian Noack
Did you do any changes other than indentation to any code inside the handlers? If ...
2 years, 10 months ago (2017-01-11 16:48:20 UTC) #2
kzar
> Did you do any changes other than indentation to any code > inside the ...
2 years, 10 months ago (2017-01-12 10:07:50 UTC) #3
Sebastian Noack
LGTM
2 years, 10 months ago (2017-01-12 10:29:02 UTC) #4
Sebastian Noack
https://codereview.adblockplus.org/29370996/diff/29370997/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29370996/diff/29370997/messageResponder.js#newcode23 messageResponder.js:23: var port = require("messaging").port; I might have judged a ...
2 years, 10 months ago (2017-01-12 11:17:59 UTC) #5
kzar
Patch Set 2 : Add polyfilly for messaging API to background.js https://codereview.adblockplus.org/29370996/diff/29370997/messageResponder.js File messageResponder.js (right): ...
2 years, 10 months ago (2017-01-13 08:02:41 UTC) #6
Sebastian Noack
https://codereview.adblockplus.org/29370996/diff/29371679/background.js File background.js (right): https://codereview.adblockplus.org/29370996/diff/29371679/background.js#newcode341 background.js:341: modules.messaging = { Can you please just use the ...
2 years, 10 months ago (2017-01-13 12:12:10 UTC) #7
kzar
https://codereview.adblockplus.org/29370996/diff/29371679/background.js File background.js (right): https://codereview.adblockplus.org/29370996/diff/29371679/background.js#newcode341 background.js:341: modules.messaging = { On 2017/01/13 12:12:10, Sebastian Noack wrote: ...
2 years, 10 months ago (2017-01-16 04:27:02 UTC) #8
Sebastian Noack
https://codereview.adblockplus.org/29370996/diff/29371679/background.js File background.js (right): https://codereview.adblockplus.org/29370996/diff/29371679/background.js#newcode341 background.js:341: modules.messaging = { On 2017/01/16 04:27:02, kzar wrote: > ...
2 years, 10 months ago (2017-01-16 13:46:38 UTC) #9
kzar
Patch Set 3 : Use EventEmitter https://codereview.adblockplus.org/29370996/diff/29371679/background.js File background.js (right): https://codereview.adblockplus.org/29370996/diff/29371679/background.js#newcode341 background.js:341: modules.messaging = { ...
2 years, 10 months ago (2017-01-16 14:25:56 UTC) #10
Sebastian Noack
https://codereview.adblockplus.org/29370996/diff/29372046/background.js File background.js (right): https://codereview.adblockplus.org/29370996/diff/29372046/background.js#newcode342 background.js:342: _eventEmitter: new EventEmitter(), Couldn't you do it like that: ...
2 years, 10 months ago (2017-01-16 15:00:03 UTC) #11
kzar
Patch Set 4 : Improve messaging implementation in background.js https://codereview.adblockplus.org/29370996/diff/29372046/background.js File background.js (right): https://codereview.adblockplus.org/29370996/diff/29372046/background.js#newcode342 background.js:342: ...
2 years, 10 months ago (2017-01-16 15:16:31 UTC) #12
Sebastian Noack
LGTM
2 years, 10 months ago (2017-01-16 16:01:25 UTC) #13
Thomas Greiner
2 years, 10 months ago (2017-01-17 17:14:42 UTC) #14
LGTM
Sign in to reply to this message.

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