Closed Bug 773351 Opened 8 years ago Closed 8 years ago

hookup the mozSocial api

Categories

(Firefox Graveyard :: SocialAPI, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 17

People

(Reporter: mixedpuppy, Assigned: mixedpuppy)

References

Details

(Whiteboard: [Fx16])

Attachments

(1 file, 6 obsolete files)

we need to inject navigator.mozSocial on the social content panels/sidebar, this bug is to port over the code from the addon.
Blocks: 770695
Attached patch mozSocialAPI.patch (obsolete) — Splinter Review
mozSocial api implementation
Attachment #641650 - Flags: feedback?(gavin.sharp)
Attached patch mozSocialAPI.patch (obsolete) — Splinter Review
Attachment #641650 - Attachment is obsolete: true
Attachment #641650 - Flags: feedback?(gavin.sharp)
Attachment #642036 - Flags: feedback?(gavin.sharp)
Attached patch modified patch (obsolete) — Splinter Review
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.
Attachment #642036 - Attachment is obsolete: true
Attachment #642036 - Flags: feedback?(gavin.sharp)
Attachment #644157 - Flags: review?(mrbkap)
Attachment #644157 - Flags: review?(mixedpuppy)
(this is on top of my updated patch for bug 755136)
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #3)
> Created attachment 644157 [details] [diff] [review]
> modified patch

I see an empty patch.
Attached patch patch (obsolete) — Splinter Review
Here's the real patch...
Attachment #644157 - Attachment is obsolete: true
Attachment #644157 - Flags: review?(mrbkap)
Attachment #644157 - Flags: review?(mixedpuppy)
Attachment #644195 - Flags: review?(mrbkap)
Attachment #644195 - Flags: review?(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.
(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 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__.
Attachment #644195 - Flags: review?(mrbkap) → review+
Attached patch updated patch (obsolete) — Splinter Review
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.
Attachment #644195 - Attachment is obsolete: true
Attachment #644195 - Flags: review?(mixedpuppy)
Attachment #644434 - Flags: review?(mixedpuppy)
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 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 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+
Attachment #644434 - Flags: review?(mixedpuppy) → review+
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
Attachment #644434 - Flags: review+ → review-
browser_frameworker test failed with the old patch.  This version changes only that failing test.
Attachment #644434 - Attachment is obsolete: true
Attachment #644844 - Attachment is obsolete: true
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
Flags: in-testsuite+
OS: Mac OS X → All
Hardware: x86 → All
Target Milestone: --- → Firefox 17
https://hg.mozilla.org/mozilla-central/rev/deaf2494a3a6
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.