hookup the mozSocial api

RESOLVED FIXED in Firefox 17

Status

()

Firefox
SocialAPI
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: mixedpuppy, Assigned: mixedpuppy)

Tracking

unspecified
Firefox 17
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Fx16])

Attachments

(1 attachment, 6 obsolete attachments)

(Assignee)

Description

5 years ago
we need to inject navigator.mozSocial on the social content panels/sidebar, this bug is to port over the code from the addon.
(Assignee)

Updated

5 years ago
Blocks: 770695
(Assignee)

Comment 1

5 years ago
Created attachment 641650 [details] [diff] [review]
mozSocialAPI.patch

mozSocial api implementation
Attachment #641650 - Flags: feedback?(gavin.sharp)
(Assignee)

Comment 2

5 years ago
Created attachment 642036 [details] [diff] [review]
mozSocialAPI.patch
Attachment #641650 - Attachment is obsolete: true
Attachment #641650 - Flags: feedback?(gavin.sharp)
Attachment #642036 - Flags: feedback?(gavin.sharp)
Created attachment 644157 [details] [diff] [review]
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.
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)
(Assignee)

Comment 5

5 years ago
(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.
Created attachment 644195 [details] [diff] [review]
patch

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)
(Assignee)

Comment 7

5 years ago
+
+    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+
Created attachment 644434 [details] [diff] [review]
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.
Attachment #644195 - Attachment is obsolete: true
Attachment #644195 - Flags: review?(mixedpuppy)
Attachment #644434 - Flags: review?(mixedpuppy)
Created attachment 644844 [details] [diff] [review]
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 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-
Created attachment 645162 [details] [diff] [review]
New patch with fix to browser_frameworker test

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
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.