Closed Bug 839445 Opened 7 years ago Closed 7 years ago

PopupNotifications need to know about location changes in background tabs

Categories

(Toolkit :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla21

People

(Reporter: dao, Assigned: dao)

References

Details

(Keywords: dev-doc-needed)

Attachments

(1 file, 1 obsolete file)

Attached patch untested patch (obsolete) — Splinter Review
Keywords: dev-doc-needed
Attached patch patchSplinter Review
Assignee: nobody → dao
Attachment #711768 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #715101 - Flags: review?(gavin.sharp)
Hrm, so thinking about this more, prior to bug 825804's patch this actually kind of worked, because the onLocationChange fired by the tab switch would cause us to then remove the notification (at least in the persistence==0 case), right? But it would be treated as a single "navigation" and not properly handle cases where persistence>0 and the background tab had been navigated while in the background.
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #3)
> Hrm, so thinking about this more, prior to bug 825804's patch this actually
> kind of worked, because the onLocationChange fired by the tab switch would
> cause us to then remove the notification (at least in the persistence==0
> case), right?

How would it? selectedBrowser.lastURI matches the browser's current URI when switching to it, so PopupNotifications.locationChange wouldn't be called.
Comment on attachment 715101 [details] [diff] [review]
patch

Can you remove the __lookupGetter__ (per bug 811857) while you're bitrotting that patch?

I think we should have locationChange() either fall noisily if not passed a browser (currently _getNotificationsForBrowser will do the wrong thing if aBrowser==null), or have it default to using the current browser's notifications - not sure which is best.

r=me with those changes.
Attachment #715101 - Flags: review?(gavin.sharp) → review+
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #5)
> I think we should have locationChange() either fall noisily if not passed a
> browser (currently _getNotificationsForBrowser will do the wrong thing if
> aBrowser==null), or have it default to using the current browser's
> notifications - not sure which is best.

I made it throw an exception similarly to PopupNotifications.show.

https://hg.mozilla.org/integration/mozilla-inbound/rev/fc380df34bd6
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/fc380df34bd6
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Blocks: 843800
You need to log in before you can comment on or make changes to this bug.