+++ This bug was initially created as a clone of Bug #1122032 +++

Using an iconbox here would involve quite a bit of coding around assumption being made that the iconbox is being used in a browser - as in window type browser - and not an iframe like chatwindow. This is the most straightforward approach to me. I couldn't find any existing tests for the PopupNotifications for the social API usecase, so I assume you expect me to create these from scratch?

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fc66d6c07cf2

Comment on attachment 8560458 [details] [diff] [review] Part 1: Support the screensharing doorhanger in Social API chat windows and allow for mutiple independent notification icons per type Review of attachment 8560458 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/socialchat.xml @@ +42,5 @@ > + ["webRTC-sharingScreen-", ""] > + ]); > + for (let [getterPrefix, idPrefix] of kAnchorMap) { > + let getter = getterPrefix + "popupnotificationanchor"; > + let anonid = (idPrefix ? idPrefix : getterPrefix) + "icon"; This: idPrefix ? idPrefix : getterPrefix can be simplified to: idPrefix || getterPrefix

Comment on attachment 8560459 [details] [diff] [review] Part 2: Allow multiple anchors for multiple notifications without using an iconbox Review of attachment 8560459 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/modules/PopupNotifications.jsm @@ +653,5 @@ > * > * @param notifications an array of Notification instances. if null, > * notifications will be retrieved off the current > * browser tab > + * @param anchors is a Set of XUL elements that the notifications panel(s) a XUL element or a Set of XUL elements @@ +683,5 @@ > // hide icons of the previous tab. > this._hideIcons(); > } > > + let anchorElements = anchors; Is there a reason why we have 2 different variables that seem to have the same content? Could we just s/anchorElements/anchors/g without making any behavior change? @@ +734,5 @@ > } > }, > > + _updateAnchorIcons: function PopupNotifications_updateAnchorIcons(notifications, > + anchorElements) { nit: fix indent. @@ +856,5 @@ > n.dismissed = false; > }); > > // ...and then show them. > + this._update(notifications, new Set([anchor])); You can revert this change.

(In reply to Mike de Boer [:mikedeboer] from comment #2) > I couldn't find any existing tests for the PopupNotifications for the social > API usecase, so I assume you expect me to create these from scratch? Hmm... I'm more interested in ensuring we don't break the non-SocialAPI cases, but given the test coverage for PopupNotifications is quite good already, I guess it's already covered. If there's a straightforward way to test the SocialAPI cases, that would be nice, but I won't block on this if it seems to be more effort than it's worth.

(In reply to Florian Quèze [:florian] [:flo] from comment #6) > Hmm... I'm more interested in ensuring we don't break the non-SocialAPI > cases, but given the test coverage for PopupNotifications is quite good > already, I guess it's already covered. This is certainly true! The try push already revealed a failing testcase, so I'll be fixing that now. > If there's a straightforward way to test the SocialAPI cases, that would be > nice, but I won't block on this if it seems to be more effort than it's > worth. I'll take a look, for sure. I think there ought to be coverage here, so I think spending two days on this won't be time a-wasted.

This failing test looks like an artifact of a broken build. Retry: https://treeherder.mozilla.org/#/jobs?repo=try&revision=285c4207b56c

Pushed to fx-team as: https://hg.mozilla.org/integration/fx-team/rev/2338f0e2a520 https://hg.mozilla.org/integration/fx-team/rev/6a6bb80ab896

https://hg.mozilla.org/mozilla-central/rev/2338f0e2a520 https://hg.mozilla.org/mozilla-central/rev/6a6bb80ab896

(In reply to Mike de Boer [:mikedeboer] from comment #0) Empty comment 0 == bad. Could you explain what support for multiple anchors was needed for? The anchor logic is so complicated that it's hard to follow and there wasn't a test to help me understand the need for this. Is the idea that there will be 2 anchors that both show the same content when clicked?

(In reply to Matthew N. [:MattN] from comment #11) > Empty comment 0 == bad. Hey Matt! It's been while... So you're absolutely right here. Won't happen again in the future. > Could you explain what support for multiple anchors was needed for? The > anchor logic is so complicated that it's hard to follow and there wasn't a > test to help me understand the need for this. Adding a test was indeed filed as a follow-up with lower priority (bug 1132404). This was decided and OK'd, because you're right that the anchor logic is complicated and the way Social API is using this atm is not captured in tests as well. This'd mean that I had to create tests for the existing functionality, which is a significant amount of work. The current use-cases for the anchors all apply when they're inside an iconbox/ box and the regression test coverage for that is pretty good. As long as we're not regressing this, I think we're in pretty good shape. In hindsight I'm quite happy that we split up the work like this, because the upcoming Hello design refresh of the conversation window sports _no_ doorhanger icons next to the window controls anymore: http://cl.ly/image/431C3y0P1C3m. This means that the work done here can be backed out, thus reverting back to the old situation without the need to support a different anchor configuration in PopupNotifications.jsm. It'd be going to far to call me clairvoyant here, but I certainly had a gut feeling that this wouldn't be a long-term solution. > Is the idea that there will be 2 anchors that both show the same content > when clicked? No, the idea here is as follows: Social API has one doorhanger anchor defined in the markup by default, which is functioning as a catch-all for all notifications that belong to that window. Now this means that _if_ Hello would request access to more devices or services, like screensharing, geolocation, etc, these would all be attached to this single anchor. The anchor icon would turn into a 'generic' icon - the blue info bubble while requesting a window to share - since PopupNotifications.jsm takes care of styling the anchors as well. We needed to keep the existing functionality intact, but also needed to add a second anchor specifically used when window sharing was requested/ activated. If I'd introduce an iconbox/ box to wrap the anchors in, I'd have to make significant changes to work around the fact that there can be a catch-all anchor in there and _not_ all the other anchors that are defined in browser.xul are present. With the changes made here I can keep the catch-all anchor for microphone & camera notifications and supply specific anchors for other features that would need a separate anchor (screensharing in this case, window and browser sharing more specifically). Perhaps you find this added functionality interesting after this explanation and would like to keep it, even after the Hello design refresh. In that case I will have to fix 1132404 and add some dev-docs someplace. Please feel free to ping me on IRC or contact me directly otherwise if I was unable to fully clarify things here!

Verified that the issue is fixed on Windows 7 64-bit, Mac OS X 10.9.5 and Ubuntu 14.04 32-bit using Firefox 38.0 RC build 2 after activating Hello screensharing feature.