747649 - click to play plugin notification actions shouldn't explicitly remove the notification

Status: RESOLVED WORKSFORME

(Firefox :: General, defect)

Description

[:Gavin Sharp [email: [e-mail]]]

PopupNotification actions don't need to remove their notification, the PopupNotifications code handles that automatically.

There are a couple places where this happens:
- in the "never allow" action: http://hg.mozilla.org/mozilla-central/annotate/d1ac8e24872c/browser/base/content/browser.js#l7323
- in activatePlugins: http://hg.mozilla.org/mozilla-central/annotate/d1ac8e24872c/browser/base/content/browser.js#l7185

The latter can also be called from the click event handler - presumably it needs to keep removing the notification in that case, but not when called from a PopupNotification action.

Comment 1

[:Felipe Gomes (needinfo for replies!)]

Does it cause any problems if they do it? Those two were left on purpose. In activatePlugins as you noticed it's necessary for the case where it's called from the click handler, and it didn't seem worth to differentiate the two callers. The one in the "never allow" action we were going to get rid of it (last chunk in bug 711618 comment 29) but IIRC Jared said that would make a test easier, and it seemed harmless so we left it there.

Comment 2

[:Gavin Sharp [email: [e-mail]]]

(In reply to Felipe Gomes (:felipe) from comment #1)
> Does it cause any problems if they do it?

It looks like it's probably harmless.

> Jared said that would make a test easier, and it seemed harmless so we left it
> there.

I'd be interested in knowing how it can help tests, it seems pretty much equivalent.

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #2)
> I'd be interested in knowing how it can help tests, it seems pretty much
> equivalent.

For some reason the notification doesn't disappear while running the tests without this code. I think it's because of the way that I'm calling the callback in the test:
> popupNotification.secondaryActions[1].callback();
Is there a better way to call a secondary action of a notification? This method is probably skipping some pre/post-callback code paths.

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

I can fix the test so that it actually opens the doorhanger and runs the same code path that the user will see.

Comment 5

[Benjamin Smedberg]

The notificiation no longer dismisses on button click by design.