When the Loop button is placed in the Hamburger menu, it still opens up a new panel, rather than sliding a panel into the Hamburger menu from the right. Additionally, with its panel, the Social API does things like sharing the iframe instance across windows/locations, repositioning in the DOM as necessary and avoiding extra loads. Hence, we should share this code between Loop and the Social API. I'm attaching an initial, non-functioning, concept patch that I created.

it basically behaves in the bottom of the browser, old style restore bar as well.

Picking this up again.

This patch now works, although there's a couple of XXX that I need to tidy up still. It has been suggested to me in other bugs that Loop should switch to using a CustomizableUI widget as that's the new thing. I've taken a short look at it, and it seems it would provide much of the infrastructure that we're needing here, however it doesn't match all the necessary constraints at the moment, e.g. not creating the iframe until it is actually used. I suspect with some extra work it could be done (but that needs someone with more experience of the code), however, I'm also thinking that transitioning to shared code between social and loop to fix this current bug also makes it easier for a future bug to change both loop and social to use the CustomizableUI widget. Hence, given this patch is almost complete, save for fixing the XXXs and checking tests, then I'm thinking it is still worth landing this as an in-between. I'm not quite 100% sure, hence, I'm looking for feedback on the proposed route (and any on the patch if appropriate).

Comment on attachment 8439748 [details] [diff] [review] WIP Share the panel construction with the social api panels. Review of attachment 8439748 [details] [diff] [review]: ----------------------------------------------------------------- just a couple of quick questions - I'll look in more detail next week. ::: browser/base/content/browser-panelframe.js @@ +1,1 @@ > +/* This Source Code Form is subject to the terms of the Mozilla Public Can this be a module? ::: browser/components/loop/MozLoopAPI.jsm @@ +24,5 @@ > + } > + this._enabled = enable; > + > + if (enable) { > + Services.obs.addObserver(injectController, "document-element-inserted", false); why do we need this? Don't we explicitly create all windows where this should be inserted? Social does this because we inject into tabs with the same origin as a social provider, but loop doesn't have that requirement.

(In reply to Mark Hammond [:markh] from comment #4) > ::: browser/base/content/browser-panelframe.js > @@ +1,1 @@ > > +/* This Source Code Form is subject to the terms of the Mozilla Public > > Can this be a module? Now I look at it again, I think that is possible. > ::: browser/components/loop/MozLoopAPI.jsm > @@ +24,5 @@ > > + } > > + this._enabled = enable; > > + > > + if (enable) { > > + Services.obs.addObserver(injectController, "document-element-inserted", false); > > why do we need this? Don't we explicitly create all windows where this > should be inserted? Social does this because we inject into tabs with the > same origin as a social provider, but loop doesn't have that requirement. I think I did it that way as it seemed simpler, and gave us the possibility of being able to load the mozLoop API into tabs (via the pref). However, now I look at it again, it looks like we should setup PanelFrame.showPopup to either return or have a callback for use in the Loop case.

Updated patch to make PanelFrame into a module. I've added the docs and fixed the XXX, as well as run the social tests against it. Loop works fine in the application menu and in the toolbar. The changes to PanelFrame where a couple of extra arguments to showPopup to pass in the window and PanelUI. I also added a callback so that Loop could inject the API into the iframe if necessary. In _attachNotificatonPanel I also changed: aParent.hidden = !SocialUI.enabled; to aParent.hidden = false; As this no longer seemed relevant (if the popup is being shown, then we should be enabled), and Shane confirmed it didn't seem necessary anymore.

Comment on attachment 8440834 [details] [diff] [review] Make Social API's panel handling generic so that Loop can use it as well, enabling Loop to function properly in the Application menu. Review of attachment 8440834 [details] [diff] [review]: ----------------------------------------------------------------- r- only because I think the use of the error listener isn't correct in this context - see below. ::: browser/base/content/browser-loop.js @@ +6,5 @@ > let LoopUI; > > XPCOMUtils.defineLazyModuleGetter(this, "injectLoopAPI", "resource:///modules/loop/MozLoopAPI.jsm"); > XPCOMUtils.defineLazyModuleGetter(this, "MozLoopService", "resource:///modules/loop/MozLoopService.jsm"); > +XPCOMUtils.defineLazyModuleGetter(this, "PanelFrame", let's not split this line. If it really did need to be indented, the style-guide calls for indented lines to match the open bracket - I'm somewhat sympathetic to 4-space indents there but only when the first param is also indented that way) - but for now, let's just be consistent. @@ +39,3 @@ > > + PanelFrame.showPopup(window, PanelUI, event.target, > + "loop", null, "about:looppanel", don't like this indentation either :) It's also not consistent with code in this patch - eg, removeEventListener above. See also https://developer.mozilla.org/en-US/docs/User:GavinSharp_JS_Style_Guidelines @@ +47,5 @@ > * delayedStartup. > */ > initialize: function() { > MozLoopService.initialize(); > + } please leave the trailing comma - it means this line doesn't need to be touched when a new entry is added. ::: browser/base/content/browser-social.js @@ +15,5 @@ > XPCOMUtils.defineLazyModuleGetter(this, "SharedFrame", > "resource:///modules/SharedFrame.jsm"); > > +XPCOMUtils.defineLazyModuleGetter(this, "PanelFrame", > + "resource:///modules/PanelFrame.jsm"); FTR, I'm fine with this indentation specifically because it is consistent with the lines above :) @@ +1187,5 @@ > // attach our notification panel if necessary > let origin = aToolbarButton.getAttribute("origin"); > let provider = Social._getProviderFromOrigin(origin); > > + PanelFrame.showPopup(window, PanelUI, aToolbarButton, more indentation nits :) ::: browser/modules/PanelFrame.jsm @@ +96,5 @@ > + if (frame.socialErrorListener) > + frame.socialErrorListener.remove(); > + if (frame.docShell) { > + frame.docShell.isActive = false; > + Social.setErrorListener(frame, this.setPanelErrorMessage.bind(this)); I don't think loop wants the social error listener, as it will have buttons to reload social providers - which will fail for loop panels. Also, I can't see how it works even for social - setPanelErrorMessage is in browser-social.js. Which concerns me, as I thought we had reasonable tests for the error listener. And finally, I if frame.docShell is null, presumably it is going to go non-null at some point in the future, at which point we'd still want to do this - but I don't see that either. I guess it's possible social has that problem too. Not sure what I'm missing here...

