727126 - Land Share code (Formerly known as F1) in Firefox

Status: RESOLVED WONTFIX

(Firefox :: General, defect)

Description

[David Dahl :ddahl]

Review and Land lab's Share (F1) functionality as a XUL addon. 

The Share code is currently written using Jetpack: https://github.com/mozilla/activities/

Comment 1

[David Dahl :ddahl]

I plan on doing a drive-by review today

Comment 2

[Shane Caraveo (:mixedpuppy)]

FYI, 

I'm planning on removing the "Worker" usage, replacing with pure postMessage, and have a plan for doing away with the "fakers".

I still have not implemented the discovery mechanism.

Once those are done, I should be able to remove addon-sdk and move it to a pure xul addon, from which we can generate patches to land.

Comment 3

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Does this integration purposes to refresh the feature of "send link with email", too?

The feature to send with email has be legacy already as the way to share URLs.
Many people use SNS to share a URL, not only e-mail.
If the new feature to share with SNS will be integrated to Firefox browser, I think it is the moment to reorganize the old sharing features.

Comment 4

[Shane Caraveo (:mixedpuppy)]

Right now send with email is not replaced, instead a share activity is made available with a set of default implementations, one of which is gmail.

Send with email could be replaced, IMO that would need to be a PM call.  Some effort in creating a new picker that can pick both online web-email interfaces and local external email clients would be necessary.

Comment 5

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Right, I understand there are some points.

However, if there is no replacing or considering, Firefox will have two similar features to share.
This problem might not talk in this bug, but I seem it should be considered.

Comment 6

[David Dahl :ddahl]

Attachment: Drive By Review just getting started

This is incomplete, I will have more code covered tomorrow

Comment 7

[David Dahl :ddahl]

Let me preface this by saying that I still need to do a bit more poking around in Jetpack's Worker code to see what everything is doing. This is a quick once-over to get some discussion going. Except for the Worker functionality that we will lose by dropping Jetpack, this is doable, but will take some time. I am not sure how much time yet:)

Some general comments:

Obviously, we need to kill off any jetpack and jQuery-related dependencies. 

Take a look at iq as a jQuery replacement: http://mxr.mozilla.org/mozilla-central/source/browser/components/tabview/iq.js

The JavaScript-navigator-property(s) should be moved into their own component files with a manifest instead of manually loading them. 

You should be able to register a chrome URI for any images you want to display in the UI via a manifest

A string bundle will need to be created as well as needeing to use the l10n functions. Localization is another issue altogther, surely the L10N community uses github these days for projects like this? 

Test suites: You can (generally) test a jsm with xpcshell tests, content DOM functionality with mochitest-plain and chrome DOM with browser-chrome mochitests. I understand you can create a Makefile for adding your tests to the testing infrastructure at build time when your xpi is packaged. (Some research is needed here, Dietrich told me about it) 

A question: Is the scope of this project right now to handle all kinds of sharing inclding any kind of media, or are we more concerned with just sharing links? There seems to be a bunch of code that is there to handle the registration of additional data types that a user might want to share? Perhaps I am wrong about this? How shall we scope this?

*** main.js ***

The jetpack unloading should be replaced by adding to each navigator-property a few observers to use for object clean up:
http://mxr.mozilla.org/mozilla-central/source/dom/base/ConsoleAPI.js?force=1#61

+MozActivitiesAPIContract = "@mozilla.org/openwebapps/mozActivities;1";
+MozActivitiesAPIClassID = Components.ID("{9175e12d-2377-5649-815b-2f49983d0ff3}");
please use var or let here, or better yet, just put these properties directly into the object

