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

Unified Diff: notification.js

Issue 29567749: Issue 5593 - Use messaging for the popup's notification code (Closed)
Patch Set: Created Oct. 6, 2017, 1:07 p.m.
Use n/p to move between diff chunks; N/P to move between comments.
Jump to:
View side-by-side diff with in-line comments
Download patch
« no previous file with comments | « dependencies ('k') | no next file » | no next file with comments »
Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
Index: notification.js
diff --git a/notification.js b/notification.js
index 18eff540b3dddc0664594f2fce064a2e3f742183..14b21733a2dac5fe7a058ad62cfd87aebac6c886 100644
--- a/notification.js
+++ b/notification.js
@@ -17,23 +17,48 @@
"use strict";
-const {require} = ext.backgroundPage.getWindow();
-
-const {Utils} = require("utils");
-const {Notification} = require("notification");
-const {getActiveNotification, shouldDisplay} = require("notificationHelper");
-
function getDocLinks(notification)
{
+ let docLinks = [];
+
if (!notification.links)
- return [];
+ return Promise.resolve(docLinks);
- let docLinks = [];
- notification.links.forEach(link =>
+ return Promise.all(
+ notification.links.map(link =>
+ {
+ return new Promise((resolve, reject) =>
+ {
+ ext.backgroundPage.sendMessage({
Manish Jethani 2017/10/06 14:26:04 We should use runtime.sendMessage here so we lose
kzar 2017/10/06 20:05:34 Done.
+ type: "app.get",
+ what: "doclink",
+ link
+ }, resolve);
+ });
+ })
+ );
+}
+
+function getActiveDisplayedNotification()
Manish Jethani 2017/10/06 14:26:04 Nit: I feel like "displayed" implies it's already
kzar 2017/10/06 20:05:34 Acknowledged.
+{
+ return new Promise((resolve, reject) =>
{
- docLinks.push(Utils.getDocLink(link));
+ ext.backgroundPage.sendMessage({
+ type: "notifications.getActive"
+ }, notification =>
Manish Jethani 2017/10/06 14:26:05 I may be wrong about this, but "notification =>" h
kzar 2017/10/06 20:05:34 I think this looks OK like it is. If we decide to
+ {
+ if (!notification)
Manish Jethani 2017/10/06 14:26:05 We would normally add braces here around the if bl
kzar 2017/10/06 20:05:34 IIRC we considered enforcing that braces for if...
+ resolve(notification);
+ else
+ {
+ ext.backgroundPage.sendMessage({
+ type: "notifications.shouldDisplay",
+ method: "popup",
+ notificationType: notification.notificationType
Manish Jethani 2017/10/06 14:26:05 It should be notification.type, not notification.n
kzar 2017/10/06 20:05:34 Acknowledged.
+ }, shouldDisplay => { resolve(shouldDisplay && notification); });
+ }
+ });
});
- return docLinks;
}
function insertMessage(element, text, links)
@@ -63,45 +88,59 @@ function insertMessage(element, text, links)
window.addEventListener("load", () =>
{
- let notification = getActiveNotification();
- if (!notification || !shouldDisplay("popup", notification.type))
- return;
-
- let texts = Notification.getLocalizedTexts(notification);
- let titleElement = document.getElementById("notification-title");
- titleElement.textContent = texts.title;
-
- let docLinks = getDocLinks(notification);
- let messageElement = document.getElementById("notification-message");
- insertMessage(messageElement, texts.message, docLinks);
-
- messageElement.addEventListener("click", event =>
+ getActiveDisplayedNotification().then(notification =>
{
- let link = event.target;
- while (link && link !== messageElement && link.localName !== "a")
- link = link.parentNode;
- if (!link)
+ if (!notification)
return;
- event.preventDefault();
- event.stopPropagation();
- ext.pages.open(link.href);
- });
- let notificationElement = document.getElementById("notification");
- notificationElement.className = notification.type;
- notificationElement.hidden = false;
- notificationElement.addEventListener("click", event =>
- {
- if (event.target.id == "notification-close")
- notificationElement.classList.add("closing");
- else if (event.target.id == "notification-optout" ||
- event.target.id == "notification-hide")
+ let titleElement = document.getElementById("notification-title");
+ let messageElement = document.getElementById("notification-message");
+
+ ext.backgroundPage.sendMessage({
+ type: "notifications.getLocalizedTexts",
+ notification
Manish Jethani 2017/10/06 14:26:04 I'm not sure how message.locale gets in there. Is
kzar 2017/10/06 20:05:34 If not specified we go with the default I think.
+ }, texts =>
{
- if (event.target.id == "notification-optout")
- Notification.toggleIgnoreCategory("*", true);
+ titleElement.textContent = texts.title;
+
+ getDocLinks(notification).then(docLinks =>
+ {
+ insertMessage(messageElement, texts.message, docLinks);
+ });
+ });
- notificationElement.hidden = true;
- notification.onClicked();
- }
- }, true);
+ messageElement.addEventListener("click", event =>
Manish Jethani 2017/10/06 14:26:04 I feel like this should be added in the getDocLink
kzar 2017/10/06 20:05:34 Done.
+ {
+ let link = event.target;
+ while (link && link !== messageElement && link.localName !== "a")
+ link = link.parentNode;
+ if (!link)
+ return;
+ event.preventDefault();
+ event.stopPropagation();
+ ext.pages.open(link.href);
+ });
+
+ let notificationElement = document.getElementById("notification");
+ notificationElement.className = notification.type;
+ notificationElement.hidden = false;
+ notificationElement.addEventListener("click", event =>
+ {
+ if (event.target.id == "notification-close")
+ notificationElement.classList.add("closing");
+ else if (event.target.id == "notification-optout" ||
+ event.target.id == "notification-hide")
+ {
+ if (event.target.id == "notification-optout")
+ {
+ ext.backgroundPage.sendMessage({
Manish Jethani 2017/10/06 14:26:04 You meant to use the togglePref function here, def
kzar 2017/10/06 20:05:34 Done.
+ type: "notifications_ignoredcategories"
+ });
+ }
+
+ notificationElement.hidden = true;
+ notification.onClicked();
+ }
+ }, true);
+ });
}, false);
« no previous file with comments | « dependencies ('k') | no next file » | no next file with comments »

Powered by Google App Engine
This is Rietveld