Add "notifications.get" message handler for the popup

[kzar]

Background

With #5593 we're replacing direct use of the background page from the popup window with messaging. We need to do this for the notification handling code too, but the required message handler has not been added to adblockplusui/messageResponder.js yet.

What to change

Add a "notifications.get" message handler to adblockplusui/messageResponders.js.

It should return an Array of active notifications, or an empty Array if there aren't any. The notifications can be fetched using getActiveNotifications() in adblockpluschrome/lib/notificationHelper.js. For each notification in the Array the texts property should be populated with the result of getLocalizedTexts() from adblockpluscore/lib/notifications.js.

The message handler should optionally take a displayMethod field, for which if specified restricts the returned notifications to the given display method e.g. "popup". The handler can make use of shouldDisplay() in adblockpluschrome/lib/notificationHelper.js to filter the notifications by the given display method.

[greiner]

I'd recommend choosing "notifications.get" and passing {active: true} instead of "notifications.getActive" to keep it consistent with the existing naming scheme.

We could even include the "shouldDisplay" information in there, for instance, by returning its associated display methods. Same applies to the localized texts because based on what I see ​how it's used this should be sufficient.

Example:

ext.backgroundPage.sendMessage({
  type: "notifications.get",
  active: true
}, ([notification]) =>
{
  if (!notification)
    return;

  let [{data, displayMethods, texts}] = notifications;
  ...
})

[kzar]

Sounds sensible to me, except I'm not sure how we could return non-active notifications. I've updated the description does it look OK to you now?

[mjethani]

notifications.get could also take a shouldDisplay argument.

[greiner]

Replying to kzar:

Sounds sensible to me, except I'm not sure how we could return non-active notifications. I've updated the description does it look OK to you now?

If there are no inactive notifications then only returning the active ones sounds good to me. I was under the impression that some of them might be inactive.

Replying to mjethani:

notifications.get could also take a shouldDisplay argument.

That'd already be covered by the "displayMethods" in the response but I guess we could instead add this to the query as

{
  type: "notifications.get",
  displayMethod: "popup"
}

to retrieve only the notifications that we are interested in. In that case we wouldn't need to pass all display methods to the UI anymore.

@kzar What do you think about this approach?

[kzar]

Cool idea, I've updated the description again.

[abpbot]

A commit referencing this issue has landed:
​Issue 5835 - Add notifications.get message handler