1202933 - Show the origin of web notifications

Status: RESOLVED FIXED

(Toolkit Graveyard :: Notifications and Alerts, defect)

Description

[Matthew N. [:MattN]]

Both Safari and Chrome show the website's origin in their notification UI and I think it was an oversight that we don't show it in ours as sites could spoof notifications from other sites. We should follow what Safari does in Notification Center and do something similar for XUL notifications and Gnome.

Comment 1

[Edwin Wong [:edwong]]

Need a design here. Triage is unsure if adding origin impacts the web app icon and overall copy layout.  We should consider a refresh of the notification layout.

Comment 2

[Matthew N. [:MattN]]

For OS X we should just copy what Safari does as we have a limited API.

Comment 3

[Bill Maggs (bmaggs)]

For OSX and Linux this approach is fine, but for Windows we don't have access to native platform and have to put the origin in our own notification design.

Comment 4

[Edwin Wong [:edwong]]

Kit's going to pick this up for this iteration

Comment 5

[Lina Butler [:lina]]

Attachment: MozReview Request: Bug 1202933, Part 1 - Show the origin for XUL notifications. r=MattN,wchen

Bug 1202933 - Show the origin of web notifications. f?MattN

Comment 6

[Lina Butler [:lina]]

Comment on attachment 8666103 [details]
MozReview Request: Bug 1202933, Part 1 - Show the origin for XUL notifications. r=MattN,wchen

Here's a first cut that adds the origin to XUL and Cocoa notifications. It looks like the principal gives us what we need, so we can avoid adding an extra param to `showAlertNotification`. I didn't look at Gnome/libnotify yet.

I think the next step is for me to build Firefox under Windows and Linux, so I can test out the UI.

Am I on the right track, Matthew? :-)

Comment 7

[Matthew N. [:MattN]]

https://reviewboard.mozilla.org/r/20443/#review18363

Yep, this is definitely on the right track. I only skimmed some parts. I think it would be good to split the cocoa and libnotify into separate commits since they will have separate reviewers.

::: toolkit/components/alerts/nsAlertsUtils.cpp:13
(Diff revision 1)
> +nsAlertsUtils::GetOrigin(nsIPrincipal* aPrincipal, nsAString& aOrigin)

I think the designs call for "via" to be prepended at least on some platforms. Perhaps this/another helper could do that too?