+function MozActivitiesAPI() {}
+MozActivitiesAPI.prototype = {
+  __proto__: NavigatorAPI.prototype,
Not sure I have ever seen the use of __proto__ like this in practice in Firefox front-end code. Not that I have looked, I do see it in some mobile code. The question then is should you put both of these navigator properties in a single source file? I am unsure.

+  _getObject: function(aWindow) {
+    return {
+      startActivity: function(activity, successCB, errorCB) {
+        let wm = Cc["@mozilla.org/appshell/window-mediator;1"].getService(Ci.nsIWindowMediator);
use Services.wm

+   let recentWindow = wm.getMostRecentWindow("navigator:browser");
Actually, you should never get a window like that, you should get it via its windowID, here is an example:

 getWindowByWindowId: function HS_getWindowByWindowId(aId)
http://mxr.mozilla.org/mozilla-central/source/browser/devtools/webconsole/HUDService.jsm#1447

You get the windowID like so: 
aWindow.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindowUtils).outerWindowID;
http://mxr.mozilla.org/mozilla-central/source/browser/devtools/webconsole/HUDService.jsm#1429

+ while (iter.hasMoreElements()) {
+    let aWindow = iter.getNext().QueryInterface(Ci.nsIDOMWindow);
+    let serviceInvocationHandler = require("services").serviceInvocationHandler;
+    aWindow.serviceInvocationHandler = new serviceInvocationHandler(aWindow);
+  }
+ function winWatcher(subject, topic) {
+    if (topic != "domwindowopened") return;
+    let serviceInvocationHandler = require("services").serviceInvocationHandler;
+    subject.serviceInvocationHandler = new serviceInvocationHandler(subject);
+  }
Adding the serviceInvocationHandler directly to the Window object may be frowned upon(as you are 'polluting the window'). This seems like something to stick in navigator via another nsIDOMGlobalPropertyInitializer, which is the best way to add a new property.

I am thinking that all of the "nsIComponentRegistrar" bits in that file can ga away ionce you are using manifests for the DOM properties.


*** services.js 

services.js will have to become a .jsm

I would tie the panel to the window ID, lazily getting each window when needed and discarding it when it is not needed. Holding on to all those window references can make this leak.

Jetpack tabs API will have to be replaced with Firefox's window tab events: TabOpen, TabClose, TabSelect and use the current tab as 'gBrowser.selectedTab'

+ // An 'agent' is trusted code running with chrome privs.  It gets a chance to
I would avoid using a label like "agent"

// hook into most aspects of a service operation to add additional value for
// the user.  This might include things like automatically bookmarking
// sites which have been shared etc.  Agents will be either builtin to
// the User-Agent (ie, into Firefox) or be extensions.
Do we need to figure this "agent" functionality out in more detailin this extension?

+  // we use document-element-inserted here rather than
+  // content-document-global-created so that other listeners using
+  // content-document-global-created will be called before us (e.g. injector.js
+  // needs to run first)
+  Services.obs.addObserver(this, 'document-element-inserted', false);
+  this.contentScriptFile = [require("self").data.url("servicesapi.js")];
Does 'document-element-inserted' fire before any events that might happen in the content DOM? Is it fired in a synchronously like 'content-document-global-created'? I wonder about race conditions here.

+    // XXX - update mediator registration to also include
+    // additional contentScriptFiles.
+    let url = this.mediator && this.mediator.url;
+    if (!url) {
+      url = require("self").data.url("service.html");
+    }
+    if (this.mediator) {
+      if (this.mediator.contentScriptFile) {
+        contentScriptFile = contentScriptFile.concat(this.mediator.contentScriptFile);
+      }
+      if (this.mediator.contentScript) {
+        contentScript = this.mediator.contentScript;
+      }
+    }

I am wondering if there is not a more "platformy" and scriptable way to inject code. Perhaps not?  

+    let thePanel = require("panel").Panel({
+      contentURL: url,
+      contentScriptFile: contentScriptFile,
+      contentScript: contentScript,
+      contentScriptWhen: "start",
+      width: this.width, height: this.height
+    });

We should switch to the xul panel with an embeddded browser element I think - then we will benefit from auto-hide nad show when switching tabs, etc.

+  get: function(activityName, cb) {
+    let activities = this._activitiesList[activityName] ? [].concat(this._activitiesList[activityName]) : [];
+    try {
+      // the owa api will need to be xpcom or something, we cannot import
+      // addon-sdk files from an external addon
We should talk about this - do you have aneed for another API that provisions activity types or something?

+      //var {FFRepoImplService} = require("openwebapps/api");
+      //FFRepoImplService.findServices(activityName, function(serviceList) {
+      //  // make a combo list of our internal activities and installed apps
+      //  activities = activities.concat(serviceList);
+      //  cb(activities);
+      //});
+    } catch (e) {
+    }
+    cb(activities);
Please use 'aCallback(activities)' or something more descriptive

+  }

+ function serviceInvocationHandler(win)
+ {
+   this._window = win;
+   this._popups = []; // save references to popups we've created already
+
+   let observerService = Cc["@mozilla.org/observer-service;1"].getService(Ci.nsIObserverService);
Please use 'Services.obs'

+  // if we open a new tab, close any mediator panels
+  win.gBrowser.tabContainer.addEventListener("TabOpen", function(e) {
+    for each (let mediator in this._popups) {
+      if (mediator.panel.isShowing) mediator.panel.hide();
+    }
+  }.bind(this));
+}

We won't need to track popups like this with the XUL panel element, TabOpen -> close others is built-in behavior as well as the reverse

+    if (topic === "openwebapp-installed" ||
+        topic === "openwebapp-uninstalled" ||
+        topic === "net:clear-active-logins")
+    {
+      // XXX TODO look at the change in the app and only reconfigure the related
+      // mediators.
What is the nexus of "Share" and open web apps?

+  removePanelsForWindow: function(evt) {
+    // this window is unloading
+    // nuke any popups targetting this window.
+    // XXX - this probably needs tweaking - what if the app is still
+    // "working" as the user navigates away from the page?  Currently
+    // there is no reasonable way to detect this though.
This function does not appear to ever be called. You can observe "inner-window-destroyed" to delete resources per window

*** mediatorapi.js ***


+function S4() {
+  return (((1+Math.random())*0x10000)|0).toString(16).substring(1);
+}
+function guid() {
+  return (S4()+S4()+"-"+S4()+"-"+S4()+"-"+S4()+"-"+S4()+S4()+S4());
+}
There is a uuid service: 
XPCOMUtils.defineLazyServiceGetter(this,
                                   "uuidGenerator",
                                   "@mozilla.org/uuid-generator;1",
                                   "nsIUUIDGenerator");
uuidGenerator.generateUUID();

I imagine you could add this to one of your navigator properties under a "utils" sub-property or something like that

+unsafeWindow.Service = Service;
is adding this property directly to the window part of an emerging spec? Again, it would be better to not add properties to the window, but rather to navigator.  

So the unsafeWindow global is part of the 'Worker' interface in jetpack. We will have to work out an alternate way to do this. Is this purely for content-privileged code injection? More research will be required here. This, I think, is a bug to file.

*** servicesapi.js ***

I am not sure why you have additional properties like "services" being attached to navigator.mozActivities here. I think you explained it too me in person, but am thinking you should be able to keep that code inside of the Javascript-navigator-property


*** nits ***

All 'if else' statements should look like:
if () {

} 
else {

}

Also, brace all if statements

Generally, you should name all functions added to a prototype: 
bar.prototype = {
  foo: function Share_foo(){},
}

Trailing commas are preferred: [1,2,3,] or { a: 1, b: 2, }

Arguments should be camelCase and prefixed with 'a': aTopic, aSubject

All opening function braces on newline. (Inline callbacks and such don't need to do this)

Comment 8

[Shane Caraveo (:mixedpuppy)]

(In reply to David Dahl :ddahl from comment #7)

In hindsight, this may have been a bit premature, I should have waited to get you this deep into the code until I had it fully moved back to a regular xul addon.

> Let me preface this by saying that I still need to do a bit more poking
> around in Jetpack's Worker code to see what everything is doing. This is a
> quick once-over to get some discussion going. Except for the Worker
> functionality that we will lose by dropping Jetpack, this is doable, but
> will take some time. I am not sure how much time yet:)

Don't bother (unless you otherwise want to), worker is being removed from use.  While there are certainly many useful things in this review, I think you looked at the wrong branch.  e.g. I've stopped using jetpack panel.  

There are two branches current work is happening in:

features/simple-share - implements share in the activities addon, and removes jetpack panel

> 
> Obviously, we need to kill off any jetpack and jQuery-related dependencies. 
> 

features/detox - where I'm really working towards removing major jetpack features, such as the worker use.

> The JavaScript-navigator-property(s) should be moved into their own
> component files with a manifest instead of manually loading them. 

re-xulification of the addon will happen once I remove major uses of jetpack features, which is getting closer in the detox branch.

> A question: Is the scope of this project right now to handle all kinds of
> sharing inclding any kind of media, or are we more concerned with just
> sharing links? There seems to be a bunch of code that is there to handle the
> registration of additional data types that a user might want to share?
> Perhaps I am wrong about this? How shall we scope this?

The share mechanism is based on web activities, which is a more general api.  Basically, we're doing activities + a set of default built-in fakers for major share providers.


> Adding the serviceInvocationHandler directly to the Window object may be
> frowned upon(as you are 'polluting the window'). This seems like something
> to stick in navigator via another nsIDOMGlobalPropertyInitializer, which is
> the best way to add a new property.

serviceInvocationHandler is on chrome windows, not content windows.  IIUC nsIDOMGlobalPropertyInitializer is for inserting into content.


> +  // we use document-element-inserted here rather than
> +  // content-document-global-created so that other listeners using
> +  // content-document-global-created will be called before us (e.g.
> injector.js
> +  // needs to run first)
> +  Services.obs.addObserver(this, 'document-element-inserted', false);
> +  this.contentScriptFile = [require("self").data.url("servicesapi.js")];
> Does 'document-element-inserted' fire before any events that might happen in
> the content DOM? Is it fired in a synchronously like
> 'content-document-global-created'? I wonder about race conditions here.

it is synchronous.


> *** mediatorapi.js ***
> *** servicesapi.js ***

These files were injected into content so no xpcom, however, in the new branches they are gone already.

Comment 9

[Shane Caraveo (:mixedpuppy)]

After some prodding, I just blocked out everything else and focused on the features/detox branch, it is now moved to a manifest based xul addon.  Still more stuff I'll do on it today to finish that out.

Comment 10

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

(In reply to David Dahl :ddahl from comment #7)
> 
> Obviously, we need to kill off any jetpack and jQuery-related dependencies. 
> 
> Take a look at iq as a jQuery replacement:
> http://mxr.mozilla.org/mozilla-central/source/browser/components/tabview/iq.
> js
> 

I think that it is no ploblem to kill of jQuery-related code completely. And also it need not to use iQ.

Currently codes (https://github.com/mozilla/activities/) does not depend so much on jQuery. If this degree of dependence, these code can be replaced by Normal DOM codes.

I recognize jQuery is useful to write cross-browser DOM codes and rich effects.
However, if the target is limited to mozilla platform, using normal DOM is better than using jQuery.

Comment 11

[David Dahl :ddahl]

Shane:

I think nsIDOMGlobalPropertyInitializer also works for Chrome windows as long as you are using the JavaScript-global-property category instead of the Navigator-global-property

Comment 12

[David Dahl :ddahl]

(In reply to Shane Caraveo (:mixedpuppy) from comment #9)
> After some prodding, I just blocked out everything else and focused on the
> features/detox branch, it is now moved to a manifest based xul addon.  Still
> more stuff I'll do on it today to finish that out.

Which branch is the latest? I see 3 branches worked on in about the same time frame of 7 days ago. Are you waiting on me to review it again, or are you still getting some of the functionality going?

Comment 13

[Shane Caraveo (:mixedpuppy)]

(In reply to David Dahl :ddahl from comment #12)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #9)
> > After some prodding, I just blocked out everything else and focused on the
> > features/detox branch, it is now moved to a manifest based xul addon.  Still
> > more stuff I'll do on it today to finish that out.
> 
> Which branch is the latest? I see 3 branches worked on in about the same
> time frame of 7 days ago. Are you waiting on me to review it again, or are
> you still getting some of the functionality going?

I think you can do a review at this point, I've a good set of bugs, all which need to be done.  You can find them under labs->f1 with whiteboard [share-activity]

Comment 14

[David Dahl :ddahl]

A few general comments first on the detox branch:

I must say you have made the code much more tidy:)

* We need to make this into a proper patch against mozilla-central as this will make future review for bugs easier to complete 
** A patch will also make it easier to figure out how to integrate Share code with mochitests
* We need the test boilerplate added - still need to figure out where to place it
* The UX needs a separate review on all three platforms. The Linux version is somewhat unpolished, see: http://img6.imagebanana.com/img/4lteneqj/Menu_124.png
* I noticed that the TabFocused event does not hide the panel when using the keyboard to switch tabs
* Will the addon be 'hidden' from the AddonManager? (Not visible in the UI) Is this possible now or does the AddonManager need a new feature?
* Will the AddonManager update this extension silently?
* Will we need a standing bug to update the allowed versions in install.rdf for each Firefox release?

Global nits:

Add "TODO: " before all of the bug references in the code

Use the new MPL boilerplate from here: http://www.mozilla.org/MPL/headers/

This form:

const {classes: Cc, interfaces: Ci, utils: Cu, resources: Cr} = Components;

...is all the rage in jetpack dev, but AFAIK, not in firefox dev, maybe it does not matter? I never use it. Maybe it is ok now? Maybe it is ok for this addon? 

Use our new "defineLazyModuleGetter": http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/loader/XPCOMUtils.jsm#250 for loading modules. its niiice.

I think jsms should start with caps: 'DefaultServices.jsm'

Name all functions please

All 'else' and 'catch' on newlines please

// modules/defaultServices.jsm

The login-manager is a property of Services.jsm, better to use it: Services.logins.*

We should ask mak to review 'frecencyForUrl' in defaultServices.jsm

// mediatorPanel.jsm
+ _createPanelOverlay: function(aWindow)
+ ...
+   tb.style.width = "660px";
+   tb.style.height = "400px";
Are we going to figure out a way for each partner content to set some custom default w/h that can be dynamically changed when needed?

+ _createPopupPanel: function(aWindow) {
+    this._createPanelOverlay(aWindow);
+    let tb = this.tabbrowser;
+    let activityRegistry = Cc["@mozilla.org/activitiesRegistry;1"]
+                            .getService(Ci.mozIActivitiesRegistry);
You should create a lazy getter that is global to the module so we can reuse this - unless loading it fresh is part of the design?

+     activityRegistry.getActivityHandlers(this.action, cb);
+    this.panel.addEventListener('popupshown', this.onPanelShown.bind(this));
+    this.panel.addEventListener('popuphidden', this.onPanelHidden.bind(this));
+    this.panel.addEventListener('TabSelect', this.onPanelShown.bind(this)); //
You may need to listen for additional events for keyboard tab navigation (I could be wrong, but it is not working right on linux)

+      buttons = [{
+        label: "try again",
+        accessKey: null,
+        callback: function () {
+          self.window.setTimeout(function () {
+            self.show();
+          }, 0);
+        }
We should avoid setTimeout if possible, setTimeout is evil in our testing infrastructure, plus I don't think that will pass review for front-end code

The 'console' object should be removed - just create a log() function for hacking (to be removed) and use Cu.reportError for things you would like to surface to the error console.

... more to come asap ...

Comment 15

[Dietrich Ayala (:dietrich)]

(In reply to David Dahl :ddahl from comment #14)
> * We need to make this into a proper patch against mozilla-central as this
> will make future review for bugs easier to complete 

There's some info about bundling here, from when I was testing the BrowserID addon: https://etherpad.mozilla.org/identity-fx

> * Will we need a standing bug to update the allowed versions in install.rdf
> for each Firefox release?

IIRC there's already magic in the Makefile to preprocess that number in. See how the feedback add-on does it (linked in above etherpad iirc)

Comment 16

[David Dahl :ddahl]

// components/api.js

Looks good except for nits

// components/registry.js

+  _getUsefulness: function activityRegistry_findMeABetterName(url, loginHost) {
+    let hosturl = Services.io.newURI(url, null, null);
+    loginHost = loginHost || hosturl.scheme+"://"+hosturl.host;
nit: use spaces between + operator

+   registerActivityHandler: function activityRegistry_registerActivityHandler(aActivityName, aURL, aManifest) {
line feed after "registerActivityHandler:"
use a shorter function prefix perhaps, you should aim for 80 char limit on lines

+       // the owa api will need to be xpcom or something, we cannot import
+      // addon-sdk files from an external addon
+      //var {FFRepoImplService} = require("openwebapps/api");
+      //FFRepoImplService.findServices(activityName, function(serviceList) {
+      //  // make a combo list of our internal activities and installed apps
+      //  activities = activities.concat(serviceList);
+      //  cb(activities);
+      //});
Is there a bug on the OWA implementation providing a service/JSM/API to consume?

+    // Check that we aren't already displaying our notification
+    if (!notification) {
+      let message = "This site supports additional functionality for Firefox, would you like to install it?";
Add this string to the string bundle? (I am unsure at this point if there is one)

+      buttons = [{
+        label: "Yes",
+        accessKey: null,
+        callback: function () {
+          aWindow.setTimeout(function () {
+            aCallback();
+          }, 0);
+        }
+      }];
Label should come from string bundle. 
Let's figure out a way to not use setTimeout here as well

in  loadManifest: function activityRegistry_loadManifest() it will make sense to drop all of the console.log for Cu.reportError
nit: newlines after else and catch

+  discoverActivity: function activityRegistry_discoverActivity(aDocument, aData) {
+    // BUG 732266 this is probably heavy weight, is there a better way to watch for
+    // links in documents?
Good question, we should ask a content hacker about this - perhaps what you are doing is fine as well?

Comment 17

[David Dahl :ddahl]

So, if this code needs to land in browser/app/profile/extensions - I can make it into a proper patch with tests in browser/base/content/test?

Or, should this code land elsewhere via the installer in a global extensions directory?

Perhaps we do not care if Share is also active in a new profile that 0.0001% of people create?

Comment 18

[Dietrich Ayala (:dietrich)]

(In reply to David Dahl :ddahl from comment #17)
> So, if this code needs to land in browser/app/profile/extensions - I can
> make it into a proper patch with tests in browser/base/content/test?
> 
> Or, should this code land elsewhere via the installer in a global extensions
> directory?

in browser/app/profile/extensions is how we bundled the Feedback add-on. allows the code to be in-tree and therefore mxr-able, change-track-able, etc.

tests can go inside the add-on directory. just need some make incantation to not bundle them in the xpi.

in fact, should probably figure out a way to ship it unzipped.

> 
> Perhaps we do not care if Share is also active in a new profile that 0.0001%
> of people create?

no comprendo.

Comment 19

[David Dahl :ddahl]

(In reply to Dietrich Ayala (:dietrich) from comment #18)
> > Perhaps we do not care if Share is also active in a new profile that 0.0001%
> > of people create?
> 
> no comprendo.

I was referring to extensions that are global but reside outside of the firefox directory like the ubuntu theme extension that is forced on you by the packaging system. Nevermind.

Comment 20

[David Dahl :ddahl]

So I have been peeking at the css, which is really not my thing. I noticed a few pngs that I never see anywhere in the UI (on linux):  list-style-image: url("../gnomestripe/share-button.png");

dietrich: perhaps we should rope someone in to better review the CSS/style/ux - where to begin there? 

Another thing I noticed was that the very first time I used share after install, I had a twitter option, and was able to use it, however, on subsequent launches, I lost the twitter option. I then restarted, went to twitter, logged in and bookmarked it. Then I started share again and had the twitter option back.

Sounds like some of the persistent storage might help with that?

Comment 21

[David Dahl :ddahl]

// overlay.xul

+ <overlay id="overlay"
+         xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
+  
+  <script type="application/x-javascript;1.8">
the type attribute should not be needed here as this is xul the latest js should be by default

I noticed you commented out:
+    //panel.setAttribute("noautohide", "true");
in modules/registry.jsm

and you have the following in the overlay:

+      // panels are not closing on TabOpen
+      gBrowser.tabContainer.addEventListener("TabOpen", function(e) {
+        var panels = document.getElementsByClassName('activities-panel');
+        for each (let panel in panels) {
+          if (panel.state == "open") panel.hidePopup();
+        }
+      });

Did you try setting noautohide to "false" when creating the panel. That should just work I thought.

Comment 22

[Shane Caraveo (:mixedpuppy)]

(In reply to David Dahl :ddahl from comment #21)
> // overlay.xul
> 
> + <overlay id="overlay"
> +        
> xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
> +  
> +  <script type="application/x-javascript;1.8">
> the type attribute should not be needed here as this is xul the latest js
> should be by default

sometimes old habits die hard.

> I noticed you commented out:
> +    //panel.setAttribute("noautohide", "true");
> in modules/registry.jsm

it is just there for the occasion I need to debug with the panel open.

> and you have the following in the overlay:
> 
> +      // panels are not closing on TabOpen
> +      gBrowser.tabContainer.addEventListener("TabOpen", function(e) {
> +        var panels = document.getElementsByClassName('activities-panel');
> +        for each (let panel in panels) {
> +          if (panel.state == "open") panel.hidePopup();
> +        }
> +      });
> 
> Did you try setting noautohide to "false" when creating the panel. That
> should just work I thought.

this is unrelated to noautohide.  There is/was one condition under which panels were not autohiding, that may have been fixed at some point, I'll have to figure out what that condition was and retest/comment the code.

Comment 23

[Shane Caraveo (:mixedpuppy)]

I've gone through, done a lot of cleanup, implemented persistent storage for activity handlers, added a pref tab in the panel (needs lots of ux love), fixed a handful of bugs.  The storage code is largely from OWA and probably needs some formatting updates.

Comment 24

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

Has there been an update on this?

Comment 25

[Dietrich Ayala (:dietrich)]

Shane, is this still moving forward? IIRC we need a decision from the Product team about how to prioritize/resource this.

Comment 26

[Shane Caraveo (:mixedpuppy)]

(In reply to Dietrich Ayala (:dietrich) from comment #25)
> Shane, is this still moving forward? IIRC we need a decision from the
> Product team about how to prioritize/resource this.

Was just chatting about share with Michael Hanson.  IMO a finish up review might be useful.  There are 3 major items left though:

- sync the Activities API with what B2G is doing in bug 715814
- UX help is desperately needed
- BizDev possibly needed, depending on UX results
- sync discovery between Share and SocialAPI

These may well be things that can all be done post-landing.

IMO Share should be a part of the SocialAPI even though they are separate addons, I think the product team should treat them as one functional feature set.

Comment 27

[Dietrich Ayala (:dietrich)]

Thanks Shane. Is the Share functionality expressed here? https://github.com/mozilla/socialapi-dev/blob/chrome-toolbar-item/docs/socialAPI.md

If it's a part of the Social API then it should be included fully in that spec.

Comment 28

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

Now that the Social API work is well underway, I don't think we'll want to duplicate this effort. See bug 765874 for the Share Button part of the Social API.