Jared Wein [:jaws] (please needinfo? me) Reporter
	Description • 12 years ago

The Notification.show method wasn't super necessary. The nice thing about it was that it meant that the caller didn't need to build up all the of the details about the notification if all it wanted to do is pop out the notification from a dismissed state.

However, this function isn't really necessary. If there are multiple places where show() would have been called, the calling of show() can just be refactored to reduce this duplication.

This patch attempts to remove the duplication and go back to just using PopupNotifications.show().

	Dana Keeler (she/her) (use needinfo) (:keeler for reviews)
	Comment 1 • 12 years ago

Comment on attachment 708000 [details] [diff] [review]
WIP patch

Review of attachment 708000 [details] [diff] [review]:
-----------------------------------------------------------------

This seems like a reasonable solution.

::: browser/base/content/browser-plugins.js
@@ +278,5 @@
>        return !isInvisible;
>      });
>  
>      let notification = PopupNotifications.getNotification("click-to-play-plugins", aBrowser);
> +    if (notification && plugins.length > 0 && !haveVisibleCTPPlugin && !this._notificationDisplayedOnce)

We probably don't need 'notification' here anymore.

::: toolkit/content/PopupNotifications.jsm
@@ -70,5 @@
>      return anchorElement;
>    },
> -
> -  reshow: function() {
> -    this.owner._reshowNotificationForAnchor(this.anchorElement);

I think _reshowNotificationForAnchor was added along with this function, so you can probably revert that, too.

	Jared Wein [:jaws] (please needinfo? me) Reporter
	Comment 2 • 12 years ago

This patch is almost ready, but is failing two tests that I need to look in to.

	Jared Wein [:jaws] (please needinfo? me) Reporter
	Comment 3 • 12 years ago

Saneyuki, would you be able to help fix the two failing tests (browser_plugnnotifications.js and browser_plugins_added_dynamically.js)?

	Tetsuharu OHZEKI [:tetsuharu] (UTC+9)
	Comment 4 • 12 years ago

I haven't read their codes well yet, but I'll saying my opinion is that:

I seem the new code sometimes calls PopupNotifications.show() with |dismissed = true|. But test files assumes that browser always show the doorhanger popup (This means always pass |dismissed = false|.) Thus the doorhanger's event callback are not called.

The patch part 3 which I proposed in bug 820678 always show the doorhanger popup with passing |dismissed = false| for avoiding this test's assumption.
However, the new codes don't continues PopupNotifications::Notification object. We cannot use this hack.

So we might need to change the test codes to non-depend on the doorhanger popup, or... continues the PopupNotifications::Notification object after calls PopupNotifications.show() via gPluginHandler.showClickToPlayNotification().

(Incidentally, I wrote my stance about removing Notification.reshow() is bug 820678's Comment38, the part of replying to Gavin.)

	Jared Wein [:jaws] (please needinfo? me) Reporter
	Comment 5 • 10 years ago

Unassigning as I haven't been working on this and I don't want to hold people up.

	Matthew N. [:MattN]
	Comment 6 • 10 years ago

Bulk move to Toolkit::Notifications and Alerts

Filter on notifications-and-alerts-component.