Don't animate icon for notifications with URL filters

[greiner]

Background

In ​ui#197 we are planning to add a notification targeted to a website the user visits.

We already have "urlFilters" for achieving that for the anti-adblock notification but we want to show a less obtrusive message in the icon popup instead. That means that both the icon and the message inside the icon popup should remain unchanged on tabs that are unrelated to the active notification.

While we can easily determine whether to show the message in the icon popup when opening it, the icon animation is global which means that we'd either have to ensure that it's only shown when a page is shown that matches any of the URL filters or that we don't animate it for any notifications with URL filters.
Looking through the code, I'd say that the latter seems far simpler.

What to change

Don't animate the toolbar icon if the active notification contains URL filters.

[sebastian]

I wonder whether it makes sense to bind the appearance of the notification and the condition under which it is shown (e.g. urlFilters) together, or whether we should rather introduce a new severity (or a separate flag) for notifications that should not animate the icon?

[greiner]

Each of those would be viable options and, admittedly, it's not ideal to make such behavior implicit which is why I'd like to disconnect the various display methods from notification types during the upcoming notification system revamp.

We could add a flag such as "sticky" to indicate that the notification should stick to any matching pages. However, since such a flag would only be relevant for notifications which have URL filters and since all such notifications are targeted to specific pages, it would seem more appropriate to make the behavior dependent on whether there are URL filters.

But please keep in mind that the main reason for disabling the icon animation is not because we don't want to animate the icon but because we can't animate the icon only for tabs with matching pages.
In comparison, if we were to just add a "animateIcon" flag, we may end up with the icon animating for a tab who doesn't have a message in the icon popup.

[greiner]

I've attached a review for the change I'm proposing. Admittedly, this is merely a workaround so I'd be happy to revisit this change soon after when we start tackling the notification system as a whole.

[kzar]

Sounds pragmatic to me, especially if you guys plan to rewrite much of this logic anyway.

[abpbot]

A commit referencing this issue has landed:
​Issue 7093 - Only animate icon for global notifications