Issue 5835 - Add some notification related message handlers

Issue 5835 - Add some notification related message handlers

Depends on this review: https://codereview.adblockplus.org/29567746/

[kzar, 2017-10-06 13:02:27]

Patch Set 1

[kzar, 2017-10-06 20:08:34]

Patch Set 2 : Updated to match the new issue description

[Sebastian Noack, 2017-10-08 01:35:35]

https://codereview.adblockplus.org/29567743/diff/29567850/messageResponder.js
File messageResponder.js (right):

https://codereview.adblockplus.org/29567743/diff/29567850/messageResponder.js...
messageResponder.js:31: const {getActiveNotification, shouldDisplay} =
require("notificationHelper");
You have to implement mocks for these APIs in background.js, otherwise this line
will throw an error when running any UI page standalone with the test server in
adblockplusui. However, for now, the mock implementation can probably be noop,
as there is no code that will use it, hence it cannot even be tested, before
there is a popup implementation inside adblockplusui. Yet, it still needs to
exist at least, otherwise undefined will be attempted to restructured here.

https://codereview.adblockplus.org/29567743/diff/29567850/messageResponder.js...
messageResponder.js:355: notification.texts =
NotificationStorage.getLocalizedTexts(notification,
It seems inappropriate to modify the original object. That way this property
will also be available to other code, while we just want to pass it to the UI.

[kzar, 2017-10-08 10:08:16]

Patch Set 3 : Addressed Sebastian's feedback

https://codereview.adblockplus.org/29567743/diff/29567850/messageResponder.js
File messageResponder.js (right):

https://codereview.adblockplus.org/29567743/diff/29567850/messageResponder.js...
messageResponder.js:31: const {getActiveNotification, shouldDisplay} =
require("notificationHelper");
On 2017/10/08 01:35:35, Sebastian Noack wrote:
> You have to implement mocks for these APIs in background.js, otherwise this
line
> will throw an error when running any UI page standalone with the test server
in
> adblockplusui. However, for now, the mock implementation can probably be noop,
> as there is no code that will use it, hence it cannot even be tested, before
> there is a popup implementation inside adblockplusui. Yet, it still needs to
> exist at least, otherwise undefined will be attempted to restructured here.

Whoops, Done.

https://codereview.adblockplus.org/29567743/diff/29567850/messageResponder.js...
messageResponder.js:355: notification.texts =
NotificationStorage.getLocalizedTexts(notification,
On 2017/10/08 01:35:35, Sebastian Noack wrote:
> It seems inappropriate to modify the original object. That way this property
> will also be available to other code, while we just want to pass it to the UI.

Good point, Done.

[Sebastian Noack, 2017-10-08 19:12:36]

That looks better. I put myself back to CC, since this is Thomas his module.

[Thomas Greiner, 2017-10-09 10:52:59]

https://codereview.adblockplus.org/29567743/diff/29569555/background.js
File background.js (right):

https://codereview.adblockplus.org/29567743/diff/29569555/background.js#newco...
background.js:145: shouldDisplay() { return true; }
Coding style: "Opening braces always go on their own line."

Alternatively, you could use an arrow function instead.

https://codereview.adblockplus.org/29567743/diff/29569555/messageResponder.js
File messageResponder.js (right):

https://codereview.adblockplus.org/29567743/diff/29569555/messageResponder.js...
messageResponder.js:357: return Object.assign({}, notification, {texts});
Detail: The third argument is redundant because you could write it as
`Object.assign({texts}, notification)`.

[kzar, 2017-10-09 11:13:51]

Patch Set 4 : Addressed nits

https://codereview.adblockplus.org/29567743/diff/29569555/background.js
File background.js (right):

https://codereview.adblockplus.org/29567743/diff/29569555/background.js#newco...
background.js:145: shouldDisplay() { return true; }
On 2017/10/09 10:52:59, Thomas Greiner wrote:
> Coding style: "Opening braces always go on their own line."
> 
> Alternatively, you could use an arrow function instead.

Done.

https://codereview.adblockplus.org/29567743/diff/29569555/messageResponder.js
File messageResponder.js (right):

https://codereview.adblockplus.org/29567743/diff/29569555/messageResponder.js...
messageResponder.js:357: return Object.assign({}, notification, {texts});
On 2017/10/09 10:52:59, Thomas Greiner wrote:
> Detail: The third argument is redundant because you could write it as
> `Object.assign({texts}, notification)`.

Good point, Done.

[Thomas Greiner, 2017-10-09 12:39:33]

LGTM