(In reply to Mark Hammond [:markh] from comment #7) > ::: browser/modules/PanelFrame.jsm > @@ +96,5 @@ > > + if (frame.socialErrorListener) > > + frame.socialErrorListener.remove(); > > + if (frame.docShell) { > > + frame.docShell.isActive = false; > > + Social.setErrorListener(frame, this.setPanelErrorMessage.bind(this)); > > Also, I can't see how it works even for social - setPanelErrorMessage is in > browser-social.js. Which concerns me, as I thought we had reasonable tests > for the error listener. > > And finally, I if frame.docShell is null, presumably it is going to go > non-null at some point in the future, at which point we'd still want to do > this - but I don't see that either. I guess it's possible social has that > problem too. I've taken a look at this, and I've found it seems broken as far back as FF 30 which is when the social panel was introduced. I used https://talkilla.mozillalabs.com to test it as that's got a out of date cert. Examining the code further there's a few issues: - frame.socialErrorListener is never going to be set, as this is a newly created iframe. - frame.docShell is null / not able to be set at this time. The docShell issue is concerning, as I don't see how to get it to work - I think the iframe needs to be created probably with about:blank and then the page loaded. Given that it is an existing social issue, I think fixing that is a bit beyond the scope of this bug, and we should put it out to its own bug. Although I'm a little concerned it may impact the architecture we need here. I'll attach the current patch which I've fixed for the issues you mentioned, and I've removed this error listener code completely.

See previous comment.

Comment on attachment 8441262 [details] [diff] [review] Make Social API's panel handling generic so that Loop can use it as well, enabling Loop to function properly in the Application menu v2 Review of attachment 8441262 [details] [diff] [review]: ----------------------------------------------------------------- Bummer it's broken for social, but I agree that shouldn't block you. Please file a followup for social and CC mixedpuppy.

As discussed on irc, tweaked the module getters in PanelFrame.jsm to not import Social, and to tidy the DynamicResizeWatcher import. Landed on inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/1693213d84b6

(In reply to Mark Hammond [:markh] from comment #10) > Bummer it's broken for social, but I agree that shouldn't block you. Please > file a followup for social and CC mixedpuppy. Filed bug 1026444.

I verify this is now fixed, however I filed bug 1053476 as I think the Loop panel could use some better UX when in the Hamburger menu.