::: toolkit/components/alerts/nsAlertsUtils.cpp:23
(Diff revision 1)
> +  bool isSystemPrincipal = false;
> +  scriptSecurityManager->IsSystemPrincipal(aPrincipal, &isSystemPrincipal);
> +  if (isSystemPrincipal) {
> +    return;

I think you also need to handle the null principal and possibly expanded principals. There is a helper on nsContentUtils to check for system or expanded.

::: toolkit/components/alerts/nsAlertsUtils.cpp:34
(Diff revision 1)
> +    if (NS_FAILED(principalURI->GetPrePath(origin))) {

Have you considered using nsIPrincipal.originNoSuffix instead? Have you seen precdence for where this approach has been used in the UI instead?

::: toolkit/components/alerts/nsXULAlerts.cpp:75
(Diff revision 1)
> +  nsAutoString subtitle;
> +  nsAlertsUtils::GetOrigin(aPrincipal, subtitle);
> +  scriptableAlertOrigin->SetData(subtitle);

I kinda wonder if we should just declare that this argument is for the origin, instead of a subtitle since the new designs have it at the bottom: https://mozilla.invisionapp.com/share/3A4A895XZ#/screens/103249717

::: toolkit/components/alerts/resources/content/alert.xul:34
(Diff revision 1)
> +        <label id="alertSubtitleLabel" class="alertSubtitle plain"/>

I think we should give an ID indicating this is for the origin since the new designs don't treat it like a subtitle. I also think you should put it at the bottom already now. I think you can put a flexible spacer element above it to have it go to the bottom.

Comment 8

[Matthew N. [:MattN]]

https://reviewboard.mozilla.org/r/20443/#review18383

::: toolkit/components/alerts/resources/content/alert.xul:33
(Diff revision 1)
>          <label id="alertTitleLabel" class="alertTitle plain"/>
> +        <label id="alertSubtitleLabel" class="alertSubtitle plain"/>
>          <label id="alertTextLabel" class="alertText plain"/>

We should also add a test in toolkit/components/alerts/test/ that the origin is displayed. You can take a look at toolkit/components/alerts/test/test_alerts_noobserve.htm for an example of how to get a reference to the alert XUL "window". Let me know if you need help with this.

Comment 9

[Lina Butler [:lina]]

Comment on attachment 8666103 [details]
MozReview Request: Bug 1202933, Part 1 - Show the origin for XUL notifications. r=MattN,wchen

Bug 1202933, Part 1 - Show the origin for XUL notifications. r?MattN

Comment 10

[Lina Butler [:lina]]

Attachment: MozReview Request: Bug 1202933, Part 2 - Show the origin for OS X notifications. r=MattN r?wchen

Bug 1202933, Part 2 - Show the origin for OS X notifications. r?MattN

Comment 11

[Lina Butler [:lina]]

OK, ready for another round of feedback. What should we do about libnotify? (Maybe prepend the origin to the summary via a localized string, like "mozilla.org | I am a notification"?)

Comment 12

[Edwin Wong [:edwong]]

are we sure we want the primary domain only (yahoo.com) - or do we want exact domain (mail.yahoo.com)?  Permissions dialog has exact domain displayed.  If you're running on Heroku or site.tumblr.com, site.blogger.com, or site.wordpress.com you'd want the exact domain.

Comment 13

[Lina Butler [:lina]]

(In reply to Edwin Wong [:edwong] from comment #12)
> are we sure we want the primary domain only (yahoo.com) - or do we want
> exact domain (mail.yahoo.com)?

The current patch shows the exact domain of the page that sent the notification. That could be unwieldy; for Yahoo Mail, we'd show "us-mg5.mail.yahoo.com", just like Chrome. But, to your point, "peoplescanningqrcodes.tumblr.com" is very different than "animalshugging.tumblr.com," so I think keeping exact is sensible.

Comment 14

[Edwin Wong [:edwong]]

chrome displays exact domain: http://people.mozilla.org/~ewong2/push/ymail2.png

Comment 15

[Bill Maggs (bmaggs)]

We need to start out with the exact domain, I would think  We simply don't have reliable sender policies or whitelists of other stuff worked out.  Over time websites will make the origins look less hinky.

Comment 16

[Matthew N. [:MattN]]

Comment on attachment 8666103 [details]
MozReview Request: Bug 1202933, Part 1 - Show the origin for XUL notifications. r=MattN,wchen

https://reviewboard.mozilla.org/r/20443/#review18865

Since I don't review C++ too much, could you flag William Chen for review of the C++ parts?

::: toolkit/components/alerts/resources/content/alert.js
(Diff revisions 1 - 2)
>  
> -  // arguments[2] --> the alert subtitle
> +  // arguments[2] --> the alert source
>    // arguments[3] --> the alert text

It's probably not a good idea to renumber these as extensions may depend on the old order. Just append yours to the bottom. This code should eventually be cleaned up to pass an object instead of relying on indices but I don't remember if that works.

::: toolkit/components/alerts/nsAlertsUtils.h:1
(Diff revision 2)
> +/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */

This doesn't seem to match the style being used. 2 vs. 4

::: toolkit/components/alerts/nsAlertsUtils.h:19
(Diff revision 2)
> +  IsActionablePrincipal(nsIPrincipal* aPrincipal);

Could you use this in the OSX code that currently has similar logic or file a follow-up for it.

::: toolkit/components/alerts/nsAlertsUtils.cpp:1
(Diff revision 2)
> +/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */

2 vs. 4

::: toolkit/components/alerts/nsAlertsUtils.cpp:20
(Diff revision 2)
> +  return aPrincipal && !(
> +    nsContentUtils::IsSystemOrExpandedPrincipal(aPrincipal) ||
> +    aPrincipal->GetIsNullPrincipal()

Why not distribute the negation inside to avoid the braces and only have one type of binary operator (`&&`)?

::: toolkit/components/alerts/test/mochitest.ini:11
(Diff revision 2)
> +[test_origin.html]

I think you'll want to skip for android here too since you're looking for XUL windows. I think the test will also fail on Linux if libnotify is installed so the checking for compatible devices should probably improve at some point. You can file a follow-up for this if it's not trivial.

::: toolkit/components/alerts/test/test_alerts.html:68
(Diff revision 2)
> +    var MAC = SpecialPowers.Cc["@mozilla.org/xre/app-info;1"]
> +                           .getService(Ci.nsIXULRuntime).OS == "Darwin";

SpecialPowers.Services.appinfo.OS

::: toolkit/components/alerts/test/test_origin.html:2
(Diff revision 2)
> +<!-- Any copyright is dedicated to the Public Domain.
> +     http://creativecommons.org/publicdomain/zero/1.0/ -->

FYI: this is no longer needed in new tests.

::: toolkit/components/alerts/test/test_origin.html:24
(Diff revision 2)
> +const MAC = Cc["@mozilla.org/xre/app-info;1"]
> +             .getService(Ci.nsIXULRuntime).OS == "Darwin";

Same comment as the other test.

::: toolkit/components/alerts/test/test_origin.html:44
(Diff revision 2)
> +    notifier.showAlertNotification(null, "Notification test",
> +                                   "Surprise! I'm here to test notifications!",
> +                                   false, alertName, observe, alertName,
> +                                   null, null, null, principal);
> +    if (MAC) {
> +      notifier.closeAlert(alertName);

I wonder if it's safe to assume you can close immediately after showing?

::: toolkit/components/alerts/test/test_origin.html:89
(Diff revision 2)
> +    is(origin, localizedOrigin, "Wrong origin for principal");

Nit: Normally the 3rd argument is like an assertion (like your others) but this one is different.

::: toolkit/components/alerts/test/test_origin.html:94
(Diff revision 2)
> +  if (!("@mozilla.org/alerts-service;1" in Cc)) {
> +    todo(false, "Alerts service does not exist in this application");
> +  } else {

Nit: use an early return instead to reduce nesting

::: toolkit/components/alerts/test/test_origin.html:103
(Diff revision 2)
> +    testNoPrincipal()
> +      .then(testSystemPrincipal)
> +      .then(testExpandedPrincipal)
> +      .then(testNullPrincipal)
> +      .then(testNodePrincipal)
> +      .catch(error => {
> +        todo(false, "Error running tests", error);
> +      })

Can you switch this file to use add_task instead? We try to use that for all new tests. See bug 1187701 comment 13.

Comment 17

[Matthew N. [:MattN]]

Comment on attachment 8667306 [details]
MozReview Request: Bug 1202933, Part 2 - Show the origin for OS X notifications. r=MattN r?wchen

https://reviewboard.mozilla.org/r/20707/#review18871

::: widget/cocoa/OSXNotificationCenter.mm:249
(Diff revision 1)
> +    notification.subtitle = [NSString stringWithCharacters:(const unichar *)subtitle.BeginReading()
> +                                                    length:subtitle.Length()];

Does nsCocoaUtils::ToNSString help here?

Comment 18

[Lina Butler [:lina]]

Comment on attachment 8666103 [details]
MozReview Request: Bug 1202933, Part 1 - Show the origin for XUL notifications. r=MattN,wchen

Bug 1202933, Part 1 - Show the origin for XUL notifications. r=MattN r?wchen

Comment 19

[Lina Butler [:lina]]

Comment on attachment 8667306 [details]
MozReview Request: Bug 1202933, Part 2 - Show the origin for OS X notifications. r=MattN r?wchen

Bug 1202933, Part 2 - Show the origin for OS X notifications. r=MattN r?wchen

Comment 20

[Lina Butler [:lina]]

https://reviewboard.mozilla.org/r/20443/#review18865

> It's probably not a good idea to renumber these as extensions may depend on the old order. Just append yours to the bottom. This code should eventually be cleaned up to pass an object instead of relying on indices but I don't remember if that works.

Oops. I thought some of the arguments are optional, but they're all passed in. It looks like `OpenWindow` takes an `nsISupports`; we can define an interface with the attributes in a follow-up bug. Thanks!

> I think you'll want to skip for android here too since you're looking for XUL windows. I think the test will also fail on Linux if libnotify is installed so the checking for compatible devices should probably improve at some point. You can file a follow-up for this if it's not trivial.

Updated the manifest, and changed the test to bail early if `@mozilla.org/system-alerts-service;1` exists.

> I wonder if it's safe to assume you can close immediately after showing?

I cargo-culted this from `test_alerts.html`. That test had two typos in it (missing `MAC`; `alertname` vs. `alertName`), which caused it to bail with a `todo`...so I don't know if this actually works. :-) Let's see what try says.

> Can you switch this file to use add_task instead? We try to use that for all new tests. See bug 1187701 comment 13.

Neat!

Comment 21

[Lina Butler [:lina]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bfca2ba5bc0b

Comment 22

[Lina Butler [:lina]]

Comment on attachment 8666103 [details]
MozReview Request: Bug 1202933, Part 1 - Show the origin for XUL notifications. r=MattN,wchen

Bug 1202933, Part 1 - Show the origin for XUL notifications. r=MattN r?wchen

Comment 23

[Lina Butler [:lina]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5a112b042821

Comment 24

[William Chen [:wchen]]

Comment on attachment 8666103 [details]
MozReview Request: Bug 1202933, Part 1 - Show the origin for XUL notifications. r=MattN,wchen

https://reviewboard.mozilla.org/r/20443/#review19037

::: toolkit/components/alerts/nsAlertsUtils.cpp:17
(Diff revision 4)
> +nsAlertsUtils::IsActionablePrincipal(nsIPrincipal* aPrincipal)

It looks like this function returns whether we should display the origin/source, in which case it should be named something like "ShouldDisplaySource". If "actionable" means something else here then it should be commented in the header.

Make this method private if there aren't any plans to use this anywhere else.

::: toolkit/components/alerts/nsAlertsUtils.cpp:26
(Diff revision 4)
> +nsAlertsUtils::GetLocalizedOrigin(nsIPrincipal* aPrincipal, nsAString& aResult)

How about renaming this GetDisplayedSource and add a comment to the header that it returns a localized source string containing the origin?

::: toolkit/components/alerts/nsAlertsUtils.cpp:53
(Diff revision 4)
> +nsAlertsUtils::GetOrigin(nsIPrincipal* aPrincipal, nsAString& aOrigin)

This method is confusing. You can get the origin out of a principal using GetOrigin. Also this method isn't actually getting the origin, but rather just the host and port which isn't sufficient to define the origin. Is this intentional?

::: toolkit/components/alerts/resources/content/alert.xul:36
(Diff revision 4)
> +        <label id="alertSourceLabel" class="alertSource plain"/>

This looks like it may leave an empty space when we don't have a source to display which will probably happen for various parts of Firefox that use the alerts service such as the download manager. I'm not sure how important this is but I just wanted to point it out.

Comment 25

[Lina Butler [:lina]]

https://reviewboard.mozilla.org/r/20443/#review19037

> It looks like this function returns whether we should display the origin/source, in which case it should be named something like "ShouldDisplaySource". If "actionable" means something else here then it should be commented in the header.
> 
> Make this method private if there aren't any plans to use this anywhere else.

It determines whether we should display the source and action buttons; the XUL backend just doesn't have the buttons yet. Added a comment to the header. Kept the method public because the OS X backend uses it, too.

> How about renaming this GetDisplayedSource and add a comment to the header that it returns a localized source string containing the origin?

Done. Renamed to `GetSource`, since we refer to the "source" everywhere else.

> This method is confusing. You can get the origin out of a principal using GetOrigin. Also this method isn't actually getting the origin, but rather just the host and port which isn't sufficient to define the origin. Is this intentional?

Agreed; I'm intentionally excluding the scheme, so this method is poorly named. Renamed to `GetSourceHostPort`.

> This looks like it may leave an empty space when we don't have a source to display which will probably happen for various parts of Firefox that use the alerts service such as the download manager. I'm not sure how important this is but I just wanted to point it out.

Switched to creating the spacer and label dynamically in `alert.js`.

Comment 26

[Lina Butler [:lina]]

Comment on attachment 8666103 [details]
MozReview Request: Bug 1202933, Part 1 - Show the origin for XUL notifications. r=MattN,wchen

Bug 1202933, Part 1 - Show the origin for XUL notifications. r=MattN r?wchen

Comment 27

[Lina Butler [:lina]]

Attachment: MozReview Request: Bug 1202933, Part 2 - Show the origin for OS X notifications. r=MattN,wchen

Bug 1202933, Part 2 - Show the origin for OS X notifications. r=MattN r?wchen

Comment 28

[Lina Butler [:lina]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1a305dfbe5b2

Comment 29

[Matthew N. [:MattN]]

https://reviewboard.mozilla.org/r/20443/#review19137

::: toolkit/components/alerts/resources/content/alert.js:43
(Diff revisions 4 - 5)
> +        let spacer = document.createElement('spacer');
> +        spacer.setAttribute('flex', '1');
> +        alertTextBox.appendChild(spacer);
> +
> +        let label = document.createElement('label');
> +        label.className = 'alertSource plain';
> +        label.setAttribute('value', window.arguments[10]);
> +        alertTextBox.appendChild(label);

Sorry to make you revert some of this but can you leave the markup in the xul file and simply toggle element.hidden? It makes it easier to test styling and to find the markup for the source line.

I also think that have the spacer and label even when they're not used would make it easier for styling because there would be consistency but we can try hiding them for now. The spacer probably doesn't even need to be hidden.

Comment 30

[Matthew N. [:MattN]]

Comment on attachment 8669366 [details]
MozReview Request: Bug 1202933, Part 2 - Show the origin for OS X notifications. r=MattN,wchen

https://reviewboard.mozilla.org/r/21185/#review19139

Comment 31

[William Chen [:wchen]]

Comment on attachment 8666103 [details]
MozReview Request: Bug 1202933, Part 1 - Show the origin for XUL notifications. r=MattN,wchen

https://reviewboard.mozilla.org/r/20443/#review19167

looks good, r+ with comments addressed and Matt's suggestions for the XUL markup.

::: toolkit/components/alerts/nsAlertsUtils.h:37
(Diff revision 5)
> +  GetSourceHostPort(nsIPrincipal* aPrincipal, nsAString& aHostPort);

either rename aHostPort -> aSource or update the comment.

::: toolkit/components/alerts/nsAlertsUtils.cpp:61
(Diff revision 5)
> +  if (NS_FAILED(aPrincipal->GetURI(getter_AddRefs(principalURI)))) {

NS_WARN_IF(NS_FAILED(aPrincipal->GetURI(getter_AddRefs(principalURI))))

::: toolkit/components/alerts/nsAlertsUtils.cpp:68
(Diff revision 5)
> +  if (NS_FAILED(principalURI->GetHostPort(hostPort))) {

NS_WARN_IF

Comment 32

[William Chen [:wchen]]

Comment on attachment 8669366 [details]
MozReview Request: Bug 1202933, Part 2 - Show the origin for OS X notifications. r=MattN,wchen

https://reviewboard.mozilla.org/r/21185/#review19169

Comment 33

[Lina Butler [:lina]]

Comment on attachment 8666103 [details]
MozReview Request: Bug 1202933, Part 1 - Show the origin for XUL notifications. r=MattN,wchen

Bug 1202933, Part 1 - Show the origin for XUL notifications. r=MattN,wchen

Comment 34

[Lina Butler [:lina]]

Comment on attachment 8669366 [details]
MozReview Request: Bug 1202933, Part 2 - Show the origin for OS X notifications. r=MattN,wchen

Bug 1202933, Part 2 - Show the origin for OS X notifications. r=MattN,wchen

Comment 35

[Lina Butler [:lina]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0eeda0282068

Comment 36

[Matthew N. [:MattN]]

https://reviewboard.mozilla.org/r/20443/#review19181

::: toolkit/components/alerts/resources/content/alert.js:34
(Diff revision 6)
>    // arguments[9] --> an optional callback listener (nsIObserver)
> +  // arguments[10] -> the localized alert source string
>  
>    switch (window.arguments.length) {
>      default:
> +    case 11: {
> +      let label = document.getElementById('alertSourceLabel');
> +      if (window.arguments[10]) {
> +        label.hidden = false;
> +        label.setAttribute('value', window.arguments[10]);
> +      } else {
> +        label.hidden = true;
> +      }

Hmm… I wonder if we should have just passed the principal along instead and exposed the helpers via XPIDL. I guess we can change it later, if needed.

Comment 37

[Lina Butler [:lina]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1763e148406574a02a516e199a820f7a33297e3d
Bug 1202933, Part 1 - Show the origin for XUL notifications. r=MattN,wchen

https://hg.mozilla.org/integration/mozilla-inbound/rev/fbcc1bb170e9b8d108ee02b30643b48e76f49db1
Bug 1202933, Part 2 - Show the origin for OS X notifications. r=MattN,wchen

Comment 38

[Matthew N. [:MattN]]

Thanks for landing this before PTO :)

(In reply to Kit Cambridge [:kitcambridge] (PTO from 2015-10-06 to 2015-10-16) from comment #37)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> fbcc1bb170e9b8d108ee02b30643b48e76f49db1
> Bug 1202933, Part 2 - Show the origin for OS X notifications. r=MattN,wchen

I think this may get merge conflicts with bug 1208295 at merge time… Our team usually uses fx-team although I guess some of our notification patches are touching dom/ and widget/ code so maybe they should be landing on inbound… We should probably try to use the same integration branch to help the sheriffs out.

Comment 39

[Lina Butler [:lina]]

(In reply to Matthew N. [:MattN] (behind on mail) from comment #38)
> I think this may get merge conflicts with bug 1208295 at merge time… Our
> team usually uses fx-team although I guess some of our notification patches
> are touching dom/ and widget/ code so maybe they should be landing on
> inbound… We should probably try to use the same integration branch to help
> the sheriffs out.

Gah, my apologies. I'll use fx-team for our work from now on.

Comment 40

[Phil Ringnalda (:philor)]

Won't matter anyway, backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/a1736287f404 for timeouts on Mulet, https://treeherder.mozilla.org/logviewer.html#?job_id=15207453&repo=mozilla-inbound.

Comment 41

[Lina Butler [:lina]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4ae52c85b11d

Comment 42

[Lina Butler [:lina]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=07c7c73cd32f

Comment 43

[Lina Butler [:lina]]

https://hg.mozilla.org/integration/fx-team/rev/d907d6e29c5e265a411ffb9eda5dd4a25640b9c7
Bug 1202933, Part 1 - Show the origin for XUL notifications. r=MattN,wchen

https://hg.mozilla.org/integration/fx-team/rev/624f967efe43130cf4a74bebad06179620be5317
Bug 1202933, Part 2 - Show the origin for OS X notifications. r=MattN,wchen

Comment 44

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d907d6e29c5e
https://hg.mozilla.org/mozilla-central/rev/624f967efe43