773351 - hookup the mozSocial api

Status: RESOLVED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

[Shane Caraveo (:mixedpuppy)]

we need to inject navigator.mozSocial on the social content panels/sidebar, this bug is to port over the code from the addon.

Comment 1

[Shane Caraveo (:mixedpuppy)]

Attachment: mozSocialAPI.patch

mozSocial api implementation

Comment 2

[Shane Caraveo (:mixedpuppy)]

Attachment: mozSocialAPI.patch

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Attachment: modified patch

Shane: I made a couple of changes to the patch:
- Initialize in SocialService instead of browser code
- adjust the attachToWindow code to use createObjectIn/makeObjectPropsNormal/etc. stuff from bug 634156 rather than sandbox insertion
- add a test (in browser/ since it depends on browser-specific UI pieces (panel UI)
- other minor cleanup

mrbkap: can you review the attachToWindow function specifically? It's attempting to inject a function into content script that returns a chrome JS object, and I'm not sure I'm doing that properly.

Comment 4

[:Gavin Sharp [email: [e-mail]]]

(this is on top of my updated patch for bug 755136)

Comment 5

[Shane Caraveo (:mixedpuppy)]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #3)
> Created attachment 644157 [details] [diff] [review]
> modified patch

I see an empty patch.

Comment 6

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch

Here's the real patch...

Comment 7

[Shane Caraveo (:mixedpuppy)]

+
+    var containingBrowser = window.QueryInterface(Ci.nsIInterfaceRequestor)
+                                  .getInterface(Ci.nsIWebNavigation)
+                                  .QueryInterface(Ci.nsIDocShell)
+                                  .chromeEventHandler;
+
+    // If the containing browser isn't one of the social browsers, nothing to
+    // do here.
+    // XXX this is app-specific, might want some mechanism for consumers to
+    // whitelist other IDs/windowtypes  
+    if (!(containingBrowser.id == "social-sidebar-browser" ||
+          containingBrowser.id == "social-notification-browser") ||
+        containingBrowser.ownerDocument.documentElement.getAttribute("windowtype") != "navigator:browser") {
+      return;
+    }
+
+    let origin = containingBrowser.getAttribute("origin");
+    if (!origin) {
+      return;
+    }
+

preventing windowtype == navigator:browser will break the service/chat window (later patch) which uses the navigator:browser window via openDialog.  Having the origin attribute takes care of ensuring that we only attach onto social content.

Comment 8

[:Gavin Sharp [email: [e-mail]]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #7)
> preventing windowtype == navigator:browser will break the service/chat
> window (later patch) which uses the navigator:browser window via openDialog.
> Having the origin attribute takes care of ensuring that we only attach onto
> social content.

OK, I'll remove that part. I wanted a more robust check than just checking the id/origin, but that's probably good enough.

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 644195 [details] [diff] [review]
patch

Review of attachment 644195 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good. Hopefully this sort of thing will get much easier with bug 760109.

::: toolkit/components/social/MozSocialAPI.jsm
@@ +89,5 @@
> +          port: "r"
> +        }
> +      };
> +    },
> +    __exposedProps__: {

mozSocialObj is never exposed directly to content, so it doesn't need __exposedProps__.

Comment 10

[:Gavin Sharp [email: [e-mail]]]

Attachment: updated patch

I cleaned a few of the __exposedProps__ mistakes (addressing at least part of bug 773162), and simplified the code a little bit per discussion with mrbkap.

Comment 11

[Mark Hammond [:markh] [:mhammond]]

Attachment: version of the patch that doesn't report leaks

Here is a new version of the same patch that doesn't report leaks while testing.  The only changes are the first 2 chunks (2 places where createAboutBlankContentViewer is called) and the removal of the code that skips the test on debug builds.  Not obsoleting the old patch to avoid stepping on toes.

Comment 12

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 644844 [details] [diff] [review]
version of the patch that doesn't report leaks

I'll split this off as a patch against bug 775779

Comment 13

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 644434 [details] [diff] [review]
updated patch

I'd be inclined to deprecate getWorker immediately (ie, add a cu.reportError when getWorker is called and add a .port property now), but given that can be done later and that we are in somewhat of a hurry, r+

Comment 14

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 644434 [details] [diff] [review]
updated patch

browser_frameworker test fails:

TEST-INFO | chrome://mochitests/content/browser/toolkit/components/social/test/browser/browser_frameworker.js | Console message: [JavaScript Error: "FrameWorker: Error handling client port control message: TypeError: this._clientWindow.wrappedJSObject is undefined

Comment 15

[Mark Hammond [:markh] [:mhammond]]

Attachment: New patch with fix to browser_frameworker test

browser_frameworker test failed with the old patch.  This version changes only that failing test.

Comment 16

[:Gavin Sharp [email: [e-mail]]]

Applied a different fix, as described on IRC (use XPCNativeWrapper.unwrap instead of wrappedJSObject), landed on inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/deaf2494a3a6

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/deaf2494a3a6