Last Comment Bug 773351 - hookup the mozSocial api
: hookup the mozSocial api
Status: RESOLVED FIXED
[Fx16]
:
Product: Firefox
Classification: Client Software
Component: SocialAPI (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Firefox 17
Assigned To: Shane Caraveo (:mixedpuppy)
:
: Shane Caraveo (:mixedpuppy)
Mentors:
Depends on: 755136
Blocks: 770695
  Show dependency treegraph
 
Reported: 2012-07-12 10:48 PDT by Shane Caraveo (:mixedpuppy)
Modified: 2013-12-27 14:27 PST (History)
4 users (show)
gavin.sharp: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
mozSocialAPI.patch (6.50 KB, patch)
2012-07-12 16:36 PDT, Shane Caraveo (:mixedpuppy)
no flags Details | Diff | Splinter Review
mozSocialAPI.patch (6.55 KB, patch)
2012-07-13 14:03 PDT, Shane Caraveo (:mixedpuppy)
no flags Details | Diff | Splinter Review
modified patch (166 bytes, patch)
2012-07-19 20:44 PDT, :Gavin Sharp [email: gavin@gavinsharp.com]
no flags Details | Diff | Splinter Review
patch (16.14 KB, patch)
2012-07-19 23:22 PDT, :Gavin Sharp [email: gavin@gavinsharp.com]
mrbkap: review+
Details | Diff | Splinter Review
updated patch (21.85 KB, patch)
2012-07-20 13:08 PDT, :Gavin Sharp [email: gavin@gavinsharp.com]
markh: review-
Details | Diff | Splinter Review
version of the patch that doesn't report leaks (18.68 KB, patch)
2012-07-22 23:11 PDT, Mark Hammond [:markh]
no flags Details | Diff | Splinter Review
New patch with fix to browser_frameworker test (18.93 KB, patch)
2012-07-23 18:29 PDT, Mark Hammond [:markh]
no flags Details | Diff | Splinter Review

Description Shane Caraveo (:mixedpuppy) 2012-07-12 10:48:23 PDT
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) 2012-07-12 16:36:04 PDT
Created attachment 641650 [details] [diff] [review]
mozSocialAPI.patch

mozSocial api implementation
Comment 2 Shane Caraveo (:mixedpuppy) 2012-07-13 14:03:57 PDT
Created attachment 642036 [details] [diff] [review]
mozSocialAPI.patch
Comment 3 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-07-19 20:44:53 PDT
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.
Comment 4 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-07-19 20:46:51 PDT
(this is on top of my updated patch for bug 755136)
Comment 5 Shane Caraveo (:mixedpuppy) 2012-07-19 21:32:30 PDT
(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.
Comment 6 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-07-19 23:22:45 PDT
Created attachment 644195 [details] [diff] [review]
patch

Here's the real patch...
Comment 7 Shane Caraveo (:mixedpuppy) 2012-07-19 23:44:07 PDT
+
+    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: gavin@gavinsharp.com] 2012-07-20 10:17:36 PDT
(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) 2012-07-20 11:51:42 PDT
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: gavin@gavinsharp.com] 2012-07-20 13:08:10 PDT
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.
Comment 11 Mark Hammond [:markh] 2012-07-22 23:11:12 PDT
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 12 Mark Hammond [:markh] 2012-07-23 16:41:05 PDT
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] 2012-07-23 16:42:41 PDT
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] 2012-07-23 18:16:51 PDT
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] 2012-07-23 18:29:11 PDT
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.
Comment 16 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-07-23 22:26:17 PDT
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] 2012-07-25 08:14:24 PDT
https://hg.mozilla.org/mozilla-central/rev/deaf2494a3a6

Note You need to log in before you can comment on or make changes to this bug.